[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
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Reviewed the new Code Review Details
|
| 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.
There was a problem hiding this comment.
Perhaps something to raise to Product.
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:
|
fedemkr
left a comment
There was a problem hiding this comment.
Looks good, just two questions that I think Product would need to revise.
| /// The master password policy captured from the identity token response during an SSO/TDE | ||
| /// login, cached (keyed by email) until the user unlocks the vault with their master | ||
| /// password. Only consumed when the email matches the account being unlocked, to avoid a | ||
| /// stale policy from one account leaking into a later unlock for a different account. | ||
| private var ssoMasterPasswordPolicy: (email: String, policy: MasterPasswordPolicyResponseModel?)? | ||
|
|
There was a problem hiding this comment.
🤔 I think there's an edge case here where it would fail where you have a user with two SSO accounts on different servers but using the same email and the stale policy from one account leaks into the other, so given the email is the same this would actually be returning the policy.
Not sure if that would be a plausible scenario in real life but perhaps something to run by Product so they're aware. If necessary we could fix this in a future PR as there are slim chances of happening.
| 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.
Perhaps something to raise to Product.
🎟️ 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.