[PM-24211]: 2FA Send Email Login validation should use AuthRequest.IsValidForAuthentication (#6695)

* fix(two-factor-controller) [PM-24211]: Update send email validation to use auth request's IsValidForAuthentication.

* refactor(login-features) [PM-24211]: Remove Core.LoginFeatures as no longer used; AuthRequest.IsValidForAuthentication should be used for any applicable use cases.

* feat(auth-request) [PM-24211]: Add tests for AuthRequest.IsValidForAuthentication.

* fix(two-factor-controller) [PM-24211]: Branching logic should return on successful send.

* chore(auth-request) [PM-24211]: Remove some old comments (solved-for).

* fix(two-factor-controller) [PM-24211]: Update some comments (clarification/naming).

* fix(two-factor-controller) [PM-24211]: Rephrase a comment (accuracy).

src/Api/Auth/Controllers/TwoFactorController.cs

@@ -9,7 +9,6 @@ using Bit.Api.Models.Response;
 using Bit.Core.Auth.Enums;
 using Bit.Core.Auth.Identity;
 using Bit.Core.Auth.Identity.TokenProviders;
-using Bit.Core.Auth.LoginFeatures.PasswordlessLogin.Interfaces;
 using Bit.Core.Auth.Models.Business.Tokenables;
 using Bit.Core.Auth.Services;
 using Bit.Core.Context;
@@ -35,7 +34,7 @@ public class TwoFactorController : Controller
     private readonly IOrganizationService _organizationService;
     private readonly UserManager<User> _userManager;
     private readonly ICurrentContext _currentContext;
-    private readonly IVerifyAuthRequestCommand _verifyAuthRequestCommand;
+    private readonly IAuthRequestRepository _authRequestRepository;
     private readonly IDuoUniversalTokenService _duoUniversalTokenService;
     private readonly IDataProtectorTokenFactory<TwoFactorAuthenticatorUserVerificationTokenable> _twoFactorAuthenticatorDataProtector;
     private readonly IDataProtectorTokenFactory<SsoEmail2faSessionTokenable> _ssoEmailTwoFactorSessionDataProtector;
@@ -47,7 +46,7 @@ public class TwoFactorController : Controller
         IOrganizationService organizationService,
         UserManager<User> userManager,
         ICurrentContext currentContext,
-        IVerifyAuthRequestCommand verifyAuthRequestCommand,
+        IAuthRequestRepository authRequestRepository,
         IDuoUniversalTokenService duoUniversalConfigService,
         IDataProtectorTokenFactory<TwoFactorAuthenticatorUserVerificationTokenable> twoFactorAuthenticatorDataProtector,
         IDataProtectorTokenFactory<SsoEmail2faSessionTokenable> ssoEmailTwoFactorSessionDataProtector,
@@ -58,7 +57,7 @@ public class TwoFactorController : Controller
         _organizationService = organizationService;
         _userManager = userManager;
         _currentContext = currentContext;
-        _verifyAuthRequestCommand = verifyAuthRequestCommand;
+        _authRequestRepository = authRequestRepository;
         _duoUniversalTokenService = duoUniversalConfigService;
         _twoFactorAuthenticatorDataProtector = twoFactorAuthenticatorDataProtector;
         _ssoEmailTwoFactorSessionDataProtector = ssoEmailTwoFactorSessionDataProtector;
@@ -350,14 +349,15 @@ public class TwoFactorController : Controller
 
         if (user != null)
         {
-            // Check if 2FA email is from Passwordless.
+            // Check if 2FA email is from a device approval ("Log in with device") scenario.
             if (!string.IsNullOrEmpty(requestModel.AuthRequestAccessCode))
             {
-                if (await _verifyAuthRequestCommand
+                var authRequest = await _authRequestRepository.GetByIdAsync(new Guid(requestModel.AuthRequestId));
-                        .VerifyAuthRequestAsync(new Guid(requestModel.AuthRequestId),
+                if (authRequest != null &&
-                            requestModel.AuthRequestAccessCode))
+                    authRequest.IsValidForAuthentication(user.Id, requestModel.AuthRequestAccessCode))
                 {
                     await _twoFactorEmailService.SendTwoFactorEmailAsync(user);
+                    return;
                 }
             }
             else if (!string.IsNullOrEmpty(requestModel.SsoEmail2FaSessionToken))

src/Core/Auth/Entities/AuthRequest.cs

@@ -49,11 +49,9 @@ public class AuthRequest : ITableObject<Guid>
 
     public bool IsExpired()
     {
-        // TODO: PM-24252 - consider using TimeProvider for better mocking in tests
         return GetExpirationDate() < DateTime.UtcNow;
     }
 
-    // TODO: PM-24252 - this probably belongs in a service.
     public bool IsValidForAuthentication(Guid userId,
         string password)
     {

src/Core/Auth/LoginFeatures/LoginServiceCollectionExtensions.cs

@@ -1,14 +0,0 @@
-using Bit.Core.Auth.LoginFeatures.PasswordlessLogin;
-using Bit.Core.Auth.LoginFeatures.PasswordlessLogin.Interfaces;
-using Microsoft.Extensions.DependencyInjection;
-
-namespace Bit.Core.Auth.LoginFeatures;
-
-public static class LoginServiceCollectionExtensions
-{
-    public static void AddLoginServices(this IServiceCollection services)
-    {
-        services.AddScoped<IVerifyAuthRequestCommand, VerifyAuthRequestCommand>();
-    }
-}
-

src/Core/Auth/LoginFeatures/PasswordlessLogin/Interfaces/IVerifyAuthRequest.cs

@@ -1,6 +0,0 @@
-namespace Bit.Core.Auth.LoginFeatures.PasswordlessLogin.Interfaces;
-
-public interface IVerifyAuthRequestCommand
-{
-    Task<bool> VerifyAuthRequestAsync(Guid authRequestId, string accessCode);
-}

src/Core/Auth/LoginFeatures/PasswordlessLogin/VerifyAuthRequest.cs

@@ -1,25 +0,0 @@
-using Bit.Core.Auth.LoginFeatures.PasswordlessLogin.Interfaces;
-using Bit.Core.Repositories;
-using Bit.Core.Utilities;
-
-namespace Bit.Core.Auth.LoginFeatures.PasswordlessLogin;
-
-public class VerifyAuthRequestCommand : IVerifyAuthRequestCommand
-{
-    private readonly IAuthRequestRepository _authRequestRepository;
-
-    public VerifyAuthRequestCommand(IAuthRequestRepository authRequestRepository)
-    {
-        _authRequestRepository = authRequestRepository;
-    }
-
-    public async Task<bool> VerifyAuthRequestAsync(Guid authRequestId, string accessCode)
-    {
-        var authRequest = await _authRequestRepository.GetByIdAsync(authRequestId);
-        if (authRequest == null || !CoreHelpers.FixedTimeEquals(authRequest.AccessCode, accessCode))
-        {
-            return false;
-        }
-        return true;
-    }
-}

src/SharedWeb/Utilities/ServiceCollectionExtensions.cs

@@ -21,7 +21,6 @@ using Bit.Core.Auth.Enums;
 using Bit.Core.Auth.Identity;
 using Bit.Core.Auth.Identity.TokenProviders;
 using Bit.Core.Auth.IdentityServer;
-using Bit.Core.Auth.LoginFeatures;
 using Bit.Core.Auth.Models.Business.Tokenables;
 using Bit.Core.Auth.Repositories;
 using Bit.Core.Auth.Services;
@@ -140,7 +139,6 @@ public static class ServiceCollectionExtensions
         services.AddScoped<IAuthRequestService, AuthRequestService>();
         services.AddScoped<IDuoUniversalTokenService, DuoUniversalTokenService>();
         services.AddScoped<ISendAuthorizationService, SendAuthorizationService>();
-        services.AddLoginServices();
         services.AddScoped<IOrganizationDomainService, OrganizationDomainService>();
         services.AddVaultServices();
         services.AddReportingServices();

test/Core.Test/Auth/Entities/AuthRequestTests.cs

@@ -0,0 +1,224 @@
+using Bit.Core.Auth.Entities;
+using Bit.Core.Auth.Enums;
+using Xunit;
+
+namespace Bit.Core.Test.Auth.Entities;
+
+public class AuthRequestTests
+{
+    [Fact]
+    public void IsValidForAuthentication_WithValidRequest_ReturnsTrue()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = DateTime.UtcNow,
+            Approved = true,
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = null,
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, accessCode);
+
+        // Assert
+        Assert.True(result);
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithWrongUserId_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var differentUserId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = DateTime.UtcNow,
+            Approved = true,
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = null,
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(differentUserId, accessCode);
+
+        // Assert
+        Assert.False(result, "Auth request should not validate for a different user");
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithWrongAccessCode_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = DateTime.UtcNow,
+            Approved = true,
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = null,
+            AccessCode = "correct-code"
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, "wrong-code");
+
+        // Assert
+        Assert.False(result);
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithoutResponseDate_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = null, // Not responded to
+            Approved = true,
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = null,
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, accessCode);
+
+        // Assert
+        Assert.False(result, "Unanswered auth requests should not be valid");
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithApprovedFalse_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = DateTime.UtcNow,
+            Approved = false, // Denied
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = null,
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, accessCode);
+
+        // Assert
+        Assert.False(result, "Denied auth requests should not be valid");
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithApprovedNull_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = DateTime.UtcNow,
+            Approved = null, // Pending
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = null,
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, accessCode);
+
+        // Assert
+        Assert.False(result, "Pending auth requests should not be valid");
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithExpiredRequest_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = DateTime.UtcNow,
+            Approved = true,
+            CreationDate = DateTime.UtcNow.AddMinutes(-20), // Expired (15 min timeout)
+            AuthenticationDate = null,
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, accessCode);
+
+        // Assert
+        Assert.False(result, "Expired auth requests should not be valid");
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithWrongType_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.Unlock, // Wrong type
+            ResponseDate = DateTime.UtcNow,
+            Approved = true,
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = null,
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, accessCode);
+
+        // Assert
+        Assert.False(result, "Only AuthenticateAndUnlock type should be valid");
+    }
+
+    [Fact]
+    public void IsValidForAuthentication_WithAlreadyUsed_ReturnsFalse()
+    {
+        // Arrange
+        var userId = Guid.NewGuid();
+        var accessCode = "test-access-code";
+        var authRequest = new AuthRequest
+        {
+            UserId = userId,
+            Type = AuthRequestType.AuthenticateAndUnlock,
+            ResponseDate = DateTime.UtcNow,
+            Approved = true,
+            CreationDate = DateTime.UtcNow,
+            AuthenticationDate = DateTime.UtcNow, // Already used
+            AccessCode = accessCode
+        };
+
+        // Act
+        var result = authRequest.IsValidForAuthentication(userId, accessCode);
+
+        // Assert
+        Assert.False(result, "Auth requests should only be valid for one-time use");
+    }
+}