From 33da8f1e7149329b562e52d0d7bb9a5129021dcc Mon Sep 17 00:00:00 2001 From: Marcell Mars Date: Thu, 10 Oct 2024 17:05:19 +0200 Subject: [PATCH] no setting requirement for additional grant scopes - the logic introduced with this PR will be applied by default, even though it introduces breaking changes if anyone relied on the previous behavior regarding personal access tokens or full access for OAuth2 third parties. --- modules/setting/oauth2.go | 34 +++++++++---------- routers/web/user/setting/applications.go | 1 - services/oauth2_provider/access_token.go | 12 +++---- .../oauth2_provider/additional_scopes_test.go | 1 - tests/integration/oauth_test.go | 8 ++--- 5 files changed, 24 insertions(+), 32 deletions(-) diff --git a/modules/setting/oauth2.go b/modules/setting/oauth2.go index f6b802f223..0d3e63e0b4 100644 --- a/modules/setting/oauth2.go +++ b/modules/setting/oauth2.go @@ -90,25 +90,23 @@ func parseScopes(sec ConfigSection, name string) []string { } var OAuth2 = struct { - Enabled bool - AccessTokenExpirationTime int64 - RefreshTokenExpirationTime int64 - InvalidateRefreshTokens bool - JWTSigningAlgorithm string `ini:"JWT_SIGNING_ALGORITHM"` - JWTSigningPrivateKeyFile string `ini:"JWT_SIGNING_PRIVATE_KEY_FILE"` - MaxTokenLength int - DefaultApplications []string - EnableAdditionalGrantScopes bool + Enabled bool + AccessTokenExpirationTime int64 + RefreshTokenExpirationTime int64 + InvalidateRefreshTokens bool + JWTSigningAlgorithm string `ini:"JWT_SIGNING_ALGORITHM"` + JWTSigningPrivateKeyFile string `ini:"JWT_SIGNING_PRIVATE_KEY_FILE"` + MaxTokenLength int + DefaultApplications []string }{ - Enabled: true, - AccessTokenExpirationTime: 3600, - RefreshTokenExpirationTime: 730, - InvalidateRefreshTokens: false, - JWTSigningAlgorithm: "RS256", - JWTSigningPrivateKeyFile: "jwt/private.pem", - MaxTokenLength: math.MaxInt16, - DefaultApplications: []string{"git-credential-oauth", "git-credential-manager", "tea"}, - EnableAdditionalGrantScopes: false, + Enabled: true, + AccessTokenExpirationTime: 3600, + RefreshTokenExpirationTime: 730, + InvalidateRefreshTokens: false, + JWTSigningAlgorithm: "RS256", + JWTSigningPrivateKeyFile: "jwt/private.pem", + MaxTokenLength: math.MaxInt16, + DefaultApplications: []string{"git-credential-oauth", "git-credential-manager", "tea"}, } func loadOAuth2From(rootCfg ConfigProvider) { diff --git a/routers/web/user/setting/applications.go b/routers/web/user/setting/applications.go index 6c2c182911..356c2ea5de 100644 --- a/routers/web/user/setting/applications.go +++ b/routers/web/user/setting/applications.go @@ -113,6 +113,5 @@ func loadApplicationsData(ctx *context.Context) { ctx.ServerError("GetOAuth2GrantsByUserID", err) return } - ctx.Data["EnableAdditionalGrantScopes"] = setting.OAuth2.EnableAdditionalGrantScopes } } diff --git a/services/oauth2_provider/access_token.go b/services/oauth2_provider/access_token.go index 6af08fe1e7..c8ac7f14d7 100644 --- a/services/oauth2_provider/access_token.go +++ b/services/oauth2_provider/access_token.go @@ -228,14 +228,10 @@ func GetOAuthGroupsForUser(ctx context.Context, user *user_model.User, onlyPubli var groups []string for _, org := range orgs { - // process additional scopes only if enabled in settings - // this could be removed once additional scopes get accepted - if setting.OAuth2.EnableAdditionalGrantScopes { - if onlyPublicGroups { - if public, err := org_model.IsPublicMembership(ctx, org.ID, user.ID); err == nil { - if !public || !org.Visibility.IsPublic() { - continue - } + if onlyPublicGroups { + if public, err := org_model.IsPublicMembership(ctx, org.ID, user.ID); err == nil { + if !public || !org.Visibility.IsPublic() { + continue } } } diff --git a/services/oauth2_provider/additional_scopes_test.go b/services/oauth2_provider/additional_scopes_test.go index cca5a3a4b7..d239229f4b 100644 --- a/services/oauth2_provider/additional_scopes_test.go +++ b/services/oauth2_provider/additional_scopes_test.go @@ -10,7 +10,6 @@ import ( ) func TestGrantAdditionalScopes(t *testing.T) { - setting.OAuth2.EnableAdditionalGrantScopes = true tests := []struct { grantScopes string expectedScopes string diff --git a/tests/integration/oauth_test.go b/tests/integration/oauth_test.go index b8fba7e12f..a36e014612 100644 --- a/tests/integration/oauth_test.go +++ b/tests/integration/oauth_test.go @@ -515,7 +515,7 @@ func TestOAuth_GrantScopesReadUserFailRepos(t *testing.T) { err := db.Insert(db.DefaultContext, grant) require.NoError(t, err) - assert.Contains(t, grant.Scope, "openid profile email read:user") + assert.ElementsMatch(t, []string{"openid", "profile", "email", "read:user"}, strings.Split(grant.Scope, " ")) ctx := loginUserWithPasswordRemember(t, user.Name, "password", true) @@ -596,7 +596,7 @@ func TestOAuth_GrantScopesReadRepositoryFailOrganization(t *testing.T) { err := db.Insert(db.DefaultContext, grant) require.NoError(t, err) - assert.Contains(t, grant.Scope, "openid profile email read:user read:repository") + assert.ElementsMatch(t, []string{"openid", "profile", "email", "read:user", "read:repository"}, strings.Split(grant.Scope, " ")) ctx := loginUserWithPasswordRemember(t, user.Name, "password", true) @@ -790,7 +790,7 @@ func TestOAuth_GrantScopesClaimGroupsAll(t *testing.T) { } } -func TestOAuth_GrantScopesEnabledClaimGroups(t *testing.T) { +func TestOAuth_GrantScopesClaimGroupsPublicOnly(t *testing.T) { defer tests.PrepareTestEnv(t)() user := unittest.AssertExistsAndLoadBean(t, &user_model.User{Name: "user2"}) @@ -819,7 +819,7 @@ func TestOAuth_GrantScopesEnabledClaimGroups(t *testing.T) { err := db.Insert(db.DefaultContext, grant) require.NoError(t, err) - assert.Contains(t, grant.Scope, "openid profile email groups") + assert.ElementsMatch(t, []string{"openid", "profile", "email", "groups"}, strings.Split(grant.Scope, " ")) ctx := loginUserWithPasswordRemember(t, user.Name, "password", true)