diff --git a/src/Core/Auth/Services/Implementations/AuthRequestService.cs b/src/Core/Auth/Services/Implementations/AuthRequestService.cs index b75a5c130..e59177d9f 100644 --- a/src/Core/Auth/Services/Implementations/AuthRequestService.cs +++ b/src/Core/Auth/Services/Implementations/AuthRequestService.cs @@ -82,34 +82,39 @@ public class AuthRequestService : IAuthRequestService /// public async Task CreateAuthRequestAsync(AuthRequestCreateRequestModel model) { - var user = await _userRepository.GetByEmailAsync(model.Email); - if (user == null) - { - throw new NotFoundException(); - } - if (!_currentContext.DeviceType.HasValue) { throw new BadRequestException("Device type not provided."); } - if (_globalSettings.PasswordlessAuth.KnownDevicesOnly) + var userNotFound = false; + var user = await _userRepository.GetByEmailAsync(model.Email); + if (user == null) + { + userNotFound = true; + } + else if (_globalSettings.PasswordlessAuth.KnownDevicesOnly) { var devices = await _deviceRepository.GetManyByUserIdAsync(user.Id); if (devices == null || !devices.Any(d => d.Identifier == model.DeviceIdentifier)) { - throw new BadRequestException( - "Login with device is only available on devices that have been previously logged in."); + userNotFound = true; } } + // Anonymous endpoints must not leak that a user exists or not + if (userNotFound) + { + throw new BadRequestException("User or known device not found."); + } + // AdminApproval requests require correlating the user and their organization if (model.Type == AuthRequestType.AdminApproval) { // TODO: When single org policy is turned on we should query for only a single organization from the current user // and create only an AuthRequest for that organization and return only that one - // This will send out the request to all organizations this user belongs to + // This will send out the request to all organizations this user belongs to var organizationUsers = await _organizationUserRepository.GetManyByUserAsync(_currentContext.UserId!.Value); if (organizationUsers.Count == 0) @@ -173,7 +178,7 @@ public class AuthRequestService : IAuthRequestService switch (authRequest.Type) { case AuthRequestType.AdminApproval: - // AdminApproval has a different expiration time, by default is 7 days compared to + // AdminApproval has a different expiration time, by default is 7 days compared to // non-AdminApproval ones having a default of 15 minutes. if (IsDateExpired(authRequest.CreationDate, _globalSettings.PasswordlessAuth.AdminRequestExpiration)) { @@ -213,7 +218,7 @@ public class AuthRequestService : IAuthRequestService await _authRequestRepository.ReplaceAsync(authRequest); - // We only want to send an approval notification if the request is approved (or null), + // We only want to send an approval notification if the request is approved (or null), // to not leak that it was denied to the originating client if it was originated by a malicious actor. if (authRequest.Approved ?? true) { diff --git a/test/Core.Test/Auth/Services/AuthRequestServiceTests.cs b/test/Core.Test/Auth/Services/AuthRequestServiceTests.cs index 3cac77405..cd7f85ae8 100644 --- a/test/Core.Test/Auth/Services/AuthRequestServiceTests.cs +++ b/test/Core.Test/Auth/Services/AuthRequestServiceTests.cs @@ -142,15 +142,19 @@ public class AuthRequestServiceTests } [Theory, BitAutoData] - public async Task CreateAuthRequestAsync_NoUser_ThrowsNotFound( + public async Task CreateAuthRequestAsync_NoUser_ThrowsBadRequest( SutProvider sutProvider, AuthRequestCreateRequestModel createModel) { + sutProvider.GetDependency() + .DeviceType + .Returns(DeviceType.Android); + sutProvider.GetDependency() .GetByEmailAsync(createModel.Email) .Returns((User?)null); - await Assert.ThrowsAsync(() => sutProvider.Sut.CreateAuthRequestAsync(createModel)); + await Assert.ThrowsAsync(() => sutProvider.Sut.CreateAuthRequestAsync(createModel)); } [Theory, BitAutoData] @@ -253,7 +257,7 @@ public class AuthRequestServiceTests /// /// Story: If a user happens to exist to more than one organization, we will send the device approval request to - /// each of them. + /// each of them. /// [Theory, BitAutoData] public async Task CreateAuthRequestAsync_AdminApproval_CreatesForEachOrganization( @@ -627,8 +631,8 @@ public class AuthRequestServiceTests } /// - /// Story: An admin approves a request for one of their org users. For auditing purposes we need to - /// log an event that correlates the action for who the request was approved for. On approval we also need to + /// Story: An admin approves a request for one of their org users. For auditing purposes we need to + /// log an event that correlates the action for who the request was approved for. On approval we also need to /// push the notification to the user. /// [Theory, BitAutoData]