diff --git a/modules/container/set.go b/modules/container/set.go index 105533f203..2d59ba5898 100644 --- a/modules/container/set.go +++ b/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)) diff --git a/modules/container/set_test.go b/modules/container/set_test.go index a8b7ff8190..44b17adebe 100644 --- a/modules/container/set_test.go +++ b/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()) } diff --git a/services/auth/source/source_group_sync.go b/services/auth/source/source_group_sync.go index 0ca3fdd2cd..d9c6ff7260 100644 --- a/services/auth/source/source_group_sync.go +++ b/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 } diff --git a/services/auth/source/source_group_sync_test.go b/services/auth/source/source_group_sync_test.go new file mode 100644 index 0000000000..e6e6fc78fe --- /dev/null +++ b/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)) + }) +}