Repository navigation
Conversation
|
|
|
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
| // Clear a reason that was previously stored for a non-TDE user. | ||
| it == ForcePasswordResetReason | ||
| .TDE_USER_WITHOUT_PASSWORD_HAS_PASSWORD_RESET_PERMISSION && | ||
| !isTdeUser |
There was a problem hiding this comment.
Do we need this?
I would expect users to either have added the password or logged out by the time this code gets to them.
There was a problem hiding this comment.
This only saves affected users one step. If a Key Connector user was promoted to admin and synced on an older build, the TDE reason is already stored on their device, and previousForcePasswordResetReason keeps it through every sync. After updating, they'd unlock through Key Connector and then land on ResetPassword, so they'd have to log out and back in once. The cleanup clears that reason on the next sync.
The fix itself doesn't depend on it. New logins and promotions on the new version never store the reason. Happy to drop it if you'd rather keep the change minimal.
There was a problem hiding this comment.
Could we drop it, I prefer to keep this code as simple as possible and there is a work-around for that edge-case scenario. Plus, once this fix merges, the issue should never happen again.
…r password A Key Connector user who is promoted to owner or admin after migrating gets stuck on the set password screen. The vault is still locked at that point and the user has no master password, so there is no way forward. The admin rule comes from Trusted Device Encryption. Android applied it to Key Connector users too, in two places: - toUserState set needsMasterPassword for Key Connector users with the reset password permission. - getForcePasswordResetReason set the TDE reset reason for any user without a master password, without checking for TDE. Web (default-sync.service.ts, sso-login.strategy.ts) and iOS (SyncService.checkTdeUserNeedsToSetPassword) only apply this rule when trustedDeviceOption is present. Android now does the same. A stored TDE reason on a non-TDE account is cleared on the next sync, so devices that already hit the bug recover.
b28e9f0 to
a810bb3
Compare
🎟️ Tracking
Reported against a Vaultwarden build with Key Connector support: acul021/vaultwarden#1
Nothing in the client logic is Vaultwarden-specific. A Bitwarden server should hit the same path when a Key Connector user is promoted to owner or admin after migration. I have not reproduced it against a Bitwarden server.
📔 Objective
A Key Connector user who is promoted to owner or admin after migrating to Key Connector cannot log in on Android anymore. The web vault and iOS handle the same account fine.
Steps to reproduce
Expected: The vault unlocks through Key Connector, as it does on web and iOS.
Actual: The app goes to the set password screen. The vault is still locked and the user has no master password, so they can't continue.
Cause
Bitwarden requires TDE users with the "manage account recovery" permission (owners, admins, custom roles with that permission) to set a master password. Android applied this rule to Key Connector users as well, in
UserStateJsonExtensions.kt:toUserStatecomputedneedsMasterPassword = truefor a user without a master password whentrustedDevice != null || keyConnectorOptions != nulland the user had the permission.RootNavViewModelthen routes toRootNavState.SetPassword.getForcePasswordResetReasonsetTDE_USER_WITHOUT_PASSWORD_HAS_PASSWORD_RESET_PERMISSIONon sync for any user without a master password who had the permission, without checking for TDE. The stored reason survives later syncs throughpreviousForcePasswordResetReason.The other clients only apply this rule to TDE:
libs/common/src/platform/sync/default-sync.service.ts: checksuserDecryptionOptions.trustedDeviceOption !== undefined.libs/auth/src/common/login-strategies/sso-login.strategy.ts: readstrustedDeviceOption?.hasManageResetPasswordPermission.SyncService.checkTdeUserNeedsToSetPassword: requirestrustedDeviceOption != nil.GetTokenResponseExtensions.toForcePasswordResetReason) already scopes this totrustedDeviceUserDecryptionOptions. Only the sync and user state paths were missing the check.Changes
needsMasterPassword: a TDE user needs a password only with the reset password permission, a Key Connector user never does, and any other user without a password still does.getForcePasswordResetReason: only sets the TDE reason whentrustedDeviceUserDecryptionOptionsis present. A previously stored TDE reason on a non-TDE account is cleared on sync, so devices that already ran into this recover without logging out.removePassword,shouldShowRemovePassword) is unchanged. It matches web.Tests
New cases in
UserStateJsonExtensionsTest:needsMasterPasswordisfalse.Existing TDE tests are unchanged and pass.
📸 Screenshots
No UI changes.