mirror of
https://github.com/fleetdm/fleet
synced 2026-04-21 13:37:30 +00:00
Fix TOCTOU race in last global admin protection (#42172)
- [X] Changes file added for user-visible changes in `changes/`, `orbit/changes/` or `ee/fleetd-chrome/changes`. See [Changes files](https://github.com/fleetdm/fleet/blob/main/docs/Contributing/guides/committing-changes.md#changes-files) for more information. - [X] Input data is properly validated, `SELECT *` is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters. ## Testing - [X] QA'd all new/changed functionality manually Server log when SCIM ingestion receives a deactivation event for the last admin: ```log ts=2026-04-01T15:23:01Z level=error msg="failed to delete fleet user on deactivation" component=SCIM err="cannot delete last global admin" ``` Server response when attempting to demote last admin: ```json { "message": "Validation Failed", "errors": [ { "name": "global_role", "reason": "cannot demote the last global admin" } ], "uuid": "1d110f56-25ac-47b8-bc96-982354474a87" } ``` Server response when attempting to delete last admin: ```json { "message": "Validation Failed", "errors": [ { "name": "id", "reason": "cannot delete the last global admin" } ], "uuid": "1448c2da-30e2-4652-a9a8-a01fc4f9b9c1" } ``` --- ## Original AI Summary - Fixes a TOCTOU race condition where two concurrent admin operations could bypass the last-global-admin guard, leaving zero admins and permanently locking out the Fleet instance (fleetdm/confidential#14827) - Introduces two new atomic datastore methods (`DeleteUserIfNotLastAdmin`, `SaveUserIfNotLastAdmin`) that wrap the admin count check and the write in a single MySQL transaction with `SELECT ... FOR UPDATE` - Fixes all four vulnerable code paths: service `DeleteUser`, service `ModifyUser` (two demotion paths), and SCIM user deletion ## Test plan - [X] Manual verification: single admin cannot be deleted or demoted 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Lucas Manuel Rodriguez <lucas@fleetdm.com>
This commit is contained in:
parent
9983693e1d
commit
7e0d0db1b1
11 changed files with 180 additions and 120 deletions
1
changes/14827-prevent-TOCTOU-last-admin
Normal file
1
changes/14827-prevent-TOCTOU-last-admin
Normal file
|
|
@ -0,0 +1 @@
|
|||
* Fixed a TOCTOU related issue when checking before deleting last admin.
|
||||
|
|
@ -32,14 +32,9 @@ func TestUserDelete(t *testing.T) {
|
|||
}, nil
|
||||
}
|
||||
|
||||
// Allow deletion by returning multiple admins exist
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 2, nil
|
||||
}
|
||||
|
||||
deletedUser := uint(0)
|
||||
|
||||
ds.DeleteUserFunc = func(ctx context.Context, id uint) error {
|
||||
ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
deletedUser = id
|
||||
return nil
|
||||
}
|
||||
|
|
@ -227,17 +222,15 @@ func TestDeleteBulkUsers(t *testing.T) {
|
|||
return nil, ¬FoundError{}
|
||||
}
|
||||
|
||||
// Allow deletion by returning multiple admins exist
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 2, nil
|
||||
}
|
||||
|
||||
deletedUser := uint(0)
|
||||
|
||||
ds.DeleteUserFunc = func(ctx context.Context, id uint) error {
|
||||
deletedUser = id
|
||||
return nil
|
||||
}
|
||||
ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return nil
|
||||
}
|
||||
|
||||
assert.Equal(t, "", RunAppForTest(t, []string{"user", "delete-users", "--csv", csvFilePath}))
|
||||
for indx, user := range users {
|
||||
|
|
|
|||
|
|
@ -605,27 +605,24 @@ func (u *UserHandler) deleteMatchingFleetUser(ctx context.Context, scimUser *fle
|
|||
return nil
|
||||
}
|
||||
|
||||
// Check if user is a global admin - if so, ensure we're not deleting the last one
|
||||
if fleetUser.GlobalRole != nil && *fleetUser.GlobalRole == fleet.RoleAdmin {
|
||||
count, err := u.ds.CountGlobalAdmins(ctx)
|
||||
if err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "count global admins")
|
||||
}
|
||||
|
||||
if count <= 1 {
|
||||
u.logger.WarnContext(ctx, "cannot delete last global admin via SCIM",
|
||||
"user_id", fleetUser.ID, "email", fleetUser.Email)
|
||||
return ctxerr.New(ctx, "cannot delete last global admin")
|
||||
}
|
||||
}
|
||||
|
||||
u.logger.InfoContext(ctx, "deleting fleet user via SCIM deletion",
|
||||
"user_id", fleetUser.ID, "email", fleetUser.Email)
|
||||
|
||||
// TODO: Ideally this should go through a Users service/module instead of directly accessing
|
||||
// the datastore. We're in the SCIM domain but accessing the Users datastore which belongs
|
||||
// to the Users domain. This would require a larger refactor to introduce a Users module.
|
||||
if err := u.ds.DeleteUser(ctx, fleetUser.ID); err != nil {
|
||||
|
||||
// Use atomic check+delete to prevent TOCTOU race when deleting the last admin.
|
||||
if fleetUser.GlobalRole != nil && *fleetUser.GlobalRole == fleet.RoleAdmin {
|
||||
if err := u.ds.DeleteUserIfNotLastAdmin(ctx, fleetUser.ID); err != nil {
|
||||
if errors.Is(err, fleet.ErrLastGlobalAdmin) {
|
||||
u.logger.WarnContext(ctx, "cannot delete last global admin via SCIM",
|
||||
"user_id", fleetUser.ID, "email", fleetUser.Email)
|
||||
return ctxerr.New(ctx, "cannot delete last global admin")
|
||||
}
|
||||
return ctxerr.Wrap(ctx, err, "delete fleet admin user")
|
||||
}
|
||||
} else if err := u.ds.DeleteUser(ctx, fleetUser.ID); err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "delete fleet user")
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -241,8 +241,8 @@ func TestDeleteMatchingFleetUser(t *testing.T) {
|
|||
mocks.ds.UserByEmailFunc = func(ctx context.Context, email string) (*fleet.User, error) {
|
||||
return fleetUser, nil
|
||||
}
|
||||
mocks.ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 1, nil // Only 1 admin
|
||||
mocks.ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
|
||||
handler := mocks.newTestHandler()
|
||||
|
|
@ -253,7 +253,7 @@ func TestDeleteMatchingFleetUser(t *testing.T) {
|
|||
assert.Contains(t, err.Error(), "cannot delete last global admin")
|
||||
|
||||
assert.True(t, mocks.ds.UserByEmailFuncInvoked)
|
||||
assert.True(t, mocks.ds.CountGlobalAdminsFuncInvoked)
|
||||
assert.True(t, mocks.ds.DeleteUserIfNotLastAdminFuncInvoked)
|
||||
assert.False(t, mocks.ds.DeleteUserFuncInvoked)
|
||||
})
|
||||
|
||||
|
|
@ -268,10 +268,7 @@ func TestDeleteMatchingFleetUser(t *testing.T) {
|
|||
mocks.ds.UserByEmailFunc = func(ctx context.Context, email string) (*fleet.User, error) {
|
||||
return fleetUser, nil
|
||||
}
|
||||
mocks.ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 3, nil // Multiple admins
|
||||
}
|
||||
mocks.ds.DeleteUserFunc = func(ctx context.Context, id uint) error {
|
||||
mocks.ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return nil
|
||||
}
|
||||
mocks.svc.NewActivityFunc = func(ctx context.Context, user *fleet.User, activity fleet.ActivityDetails) error {
|
||||
|
|
@ -284,7 +281,7 @@ func TestDeleteMatchingFleetUser(t *testing.T) {
|
|||
err := handler.deleteMatchingFleetUser(t.Context(), scimUser)
|
||||
require.NoError(t, err)
|
||||
|
||||
assert.True(t, mocks.ds.DeleteUserFuncInvoked)
|
||||
assert.True(t, mocks.ds.DeleteUserIfNotLastAdminFuncInvoked)
|
||||
})
|
||||
|
||||
t.Run("matches on scim_user_emails when userName is not email", func(t *testing.T) {
|
||||
|
|
@ -418,8 +415,8 @@ func TestUserHandlerDelete(t *testing.T) {
|
|||
return fleetUser, nil
|
||||
}
|
||||
// Last admin - Fleet user deletion will fail
|
||||
mocks.ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 1, nil
|
||||
mocks.ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
// SCIM user deletion should still succeed
|
||||
mocks.ds.DeleteScimUserFunc = func(ctx context.Context, id uint) error {
|
||||
|
|
|
|||
|
|
@ -396,13 +396,65 @@ func (ds *Datastore) DeleteUser(ctx context.Context, id uint) error {
|
|||
return ds.deleteEntity(ctx, usersTable, id)
|
||||
}
|
||||
|
||||
func (ds *Datastore) CountGlobalAdmins(ctx context.Context) (int, error) {
|
||||
var count int
|
||||
err := sqlx.GetContext(ctx, ds.writer(ctx), &count, `SELECT COUNT(*) FROM users WHERE global_role = 'admin'`)
|
||||
if err != nil {
|
||||
return 0, ctxerr.Wrap(ctx, err, "count global admins")
|
||||
}
|
||||
return count, nil
|
||||
// DeleteUserIfNotLastAdmin atomically checks that the user being deleted is not the
|
||||
// last global admin before deleting. It uses SELECT ... FOR UPDATE to prevent concurrent
|
||||
// requests from bypassing the check (TOCTOU race condition).
|
||||
func (ds *Datastore) DeleteUserIfNotLastAdmin(ctx context.Context, id uint) error {
|
||||
return ds.withTx(ctx, func(tx sqlx.ExtContext) error {
|
||||
// Lock the admin rows to prevent concurrent modifications.
|
||||
var count int
|
||||
if err := sqlx.GetContext(ctx, tx, &count,
|
||||
`SELECT COUNT(*) FROM users WHERE global_role = 'admin' FOR UPDATE`); err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "count global admins for delete")
|
||||
}
|
||||
if count <= 1 {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
|
||||
// Transfer user data to deleted_users table for audit/activity purposes.
|
||||
stmt := `
|
||||
INSERT INTO users_deleted (id, name, email)
|
||||
SELECT u.id, u.name, u.email
|
||||
FROM users AS u
|
||||
WHERE u.id = ?
|
||||
ON DUPLICATE KEY UPDATE
|
||||
name = u.name,
|
||||
email = u.email`
|
||||
if _, err := tx.ExecContext(ctx, stmt, id); err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "populate users_deleted entry")
|
||||
}
|
||||
|
||||
res, err := tx.ExecContext(ctx, `DELETE FROM users WHERE id = ?`, id)
|
||||
if err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "delete user")
|
||||
}
|
||||
rows, _ := res.RowsAffected()
|
||||
if rows != 1 {
|
||||
return ctxerr.Wrap(ctx, notFound("User").WithID(id))
|
||||
}
|
||||
return nil
|
||||
})
|
||||
}
|
||||
|
||||
// SaveUserIfNotLastAdmin atomically checks that there's more than one admin
|
||||
// before saving the user. Returns ErrLastGlobalAdmin if there's only one last global admin.
|
||||
//
|
||||
// It uses SELECT ... FOR UPDATE to prevent concurrent requests from bypassing
|
||||
// the check (TOCTOU race condition).
|
||||
func (ds *Datastore) SaveUserIfNotLastAdmin(ctx context.Context, user *fleet.User) error {
|
||||
return ds.withTx(ctx, func(tx sqlx.ExtContext) error {
|
||||
// Lock the admin rows to prevent concurrent modifications.
|
||||
var count int
|
||||
if err := sqlx.GetContext(ctx, tx, &count,
|
||||
`SELECT COUNT(*) FROM users WHERE global_role = 'admin' FOR UPDATE`); err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "count global admins for save")
|
||||
}
|
||||
if count <= 1 {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
|
||||
return saveUserDB(ctx, tx, user)
|
||||
})
|
||||
}
|
||||
|
||||
func tableRowsCount(ctx context.Context, db sqlx.QueryerContext, tableName string) (int, error) {
|
||||
|
|
|
|||
|
|
@ -81,8 +81,13 @@ type Datastore interface {
|
|||
SaveUsers(ctx context.Context, users []*User) error
|
||||
// DeleteUser permanently deletes the user identified by the provided ID.
|
||||
DeleteUser(ctx context.Context, id uint) error
|
||||
// CountGlobalAdmins returns the count of users with the global admin role.
|
||||
CountGlobalAdmins(ctx context.Context) (int, error)
|
||||
// DeleteUserIfNotLastAdmin atomically checks that the user being deleted
|
||||
// is not the last global admin before deleting. Returns ErrLastGlobalAdmin
|
||||
// if the user is the last global admin.
|
||||
DeleteUserIfNotLastAdmin(ctx context.Context, id uint) error
|
||||
// SaveUserIfNotLastAdmin atomically checks that there's more than one admin
|
||||
// before saving the user. Returns ErrLastGlobalAdmin if there's only one last global admin.
|
||||
SaveUserIfNotLastAdmin(ctx context.Context, user *User) error
|
||||
// PendingEmailChange creates a record with a pending email change for a user identified by uid. The change record
|
||||
// is keyed by a unique token. The token is emailed to the user with a link that they can use to confirm the change.
|
||||
PendingEmailChange(ctx context.Context, userID uint, newEmail, token string) error
|
||||
|
|
|
|||
|
|
@ -12,6 +12,9 @@ import (
|
|||
"golang.org/x/crypto/bcrypt"
|
||||
)
|
||||
|
||||
// ErrLastGlobalAdmin is returned when an operation would remove the last global admin.
|
||||
var ErrLastGlobalAdmin = errors.New("cannot remove the last global admin")
|
||||
|
||||
// UserSummary contains the minimal user fields.
|
||||
type UserSummary struct {
|
||||
ID uint `db:"id"`
|
||||
|
|
@ -362,6 +365,9 @@ func (u *User) ValidatePassword(password string) error {
|
|||
}
|
||||
|
||||
func (u *User) SetPassword(plaintext string, keySize, cost int) error {
|
||||
if u.SSOEnabled {
|
||||
return errors.New("set password for single sign on user not allowed")
|
||||
}
|
||||
if err := ValidatePasswordRequirements(plaintext); err != nil {
|
||||
return err
|
||||
}
|
||||
|
|
|
|||
|
|
@ -81,7 +81,9 @@ type SaveUsersFunc func(ctx context.Context, users []*fleet.User) error
|
|||
|
||||
type DeleteUserFunc func(ctx context.Context, id uint) error
|
||||
|
||||
type CountGlobalAdminsFunc func(ctx context.Context) (int, error)
|
||||
type DeleteUserIfNotLastAdminFunc func(ctx context.Context, id uint) error
|
||||
|
||||
type SaveUserIfNotLastAdminFunc func(ctx context.Context, user *fleet.User) error
|
||||
|
||||
type PendingEmailChangeFunc func(ctx context.Context, userID uint, newEmail string, token string) error
|
||||
|
||||
|
|
@ -1931,8 +1933,11 @@ type DataStore struct {
|
|||
DeleteUserFunc DeleteUserFunc
|
||||
DeleteUserFuncInvoked bool
|
||||
|
||||
CountGlobalAdminsFunc CountGlobalAdminsFunc
|
||||
CountGlobalAdminsFuncInvoked bool
|
||||
DeleteUserIfNotLastAdminFunc DeleteUserIfNotLastAdminFunc
|
||||
DeleteUserIfNotLastAdminFuncInvoked bool
|
||||
|
||||
SaveUserIfNotLastAdminFunc SaveUserIfNotLastAdminFunc
|
||||
SaveUserIfNotLastAdminFuncInvoked bool
|
||||
|
||||
PendingEmailChangeFunc PendingEmailChangeFunc
|
||||
PendingEmailChangeFuncInvoked bool
|
||||
|
|
@ -4780,11 +4785,18 @@ func (s *DataStore) DeleteUser(ctx context.Context, id uint) error {
|
|||
return s.DeleteUserFunc(ctx, id)
|
||||
}
|
||||
|
||||
func (s *DataStore) CountGlobalAdmins(ctx context.Context) (int, error) {
|
||||
func (s *DataStore) DeleteUserIfNotLastAdmin(ctx context.Context, id uint) error {
|
||||
s.mu.Lock()
|
||||
s.CountGlobalAdminsFuncInvoked = true
|
||||
s.DeleteUserIfNotLastAdminFuncInvoked = true
|
||||
s.mu.Unlock()
|
||||
return s.CountGlobalAdminsFunc(ctx)
|
||||
return s.DeleteUserIfNotLastAdminFunc(ctx, id)
|
||||
}
|
||||
|
||||
func (s *DataStore) SaveUserIfNotLastAdmin(ctx context.Context, user *fleet.User) error {
|
||||
s.mu.Lock()
|
||||
s.SaveUserIfNotLastAdminFuncInvoked = true
|
||||
s.mu.Unlock()
|
||||
return s.SaveUserIfNotLastAdminFunc(ctx, user)
|
||||
}
|
||||
|
||||
func (s *DataStore) PendingEmailChange(ctx context.Context, userID uint, newEmail string, token string) error {
|
||||
|
|
|
|||
|
|
@ -303,9 +303,8 @@ func (s *integrationSSOTestSuite) TestSSOLoginDisallowedWithPremiumRoles() {
|
|||
Email: "sso_user2@example.com",
|
||||
GlobalRole: ptr.String(role),
|
||||
SSOEnabled: true,
|
||||
Password: []byte{},
|
||||
}
|
||||
password := test.GoodPassword
|
||||
require.NoError(t, u.SetPassword(password, 10, 10))
|
||||
u, err := s.ds.NewUser(t.Context(), u)
|
||||
require.NoError(t, err)
|
||||
|
||||
|
|
@ -446,9 +445,8 @@ func (s *integrationSSOTestSuite) TestSSOLoginWithMetadata() {
|
|||
Email: "sso_user2@example.com",
|
||||
GlobalRole: ptr.String(fleet.RoleObserver),
|
||||
SSOEnabled: true,
|
||||
Password: []byte{},
|
||||
}
|
||||
password := test.GoodPassword
|
||||
require.NoError(t, u.SetPassword(password, 10, 10))
|
||||
_, _ = s.ds.NewUser(context.Background(), u)
|
||||
|
||||
body := s.LoginSSOUser("sso_user2", "user123#")
|
||||
|
|
@ -487,9 +485,8 @@ func (s *integrationSSOTestSuite) TestSSOLoginNoEntityID() {
|
|||
Email: "sso_user2@example.com",
|
||||
GlobalRole: ptr.String(fleet.RoleObserver),
|
||||
SSOEnabled: true,
|
||||
Password: []byte{},
|
||||
}
|
||||
password := test.GoodPassword
|
||||
require.NoError(t, u.SetPassword(password, 10, 10))
|
||||
_, _ = s.ds.NewUser(context.Background(), u)
|
||||
|
||||
body := s.LoginSSOUser("sso_user2", "user123#")
|
||||
|
|
@ -526,9 +523,8 @@ func (s *integrationSSOTestSuite) TestSSOLoginSAMLResponseTampered() {
|
|||
Email: "sso_user2@example.com",
|
||||
GlobalRole: ptr.String(fleet.RoleObserver),
|
||||
SSOEnabled: true,
|
||||
Password: []byte{},
|
||||
}
|
||||
password := test.GoodPassword
|
||||
require.NoError(t, u.SetPassword(password, 10, 10))
|
||||
_, _ = s.ds.NewUser(context.Background(), u)
|
||||
|
||||
var (
|
||||
|
|
|
|||
|
|
@ -532,6 +532,8 @@ func (svc *Service) ModifyUser(ctx context.Context, userID uint, p fleet.UserPay
|
|||
|
||||
currentUser := authz.UserFromContext(ctx)
|
||||
|
||||
var isGlobalAdminDemotion bool
|
||||
|
||||
if p.GlobalRole != nil && *p.GlobalRole != "" {
|
||||
if currentUser.GlobalRole == nil {
|
||||
return nil, authz.ForbiddenWithInternal(
|
||||
|
|
@ -544,32 +546,15 @@ func (svc *Service) ModifyUser(ctx context.Context, userID uint, p fleet.UserPay
|
|||
return nil, fleet.NewInvalidArgumentError("teams", "may not be specified with global_role")
|
||||
}
|
||||
|
||||
// Check if demoting from admin - ensure we're not demoting the last one
|
||||
if user.GlobalRole != nil && *user.GlobalRole == fleet.RoleAdmin {
|
||||
if *p.GlobalRole != fleet.RoleAdmin {
|
||||
count, err := svc.ds.CountGlobalAdmins(ctx)
|
||||
if err != nil {
|
||||
return nil, ctxerr.Wrap(ctx, err, "count global admins")
|
||||
}
|
||||
if count <= 1 {
|
||||
return nil, fleet.NewInvalidArgumentError("global_role", "cannot demote the last global admin")
|
||||
}
|
||||
}
|
||||
}
|
||||
// Track whether this is a demotion from global admin so we can
|
||||
// use an atomic check+save later to prevent TOCTOU races.
|
||||
isGlobalAdminDemotion = user.GlobalRole != nil && *user.GlobalRole == fleet.RoleAdmin && *p.GlobalRole != fleet.RoleAdmin
|
||||
|
||||
user.GlobalRole = p.GlobalRole
|
||||
user.Teams = []fleet.UserTeam{}
|
||||
} else if p.Teams != nil {
|
||||
// Check if demoting from admin by assigning teams (which removes global role)
|
||||
if user.GlobalRole != nil && *user.GlobalRole == fleet.RoleAdmin {
|
||||
count, err := svc.ds.CountGlobalAdmins(ctx)
|
||||
if err != nil {
|
||||
return nil, ctxerr.Wrap(ctx, err, "count global admins")
|
||||
}
|
||||
if count <= 1 {
|
||||
return nil, fleet.NewInvalidArgumentError("teams", "cannot demote the last global admin")
|
||||
}
|
||||
}
|
||||
// Track whether this is a demotion from global admin by assigning teams.
|
||||
isGlobalAdminDemotion = user.GlobalRole != nil && *user.GlobalRole == fleet.RoleAdmin
|
||||
|
||||
if !isAdminOfTheModifiedTeams(currentUser, user.Teams, *p.Teams) {
|
||||
return nil, authz.ForbiddenWithInternal(
|
||||
|
|
@ -581,10 +566,35 @@ func (svc *Service) ModifyUser(ctx context.Context, userID uint, p fleet.UserPay
|
|||
user.GlobalRole = nil
|
||||
}
|
||||
|
||||
if p.NewPassword != nil {
|
||||
switch {
|
||||
case isGlobalAdminDemotion:
|
||||
// Use atomic check+save to prevent TOCTOU race when demoting the last admin.
|
||||
// We must set the password before saving if a new password was also provided.
|
||||
if p.NewPassword != nil {
|
||||
if err = user.SetPassword(*p.NewPassword, svc.config.Auth.SaltKeySize, svc.config.Auth.BcryptCost); err != nil {
|
||||
return nil, ctxerr.Wrap(ctx, err, "setting new password")
|
||||
}
|
||||
}
|
||||
err = svc.ds.SaveUserIfNotLastAdmin(ctx, user)
|
||||
if errors.Is(err, fleet.ErrLastGlobalAdmin) {
|
||||
if p.GlobalRole != nil {
|
||||
return nil, fleet.NewInvalidArgumentError("global_role", "cannot demote the last global admin")
|
||||
}
|
||||
return nil, fleet.NewInvalidArgumentError("teams", "cannot demote the last global admin")
|
||||
}
|
||||
if err == nil && p.NewPassword != nil {
|
||||
// Clean up password reset requests and sessions like setNewPassword does.
|
||||
if err := svc.ds.DeletePasswordResetRequestsForUser(ctx, user.ID); err != nil {
|
||||
return nil, ctxerr.Wrap(ctx, err, "deleting password reset requests after password change")
|
||||
}
|
||||
if err := svc.ds.DestroyAllSessionsForUser(ctx, user.ID); err != nil {
|
||||
return nil, ctxerr.Wrap(ctx, err, "destroying sessions after password change")
|
||||
}
|
||||
}
|
||||
case p.NewPassword != nil:
|
||||
// setNewPassword takes care of calling saveUser
|
||||
err = svc.setNewPassword(ctx, user, *p.NewPassword, true)
|
||||
} else {
|
||||
default:
|
||||
err = svc.saveUser(ctx, user)
|
||||
}
|
||||
if err != nil {
|
||||
|
|
@ -639,18 +649,15 @@ func (svc *Service) DeleteUser(ctx context.Context, id uint) (*fleet.User, error
|
|||
return nil, err
|
||||
}
|
||||
|
||||
// prevent deleting admin if they are the last one
|
||||
// Atomically check that we're not deleting the last global admin before deleting.
|
||||
if user.GlobalRole != nil && *user.GlobalRole == fleet.RoleAdmin {
|
||||
count, err := svc.ds.CountGlobalAdmins(ctx)
|
||||
if err != nil {
|
||||
return nil, ctxerr.Wrap(ctx, err, "count global admins")
|
||||
if err := svc.ds.DeleteUserIfNotLastAdmin(ctx, id); err != nil {
|
||||
if errors.Is(err, fleet.ErrLastGlobalAdmin) {
|
||||
return nil, fleet.NewInvalidArgumentError("id", "cannot delete the last global admin")
|
||||
}
|
||||
return nil, err
|
||||
}
|
||||
if count <= 1 {
|
||||
return nil, fleet.NewInvalidArgumentError("id", "cannot delete the last global admin")
|
||||
}
|
||||
}
|
||||
|
||||
if err := svc.ds.DeleteUser(ctx, id); err != nil {
|
||||
} else if err := svc.ds.DeleteUser(ctx, id); err != nil {
|
||||
return nil, err
|
||||
}
|
||||
|
||||
|
|
@ -1106,9 +1113,6 @@ func (svc *Service) setNewPassword(ctx context.Context, user *fleet.User, passwo
|
|||
if err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "setting new password")
|
||||
}
|
||||
if user.SSOEnabled {
|
||||
return ctxerr.New(ctx, "set password for single sign on user not allowed")
|
||||
}
|
||||
err = svc.saveUser(ctx, user)
|
||||
if err != nil {
|
||||
return ctxerr.Wrap(ctx, err, "saving changed password")
|
||||
|
|
|
|||
|
|
@ -98,8 +98,11 @@ func TestUserAuth(t *testing.T) {
|
|||
ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) {
|
||||
return &fleet.AppConfig{}, nil
|
||||
}
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 2, nil // Return 2 to allow operations that check for last admin
|
||||
ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return nil // Allow delete (multiple admins exist)
|
||||
}
|
||||
ds.SaveUserIfNotLastAdminFunc = func(ctx context.Context, user *fleet.User) error {
|
||||
return nil // Allow save (multiple admins exist)
|
||||
}
|
||||
|
||||
testCases := []struct {
|
||||
|
|
@ -1507,8 +1510,8 @@ func TestDeleteUserLastAdminProtection(t *testing.T) {
|
|||
ds.UserByIDFunc = func(ctx context.Context, id uint) (*fleet.User, error) {
|
||||
return adminUser, nil
|
||||
}
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 1, nil
|
||||
ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
|
||||
_, err := svc.DeleteUser(ctx, adminUser.ID)
|
||||
|
|
@ -1525,10 +1528,7 @@ func TestDeleteUserLastAdminProtection(t *testing.T) {
|
|||
ds.UserByIDFunc = func(ctx context.Context, id uint) (*fleet.User, error) {
|
||||
return adminUser, nil
|
||||
}
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 3, nil
|
||||
}
|
||||
ds.DeleteUserFunc = func(ctx context.Context, id uint) error {
|
||||
ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return nil
|
||||
}
|
||||
ds.AppConfigFunc = func(ctx context.Context) (*fleet.AppConfig, error) {
|
||||
|
|
@ -1536,7 +1536,7 @@ func TestDeleteUserLastAdminProtection(t *testing.T) {
|
|||
}
|
||||
_, err := svc.DeleteUser(ctx, adminUser.ID)
|
||||
require.NoError(t, err)
|
||||
assert.True(t, ds.DeleteUserFuncInvoked)
|
||||
assert.True(t, ds.DeleteUserIfNotLastAdminFuncInvoked)
|
||||
})
|
||||
|
||||
t.Run("prevents deleting last global admin even if api-only user", func(t *testing.T) {
|
||||
|
|
@ -1551,8 +1551,8 @@ func TestDeleteUserLastAdminProtection(t *testing.T) {
|
|||
ds.UserByIDFunc = func(ctx context.Context, id uint) (*fleet.User, error) {
|
||||
return apiOnlyAdmin, nil
|
||||
}
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 1, nil
|
||||
ds.DeleteUserIfNotLastAdminFunc = func(ctx context.Context, id uint) error {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
|
||||
_, err := svc.DeleteUser(ctx, apiOnlyAdmin.ID)
|
||||
|
|
@ -1583,7 +1583,7 @@ func TestDeleteUserLastAdminProtection(t *testing.T) {
|
|||
_, err := svc.DeleteUser(ctx, maintainerUser.ID)
|
||||
require.NoError(t, err)
|
||||
assert.True(t, ds.DeleteUserFuncInvoked)
|
||||
assert.False(t, ds.CountGlobalAdminsFuncInvoked)
|
||||
assert.False(t, ds.DeleteUserIfNotLastAdminFuncInvoked)
|
||||
})
|
||||
}
|
||||
|
||||
|
|
@ -1602,8 +1602,8 @@ func TestModifyUserLastAdminProtection(t *testing.T) {
|
|||
adminUser := newAdminTestUser(nil)
|
||||
ds, svc, ctx := setupAdminTestContext(t, adminUser)
|
||||
setupModifyUserMocks(ds, adminUser)
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 1, nil
|
||||
ds.SaveUserIfNotLastAdminFunc = func(ctx context.Context, u *fleet.User) error {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
|
||||
_, err := svc.ModifyUser(ctx, adminUser.ID, fleet.UserPayload{
|
||||
|
|
@ -1619,8 +1619,8 @@ func TestModifyUserLastAdminProtection(t *testing.T) {
|
|||
adminUser := newAdminTestUser(nil)
|
||||
ds, svc, ctx := setupAdminTestContext(t, adminUser)
|
||||
setupModifyUserMocks(ds, adminUser)
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 1, nil
|
||||
ds.SaveUserIfNotLastAdminFunc = func(ctx context.Context, u *fleet.User) error {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
ds.TeamsSummaryFunc = func(ctx context.Context) ([]*fleet.TeamSummary, error) {
|
||||
return []*fleet.TeamSummary{{ID: 1}}, nil
|
||||
|
|
@ -1640,10 +1640,7 @@ func TestModifyUserLastAdminProtection(t *testing.T) {
|
|||
adminUser := newAdminTestUser(nil)
|
||||
ds, svc, ctx := setupAdminTestContext(t, adminUser)
|
||||
setupModifyUserMocks(ds, adminUser)
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 3, nil
|
||||
}
|
||||
ds.SaveUserFunc = func(ctx context.Context, u *fleet.User) error {
|
||||
ds.SaveUserIfNotLastAdminFunc = func(ctx context.Context, u *fleet.User) error {
|
||||
return nil
|
||||
}
|
||||
_, err := svc.ModifyUser(ctx, adminUser.ID, fleet.UserPayload{
|
||||
|
|
@ -1663,8 +1660,8 @@ func TestModifyUserLastAdminProtection(t *testing.T) {
|
|||
GlobalRole: ptr.String(fleet.RoleAdmin),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
// CountGlobalAdmins should NOT have been called since role isn't changing
|
||||
assert.False(t, ds.CountGlobalAdminsFuncInvoked)
|
||||
// SaveUserIfNotLastAdmin should NOT have been called since role isn't changing
|
||||
assert.False(t, ds.SaveUserIfNotLastAdminFuncInvoked)
|
||||
})
|
||||
|
||||
t.Run("prevents demoting last global admin even if api-only user", func(t *testing.T) {
|
||||
|
|
@ -1677,8 +1674,8 @@ func TestModifyUserLastAdminProtection(t *testing.T) {
|
|||
apiOnly: true,
|
||||
})
|
||||
setupModifyUserMocks(ds, apiOnlyAdmin)
|
||||
ds.CountGlobalAdminsFunc = func(ctx context.Context) (int, error) {
|
||||
return 1, nil
|
||||
ds.SaveUserIfNotLastAdminFunc = func(ctx context.Context, u *fleet.User) error {
|
||||
return fleet.ErrLastGlobalAdmin
|
||||
}
|
||||
|
||||
_, err := svc.ModifyUser(ctx, apiOnlyAdmin.ID, fleet.UserPayload{
|
||||
|
|
|
|||
Loading…
Reference in a new issue