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-15614] Allow Users to opt out of new device verification #5176

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
19 changes: 18 additions & 1 deletion src/Api/Auth/Controllers/AccountsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -964,11 +964,28 @@ public async Task VerifyOTP([FromBody] VerifyOTPRequestModel model)
[RequireFeature(FeatureFlagKeys.NewDeviceVerification)]
[AllowAnonymous]
[HttpPost("resend-new-device-otp")]
public async Task ResendNewDeviceOtpAsync([FromBody] UnauthenticatedSecretVerificatioRequestModel request)
public async Task ResendNewDeviceOtpAsync([FromBody] UnauthenticatedSecretVerificationRequestModel request)
{
await _userService.ResendNewDeviceVerificationEmail(request.Email, request.Secret);
}

[RequireFeature(FeatureFlagKeys.NewDeviceVerification)]
[HttpPost("verify-devices")]
[HttpPut("verify-devices")]
public async Task SetUserVerifyDevicesAsync([FromBody] SetVerifyDevicesRequestModel request)
{
var user = await _userService.GetUserByPrincipalAsync(User) ?? throw new UnauthorizedAccessException();

if (!await _userService.VerifySecretAsync(user, request.Secret))
{
await Task.Delay(2000);
throw new BadRequestException(string.Empty, "User verification failed.");
}
user.VerifyDevices = request.VerifyDevices;

await _userService.SaveUserAsync(user);
}

