[PM-13239] Add isPreAuth to ClientService.generator() calls #1045
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## release/2024.10-rc1 #1045 +/- ##
=======================================================
- Coverage 89.07% 89.07% -0.01%
=======================================================
Files 660 660
Lines 41304 41301 -3
=======================================================
- Hits 36793 36790 -3
Misses 4511 4511 ☔ View full report in Codecov by Sentry. |
|
New Issues
|
KatherineInCode
requested review from
matt-livefront,
phil-livefront,
victor-livefront,
brant-livefront and
a team
as code owners
| /// - Parameter isPreAuth: Whether the client is being used for a user prior to authentication | ||
| /// (when the user's ID doesn't yet exist). This primarily will happen in SSO flows. | ||
| /// | ||
| func generators(isPreAuth: Bool) async throws -> ClientGeneratorsProtocol { |
There was a problem hiding this comment.
🎨 I think we could default this to false here if we don't want to always specify it when not in pre-auth flows. That would match the auth() function, although I think we could also default that rather than specify an alternate function.
ios/BitwardenShared/Core/Platform/Services/ClientService.swift
Lines 74 to 86 in e8fc366
There was a problem hiding this comment.
🤔 I see pros and cons with any of the approaches because if we provide default values then someone could introduce a bug by copy/pasting a generators() call without parameter and not knowing they need to specify isPreAuth: false for that on pre auth flows. OTOH, it feels great not to specify the parameter as it would be false in the majority of use cases and it introduces less code changes.
🤔 Also I've been thinking on this for a while on trying not to pass the parameter at all and have some "context" where we can check whether the current flow is actually pre-auth or not so we reduce the chance of introducing bugs on these calls. I have some ideas but need to keep thinking on some cases, for now I see any of the two approaches working fine and we'd just need to keep an eye when calling it and on the PR reviews to be sure we're in the right flow.
There was a problem hiding this comment.
@matt-livefront
I missed that, and I agree—though I don't think you can provide defaults in a protocol. It makes sense for both auth and generator to match here, though.
@fedemkr
I would definitely much rather some sort of broader context that we can check, rather than passing this around so much. I originally wrote it to just add that flag to all of the calls and it was a massive spiderweb of them.
Some of this is also motivated by the fact that in Debug modes, if we log an error we intentionally crash; the issue this is fixing wouldn't actually ever happen in a Release.
There was a problem hiding this comment.
@KatherineInCode This function is in the extension, which I think would allow adding a default value.
@fedemkr I think the context idea is a good one. In regard to default values, between crashlytics logging and crashing in debug builds these pretty easy to find if you miss a spot. And we don't have too many spots where we use the SDK prior to authentication, so I like the default value for those reasons.
There was a problem hiding this comment.
@matt-livefront Oh! I see what you mean! Yes, I like that idea and can do something to that effect
KatherineInCode
force-pushed
the
pm-13239/sso-crash
branch
from
333206a to
8a3c386
Compare
|
@fedemkr @matt-livefront I updated which branch this is going against because this fixes an issue found with the release |
Would it be better to merge this into main and then have a separate PR to pull it into the release branch? That prevents a fix from ever being left out of main. I'm curious what issue this fixes with the release - I thought this should all work without the isPreAuth flag. |
|
@matt-livefront The idea with the RC branches is they'll be regularly merged into The ticket in question is listed as a blocker for the release, and was found in regression testing the release, as I understand it; hence why the putting it in the release branch. @differsthecat Does that all seem right to you? |
Ah, ok.
When you said it was a blocker, I assumed this change was causing some larger issue. But crashing in debug builds is what I would expect, worth nothing that this wouldn't have crashed in production builds though. |
I did in the PR objective, but I'll also add a note to the ticket about it. |
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-13239
📔 Objective
This fixes an error in the SSO flow that caused a crash on debug builds. This was happening because the SSO flow uses the Generator for TDE things before the user is authorized, but was not indicating that we were pre-auth at that time; in release builds this was just outputting an error log and continuing, but that crashes on debug builds.
This fix just adds the
isPreAuthflag to thegenerator()function, and handles things appropriately upstream.⏰ 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