PS-82 check send 2FA email for new devices on TwoFactorController sen…

…d-email-login (#1977)
bitwarden · Apr 28, 2022 · a7a4589 · a7a4589
1 parent 68f875b
commit a7a4589
Show file tree

Hide file tree

Showing 6 changed files with 161 additions and 21 deletions.
diff --git a/src/Api/Controllers/TwoFactorController.cs b/src/Api/Controllers/TwoFactorController.cs
@@ -289,7 +289,16 @@ public async Task SendEmailLogin([FromBody] TwoFactorEmailRequestModel model)
             {
                 if (await _userService.VerifySecretAsync(user, model.Secret))
                 {
-                    await _userService.SendTwoFactorEmailAsync(user);
+                    var isBecauseNewDeviceLogin = false;
+                    if (user.GetTwoFactorProvider(TwoFactorProviderType.Email) is null
+                        &&
+                        await _userService.Needs2FABecauseNewDeviceAsync(user, model.DeviceIdentifier, null))
+                    {
+                        model.ToUser(user);
+                        isBecauseNewDeviceLogin = true;
+                    }
+
+                    await _userService.SendTwoFactorEmailAsync(user, isBecauseNewDeviceLogin);
                     return;
                 }
             }

diff --git a/src/Api/Models/Request/TwoFactorRequestModels.cs b/src/Api/Models/Request/TwoFactorRequestModels.cs
@@ -203,6 +203,8 @@ public class TwoFactorEmailRequestModel : SecretVerificationRequestModel
         [StringLength(256)]
         public string Email { get; set; }
 
+        public string DeviceIdentifier { get; set; }
+
         public User ToUser(User extistingUser)
         {
             var providers = extistingUser.GetTwoFactorProviders();

diff --git a/src/Core/IdentityServer/BaseRequestValidator.cs b/src/Core/IdentityServer/BaseRequestValidator.cs
@@ -319,9 +319,11 @@ protected async Task BuildErrorResultAsync(string message, bool twoFactorRequest
 
             var requires2FA = individualRequired || firstEnabledOrg != null;
             var requires2FABecauseNewDevice = !requires2FA
-                                              && user.EmailVerified
-                                              && request.GrantType != "authorization_code"
-                                              && await IsNewDeviceAndNotTheFirstOneAsync(user, request);
+                                              &&
+                                              await _userService.Needs2FABecauseNewDeviceAsync(
+                                                    user,
+                                                    GetDeviceFromRequest(request)?.Identifier,
+                                                    request.GrantType);
 
             requires2FA = requires2FA || requires2FABecauseNewDevice;
 
@@ -536,22 +538,6 @@ protected async Task<Device> GetKnownDeviceAsync(User user, ValidatedTokenReques
             return await _deviceRepository.GetByIdentifierAsync(GetDeviceFromRequest(request).Identifier, user.Id);
         }
 
-        protected async Task<bool> IsNewDeviceAndNotTheFirstOneAsync(User user, ValidatedTokenRequest request)
-        {
-            if (user == null)
-            {
-                return default;
-            }
-
-            var devices = await _deviceRepository.GetManyByUserIdAsync(user.Id);
-            if (!devices.Any())
-            {
-                return false;
-            }
-
-            return !devices.Any(d => d.Identifier == GetDeviceFromRequest(request)?.Identifier);
-        }
-
         private async Task<Device> SaveDeviceAsync(User user, ValidatedTokenRequest request)
         {
             var device = GetDeviceFromRequest(request);

diff --git a/src/Core/Services/IUserService.cs b/src/Core/Services/IUserService.cs
@@ -78,5 +78,6 @@ public interface IUserService
         Task SendOTPAsync(User user);
         Task<bool> VerifyOTPAsync(User user, string token);
         Task<bool> VerifySecretAsync(User user, string secret);
+        Task<bool> Needs2FABecauseNewDeviceAsync(User user, string deviceIdentifier, string grantType);
     }
 }
diff --git a/src/Core/Services/Implementations/UserService.cs b/src/Core/Services/Implementations/UserService.cs
@@ -17,7 +17,9 @@
 using Fido2NetLib;
 using Fido2NetLib.Objects;
 using Microsoft.AspNetCore.DataProtection;
+using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Identity;
+using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Logging;
 using Microsoft.Extensions.Options;
 using File = System.IO.File;
@@ -51,6 +53,8 @@ public class UserService : UserManager<User>, IUserService, IDisposable
         private readonly GlobalSettings _globalSettings;
         private readonly IOrganizationService _organizationService;
         private readonly IProviderUserRepository _providerUserRepository;
+        private readonly IDeviceRepository _deviceRepository;
+        private readonly IWebHostEnvironment _environment;
 
         public UserService(
             IUserRepository userRepository,
@@ -79,7 +83,9 @@ public class UserService : UserManager<User>, IUserService, IDisposable
             ICurrentContext currentContext,
             GlobalSettings globalSettings,
             IOrganizationService organizationService,
-            IProviderUserRepository providerUserRepository)
+            IProviderUserRepository providerUserRepository,
+            IDeviceRepository deviceRepository,
+            IWebHostEnvironment environment)
             : base(
                   store,
                   optionsAccessor,
@@ -114,6 +120,8 @@ public class UserService : UserManager<User>, IUserService, IDisposable
             _globalSettings = globalSettings;
             _organizationService = organizationService;
             _providerUserRepository = providerUserRepository;
+            _deviceRepository = deviceRepository;
+            _environment = environment;
         }
 
         public Guid? GetProperUserId(ClaimsPrincipal principal)
@@ -1408,5 +1416,29 @@ public async Task<bool> VerifySecretAsync(User user, string secret)
                 ? await VerifyOTPAsync(user, secret)
                 : await CheckPasswordAsync(user, secret);
         }
