Skip to content

[PM-28519] Remove Emergency Access Contacts for AutoConfirm Org Flows#7123

Merged
sven-bitwarden merged 6 commits intomainfrom
ac/pm-28519/remove-ea-on-auto-confirm
Mar 5, 2026
Merged

[PM-28519] Remove Emergency Access Contacts for AutoConfirm Org Flows#7123
sven-bitwarden merged 6 commits intomainfrom
ac/pm-28519/remove-ea-on-auto-confirm

Conversation

@sven-bitwarden
Copy link
Contributor

@sven-bitwarden sven-bitwarden commented Mar 2, 2026

🎟️ Tracking

PM-28519

📔 Objective

To prevent unintended access of org keys from users' emergency access contacts, we are eliminating all these contacts from accepted/confirmed users in the organization as they go through user flows, and when the policy is enabled.

Technical Implementation

We already have extensive policy validation setup through PolicyRequirements, but the entire process is self-contained (between loading && validating data). We needed an extension, at the time a policy is enforced (User is being accepted/confirmed/restored), to know that a policy is enabled and validated.

This was accomplished by exposing an overload to automatic confirmation policy validation that takes a PolicyRequirement, which avoids duplicitous data loading. We can use that requirement to both validate and check enabled status.

📸 Screenshots

EA Removed On Policy Save

Screen.Recording.2026-03-02.at.3.46.29.PM.mov

EA Removed On Invite Accept

Screen.Recording.2026-03-02.at.3.54.32.PM.mov

EA Removed On Confirm User

Note

When confirmed, users do not have access to the EA page anymore, so I removed the user from the auto-confirm org before demonstrating their EA contacts were removed.

Screen.Recording.2026-03-02.at.3.56.23.PM.mov

EA Removed on Restore Accepted User

Screen.Recording.2026-03-02.at.4.06.46.PM.mov

@sven-bitwarden sven-bitwarden changed the title Remove emergency access from all organization users on policy enable,… [PM-28519]: Remove Emergency Access Contacts for AutoConfirm Org Flows Mar 2, 2026
@sven-bitwarden sven-bitwarden changed the title [PM-28519]: Remove Emergency Access Contacts for AutoConfirm Org Flows [PM-28519] Remove Emergency Access Contacts for AutoConfirm Org Flows Mar 2, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Mar 2, 2026

Logo
Checkmarx One – Scan Summary & Details429dd170-6f38-4fe1-8fcc-415afc04eebf

Great job! No new security vulnerabilities introduced in this pull request

@codecov
Copy link

codecov bot commented Mar 2, 2026

Codecov Report

❌ Patch coverage is 98.18182% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 56.61%. Comparing base (c97ccc1) to head (19d9d94).
⚠️ Report is 19 commits behind head on main.

Files with missing lines Patch % Lines
...ors/AutomaticUserConfirmationPolicyEventHandler.cs 94.11% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7123      +/-   ##
==========================================
- Coverage   60.65%   56.61%   -4.04%     
==========================================
  Files        2016     2016              
  Lines       88416    88458      +42     
  Branches     7882     7888       +6     
==========================================
- Hits        53625    50083    -3542     
- Misses      32887    36554    +3667     
+ Partials     1904     1821      -83     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sven-bitwarden sven-bitwarden added needs-qa ai-review Request a Claude code review labels Mar 2, 2026
@sven-bitwarden sven-bitwarden marked this pull request as ready for review March 2, 2026 21:59
@sven-bitwarden sven-bitwarden requested a review from a team as a code owner March 2, 2026 21:59
@sven-bitwarden sven-bitwarden requested a review from JimmyVo16 March 2, 2026 21:59
@claude
Copy link
Contributor

claude bot commented Mar 2, 2026

Claude finished @sven-bitwarden's task in 4m 46s —— View job


Review Summary

PR: PM-28519 Remove Emergency Access Contacts for AutoConfirm Org Flows

Overview

This PR removes emergency access (EA) contacts from users in organizations with the Automatic User Confirmation policy. EA records are deleted in four flows: when the policy is first enabled (bulk deletion for all org users), and when individual users are accepted, confirmed, or restored. The implementation also refactors AutomaticUserConfirmationPolicyEnforcementValidator to expose an overload accepting an externally-fetched PolicyRequirement, avoiding redundant queries.

Previous Review Items

All previously flagged items have been addressed:

  • Guard logic added to OnSaveSideEffectsAsync (only fires on initial enable)
  • ExecutePreUpsertSideEffectAsync now delegates to OnSaveSideEffectsAsync for dual save-command compatibility
  • Unit tests added for ExecutePreUpsertSideEffectAsync covering enable, disable, and already-enabled scenarios

Findings

