diff --git a/src/Api/Auth/Controllers/TwoFactorController.cs b/src/Api/Auth/Controllers/TwoFactorController.cs index 0af46fb57c19..ba6cf6685996 100644 --- a/src/Api/Auth/Controllers/TwoFactorController.cs +++ b/src/Api/Auth/Controllers/TwoFactorController.cs @@ -9,7 +9,6 @@ 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 _userManager; private readonly ICurrentContext _currentContext; - private readonly IVerifyAuthRequestCommand _verifyAuthRequestCommand; + private readonly IAuthRequestRepository _authRequestRepository; private readonly IDuoUniversalTokenService _duoUniversalTokenService; private readonly IDataProtectorTokenFactory _twoFactorAuthenticatorDataProtector; private readonly IDataProtectorTokenFactory _ssoEmailTwoFactorSessionDataProtector; @@ -47,7 +46,7 @@ public TwoFactorController( IOrganizationService organizationService, UserManager userManager, ICurrentContext currentContext, - IVerifyAuthRequestCommand verifyAuthRequestCommand, + IAuthRequestRepository authRequestRepository, IDuoUniversalTokenService duoUniversalConfigService, IDataProtectorTokenFactory twoFactorAuthenticatorDataProtector, IDataProtectorTokenFactory ssoEmailTwoFactorSessionDataProtector, @@ -58,7 +57,7 @@ public TwoFactorController( _organizationService = organizationService; _userManager = userManager; _currentContext = currentContext; - _verifyAuthRequestCommand = verifyAuthRequestCommand; + _authRequestRepository = authRequestRepository; _duoUniversalTokenService = duoUniversalConfigService; _twoFactorAuthenticatorDataProtector = twoFactorAuthenticatorDataProtector; _ssoEmailTwoFactorSessionDataProtector = ssoEmailTwoFactorSessionDataProtector; @@ -350,14 +349,15 @@ public async Task SendEmailLoginAsync([FromBody] TwoFactorEmailRequestModel requ 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 - .VerifyAuthRequestAsync(new Guid(requestModel.AuthRequestId), - requestModel.AuthRequestAccessCode)) + var authRequest = await _authRequestRepository.GetByIdAsync(new Guid(requestModel.AuthRequestId)); + if (authRequest != null && + authRequest.IsValidForAuthentication(user.Id, requestModel.AuthRequestAccessCode)) { await _twoFactorEmailService.SendTwoFactorEmailAsync(user); + return; } } else if (!string.IsNullOrEmpty(requestModel.SsoEmail2FaSessionToken)) diff --git a/src/Core/Auth/Entities/AuthRequest.cs b/src/Core/Auth/Entities/AuthRequest.cs index 2117c575c0fc..38dc0534c1e3 100644 --- a/src/Core/Auth/Entities/AuthRequest.cs +++ b/src/Core/Auth/Entities/AuthRequest.cs @@ -49,11 +49,9 @@ public bool IsSpent() 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) { diff --git a/src/Core/Auth/LoginFeatures/LoginServiceCollectionExtensions.cs b/src/Core/Auth/LoginFeatures/LoginServiceCollectionExtensions.cs deleted file mode 100644 index f8caad448b32..000000000000 --- a/src/Core/Auth/LoginFeatures/LoginServiceCollectionExtensions.cs +++ /dev/null @@ -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(); - } -} - diff --git a/src/Core/Auth/LoginFeatures/PasswordlessLogin/Interfaces/IVerifyAuthRequest.cs b/src/Core/Auth/LoginFeatures/PasswordlessLogin/Interfaces/IVerifyAuthRequest.cs deleted file mode 100644 index e5da1b06d822..000000000000 --- a/src/Core/Auth/LoginFeatures/PasswordlessLogin/Interfaces/IVerifyAuthRequest.cs +++ /dev/null @@ -1,6 +0,0 @@ -namespace Bit.Core.Auth.LoginFeatures.PasswordlessLogin.Interfaces; - -public interface IVerifyAuthRequestCommand -{ - Task VerifyAuthRequestAsync(Guid authRequestId, string accessCode); -} diff --git a/src/Core/Auth/LoginFeatures/PasswordlessLogin/VerifyAuthRequest.cs b/src/Core/Auth/LoginFeatures/PasswordlessLogin/VerifyAuthRequest.cs deleted file mode 100644 index 7def7fea7683..000000000000 --- a/src/Core/Auth/LoginFeatures/PasswordlessLogin/VerifyAuthRequest.cs +++ /dev/null @@ -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 VerifyAuthRequestAsync(Guid authRequestId, string accessCode) - { - var authRequest = await _authRequestRepository.GetByIdAsync(authRequestId); - if (authRequest == null || !CoreHelpers.FixedTimeEquals(authRequest.AccessCode, accessCode)) - { - return false; - } - return true; - } -} diff --git a/src/SharedWeb/Utilities/ServiceCollectionExtensions.cs b/src/SharedWeb/Utilities/ServiceCollectionExtensions.cs index 587ddb65a4a1..3d9a79a2e15c 100644 --- a/src/SharedWeb/Utilities/ServiceCollectionExtensions.cs +++ b/src/SharedWeb/Utilities/ServiceCollectionExtensions.cs @@ -21,7 +21,6 @@ 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 void AddBaseServices(this IServiceCollection services, IGlobalSett services.AddScoped(); services.AddScoped(); services.AddScoped(); - services.AddLoginServices(); services.AddScoped(); services.AddVaultServices(); services.AddReportingServices(); diff --git a/test/Core.Test/Auth/Entities/AuthRequestTests.cs b/test/Core.Test/Auth/Entities/AuthRequestTests.cs new file mode 100644 index 000000000000..9efeb1ded1c4 --- /dev/null +++ b/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"); + } +}