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-2998] Move Approving Device Check #3101

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
10 changes: 0 additions & 10 deletions src/Api/Controllers/DevicesController.cs
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
using Bit.Api.Models.Request;
using Bit.Api.Models.Response;
using Bit.Core.Entities;
using Bit.Core.Enums;
using Bit.Core.Exceptions;
using Bit.Core.Repositories;
using Bit.Core.Services;
Expand Down Expand Up @@ -66,15 +65,6 @@ public async Task<ListResponseModel<DeviceResponseModel>> Get()
return new ListResponseModel<DeviceResponseModel>(responses);
}

[HttpPost("exist-by-types")]
public async Task<ActionResult<bool>> GetExistenceByTypes([FromBody] DeviceType[] deviceTypes)
{
var userId = _userService.GetProperUserId(User).Value;
var devices = await _deviceRepository.GetManyByUserIdAsync(userId);
var userHasDeviceOfTypes = devices.Any(d => deviceTypes.Contains(d.Type));
return Ok(userHasDeviceOfTypes);
}

[HttpPost("")]
public async Task<DeviceResponseModel> Post([FromBody] DeviceRequestModel model)
{
Expand Down
7 changes: 6 additions & 1 deletion src/Core/Auth/Models/Api/Response/UserDecryptionOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,17 @@ public UserDecryptionOptions() : base("userDecryptionOptions")
public class TrustedDeviceUserDecryptionOption
{
public bool HasAdminApproval { get; }
public bool HasLoginApprovingDevice { get; }
public string? EncryptedPrivateKey { get; }
public string? EncryptedUserKey { get; }

public TrustedDeviceUserDecryptionOption(bool hasAdminApproval, string? encryptedPrivateKey, string? encryptedUserKey)
public TrustedDeviceUserDecryptionOption(bool hasAdminApproval,
bool hasLoginApprovingDevice,
string? encryptedPrivateKey,
string? encryptedUserKey)
{
HasAdminApproval = hasAdminApproval;
HasLoginApprovingDevice = hasLoginApprovingDevice;
EncryptedPrivateKey = encryptedPrivateKey;
EncryptedUserKey = encryptedUserKey;
}
Expand Down
21 changes: 21 additions & 0 deletions src/Core/Utilities/DeviceTypes.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
using Bit.Core.Enums;

namespace Bit.Core.Utilities;

public static class DeviceTypes
{
public static IReadOnlyCollection<DeviceType> MobileTypes { get; } = new[]
{
DeviceType.Android,
DeviceType.iOS,
DeviceType.AndroidAmazon,
};

public static IReadOnlyCollection<DeviceType> DesktopTypes { get; } = new[]
{
DeviceType.LinuxDesktop,
DeviceType.MacOsDesktop,
DeviceType.WindowsDesktop,
DeviceType.UWP,
};
}
14 changes: 13 additions & 1 deletion src/Identity/IdentityServer/BaseRequestValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
using Bit.Core.Settings;
using Bit.Core.Tokens;
using Bit.Core.Utilities;
using Bit.Identity.Utilities;
using IdentityServer4.Validation;
using Microsoft.AspNetCore.Identity;

Expand Down Expand Up @@ -614,10 +615,21 @@ private async Task<UserDecryptionOptions> CreateUserDecryptionOptionsAsync(User
encryptedPrivateKey = device.EncryptedPrivateKey;
encryptedUserKey = device.EncryptedUserKey;
}

var allDevices = await _deviceRepository.GetManyByUserIdAsync(user.Id);
// Checks if the current user has any devices that are capable of approving login with device requests except for
justindbaur marked this conversation as resolved.
Show resolved Hide resolved
// their current device.
// NOTE: this doesn't check for if the users have configured the devices to be capable of approving requests as that is a client side setting.
var hasLoginApprovingDevice = allDevices
.Where(d => d.Identifier != device.Identifier && LoginApprovingDeviceTypes.Types.Contains(d.Type))
.Any();

var hasAdminApproval = await PolicyService.AnyPoliciesApplicableToUserAsync(user.Id, PolicyType.ResetPassword);
// TrustedDeviceEncryption only exists for SSO, but if that ever changes this value won't always be true
userDecryptionOption.TrustedDeviceOption = new TrustedDeviceUserDecryptionOption(hasAdminApproval,
encryptedPrivateKey, encryptedUserKey);
hasLoginApprovingDevice,
encryptedPrivateKey,
encryptedUserKey);
}

return userDecryptionOption;
Expand Down
19 changes: 19 additions & 0 deletions src/Identity/Utilities/LoginApprovingDeviceTypes.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
using Bit.Core.Enums;
using Bit.Core.Utilities;

namespace Bit.Identity.Utilities;

