Validate the user still exists during account reset

Prior to this, any reset links created before a user was deleted would still be valid and when used would recreate the deleted user. With this change, the user is validated to still exist prior to honoring the reset request and if the user no longer exists any remaining reset tokens are deleted.
gravitational · Jan 4, 2024 · 72cbd3e · 72cbd3e
1 parent d22eca1
commit 72cbd3e
Show file tree

Hide file tree

Showing 2 changed files with 76 additions and 25 deletions.
diff --git a/lib/auth/password.go b/lib/auth/password.go
@@ -306,16 +306,27 @@ func (a *Server) changeUserAuthentication(ctx context.Context, req *proto.Change
 		return nil, trace.BadParameter("expired token")
 	}
 
-	err = a.changeUserSecondFactor(ctx, req, token)
-	if err != nil {
+	// Check if the user still exists before potentially recreating the user
+	// below. If the user was deleted, do NOT honor the request and delete any
+	// other tokens associated with the user.
+	if _, err := a.GetUser(token.GetUser(), false); err != nil {
+		if trace.IsNotFound(err) {
+			// Delete any remaining tokens for users that no longer exist.
+			if err := a.deleteUserTokens(ctx, token.GetUser()); err != nil {
+				return nil, trace.Wrap(err)
+			}
+		}
+		return nil, trace.Wrap(err)
+	}
+
+	if err := a.changeUserSecondFactor(ctx, req, token); err != nil {
 		return nil, trace.Wrap(err)
 	}
 
 	username := token.GetUser()
 	// Delete this token first to minimize the chances
 	// of partially updated user with still valid token.
-	err = a.deleteUserTokens(ctx, username)
-	if err != nil {
+	if err := a.deleteUserTokens(ctx, username); err != nil {
 		return nil, trace.Wrap(err)
 	}
 

diff --git a/lib/auth/tls_test.go b/lib/auth/tls_test.go
@@ -2846,7 +2846,10 @@ func TestChangeUserAuthenticationSettings(t *testing.T) {
 
 	authPreference, err := types.NewAuthPreference(types.AuthPreferenceSpecV2{
 		Type:         constants.Local,
-		SecondFactor: constants.SecondFactorOTP,
+		SecondFactor: constants.SecondFactorWebauthn,
+		Webauthn: &types.Webauthn{
+			RPID: "localhost",
+		},
 	})
 	require.NoError(t, err)
 
@@ -2858,32 +2861,69 @@ func TestChangeUserAuthenticationSettings(t *testing.T) {
 	clt, err := testSrv.NewClient(TestAdmin())
 	require.NoError(t, err)
 
-	_, _, err = CreateUserAndRole(clt, username, []string{"role1"}, nil)
-	require.NoError(t, err)
+	t.Run("Reset works when user exists", func(t *testing.T) {
+		_, _, err = CreateUserAndRole(clt, username, []string{"role1"}, nil)
+		require.NoError(t, err)
 
-	token, err := testSrv.Auth().CreateResetPasswordToken(ctx, CreateUserTokenRequest{
-		Name: username,
-		TTL:  time.Hour,
-	})
-	require.NoError(t, err)
+		token, err := testSrv.Auth().CreateResetPasswordToken(ctx, CreateUserTokenRequest{
+			Name: username,
+			TTL:  time.Hour,
+		})
+		require.NoError(t, err)
 
-	res, err := testSrv.Auth().CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{
-		TokenID:    token.GetName(),
-		DeviceType: proto.DeviceType_DEVICE_TYPE_TOTP,
+		res, err := testSrv.Auth().CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{
+			TokenID:    token.GetName(),
+			DeviceType: proto.DeviceType_DEVICE_TYPE_WEBAUTHN,
+		})
+		require.NoError(t, err)
+
+		_, registerSolved, err := NewTestDeviceFromChallenge(res)
+		require.NoError(t, err)
+
+		_, err = testSrv.Auth().ChangeUserAuthentication(ctx, &proto.ChangeUserAuthenticationRequest{
+			TokenID:                token.GetName(),
+			NewPassword:            []byte("qweqweqwe"),
+			NewMFARegisterResponse: registerSolved,
+		})
+		require.NoError(t, err)
 	})
-	require.NoError(t, err)
 
-	otpToken, err := totp.GenerateCode(res.GetTOTP().GetSecret(), testSrv.Clock().Now())
-	require.NoError(t, err)
+	t.Run("Reset link not allowed when user does not exist", func(t *testing.T) {
+		var tokenID string
+		var resp *proto.MFARegisterResponse
+		for i := 0; i < 5; i++ {
+			token, err := testSrv.Auth().CreateResetPasswordToken(ctx, CreateUserTokenRequest{
+				Name: username,
+				TTL:  time.Hour,
+			})
+			require.NoError(t, err)
+
+			res, err := testSrv.Auth().CreateRegisterChallenge(ctx, &proto.CreateRegisterChallengeRequest{
+				TokenID:    token.GetName(),
+				DeviceType: proto.DeviceType_DEVICE_TYPE_WEBAUTHN,
+			})
+			require.NoError(t, err)
+
+			_, registerSolved, err := NewTestDeviceFromChallenge(res)
+			require.NoError(t, err)
 
-	_, err = testSrv.Auth().ChangeUserAuthentication(ctx, &proto.ChangeUserAuthenticationRequest{
-		TokenID:     token.GetName(),
-		NewPassword: []byte("qweqweqwe"),
-		NewMFARegisterResponse: &proto.MFARegisterResponse{Response: &proto.MFARegisterResponse_TOTP{
-			TOTP: &proto.TOTPRegisterResponse{Code: otpToken},
-		}},
+			tokenID = token.GetName()
+			resp = registerSolved
+		}
+
+		require.NoError(t, testSrv.Auth().DeleteUser(ctx, username))
+
+		_, err = testSrv.Auth().ChangeUserAuthentication(ctx, &proto.ChangeUserAuthenticationRequest{
+			TokenID:                tokenID,
+			NewPassword:            []byte("qweqweqwe"),
+			NewMFARegisterResponse: resp,
+		})
+		require.Error(t, err)
+
+		tokens, err := testSrv.Auth().GetUserTokens(ctx)
+		require.NoError(t, err)
+		require.Empty(t, tokens)
 	})
-	require.NoError(t, err)
 }
 
 // TestLoginNoLocalAuth makes sure that logins for local accounts can not be