diff --git a/modules/google/groups.go b/modules/google/groups.go index 58c3823357..b941d37cab 100644 --- a/modules/google/groups.go +++ b/modules/google/groups.go @@ -29,18 +29,20 @@ const maxGroupPages = 20 // Client calls Google Workspace APIs. type Client struct { - httpClient *http.Client - groupsEndpoint string - claimName string + httpClient *http.Client + groupsEndpoint string + claimName string + failLoginOnAdditionalInfoError bool } // NewClient creates a Client using the given authenticated HTTP client. // The client should be built from an OAuth2 token carrying IAMScope. -func NewClient(httpClient *http.Client, claimName string) *Client { +func NewClient(httpClient *http.Client, claimName string, failLoginOnAdditionalInfoError bool) *Client { return &Client{ - httpClient: httpClient, - groupsEndpoint: defaultIAMGroupsEndpoint, - claimName: claimName, + httpClient: httpClient, + groupsEndpoint: defaultIAMGroupsEndpoint, + claimName: claimName, + failLoginOnAdditionalInfoError: failLoginOnAdditionalInfoError, } } @@ -129,3 +131,8 @@ func (c *Client) FetchAdditionalInfo(ctx context.Context, user goth.User) (goth. user.RawData[c.claimName] = groups return user, nil } + +// FailLoginOnAdditionalInfoError implements oauth2.AdditionalInfoProvider. +func (c *Client) FailLoginOnAdditionalInfoError() bool { + return c.failLoginOnAdditionalInfoError +} diff --git a/modules/google/groups_test.go b/modules/google/groups_test.go index b53c53e032..d5bc83567d 100644 --- a/modules/google/groups_test.go +++ b/modules/google/groups_test.go @@ -11,13 +11,14 @@ import ( "strings" "testing" + "github.com/markbates/goth" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func newTestClient(t *testing.T, server *httptest.Server) *Client { t.Helper() - c := NewClient(server.Client(), "groups") + c := NewClient(server.Client(), "groups", false) c.groupsEndpoint = server.URL return c } @@ -124,3 +125,50 @@ func TestFetchGoogleGroups_InvalidJSON(t *testing.T) { require.Error(t, err) assert.Nil(t, groups) } + +func TestFetchAdditionalInfo_InjectsClaimBeforeValidation(t *testing.T) { + server := mockGroupsServer(t, "user@example.com", [][]string{ + {"required-group@example.com"}, + }) + defer server.Close() + + c := newTestClient(t, server) + c.claimName = "groups" + + user := goth.User{ + Email: "user@example.com", + RawData: map[string]any{}, + } + + enriched, err := c.FetchAdditionalInfo(context.Background(), user) + require.NoError(t, err) + + // Verify the claim is present and contains the group — simulating what + // RequiredClaimName validation would check after enrichment runs. + groups, ok := enriched.RawData["groups"] + require.True(t, ok, "groups claim must be present in RawData after enrichment") + groupSlice, ok := groups.([]string) + require.True(t, ok) + assert.Contains(t, groupSlice, "required-group@example.com") +} + +func TestFetchAdditionalInfo_ErrorDoesNotInjectClaim(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = fmt.Fprint(w, `{"error":"forbidden"}`) + })) + defer server.Close() + + c := newTestClient(t, server) + c.claimName = "groups" + + user := goth.User{ + Email: "user@example.com", + RawData: map[string]any{}, + } + + enriched, err := c.FetchAdditionalInfo(context.Background(), user) + require.Error(t, err) + _, hasGroups := enriched.RawData["groups"] + assert.False(t, hasGroups) +} diff --git a/routers/web/auth/oauth.go b/routers/web/auth/oauth.go index 83726bfd81..750b90c3fa 100644 --- a/routers/web/auth/oauth.go +++ b/routers/web/auth/oauth.go @@ -232,6 +232,10 @@ func claimValueToStringSet(claimValue any) container.Set[string] { } func syncGroupsToTeams(ctx *context.Context, source *oauth2.Source, gothUser *goth.User, u *user_model.User) error { + if !shouldSyncFromGroupClaim(source, gothUser) { + return nil + } + if source.GroupTeamMap != "" || source.GroupTeamMapRemoval { groupTeamMapping, err := auth_module.UnmarshalGroupTeamMapping(source.GroupTeamMap) if err != nil { @@ -257,7 +261,22 @@ func getClaimedGroups(source *oauth2.Source, gothUser *goth.User) container.Set[ return claimValueToStringSet(groupClaims) } +func shouldSyncFromGroupClaim(source *oauth2.Source, gothUser *goth.User) bool { + // Keep historical behavior for all providers except Google Workspace: + // if the claim is missing, group-derived sync still runs on an empty set. + if source.Provider != "gplus" { + return true + } + + _, hasGroupClaim := gothUser.RawData[source.GroupClaimName] + return hasGroupClaim +} + func getUserAdminAndRestrictedFromGroupClaims(source *oauth2.Source, gothUser *goth.User) (isAdmin optional.Option[user_service.UpdateOptionField[bool]], isRestricted optional.Option[bool]) { + if !shouldSyncFromGroupClaim(source, gothUser) { + return isAdmin, isRestricted + } + groups := getClaimedGroups(source, gothUser) if source.AdminGroup != "" { @@ -461,6 +480,26 @@ func oAuth2UserLoginCallback(ctx *context.Context, authSource *auth.Source, requ return nil, goth.User{}, err } + // Enrichment must run before RequiredClaimName validation so that claims + // injected by the provider (e.g. Google Workspace groups fetched via the + // Cloud Identity API) are available when the required-claim check executes. + // Moving this block after the RequiredClaimName check would cause users to + // be incorrectly rejected when RequiredClaimName references an injected claim. + if provider := oauth2.GetAdditionalInfoProvider(oauth2Source, &gothUser); provider != nil { + enriched, err := provider.FetchAdditionalInfo(ctx, gothUser) + if err != nil { + log.Warn("OAuth2: failed to fetch additional info for %s: %v", gothUser.Email, err) + if provider.FailLoginOnAdditionalInfoError() { + // Fail closed only when login directly depends on the group claim + // (for example RequiredClaimName == GroupClaimName). Other + // group-based sync features are fail-open and preserve prior state. + return nil, goth.User{}, user_model.ErrUserProhibitLogin{Name: gothUser.UserID} + } + } else { + gothUser = enriched + } + } + if oauth2Source.RequiredClaimName != "" { claimInterface, has := gothUser.RawData[oauth2Source.RequiredClaimName] if !has { @@ -476,15 +515,6 @@ func oAuth2UserLoginCallback(ctx *context.Context, authSource *auth.Source, requ } } - if provider := oauth2.GetAdditionalInfoProvider(oauth2Source, &gothUser); provider != nil { - enriched, err := provider.FetchAdditionalInfo(ctx, gothUser) - if err != nil { - log.Warn("OAuth2: failed to fetch additional info for %s: %v", gothUser.Email, err) - } else { - gothUser = enriched - } - } - user := &user_model.User{ LoginName: gothUser.UserID, LoginType: auth.OAuth2, diff --git a/routers/web/auth/oauth_group_claims_test.go b/routers/web/auth/oauth_group_claims_test.go new file mode 100644 index 0000000000..d3e5a4f329 --- /dev/null +++ b/routers/web/auth/oauth_group_claims_test.go @@ -0,0 +1,108 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package auth + +import ( + "testing" + + "code.gitea.io/gitea/modules/optional" + "code.gitea.io/gitea/services/auth/source/oauth2" + user_service "code.gitea.io/gitea/services/user" + + "github.com/markbates/goth" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestShouldSyncFromGroupClaim(t *testing.T) { + t.Run("google claim missing", func(t *testing.T) { + source := &oauth2.Source{ + Provider: "gplus", + GroupClaimName: "groups", + } + user := &goth.User{ + RawData: map[string]any{}, + } + assert.False(t, shouldSyncFromGroupClaim(source, user)) + }) + + t.Run("google claim present and empty", func(t *testing.T) { + source := &oauth2.Source{ + Provider: "gplus", + GroupClaimName: "groups", + } + user := &goth.User{ + RawData: map[string]any{ + "groups": []string{}, + }, + } + assert.True(t, shouldSyncFromGroupClaim(source, user)) + }) + + t.Run("non google provider keeps old behavior", func(t *testing.T) { + source := &oauth2.Source{ + Provider: "openidConnect", + GroupClaimName: "groups", + } + user := &goth.User{ + RawData: map[string]any{}, + } + assert.True(t, shouldSyncFromGroupClaim(source, user)) + }) +} + +func TestGetUserAdminAndRestrictedFromGroupClaims_GoogleMissingClaim(t *testing.T) { + source := &oauth2.Source{ + Provider: "gplus", + GroupClaimName: "groups", + AdminGroup: "g-admins@example.com", + RestrictedGroup: "g-restricted@example.com", + } + user := &goth.User{ + RawData: map[string]any{}, + } + + isAdmin, isRestricted := getUserAdminAndRestrictedFromGroupClaims(source, user) + + assert.False(t, isAdmin.Has()) + assert.Equal(t, optional.None[bool](), isRestricted) +} + +func TestGetUserAdminAndRestrictedFromGroupClaims_GoogleEmptyClaim(t *testing.T) { + source := &oauth2.Source{ + Provider: "gplus", + GroupClaimName: "groups", + AdminGroup: "g-admins@example.com", + RestrictedGroup: "g-restricted@example.com", + } + user := &goth.User{ + RawData: map[string]any{ + "groups": []string{}, + }, + } + + isAdmin, isRestricted := getUserAdminAndRestrictedFromGroupClaims(source, user) + + require.True(t, isAdmin.Has()) + assert.Equal(t, user_service.UpdateOptionFieldFromSync(false), isAdmin) + assert.Equal(t, optional.Some(false), isRestricted) +} + +func TestGetUserAdminAndRestrictedFromGroupClaims_NonGoogleMissingClaim(t *testing.T) { + source := &oauth2.Source{ + Provider: "openidConnect", + GroupClaimName: "groups", + AdminGroup: "g-admins@example.com", + RestrictedGroup: "g-restricted@example.com", + } + user := &goth.User{ + RawData: map[string]any{}, + } + + isAdmin, isRestricted := getUserAdminAndRestrictedFromGroupClaims(source, user) + + require.True(t, isAdmin.Has()) + assert.Equal(t, user_service.UpdateOptionFieldFromSync(false), isAdmin) + assert.Equal(t, optional.Some(false), isRestricted) +} diff --git a/services/auth/source/oauth2/additional_info_provider.go b/services/auth/source/oauth2/additional_info_provider.go index d9146c4967..fb31864aad 100644 --- a/services/auth/source/oauth2/additional_info_provider.go +++ b/services/auth/source/oauth2/additional_info_provider.go @@ -20,6 +20,7 @@ import ( // with any extra data injected into RawData. type AdditionalInfoProvider interface { FetchAdditionalInfo(ctx context.Context, user goth.User) (goth.User, error) + FailLoginOnAdditionalInfoError() bool } // GetAdditionalInfoProvider returns an AdditionalInfoProvider for the given @@ -39,8 +40,21 @@ func GetAdditionalInfoProvider(source *Source, gothUser *goth.User) AdditionalIn // This is intentional — the token is issued moments before this // call during the login flow and is guaranteed to be fresh. authenticatedClient := go_oauth2.NewClient(context.Background(), go_oauth2.StaticTokenSource(oauthToken)) - return google_module.NewClient(authenticatedClient, claimName) + return google_module.NewClient(authenticatedClient, claimName, isGoogleGroupClaimRequiredForLoginFlow(source)) } } return nil } + +func isGoogleGroupClaimRequiredForLoginFlow(source *Source) bool { + groupClaimName := source.GroupClaimName + if groupClaimName == "" { + groupClaimName = "groups" + } + + // Fail closed only when login itself depends on the group claim. + // + // Admin/restricted/team sync can preserve the user's previous state when the + // group claim is missing, so those options intentionally stay fail-open. + return source.RequiredClaimName == groupClaimName +} diff --git a/services/auth/source/oauth2/additional_info_provider_test.go b/services/auth/source/oauth2/additional_info_provider_test.go new file mode 100644 index 0000000000..a4b7af1881 --- /dev/null +++ b/services/auth/source/oauth2/additional_info_provider_test.go @@ -0,0 +1,62 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package oauth2 + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestIsGoogleGroupClaimRequiredForLoginFlow(t *testing.T) { + t.Run("no group-dependent options", func(t *testing.T) { + source := &Source{ + GroupClaimName: "groups", + } + assert.False(t, isGoogleGroupClaimRequiredForLoginFlow(source)) + }) + + t.Run("required claim uses group claim", func(t *testing.T) { + source := &Source{ + GroupClaimName: "custom_groups", + RequiredClaimName: "custom_groups", + } + assert.True(t, isGoogleGroupClaimRequiredForLoginFlow(source)) + }) + + t.Run("required claim uses default groups claim", func(t *testing.T) { + source := &Source{ + RequiredClaimName: "groups", + } + assert.True(t, isGoogleGroupClaimRequiredForLoginFlow(source)) + }) + + t.Run("admin group configured", func(t *testing.T) { + source := &Source{ + AdminGroup: "admins@example.com", + } + assert.False(t, isGoogleGroupClaimRequiredForLoginFlow(source)) + }) + + t.Run("restricted group configured", func(t *testing.T) { + source := &Source{ + RestrictedGroup: "restricted@example.com", + } + assert.False(t, isGoogleGroupClaimRequiredForLoginFlow(source)) + }) + + t.Run("group team mapping configured", func(t *testing.T) { + source := &Source{ + GroupTeamMap: "{\"a\": {\"org\": [\"team\"]}}", + } + assert.False(t, isGoogleGroupClaimRequiredForLoginFlow(source)) + }) + + t.Run("group team mapping removal enabled", func(t *testing.T) { + source := &Source{ + GroupTeamMapRemoval: true, + } + assert.False(t, isGoogleGroupClaimRequiredForLoginFlow(source)) + }) +}