From ef5479af71c47d55e3aa63fa60debd26f7bed9c7 Mon Sep 17 00:00:00 2001 From: Mathieu Fenniak Date: Fri, 24 Apr 2026 18:19:58 +0200 Subject: [PATCH] refactor: split "basic" and "oauth2" authentication impl into smaller single-purpose components (#12236) Forgejo's `basic` and `oauth2` authentication methods perform five distinct types of authentication: - Username and password authentication - Personal access tokens - OAuth2 access tokens - Forgejo Action's `${{ forgejo.token }}` -- task-based static tokens - Forgejo Action's `${{ env.ACTIONS_RUNTIME_TOKEN }}` JWT, which is the authentication method used for `upload-artifact` (mirroring GitHub's implementation) `basic` and `oauth2` both supported almost all of these methods, resulting in quite a bit of code duplication between them. This PR splits personal access tokens into `access_token.go`, Action's task-based tokens into `action_task_token.go`, and Action's JWT tokens into `action_runtime_token.go`. **Note:** There is one peculiar side-effect that is worth discussing. Previously, `Authorization: Basic ...` was handled by one complex code path in basic.go, and `Authorization: Bearer ...` was handled by another in oauth2.go, and if authorization failed and a 401 was returned, a single error message would be returned to the user. Now, as multiple authorization methods may look at `Authorization: Basic ...` and provide their own reason why authorization didn't work, a 401 response has multiple reasons for a lack of authorization listed: ``` 401 Unauthorized ... failure to authenticate with oauth2 access token: not a JWT Basic authorization is not allowed while having security keys enrolled access token does not exist [sha: notpassword] task with token "notpassword": resource does not exist ``` A couple tests have been adapted to check that the result contains their expected response, rather than is equal-to or prefixed-with their expected result. This is caused by the "auth group" joining together any "invalid credentials" errors, and, to a certain extent it is useful to understand why the authorization request failed. But it's a bit obscure as well. ## Checklist The [contributor guide](https://forgejo.org/docs/next/contributor/) contains information that will be helpful to first time contributors. All work and communication must conform to Forgejo's [AI Agreement](https://codeberg.org/forgejo/governance/src/branch/main/AIAgreement.md). There also are a few [conditions for merging Pull Requests in Forgejo repositories](https://codeberg.org/forgejo/governance/src/branch/main/PullRequestsAgreement.md). You are also welcome to join the [Forgejo development chatroom](https://matrix.to/#/#forgejo-development:matrix.org). ### Tests for Go changes - I added test coverage for Go changes... - [ ] in their respective `*_test.go` for unit tests. - [ ] in the `tests/integration` directory if it involves interactions with a live Forgejo server. - Relying on integration testing for regression checks. - I ran... - [x] `make pr-go` before pushing ### Documentation - [ ] I created a pull request [to the documentation](https://codeberg.org/forgejo/docs) to explain to Forgejo users how to use this change. - [x] I did not document these changes and I do not expect someone else to do it. ### Release notes - [ ] This change will be noticed by a Forgejo user or admin (feature, bug fix, performance, etc.). I suggest to include a release note for this change. - [x] This change is not visible to a Forgejo user or admin (refactor, dependency upgrade, etc.). I think there is no need to add a release note for this change. Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/12236 Reviewed-by: Andreas Ahlenstorf --- routers/api/packages/api.go | 11 +- routers/api/shared/middleware.go | 3 + routers/web/auth/oauth.go | 15 +- routers/web/web.go | 6 + services/auth/interface.go | 10 +- services/auth/method/access_token.go | 78 ++++++ services/auth/method/action_runtime_token.go | 67 +++++ .../auth/method/action_runtime_token_test.go | 74 ++++++ services/auth/method/action_task_token.go | 67 +++++ services/auth/method/auth_result_oauth.go | 9 +- services/auth/method/basic.go | 71 +----- services/auth/method/oauth2.go | 228 ++++++------------ services/auth/method/oauth2_test.go | 117 --------- services/auth/method/util.go | 71 ++++++ services/auth/method/util_test.go | 43 ++++ tests/integration/api_twofa_test.go | 12 +- 16 files changed, 514 insertions(+), 368 deletions(-) create mode 100644 services/auth/method/access_token.go create mode 100644 services/auth/method/action_runtime_token.go create mode 100644 services/auth/method/action_runtime_token_test.go create mode 100644 services/auth/method/action_task_token.go delete mode 100644 services/auth/method/oauth2_test.go create mode 100644 services/auth/method/util.go create mode 100644 services/auth/method/util_test.go diff --git a/routers/api/packages/api.go b/routers/api/packages/api.go index 7096cd9e51..4a35948eca 100644 --- a/routers/api/packages/api.go +++ b/routers/api/packages/api.go @@ -193,6 +193,9 @@ func CommonRoutes() *web.Route { verifyAuth(r, []auth.Method{ &auth_method.OAuth2{}, &auth_method.Basic{}, + &auth_method.AccessToken{}, + &auth_method.ActionRuntimeToken{}, + &auth_method.ActionTaskToken{}, &nuget.Auth{}, &conan.Auth{}, &chef.Auth{}, @@ -836,7 +839,13 @@ func ContainerRoutes() *web.Route { r.Use(context.PackageContexter()) - verifyContainerAuth(r, []auth.Method{&auth_method.Basic{}, &container.Auth{}}) + verifyContainerAuth(r, []auth.Method{ + &auth_method.Basic{}, + &auth_method.AccessToken{}, + &auth_method.ActionRuntimeToken{}, + &auth_method.ActionTaskToken{}, + &container.Auth{}, + }) r.Get("", container.ReqContainerAccess, container.DetermineSupport) r.Group("/token", func() { diff --git a/routers/api/shared/middleware.go b/routers/api/shared/middleware.go index 5a2debf3af..9dd1605783 100644 --- a/routers/api/shared/middleware.go +++ b/routers/api/shared/middleware.go @@ -50,6 +50,9 @@ func buildAuthGroup() *auth_method.Group { &auth_method.OAuth2{}, &auth_method.HTTPSign{}, &auth_method.Basic{}, // FIXME: this should be removed once we don't allow basic auth in API + &auth_method.AccessToken{}, + &auth_method.ActionRuntimeToken{}, + &auth_method.ActionTaskToken{}, ) if setting.Service.EnableReverseProxyAuthAPI { group.Add(&auth_method.ReverseProxy{}) diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index dae71dd377..6335f2ecc0 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -33,7 +33,6 @@ import ( "forgejo.org/modules/util" "forgejo.org/modules/web" "forgejo.org/modules/web/middleware" - auth_method "forgejo.org/services/auth/method" source_service "forgejo.org/services/auth/source" "forgejo.org/services/auth/source/oauth2" "forgejo.org/services/context" @@ -294,7 +293,9 @@ func ifOnlyPublicGroups(scopes string) bool { // InfoOAuth manages request for userinfo endpoint func InfoOAuth(ctx *context.Context) { - if ctx.Doer == nil || !ctx.Authentication.IsOAuth2JWTAuthentication() { + hasGrantScopes, grantScopes := ctx.Authentication.OAuth2GrantScopes().Get() + + if ctx.Doer == nil || !hasGrantScopes { ctx.Resp.Header().Set("WWW-Authenticate", `Bearer realm=""`) ctx.PlainText(http.StatusUnauthorized, "no valid authorization") return @@ -308,17 +309,7 @@ func InfoOAuth(ctx *context.Context) { Picture: ctx.Doer.AvatarLink(ctx), } - var token string - if auHead := ctx.Req.Header.Get("Authorization"); auHead != "" { - auths := strings.Fields(auHead) - if len(auths) == 2 && (auths[0] == "token" || strings.ToLower(auths[0]) == "bearer") { - token = auths[1] - } - } - - _, grantScopes := auth_method.CheckOAuthAccessToken(ctx, token) onlyPublicGroups := ifOnlyPublicGroups(grantScopes) - groups, err := getOAuthGroupsForUser(ctx, ctx.Doer, onlyPublicGroups) if err != nil { ctx.ServerError("Oauth groups for user", err) diff --git a/routers/web/web.go b/routers/web/web.go index 9d8ac332a8..5839e7d30e 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -111,6 +111,12 @@ func buildAuthGroup() *auth_method.Group { group.Add(&auth_method.OAuth2{}) // FIXME: this should be removed and only applied in download and oauth related routers group.Add(&auth_method.Basic{}) // FIXME: this should be removed and only applied in download and git/lfs routers + // FIXME: extracted from OAuth2 & Basic -- these methods have internal URL filters that should be moved into + // middlewares (if we can figure out the right way to do that), similar to the notes on OAuth2 & Basic above. + group.Add(&auth_method.AccessToken{}) + group.Add(&auth_method.ActionRuntimeToken{}) + group.Add(&auth_method.ActionTaskToken{}) + if setting.Service.EnableReverseProxyAuth { group.Add(&auth_method.ReverseProxy{}) // reverseproxy should before Session, otherwise the header will be ignored if user has login } diff --git a/services/auth/interface.go b/services/auth/interface.go index bbfe641520..a5ed33a79b 100644 --- a/services/auth/interface.go +++ b/services/auth/interface.go @@ -97,9 +97,9 @@ type AuthenticationResult interface { // Identifies if the authentication was performed by a reverse proxy. IsReverseProxyAuthentication() bool - // Identifies specifically that the OAuth2 JWT authentication method was used. If so, some related OAuth2 API - // endpoints may be accessible that otherwise wouldn't be. - IsOAuth2JWTAuthentication() bool + // Defined scopes of the [OAuth2Grant] as a comma-separated string, if authenticated via an OAuth access token JWT. + // Otherwise, None. + OAuth2GrantScopes() optional.Option[string] // If authenticated as an Actions task (using ${{ forgejo.token }}), then indicates the specific task that performed // the authentication. @@ -108,8 +108,8 @@ type AuthenticationResult interface { type BaseAuthenticationResult struct{} -func (*BaseAuthenticationResult) IsOAuth2JWTAuthentication() bool { - return false +func (*BaseAuthenticationResult) OAuth2GrantScopes() optional.Option[string] { + return optional.None[string]() } func (*BaseAuthenticationResult) IsPasswordAuthentication() bool { diff --git a/services/auth/method/access_token.go b/services/auth/method/access_token.go new file mode 100644 index 0000000000..f1a5e8e766 --- /dev/null +++ b/services/auth/method/access_token.go @@ -0,0 +1,78 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package method + +import ( + "fmt" + "net/http" + + auth_model "forgejo.org/models/auth" + user_model "forgejo.org/models/user" + "forgejo.org/modules/log" + "forgejo.org/modules/optional" + "forgejo.org/modules/web/middleware" + "forgejo.org/services/auth" + "forgejo.org/services/authz" +) + +var _ auth.Method = &AccessToken{} + +type AccessToken struct{} + +func (a *AccessToken) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { + // Authentication previously was performed in a single routine for `Authorization: Basic ...` and `Authorization: + // Bearer ...`, and both routines had separate URL exclusion lists onto which they wouldn't apply. That behaviour + // is maintained by cloning those conditions here and deciding whether to look at basic/bearer auth, or not. In the + // future this should be removed and migrated to route-specific middleware. + legacySkipBasic := !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) + legacySkipFormAndBearer := !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && !isGitRawOrAttachPath(req) && !isArchivePath(req) + + maybeAuthToken := a.getTokenFromRequest(req, legacySkipBasic, legacySkipFormAndBearer) + if !maybeAuthToken.Has() { + return &auth.AuthenticationNotAttempted{} + } + _, authToken := maybeAuthToken.Get() + + // check personal access token + token, err := auth_model.GetAccessTokenBySHA(req.Context(), authToken) + if auth_model.IsErrAccessTokenNotExist(err) || auth_model.IsErrAccessTokenEmpty(err) { + return &auth.AuthenticationAttemptedIncorrectCredential{Error: err} + } else if err != nil { + return &auth.AuthenticationError{Error: fmt.Errorf("access token GetAccessTokenBySHA: %w", err)} + } + + log.Trace("AccessToken: Valid AccessToken for user[%d]", token.UID) + u, err := user_model.GetUserByID(req.Context(), token.UID) + if err != nil { + return &auth.AuthenticationError{Error: fmt.Errorf("access token GetUserByID: %w", err)} + } + + if err = token.UpdateLastUsed(req.Context()); err != nil { + log.Error("UpdateLastUsed: %v", err) + } + + reducer, err := authz.GetAuthorizationReducerForAccessToken(req.Context(), token) + if err != nil { + return &auth.AuthenticationError{Error: fmt.Errorf("access token GetAuthorizationReducerForAccessToken: %w", err)} + } + + return &auth.AuthenticationSuccess{Result: &accessTokenAuthenticationResult{user: u, scope: token.Scope, reducer: reducer}} +} + +func (a *AccessToken) getTokenFromRequest(req *http.Request, skipBasic, skipFormAndBearer bool) optional.Option[string] { + if !skipFormAndBearer { + if has, token := tokenFromForm(req).Get(); has { + return optional.Some(token) + } + if has, token := tokenFromAuthorizationBearer(req).Get(); has { + return optional.Some(token) + } + } + if !skipBasic { + if has, token := tokenFromAuthorizationBasic(req).Get(); has { + return optional.Some(token) + } + } + return optional.None[string]() +} diff --git a/services/auth/method/action_runtime_token.go b/services/auth/method/action_runtime_token.go new file mode 100644 index 0000000000..2d15476424 --- /dev/null +++ b/services/auth/method/action_runtime_token.go @@ -0,0 +1,67 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package method + +import ( + "context" + "errors" + "net/http" + + actions_model "forgejo.org/models/actions" + user_model "forgejo.org/models/user" + "forgejo.org/modules/optional" + "forgejo.org/modules/web/middleware" + "forgejo.org/services/actions" + "forgejo.org/services/auth" +) + +var _ auth.Method = &ActionRuntimeToken{} + +type ActionRuntimeToken struct{} + +func (a *ActionRuntimeToken) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { + // In the future this should be removed and migrated to route-specific middleware: + if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && !isGitRawOrAttachPath(req) && !isArchivePath(req) { + return &auth.AuthenticationNotAttempted{} + } + + maybeAuthToken := a.getTokenFromRequest(req) + if !maybeAuthToken.Has() { + return &auth.AuthenticationNotAttempted{} + } + _, authToken := maybeAuthToken.Get() + + taskID, err := actions.TokenToTaskID(authToken) + if err != nil { + return &auth.AuthenticationAttemptedIncorrectCredential{Error: err} + } + + if !checkTaskIsRunning(req.Context(), taskID) { + return &auth.AuthenticationAttemptedIncorrectCredential{Error: errors.New("failure to authenticate with action runtime token: task is no longer running")} + } + + return &auth.AuthenticationSuccess{Result: &actionsTaskTokenAuthenticationResult{user: user_model.NewActionsUser(), taskID: taskID}} +} + +func (a *ActionRuntimeToken) getTokenFromRequest(req *http.Request) optional.Option[string] { + if has, token := tokenFromForm(req).Get(); has { + return optional.Some(token) + } + if has, token := tokenFromAuthorizationBearer(req).Get(); has { + return optional.Some(token) + } + return optional.None[string]() +} + +// CheckTaskIsRunning verifies that the TaskID corresponds to a running task +func checkTaskIsRunning(ctx context.Context, taskID int64) bool { + // Verify the task exists + task, err := actions_model.GetTaskByID(ctx, taskID) + if err != nil { + return false + } + + // Verify that it's running + return task.Status == actions_model.StatusRunning +} diff --git a/services/auth/method/action_runtime_token_test.go b/services/auth/method/action_runtime_token_test.go new file mode 100644 index 0000000000..f5bae45e96 --- /dev/null +++ b/services/auth/method/action_runtime_token_test.go @@ -0,0 +1,74 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package method + +import ( + "fmt" + "net/http" + "net/url" + "testing" + + actions_model "forgejo.org/models/actions" + "forgejo.org/models/unittest" + user_model "forgejo.org/models/user" + "forgejo.org/services/actions" + auth_service "forgejo.org/services/auth" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestActionRuntimeTokenVerify(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + t.Run("Actions JWT", func(t *testing.T) { + const RunningTaskID = 47 + task := &actions_model.ActionTask{ + ID: RunningTaskID, + Job: &actions_model.ActionRunJob{ + ID: 2, + RunID: 1, + }, + } + token, err := actions.CreateAuthorizationToken(task, map[string]any{}, false) + require.NoError(t, err) + + req := http.Request{ + URL: &url.URL{Path: "/api/v1/"}, + Header: map[string][]string{ + "Authorization": {fmt.Sprintf("Bearer %s", token)}, + }, + } + + o := ActionRuntimeToken{} + output := o.Verify(&req, nil, nil) + ar, authSuccess := output.(*auth_service.AuthenticationSuccess) + require.True(t, authSuccess, "expected type AuthenticationSuccess, but was: %#v", output) + authResult := ar.Result + assert.Equal(t, int64(user_model.ActionsUserID), authResult.User().ID) + isActionsToken, authTaskID := authResult.ActionsTaskID().Get() + assert.True(t, isActionsToken) + assert.Equal(t, int64(RunningTaskID), authTaskID) + }) +} + +func TestCheckTaskIsRunning(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + cases := map[string]struct { + TaskID int64 + Expected bool + }{ + "Running": {TaskID: 47, Expected: true}, + "Missing": {TaskID: 1, Expected: false}, + "Cancelled": {TaskID: 46, Expected: false}, + } + + for name := range cases { + c := cases[name] + t.Run(name, func(t *testing.T) { + actual := checkTaskIsRunning(t.Context(), c.TaskID) + assert.Equal(t, c.Expected, actual) + }) + } +} diff --git a/services/auth/method/action_task_token.go b/services/auth/method/action_task_token.go new file mode 100644 index 0000000000..b5e1ab4f59 --- /dev/null +++ b/services/auth/method/action_task_token.go @@ -0,0 +1,67 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package method + +import ( + "errors" + "fmt" + "net/http" + + actions_model "forgejo.org/models/actions" + user_model "forgejo.org/models/user" + "forgejo.org/modules/log" + "forgejo.org/modules/optional" + "forgejo.org/modules/util" + "forgejo.org/modules/web/middleware" + "forgejo.org/services/auth" +) + +var _ auth.Method = &ActionTaskToken{} + +type ActionTaskToken struct{} + +func (a *ActionTaskToken) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { + // Authentication previously was performed in a single routine for `Authorization: Basic ...` and `Authorization: + // Bearer ...`, and both routines had separate URL exclusion lists onto which they wouldn't apply. That behaviour + // is maintained by cloning those conditions here and deciding whether to look at basic/bearer auth, or not. In the + // future this should be removed and migrated to route-specific middleware. + legacySkipBasic := !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) + legacySkipFormAndBearer := !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && !isGitRawOrAttachPath(req) && !isArchivePath(req) + + maybeAuthToken := a.getTokenFromRequest(req, legacySkipBasic, legacySkipFormAndBearer) + if !maybeAuthToken.Has() { + return &auth.AuthenticationNotAttempted{} + } + _, authToken := maybeAuthToken.Get() + + // check task token + task, err := actions_model.GetRunningTaskByToken(req.Context(), authToken) + if err != nil && errors.Is(err, util.ErrNotExist) { + return &auth.AuthenticationAttemptedIncorrectCredential{Error: err} + } else if err != nil { + return &auth.AuthenticationError{Error: fmt.Errorf("action task token GetRunningTaskByToken: %w", err)} + } else if task == nil { + return &auth.AuthenticationError{Error: errors.New("failed to retrieve non-nil task")} + } + + log.Trace("Basic Authorization: Valid AccessToken for task[%d]", task.ID) + return &auth.AuthenticationSuccess{Result: &actionsTaskTokenAuthenticationResult{user: user_model.NewActionsUser(), taskID: task.ID}} +} + +func (a *ActionTaskToken) getTokenFromRequest(req *http.Request, skipBasic, skipFormAndBearer bool) optional.Option[string] { + if !skipFormAndBearer { + if has, token := tokenFromForm(req).Get(); has { + return optional.Some(token) + } + if has, token := tokenFromAuthorizationBearer(req).Get(); has { + return optional.Some(token) + } + } + if !skipBasic { + if has, token := tokenFromAuthorizationBasic(req).Get(); has { + return optional.Some(token) + } + } + return optional.None[string]() +} diff --git a/services/auth/method/auth_result_oauth.go b/services/auth/method/auth_result_oauth.go index ce451e6459..392e616837 100644 --- a/services/auth/method/auth_result_oauth.go +++ b/services/auth/method/auth_result_oauth.go @@ -14,8 +14,9 @@ var _ auth.AuthenticationResult = &oAuth2JWTAuthenticationResult{} type oAuth2JWTAuthenticationResult struct { *auth.BaseAuthenticationResult - user *user_model.User - scope optional.Option[auth_model.AccessTokenScope] + user *user_model.User + scope optional.Option[auth_model.AccessTokenScope] + grantScopes string } func (*oAuth2JWTAuthenticationResult) IsOAuth2JWTAuthentication() bool { @@ -29,3 +30,7 @@ func (r *oAuth2JWTAuthenticationResult) User() *user_model.User { func (r *oAuth2JWTAuthenticationResult) Scope() optional.Option[auth_model.AccessTokenScope] { return r.scope } + +func (r *oAuth2JWTAuthenticationResult) OAuth2GrantScopes() optional.Option[string] { + return optional.Some(r.grantScopes) +} diff --git a/services/auth/method/basic.go b/services/auth/method/basic.go index 905deb0652..8385bea2fd 100644 --- a/services/auth/method/basic.go +++ b/services/auth/method/basic.go @@ -10,18 +10,15 @@ import ( "net/http" "strings" - actions_model "forgejo.org/models/actions" auth_model "forgejo.org/models/auth" user_model "forgejo.org/models/user" "forgejo.org/modules/base" "forgejo.org/modules/log" - "forgejo.org/modules/optional" "forgejo.org/modules/setting" "forgejo.org/modules/util" "forgejo.org/modules/web/middleware" "forgejo.org/services/auth" "forgejo.org/services/auth/source/db" - "forgejo.org/services/authz" ) // Ensure the struct implements the interface. @@ -34,10 +31,8 @@ var ( // header. type Basic struct{} -// Verify extracts and validates Basic data (username and password/token) from the -// "Authorization" header of the request and returns the corresponding user object for that -// name/token on successful validation. -// Returns nil if header is empty or validation fails. +// Verify extracts and validates Basic data (username and password/token) from the "Authorization" header of the request +// and returns the corresponding user object for that name/token on successful validation. func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { // Basic authentication should only fire on API, Download or on Git or LFSPaths if !middleware.IsAPIPath(req) && !isContainerPath(req) && !isAttachmentDownload(req) && !isGitRawOrAttachOrLFSPath(req) { @@ -56,67 +51,6 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionS uname, passwd, _ := base.BasicAuthDecode(auths[1]) - // Check if username or password is a token - isUsernameToken := len(passwd) == 0 || passwd == "x-oauth-basic" - // Assume username is token - authToken := uname - if !isUsernameToken { - log.Trace("Basic Authorization: Attempting login for: %s", uname) - // Assume password is token - authToken = passwd - } else { - log.Trace("Basic Authorization: Attempting login with username as token") - } - - // check oauth2 token - uid, grantScopes := CheckOAuthAccessToken(req.Context(), authToken) - if uid != 0 { - log.Trace("Basic Authorization: Valid OAuthAccessToken for user[%d]", uid) - - u, err := user_model.GetUserByID(req.Context(), uid) - if err != nil { - return &auth.AuthenticationError{Error: fmt.Errorf("basic auth GetUserByID: %w", err)} - } - - var scope auth_model.AccessTokenScope - if grantScopes != "" { - scope = auth_model.AccessTokenScope(grantScopes) - } else { - scope = auth_model.AccessTokenScopeAll // fallback to all - } - return &auth.AuthenticationSuccess{Result: &oAuth2JWTAuthenticationResult{user: u, scope: optional.Some(scope)}} - } - - // check personal access token - token, err := auth_model.GetAccessTokenBySHA(req.Context(), authToken) - if err == nil { - log.Trace("Basic Authorization: Valid AccessToken for user[%d]", token.UID) - u, err := user_model.GetUserByID(req.Context(), token.UID) - if err != nil { - return &auth.AuthenticationError{Error: fmt.Errorf("basic auth GetUserByID for access token: %w", err)} - } - - if err = token.UpdateLastUsed(req.Context()); err != nil { - log.Error("UpdateLastUsed: %v", err) - } - - reducer, err := authz.GetAuthorizationReducerForAccessToken(req.Context(), token) - if err != nil { - return &auth.AuthenticationError{Error: fmt.Errorf("basic auth GetAuthorizationReducerForAccessToken: %w", err)} - } - - return &auth.AuthenticationSuccess{Result: &accessTokenAuthenticationResult{user: u, scope: token.Scope, reducer: reducer}} - } else if !auth_model.IsErrAccessTokenNotExist(err) && !auth_model.IsErrAccessTokenEmpty(err) { - log.Error("GetAccessTokenBySha: %v", err) - } - - // check task token - task, err := actions_model.GetRunningTaskByToken(req.Context(), authToken) - if err == nil && task != nil { - log.Trace("Basic Authorization: Valid AccessToken for task[%d]", task.ID) - return &auth.AuthenticationSuccess{Result: &actionsTaskTokenAuthenticationResult{user: user_model.NewActionsUser(), taskID: task.ID}} - } - if !setting.Service.EnableBasicAuth { return &auth.AuthenticationAttemptedIncorrectCredential{Error: errors.New("basic authentication by username & password is disabled")} } @@ -147,7 +81,6 @@ func (b *Basic) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionS } log.Trace("Basic Authorization: Logged in user %-v", u) - return &auth.AuthenticationSuccess{Result: &basicPaswordAuthenticationResult{user: u}} } diff --git a/services/auth/method/oauth2.go b/services/auth/method/oauth2.go index 8e07a2ecca..9eeef6a6c1 100644 --- a/services/auth/method/oauth2.go +++ b/services/auth/method/oauth2.go @@ -5,25 +5,21 @@ package method import ( - "context" + "errors" "fmt" "net/http" "slices" "strings" "time" - actions_model "forgejo.org/models/actions" auth_model "forgejo.org/models/auth" user_model "forgejo.org/models/user" "forgejo.org/modules/log" "forgejo.org/modules/optional" "forgejo.org/modules/setting" - "forgejo.org/modules/util" "forgejo.org/modules/web/middleware" - "forgejo.org/services/actions" "forgejo.org/services/auth" "forgejo.org/services/auth/source/oauth2" - "forgejo.org/services/authz" ) // Ensure the struct implements the interface. @@ -70,174 +66,86 @@ func grantAdditionalScopes(grantScopes string) string { return "" } -// CheckOAuthAccessToken returns uid of user from oauth token -// + non default openid scopes requested -func CheckOAuthAccessToken(ctx context.Context, accessToken string) (int64, string) { - if !setting.OAuth2.Enabled { - return 0, "" - } - // JWT tokens require a "." - if !strings.Contains(accessToken, ".") { - return 0, "" - } - token, err := oauth2.ParseToken(accessToken, oauth2.DefaultSigningKey) - if err != nil { - log.Trace("oauth2.ParseToken: %v", err) - return 0, "" - } - var grant *auth_model.OAuth2Grant - if grant, err = auth_model.GetOAuth2GrantByID(ctx, token.GrantID); err != nil || grant == nil { - return 0, "" - } - if token.Type != oauth2.TypeAccessToken { - return 0, "" - } - if token.ExpiresAt.Before(time.Now()) || token.IssuedAt.After(time.Now()) { - return 0, "" - } - grantScopes := grantAdditionalScopes(grant.Scope) - return grant.UserID, grantScopes -} - -// CheckTaskIsRunning verifies that the TaskID corresponds to a running task -func CheckTaskIsRunning(ctx context.Context, taskID int64) bool { - // Verify the task exists - task, err := actions_model.GetTaskByID(ctx, taskID) - if err != nil { - return false - } - - // Verify that it's running - return task.Status == actions_model.StatusRunning -} - -// OAuth2 implements the Auth interface and authenticates requests -// (API requests only) by looking for an OAuth token in query parameters or the -// "Authorization" header. +// OAuth2 implements the Auth interface and authenticates requests (API requests only) by looking for an OAuth token in +// query parameters or the "Authorization" header. type OAuth2 struct{} -// parseToken returns the token from request, and a boolean value -// representing whether the token exists or not -func parseToken(req *http.Request) (string, bool) { - _ = req.ParseForm() - if !setting.DisableQueryAuthToken { - // Check token. - if token := req.Form.Get("token"); token != "" { - return token, true - } - // Check access token. - if token := req.Form.Get("access_token"); token != "" { - return token, true - } - } else if req.Form.Get("token") != "" || req.Form.Get("access_token") != "" { - log.Warn("API token sent in query string but DISABLE_QUERY_AUTH_TOKEN=true") - } - - // check header token - if auHead := req.Header.Get("Authorization"); auHead != "" { - auths := strings.Fields(auHead) - if len(auths) == 2 && (util.ASCIIEqualFold(auths[0], "token") || util.ASCIIEqualFold(auths[0], "bearer")) { - return auths[1], true - } - } - return "", false -} - -// userIDFromToken returns the user id corresponding to the OAuth token. -// It will set 'IsApiToken' to true if the token is an API token and -// set 'ApiTokenScope' to the scope of the access token -func (o *OAuth2) userIDFromToken(ctx context.Context, tokenSHA string) auth.MethodOutput { - if tokenSHA == "" { - return &auth.AuthenticationAttemptedIncorrectCredential{Error: auth_model.ErrAccessTokenEmpty{}} - } - // Let's see if token is valid. - if strings.Contains(tokenSHA, ".") { - // First attempt to decode an actions JWT, returning the actions user - if taskID, err := actions.TokenToTaskID(tokenSHA); err == nil { - if CheckTaskIsRunning(ctx, taskID) { - return &auth.AuthenticationSuccess{Result: &actionsTaskTokenAuthenticationResult{user: user_model.NewActionsUser(), taskID: taskID}} - } - } - - // Otherwise, check if this is an OAuth access token - uid, grantScopes := CheckOAuthAccessToken(ctx, tokenSHA) - var accessTokenScope optional.Option[auth_model.AccessTokenScope] - if uid != 0 { - if grantScopes != "" { - accessTokenScope = optional.Some(auth_model.AccessTokenScope(grantScopes)) - } else { - accessTokenScope = optional.Some(auth_model.AccessTokenScopeAll) // fallback to all - } - } - user, err := user_model.GetPossibleUserByID(ctx, uid) - if err != nil { - if user_model.IsErrUserNotExist(err) { - return &auth.AuthenticationAttemptedIncorrectCredential{Error: err} - } - return &auth.AuthenticationError{Error: fmt.Errorf("oauth2 GetPossibleUserByID: %w", err)} - } - return &auth.AuthenticationSuccess{Result: &oAuth2JWTAuthenticationResult{user: user, scope: accessTokenScope}} - } - - t, err := auth_model.GetAccessTokenBySHA(ctx, tokenSHA) - if err != nil { - if auth_model.IsErrAccessTokenNotExist(err) { - // check task token - task, err := actions_model.GetRunningTaskByToken(ctx, tokenSHA) - if err == nil && task != nil { - log.Trace("Basic Authorization: Valid AccessToken for task[%d]", task.ID) - return &auth.AuthenticationSuccess{Result: &actionsTaskTokenAuthenticationResult{user: user_model.NewActionsUser(), taskID: task.ID}} - } - } else if !auth_model.IsErrAccessTokenNotExist(err) && !auth_model.IsErrAccessTokenEmpty(err) { - return &auth.AuthenticationError{Error: fmt.Errorf("oauth2 GetAccessTokenBySHA: %w", err)} - } - return &auth.AuthenticationAttemptedIncorrectCredential{Error: err} - } - if err := t.UpdateLastUsed(ctx); err != nil { - log.Error("UpdateLastUsed: %v", err) - } - if t.UID == 0 { - return &auth.AuthenticationAttemptedIncorrectCredential{Error: err} - } - - reducer, err := authz.GetAuthorizationReducerForAccessToken(ctx, t) - if err != nil { - return &auth.AuthenticationError{Error: fmt.Errorf("oauth2 GetAuthorizationReducerForAccessToken: %w", err)} - } - - u, err := user_model.GetUserByID(ctx, t.UID) - if err != nil { - return &auth.AuthenticationError{Error: fmt.Errorf("oauth2 GetUserByID: %w", err)} - } - - return &auth.AuthenticationSuccess{Result: &accessTokenAuthenticationResult{user: u, scope: t.Scope, reducer: reducer}} -} - -// Verify extracts the user ID from the OAuth token in the query parameters -// or the "Authorization" header and returns the corresponding user object for that ID. -// If verification is successful returns an existing user object. -// Returns nil if verification fails. func (o *OAuth2) Verify(req *http.Request, w http.ResponseWriter, _ auth.SessionStore) auth.MethodOutput { + if !setting.OAuth2.Enabled { + return &auth.AuthenticationNotAttempted{} + } // These paths are not API paths, but we still want to check for tokens because they maybe in the API returned URLs if !middleware.IsAPIPath(req) && !isAttachmentDownload(req) && !isAuthenticatedTokenRequest(req) && !isGitRawOrAttachPath(req) && !isArchivePath(req) { return &auth.AuthenticationNotAttempted{} } - token, ok := parseToken(req) - if !ok { + maybeAuthToken := o.getTokenFromRequest(req) + if !maybeAuthToken.Has() { return &auth.AuthenticationNotAttempted{} } + _, authToken := maybeAuthToken.Get() - return o.userIDFromToken(req.Context(), token) + token, err := oauth2.ParseToken(authToken, oauth2.DefaultSigningKey) + if err != nil { + log.Trace("oauth2.ParseToken: %v", err) + return &auth.AuthenticationAttemptedIncorrectCredential{Error: err} + } + + var grant *auth_model.OAuth2Grant + if grant, err = auth_model.GetOAuth2GrantByID(req.Context(), token.GrantID); err != nil { + return &auth.AuthenticationError{Error: fmt.Errorf("oauth2 GetOAuth2GrantByID: %w", err)} + } else if grant == nil { + return &auth.AuthenticationAttemptedIncorrectCredential{Error: errors.New("oauth2 grant not found or revoked")} + } + if token.Type != oauth2.TypeAccessToken { + return &auth.AuthenticationAttemptedIncorrectCredential{Error: errors.New("token was not an oauth2 access token")} + } + if token.ExpiresAt.Before(time.Now()) || token.IssuedAt.After(time.Now()) { + return &auth.AuthenticationAttemptedIncorrectCredential{Error: errors.New("token was expired")} + } + if grant.UserID == 0 { + return &auth.AuthenticationError{Error: errors.New("oauth2 invalid grant user id")} + } + + var accessTokenScope optional.Option[auth_model.AccessTokenScope] + grantScopes := grantAdditionalScopes(grant.Scope) + if grantScopes != "" { + accessTokenScope = optional.Some(auth_model.AccessTokenScope(grantScopes)) + } else { + accessTokenScope = optional.Some(auth_model.AccessTokenScopeAll) // fallback to all + } + + user, err := user_model.GetPossibleUserByID(req.Context(), grant.UserID) + if err != nil { + if !user_model.IsErrUserNotExist(err) { + return &auth.AuthenticationError{Error: fmt.Errorf("oauth2 GetPossibleUserByID: %w", err)} + } + return &auth.AuthenticationAttemptedIncorrectCredential{Error: errors.New("oauth2 grant owner does not exist")} + } + + return &auth.AuthenticationSuccess{ + Result: &oAuth2JWTAuthenticationResult{ + user: user, + scope: accessTokenScope, + grantScopes: grantScopes, + }, + } +} + +func (*OAuth2) getTokenFromRequest(req *http.Request) optional.Option[string] { + if has, token := tokenFromForm(req).Get(); has { + return optional.Some(token) + } + if has, token := tokenFromAuthorizationBasic(req).Get(); has { + return optional.Some(token) + } + if has, token := tokenFromAuthorizationBearer(req).Get(); has { + return optional.Some(token) + } + return optional.None[string]() } func isAuthenticatedTokenRequest(req *http.Request) bool { - switch req.URL.Path { - case "/login/oauth/userinfo": - fallthrough - case "/login/oauth/introspect": - return true - } - return false + return req.URL.Path == "/login/oauth/userinfo" } diff --git a/services/auth/method/oauth2_test.go b/services/auth/method/oauth2_test.go deleted file mode 100644 index 5ad103fe58..0000000000 --- a/services/auth/method/oauth2_test.go +++ /dev/null @@ -1,117 +0,0 @@ -// Copyright 2024 The Gitea Authors. All rights reserved. -// SPDX-License-Identifier: MIT - -package method - -import ( - "net/http" - "testing" - - actions_model "forgejo.org/models/actions" - "forgejo.org/models/auth" - "forgejo.org/models/unittest" - user_model "forgejo.org/models/user" - "forgejo.org/services/actions" - auth_service "forgejo.org/services/auth" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestUserIDFromToken(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) - - t.Run("Actions JWT", func(t *testing.T) { - const RunningTaskID = 47 - task := &actions_model.ActionTask{ - ID: RunningTaskID, - Job: &actions_model.ActionRunJob{ - ID: 2, - RunID: 1, - }, - } - token, err := actions.CreateAuthorizationToken(task, map[string]any{}, false) - require.NoError(t, err) - - o := OAuth2{} - output := o.userIDFromToken(t.Context(), token) - ar, authSuccess := output.(*auth_service.AuthenticationSuccess) - require.True(t, authSuccess, "expected type AuthenticationSuccess, but was: %#v", output) - authResult := ar.Result - assert.Equal(t, int64(user_model.ActionsUserID), authResult.User().ID) - isActionsToken, authTaskID := authResult.ActionsTaskID().Get() - assert.True(t, isActionsToken) - assert.Equal(t, int64(RunningTaskID), authTaskID) - }) - - t.Run("Actions error-JWT", func(t *testing.T) { - cases := map[string]struct { - Token string - Error error - }{ - "Empty": {"", auth.ErrAccessTokenEmpty{}}, - "To short": {"abc", auth.ErrAccessTokenNotExist{Token: "abc"}}, - } - - o := OAuth2{} - for name, c := range cases { - t.Run(name, func(t *testing.T) { - output := o.userIDFromToken(t.Context(), c.Token) - - ar, authFailure := output.(*auth_service.AuthenticationAttemptedIncorrectCredential) - require.True(t, authFailure, "expected type AuthenticationAttemptedIncorrectCredential, but was: %#v", output) - err := ar.Error - - require.ErrorIs(t, err, c.Error) - }) - } - }) -} - -func TestCheckTaskIsRunning(t *testing.T) { - require.NoError(t, unittest.PrepareTestDatabase()) - cases := map[string]struct { - TaskID int64 - Expected bool - }{ - "Running": {TaskID: 47, Expected: true}, - "Missing": {TaskID: 1, Expected: false}, - "Cancelled": {TaskID: 46, Expected: false}, - } - - for name := range cases { - c := cases[name] - t.Run(name, func(t *testing.T) { - actual := CheckTaskIsRunning(t.Context(), c.TaskID) - assert.Equal(t, c.Expected, actual) - }) - } -} - -func TestParseToken(t *testing.T) { - cases := map[string]struct { - Header string - ExpectedToken string - Expected bool - }{ - "Token Uppercase": {Header: "Token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Token Lowercase": {Header: "token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Token Unicode": {Header: "to\u212Aen 1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, - "Bearer Uppercase": {Header: "Bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Bearer Lowercase": {Header: "bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, - "Missing type": {Header: "1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, - "Three Parts": {Header: "abc 1234567890 test", ExpectedToken: "", Expected: false}, - "Token Three Parts": {Header: "Token 1234567890 test", ExpectedToken: "", Expected: false}, - } - - for name := range cases { - c := cases[name] - t.Run(name, func(t *testing.T) { - req, _ := http.NewRequest("GET", "/", nil) - req.Header.Add("Authorization", c.Header) - ActualToken, ActualSuccess := parseToken(req) - assert.Equal(t, c.ExpectedToken, ActualToken) - assert.Equal(t, c.Expected, ActualSuccess) - }) - } -} diff --git a/services/auth/method/util.go b/services/auth/method/util.go new file mode 100644 index 0000000000..8f3d2d355d --- /dev/null +++ b/services/auth/method/util.go @@ -0,0 +1,71 @@ +// Copyright 2026 The Forgejo Authors. All rights reserved. +// SPDX-License-Identifier: GPL-3.0-or-later + +package method + +import ( + "net/http" + "strings" + + "forgejo.org/modules/base" + "forgejo.org/modules/log" + "forgejo.org/modules/optional" + "forgejo.org/modules/setting" + "forgejo.org/modules/util" +) + +func tokenFromForm(req *http.Request) optional.Option[string] { + _ = req.ParseForm() + if !setting.DisableQueryAuthToken { + // Check token. + if token := req.Form.Get("token"); token != "" { + return optional.Some(token) + } + // Check access token. + if token := req.Form.Get("access_token"); token != "" { + return optional.Some(token) + } + } else if req.Form.Get("token") != "" || req.Form.Get("access_token") != "" { + log.Warn("API token sent in query string but DISABLE_QUERY_AUTH_TOKEN=true") + } + return optional.None[string]() +} + +func tokenFromAuthorizationBearer(req *http.Request) optional.Option[string] { + authorization := req.Header.Get("Authorization") + if len(authorization) != 0 { + auths := strings.Fields(authorization) + if len(auths) == 2 && (util.ASCIIEqualFold(auths[0], "token") || util.ASCIIEqualFold(auths[0], "bearer")) { + return optional.Some(auths[1]) + } + } + return optional.None[string]() +} + +func tokenFromAuthorizationBasic(req *http.Request) optional.Option[string] { + authorization := req.Header.Get("Authorization") + if len(authorization) != 0 { + auths := strings.SplitN(authorization, " ", 2) + if len(auths) == 2 && strings.ToLower(auths[0]) == "basic" { + uname, passwd, err := base.BasicAuthDecode(auths[1]) + if err != nil { + // Client provided a `Authorization: Basic ...`, but then [...] either couldn't be base64 decoded, or + // didn't contain a ":" for username/password separation. If we return `None`, it'll indicate to the + // caller that `Authorization: Basic [...]` wasn't present and skip authentication, so intead we'll + // return Some with an empty token to trigger a 401 error. + log.Debug("unexpected error in BasicAuthDecode(%q): %s", auths[1], err) + return optional.Some("") + } + + // Usually we'll use the password as the access token (or oauth token), but if the password is empty or + // `x-oauth-basic`, we'll use the username as a token. This behaviour is inherited from GitHub's OAuth Git + // over HTTPS behaviour. + if len(passwd) == 0 || passwd == "x-oauth-basic" { + return optional.Some(uname) + } + + return optional.Some(passwd) + } + } + return optional.None[string]() +} diff --git a/services/auth/method/util_test.go b/services/auth/method/util_test.go new file mode 100644 index 0000000000..9183701e4f --- /dev/null +++ b/services/auth/method/util_test.go @@ -0,0 +1,43 @@ +// Copyright 2024 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package method + +import ( + "net/http" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestTokenFromAuthorizationBearer(t *testing.T) { + cases := map[string]struct { + Header string + ExpectedToken string + Expected bool + }{ + "Token Uppercase": {Header: "Token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Token Lowercase": {Header: "token 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Token Unicode": {Header: "to\u212Aen 1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, + "Bearer Uppercase": {Header: "Bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Bearer Lowercase": {Header: "bearer 1234567890123456789012345687901325467890", ExpectedToken: "1234567890123456789012345687901325467890", Expected: true}, + "Missing type": {Header: "1234567890123456789012345687901325467890", ExpectedToken: "", Expected: false}, + "Three Parts": {Header: "abc 1234567890 test", ExpectedToken: "", Expected: false}, + "Token Three Parts": {Header: "Token 1234567890 test", ExpectedToken: "", Expected: false}, + } + + for name := range cases { + c := cases[name] + t.Run(name, func(t *testing.T) { + req, _ := http.NewRequest("GET", "/", nil) + req.Header.Add("Authorization", c.Header) + maybeToken := tokenFromAuthorizationBearer(req) + if hasToken, token := maybeToken.Get(); hasToken { + assert.True(t, c.Expected) + assert.Equal(t, c.ExpectedToken, token) + } else { + assert.False(t, c.Expected) + } + }) + } +} diff --git a/tests/integration/api_twofa_test.go b/tests/integration/api_twofa_test.go index ab173fc9f9..867dfe1e56 100644 --- a/tests/integration/api_twofa_test.go +++ b/tests/integration/api_twofa_test.go @@ -6,6 +6,7 @@ package integration import ( "fmt" "net/http" + "slices" "strings" "testing" "time" @@ -82,7 +83,7 @@ func TestAPIWebAuthn(t *testing.T) { DecodeJSON(t, resp, &userParsed) - assert.Equal(t, "Basic authorization is not allowed while having security keys enrolled", userParsed.Message) + assert.Contains(t, userParsed.Message, "Basic authorization is not allowed while having security keys enrolled\n") } func TestAPIWithRequiredTwoFactor(t *testing.T) { @@ -144,7 +145,14 @@ func TestAPIWithRequiredTwoFactor(t *testing.T) { var response userResponse DecodeJSON(t, resp, &response) - assert.True(t, strings.HasPrefix(response.Message, messagePrefix)) + assert.True(t, + slices.ContainsFunc( + strings.Split(response.Message, "\n"), + func(msg string) bool { + return strings.HasPrefix(msg, messagePrefix) + }, + ), + "expected prefix %q, but response message was %q", messagePrefix, response.Message) } }