+
+        public async Task<bool> Needs2FABecauseNewDeviceAsync(User user, string deviceIdentifier, string grantType)
+        {
+            return user.EmailVerified
+                   && grantType != "authorization_code"
+                   && !_environment.IsDevelopment()
+                   && await IsNewDeviceAndNotTheFirstOneAsync(user, deviceIdentifier);
+        }
+
+        private async Task<bool> IsNewDeviceAndNotTheFirstOneAsync(User user, string deviceIdentifier)
+        {
+            if (user == null)
+            {
+                return default;
+            }
+
+            var devices = await _deviceRepository.GetManyByUserIdAsync(user.Id);
+            if (!devices.Any())
+            {
+                return false;
+            }
+
+            return !devices.Any(d => d.Identifier == deviceIdentifier);
+        }
     }
 }
diff --git a/test/Core.Test/Services/UserServiceTests.cs b/test/Core.Test/Services/UserServiceTests.cs
@@ -7,11 +7,14 @@
 using Bit.Core.Enums;
 using Bit.Core.Models;
 using Bit.Core.Models.Business;
+using Bit.Core.Repositories;
 using Bit.Core.Services;
 using Bit.Test.Common.AutoFixture;
 using Bit.Test.Common.AutoFixture.Attributes;
 using Bit.Test.Common.Helpers;
+using Microsoft.AspNetCore.Hosting;
 using Microsoft.AspNetCore.Identity;
+using Microsoft.Extensions.Hosting;
 using NSubstitute;
 using NSubstitute.ReceivedExtensions;
 using Xunit;
@@ -156,5 +159,112 @@ public async Task SendTwoFactorEmailAsync_ExceptionBecauseNoProviderEmailMetadat
 
             await Assert.ThrowsAsync<ArgumentNullException>("No email.", () => sutProvider.Sut.SendTwoFactorEmailAsync(user));
         }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task Needs2FABecauseNewDeviceAsync_ReturnsTrue(SutProvider<UserService> sutProvider, User user)
