From 5109687831474e0c47be1a66984ebe00735f429c Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 9 Apr 2026 20:34:10 +0200 Subject: [PATCH 1/4] Fix OAuth2 group sync removing sole org owner when multiple groups map same team When multiple OAuth2 groups map to the same org team (e.g. Owners) and the user is in only one of those groups, `resolveMappedMemberships` put the team in both the add and remove lists. Since removal runs before addition, removing the sole owner failed with `ErrLastOrgOwner`, which the OAuth handler surfaced as HTTP 500, locking the user out on login. Fix by deduplicating: after building both lists, exclude any team from the remove list that is also in the add list for that org. Co-Authored-By: Claude (Opus 4.6) --- services/auth/source/source_group_sync.go | 18 ++++++++ .../auth/source/source_group_sync_test.go | 41 +++++++++++++++++++ 2 files changed, 59 insertions(+) create mode 100644 services/auth/source/source_group_sync_test.go diff --git a/services/auth/source/source_group_sync.go b/services/auth/source/source_group_sync.go index 9cb7d4165c..69554feace 100644 --- a/services/auth/source/source_group_sync.go +++ b/services/auth/source/source_group_sync.go @@ -60,6 +60,24 @@ func resolveMappedMemberships(sourceUserGroups container.Set[string], sourceGrou } } } + + // If any group grants a team membership, do not remove it even if another + // group the user is not in also maps to that team. + for org, addTeams := range membershipsToAdd { + removeTeams, ok := membershipsToRemove[org] + if !ok { + continue + } + addSet := container.SetOf(addTeams...) + filtered := make([]string, 0, len(removeTeams)) + for _, team := range removeTeams { + if !addSet.Contains(team) { + filtered = append(filtered, team) + } + } + membershipsToRemove[org] = filtered + } + return membershipsToAdd, membershipsToRemove } 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..dae2250403 --- /dev/null +++ b/services/auth/source/source_group_sync_test.go @@ -0,0 +1,41 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package source + +import ( + "testing" + + "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" + "code.gitea.io/gitea/modules/container" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestMain(m *testing.M) { + unittest.MainTest(m, &unittest.TestOptions{}) +} + +// Fixture: team 1 = org3's "Owners" team, num_members=1, sole member is user2. +func TestSyncGroupsToTeams_LastOwnerNotRemoved(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + sourceUserGroups := container.SetOf("groupA") + sourceGroupTeamMapping := map[string]map[string][]string{ + "groupA": {"org3": {"Owners"}}, + "groupB": {"org3": {"Owners", "Developers"}}, + } + + // Verify deduplication: Owners must not be in remove list when groupA grants it. + membershipsToAdd, membershipsToRemove := resolveMappedMemberships(sourceUserGroups, sourceGroupTeamMapping) + assert.Contains(t, membershipsToAdd["org3"], "Owners") + assert.NotContains(t, membershipsToRemove["org3"], "Owners") + assert.Contains(t, membershipsToRemove["org3"], "Developers") + + // End-to-end: sync must not fail with ErrLastOrgOwner. + err := SyncGroupsToTeams(t.Context(), user2, sourceUserGroups, sourceGroupTeamMapping, true) + require.NoError(t, err) +} From 94891ebf4ef4fe229bfa5865cccaa5cad9b81cba Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 9 Apr 2026 20:51:34 +0200 Subject: [PATCH 2/4] Address review: delete empty remove entries, use existing fixture team Co-Authored-By: Claude (Opus 4.6) --- services/auth/source/source_group_sync.go | 6 +++++- services/auth/source/source_group_sync_test.go | 4 ++-- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/services/auth/source/source_group_sync.go b/services/auth/source/source_group_sync.go index 69554feace..a95d357c4c 100644 --- a/services/auth/source/source_group_sync.go +++ b/services/auth/source/source_group_sync.go @@ -75,7 +75,11 @@ func resolveMappedMemberships(sourceUserGroups container.Set[string], sourceGrou filtered = append(filtered, team) } } - membershipsToRemove[org] = filtered + if len(filtered) > 0 { + membershipsToRemove[org] = filtered + } else { + delete(membershipsToRemove, org) + } } return membershipsToAdd, membershipsToRemove diff --git a/services/auth/source/source_group_sync_test.go b/services/auth/source/source_group_sync_test.go index dae2250403..6eecaac150 100644 --- a/services/auth/source/source_group_sync_test.go +++ b/services/auth/source/source_group_sync_test.go @@ -26,14 +26,14 @@ func TestSyncGroupsToTeams_LastOwnerNotRemoved(t *testing.T) { sourceUserGroups := container.SetOf("groupA") sourceGroupTeamMapping := map[string]map[string][]string{ "groupA": {"org3": {"Owners"}}, - "groupB": {"org3": {"Owners", "Developers"}}, + "groupB": {"org3": {"Owners", "team1"}}, } // Verify deduplication: Owners must not be in remove list when groupA grants it. membershipsToAdd, membershipsToRemove := resolveMappedMemberships(sourceUserGroups, sourceGroupTeamMapping) assert.Contains(t, membershipsToAdd["org3"], "Owners") assert.NotContains(t, membershipsToRemove["org3"], "Owners") - assert.Contains(t, membershipsToRemove["org3"], "Developers") + assert.Contains(t, membershipsToRemove["org3"], "team1") // End-to-end: sync must not fail with ErrLastOrgOwner. err := SyncGroupsToTeams(t.Context(), user2, sourceUserGroups, sourceGroupTeamMapping, true) From c48792f743c75847ae10148f9f8e7a079e53ef01 Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 23 Apr 2026 12:04:46 +0200 Subject: [PATCH 3/4] Address review: clearer comment, stronger test, dedup unit test - Tighten the dedup comment to focus on the ErrLastOrgOwner reason. - In the integration test, assert up-front that org3 Owners has exactly one member so the regression guard is explicit rather than implicit in the fixture. - Add a pure unit test for resolveMappedMemberships covering the branch where all removals overlap adds (org key deleted from remove map) and multiple orgs. Co-Authored-By: Claude (Opus 4.7) --- services/auth/source/source_group_sync.go | 4 +-- .../auth/source/source_group_sync_test.go | 34 ++++++++++++++++--- 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/services/auth/source/source_group_sync.go b/services/auth/source/source_group_sync.go index a95d357c4c..aa2dd812ea 100644 --- a/services/auth/source/source_group_sync.go +++ b/services/auth/source/source_group_sync.go @@ -61,8 +61,8 @@ func resolveMappedMemberships(sourceUserGroups container.Set[string], sourceGrou } } - // If any group grants a team membership, do not remove it even if another - // group the user is not in also maps to that team. + // If another group grants the same team, don't remove it — for the Owners + // team this would fail with ErrLastOrgOwner before the add runs. for org, addTeams := range membershipsToAdd { removeTeams, ok := membershipsToRemove[org] if !ok { diff --git a/services/auth/source/source_group_sync_test.go b/services/auth/source/source_group_sync_test.go index 6eecaac150..f7c7e54d6e 100644 --- a/services/auth/source/source_group_sync_test.go +++ b/services/auth/source/source_group_sync_test.go @@ -6,6 +6,7 @@ package source import ( "testing" + "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/unittest" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" @@ -18,24 +19,49 @@ func TestMain(m *testing.M) { unittest.MainTest(m, &unittest.TestOptions{}) } -// Fixture: team 1 = org3's "Owners" team, num_members=1, sole member is user2. func TestSyncGroupsToTeams_LastOwnerNotRemoved(t *testing.T) { require.NoError(t, unittest.PrepareTestDatabase()) + // Fixtures: in org3, user2 is the only member of the "Owners" team, and + // team "team1" (a non-owner team) has user2 and user4 as members. user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + org3 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) + ownersTeam, err := org3.GetOwnerTeam(t.Context()) + require.NoError(t, err) + require.Equal(t, 1, ownersTeam.NumMembers) + sourceUserGroups := container.SetOf("groupA") sourceGroupTeamMapping := map[string]map[string][]string{ "groupA": {"org3": {"Owners"}}, "groupB": {"org3": {"Owners", "team1"}}, } - // Verify deduplication: Owners must not be in remove list when groupA grants it. + // Deduplication: "Owners" must not be in the remove list when groupA grants it, + // while "team1" (only mapped by groupB, which the user is not in) must remain. membershipsToAdd, membershipsToRemove := resolveMappedMemberships(sourceUserGroups, sourceGroupTeamMapping) assert.Contains(t, membershipsToAdd["org3"], "Owners") assert.NotContains(t, membershipsToRemove["org3"], "Owners") assert.Contains(t, membershipsToRemove["org3"], "team1") // End-to-end: sync must not fail with ErrLastOrgOwner. - err := SyncGroupsToTeams(t.Context(), user2, sourceUserGroups, sourceGroupTeamMapping, true) - require.NoError(t, err) + require.NoError(t, SyncGroupsToTeams(t.Context(), user2, sourceUserGroups, sourceGroupTeamMapping, true)) +} + +func TestResolveMappedMemberships_Dedup(t *testing.T) { + sourceUserGroups := container.SetOf("in1", "in2") + sourceGroupTeamMapping := map[string]map[string][]string{ + "in1": {"orgA": {"Owners"}, "orgB": {"team1"}}, + "in2": {"orgB": {"team1"}}, // duplicate add, no effect on remove + "out1": {"orgA": {"Owners"}, "orgB": {"team2"}}, // orgA fully overlaps add; orgB team2 has no add + "out2": {"orgC": {"team3"}}, // no add for orgC + } + + membershipsToAdd, membershipsToRemove := resolveMappedMemberships(sourceUserGroups, sourceGroupTeamMapping) + + assert.ElementsMatch(t, []string{"Owners"}, membershipsToAdd["orgA"]) + assert.ElementsMatch(t, []string{"team1", "team1"}, membershipsToAdd["orgB"]) + _, hasOrgA := membershipsToRemove["orgA"] + assert.False(t, hasOrgA, "orgA must be deleted from remove map when all removals overlap adds") + assert.ElementsMatch(t, []string{"team2"}, membershipsToRemove["orgB"]) + assert.ElementsMatch(t, []string{"team3"}, membershipsToRemove["orgC"]) } From 311aac4acba6eb2625bdf987afb3649f71d486af Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 23 Apr 2026 12:26:25 +0200 Subject: [PATCH 4/4] Skip sole-owner removal during group sync instead of aborting login MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Even with the add/remove dedup in place, a user who is no longer in any group that maps to Owners but is the sole owner of the org still hits ErrLastOrgOwner during sync, which aborts OAuth login. Catch that one error in syncGroupsToTeamsCached, log a warning, and continue — the org invariant is preserved, login succeeds, and the next sync converges after an admin promotes another owner. Also iterate membershipsToRemove (not membershipsToAdd) in the dedup loop so we skip set allocation when an org has no removes, and add a dedicated test plus a post-sync owner-membership assertion in the existing test. Co-Authored-By: Claude (Opus 4.7) --- services/auth/source/source_group_sync.go | 9 ++++-- .../auth/source/source_group_sync_test.go | 28 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/services/auth/source/source_group_sync.go b/services/auth/source/source_group_sync.go index aa2dd812ea..5317b4b7ff 100644 --- a/services/auth/source/source_group_sync.go +++ b/services/auth/source/source_group_sync.go @@ -63,8 +63,8 @@ func resolveMappedMemberships(sourceUserGroups container.Set[string], sourceGrou // If another group grants the same team, don't remove it — for the Owners // team this would fail with ErrLastOrgOwner before the add runs. - for org, addTeams := range membershipsToAdd { - removeTeams, ok := membershipsToRemove[org] + for org, removeTeams := range membershipsToRemove { + addTeams, ok := membershipsToAdd[org] if !ok { continue } @@ -128,6 +128,11 @@ 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 { + // Skip sole-owner removal so OAuth login still succeeds. + 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 index f7c7e54d6e..b003eb0488 100644 --- a/services/auth/source/source_group_sync_test.go +++ b/services/auth/source/source_group_sync_test.go @@ -45,6 +45,34 @@ func TestSyncGroupsToTeams_LastOwnerNotRemoved(t *testing.T) { // End-to-end: sync must not fail with ErrLastOrgOwner. require.NoError(t, SyncGroupsToTeams(t.Context(), user2, sourceUserGroups, sourceGroupTeamMapping, true)) + + // User2 must still be a member of the Owners team. + stillOwner, err := organization.IsTeamMember(t.Context(), org3.ID, ownersTeam.ID, user2.ID) + require.NoError(t, err) + assert.True(t, stillOwner) +} + +func TestSyncGroupsToTeams_LastOwnerRemovalSkippedWhenNotGranted(t *testing.T) { + require.NoError(t, unittest.PrepareTestDatabase()) + + user2 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 2}) + org3 := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: 3}) + ownersTeam, err := org3.GetOwnerTeam(t.Context()) + require.NoError(t, err) + require.Equal(t, 1, ownersTeam.NumMembers) + + // User is in no mapped group, so Owners is scheduled for removal with no + // offsetting add. The remove must be attempted and gracefully skipped. + sourceUserGroups := container.SetOf[string]() + sourceGroupTeamMapping := map[string]map[string][]string{ + "groupA": {"org3": {"Owners"}}, + } + + require.NoError(t, SyncGroupsToTeams(t.Context(), user2, sourceUserGroups, sourceGroupTeamMapping, true)) + + stillOwner, err := organization.IsTeamMember(t.Context(), org3.ID, ownersTeam.ID, user2.ID) + require.NoError(t, err) + assert.True(t, stillOwner, "sole owner must remain after sync that couldn't remove them") } func TestResolveMappedMemberships_Dedup(t *testing.T) {