-
Notifications
You must be signed in to change notification settings - Fork 1.4k
[PM-22408] Remove setMasterKeyEncryptedUserKey from KeyService #15087
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
base: main
Are you sure you want to change the base?
Conversation
Great job, no security vulnerabilities found in this Pull Request |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #15087 +/- ##
==========================================
+ Coverage 36.84% 36.88% +0.03%
==========================================
Files 3208 3208
Lines 92719 92725 +6
Branches 13935 13939 +4
==========================================
+ Hits 34164 34197 +33
+ Misses 57150 57120 -30
- Partials 1405 1408 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-22408
📔 Objective
This PR removes the
KeyService
'ssetMasterKeyEncryptedUserKey
method in favor of using themasterPasswordService
'ssetMasterKeyEncryptedUserKey
and requiring consumers to pass in a userID.This is a part of the effort to remove the
KeyService
's usage ofActiveUserState
and require UserID for methods.Team reviewer notes:
Previously, there was a bug when passing in a null string to
KeyService
'ssetMasterKeyEncryptedUserKey
would call themasterPasswordService
like so.This counterintuitively didn't throw an error, and the stateProvider attempts to clear the entry for
masterKeyEncryptedUserKey
(which doesn't exist at the time).For these code changes, I'm preserving the previous behavior (no error), but make it clear we skip setting
setMasterKeyEncryptedUserKey
when thekey
response isn't there.Auth Team
Since we need the
EncString
in all the login strategies, it made sense to me to make theIdentityTokenResponse
key
aEncString
. I also made it optional to indicate it isn't always expected.If the team prefers something else, please let me know.
Platform team
I changed
ProfileResponse
to stay consistent with the Auth approach. If the team prefers something else, please let me know.⏰ 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