[PM-33951] automatically confirm pending users on admin login

[JaredScar]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-33951

📔 Objective

When an admin logins to the browser extension for an organization, we want users who accepted an invite to be confirmed as users automatically. When the extension is locked, unlocking should also proceed to accept the users pending confirmation.

[JaredScar]

@claude review please

[claude]

Claude finished @JaredScar's task in 4m 26s —— View job

---

Claude Code Review

• Read changed files
• Analyze new API service & response model
• Analyze auto-confirm service changes & tests
• Analyze main.background.ts integration
• Post review

---

Overall the change is well-scoped and tested. The feature flag gating and the guard rails (canManageAutoConfirm$ + configuration$.enabled) are in place, and the new API surface mirrors the existing singular auto-confirm pattern. A few issues worth addressing before merging, ordered roughly by impact.

🟠 Potentially duplicate sweeps on every accounts$ emission

libs/auto-confirm/src/services/default-auto-confirm.service.ts:52-74

this.accountService.accounts$
  .pipe(
    switchMap((accounts) =>
      merge(
        ...Object.keys(accounts).map((userId) =>
          this.authService.authStatusFor$(userId as UserId).pipe(
            startWith(AuthenticationStatus.LoggedOut),
            distinctUntilChanged(),
            pairwise(),
            filter(
              ([prev, curr]) =>
                curr === AuthenticationStatus.Unlocked &&
                prev !== AuthenticationStatus.Unlocked,
            ),
            map(() => userId as UserId),
          ),
        ),
      ),
    ),
  )
  .subscribe((userId) => void this.bulkAutoConfirmPendingUsers(userId));

Every time accounts$ emits (add/remove/update of any account), switchMap tears down and re-subscribes the inner merge. For each existing user whose status is currently Unlocked, the new subscription replays LoggedOut (via startWith) followed by Unlocked, which passes pairwise → filter → sweep. So adding a second account will re-run the sweep for the already-unlocked admin — and in general any account-map churn will cause duplicate API hits to /pending-auto-confirm and /bulk-auto-confirm.

Consider tracking "already-handled" userIds (e.g., scan over accounts$ to emit only newly-added userIds, then subscribe to authStatusFor$ per new userId without a startWith(LoggedOut) — instead resolve the current status once and only trigger if it starts Unlocked). Alternatively, debounce or guard with a Set<UserId> of users already swept this session.

🟠 N sequential HTTP requests for public keys

libs/auto-confirm/src/services/default-auto-confirm.service.ts:192-205

const confirmEntries = await Promise.all(
  pendingResponse.data.map(async (pendingUser) => {
    const publicKeyResponse = await this.apiService.getUserPublicKey(pendingUser.userId);
    ...
  }),
);

For N pending users this fires N parallel GET /users/{id}/public-key calls. There is already a bulk endpoint on this very service: postOrganizationUsersPublicKey(organizationId, ids[]) returning ListResponse<OrganizationUserBulkPublicKeyResponse>. Using it would be one request regardless of N and reduce server load / rate-limit risk for large orgs.

🟡 Unsafe as string cast on an encrypted key

libs/auto-confirm/src/services/default-auto-confirm.service.ts:201

key: confirmRequest.key as string,

OrganizationUserConfirmRequest.key is EncryptedString | undefined (branded SdkEncString). The bulk request entry is typed string. The as string silences the type error but also silences the undefined case. If buildConfirmRequest ever produces { key: undefined } (e.g., organization key missing), you would push undefined into keys[].key and the server would reject the whole bulk call. At minimum, guard for undefined and skip / warn; ideally align the bulk type to EncryptedString so the compiler helps.

🟡 Silent failure in the background subscription

libs/auto-confirm/src/services/default-auto-confirm.service.ts:73

.subscribe((userId) => void this.bulkAutoConfirmPendingUsers(userId));

A rejection from bulkAutoConfirmPendingUsers becomes an unhandled promise rejection; and an error from any of the inner operators will terminate the outer subscription for the remainder of the session (no catchError/retry). Since this runs in the browser extension background, an intermittent network blip at unlock time would permanently disable the sweep until the extension restarts. Wrap with catchError(...) and log, and consider a try/catch around the async call.

🟡 Relying on orgs[0] for the auto-confirm org

libs/auto-confirm/src/services/default-auto-confirm.service.ts:178-183 (and a similar orgs[0] in canManageAutoConfirm$)

The comment notes "auto-confirm does not allow the user to be part of any other organization". If that invariant is ever relaxed (or temporarily violated during sync / during provider relationships), orgs[0] could point at the wrong org and the sweep would silently no-op or hit the wrong endpoint. Safer: filter by org.useAutomaticUserConfirmation (or another explicit capability) and proceed only on exactly one match.

🟡 startWith(LoggedOut) interacts poorly with distinctUntilChanged

libs/auto-confirm/src/services/default-auto-confirm.service.ts:59-61

If the current status is LoggedOut, distinctUntilChanged suppresses the startWith duplicate, then a later transition to Locked would pair as [LoggedOut, Locked] – not a sweep, fine. But if the current status is Locked, you produce [LoggedOut, Locked] then [Locked, Unlocked] — also fine. The real issue is the one above (re-subscription on every accounts$ tick). Simplest fix: replace the startWith trick with a one-time read of the current status and only emit on genuine transitions going forward.

