[PM-40314] fix: Enforce Master Password policy on SSO/TDE vault unlock - #2980
[PM-40314] fix: Enforce Master Password policy on SSO/TDE vault unlock#2980matt-livefront wants to merge 1 commit into
Conversation
matt-livefront
added
ai-review
github-actions
Bot
added
the
app:password-manager
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| do { | ||
| try await authService.checkMasterPasswordPolicyAfterUnlock( | ||
| email: account.profile.email, | ||
| masterPassword: password, | ||
| ) | ||
| } catch { | ||
| // Don't let a policy-check failure block an unlock that already succeeded. | ||
| errorReporter.log(error: error) | ||
| } |
There was a problem hiding this comment.
❓ QUESTION: Running this on every master-password unlock means the Update Master Password screen can now be triggered from inside an app extension.
Trace and consideration
VaultUnlockProcessor.unlockVault() calls unlockVaultWithPassword and then coordinator.handleEvent(.didCompleteAuth), which routes through AuthRouter.completeAuthRedirect():
if account.profile.forcePasswordResetReason != nil {
return .updateMasterPassword
}
if !isInAppExtension {
...
}The forcePasswordResetReason check sits above the isInAppExtension guard, so this path applies in the AutoFill/Share/Action extensions too. An SSO/TDE user whose master password violates the policy — precisely the population this PR targets — may well perform their first master-password unlock inside the AutoFill extension. They would then be dropped into Update Master Password mid-autofill (which triggers settingsRepository.fetchSync() and, on success, logout) instead of getting their credential.
This was reachable before only via a reason already persisted from a main-app login; this change makes the extension itself a place where the reason first gets set. Was the extension context considered? If enforcement should be main-app-only, gating the new call (or the router's redirect) on isInAppExtension would keep autofill unaffected — the reason would still be set on the next main-app unlock. Note AppProcessor.removeMasterPassword already applies exactly this kind of guard.
The PR description discusses the vault-timeout re-lock flow but not extensions, so flagging in case it was an unconsidered side effect rather than an intentional trade-off.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2980 +/- ##
==========================================
+ Coverage 79.50% 81.80% +2.29%
==========================================
Files 1170 1047 -123
Lines 75192 67934 -7258
==========================================
- Hits 59781 55572 -4209
+ Misses 15411 12362 -3049 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
1 participant
🎟️ Tracking
PM-40314
📔 Objective
Users signing in via SSO or TDE with a master password were not being checked against their organization's active Master Password policy, and were never prompted to update a non-conforming master password — unlike non-SSO logins, which already enforce this correctly.
Non-SSO login already runs this check via
AuthService.checkMasterPasswordPolicies→requirePasswordChange, settingforcePasswordResetReasonon failure so the existingAuthRouter+Redirectslogic redirects to the Update Master Password screen. SSO and TDE master-password entry both converge onAuthRepository.unlockVaultWithPassword(password:), which never called this check.IdentityTokenResponseModel.masterPasswordPolicyinAuthService.loginWithSingleSignOn, keyed by email, immediately after the identity token response is fetched — before branching into deviceKey / masterPassword / keyConnector / TDE-decryption-options handling, so it's captured regardless of which branch is taken. Also captured inloginWithTwoFactorCode, since an SSO/TDE login that also requires 2FA gets its final identity token response there instead, after the initialloginWithSingleSignOnattempt threwtwoFactorRequiredbefore it could cache anything.AuthService.checkMasterPasswordPolicyAfterUnlock(email:masterPassword:)method that consumes that cache (clearing it unconditionally on read, and only trusting it when the email matches, to avoid a stale policy leaking into an unrelated account's later unlock) and reuses the existingcheckMasterPasswordPoliciesorchestration, falling back toPolicyService.getMasterPasswordPolicyOptions()when nothing is cached.AuthRepository.unlockVaultWithPassword(password:)after a successful unlock, non-fatally (logged, not thrown) so a policy-check failure can never be mistaken for a wrong master password or block an unlock that already succeeded.