-
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
Auth/PM-7322 - Registration with Email verification - Finish registration endpoint #4182
Merged
JaredSnider-Bitwarden
merged 44 commits into
main
from
auth/pm-7322/email-verification-complete-registration-endpoint
Jul 2, 2024
Merged
Auth/PM-7322 - Registration with Email verification - Finish registration endpoint #4182
JaredSnider-Bitwarden
merged 44 commits into
main
from
auth/pm-7322/email-verification-complete-registration-endpoint
Jul 2, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JaredSnider-Bitwarden
changed the title
PM-7322 - AccountsController.cs - create empty method + empty req mod…
Auth/PM-7322 - Registration with Email verification - Finish registration endpoint
Jun 13, 2024
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4182 +/- ##
==========================================
+ Coverage 40.90% 41.18% +0.27%
==========================================
Files 1260 1262 +2
Lines 60103 60248 +145
Branches 5488 5499 +11
==========================================
+ Hits 24586 24811 +225
+ Misses 34382 34296 -86
- Partials 1135 1141 +6 ☔ View full report in Codecov by Sentry. |
This comment was marked as off-topic.
This comment was marked as off-topic.
Base automatically changed from
auth/pm-5092/email-verification-send-email-endpoint
to
main
June 19, 2024 17:54
…el to be able to create draft PR.
…rgInviteToken as we are adding a new email verification token to the mix.
JaredSnider-Bitwarden
force-pushed
the
auth/pm-7322/email-verification-complete-registration-endpoint
branch
from
June 19, 2024 18:26
9fb606a
to
67a18fd
Compare
…istration-endpoint
…orgInvite is optional.
…one bad request path.
…gic to always validate the token if we have one.
… to be more clear + validate org invite token even in open registration scenarios + added tests.
… use the new command.
…erify email as it's the only thing we need to verify + updated tests.
…timing attack delays per architecture discussion with a default of keeping them around.
…lAsync - must URL encode email to avoid invalid email upon submission to server on complete registration step
…nd new process to consider new feature flag for delays.
…c into private helper.
…ord instead of required annotations as it is easier to catch mistakes.
…istration-endpoint
…ndpoint' of https://github.com/bitwarden/server into auth/pm-7322/email-verification-complete-registration-endpoint
…rification-complete-registration-endpoint
…ication_WithOrgInviteToken_Succeeds
audreyality
approved these changes
Jul 1, 2024
ike-kottlowski
approved these changes
Jul 2, 2024
JaredSnider-Bitwarden
deleted the
auth/pm-7322/email-verification-complete-registration-endpoint
branch
July 2, 2024 21:03
withinfocus
pushed a commit
that referenced
this pull request
Jul 9, 2024
…tion endpoint (#4182) * PM-7322 - AccountsController.cs - create empty method + empty req model to be able to create draft PR. * PM-7322 - Start on RegisterFinishRequestModel.cs * PM-7322 - WIP on Complete Registration endpoint * PM-7322 - UserService.cs - RegisterUserAsync - Tweak of token to be orgInviteToken as we are adding a new email verification token to the mix. * PM-7322 - UserService - Rename MP to MPHash * PM-7322 - More WIP progress on getting new finish registration process in place. * PM-7322 Create IRegisterUserCommand * PM-7322 - RegisterUserCommand.cs - first WIP draft * PM-7322 - Implement use of new command in Identity. * PM-7322 - Rename RegisterUserViaOrgInvite to just be RegisterUser as orgInvite is optional. * PM07322 - Test RegisterUserCommand.RegisterUser(...) happy paths and one bad request path. * PM-7322 - More WIP on RegisterUserCommand.cs and tests * PM-7322 - RegisterUserCommand.cs - refactor ValidateOrgInviteToken logic to always validate the token if we have one. * PM-7322 - RegisterUserCommand.cs - Refactor OrgInviteToken validation to be more clear + validate org invite token even in open registration scenarios + added tests. * PM-7322 - Add more test coverage to RegisterUserWithOptionalOrgInvite * PM-7322 - IRegisterUserCommand - DOCS * PM-7322 - Test RegisterUser * PM-7322 - IRegisterUserCommand - Add more docs. * PM-7322 - Finish updating all existing user service register calls to use the new command. * PM-7322 - RegistrationEmailVerificationTokenable.cs changes + tests * PM-7322 - RegistrationEmailVerificationTokenable.cs changed to only verify email as it's the only thing we need to verify + updated tests. * PM-7322 - Get RegisterUserViaEmailVerificationToken built and tested * PM-7322 - AccountsController.cs - get bones of PostRegisterFinish in place * PM-7322 - SendVerificationEmailForRegistrationCommand - Feature flag timing attack delays per architecture discussion with a default of keeping them around. * PM-7322 - RegisterFinishRequestModel.cs - EmailVerificationToken must be optional for org invite scenarios. * PM-7322 - HandlebarsMailService.cs - SendRegistrationVerificationEmailAsync - must URL encode email to avoid invalid email upon submission to server on complete registration step * PM-7322 - RegisterUserCommandTests.cs - add API key assertions * PM-7322 - Clean up RegisterUserCommand.cs * PM-7322 - Refactor AccountsController.cs existing org invite method and new process to consider new feature flag for delays. * PM-7322 - Add feature flag svc to AccountsControllerTests.cs + add TODO * PM-7322 - AccountsController.cs - Refactor shared IdentityResult logic into private helper. * PM-7322 - Work on getting PostRegisterFinish tests in place. * PM-7322 - AccountsControllerTests.cs - test new method. * PM-7322 - RegisterFinishRequestModel.cs - Update to use required keyword instead of required annotations as it is easier to catch mistakes. * PM-7322 - Fix misspelling * PM-7322 - Integration tests for RegistrationWithEmailVerification * PM-7322 - Fix leaky integration tests. * PM-7322 - Another leaky test fix. * PM-7322 - AccountsControllerTests.cs - fix RegistrationWithEmailVerification_WithOrgInviteToken_Succeeds * PM-7322 - AccountsControllerTests.cs - Finish out integration test suite!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
🎟️ Tracking
PM-7322
Associated Clients PR: bitwarden/clients#9653
📔 Objective(s)
UserService
and into the newRegisterUserCommand
so we could add onto itPostRegisterFinish
endpoint to the IdentityAccountsController
which calls the newRegisterUserCommand.RegisterUserViaEmailVerificationToken(...)
method to register a user.Important note: during the
RegisterUserCommand
refactoring, I discovered missing validation on for the existing org invite process. We previously only validated the org invite token if a self hosted org had disabled open user registration, but we should have been validating if it was provided in all scenarios - only the error message should have changed if open registration was disabled.📸 Screenshots
SSO JIT User Registration still works - This tests the new
RegisterUserCommand.RegisterUser(...)
logicPM-7322.-.Web.-.SSO.JIT.User.registration.still.works.mov
Org Invite User Registration still works - This tests the new
RegisterUserCommand.RegisterUserWithOptionalOrgInvite(...)
logicPM-7322.-.Web.-.Org.Invite.User.Registration.still.works.mov
⏰ 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