-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(active-user-state-refactor): [PM-12040] Remove ActiveUserSta… #13149
base: main
Are you sure you want to change the base?
refactor(active-user-state-refactor): [PM-12040] Remove ActiveUserSta… #13149
Conversation
…te from SSO Service - First pass of work to update the state. In the middle of testing.
…e-user-state-from-sso-login
…om SSO Service - Fix for jslib-services.module.ts
Fixed Issues (2)Great job! The following issues were fixed in this Pull Request
|
…e-user-state-from-sso-login
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #13149 +/- ##
==========================================
+ Coverage 35.48% 35.54% +0.05%
==========================================
Files 3007 3008 +1
Lines 90899 90933 +34
Branches 16908 16917 +9
==========================================
+ Hits 32260 32319 +59
+ Misses 56137 56109 -28
- Partials 2502 2505 +3 ☔ View full report in Codecov by Sentry. |
…om SSO Service - Fix main.background.ts
4ade22d
to
0091a20
Compare
…om SSO Service - Added simple tests
…om SSO Service - Tiny touchups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work on this! Would you mind adding recordings of SSO login working still on each client on the PR? Just a few nits below:
libs/common/src/auth/abstractions/sso-login.service.abstraction.ts
Outdated
Show resolved
Hide resolved
I think some of the flows seem to concern TDE here and here. Am I understanding that correctly? In my own testing of these changes with SSO flows without TDE I didn't see these populate with values and moved on before chasing the end of the trail there. I'll give it another go. |
…e-user-state-from-sso-login
…om SSO Service - Few fixes to resolve comments
Yes, sorry I missed that. Let me know if you need any help running through the SSO TDE flows. |
…om SSO Service - Changed place where userId is loaded
…rom SSO Service - Fixed test
@JaredSnider-Bitwarden I have uploaded videos of all the clients going through at least master password flows. Would you like me to verify TDE flows for all of them as well? |
No, the videos were sufficient! Thank you! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12040
📔 Objective
Update the SSO Login Service to use SingleUserState instead of deprecated ActiveUserState.
Screenshots
Web TDE Flow
Screen.Recording.2025-02-02.at.10.11.01.PM.mov
Web Master Password Flow
Screen.Recording.2025-02-02.at.10.12.06.PM.mov
Desktop Master Password Flow
Screen.Recording.2025-02-03.at.11.02.40.AM.mov
Browser Master Password Flow
Screen.Recording.2025-02-03.at.11.15.05.AM.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