diff --git a/services/auth/source/oauth2/source_group_sync_test.go b/services/auth/source/oauth2/source_group_sync_test.go new file mode 100644 index 0000000000..77430b53e9 --- /dev/null +++ b/services/auth/source/oauth2/source_group_sync_test.go @@ -0,0 +1,54 @@ +// Copyright 2026 The Gitea Authors. All rights reserved. +// SPDX-License-Identifier: MIT + +package oauth2 + +import ( + "sync" + "testing" + + "code.gitea.io/gitea/models/db" + "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" + auth_source "code.gitea.io/gitea/services/auth/source" + + "github.com/stretchr/testify/assert" +) + +func TestSyncGroupsToTeamsConcurrentRuns(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2}) + org := unittest.AssertExistsAndLoadBean(t, &organization.Organization{ID: team.OrgID}) + + sourceGroupTeamMapping := map[string]map[string][]string{ + "ldap-group": { + org.Name: []string{team.Name}, + }, + } + + var wg sync.WaitGroup + for range 10 { + wg.Go(func() { + assert.NoError(t, auth_source.SyncGroupsToTeams(ctx, user, container.SetOf("ldap-group"), sourceGroupTeamMapping, true)) + }) + wg.Go(func() { + assert.NoError(t, auth_source.SyncGroupsToTeams(ctx, user, container.Set[string]{}, sourceGroupTeamMapping, true)) + }) + } + wg.Wait() + + memberCount, err := db.GetEngine(ctx).Count(&organization.TeamUser{OrgID: team.OrgID, TeamID: team.ID, UID: user.ID}) + assert.NoError(t, err) + assert.LessOrEqual(t, memberCount, int64(1)) + + team = unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: team.ID}) + totalCount, err := db.GetEngine(ctx).Count(&organization.TeamUser{OrgID: team.OrgID, TeamID: team.ID}) + assert.NoError(t, err) + assert.EqualValues(t, team.NumMembers, totalCount) + assert.GreaterOrEqual(t, team.NumMembers, 0) +} diff --git a/services/auth/source/source_group_sync.go b/services/auth/source/source_group_sync.go index 9cb7d4165c..c28444b9f6 100644 --- a/services/auth/source/source_group_sync.go +++ b/services/auth/source/source_group_sync.go @@ -10,6 +10,7 @@ import ( "code.gitea.io/gitea/models/organization" user_model "code.gitea.io/gitea/models/user" "code.gitea.io/gitea/modules/container" + "code.gitea.io/gitea/modules/globallock" "code.gitea.io/gitea/modules/log" org_service "code.gitea.io/gitea/services/org" ) @@ -94,21 +95,26 @@ func syncGroupsToTeamsCached(ctx context.Context, user *user_model.User, orgTeam teamCache[orgName+teamName] = team } - isMember, err := organization.IsTeamMember(ctx, org.ID, team.ID, user.ID) - if err != nil { - return err - } + if err := globallock.LockAndDo(ctx, fmt.Sprintf("group_sync_team_%d", team.ID), func(ctx context.Context) error { + isMember, err := organization.IsTeamMember(ctx, org.ID, team.ID, user.ID) + if err != nil { + return err + } - if action == syncAdd && !isMember { - if err := org_service.AddTeamMember(ctx, team, user); err != nil { - log.Error("group sync: Could not add user to team: %v", err) - return err - } - } else if action == syncRemove && isMember { - if err := org_service.RemoveTeamMember(ctx, team, user); err != nil { - log.Error("group sync: Could not remove user from team: %v", err) - return err + if action == syncAdd && !isMember { + if err := org_service.AddTeamMember(ctx, team, user); err != nil { + log.Error("group sync: Could not add user to team: %v", err) + return err + } + } else if action == syncRemove && isMember { + if err := org_service.RemoveTeamMember(ctx, team, user); err != nil { + log.Error("group sync: Could not remove user from team: %v", err) + return err + } } + return nil + }); err != nil { + return err } } } diff --git a/services/org/team.go b/services/org/team.go index 6c92ee4f44..3ccef4ae8e 100644 --- a/services/org/team.go +++ b/services/org/team.go @@ -233,13 +233,20 @@ func AddTeamMember(ctx context.Context, team *organization.Team, user *user_mode sess := db.GetEngine(ctx) - if err := db.Insert(ctx, &organization.TeamUser{ - UID: user.ID, - OrgID: team.OrgID, - TeamID: team.ID, - }); err != nil { + res, err := sess.Exec("INSERT INTO team_user (org_id, team_id, uid) SELECT ?, ?, ? WHERE NOT EXISTS (SELECT 1 FROM team_user WHERE org_id=? AND team_id=? AND uid=?)", + team.OrgID, team.ID, user.ID, team.OrgID, team.ID, user.ID) + if err != nil { return err - } else if _, err := sess.Incr("num_members").ID(team.ID).Update(new(organization.Team)); err != nil { + } + rowsAffected, err := res.RowsAffected() + if err != nil { + return err + } + if rowsAffected == 0 { + return nil + } + + if _, err := sess.Incr("num_members").ID(team.ID).Update(new(organization.Team)); err != nil { return err } @@ -285,8 +292,6 @@ func removeTeamMember(ctx context.Context, team *organization.Team, user *user_m return organization.ErrLastOrgOwner{UID: user.ID} } - team.NumMembers-- - repos, err := repo_model.GetTeamRepositories(ctx, &repo_model.SearchTeamRepoOptions{ TeamID: team.ID, }) @@ -294,18 +299,24 @@ func removeTeamMember(ctx context.Context, team *organization.Team, user *user_m return err } - if _, err := e.Delete(&organization.TeamUser{ + rowsAffected, err := e.Delete(&organization.TeamUser{ UID: user.ID, OrgID: team.OrgID, TeamID: team.ID, - }); err != nil { + }) + if err != nil { return err - } else if _, err = e. - ID(team.ID). - Cols("num_members"). - Update(team); err != nil { + } + if rowsAffected == 0 { + return nil + } + + if _, err = e.Decr("num_members").ID(team.ID).Update(new(organization.Team)); err != nil { return err } + if team.NumMembers > 0 { + team.NumMembers-- + } // Delete access to team repositories. If any user or repo is missing, we can continue. for _, repo := range repos { diff --git a/services/org/team_test.go b/services/org/team_test.go index 5cb588b7dd..e03ab1ea2d 100644 --- a/services/org/team_test.go +++ b/services/org/team_test.go @@ -6,8 +6,10 @@ package org import ( "fmt" "strings" + "sync" "testing" + "code.gitea.io/gitea/models/db" issues_model "code.gitea.io/gitea/models/issues" "code.gitea.io/gitea/models/organization" "code.gitea.io/gitea/models/perm" @@ -328,3 +330,39 @@ func TestIncludesAllRepositoriesTeams(t *testing.T) { } assert.NoError(t, DeleteOrganization(t.Context(), org, false), "DeleteOrganization") } + +func TestTeamMemberConcurrentAddRemoveIdempotent(t *testing.T) { + assert.NoError(t, unittest.PrepareTestDatabase()) + + ctx := t.Context() + team := unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: 2}) + user := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 4}) + + var wg sync.WaitGroup + for range 8 { + wg.Go(func() { + assert.NoError(t, AddTeamMember(ctx, team, user)) + }) + } + wg.Wait() + + count, err := db.GetEngine(ctx).Count(&organization.TeamUser{OrgID: team.OrgID, TeamID: team.ID, UID: user.ID}) + assert.NoError(t, err) + assert.EqualValues(t, 1, count) + + for range 8 { + wg.Go(func() { + assert.NoError(t, RemoveTeamMember(ctx, team, user)) + }) + } + wg.Wait() + + count, err = db.GetEngine(ctx).Count(&organization.TeamUser{OrgID: team.OrgID, TeamID: team.ID, UID: user.ID}) + assert.NoError(t, err) + assert.EqualValues(t, 0, count) + + team = unittest.AssertExistsAndLoadBean(t, &organization.Team{ID: team.ID}) + memberCount, err := db.GetEngine(ctx).Count(&organization.TeamUser{OrgID: team.OrgID, TeamID: team.ID}) + assert.NoError(t, err) + assert.EqualValues(t, team.NumMembers, memberCount) +}