From 311aac4acba6eb2625bdf987afb3649f71d486af Mon Sep 17 00:00:00 2001
From: silverwind <me@silverwind.io>
Date: Thu, 23 Apr 2026 12:26:25 +0200
Subject: [PATCH] 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) <noreply@anthropic.com>
---
 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) {