fix: make auth source group sync correctly handle team removal (#37161)

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

modules/container/set.go

@@ -52,6 +52,28 @@ func (s Set[T]) Remove(value T) bool {
 	return false
 }
 
+// RemoveFromSet removes the specified elements from the set.
+// Returns the number of elements successfully removed.
+func (s Set[T]) RemoveFromSet(o Set[T]) (n int) {
+	for value := range o {
+		if s.Remove(value) {
+			n++
+		}
+	}
+	return n
+}
+
+// RemoveFromSlice removes the specified elements from the slice.
+// Returns the number of elements successfully removed.
+func (s Set[T]) RemoveFromSlice(o []T) (n int) {
+	for _, value := range o {
+		if s.Remove(value) {
+			n++
+		}
+	}
+	return n
+}
+
 // Values gets a list of all elements in the set.
 func (s Set[T]) Values() []T {
 	keys := make([]T, 0, len(s))

modules/container/set_test.go

@@ -35,4 +35,14 @@ func TestSet(t *testing.T) {
 	assert.False(t, s.Contains("key1"))
 	assert.True(t, s.Contains("key6"))
 	assert.True(t, s.Contains("key7"))
+
+	s = SetOf("a", "b", "c")
+	n := s.RemoveFromSet(SetOf("b", "c", "d"))
+	assert.Equal(t, 2, n)
+	assert.ElementsMatch(t, []string{"a"}, s.Values())
+
+	s = SetOf("a", "b", "c")
+	n = s.RemoveFromSlice([]string{"b", "c", "d"})
+	assert.Equal(t, 2, n)
+	assert.ElementsMatch(t, []string{"a"}, s.Values())
 }

routers/web/auth/oauth2_provider.go

@@ -207,9 +207,24 @@ func IntrospectOAuth(ctx *context.Context) {
 	ctx.JSON(http.StatusOK, response)
 }
 
+func oauthDoerAuthorizePreCheck(ctx *context.Context, formState string) bool {
+	if ctx.DoerNeedTwoFactorAuth() {
+		handleAuthorizeError(ctx, AuthorizeError{
+			ErrorCode:        ErrorCodeAccessDenied,
+			ErrorDescription: "two-factor authentication is required",
+			State:            formState,
+		}, "")
+		return false
+	}
+	return true
+}
+
 // AuthorizeOAuth manages authorize requests
 func AuthorizeOAuth(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.AuthorizationForm)
+	if !oauthDoerAuthorizePreCheck(ctx, form.State) {
+		return
+	}
 	errs := binding.Errors{}
 	errs = form.Validate(ctx.Req, errs)
 	if len(errs) > 0 {
@@ -385,6 +400,10 @@ func AuthorizeOAuth(ctx *context.Context) {
 // GrantApplicationOAuth manages the post request submitted when a user grants access to an application
 func GrantApplicationOAuth(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.GrantApplicationForm)
+	if !oauthDoerAuthorizePreCheck(ctx, form.State) {
+		return
+	}
+
 	if ctx.Session.Get("client_id") != form.ClientID || ctx.Session.Get("state") != form.State ||
 		ctx.Session.Get("redirect_uri") != form.RedirectURI {
 		ctx.HTTPError(http.StatusBadRequest)

services/auth/source/source_group_sync.go

@@ -6,6 +6,7 @@ package source
 import (
 	"context"
 	"fmt"
+	"strings"
 
 	"gitea.dev/models/organization"
 	user_model "gitea.dev/models/user"
@@ -45,21 +46,38 @@ func SyncGroupsToTeamsCached(ctx context.Context, user *user_model.User, sourceU
 	return nil
 }
 
-func resolveMappedMemberships(sourceUserGroups container.Set[string], sourceGroupTeamMapping map[string]map[string][]string) (map[string][]string, map[string][]string) {
-	membershipsToAdd := map[string][]string{}
-	membershipsToRemove := map[string][]string{}
-	for group, memberships := range sourceGroupTeamMapping {
+func resolveMappedMemberships(sourceUserGroups container.Set[string], groupOrgTeamsMapping map[string]map[string][]string) (membershipsToAdd, membershipsToRemove map[string][]string) {
+	membershipsToAdd, membershipsToRemove = map[string][]string{}, map[string][]string{}
+	for group, orgTeams := range groupOrgTeamsMapping {
 		isUserInGroup := sourceUserGroups.Contains(group)
 		if isUserInGroup {
-			for org, teams := range memberships {
-				membershipsToAdd[org] = append(membershipsToAdd[org], teams...)
+			for org, teams := range orgTeams {
+				for _, teamName := range teams {
+					membershipsToAdd[org] = append(membershipsToAdd[org], strings.ToLower(teamName))
+				}
 			}
 		} else {
-			for org, teams := range memberships {
-				membershipsToRemove[org] = append(membershipsToRemove[org], teams...)
+			for org, teams := range orgTeams {
+				for _, teamName := range teams {
+					membershipsToRemove[org] = append(membershipsToRemove[org], strings.ToLower(teamName))
+				}
 			}
 		}
 	}
+
+	// If another group grants the same team (to add), don't remove it
+	for org, removeTeams := range membershipsToRemove {
+		removeTeamSet := container.SetOf(removeTeams...)
+		removedCount := removeTeamSet.RemoveFromSlice(membershipsToAdd[org])
+		if removedCount > 0 {
+			removeTeams = removeTeamSet.Values()
+			membershipsToRemove[org] = removeTeams
+			if len(removeTeams) == 0 {
+				delete(membershipsToRemove, org)
+			}
+		}
+	}
+
 	return membershipsToAdd, membershipsToRemove
 }
 
@@ -106,6 +124,10 @@ func syncGroupsToTeamsCached(ctx context.Context, user *user_model.User, orgTeam
 				}
 			} else if action == syncRemove && isMember {
 				if err := org_service.RemoveTeamMember(ctx, team, user); err != nil {
+					if organization.IsErrLastOrgOwner(err) {
+						log.Warn("group sync: Skipping removal of last owner in org %s for user %s: %v", org.Name, user.Name, err)
+						continue
+					}
 					log.Error("group sync: Could not remove user from team: %v", err)
 					return err
 				}

services/auth/source/source_group_sync_test.go

@@ -0,0 +1,72 @@
+// Copyright 2026 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package source
+
+import (
+	"testing"
+
+	"gitea.dev/models/organization"
+	"gitea.dev/models/unittest"
+	user_model "gitea.dev/models/user"
+	"gitea.dev/modules/container"
+	org_service "gitea.dev/services/org"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestMain(m *testing.M) {
+	unittest.MainTest(m, &unittest.TestOptions{})
+}
+
+func TestSyncGroupsToTeams(t *testing.T) {
+	require.NoError(t, unittest.PrepareTestDatabase())
+
+	t.Run("SyncAddRemove", func(t *testing.T) {
+		sourceUserGroups := container.SetOf("groupA")
+		sourceGroupTeamMapping := map[string]map[string][]string{
+			"groupA": {"org3": {"Owners", "team1"}},
+			"groupB": {"org3": {"Owners", "Team2"}},
+		}
+
+		// Deduplication: "Owners" must not be in the remove list when groupA grants it,
+		// while "team2" (only mapped by groupB, which the user is not in) must remain in the remove list.
+		membershipsToAdd, membershipsToRemove := resolveMappedMemberships(sourceUserGroups, sourceGroupTeamMapping)
+		assert.ElementsMatch(t, []string{"owners", "team1"}, membershipsToAdd["org3"])
+		assert.ElementsMatch(t, []string{"team2"}, membershipsToRemove["org3"])
+	})
+
+	t.Run("LastOwnerRemovalSkipped", func(t *testing.T) {
+		user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2})
+		user4 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4})
+		org3 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3})
+
+		getUserTeamNames := func(t *testing.T) (ret []string) {
+			userTeams, err := organization.GetUserOrgTeams(t.Context(), org3.ID, user2.ID)
+			require.NoError(t, err)
+			for _, team := range userTeams {
+				ret = append(ret, team.Name)
+			}
+			return ret
+		}
+
+		// The last owner should not be removed from the owners team
+		// "teamCreateRepo" is always kept because it is not listed in the group mapping
+		testSyncUserWithoutGroupMapping := func(t *testing.T) {
+			userGroup := container.SetOf("user2Group")
+			sourceGroupTeamMapping := map[string]map[string][]string{"otherGroup": {"org3": []string{"Owners", "TEAM1"}}}
+			require.NoError(t, SyncGroupsToTeams(t.Context(), user2, userGroup, sourceGroupTeamMapping, true))
+		}
+
+		// 1. "user2" is the only owner, so its "owners" team is kept
+		assert.ElementsMatch(t, []string{"Owners", "team1", "teamCreateRepo"}, getUserTeamNames(t))
+		testSyncUserWithoutGroupMapping(t)
+		assert.ElementsMatch(t, []string{"Owners", "teamCreateRepo"}, getUserTeamNames(t))
+		// 2. there are other owners, so the user2 is removed from the "owners" team
+		teamOwners, _ := organization.GetTeam(t.Context(), org3.ID, "owners")
+		_ = org_service.AddTeamMember(t.Context(), teamOwners, user4)
+		testSyncUserWithoutGroupMapping(t)
+		assert.ElementsMatch(t, []string{"teamCreateRepo"}, getUserTeamNames(t))
+	})
+}