+        {
+            user.Id = Guid.NewGuid();
+            user.EmailVerified = true;
+            const string deviceIdToCheck = "7b01b586-b210-499f-8d52-0c3fdaa646fc";
+            const string deviceIdInRepo = "ea29126c-91b7-4cc4-8ce6-00105b37f64a";
+
+            sutProvider.GetDependency<IDeviceRepository>()
+                       .GetManyByUserIdAsync(user.Id)
+                       .Returns(Task.FromResult<ICollection<Device>>(new List<Device>
+                       {
+                            new Device { Identifier = deviceIdInRepo }
+                       }));
+
+            Assert.True(await sutProvider.Sut.Needs2FABecauseNewDeviceAsync(user, deviceIdToCheck, "password"));
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task Needs2FABecauseNewDeviceAsync_ReturnsFalse_When_GranType_Is_AuthorizationCode(SutProvider<UserService> sutProvider, User user)
+        {
+            user.Id = Guid.NewGuid();
+            user.EmailVerified = true;
+            const string deviceIdToCheck = "7b01b586-b210-499f-8d52-0c3fdaa646fc";
+            const string deviceIdInRepo = "ea29126c-91b7-4cc4-8ce6-00105b37f64a";
+
+            sutProvider.GetDependency<IDeviceRepository>()
+                       .GetManyByUserIdAsync(user.Id)
+                       .Returns(Task.FromResult<ICollection<Device>>(new List<Device>
+                       {
+                            new Device { Identifier = deviceIdInRepo }
+                       }));
+
+            Assert.False(await sutProvider.Sut.Needs2FABecauseNewDeviceAsync(user, deviceIdToCheck, "authorization_code"));
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task Needs2FABecauseNewDeviceAsync_ReturnsFalse_When_Email_Is_Not_Verified(SutProvider<UserService> sutProvider, User user)
+        {
+            user.Id = Guid.NewGuid();
+            user.EmailVerified = false;
+            const string deviceIdToCheck = "7b01b586-b210-499f-8d52-0c3fdaa646fc";
+            const string deviceIdInRepo = "ea29126c-91b7-4cc4-8ce6-00105b37f64a";
+
+            sutProvider.GetDependency<IDeviceRepository>()
+                       .GetManyByUserIdAsync(user.Id)
+                       .Returns(Task.FromResult<ICollection<Device>>(new List<Device>
+                       {
+                            new Device { Identifier = deviceIdInRepo }
+                       }));
+
+            Assert.False(await sutProvider.Sut.Needs2FABecauseNewDeviceAsync(user, deviceIdToCheck, "password"));
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task Needs2FABecauseNewDeviceAsync_ReturnsFalse_When_Is_The_First_Device(SutProvider<UserService> sutProvider, User user)
+        {
+            user.Id = Guid.NewGuid();
+            user.EmailVerified = true;
+            const string deviceIdToCheck = "7b01b586-b210-499f-8d52-0c3fdaa646fc";
+
+            sutProvider.GetDependency<IDeviceRepository>()
+                       .GetManyByUserIdAsync(user.Id)
+                       .Returns(Task.FromResult<ICollection<Device>>(new List<Device>()));
+
+            Assert.False(await sutProvider.Sut.Needs2FABecauseNewDeviceAsync(user, deviceIdToCheck, "password"));
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task Needs2FABecauseNewDeviceAsync_ReturnsFalse_When_DeviceId_Is_Already_In_Repo(SutProvider<UserService> sutProvider, User user)
+        {
+            user.Id = Guid.NewGuid();
+            user.EmailVerified = true;
+            const string deviceIdToCheck = "7b01b586-b210-499f-8d52-0c3fdaa646fc";
+
+            sutProvider.GetDependency<IDeviceRepository>()
+                       .GetManyByUserIdAsync(user.Id)
+                       .Returns(Task.FromResult<ICollection<Device>>(new List<Device>
+                       {
+                            new Device { Identifier = deviceIdToCheck }
+                       }));
+
+            Assert.False(await sutProvider.Sut.Needs2FABecauseNewDeviceAsync(user, deviceIdToCheck, "password"));
+        }
+
+        [Theory, CustomAutoData(typeof(SutProviderCustomization))]
+        public async Task Needs2FABecauseNewDeviceAsync_ReturnsFalse_When_Environment_Is_Development(SutProvider<UserService> sutProvider, User user)
+        {
+            user.Id = Guid.NewGuid();
+            user.EmailVerified = true;
+            const string deviceIdToCheck = "7b01b586-b210-499f-8d52-0c3fdaa646fc";
+            const string deviceIdInRepo = "ea29126c-91b7-4cc4-8ce6-00105b37f64a";
+
+            sutProvider.GetDependency<IDeviceRepository>()
+                       .GetManyByUserIdAsync(user.Id)
+                       .Returns(Task.FromResult<ICollection<Device>>(new List<Device>
+                       {
+                            new Device { Identifier = deviceIdInRepo }
+                       }));
+
+            sutProvider.GetDependency<IWebHostEnvironment>()
+                       .EnvironmentName
+                       .Returns(Environments.Development);
+
+            Assert.False(await sutProvider.Sut.Needs2FABecauseNewDeviceAsync(user, deviceIdToCheck, "password"));
+        }
     }
 }