Severity File Description
⚠️ IMPORTANT AcceptOrgUserCommand.cs EA deletion in HandleAutomaticUserConfirmationPolicyAsync runs before ValidateTwoFactorAuthenticationPolicyAsync. If TwoFactor validation fails after AutoConfirm passes, EA contacts are permanently deleted but the accept operation is rejected. The Confirm and Restore flows do not have this issue because they validate 2FA before the AutoConfirm block.

What Looks Good

  • The guard logic in OnSaveSideEffectsAsync correctly prevents EA deletion on disable and re-save, matching patterns in SingleOrgPolicyValidator and TwoFactorAuthenticationPolicyValidator
  • The IAutomaticUserConfirmationPolicyEnforcementValidator overload cleanly avoids duplicate policy queries while keeping the original method as a convenience entry point
  • Test coverage is thorough across all four flows, including positive, negative, and edge cases (invited users with null UserId filtered out in bulk deletion)
  • The Confirm and Restore flows correctly position EA deletion after all other policy validation completes

await deleteEmergencyAccessCommand.DeleteAllByUserIdsAsync(orgUserIds);
}

public Task OnSaveSideEffectsAsync(PolicyUpdate policyUpdate, Policy? currentPolicy) => Task.CompletedTask;
Copy link
Contributor

Choose a reason for hiding this comment

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

Just want to preface this by saying that we need documentation around this, and we already have a ticket for it. I’ll move it higher up on my priority list.

The review: You're adding ExecutePreUpsertSideEffectAsync, so you need to have OnSaveSideEffectsAsync call ExecutePreUpsertSideEffectAsync so that this handler gets triggered for both SavePolicyCommand and VNextSavePolicyCommand. Here’s an example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

makes sense/thanks for the example - updated.

Copy link
Contributor

@JimmyVo16 JimmyVo16 left a comment

Choose a reason for hiding this comment

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

Everything looks good. Just one small change request to ensure the handler works for both SavePolicyCommand and VNextSavePolicyCommand.

@sven-bitwarden sven-bitwarden requested a review from JimmyVo16 March 3, 2026 18:00
Comment on lines +276 to +279

if (policyRequirement.IsEnabled(orgUser.OrganizationId))
{
await _deleteEmergencyAccessCommand.DeleteAllByUserIdAsync(user.Id);
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ IMPORTANT: Destructive EA deletion executes before all validation is complete

In AcceptOrgUserAsync, HandleAutomaticUserConfirmationPolicyAsync runs at line 178 -- before ValidateTwoFactorAuthenticationPolicyAsync at line 200. If the AutoConfirm policy is enabled and the user passes its compliance check, their emergency access contacts are deleted here at line 279. But if ValidateTwoFactorAuthenticationPolicyAsync subsequently throws (user lacks 2FA, org requires it), the accept operation fails while the EA records are already permanently deleted.

This does not affect the Confirm or Restore flows because those check 2FA before the AutoConfirm block.

Scenario:

  1. Org has both AutoConfirm and TwoFactor policies enabled
  2. User without 2FA accepts invite
  3. AutoConfirm compliance passes (no other orgs, not a provider) -- EA deleted
  4. TwoFactor check fails -- BadRequestException thrown
  5. User cannot accept, but EA contacts are gone

Suggested fix: Move the EA deletion out of HandleAutomaticUserConfirmationPolicyAsync and into AcceptOrgUserAsync after all validation passes (e.g., just before line 202 where status is updated). The validation-only check can remain where it is. For example:

if (_featureService.IsEnabled(FeatureFlagKeys.AutomaticConfirmUsers))
{
    await HandleAutomaticUserConfirmationPolicyAsync(orgUser, allOrgUsers, user);
}

// ... SingleOrg checks ...
// ... TwoFactor check ...

// Delete EA only after all validation has passed
if (_featureService.IsEnabled(FeatureFlagKeys.AutomaticConfirmUsers))
{
    var policyRequirement = await _policyRequirementQuery
        .GetAsync<AutomaticUserConfirmationPolicyRequirement>(user.Id);
    if (policyRequirement.IsEnabled(orgUser.OrganizationId))
    {
        await _deleteEmergencyAccessCommand.DeleteAllByUserIdAsync(user.Id);
    }
}

This would require removing the deletion from HandleAutomaticUserConfirmationPolicyAsync and having that method only perform validation. This matches the approach already taken in the Confirm and Restore flows where non-recoverable checks complete before the EA deletion runs.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 3, 2026

@sven-bitwarden sven-bitwarden merged commit 43d3c41 into main Mar 5, 2026
51 of 52 checks passed
@sven-bitwarden sven-bitwarden deleted the ac/pm-28519/remove-ea-on-auto-confirm branch March 5, 2026 15:56
prograhamming pushed a commit that referenced this pull request Mar 16, 2026
…#7123)

* Remove emergency access from all organization users on policy enable, or when accepted/restored

* Use correct policy save system

* Add additional tests

* Implement both PreUpsert and OnSave side effects
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants