fix(user-decryption-options) [PM-23174]: ManageAccountRecovery Permission Forces Master Password Set (#6230)

* fix(user-decryption-options): ManageAccountRecovery Permission Forces MP Set - Update tests, add OrganizationUser fixture customization for Permissions

* fix(user-decryption-options): ManageAccountRecovery Permission Forces MP Set - Update hasManageResetPasswordPermission evaluation.

* PM-23174 - Add TODO for endpoint per sync discussion with Dave

* fix(user-decryption-options): ManageAccountRecovery Permission Forces MP Set - Clean up comments.

* fix(user-decryption-options): ManageAccountRecovery Permission Forces MP Set - Remove an outdated comment.

* fix(user-decryption-options): ManageAccountRecovery Permission Forces MP Set - Elaborate on comments around Organization User invite-time evaluation.

* fix(user-decryption-options): Use currentContext for Provider relationships, update comments, and feature flag the change.

* fix(user-decryption-options): Update test suite and provide additional comments for future flag removal.

---------

Co-authored-by: Jared Snider <jsnider@bitwarden.com>

src/Core/Constants.cs

@@ -144,6 +144,8 @@ public static class FeatureFlagKeys
     public const string Otp6Digits = "pm-18612-otp-6-digits";
     public const string FailedTwoFactorEmail = "pm-24425-send-2fa-failed-email";
     public const string DisableAlternateLoginMethods = "pm-22110-disable-alternate-login-methods";
+    public const string PM23174ManageAccountRecoveryPermissionDrivesTheNeedToSetMasterPassword =
+        "pm-23174-manage-account-recovery-permission-drives-the-need-to-set-master-password";
 
     /* Autofill Team */
     public const string IdpAutoSubmitLogin = "idp-auto-submit-login";

src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs

@@ -1,4 +1,5 @@
-using Bit.Core.Auth.Entities;
+using Bit.Core;
+using Bit.Core.Auth.Entities;
 using Bit.Core.Auth.Enums;
 using Bit.Core.Auth.Models.Api.Response;
 using Bit.Core.Auth.Utilities;
@@ -7,6 +8,7 @@ using Bit.Core.Entities;
 using Bit.Core.Enums;
 using Bit.Core.KeyManagement.Models.Response;
 using Bit.Core.Repositories;
+using Bit.Core.Services;
 using Bit.Core.Utilities;
 using Bit.Identity.Utilities;
 
@@ -24,6 +26,7 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
     private readonly IDeviceRepository _deviceRepository;
     private readonly IOrganizationUserRepository _organizationUserRepository;
     private readonly ILoginApprovingClientTypes _loginApprovingClientTypes;
+    private readonly IFeatureService _featureService;
 
     private UserDecryptionOptions _options = new UserDecryptionOptions();
     private User _user = null!;
@@ -34,13 +37,15 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
         ICurrentContext currentContext,
         IDeviceRepository deviceRepository,
         IOrganizationUserRepository organizationUserRepository,
-        ILoginApprovingClientTypes loginApprovingClientTypes
+        ILoginApprovingClientTypes loginApprovingClientTypes,
+        IFeatureService featureService
     )
     {
         _currentContext = currentContext;
         _deviceRepository = deviceRepository;
         _organizationUserRepository = organizationUserRepository;
         _loginApprovingClientTypes = loginApprovingClientTypes;
+        _featureService = featureService;
     }
 
     public IUserDecryptionOptionsBuilder ForUser(User user)
@@ -65,8 +70,10 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
     {
         if (credential.GetPrfStatus() == WebAuthnPrfStatus.Enabled)
         {
-            _options.WebAuthnPrfOption = new WebAuthnPrfDecryptionOption(credential.EncryptedPrivateKey, credential.EncryptedUserKey);
+            _options.WebAuthnPrfOption =
+                new WebAuthnPrfDecryptionOption(credential.EncryptedPrivateKey, credential.EncryptedUserKey);
         }
+
         return this;
     }
 
