diff options
| -rw-r--r-- | routers/api/v1/api.go | 2 | ||||
| -rw-r--r-- | services/oauth2_provider/access_token.go | 22 | ||||
| -rw-r--r-- | services/oauth2_provider/additional_scopes_test.go | 5 | ||||
| -rw-r--r-- | tests/integration/oauth_test.go | 4 |
4 files changed, 21 insertions, 12 deletions
diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go index 0079e8dc87..aee76325a8 100644 --- a/routers/api/v1/api.go +++ b/routers/api/v1/api.go @@ -321,7 +321,7 @@ func tokenRequiresScopes(requiredScopeCategories ...auth_model.AccessTokenScopeC } if !allow { - ctx.Error(http.StatusForbidden, "tokenRequiresScope", fmt.Sprintf("token does not have at least one of required scope(s): %v", requiredScopes)) + ctx.Error(http.StatusForbidden, "tokenRequiresScope", fmt.Sprintf("token does not have at least one of required scope(s), required=%v, token scope=%v", requiredScopes, scope)) return } diff --git a/services/oauth2_provider/access_token.go b/services/oauth2_provider/access_token.go index d94e15d5f2..77ddce0534 100644 --- a/services/oauth2_provider/access_token.go +++ b/services/oauth2_provider/access_token.go @@ -74,26 +74,32 @@ type AccessTokenResponse struct { // GrantAdditionalScopes returns valid scopes coming from grant func GrantAdditionalScopes(grantScopes string) auth.AccessTokenScope { // scopes_supported from templates/user/auth/oidc_wellknown.tmpl - scopesSupported := []string{ + generalScopesSupported := []string{ "openid", "profile", "email", "groups", } - var tokenScopes []string - for _, tokenScope := range strings.Split(grantScopes, " ") { - if slices.Index(scopesSupported, tokenScope) == -1 { - tokenScopes = append(tokenScopes, tokenScope) + var accessScopes []string // the scopes for access control, but not for general information + for _, scope := range strings.Split(grantScopes, " ") { + if scope != "" && !slices.Contains(generalScopesSupported, scope) { + accessScopes = append(accessScopes, scope) } } // since version 1.22, access tokens grant full access to the API // with this access is reduced only if additional scopes are provided - accessTokenScope := auth.AccessTokenScope(strings.Join(tokenScopes, ",")) - if accessTokenWithAdditionalScopes, err := accessTokenScope.Normalize(); err == nil && len(tokenScopes) > 0 { - return accessTokenWithAdditionalScopes + if len(accessScopes) > 0 { + accessTokenScope := auth.AccessTokenScope(strings.Join(accessScopes, ",")) + if normalizedAccessTokenScope, err := accessTokenScope.Normalize(); err == nil { + return normalizedAccessTokenScope + } + // TODO: if there are invalid access scopes (err != nil), + // then it is treated as "all", maybe in the future we should make it stricter to return an error + // at the moment, to avoid breaking 1.22 behavior, invalid tokens are also treated as "all" } + // fallback, empty access scope is treated as "all" access return auth.AccessTokenScopeAll } diff --git a/services/oauth2_provider/additional_scopes_test.go b/services/oauth2_provider/additional_scopes_test.go index d239229f4b..2d4df7aea2 100644 --- a/services/oauth2_provider/additional_scopes_test.go +++ b/services/oauth2_provider/additional_scopes_test.go @@ -14,6 +14,7 @@ func TestGrantAdditionalScopes(t *testing.T) { grantScopes string expectedScopes string }{ + {"", "all"}, // for old tokens without scope, treat it as "all" {"openid profile email", "all"}, {"openid profile email groups", "all"}, {"openid profile email all", "all"}, @@ -22,12 +23,14 @@ func TestGrantAdditionalScopes(t *testing.T) { {"read:user read:repository", "read:repository,read:user"}, {"read:user write:issue public-only", "public-only,write:issue,read:user"}, {"openid profile email read:user", "read:user"}, + + // TODO: at the moment invalid tokens are treated as "all" to avoid breaking 1.22 behavior (more details are in GrantAdditionalScopes) {"read:invalid_scope", "all"}, {"read:invalid_scope,write:scope_invalid,just-plain-wrong", "all"}, } for _, test := range tests { - t.Run(test.grantScopes, func(t *testing.T) { + t.Run("scope:"+test.grantScopes, func(t *testing.T) { result := GrantAdditionalScopes(test.grantScopes) assert.Equal(t, test.expectedScopes, string(result)) }) diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index feb262b50e..f177bd3a23 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -565,7 +565,7 @@ func TestOAuth_GrantScopesReadUserFailRepos(t *testing.T) { errorParsed := new(errorResponse) require.NoError(t, json.Unmarshal(errorResp.Body.Bytes(), errorParsed)) - assert.Contains(t, errorParsed.Message, "token does not have at least one of required scope(s): [read:repository]") + assert.Contains(t, errorParsed.Message, "token does not have at least one of required scope(s), required=[read:repository]") } func TestOAuth_GrantScopesReadRepositoryFailOrganization(t *testing.T) { @@ -708,7 +708,7 @@ func TestOAuth_GrantScopesReadRepositoryFailOrganization(t *testing.T) { errorParsed := new(errorResponse) require.NoError(t, json.Unmarshal(errorResp.Body.Bytes(), errorParsed)) - assert.Contains(t, errorParsed.Message, "token does not have at least one of required scope(s): [read:user read:organization]") + assert.Contains(t, errorParsed.Message, "token does not have at least one of required scope(s), required=[read:user read:organization]") } func TestOAuth_GrantScopesClaimPublicOnlyGroups(t *testing.T) { |