refactor(DeviceValidator): [Auth/PM-24362] Misc improvements (#6152)

* PM-24362 - DeviceValidator - (1) refactor name of NewDeviceOtpRequest --> RequestHasNewDeviceVerificationOtp (2) Move auth request rejection check above normal NDV check and remove auth request check from NDV check

* PM-24362 - Update DeviceValidatorTests + add new scenario

src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs

@@ -13,6 +13,7 @@ using Bit.Core.Repositories;
 using Bit.Core.Services;
 using Bit.Core.Settings;
 using Bit.Identity.IdentityServer.Enums;
+using Duende.IdentityServer.Models;
 using Duende.IdentityServer.Validation;
 using Microsoft.Extensions.Caching.Distributed;
 
@@ -55,8 +56,10 @@ public class DeviceValidator(
             return false;
         }
 
-        // if not a new device request then check if the device is known
+        // Check if the request has a NewDeviceOtp, if it does we can assume it is an unknown device
-        if (!NewDeviceOtpRequest(request))
+        // that has already been prompted for new device verification so we don't
+        // have to hit the database to check if the device is known to avoid unnecessary database calls.
+        if (!RequestHasNewDeviceVerificationOtp(request))
         {
             var knownDevice = await GetKnownDeviceAsync(context.User, requestDevice);
             // if the device is know then we return the device fetched from the database
@@ -69,14 +72,23 @@ public class DeviceValidator(
             }
         }
 
-        // We have established that the device is unknown at this point; begin new device verification
+        // The device is either unknown or the request has a NewDeviceOtp (implies unknown device)
-        // for standard password grant type requests
+
-        // Note: the auth request flow re-uses the resource owner password flow but new device verification
-        // is not required for auth requests
         var rawAuthRequestId = request.Raw["AuthRequest"]?.ToLowerInvariant();
         var isAuthRequest = !string.IsNullOrEmpty(rawAuthRequestId);
-        if (request.GrantType == PasswordGrantType &&
+
-            !isAuthRequest &&
+        // Device unknown, but if we are in an auth request flow, this is not valid
+        // as we only support auth request authN requests on known devices
+        // Note: we re-use the resource owner password flow for auth requests
+        if (request.GrantType == GrantType.ResourceOwnerPassword && isAuthRequest)
+        {
+            (context.ValidationErrorResult, context.CustomResponse) =
+                BuildDeviceErrorResult(DeviceValidationResultType.AuthRequestFlowUnknownDevice);
+            return false;
+        }
+
+        // Enforce new device verification for resource owner password flow (just normal password flow)
+        if (request.GrantType == GrantType.ResourceOwnerPassword &&
             context is { TwoFactorRequired: false, SsoRequired: false } &&
             _globalSettings.EnableNewDeviceVerification)
         {
@@ -93,15 +105,6 @@ public class DeviceValidator(
             }
         }
 
-        // Device still unknown, but if we are in an auth request flow, this is not valid
-        // as we only support auth request authN requests on known devices
-        if (request.GrantType == PasswordGrantType && isAuthRequest)
-        {
-            (context.ValidationErrorResult, context.CustomResponse) =
-                BuildDeviceErrorResult(DeviceValidationResultType.AuthRequestFlowUnknownDevice);
-            return false;
-        }
-
         // At this point we have established either new device verification is not required or the NewDeviceOtp is valid,
         // so we save the device to the database and proceed with authentication
         requestDevice.UserId = context.User.Id;
@@ -247,7 +250,7 @@ public class DeviceValidator(
     /// </summary>
     /// <param name="request"></param>
     /// <returns></returns>
-    public static bool NewDeviceOtpRequest(ValidatedRequest request)
+    public static bool RequestHasNewDeviceVerificationOtp(ValidatedRequest request)
     {
         return !string.IsNullOrEmpty(request.Raw["NewDeviceOtp"]?.ToString());
     }

test/Identity.Test/IdentityServer/DeviceValidatorTests.cs

@@ -9,6 +9,7 @@ using Bit.Core.Settings;
 using Bit.Identity.IdentityServer;
 using Bit.Identity.IdentityServer.RequestValidators;
 using Bit.Test.Common.AutoFixture.Attributes;
+using Duende.IdentityServer.Models;
 using Duende.IdentityServer.Validation;
 using Microsoft.Extensions.Caching.Distributed;
 using Microsoft.Extensions.Logging;
@@ -336,7 +337,7 @@ public class DeviceValidatorTests
         [AuthFixtures.ValidatedTokenRequest] ValidatedTokenRequest request)
     {
         // Arrange
-        request.GrantType = "password";
+        request.GrantType = GrantType.ResourceOwnerPassword;
         context.TwoFactorRequired = twoFactoRequired;
         context.SsoRequired = ssoRequired;
         if (context.User != null)
@@ -359,6 +360,48 @@ public class DeviceValidatorTests
         var expectedErrorMessage = "auth request flow unsupported on unknown device";
         var actualResponse = (ErrorResponseModel)context.CustomResponse["ErrorModel"];
         Assert.Equal(expectedErrorMessage, actualResponse.Message);
+        await _deviceService.Received(0).SaveAsync(Arg.Any<Device>());
+    }
+
+    [Theory]
+    [BitAutoData(false, false)]
+    [BitAutoData(true, false)]
+    [BitAutoData(true, true)]
+    [BitAutoData(true, false)]
+    public async void ValidateRequestDeviceAsync_IsAuthRequest_NewDeviceOtp_Errors(
+        bool twoFactoRequired, bool ssoRequired,
+        CustomValidatorRequestContext context,
+        [AuthFixtures.ValidatedTokenRequest] ValidatedTokenRequest request)
+    {
+        // Arrange
+        request.GrantType = GrantType.ResourceOwnerPassword;
+        context.TwoFactorRequired = twoFactoRequired;
+        context.SsoRequired = ssoRequired;
+        if (context.User != null)
+        {
+            context.User.CreationDate = DateTime.UtcNow - TimeSpan.FromDays(365);
+        }
+
+        AddValidDeviceToRequest(request);
+
+        request.Raw.Add("AuthRequest", "authRequest");
+        // Simulate a new device OTP being present in the request in addition to the auth request
+        // We don't check known device if an new device OTP is present, but we still
+        // want to ensure that the auth request attempt is rejected
+        var newDeviceOtp = "123456";
+        request.Raw.Add("NewDeviceOtp", newDeviceOtp);
+
+        // Act
+        var result = await _sut.ValidateRequestDeviceAsync(request, context);
+
+        // Assert
+        Assert.False(result);
+        Assert.NotNull(context.CustomResponse["ErrorModel"]);
+        var expectedErrorMessage = "auth request flow unsupported on unknown device";
+        var actualResponse = (ErrorResponseModel)context.CustomResponse["ErrorModel"];
+        Assert.Equal(expectedErrorMessage, actualResponse.Message);
+        await _deviceService.Received(0).SaveAsync(Arg.Any<Device>());
+        await _deviceRepository.DidNotReceive().GetByIdentifierAsync(Arg.Any<string>(), Arg.Any<Guid>());
     }
 
     [Theory, BitAutoData]
@@ -617,7 +660,7 @@ public class DeviceValidatorTests
         // Autodata arranges
 
         // Act
-        var result = DeviceValidator.NewDeviceOtpRequest(request);
+        var result = DeviceValidator.RequestHasNewDeviceVerificationOtp(request);
 
         // Assert
         Assert.False(result);
@@ -631,7 +674,7 @@ public class DeviceValidatorTests
         request.Raw["NewDeviceOtp"] = "123456";
 
         // Act
-        var result = DeviceValidator.NewDeviceOtpRequest(request);
+        var result = DeviceValidator.RequestHasNewDeviceVerificationOtp(request);
 
         // Assert
         Assert.True(result);
@@ -655,7 +698,7 @@ public class DeviceValidatorTests
         ValidatedTokenRequest request)
     {
         context.KnownDevice = false;
-        request.GrantType = "password";
+        request.GrantType = GrantType.ResourceOwnerPassword;
         context.TwoFactorRequired = false;
         context.SsoRequired = false;
         if (context.User != null)