-
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
[PM-12700] Add private key regeneration process #11829
Conversation
New Issues
|
Looks like those tests are failing on main and aren't related to this PR. |
Codecov ReportAttention: Patch coverage is
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #11829 +/- ##
==========================================
+ Coverage 33.45% 33.48% +0.02%
==========================================
Files 2909 2917 +8
Lines 91070 91159 +89
Branches 17337 17350 +13
==========================================
+ Hits 30467 30523 +56
- Misses 58197 58229 +32
- Partials 2406 2407 +1 ☔ View full report in Codecov by Sentry. |
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.
LGTM. I have added few optional changes.
* Service calls that should be included in this method are only those required to be awaited after successful login. | ||
* @param userId The user id. | ||
*/ | ||
abstract run: (userId: UserId) => Promise<void>; |
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.
🤔 function instead of property ?
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.
I think this is what you're asking for ? 5466be0
It looks like KM follows a standard, so I swapped to that. In the rest of the code base, they're both used.
Not sure what the difference is?
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.
Methods declared as arrow functions in an abstract class are treated as properties in TypeScript, not as traditional methods. Generally, we would want actual functions to be treated as functions. It makes things like testing easier as well.
...ser-asymmetric-key-regeneration/abstractions/user-asymmetric-key-regeneration-api.service.ts
Outdated
Show resolved
Hide resolved
...rc/user-asymmetric-key-regeneration/abstractions/user-asymmetric-key-regeneration.service.ts
Outdated
Show resolved
Hide resolved
...ser-asymmetric-key-regeneration/services/default-user-asymmetric-key-regeneration.service.ts
Outdated
Show resolved
Hide resolved
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.
Auth files look great! Tyvm for implementing the new service we discussed!
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.
LGTM
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-12700
📔 Objective
Requires server work in bitwarden/server#4929
The purpose of this PR is to add the private key regeneration process on UI refreshed login and lock components.
Old components are out of scope for this feature.
This feature is behind a feature flag.
Target flows are:
Out of scope are the login flows:
Private key regeneration will be added to these once the UI refresh rewrite for them is merged into main in follow-up PRs.
The
LoginSuccessHandlerService
was the requested approach for the auth team and why it was created under their ownership.⏰ 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