From 4baf9f612403d501d0ef07bce422b8243455f891 Mon Sep 17 00:00:00 2001 From: joerger Date: Tue, 15 Oct 2024 11:39:47 -0700 Subject: [PATCH] Prevent deleting last passwordless device with no password. --- lib/auth/auth.go | 4 ++-- lib/auth/grpcserver_test.go | 44 +++++++++++++++++++------------------ 2 files changed, 25 insertions(+), 23 deletions(-) diff --git a/lib/auth/auth.go b/lib/auth/auth.go index ea5fb1d5954ae..95c6d57dfcec7 100644 --- a/lib/auth/auth.go +++ b/lib/auth/auth.go @@ -3888,11 +3888,11 @@ func (a *Server) deleteMFADeviceSafely(ctx context.Context, user, deviceName str // Check whether the device to delete is the last passwordless device, // and whether deleting it would lockout the user from login. // - // TODO(Joerger): the user may already be locked out from login if a password + // Note: the user may already be locked out from login if a password // is not set and passwordless is disabled. Prevent them from deleting // their last passkey to prevent them from being locked out further, // in the case of passwordless being re-enabled. - if isPasskey(deviceToDelete) && remainingPasskeys == 0 && readOnlyAuthPref.GetAllowPasswordless() { + if isPasskey(deviceToDelete) && remainingPasskeys == 0 { u, err := a.Services.GetUser(ctx, user, false /* withSecrets */) if err != nil { return nil, trace.Wrap(err) diff --git a/lib/auth/grpcserver_test.go b/lib/auth/grpcserver_test.go index 6e85e400f4dd1..58008430aee5c 100644 --- a/lib/auth/grpcserver_test.go +++ b/lib/auth/grpcserver_test.go @@ -464,27 +464,6 @@ func TestDeletingLastPasswordlessDevice(t *testing.T) { ) }, }, - { - // TODO(Joerger): the user may already be locked out from login if a password - // is not set and passwordless is disabled. Prevent them from deleting - // their last passkey to prevent them from being locked out further, - // in the case of passwordless being re-enabled. - name: "OK other MFAs, but no password set, passwordless is off", - setup: func(t *testing.T, _ string, userClient *authclient.Client, pwdlessDev *TestDevice) { - // Register a non-passwordless device without adding a password. - _, err := RegisterTestDevice(ctx, userClient, "another-dev", proto.DeviceType_DEVICE_TYPE_TOTP, pwdlessDev, WithTestDeviceClock(clock)) - require.NoError(t, err, "RegisterTestDevice") - - authPref, err := authServer.GetAuthPreference(ctx) - require.NoError(t, err, "GetAuthPreference") - - // Turn off passwordless authentication. - authPref.SetAllowPasswordless(false) - _, err = authServer.UpsertAuthPreference(ctx, authPref) - require.NoError(t, err, "UpsertAuthPreference") - }, - checkErr: require.NoError, - }, { name: "OK extra passwordless device", setup: func(t *testing.T, username string, userClient *authclient.Client, pwdlessDev *TestDevice) { @@ -560,6 +539,29 @@ func TestDeletingLastPasswordlessDevice(t *testing.T) { ) }, }, + { + name: "NOK other MFAs, but no password set, passwordless is off", + setup: func(t *testing.T, _ string, userClient *authclient.Client, pwdlessDev *TestDevice) { + // Register a non-passwordless device without adding a password. + _, err := RegisterTestDevice(ctx, userClient, "another-dev", proto.DeviceType_DEVICE_TYPE_TOTP, pwdlessDev, WithTestDeviceClock(clock)) + require.NoError(t, err, "RegisterTestDevice") + + authPref, err := authServer.GetAuthPreference(ctx) + require.NoError(t, err, "GetAuthPreference") + + // Turn off passwordless authentication. + authPref.SetAllowPasswordless(false) + _, err = authServer.UpsertAuthPreference(ctx, authPref) + require.NoError(t, err, "UpsertAuthPreference") + }, + checkErr: func(t require.TestingT, err error, _ ...any) { + require.ErrorContains(t, + err, + "cannot delete last passwordless credential for user", + "Unexpected error deleting last passwordless device", + ) + }, + }, } for i, test := range tests {