Lift in user_id into login method #318
Conversation
|
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #318 +/- ##
==========================================
- Coverage 71.76% 71.70% -0.07%
==========================================
Files 224 224
Lines 18384 18405 +21
==========================================
+ Hits 13193 13197 +4
- Misses 5191 5208 +17 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
There was a problem hiding this comment.
I feel like moving the user_uuid into login_method defeats a bit the initial point of the change, which was to have an immutable user id associated to an SDK client instance. Because of compatibility we couldn't just include it as part of client settings, so we changed it to be set-once only.
By moving it into the LoginMethod we're back to a situation where a client could be modified to be for a different user. Maybe what we really need to do is to also allow setting login_method once.
|
I think the more we can couple these things and make them immutable the better. I would be for changing LoginMethod to be once lock, there might be edge cases where you want to move between login methods i.e. logging in using username but transitioning to TDE? |
|
I think making as many fields immutable or initialized during construction makes sense, but there may be a lot of edge cases since a lot of things can change. For example, on mobile a lot of calls to We also store email and KDF settings in LoginMethod, which potentially would need changing during key rotation or email change, though we're not handling those right now. |
🎟️ Tracking
📔 Objective
Refactors
user_idto be part of the Login Method. This seems like a more logical place to put it since in a fully SDK based flow you can always get the id as part of the access token. And it decreases us from three states to two.⏰ Reminders before review
team
🦮 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 confirmedissue 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