diff --git a/internal/auth/team_db.go b/internal/auth/team_db.go index df6d6cbf5..6d9d413a9 100644 --- a/internal/auth/team_db.go +++ b/internal/auth/team_db.go @@ -78,6 +78,14 @@ func (db *pgdb) getTeamByID(ctx context.Context, id string) (*Team, error) { return teamRow(result).toTeam(), nil } +func (db *pgdb) getTeamForUpdate(ctx context.Context, id string) (*Team, error) { + result, err := db.FindTeamByIDForUpdate(ctx, sql.String(id)) + if err != nil { + return nil, sql.Error(err) + } + return teamRow(result).toTeam(), nil +} + func (db *pgdb) listTeams(ctx context.Context, organization string) ([]*Team, error) { result, err := db.FindTeamsByOrg(ctx, sql.String(organization)) if err != nil { diff --git a/internal/auth/user_service.go b/internal/auth/user_service.go index bf5c370af..95d83b457 100644 --- a/internal/auth/user_service.go +++ b/internal/auth/user_service.go @@ -128,7 +128,7 @@ func (a *service) AddTeamMembership(ctx context.Context, opts TeamMembershipOpti return nil } -// RemoveTeamMembership removes a user from a team. If opts.Tx is non-nil then database +// RemoveTeamMembership removes users from a team. If opts.Tx is non-nil then database // queries are made within that transaction. func (a *service) RemoveTeamMembership(ctx context.Context, opts TeamMembershipOptions) error { db := a.db @@ -136,7 +136,7 @@ func (a *service) RemoveTeamMembership(ctx context.Context, opts TeamMembershipO db = newDB(opts.Tx, a.db.Logger) } - team, err := db.getTeamByID(ctx, opts.TeamID) + team, err := db.getTeamForUpdate(ctx, opts.TeamID) if err != nil { a.Error(err, "retrieving team", "team_id", opts.TeamID) return err @@ -147,12 +147,13 @@ func (a *service) RemoveTeamMembership(ctx context.Context, opts TeamMembershipO return err } + // check whether all members of the owners group are going to be deleted + // (which is not allowed) if team.Name == "owners" { - owners, err := a.ListTeamMembers(ctx, team.ID) - if err != nil { + if owners, err := db.listTeamMembers(ctx, team.ID); err != nil { + a.Error(err, "removing team membership: listing team members", "team_id", team.ID, "subject", subject) return err - } - if len(owners) == 1 { + } else if len(owners) <= len(opts.Usernames) { return ErrCannotDeleteOnlyOwner } } diff --git a/internal/integration/user_test.go b/internal/integration/user_test.go index c44f765b6..1bfe81528 100644 --- a/internal/integration/user_test.go +++ b/internal/integration/user_test.go @@ -200,9 +200,12 @@ func TestUser(t *testing.T) { owners, err := svc.GetTeam(ctx, org.Name, "owners") require.NoError(t, err) + // add another owner + another := svc.createUser(t, auth.WithTeams(owners)) + // try to delete both members from the owners team err = svc.RemoveTeamMembership(ctx, auth.TeamMembershipOptions{ - Usernames: []string{owner.Username}, + Usernames: []string{owner.Username, another.Username}, TeamID: owners.ID, }) assert.Equal(t, auth.ErrCannotDeleteOnlyOwner, err)