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

[EC-400] Code clean up Device Verification #2601

Merged
merged 4 commits into from
Feb 17, 2023
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
63 changes: 10 additions & 53 deletions src/Api/Controllers/TwoFactorController.cs
Original file line number Diff line number Diff line change
Expand Up @@ -295,21 +295,14 @@ public async Task SendEmailLogin([FromBody] TwoFactorEmailRequestModel model)
if (await _verifyAuthRequestCommand
.VerifyAuthRequestAsync(new Guid(model.AuthRequestId), model.AuthRequestAccessCode))
{
var isBecauseNewDeviceLogin = await IsNewDeviceLoginAsync(user, model);

await _userService.SendTwoFactorEmailAsync(user, isBecauseNewDeviceLogin);
await _userService.SendTwoFactorEmailAsync(user);
return;
}
}
else
else if (await _userService.VerifySecretAsync(user, model.Secret))
{
if (await _userService.VerifySecretAsync(user, model.Secret))
{
var isBecauseNewDeviceLogin = await IsNewDeviceLoginAsync(user, model);

await _userService.SendTwoFactorEmailAsync(user, isBecauseNewDeviceLogin);
return;
}
await _userService.SendTwoFactorEmailAsync(user);
return;
}
}

Expand Down Expand Up @@ -390,41 +383,18 @@ public async Task PostRecover([FromBody] TwoFactorRecoveryRequestModel model)
}
}

[Obsolete("Leaving this for backwards compatibilty on clients")]
[HttpGet("get-device-verification-settings")]
public async Task<DeviceVerificationResponseModel> GetDeviceVerificationSettings()
public Task<DeviceVerificationResponseModel> GetDeviceVerificationSettings()
{
var user = await _userService.GetUserByPrincipalAsync(User);
if (user == null)
{
throw new UnauthorizedAccessException();
}

if (User.Claims.HasSsoIdP())
{
return new DeviceVerificationResponseModel(false, false);
}

var canUserEditDeviceVerificationSettings = _userService.CanEditDeviceVerificationSettings(user);
return new DeviceVerificationResponseModel(canUserEditDeviceVerificationSettings, canUserEditDeviceVerificationSettings && user.UnknownDeviceVerificationEnabled);
return Task.FromResult(new DeviceVerificationResponseModel(false, false));
}

[Obsolete("Leaving this for backwards compatibilty on clients")]
[HttpPut("device-verification-settings")]
public async Task<DeviceVerificationResponseModel> PutDeviceVerificationSettings([FromBody] DeviceVerificationRequestModel model)
public Task<DeviceVerificationResponseModel> PutDeviceVerificationSettings([FromBody] DeviceVerificationRequestModel model)
{
var user = await _userService.GetUserByPrincipalAsync(User);
if (user == null)
{
throw new UnauthorizedAccessException();
}
if (!_userService.CanEditDeviceVerificationSettings(user)
|| User.Claims.HasSsoIdP())
{
throw new InvalidOperationException("Can't update device verification settings");
}

model.ToUser(user);
await _userService.SaveUserAsync(user);
return new DeviceVerificationResponseModel(true, user.UnknownDeviceVerificationEnabled);
return Task.FromResult(new DeviceVerificationResponseModel(false, false));
}

private async Task<User> CheckAsync(SecretVerificationRequestModel model, bool premium)
Expand Down Expand Up @@ -467,17 +437,4 @@ private async Task ValidateYubiKeyAsync(User user, string name, string value)
await Task.Delay(500);
}
}

private async Task<bool> IsNewDeviceLoginAsync(User user, TwoFactorEmailRequestModel model)
{
if (user.GetTwoFactorProvider(TwoFactorProviderType.Email) is null
&&
await _userService.Needs2FABecauseNewDeviceAsync(user, model.DeviceIdentifier, null))
{
model.ToUser(user);
return true;
}

return false;
}
}
8 changes: 1 addition & 7 deletions src/Api/Models/Request/DeviceVerificationRequestModel.cs
Original file line number Diff line number Diff line change
@@ -1,16 +1,10 @@
using System.ComponentModel.DataAnnotations;
using Bit.Core.Entities;

namespace Bit.Api.Models.Request;

public class DeviceVerificationRequestModel
{
[Obsolete("Leaving this for backwards compatibilty on clients")]
[Required]
public bool UnknownDeviceVerificationEnabled { get; set; }

public User ToUser(User user)
{
user.UnknownDeviceVerificationEnabled = UnknownDeviceVerificationEnabled;
return user;
}
}
2 changes: 0 additions & 2 deletions src/Api/Models/Request/TwoFactorRequestModels.cs
Original file line number Diff line number Diff line change
Expand Up @@ -202,8 +202,6 @@ public class TwoFactorEmailRequestModel : SecretVerificationRequestModel
[StringLength(256)]
public string Email { get; set; }

public string DeviceIdentifier { get; set; }

public string AuthRequestId { get; set; }

public User ToUser(User extistingUser)
Expand Down
1 change: 1 addition & 0 deletions src/Api/Models/Response/DeviceVerificationResponseModel.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

