From 5109687831474e0c47be1a66984ebe00735f429c Mon Sep 17 00:00:00 2001 From: silverwind Date: Thu, 9 Apr 2026 20:34:10 +0200 Subject: [PATCH] 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) +}