@@ -74,7 +81,7 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
     {
         BuildMasterPasswordUnlock();
         BuildKeyConnectorOptions();
-        await BuildTrustedDeviceOptions();
+        await BuildTrustedDeviceOptionsAsync();
 
         return _options;
     }
@@ -87,13 +94,14 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
         }
 
         var ssoConfigurationData = _ssoConfig.GetData();
-        if (ssoConfigurationData is { MemberDecryptionType: MemberDecryptionType.KeyConnector } && !string.IsNullOrEmpty(ssoConfigurationData.KeyConnectorUrl))
+        if (ssoConfigurationData is { MemberDecryptionType: MemberDecryptionType.KeyConnector } &&
+            !string.IsNullOrEmpty(ssoConfigurationData.KeyConnectorUrl))
         {
             _options.KeyConnectorOption = new KeyConnectorUserDecryptionOption(ssoConfigurationData.KeyConnectorUrl);
         }
     }
 
-    private async Task BuildTrustedDeviceOptions()
+    private async Task BuildTrustedDeviceOptionsAsync()
     {
         // TrustedDeviceEncryption only exists for SSO, if that changes then these guards should change
         if (_ssoConfig == null)
@@ -101,7 +109,8 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
             return;
         }
 
-        var isTdeActive = _ssoConfig.GetData() is { MemberDecryptionType: MemberDecryptionType.TrustedDeviceEncryption };
+        var isTdeActive = _ssoConfig.GetData() is
+        { MemberDecryptionType: MemberDecryptionType.TrustedDeviceEncryption };
         var isTdeOffboarding = !_user.HasMasterPassword() && _device != null && _device.IsTrusted() && !isTdeActive;
         if (!isTdeActive && !isTdeOffboarding)
         {
@@ -120,25 +129,51 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
         if (_device != null)
         {
             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
-            // 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.
-            hasLoginApprovingDevice = allDevices.Any(d => d.Identifier != _device.Identifier && _loginApprovingClientTypes.TypesThatCanApprove.Contains(DeviceTypes.ToClientType(d.Type)));
+            // Checks if the current user has any devices that are capable of approving login with device requests
+            // except for their current device.
+            hasLoginApprovingDevice = allDevices.Any(d =>
+                d.Identifier != _device.Identifier &&
+                _loginApprovingClientTypes.TypesThatCanApprove.Contains(DeviceTypes.ToClientType(d.Type)));
         }
 
-        // Determine if user has manage reset password permission as post sso logic requires it for forcing users with this permission to set a MP
+        // Just-in-time-provisioned users, which can include users invited to a TDE organization with SSO and granted
+        // the Admin/Owner role or Custom user role with ManageResetPassword permission, will not have claims available
+        // in context to reflect this permission if granted as part of an invite for the current organization.
+        // Therefore, as written today, CurrentContext will not surface those permissions for those users.
+        // In order to make this check accurate at first login for all applicable cases, we have to go back to the
+        // database record.
+        // In the TDE flow, the users will have been JIT-provisioned at SSO callback time, and the relationship between
+        // user and organization user will have been codified.
+        var organizationUser = await _organizationUserRepository.GetByOrganizationAsync(_ssoConfig.OrganizationId, _user.Id);
         var hasManageResetPasswordPermission = false;
-        // when a user is being created via JIT provisioning, they will not have any orgs so we can't assume we will have orgs here
-        if (_currentContext.Organizations != null && _currentContext.Organizations.Any(o => o.Id == _ssoConfig.OrganizationId))
+        if (_featureService.IsEnabled(FeatureFlagKeys.PM23174ManageAccountRecoveryPermissionDrivesTheNeedToSetMasterPassword))
         {
-            // TDE requires single org so grabbing first org & id is fine.
-            hasManageResetPasswordPermission = await _currentContext.ManageResetPassword(_ssoConfig!.OrganizationId);
+            hasManageResetPasswordPermission = await EvaluateHasManageResetPasswordPermission();
         }
+        else
+        {
+            // TODO: PM-26065 remove use of above feature flag from the server, and remove this branching logic, which
+            // has been replaced by EvaluateHasManageResetPasswordPermission.
+            // Determine if user has manage reset password permission as post sso logic requires it for forcing users with this permission to set a MP.
+            // When removing feature flags, please also see notes and removals intended for test suite in
+            // Build_WhenManageResetPasswordPermissions_ShouldReturnHasManageResetPasswordPermissionTrue.
+
+            // when a user is being created via JIT provisioning, they will not have any orgs so we can't assume we will have orgs here
+            if (_currentContext.Organizations != null && _currentContext.Organizations.Any(o => o.Id == _ssoConfig.OrganizationId))
+            {
+                // TDE requires single org so grabbing first org & id is fine.
+                hasManageResetPasswordPermission = await _currentContext.ManageResetPassword(_ssoConfig!.OrganizationId);
+            }
 
-        // If sso configuration data is not null then I know for sure that ssoConfiguration isn't null
-        var organizationUser = await _organizationUserRepository.GetByOrganizationAsync(_ssoConfig.OrganizationId, _user.Id);
+            // If sso configuration data is not null then I know for sure that ssoConfiguration isn't null
+
+            // NOTE: Commented from original impl because the organization user repository call has been hoisted to support
+            // branching paths through flagging.
+            //organizationUser = await _organizationUserRepository.GetByOrganizationAsync(_ssoConfig.OrganizationId, _user.Id);
+
+            hasManageResetPasswordPermission |= organizationUser != null && (organizationUser.Type == OrganizationUserType.Owner || organizationUser.Type == OrganizationUserType.Admin);
+        }
 
-        hasManageResetPasswordPermission |= organizationUser != null && (organizationUser.Type == OrganizationUserType.Owner || organizationUser.Type == OrganizationUserType.Admin);
         // They are only able to be approved by an admin if they have enrolled is reset password
         var hasAdminApproval = organizationUser != null && !string.IsNullOrEmpty(organizationUser.ResetPasswordKey);
 
@@ -149,6 +184,31 @@ public class UserDecryptionOptionsBuilder : IUserDecryptionOptionsBuilder
             isTdeOffboarding,
             encryptedPrivateKey,
             encryptedUserKey);