public static class LoginApprovingDeviceTypes
{
private static readonly IReadOnlyCollection<DeviceType> _deviceTypes;

static LoginApprovingDeviceTypes()
{
var deviceTypes = new List<DeviceType>();
deviceTypes.AddRange(DeviceTypes.DesktopTypes);
deviceTypes.AddRange(DeviceTypes.MobileTypes);
_deviceTypes = deviceTypes.AsReadOnly();
}

public static IReadOnlyCollection<DeviceType> Types => _deviceTypes;
}
80 changes: 78 additions & 2 deletions test/Identity.IntegrationTest/Endpoints/IdentityServerSsoTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -237,7 +237,8 @@ public async Task SsoLogin_TrustedDeviceEncryptionAndNoMasterPassword_ReturnsOne
// "Object": "userDecryptionOptions"
// "HasMasterPassword": false,
// "TrustedDeviceOption": {
// "HasAdminApproval": true
// "HasAdminApproval": true,
// "HasLoginApprovingDevice": false
// }
// }

Expand All @@ -247,7 +248,82 @@ public async Task SsoLogin_TrustedDeviceEncryptionAndNoMasterPassword_ReturnsOne
// This asserts that device keys are not coming back in the response because this should be a new device.
// if we ever add new properties that come back from here it is fine to change the expected number of properties
// but it should still be asserted in some way that keys are not amongst them.
Assert.Single(trustedDeviceOption.EnumerateObject());
Assert.Collection(trustedDeviceOption.EnumerateObject(),
p => { Assert.Equal("HasAdminApproval", p.Name); Assert.Equal(JsonValueKind.False, p.Value.ValueKind); },
p => { Assert.Equal("HasLoginApprovingDevice", p.Name); Assert.Equal(JsonValueKind.False, p.Value.ValueKind); });
}

/// <summary>
/// If a user has a device that is able to accept login with device requests, we should return that state
/// with the user decryption options.
/// </summary>
[Fact]
public async Task SsoLogin_TrustedDeviceEncryptionAndNoMasterPassword_HasLoggingApprovingDevice_ReturnsTrue()
{
// Arrange
var challenge = new string('c', 50);
var factory = await CreateFactoryAsync(new SsoConfigurationData
{
MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption,
}, challenge);

await UpdateUserAsync(factory, user => user.MasterPassword = null);
var userRepository = factory.Services.GetRequiredService<IUserRepository>();
var user = await userRepository.GetByEmailAsync(TestEmail);

var deviceRepository = factory.Services.GetRequiredService<IDeviceRepository>();
await deviceRepository.CreateAsync(new Device
{
Identifier = "my_other_device",
Type = DeviceType.Android,
Name = "Android",
UserId = user.Id,
});

// Act
var context = await factory.Server.PostAsync("/connect/token", new FormUrlEncodedContent(new Dictionary<string, string>
{
{ "scope", "api offline_access" },
{ "client_id", "web" },
{ "deviceType", "10" },
{ "deviceIdentifier", "test_id" },
{ "deviceName", "firefox" },
{ "twoFactorToken", "TEST"},
{ "twoFactorProvider", "5" }, // RememberMe Provider
{ "twoFactorRemember", "0" },
{ "grant_type", "authorization_code" },
{ "code", "test_code" },
{ "code_verifier", challenge },
{ "redirect_uri", "https://localhost:8080/sso-connector.html" }
}));

// Assert
// If the organization has selected TrustedDeviceEncryption but the user still has their master password
// they can decrypt with either option
Assert.Equal(StatusCodes.Status200OK, context.Response.StatusCode);
using var responseBody = await AssertHelper.AssertResponseTypeIs<JsonDocument>(context);
var root = responseBody.RootElement;
AssertHelper.AssertJsonProperty(root, "access_token", JsonValueKind.String);
var userDecryptionOptions = AssertHelper.AssertJsonProperty(root, "UserDecryptionOptions", JsonValueKind.Object);

// Expected to look like:
// "UserDecryptionOptions": {
// "Object": "userDecryptionOptions"
// "HasMasterPassword": false,
// "TrustedDeviceOption": {
// "HasAdminApproval": true,
// "HasLoginApprovingDevice": true
// }
// }

var trustedDeviceOption = AssertHelper.AssertJsonProperty(userDecryptionOptions, "TrustedDeviceOption", JsonValueKind.Object);

// This asserts that device keys are not coming back in the response because this should be a new device.
// if we ever add new properties that come back from here it is fine to change the expected number of properties
// but it should still be asserted in some way that keys are not amongst them.
Assert.Collection(trustedDeviceOption.EnumerateObject(),
p => { Assert.Equal("HasAdminApproval", p.Name); Assert.Equal(JsonValueKind.False, p.Value.ValueKind); },
p => { Assert.Equal("HasLoginApprovingDevice", p.Name); Assert.Equal(JsonValueKind.True, p.Value.ValueKind); });
}

/// <summary>
Expand Down