Fix issue where non-SSO users are forced into an SSO flow #951
Conversation
|
No New Or Fixed Issues Found |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #951 +/- ##
=======================================
Coverage 88.70% 88.70%
=======================================
Files 630 630
Lines 39685 39685
=======================================
Hits 35201 35201
Misses 4484 4484 ☔ View full report in Codecov by Sentry. |
KatherineInCode
requested review from
matt-livefront,
phil-livefront,
victor-livefront,
brant-livefront and
a team
as code owners
| @@ -364,7 +364,7 @@ extension AuthRouter { | |||
| return .landingSoftLoggedOut(email: activeAccount.profile.email) | |||
| } | |||
|
|
|||
| let hasMasterPassword = activeAccount.profile.userDecryptionOptions?.hasMasterPassword == true | |||
| let hasMasterPassword = activeAccount.profile.userDecryptionOptions?.hasMasterPassword ?? true | |||
There was a problem hiding this comment.
🤔 Would this fall on a wrong behavior on a TDE user who's actually looking to go through this flow and somehow userDecryptionOption is nil as well?
I mean AFAIK userDecryptionOption should never be nil; it was marked as optional only to provide support for old server versions if I'm not mistaken. So there must be something that is setting that to nil somehow. Maybe we can reach to server folks to confirm userDecryptionOptions is always sent in last 3 major versions of server both when logging in with whichever method the user chooses and also in the sync endpoints.
There was a problem hiding this comment.
My current theory as to what happened is that somehow the userDecryptionOptions didn't properly carry forward through an app upgrade migration, and we don't fetch a new one until the user has signed in—but because of the SSO prompt, they can't sign in.
I dug through our code as thoroughly as I could, and if we ever set it to anything outside of what the server hands us or we pull off of disk, I couldn't find it. It might be worth checking with the server team, yeah. It may be that self-hosted users are running into an issue where their server isn't sending this value 🤔
Ultimately, though, I think there are significantly more MP users than SSO users, and so I'd rather default to the MP users and assume if we don't have that object then the user has a master massword—which is what the state service does already.
There was a problem hiding this comment.
I'll approve but I believe we should check with server folks just to see if we need to make any additional changes or take into consideration that there may be occasions where userDecryptionOptions is sent with nil value.
Btw, it could also have been an issue in MAUI code that made it null and as you say in the migration it stayed like that.
There was a problem hiding this comment.
I think the only time the app gets userDecryptionOptions is when the account is created or logged into. I don't think it's returned as part of sync or token refresh. So if it was nil when the account was created or last logged into (maybe possible for old accounts?), it would still be nil in user defaults.
There was a problem hiding this comment.
@fedemkr I happened to see this since I was curious if TDE was involved in this issue. The intent of the server code is that the userDecryptionOptions would always be non-null. It's initialized here and populated with values depending on the user's account information.
The construct should be returned with all requests since TDE was introduced, back in 2023.8.2 if I remember correctly.
There was a problem hiding this comment.
Great thanks for the info @trmartin4!!! That version is way more than 3 major server versions ago so we can be 99% sure, that's not causing the issue 😄
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12343
📔 Objective
Users who aren't SSO users are being forced into the SSO flow, particularly with the extension. This is likely because somehow their account in UserDefaults doesn't have user decryption options set. With this change, if we can't find those options, we assume the user has a master password, so we won't go into the SSO flow.
⏰ 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