Remove modules/context from SSPI

[KN4CK3R]

Related #27028

Extracted from another PR I'm working on.

This PR removes the dependency of modules/context from SSPI.
SSPI does not work nicely with our current auth design because it needs a specific workflow (request-response challenge). The old code renders a (bugged version) of the login page and needs modules/context to do that.

The new code renders the login page now for every failed auth method and shows a generic error to hide possible secrets in the message. This happens only if the error occured on the login page. For all other routes the old behaviour is active. SSPI needs a 401 Unauthorized response code. This can't be set in the auth handler because ctx.Written blocks the processing of other handlers. Therefore the login page now renders with the 401 status code. For a normal web browser user this is not noticeable.

[wxiaoguang]

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

[KN4CK3R]

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)

[wxiaoguang]

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)

[KN4CK3R]

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.

[wxiaoguang]

How do you mock a vendor package?

-> Make SSPI auth mockable #27036

[KN4CK3R]

I think the original code is also broken:

The SSPI auth is only active on /user/login, attachment download and api routes.

if !s.shouldAuthenticate(req) {
	return nil, nil
}

The first request always results in an expected error because no Authorization header is present.

userInfo, outToken, err := sspiAuth.Authenticate(req, w)
if err != nil {

Now the WWW-Authenticate header and the 401 response are written.

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)

gitea_context.GetWebContext(req) tries to get the web Context from the request which is nil for api requests. Therefore this call fails then.

[wxiaoguang]

Actually it does work (for web users): SSPI auth error after update to 1.20.0 #25952

But for API ... no idea. I guess few people really need it, and I do not have enough understanding nor have the test env for it.

[KN4CK3R]

What do you think about a new interface ChallengeResponseMethod? This could be used to separate the existing Methods from this logically different type. The other Methods take a look at the request and decide if they can derive a user from that. A ChallengeResponseMethod is different because it actively challenges authentification by crafting a response. Using different interfaces with different method signatures could allow for cleaner code here.

sample code:

func Auth(ctx *context.Base, method Method, challengeRespondMethod ChallengeRespondMethod) (*user_model.User, error) {
	user, err := method.Verify(ctx.Req)
	if err != nil {
		return nil, err
	}
	if user != nil {
		return user, nil
	}

	result, err := challengeRespondMethod.Verify(ctx.Req, ctx.Resp)
	if err != nil {
		return nil, err
	}
	if result.User != nil {
		return result.User, nil
	}

	// only in web context
	if result.RedirectToLogin {
		ctx.Redirect(...)
	}

	return nil, nil
}

[wxiaoguang]

What do you think about a new interface ChallengeResponseMethod?

Instead introducing more interfaces, could the Verify function take the work? eg:

user, responder, err := method.Verify(ctx.Req)
if err != nil {
    return ..., err
}
if responder != nil {
    responder.WriteTo(ctx.Resp)
}

ps: what do you think about Make SSPI auth mockable #27036 ?