From 9ed768adc426636b6fbcdb389ba89ba039bc7da3 Mon Sep 17 00:00:00 2001
From: wxiaoguang <wxiaoguang@gmail.com>
Date: Tue, 26 Nov 2024 10:03:02 +0800
Subject: [PATCH] Improve oauth2 scope token handling (#32633)

---
 routers/api/v1/api.go                         |  2 +-
 services/oauth2_provider/access_token.go      | 22 ++++++++++++-------
 .../oauth2_provider/additional_scopes_test.go |  5 ++++-
 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) {