fix: prevent empty owners team (#499)

Owners team cannot be empty. OTF did prevent this, but didn't account
for the scenario in which _multiple_ team members are deleted at once.

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 {

internal/auth/user_service.go

@@ -128,15 +128,15 @@ 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
 	if opts.Tx != nil {
 		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
 		}
 	}

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)