🟡 Duplication between autoConfirmUser and bulkAutoConfirmPendingUsers

Both methods repeat the same canManage/enabled/org resolution boilerplate. Consider extracting a single guard (e.g., resolveAutoConfirmOrg(userId): Promise<Organization | null>) that returns the org only when all preconditions are met.

🟢 Minor

• libs/auto-confirm/src/services/default-auto-confirm.service.ts:74 – missing blank line between initBulkAutoConfirmOnLoginSweep and autoConfirmState.
• Spec file libs/auto-confirm/src/services/default-auto-confirm.service.spec.ts:507 calls Utils.fromB64ToArray(mockPublicKey) at describe init time; the real Utils will throw if input is not valid base64. It happens to work for "mockPublicKeyBase64" but is fragile — compute it inside beforeEach or use a literal Uint8Array.
• The new JSDoc on getPendingAutoConfirmUsers and postBulkOrganizationUserAutoConfirm is clear. Consider also documenting the new BulkAutoConfirmOnLogin flag owner/expected removal date (or link to the PM ticket) in the feature-flag file.

✅ Things that look good

• Feature-flag gate (FeatureFlag.BulkAutoConfirmOnLogin defaulted to FALSE) at the top of bulkAutoConfirmPendingUsers.
• canManageAutoConfirm$ reuse — keeps the policy/permission check in one place.
• DI wiring in libs/angular/src/services/jslib-services.module.ts matches the new constructor signature.
• Test coverage for feature-flag disabled / cannot manage / disabled state / no pending users / no org / single vs multiple pending users / unlock transitions — nice.

---

Suggested follow-ups

• Fix startWith-induced duplicate sweep on accounts$ changes →
• Use bulk public-key endpoint →
• Guard against undefined encrypted key →
• Add error handling to background sweep subscription →
  · Branch: ac/pm-33919-automatically-confirm-pending-users-on-admin-login

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the bulk auto-confirm-on-login feature added to DefaultAutomaticUserConfirmationService. The change introduces a feature-flagged sweep that fires when an admin transitions to the Unlocked authentication state, fetches pending-accepted users, and bulk-confirms them via two new API service methods. The implementation is feature-flag gated (pm-35803-browser-auto-confirm-log-in), follows existing patterns from the per-user autoConfirmUser flow, and includes thorough unit tests covering feature flag, permission, configuration, deduplication, and Locked→Unlocked transition paths.

Code Review Details

No findings to flag.

Notes from analysis (informational, not findings):

• The seenUserIds deduplication concern was raised and resolved in a prior review thread; the author's reasoning that accounts$ is a snapshot feed which re-emits on any account change is correct.
• The fire-and-forget .catch(() => {}) on the sweep is explicitly intentional per the inline comment ("errors are transient"). With no LogService currently injected into the auto-confirm service, adding telemetry would require a constructor change. Consider adding logging in a follow-up if silent failures become a debugging pain point.
• buildConfirmRequest is invoked once per pending user (encrypting the default collection name N times) but only confirmEntries[0].defaultUserCollectionName is forwarded to the bulk request. This matches existing semantics (the org-key-encrypted collection name is equivalent across users) — minor wasted work, not a correctness issue.
• DefaultAutomaticUserConfirmationService is wired both via apps/browser/.../main.background.ts and libs/angular/.../jslib-services.module.ts, so the on-login sweep will execute on every Angular client where the feature flag is enabled (web/desktop/browser). Server-side flag targeting is the intended gate.

[github-actions]

Checkmarx One – Scan Summary & Details – a2560982-ebcc-45f5-81fa-7bbd20f87f26

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

[sven-bitwarden]

@JaredScar Tests are failing ❌

[codecov]

Codecov Report

❌ Patch coverage is 79.72973% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.11%. Comparing base (d3a8f77) to head (4451572).
⚠️ Report is 12 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...nfirm/src/services/default-auto-confirm.service.ts	87.30%	4 Missing and 4 partials ⚠️
.../services/default-organization-user-api.service.ts	0.00%	4 Missing ⚠️
...organization-user-pending-auto-confirm.response.ts	40.00%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #20331      +/-   ##
==========================================
+ Coverage   47.09%   47.11%   +0.01%     
==========================================
  Files        3938     3939       +1     
  Lines      119600   119659      +59     
  Branches    18350    18357       +7     
==========================================
+ Hits        56329    56376      +47     
- Misses      59006    59015       +9     
- Partials     4265     4268       +3     

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[JaredScar]

@claude please re-review

[JaredScar]

@claude Where is my re-review?

[sven-bitwarden]

Do we need to deduplicate here? This observable should returns a list of AccountInfo where each key is an UserId, and the AddAccount method in the service doesn't seem to allow for duplicates at all

[JaredScar]

We do because accounts$ is a snapshot feed. Any time something about the account changes, a whole map of accounts is pushed out to this observable... Therefore, we need to make sure we deduplicate here to prevent tracking the same user ID more than once.

[bw-ghapp]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and all feature flags disabled.

✅ Fortunately, these BIT tests have passed! 🎉

[bw-ghapp]

Changes in this PR impact the Autofill experience of the browser client

BIT has tested the core experience with these changes and the feature flag configuration used by vault.bitwarden.com.

✅ Fortunately, these BIT tests have passed! 🎉

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud