Skip to content

Commit

Permalink
[PM-15614] Allow Users to opt out of new device verification (#5176)
Browse files Browse the repository at this point in the history
feat(NewDeviceVerification) : 
* Created database migration scripts for VerifyDevices column in [dbo].[User].
* Updated DeviceValidator to check if user has opted out of device verification.
* Added endpoint to AccountsController.cs to allow editing of new User.VerifyDevices property.
* Added tests for new methods and endpoint.
* Updating queries to track [dbo].[User].[VerifyDevices].
* Updated DeviceValidator to set `User.EmailVerified` property during the New Device Verification flow.
  • Loading branch information
ike-kottlowski authored Jan 8, 2025
1 parent 481a766 commit a84ef07
Show file tree
Hide file tree
Showing 21 changed files with 9,459 additions and 9 deletions.
19 changes: 18 additions & 1 deletion src/Api/Auth/Controllers/AccountsController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -969,11 +969,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
14 changes: 13 additions & 1 deletion src/Identity/IdentityServer/RequestValidators/DeviceValidator.cs
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
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public async Task SignUp_PM_Family_Passes(PlanType planType, OrganizationSignup
signup.PremiumAccessAddon = false;
signup.UseSecretsManager = false;
signup.IsFromSecretsManagerTrial = false;
signup.IsFromProvider = false;

var result = await sutProvider.Sut.SignUpOrganizationAsync(signup);

Expand Down Expand Up @@ -85,6 +86,7 @@ public async Task SignUp_AssignsOwnerToDefaultCollection
signup.PaymentMethodType = PaymentMethodType.Card;
signup.PremiumAccessAddon = false;
signup.UseSecretsManager = false;
signup.IsFromProvider = false;

// Extract orgUserId when created
Guid? orgUserId = null;
Expand Down Expand Up @@ -128,6 +130,8 @@ public async Task SignUp_SM_Passes(PlanType planType, OrganizationSignup signup,
signup.PaymentMethodType = PaymentMethodType.Card;
signup.PremiumAccessAddon = false;
signup.IsFromSecretsManagerTrial = false;
signup.IsFromProvider = false;


var result = await sutProvider.Sut.SignUpOrganizationAsync(signup);

Expand Down Expand Up @@ -196,6 +200,7 @@ public async Task SignUpAsync_SecretManager_AdditionalServiceAccounts_NotAllowed
signup.PremiumAccessAddon = false;
signup.AdditionalServiceAccounts = 10;
signup.AdditionalStorageGb = 0;
signup.IsFromProvider = false;

var exception = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SignUpOrganizationAsync(signup));
Expand All @@ -213,6 +218,7 @@ public async Task SignUpAsync_SMSeatsGreatThanPMSeat_ShouldThrowException(Organi
signup.PaymentMethodType = PaymentMethodType.Card;
signup.PremiumAccessAddon = false;
signup.AdditionalServiceAccounts = 10;
signup.IsFromProvider = false;

var exception = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SignUpOrganizationAsync(signup));
Expand All @@ -230,6 +236,7 @@ public async Task SignUpAsync_InvalidateServiceAccount_ShouldThrowException(Orga
signup.PaymentMethodType = PaymentMethodType.Card;
signup.PremiumAccessAddon = false;
signup.AdditionalServiceAccounts = -10;
signup.IsFromProvider = false;

var exception = await Assert.ThrowsAsync<BadRequestException>(
() => sutProvider.Sut.SignUpOrganizationAsync(signup));
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

0 comments on commit a84ef07

Please sign in to comment.