Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove modules/context from SSPI #27027

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+23 −35
Open

Remove modules/context from SSPI #27027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3f342e7
Remove `modules/context` from sspi.
KN4CK3R Sep 11, 2023
003c72c
fix
KN4CK3R Sep 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions modules/web/middleware/request.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,7 @@ import (
func IsAPIPath(req *http.Request) bool {
return strings.HasPrefix(req.URL.Path, "/api/")
}

func IsLoginPath(req *http.Request) bool {
return strings.HasPrefix(req.URL.Path, "/user/login")
}
2 changes: 1 addition & 1 deletion options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,7 @@ authorize_application_description = If you grant the access, it will be able to
authorize_title = Authorize "%s" to access your account?
authorization_failed = Authorization failed
authorization_failed_desc = The authorization failed because we detected an invalid request. Please contact the maintainer of the app you have tried to authorize.
sspi_auth_failed = SSPI authentication failed
authorization.error = Authorization failed. If this error persists, please contact the site administrator.
password_pwned = The password you chose is on a <a target="_blank" rel="noopener noreferrer" href="https://haveibeenpwned.com/Passwords">list of stolen passwords</a> previously exposed in public data breaches. Please try again with a different password and consider changing this password elsewhere too.
password_pwned_err = Could not complete request to HaveIBeenPwned

Expand Down
4 changes: 2 additions & 2 deletions routers/web/auth/auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,7 @@ func SignIn(ctx *context.Context) {
context.SetCaptchaData(ctx)
}

ctx.HTML(http.StatusOK, tplSignIn)
ctx.HTML(http.StatusUnauthorized, tplSignIn)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any website use 401 for a sign-in page?

Copy link
Member Author

@KN4CK3R KN4CK3R Sep 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know. It's not forbidden by the web standards. Do you have another idea how to handle this?

-       ctx.HTML(http.StatusUnauthorized, tplSignIn)
+       status := http.StatusOK
+       if ctx.Flash.ErrorMsg != "" {
+               status = http.StatusUnauthorized
+       }
+       ctx.HTML(status, tplSignIn)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't looked into the problem carefully.

Maybe something could be tried:

  1. Use some test to show how SSPI works (mock the vendor package to make the test runnable in Gitea's CI)
  2. Base on 1, Separate the SSPI response code logic from middleware.go . Or, separate the SSPI login logic into a new handler: /user/login/sspi, then it could do anything it needs.

(Just some initial thoughts)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use some test to show how SSPI works (mock the vendor package to make the test runnable in Gitea's CI)

How do you mock a vendor package?

Thought about 2 too, but I didn't want to duplicate other logic.

The SSPI workflow is (don't have a setup to test it):

  1. Client sends request
  2. Server responds with WWW-Authenticate Negotiate
  3. Client responds with Authorization xxx
  4. Server responds with requested ressource (or again 2. if there are multiple "layers" of authorization needed)

The requests and responds should not be visible to the user, they don't have to see content. The original code just renders the login page if an error occurs which stops the automatic flow. Then they see the normal login page and can try again/login with another method.

If we agree we don't need the login page bonus, we could just do something like:

func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore, sess SessionStore) (*user_model.User, error) {
	...
	userInfo, outToken, err := sspiAuth.Authenticate(req, w)
	if err != nil {
		sspiAuth.AppendAuthenticateHeader(w, outToken)
		w.WriteHeader(http.StatusUnauthorized)
		w.Write([]byte("SSPI auth failed. Visit /user/login to login."))
		return nil, err
	}

But that still is bad design for such a method.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you mock a vendor package?

-> Make SSPI auth mockable #27036

}

// SignInPost response for sign in request
Expand All @@ -184,7 +184,7 @@ func SignInPost(ctx *context.Context) {
ctx.Data["EnableSSPI"] = auth.IsSSPIEnabled()

if ctx.HasError() {
ctx.HTML(http.StatusOK, tplSignIn)
ctx.HTML(http.StatusUnauthorized, tplSignIn)
return
}

Expand Down
8 changes: 7 additions & 1 deletion services/auth/middleware.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,13 @@ func Auth(authMethod Method) func(*context.Context) {
ar, err := authShared(ctx.Base, ctx.Session, authMethod)
if err != nil {
log.Error("Failed to verify user: %v", err)
ctx.Error(http.StatusUnauthorized, "Verify")

// If the error occurs on the login page, fallthrough and render the login form again with a generic error message.
if middleware.IsLoginPath(ctx.Req) {
ctx.Flash.Error(ctx.Locale.Tr("auth.authorization.error"), true)
} else {
ctx.Error(http.StatusUnauthorized, "Verify")
}
return
}
ctx.Doer = ar.Doer
Expand Down
40 changes: 9 additions & 31 deletions services/auth/sspi_windows.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,14 @@ package auth
import (
"errors"
"net/http"
"strconv"
"strings"
"sync"

"code.gitea.io/gitea/models/auth"
"code.gitea.io/gitea/models/avatars"
user_model "code.gitea.io/gitea/models/user"
"code.gitea.io/gitea/modules/base"
gitea_context "code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"
"code.gitea.io/gitea/modules/web/middleware"
"code.gitea.io/gitea/services/auth/source/sspi"
Expand All @@ -24,10 +22,6 @@ import (
"github.com/quasoft/websspi"
)

const (
tplSignIn base.TplName = "user/auth/signin"
)

var (
// sspiAuth is a global instance of the websspi authentication package,
// which is used to avoid acquiring the server credential handle on
Expand Down Expand Up @@ -77,20 +71,9 @@ func (s *SSPI) Verify(req *http.Request, w http.ResponseWriter, store DataStore,
log.Trace("SSPI Authorization: Attempting to authenticate")
userInfo, outToken, err := sspiAuth.Authenticate(req, w)
if err != nil {
log.Warn("Authentication failed with error: %v\n", err)
sspiAuth.AppendAuthenticateHeader(w, outToken)

// Include the user login page in the 401 response to allow the user
// to login with another authentication method if SSPI authentication
// fails
store.GetData()["Flash"] = map[string]string{
"ErrorMsg": err.Error(),
}
store.GetData()["EnableOpenIDSignIn"] = setting.Service.EnableOpenIDSignIn
store.GetData()["EnableSSPI"] = true
// in this case, the Verify function is called in Gitea's web context
// FIXME: it doesn't look good to render the page here, why not redirect?
gitea_context.GetWebContext(req).HTML(http.StatusUnauthorized, tplSignIn)
// The SSPI workflow requires a 401 StatusUnauthorized response code
// which gets set by the auth routes.
return nil, err
}
if outToken != "" {
Expand Down Expand Up @@ -145,18 +128,14 @@ func (s *SSPI) getConfig() (*sspi.Source, error) {
}

func (s *SSPI) shouldAuthenticate(req *http.Request) (shouldAuth bool) {
shouldAuth = false
path := strings.TrimSuffix(req.URL.Path, "/")
if path == "/user/login" {
if middleware.IsLoginPath(req) {
if req.FormValue("user_name") != "" && req.FormValue("password") != "" {
shouldAuth = false
} else if req.FormValue("auth_with_sspi") == "1" {
shouldAuth = true
return false
}
} else if middleware.IsAPIPath(req) || isAttachmentDownload(req) {
shouldAuth = true
b, _ := strconv.ParseBool(req.FormValue("auth_with_sspi"))
return b
}
return shouldAuth
return middleware.IsAPIPath(req) || isAttachmentDownload(req)
}

// newUser creates a new user object for the purpose of automatic registration
Expand All @@ -171,11 +150,10 @@ func (s *SSPI) newUser(username string, cfg *sspi.Source) (*user_model.User, err
UseCustomAvatar: true,
Avatar: avatars.DefaultAvatarLink(),
}
emailNotificationPreference := user_model.EmailNotificationsDisabled
overwriteDefault := &user_model.CreateUserOverwriteOptions{
IsActive: util.OptionalBoolOf(cfg.AutoActivateUsers),
KeepEmailPrivate: util.OptionalBoolTrue,
EmailNotificationsPreference: &emailNotificationPreference,
EmailNotificationsPreference: util.ToPointer(user_model.EmailNotificationsDisabled),
}
if err := user_model.CreateUser(user, overwriteDefault); err != nil {
return nil, err
Expand Down
Loading