Conversation
|
Great job! No new security vulnerabilities introduced in this pull request |
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
There was a problem hiding this comment.
Auth domain changes LGTM!
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7094 +/- ##
==========================================
- Coverage 60.71% 56.70% -4.02%
==========================================
Files 2013 2013
Lines 88131 88191 +60
Branches 7846 7861 +15
==========================================
- Hits 53512 50009 -3503
- Misses 32716 36362 +3646
+ Partials 1903 1820 -83 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
eliykat
left a comment
eliykat
left a comment
There was a problem hiding this comment.
Much nicer review experience, thank you.
plans/2fa-account-recovery.md
Outdated
| // Validate | ||
| var validationResult = await _adminRecoverAccountValidator.ValidateAsync(commandRequest); | ||
| if (validationResult.IsError) | ||
| { | ||
| return Handle(validationResult.AsError); // maps BadRequestError → 400, NotFoundError → 404 | ||
| } |
There was a problem hiding this comment.
We call validation from inside the command, so that command inputs are always guaranteed to be validated. I can see an argument for doing it from the controller so that they're fully decoupled, but it's not our current practice.
plans/2fa-account-recovery.md
Outdated
| return TypedResults.NoContent(); | ||
| ``` | ||
|
|
||
| Add a `Handle(Error)` helper or use existing `Handle(CommandResult)` — need to check how other controllers handle `ValidationResult` errors. The `BaseAdminConsoleController.Handle(CommandResult)` handles `Error` types, so we can map: |
There was a problem hiding this comment.
We would use Handle(CommandResult) as this suggests. However, this command returns an IdentityResult for some reason rather than a CommandResult. You can either update the command to return a CommandResult, or - to limit scope - just throw your errors inside the command (including validation failures).
|
New proposal seems to have simplified things a bit |
|
3 participants
🎟️ Tracking
https://bitwarden.atlassian.net/browse/BW-138
📔 Objective
Org admins can recover 2FA on a member's account.
📸 Screenshots