+        return;
+
+        async Task<bool> EvaluateHasManageResetPasswordPermission()
+        {
+            // PM-23174
+            // Determine if user has manage reset password permission as post sso logic requires it for forcing users with this permission to set a MP
+            if (organizationUser == null)
+            {
+                return false;
+            }
+
+            var organizationUserHasResetPasswordPermission =
+                // The repository will pull users in all statuses, so we also need to ensure that revoked-status users do not have
+                // permissions sent down.
+                organizationUser.Status is OrganizationUserStatusType.Invited or OrganizationUserStatusType.Accepted or
+                    OrganizationUserStatusType.Confirmed &&
+                // Admins and owners get ManageResetPassword functionally "for free" through their role.
+                (organizationUser.Type is OrganizationUserType.Admin or OrganizationUserType.Owner ||
+                 // Custom users can have the ManagePasswordReset permission assigned directly.
+                 organizationUser.GetPermissions() is { ManageResetPassword: true });
+
+            return organizationUserHasResetPasswordPermission ||
+                   // A provider user for the given organization gets ManageResetPassword through that relationship.
+                   await _currentContext.ProviderUserForOrgAsync(_ssoConfig.OrganizationId);
+        }
     }
 
     private void BuildMasterPasswordUnlock()

test/Identity.Test/AutoFixture/OrganizationUserFixtures.cs

