Fixed issues identified in PR review

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
+}

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)
+}

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,

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)
+}

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
+}

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))
+	})
+}