Mirrors/gitea
0
0
Fork 0
mirror of https://github.com/go-gitea/gitea.git synced 2026-05-12 11:13:21 +02:00
Code Issues Activity

Skip sole-owner removal during group sync instead of aborting login

Browse Source 
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) <noreply@anthropic.com>
This commit is contained in:
silverwind 2026-04-23 12:26:25 +02:00
parent c48792f743
commit 311aac4acb
No known key found for this signature in database
GPG Key ID: 2E62B41C93869443
2 changed files with 35 additions and 2 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
services/auth/source/source_group_sync.go
View File

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

28
services/auth/source/source_group_sync_test.go
View File

@ -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) {