@@ -0,0 +1,26 @@
+using System.Reflection;
+using AutoFixture;
+using AutoFixture.Xunit2;
+using Bit.Core.Entities;
+using Bit.Core.Models.Data;
+using Bit.Core.Utilities;
+
+namespace Bit.Identity.Test.AutoFixture;
+
+internal class OrganizationUserWithDefaultPermissionsCustomization : ICustomization
+{
+    public void Customize(IFixture fixture)
+    {
+        fixture.Customize<OrganizationUser>(composer => composer
+            // On OrganizationUser, Permissions can be JSON data (as string) or sometimes null.
+            // Entity APIs should prevent it from being anything else.
+            // An un-modified fixture for OrganizationUser will return a bare string Permissions{guid}
+            // in the member, throwing a JsonException on deserialization of a bare string.
+            .With(organizationUser => organizationUser.Permissions, CoreHelpers.ClassToJsonData(new Permissions())));
+    }
+}
+
+public class OrganizationUserWithDefaultPermissionsAttribute : CustomizeAttribute
+{
+    public override ICustomization GetCustomization(ParameterInfo parameter) => new OrganizationUserWithDefaultPermissionsCustomization();
+}

test/Identity.Test/IdentityServer/UserDecryptionOptionsBuilderTests.cs

@@ -1,15 +1,20 @@
-using Bit.Core.Auth.Entities;
+using Bit.Core;
+using Bit.Core.Auth.Entities;
 using Bit.Core.Auth.Enums;
 using Bit.Core.Auth.Models.Data;
 using Bit.Core.Context;
 using Bit.Core.Entities;
 using Bit.Core.Enums;
+using Bit.Core.Models.Data;
 using Bit.Core.Repositories;
+using Bit.Core.Services;
 using Bit.Identity.IdentityServer;
+using Bit.Identity.Test.AutoFixture;
 using Bit.Identity.Utilities;
 using Bit.Test.Common.AutoFixture.Attributes;
 using NSubstitute;
 using Xunit;
+using User = Bit.Core.Entities.User;
 
 namespace Bit.Identity.Test.IdentityServer;
 
@@ -20,6 +25,7 @@ public class UserDecryptionOptionsBuilderTests
     private readonly IOrganizationUserRepository _organizationUserRepository;
     private readonly ILoginApprovingClientTypes _loginApprovingClientTypes;
     private readonly UserDecryptionOptionsBuilder _builder;
+    private readonly IFeatureService _featureService;
 
     public UserDecryptionOptionsBuilderTests()
     {
@@ -27,7 +33,8 @@ public class UserDecryptionOptionsBuilderTests
         _deviceRepository = Substitute.For<IDeviceRepository>();
         _organizationUserRepository = Substitute.For<IOrganizationUserRepository>();
         _loginApprovingClientTypes = Substitute.For<ILoginApprovingClientTypes>();
-        _builder = new UserDecryptionOptionsBuilder(_currentContext, _deviceRepository, _organizationUserRepository, _loginApprovingClientTypes);
+        _featureService = Substitute.For<IFeatureService>();
+        _builder = new UserDecryptionOptionsBuilder(_currentContext, _deviceRepository, _organizationUserRepository, _loginApprovingClientTypes, _featureService);
         var user = new User();
         _builder.ForUser(user);
     }
@@ -220,19 +227,65 @@ public class UserDecryptionOptionsBuilderTests
         Assert.False(result.TrustedDeviceOption?.HasLoginApprovingDevice);
     }
 
