From 000d1f2f6ef3f6a679ed1b0d174cd18b8b8741dd Mon Sep 17 00:00:00 2001 From: Jared Snider <116684653+JaredSnider-Bitwarden@users.noreply.github.com> Date: Wed, 6 Aug 2025 10:18:57 -0400 Subject: [PATCH] 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 --- .../RequestValidators/DeviceValidator.cs | 39 +++++++------- .../IdentityServer/DeviceValidatorTests.cs | 51 +++++++++++++++++-- 2 files changed, 68 insertions(+), 22 deletions(-) diff --git a/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs b/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs index 80eb455519..d9a4fdb485 100644 --- a/src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs +++ b/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 - if (!NewDeviceOtpRequest(request)) + // Check if the request has a NewDeviceOtp, if it does we can assume it is an unknown device + // 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 - // 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 + // The device is either unknown or the request has a NewDeviceOtp (implies unknown device) + 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( /// /// /// - public static bool NewDeviceOtpRequest(ValidatedRequest request) + public static bool RequestHasNewDeviceVerificationOtp(ValidatedRequest request) { return !string.IsNullOrEmpty(request.Raw["NewDeviceOtp"]?.ToString()); } diff --git a/test/Identity.Test/IdentityServer/DeviceValidatorTests.cs b/test/Identity.Test/IdentityServer/DeviceValidatorTests.cs index 551f34b90a..7cb16da4ec 100644 --- a/test/Identity.Test/IdentityServer/DeviceValidatorTests.cs +++ b/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()); + } + + [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()); + await _deviceRepository.DidNotReceive().GetByIdentifierAsync(Arg.Any(), Arg.Any()); } [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)