-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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-5092 - Registration with Email verification - Send Email Verification Endpoint #4173
Auth/PM-5092 - Registration with Email verification - Send Email Verification Endpoint #4173
Conversation
โฆterSendEmailVerification
โฆts (still WIP).
โฆsting verify email.
โฆtly written + register as scoped.
โฆ the existing email verification token.
โฆd call to mail service to send email.
โฆon logic in place.
โฆ remove modelState invalid check as .NET literally executes this validation pre-method execution.
โฆustom email for registration verify email.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4173 +/- ##
==========================================
+ Coverage 40.60% 40.68% +0.07%
==========================================
Files 1232 1239 +7
Lines 59518 59492 -26
Branches 5470 5434 -36
==========================================
+ Hits 24170 24206 +36
+ Misses 34208 34160 -48
+ Partials 1140 1126 -14 โ View full report in Codecov by Sentry. |
This comment was marked as off-topic.
This comment was marked as off-topic.
โฆhe 2 params which should have been nullable based on the constructor logic (2) Add new ReferenceEventType.cs for email verification register submit (3) Update AccountsController.cs to log new reference event (4) Update tests
โฆpose, and token id to include registration to differentiate it from the existing email verification token.
โฆme as it is optional.
After addressing all PR feedback, I ran through the 4 primary test scenarios for the endpoint again using web. |
โฆred in client side registration step to create a master key.
โฆttps://github.com/bitwarden/server into auth/pm-5092/email-verification-send-email-endpoint
@@ -4,9 +4,11 @@ namespace Bit.Core.Auth.Models.Mail; | |||
|
|||
public class RegisterVerifyEmail : BaseMailModel | |||
{ | |||
public string Url => string.Format("{0}/finish-signup?token={1}", | |||
public string Url => string.Format("{0}/finish-signup?token={1}&email={2}", |
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.
Adding email to URL so that the registration finish component can have access to email in order to create a master key once the user enters their MP.
โฆath and PlanUpgradePath nullable
โฆe reference event.
...uth/UserFeatures/Registration/Implementations/SendVerificationEmailForRegistrationCommand.cs
Show resolved
Hide resolved
a9d0b99
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.
One recommendation, but it's by no means required.
Merging as all the new changes are feature flagged. |
๐๏ธ Tracking
PM-5092
Associated clients PR: bitwarden/clients#9573
๐ Objective
To create a new endpoint to service the new email verification step in the new registration process.
โฐ 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