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

Address review: clearer comment, stronger test, dedup unit test

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

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

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

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

@ -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"])
}