private async Task<IEnumerable<Guid>> GetOrganizationIdsManagingUserAsync(Guid userId)
{
var organizationManagingUser = await _userService.GetOrganizationsManagingUserAsync(userId);
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
๏ปฟusing System.ComponentModel.DataAnnotations;

namespace Bit.Api.Auth.Models.Request.Accounts;

public class SetVerifyDevicesRequestModel : SecretVerificationRequestModel
{
[Required]
public bool VerifyDevices { get; set; }
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

namespace Bit.Api.Auth.Models.Request.Accounts;

public class UnauthenticatedSecretVerificatioRequestModel : SecretVerificationRequestModel
public class UnauthenticatedSecretVerificationRequestModel : SecretVerificationRequestModel
{
[Required]
[StrictEmailAddress]
Expand Down
1 change: 1 addition & 0 deletions src/Core/Entities/User.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ public class User : ITableObject<Guid>, IStorableSubscriber, IRevisable, ITwoFac
public DateTime? LastKdfChangeDate { get; set; }
public DateTime? LastKeyRotationDate { get; set; }
public DateTime? LastEmailChangeDate { get; set; }
public bool VerifyDevices { get; set; } = true;

public void SetNewId()
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ await _mailService.SendNewDeviceLoggedInEmail(context.User.Email, deviceType, no
/// </summary>
/// <param name="user">user attempting to authenticate</param>
/// <param name="ValidatedRequest">The Request is used to check for the NewDeviceOtp and for the raw device data</param>
/// <returns>returns deviceValtaionResultType</returns>
/// <returns>returns deviceValidationResultType</returns>
private async Task<DeviceValidationResultType> HandleNewDeviceVerificationAsync(User user, ValidatedRequest request)
{
// currently unreachable due to backward compatibility
Expand All @@ -125,6 +125,12 @@ private async Task<DeviceValidationResultType> HandleNewDeviceVerificationAsync(
return DeviceValidationResultType.InvalidUser;
}

// Has the User opted out of new device verification
if (!user.VerifyDevices)
{
return DeviceValidationResultType.Success;
}

// CS exception flow
// Check cache for user information
var cacheKey = string.Format(AuthConstants.NewDeviceVerificationExceptionCacheKeyFormat, user.Id.ToString());
Expand All @@ -146,6 +152,12 @@ private async Task<DeviceValidationResultType> HandleNewDeviceVerificationAsync(
var otpValid = await _userService.VerifyOTPAsync(user, newDeviceOtp);
if (otpValid)
{
// In order to get here they would have to have access to their email so we verify it if it's not already
if (!user.EmailVerified)
{
user.EmailVerified = true;
await _userService.SaveUserAsync(user);
}
return DeviceValidationResultType.Success;
}
return DeviceValidationResultType.InvalidNewDeviceOtp;
Expand Down
9 changes: 6 additions & 3 deletions src/Sql/dbo/Stored Procedures/User_Create.sql
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
@LastPasswordChangeDate DATETIME2(7) = NULL,
@LastKdfChangeDate DATETIME2(7) = NULL,
@LastKeyRotationDate DATETIME2(7) = NULL,
@LastEmailChangeDate DATETIME2(7) = NULL
@LastEmailChangeDate DATETIME2(7) = NULL,
@VerifyDevices BIT = 1
AS
BEGIN
SET NOCOUNT ON
Expand Down Expand Up @@ -88,7 +89,8 @@ BEGIN
[LastPasswordChangeDate],
[LastKdfChangeDate],
[LastKeyRotationDate],
[LastEmailChangeDate]
[LastEmailChangeDate],
[VerifyDevices]
)
VALUES
(
Expand Down Expand Up @@ -133,6 +135,7 @@ BEGIN
@LastPasswordChangeDate,
@LastKdfChangeDate,
@LastKeyRotationDate,
@LastEmailChangeDate
@LastEmailChangeDate,
@VerifyDevices
)
END
6 changes: 4 additions & 2 deletions src/Sql/dbo/Stored Procedures/User_Update.sql
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,8 @@
@LastPasswordChangeDate DATETIME2(7) = NULL,
@LastKdfChangeDate DATETIME2(7) = NULL,
@LastKeyRotationDate DATETIME2(7) = NULL,
@LastEmailChangeDate DATETIME2(7) = NULL
@LastEmailChangeDate DATETIME2(7) = NULL,
@VerifyDevices BIT = 1
AS
BEGIN
SET NOCOUNT ON
Expand Down Expand Up @@ -88,7 +89,8 @@ BEGIN
[LastPasswordChangeDate] = @LastPasswordChangeDate,
[LastKdfChangeDate] = @LastKdfChangeDate,
[LastKeyRotationDate] = @LastKeyRotationDate,
[LastEmailChangeDate] = @LastEmailChangeDate
[LastEmailChangeDate] = @LastEmailChangeDate,
[VerifyDevices] = @VerifyDevices
WHERE
[Id] = @Id
END
3 changes: 2 additions & 1 deletion src/Sql/dbo/Tables/User.sql
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,12 @@
[UsesKeyConnector] BIT NOT NULL,
[FailedLoginCount] INT CONSTRAINT [D_User_FailedLoginCount] DEFAULT ((0)) NOT NULL,
[LastFailedLoginDate] DATETIME2 (7) NULL,
[AvatarColor] VARCHAR(7) NULL,
[AvatarColor] VARCHAR(7) NULL,
[LastPasswordChangeDate] DATETIME2 (7) NULL,
[LastKdfChangeDate] DATETIME2 (7) NULL,
[LastKeyRotationDate] DATETIME2 (7) NULL,
[LastEmailChangeDate] DATETIME2 (7) NULL,
[VerifyDevices] BIT DEFAULT ((1)) NOT NULL,
CONSTRAINT [PK_User] PRIMARY KEY CLUSTERED ([Id] ASC)
);

Expand Down
43 changes: 43 additions & 0 deletions test/Api.Test/Auth/Controllers/AccountsControllerTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -563,6 +563,49 @@ public async Task Delete_WhenAccountDeprovisioningIsEnabled_WithUserNotManagedBy
await _userService.Received(1).DeleteAsync(user);
}

[Theory]
[BitAutoData]
public async Task SetVerifyDevices_WhenUserDoesNotExist_ShouldThrowUnauthorizedAccessException(
SetVerifyDevicesRequestModel model)
{
// Arrange
_userService.GetUserByPrincipalAsync(Arg.Any<ClaimsPrincipal>()).Returns(Task.FromResult((User)null));

// Act & Assert
await Assert.ThrowsAsync<UnauthorizedAccessException>(() => _sut.SetUserVerifyDevicesAsync(model));
}

[Theory]
[BitAutoData]
public async Task SetVerifyDevices_WhenInvalidSecret_ShouldFail(
User user, SetVerifyDevicesRequestModel model)
{
// Arrange
_userService.GetUserByPrincipalAsync(Arg.Any<ClaimsPrincipal>()).Returns(Task.FromResult((user)));
_userService.VerifySecretAsync(user, Arg.Any<string>()).Returns(Task.FromResult(false));

// Act & Assert
await Assert.ThrowsAsync<BadRequestException>(() => _sut.SetUserVerifyDevicesAsync(model));
}

[Theory]
[BitAutoData]
public async Task SetVerifyDevices_WhenRequestValid_ShouldSucceed(
User user, SetVerifyDevicesRequestModel model)
{
// Arrange
user.VerifyDevices = false;
model.VerifyDevices = true;
_userService.GetUserByPrincipalAsync(Arg.Any<ClaimsPrincipal>()).Returns(Task.FromResult((user)));
_userService.VerifySecretAsync(user, Arg.Any<string>()).Returns(Task.FromResult(true));

// Act
await _sut.SetUserVerifyDevicesAsync(model);

await _userService.Received(1).SaveUserAsync(user);
Assert.Equal(model.VerifyDevices, user.VerifyDevices);
}

// Below are helper functions that currently belong to this
// test class, but ultimately may need to be split out into
// something greater in order to share common test steps with
Expand Down
24 changes: 24 additions & 0 deletions test/Identity.Test/IdentityServer/DeviceValidatorTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -429,6 +429,30 @@ public async void HandleNewDeviceVerificationAsync_UserNull_ContextModified_Retu
Assert.Equal(expectedErrorMessage, actualResponse.Message);
}

[Theory, BitAutoData]
public async void HandleNewDeviceVerificationAsync_VerifyDevicesFalse_ReturnsSuccess(
CustomValidatorRequestContext context,
[AuthFixtures.ValidatedTokenRequest] ValidatedTokenRequest request)
{
// Arrange
ArrangeForHandleNewDeviceVerificationTest(context, request);
_featureService.IsEnabled(FeatureFlagKeys.NewDeviceVerification).Returns(true);
_globalSettings.EnableNewDeviceVerification = true;
context.User.VerifyDevices = false;

// 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_UserHasCacheValue_ReturnsSuccess(
CustomValidatorRequestContext context,
Expand Down
Loading
Loading