namespace Bit.Api.Models.Response;

[Obsolete("Leaving this for backwards compatibilty on clients")]
public class DeviceVerificationResponseModel : ResponseModel
{
public DeviceVerificationResponseModel(bool isDeviceVerificationSectionEnabled, bool unknownDeviceVerificationEnabled)
Expand Down
1 change: 0 additions & 1 deletion src/Core/Entities/User.cs
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ public class User : ITableObject<Guid>, ISubscriber, IStorable, IStorableSubscri
public bool UsesKeyConnector { get; set; }
public int FailedLoginCount { get; set; }
public DateTime? LastFailedLoginDate { get; set; }
public bool UnknownDeviceVerificationEnabled { get; set; }
[MaxLength(7)]
public string AvatarColor { get; set; }
public DateTime? LastPasswordChangeDate { get; set; }
Expand Down

This file was deleted.

This file was deleted.

1 change: 0 additions & 1 deletion src/Core/Services/IMailService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ public interface IMailService
Task SendChangeEmailAlreadyExistsEmailAsync(string fromEmail, string toEmail);
Task SendChangeEmailEmailAsync(string newEmailAddress, string token);
Task SendTwoFactorEmailAsync(string email, string token);
Task SendNewDeviceLoginTwoFactorEmailAsync(string email, string token);
Task SendNoMasterPasswordHintEmailAsync(string email);
Task SendMasterPasswordHintEmailAsync(string email, string hint);
Task SendOrganizationInviteEmailAsync(string organizationName, OrganizationUser orgUser, ExpiringToken token);
Expand Down
4 changes: 1 addition & 3 deletions src/Core/Services/IUserService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ public interface IUserService
Task<IdentityResult> RegisterUserAsync(User user, string masterPassword, string token, Guid? orgUserId);
Task<IdentityResult> RegisterUserAsync(User user);
Task SendMasterPasswordHintAsync(string email);
Task SendTwoFactorEmailAsync(User user, bool isBecauseNewDeviceLogin = false);
Task SendTwoFactorEmailAsync(User user);
Task<bool> VerifyTwoFactorEmailAsync(User user, string token);
Task<CredentialCreateOptions> StartWebAuthnRegistrationAsync(User user);
Task<bool> DeleteWebAuthnKeyAsync(User user, int id);
Expand Down Expand Up @@ -76,6 +76,4 @@ Task<UserLicense> GenerateLicenseAsync(User user, SubscriptionInfo subscriptionI
Task SendOTPAsync(User user);
Task<bool> VerifyOTPAsync(User user, string token);
Task<bool> VerifySecretAsync(User user, string secret);
Task<bool> Needs2FABecauseNewDeviceAsync(User user, string deviceIdentifier, string grantType);
bool CanEditDeviceVerificationSettings(User user);
}
Original file line number Diff line number Diff line change
Expand Up @@ -323,7 +323,6 @@ public async Task PasswordAsync(Guid id, User requestingUser, string newMasterPa
grantor.Key = key;
// Disable TwoFactor providers since they will otherwise block logins
grantor.SetTwoFactorProviders(new Dictionary<TwoFactorProviderType, TwoFactorProvider>());
grantor.UnknownDeviceVerificationEnabled = false;
await _userRepository.ReplaceAsync(grantor);

// Remove grantor from all organizations unless Owner
Expand Down
15 changes: 0 additions & 15 deletions src/Core/Services/Implementations/HandlebarsMailService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -113,21 +113,6 @@ public async Task SendTwoFactorEmailAsync(string email, string token)
await _mailDeliveryService.SendEmailAsync(message);
}

public async Task SendNewDeviceLoginTwoFactorEmailAsync(string email, string token)
{
var message = CreateDefaultMessage("New Device Login Verification Code", email);
var model = new EmailTokenViewModel
{
Token = token,
WebVaultUrl = _globalSettings.BaseServiceUri.VaultWithHash,
SiteName = _globalSettings.SiteName
};
await AddMessageContentAsync(message, "NewDeviceLoginTwoFactorEmail", model);
message.MetaData.Add("SendGridBypassListManagement", true);
message.Category = "TwoFactorEmail";
await _mailDeliveryService.SendEmailAsync(message);
}

public async Task SendMasterPasswordHintEmailAsync(string email, string hint)
{
var message = CreateDefaultMessage("Your Master Password Hint", email);
Expand Down
46 changes: 2 additions & 44 deletions src/Core/Services/Implementations/UserService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ public class UserService : UserManager<User>, IUserService, IDisposable
private readonly IGlobalSettings _globalSettings;
private readonly IOrganizationService _organizationService;
private readonly IProviderUserRepository _providerUserRepository;
private readonly IDeviceRepository _deviceRepository;
private readonly IStripeSyncService _stripeSyncService;

public UserService(
Expand Down Expand Up @@ -77,7 +76,6 @@ public UserService(
IGlobalSettings globalSettings,
IOrganizationService organizationService,
IProviderUserRepository providerUserRepository,
IDeviceRepository deviceRepository,
IStripeSyncService stripeSyncService)
: base(
store,
Expand Down Expand Up @@ -113,7 +111,6 @@ public UserService(
_globalSettings = globalSettings;
_organizationService = organizationService;
_providerUserRepository = providerUserRepository;
_deviceRepository = deviceRepository;
_stripeSyncService = stripeSyncService;
}

Expand Down Expand Up @@ -353,7 +350,7 @@ public async Task SendMasterPasswordHintAsync(string email)
await _mailService.SendMasterPasswordHintEmailAsync(email, user.MasterPasswordHint);
}

public async Task SendTwoFactorEmailAsync(User user, bool isBecauseNewDeviceLogin = false)
public async Task SendTwoFactorEmailAsync(User user)
{
var provider = user.GetTwoFactorProvider(TwoFactorProviderType.Email);
if (provider == null || provider.MetaData == null || !provider.MetaData.ContainsKey("Email"))
Expand All @@ -365,14 +362,7 @@ public async Task SendTwoFactorEmailAsync(User user, bool isBecauseNewDeviceLogi
var token = await base.GenerateUserTokenAsync(user, TokenOptions.DefaultEmailProvider,
"2faEmail:" + email);

if (isBecauseNewDeviceLogin)
{
await _mailService.SendNewDeviceLoginTwoFactorEmailAsync(email, token);
}
else
{
await _mailService.SendTwoFactorEmailAsync(email, token);
}
await _mailService.SendTwoFactorEmailAsync(email, token);
}

public async Task<bool> VerifyTwoFactorEmailAsync(User user, string token)
Expand Down Expand Up @@ -1478,36 +1468,4 @@ public async Task<bool> VerifySecretAsync(User user, string secret)
? await VerifyOTPAsync(user, secret)
: await CheckPasswordAsync(user, secret);
}

public async Task<bool> Needs2FABecauseNewDeviceAsync(User user, string deviceIdentifier, string grantType)
{
return CanEditDeviceVerificationSettings(user)
&& user.UnknownDeviceVerificationEnabled
&& grantType != "authorization_code"
&& await IsNewDeviceAndNotTheFirstOneAsync(user, deviceIdentifier);
}

public bool CanEditDeviceVerificationSettings(User user)
{
return _globalSettings.TwoFactorAuth.EmailOnNewDeviceLogin
&& user.EmailVerified
&& !user.UsesKeyConnector
&& !(user.GetTwoFactorProviders()?.Any() ?? false);
}

private async Task<bool> IsNewDeviceAndNotTheFirstOneAsync(User user, string deviceIdentifier)
{
if (user == null)
{
return default;
}

var devices = await _deviceRepository.GetManyByUserIdAsync(user.Id);
if (!devices.Any())
{
return false;
}

return !devices.Any(d => d.Identifier == deviceIdentifier);
}
}
5 changes: 0 additions & 5 deletions src/Core/Services/NoopImplementations/NoopMailService.cs
Original file line number Diff line number Diff line change
Expand Up @@ -72,11 +72,6 @@ public Task SendTwoFactorEmailAsync(string email, string token)
return Task.FromResult(0);
}

public Task SendNewDeviceLoginTwoFactorEmailAsync(string email, string token)
{
return Task.CompletedTask;
}

public Task SendWelcomeEmailAsync(User user)
{
return Task.FromResult(0);
Expand Down
6 changes: 0 additions & 6 deletions src/Core/Settings/GlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,6 @@ public virtual string LicenseDirectory
public virtual AppleIapSettings AppleIap { get; set; } = new AppleIapSettings();
public virtual ISsoSettings Sso { get; set; } = new SsoSettings();
public virtual StripeSettings Stripe { get; set; } = new StripeSettings();
public virtual ITwoFactorAuthSettings TwoFactorAuth { get; set; } = new TwoFactorAuthSettings();
public virtual DistributedIpRateLimitingSettings DistributedIpRateLimiting { get; set; } =
new DistributedIpRateLimitingSettings();
public virtual IPasswordlessAuthSettings PasswordlessAuth { get; set; } = new PasswordlessAuthSettings();
Expand Down Expand Up @@ -510,11 +509,6 @@ public class StripeSettings
public int MaxNetworkRetries { get; set; } = 2;
}

public class TwoFactorAuthSettings : ITwoFactorAuthSettings
{
public bool EmailOnNewDeviceLogin { get; set; } = false;
}

public class DistributedIpRateLimitingSettings
{
public bool Enabled { get; set; } = true;
Expand Down
1 change: 0 additions & 1 deletion src/Core/Settings/IGlobalSettings.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ public interface IGlobalSettings
IFileStorageSettings Attachment { get; set; }
IConnectionStringSettings Storage { get; set; }
IBaseServiceUriSettings BaseServiceUri { get; set; }
ITwoFactorAuthSettings TwoFactorAuth { get; set; }
ISsoSettings Sso { get; set; }
ILogLevelSettings MinLogLevel { get; set; }
IPasswordlessAuthSettings PasswordlessAuth { get; set; }
Expand Down
6 changes: 0 additions & 6 deletions src/Core/Settings/ITwoFactorAuthSettings.cs

This file was deleted.

Loading