Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
2 changes: 0 additions & 2 deletions src/Core/Constants.cs
Original file line number Diff line number Diff line change
Expand Up @@ -158,8 +158,6 @@ public static class FeatureFlagKeys
public const string Otp6Digits = "pm-18612-otp-6-digits";
public const string PM24579_PreventSsoOnExistingNonCompliantUsers = "pm-24579-prevent-sso-on-existing-non-compliant-users";
public const string DisableAlternateLoginMethods = "pm-22110-disable-alternate-login-methods";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โœ… Feature flag constant correctly removed. The removal is clean and doesn't affect any adjacent feature flags.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โœ… Feature flag constant correctly removed. The removal is clean and doesn't affect any adjacent feature flags.

public const string PM23174ManageAccountRecoveryPermissionDrivesTheNeedToSetMasterPassword =
"pm-23174-manage-account-recovery-permission-drives-the-need-to-set-master-password";
public const string RecoveryCodeSupportForSsoRequiredUsers = "pm-21153-recovery-code-support-for-sso-required";
public const string MJMLBasedEmailTemplates = "mjml-based-email-templates";
public const string MjmlWelcomeEmailTemplates = "pm-21741-mjml-welcome-email";
Expand Down
39 changes: 3 additions & 36 deletions src/Identity/IdentityServer/UserDecryptionOptionsBuilder.cs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
๏ปฟusing Bit.Core;
using Bit.Core.Auth.Entities;
๏ปฟusing Bit.Core.Auth.Entities;
using Bit.Core.Auth.Enums;
using Bit.Core.Auth.Models.Api.Response;
using Bit.Core.Auth.Utilities;
Expand All @@ -8,7 +7,6 @@
using Bit.Core.Enums;
using Bit.Core.KeyManagement.Models.Api.Response;
using Bit.Core.Repositories;
using Bit.Core.Services;
using Bit.Core.Utilities;
using Bit.Identity.Utilities;

Expand All @@ -26,8 +24,6 @@ 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!;
private SsoConfig? _ssoConfig;
Expand All @@ -37,15 +33,13 @@ public UserDecryptionOptionsBuilder(
ICurrentContext currentContext,
IDeviceRepository deviceRepository,
IOrganizationUserRepository organizationUserRepository,
ILoginApprovingClientTypes loginApprovingClientTypes,
IFeatureService featureService
ILoginApprovingClientTypes loginApprovingClientTypes
)
{
_currentContext = currentContext;
_deviceRepository = deviceRepository;
_organizationUserRepository = organizationUserRepository;
_loginApprovingClientTypes = loginApprovingClientTypes;
_featureService = featureService;
}

public IUserDecryptionOptionsBuilder ForUser(User user)
Expand Down Expand Up @@ -145,34 +139,7 @@ private async Task BuildTrustedDeviceOptionsAsync()
// 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;
if (_featureService.IsEnabled(FeatureFlagKeys.PM23174ManageAccountRecoveryPermissionDrivesTheNeedToSetMasterPassword))
{
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

// 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);
}
var hasManageResetPasswordPermission = await EvaluateHasManageResetPasswordPermission();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good simplification! The feature flag branching logic has been correctly removed, and now the code always uses the EvaluateHasManageResetPasswordPermission() method.

However, there's an outdated comment at line 158 that still references "PM-23174". While it's not harmful, it would be cleaner to either:

  1. Remove the PM-23174 reference since the ticket is now complete, or
  2. Remove the comment entirely since the method name is self-documenting

Minor recommendation: Update or remove the PM-23174 comment at line 158.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โœ… Good simplification! The feature flag branching logic has been correctly removed, and now the code always uses the EvaluateHasManageResetPasswordPermission() method, which is the intended final behavior.


// 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);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
๏ปฟusing Bit.Core;
using Bit.Core.Auth.Entities;
๏ปฟ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;
Expand All @@ -25,16 +23,14 @@ public class UserDecryptionOptionsBuilderTests
private readonly IOrganizationUserRepository _organizationUserRepository;
private readonly ILoginApprovingClientTypes _loginApprovingClientTypes;
private readonly UserDecryptionOptionsBuilder _builder;
private readonly IFeatureService _featureService;

public UserDecryptionOptionsBuilderTests()
{
_currentContext = Substitute.For<ICurrentContext>();
_deviceRepository = Substitute.For<IDeviceRepository>();
_organizationUserRepository = Substitute.For<IOrganizationUserRepository>();
_loginApprovingClientTypes = Substitute.For<ILoginApprovingClientTypes>();
_featureService = Substitute.For<IFeatureService>();
_builder = new UserDecryptionOptionsBuilder(_currentContext, _deviceRepository, _organizationUserRepository, _loginApprovingClientTypes, _featureService);
_builder = new UserDecryptionOptionsBuilder(_currentContext, _deviceRepository, _organizationUserRepository, _loginApprovingClientTypes);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good cleanup! The IFeatureService dependency has been correctly removed from the test class since the feature flag is no longer needed.

However, there's an outdated comment at lines 226-230 (not part of this diff) that still references this feature flag and states "When removing the server flag, please also remove this test, and remove the FeatureService dependency from this suite and the following test."

The FeatureService dependency has been removed, but the comment remains and is now misleading. The test Build_WhenManageResetPasswordPermissions_ShouldReturnHasManageResetPasswordPermissionTrue is still valuable and should remain, but the comment should be removed or updated.

Recommendation: In a follow-up commit, remove the outdated comment block at lines 226-230 of this file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

โœ… Good cleanup! The IFeatureService dependency has been correctly removed from the test class since the feature flag is no longer needed.

var user = new User();
_builder.ForUser(user);
}
Expand Down Expand Up @@ -274,8 +270,6 @@ public async Task Build_WhenManageResetPasswordPermissions_ShouldFetchUserFromRe
[OrganizationUserWithDefaultPermissions] OrganizationUser organizationUser,
User user)
{
_featureService.IsEnabled(FeatureFlagKeys.PM23174ManageAccountRecoveryPermissionDrivesTheNeedToSetMasterPassword)
.Returns(true);
configurationData.MemberDecryptionType = MemberDecryptionType.TrustedDeviceEncryption;
ssoConfig.Data = configurationData.Serialize();
ssoConfig.OrganizationId = organization.Id;
Expand Down
Loading