-    [Theory, BitAutoData]
+    /// <summary>
+    /// This logic has been flagged as part of PM-23174.
+    /// When removing the server flag, please also remove this test, and remove the FeatureService
+    /// dependency from this suite and the following test.
+    /// </summary>
+    /// <param name="organizationUserType"></param>
+    /// <param name="ssoConfig"></param>
+    /// <param name="configurationData"></param>
+    /// <param name="organization"></param>
+    /// <param name="organizationUser"></param>
+    /// <param name="user"></param>
+    [Theory]
+    [BitAutoData(OrganizationUserType.Custom)]
     public async Task Build_WhenManageResetPasswordPermissions_ShouldReturnHasManageResetPasswordPermissionTrue(
+        OrganizationUserType organizationUserType,
         SsoConfig ssoConfig,
         SsoConfigurationData configurationData,
-        CurrentContextOrganization organization)
+        CurrentContextOrganization organization,
+        [OrganizationUserWithDefaultPermissions] OrganizationUser organizationUser,
+        User user)
     {
         configurationData.MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption;
         ssoConfig.Data = configurationData.Serialize();
         ssoConfig.OrganizationId = organization.Id;
-        _currentContext.Organizations.Returns(new List<CurrentContextOrganization>(new CurrentContextOrganization[] { organization }));
+        _currentContext.Organizations.Returns([organization]);
         _currentContext.ManageResetPassword(organization.Id).Returns(true);
+        organizationUser.Type = organizationUserType;
+        organizationUser.OrganizationId = organization.Id;
+        organizationUser.UserId = user.Id;
+        organizationUser.SetPermissions(new Permissions() { ManageResetPassword = true });
+        _organizationUserRepository.GetByOrganizationAsync(ssoConfig.OrganizationId, user.Id).Returns(organizationUser);
 
-        var result = await _builder.WithSso(ssoConfig).BuildAsync();
+        var result = await _builder.ForUser(user).WithSso(ssoConfig).BuildAsync();
+
+        Assert.True(result.TrustedDeviceOption?.HasManageResetPasswordPermission);
+    }
+
+    [Theory]
+    [BitAutoData(OrganizationUserType.Custom)]
+    public async Task Build_WhenManageResetPasswordPermissions_ShouldFetchUserFromRepositoryAndReturnHasManageResetPasswordPermissionTrue(
+        OrganizationUserType organizationUserType,
+        SsoConfig ssoConfig,
+        SsoConfigurationData configurationData,
+        CurrentContextOrganization organization,
+        [OrganizationUserWithDefaultPermissions] OrganizationUser organizationUser,
+        User user)
+    {
+        _featureService.IsEnabled(FeatureFlagKeys.PM23174ManageAccountRecoveryPermissionDrivesTheNeedToSetMasterPassword)
+            .Returns(true);
+        configurationData.MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption;
+        ssoConfig.Data = configurationData.Serialize();
+        ssoConfig.OrganizationId = organization.Id;
+        organizationUser.Type = organizationUserType;
+        organizationUser.OrganizationId = organization.Id;
+        organizationUser.UserId = user.Id;
+        organizationUser.SetPermissions(new Permissions() { ManageResetPassword = true });
+        _organizationUserRepository.GetByOrganizationAsync(ssoConfig.OrganizationId, user.Id).Returns(organizationUser);
+
+        var result = await _builder.ForUser(user).WithSso(ssoConfig).BuildAsync();
 
         Assert.True(result.TrustedDeviceOption?.HasManageResetPasswordPermission);
     }
@@ -241,7 +294,7 @@ public class UserDecryptionOptionsBuilderTests
     public async Task Build_WhenIsOwnerInvite_ShouldReturnHasManageResetPasswordPermissionTrue(
         SsoConfig ssoConfig,
         SsoConfigurationData configurationData,
-        OrganizationUser organizationUser,
+        [OrganizationUserWithDefaultPermissions] OrganizationUser organizationUser,
         User user)
     {
         configurationData.MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption;
@@ -258,7 +311,7 @@ public class UserDecryptionOptionsBuilderTests
     public async Task Build_WhenIsAdminInvite_ShouldReturnHasManageResetPasswordPermissionTrue(
         SsoConfig ssoConfig,
         SsoConfigurationData configurationData,
-        OrganizationUser organizationUser,
+        [OrganizationUserWithDefaultPermissions] OrganizationUser organizationUser,
         User user)
     {
         configurationData.MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption;
@@ -275,7 +328,7 @@ public class UserDecryptionOptionsBuilderTests
     public async Task Build_WhenUserHasEnrolledIntoPasswordReset_ShouldReturnHasAdminApprovalTrue(
         SsoConfig ssoConfig,
         SsoConfigurationData configurationData,
-        OrganizationUser organizationUser,
+        [OrganizationUserWithDefaultPermissions] OrganizationUser organizationUser,
         User user)
     {
         configurationData.MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption;