Auth/PM-19555 - Fix multi account logout on lock screens not redirecting properly #14630
Conversation
…service and register with service modules
…need for extension-login-decryption-options.service
…direct guard routing (2) Remove logout method from account switcher service (3) use new NewActiveUser type
JaredSnider-Bitwarden
changed the title
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #14630 +/- ##
==========================================
+ Coverage 36.28% 36.34% +0.06%
==========================================
Files 3197 3190 -7
Lines 92345 92116 -229
Branches 16566 16526 -40
==========================================
- Hits 33503 33479 -24
+ Misses 56448 56246 -202
+ Partials 2394 2391 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
New Issues (1)Checkmarx found the following issues in this Pull Request
|
| // navigate to root so redirect guard can properly route next active user or null user to correct page | ||
| await this.router.navigate(["/"]); |
There was a problem hiding this comment.
Product signed off on this change in behavior so next up users are left in the correct default state instead of on any screen that the first user was using.
…ocked state works now.
…ccount-logout-on-lock-screens-defect + merge conflict fixes
| if (result?.authenticationStatus === AuthenticationStatus.Unlocked) { | ||
| // navigate to root so redirect guard can properly route next active user or null user to correct page | ||
| await this.router.navigate(["/"]); |
There was a problem hiding this comment.
❓- In this case, do we need to go through the redirect guard? Since we are dealing with an Unlocked user, can we just navigate to vault here?
There was a problem hiding this comment.
Good call! Done and retested!
|
| .userDecryptionOptionsById$(msg.userId) | ||
| .pipe( | ||
| map( | ||
| (decryptionOptions) => decryptionOptions?.trustedDeviceOption != null ?? false, |
There was a problem hiding this comment.
Left hand is always a boolean (never null/undefined), so right hand will never be hit. Is this a mistake? See also Desktop app.component.ts.
As a sidenote, we have similar logic 2x in device-trust.service.implementation.ts
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-19555
📔 Objective
To resolve a bug where the following happens:
Lock Component bug example:
PM-19555.Lock.component.bad.behavior.mov
This PR starts the consolidation of the distributed logout logic. This is not finished, and I've flagged future work for us under https://bitwarden.atlassian.net/browse/PM-21212.
Secondary bugs fixed:
Extension - The login decryption options component didn't properly show the account switcher.
Extension & Desktop - User's could swap to an account in a TDE locked state, but it wouldn't properly get the user to the login initiated screen
📸 Screenshots
Lock Component fixed
PM-19555.Lock.component.fixed.behavior.mov
TDE Login Decryption Options component fixed (note: the account switcher is now properly shown on this component):
PM-19555.Extension.-.Account.switching.now.works.for.users.in.a.TDE.locked.state.mov
Account switcher new behavior (redirects instead of just going back):
PM-19555.Account.switcher.component.new.behavior.mov
Secondary Bugs fixed behavior (account switching now works for users in a TDE locked state):
PM-19555.Desktop.-.Account.switching.now.works.for.users.in.a.TDE.locked.state.mov
PM-19555.Extension.-.Account.switching.now.works.for.users.in.a.TDE.locked.state.mov
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes