Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weโ€™ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PM-12994] Check for New Device Verification Exception #5153

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Core/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ public static class AuthConstants
public static readonly RangeConstant ARGON2_ITERATIONS = new(2, 10, 3);
public static readonly RangeConstant ARGON2_MEMORY = new(15, 1024, 64);
public static readonly RangeConstant ARGON2_PARALLELISM = new(1, 16, 4);

public static readonly string NewDeviceVerificationExceptionCacheKeyFormat = "NewDeviceVerificationException_{0}";
}

public class RangeConstant
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Bit.Core.Settings;
using Bit.Identity.IdentityServer.Enums;
using Duende.IdentityServer.Validation;
using Microsoft.Extensions.Caching.Distributed;

namespace Bit.Identity.IdentityServer.RequestValidators;

Expand All @@ -20,6 +21,8 @@ public class DeviceValidator(
IMailService mailService,
ICurrentContext currentContext,
IUserService userService,
IDistributedCache distributedCache,
ILogger<DeviceValidator> logger,
IFeatureService featureService) : IDeviceValidator
{
private readonly IDeviceService _deviceService = deviceService;
Expand All @@ -28,6 +31,8 @@ public class DeviceValidator(
private readonly IMailService _mailService = mailService;
private readonly ICurrentContext _currentContext = currentContext;
private readonly IUserService _userService = userService;
private readonly IDistributedCache distributedCache = distributedCache;
private readonly ILogger<DeviceValidator> _logger = logger;
private readonly IFeatureService _featureService = featureService;

public async Task<bool> ValidateRequestDeviceAsync(ValidatedTokenRequest request, CustomValidatorRequestContext context)
Expand Down Expand Up @@ -67,7 +72,6 @@ public async Task<bool> ValidateRequestDeviceAsync(ValidatedTokenRequest request
!context.SsoRequired &&
_globalSettings.EnableNewDeviceVerification)
{
// We only want to return early if the device is invalid or there is an error
var validationResult = await HandleNewDeviceVerificationAsync(context.User, request);
if (validationResult != DeviceValidationResultType.Success)
{
Expand Down Expand Up @@ -121,6 +125,18 @@ private async Task<DeviceValidationResultType> HandleNewDeviceVerificationAsync(
return DeviceValidationResultType.InvalidUser;
}

// CS exception flow
// Check cache for user information
var cacheKey = string.Format(AuthConstants.NewDeviceVerificationExceptionCacheKeyFormat, user.Id.ToString());
var cacheValue = await distributedCache.GetAsync(cacheKey);
if (cacheValue != null)
{
// if found in cache return success result and remove from cache
await distributedCache.RemoveAsync(cacheKey);
_logger.LogInformation("New device verification exception for user {UserId} found in cache", user.Id);
return DeviceValidationResultType.Success;
}

// parse request for NewDeviceOtp to validate
var newDeviceOtp = request.Raw["NewDeviceOtp"]?.ToString();
// we only check null here since an empty OTP will be considered an incorrect OTP
Expand Down
38 changes: 37 additions & 1 deletion test/Identity.Test/IdentityServer/DeviceValidatorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
using Bit.Identity.IdentityServer.RequestValidators;
using Bit.Test.Common.AutoFixture.Attributes;
using Duende.IdentityServer.Validation;
using Microsoft.Extensions.Caching.Distributed;
using Microsoft.Extensions.Logging;
using NSubstitute;
using Xunit;
using AuthFixtures = Bit.Identity.Test.AutoFixture;
Expand All @@ -24,6 +26,8 @@ public class DeviceValidatorTests
private readonly IMailService _mailService;
private readonly ICurrentContext _currentContext;
private readonly IUserService _userService;
private readonly IDistributedCache _distributedCache;
private readonly Logger<DeviceValidator> _logger;
private readonly IFeatureService _featureService;
private readonly DeviceValidator _sut;

Expand All @@ -35,6 +39,8 @@ public DeviceValidatorTests()
_mailService = Substitute.For<IMailService>();
_currentContext = Substitute.For<ICurrentContext>();
_userService = Substitute.For<IUserService>();
_distributedCache = Substitute.For<IDistributedCache>();
_logger = new Logger<DeviceValidator>(Substitute.For<ILoggerFactory>());
_featureService = Substitute.For<IFeatureService>();
_sut = new DeviceValidator(
_deviceService,
Expand All @@ -43,6 +49,8 @@ public DeviceValidatorTests()
_mailService,
_currentContext,
_userService,
_distributedCache,
_logger,
_featureService);
}

Expand All @@ -51,7 +59,7 @@ public async void GetKnownDeviceAsync_UserNull_ReturnsFalse(
Device device)
{
// Arrange
// AutoData arrages
// AutoData arranges

// Act
var result = await _sut.GetKnownDeviceAsync(null, device);
Expand Down Expand Up @@ -421,6 +429,30 @@ public async void HandleNewDeviceVerificationAsync_UserNull_ContextModified_Retu
Assert.Equal(expectedErrorMessage, actualResponse.Message);
}

[Theory, BitAutoData]
public async void HandleNewDeviceVerificationAsync_UserHasCacheValue_ReturnsSuccess(
CustomValidatorRequestContext context,
[AuthFixtures.ValidatedTokenRequest] ValidatedTokenRequest request)
{
// Arrange
ArrangeForHandleNewDeviceVerificationTest(context, request);
_featureService.IsEnabled(FeatureFlagKeys.NewDeviceVerification).Returns(true);
_globalSettings.EnableNewDeviceVerification = true;
_distributedCache.GetAsync(Arg.Any<string>()).Returns([1]);

// Act
var result = await _sut.ValidateRequestDeviceAsync(request, context);

// Assert
await _userService.Received(0).SendOTPAsync(context.User);
await _deviceService.Received(1).SaveAsync(Arg.Any<Device>());

Assert.True(result);
Assert.False(context.CustomResponse.ContainsKey("ErrorModel"));
Assert.Equal(context.User.Id, context.Device.UserId);
Assert.NotNull(context.Device);
}

[Theory, BitAutoData]
public async void HandleNewDeviceVerificationAsync_NewDeviceOtpValid_ReturnsSuccess(
CustomValidatorRequestContext context,
Expand All @@ -430,6 +462,7 @@ public async void HandleNewDeviceVerificationAsync_NewDeviceOtpValid_ReturnsSucc
ArrangeForHandleNewDeviceVerificationTest(context, request);
_featureService.IsEnabled(FeatureFlagKeys.NewDeviceVerification).Returns(true);
_globalSettings.EnableNewDeviceVerification = true;
_distributedCache.GetAsync(Arg.Any<string>()).Returns(null as byte[]);

var newDeviceOtp = "123456";
request.Raw.Add("NewDeviceOtp", newDeviceOtp);
Expand Down Expand Up @@ -461,6 +494,7 @@ public async void HandleNewDeviceVerificationAsync_NewDeviceOtpInvalid_ReturnsIn
ArrangeForHandleNewDeviceVerificationTest(context, request);
_featureService.IsEnabled(FeatureFlagKeys.NewDeviceVerification).Returns(true);
_globalSettings.EnableNewDeviceVerification = true;
_distributedCache.GetAsync(Arg.Any<string>()).Returns(null as byte[]);

request.Raw.Add("NewDeviceOtp", newDeviceOtp);

Expand Down Expand Up @@ -489,6 +523,7 @@ public async void HandleNewDeviceVerificationAsync_UserHasNoDevices_ReturnsSucce
ArrangeForHandleNewDeviceVerificationTest(context, request);
_featureService.IsEnabled(FeatureFlagKeys.NewDeviceVerification).Returns(true);
_globalSettings.EnableNewDeviceVerification = true;
_distributedCache.GetAsync(Arg.Any<string>()).Returns([1]);
_deviceRepository.GetManyByUserIdAsync(context.User.Id).Returns([]);

// Act
Expand All @@ -515,6 +550,7 @@ public async void HandleNewDeviceVerificationAsync_NewDeviceOtpEmpty_UserHasDevi
_featureService.IsEnabled(FeatureFlagKeys.NewDeviceVerification).Returns(true);
_globalSettings.EnableNewDeviceVerification = true;
_deviceRepository.GetManyByUserIdAsync(context.User.Id).Returns([new Device()]);
_distributedCache.GetAsync(Arg.Any<string>()).Returns(null as byte[]);

// Act
var result = await _sut.ValidateRequestDeviceAsync(request, context);
Expand Down
Loading