-
Notifications
You must be signed in to change notification settings - Fork 221
feat(auth): Use libs email in auth server #19952
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
| mockFxaMailer.sendVerifyEmail.callCount, | ||
| 0, | ||
| 'mailer.sendVerifyEmail was not called' | ||
| 'mockFxaMailer.sendVerifyEmail was called' |
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.
was not?
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.
Good call - I was fixing some of them since they weren't right and just mixed this one up
b5529cc to
9ecdac7
Compare
| * @param opts | ||
| * @returns | ||
| */ | ||
| async sendVerifySecondaryCodeEmail( |
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.
So, this was the problem function in MOST of the integration and functional test failures. Because of the setup we just take whatever to is provided on the options. Since we use the formatter and pass in the account, it was trying to send the verification code to the primary email address, not secondary.
I figured it makes most sense to address that here. email is a required property on the type (used in the template) and so, we can swap the to out right before we send
Because: - We've moved email sending and rendering out of auth-server This Commit: - Updates several emails to send using the new fxa-mailer, renderer and sender Closes: FXA-12883
| if (fxaMailer.canSend('passwordChangeRequired')) { | ||
| // the new fxa-mailer is more type script, so we create a 'fake' | ||
| // request to satisfy the type checking | ||
| const request = { |
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.
@dschom - curious if you have a better idea for how to handle this. The mailer functions being type safe means it's expecting all of this, but also I don't want to just cast request as unknown as any
| resetLink: this.linkBuilder.buildResetLink(template, metricsEnabled, { | ||
| email: opts.to, | ||
| }), | ||
| link: this.linkBuilder.buildResetLink(template, metricsEnabled, { |
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.
Just double checking that both resetLink and link are both used and are supposed to have the same value. It's possible 'link' is not needed here, but I didn't fully check this.
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'll double check all of these - when I dug through the original implementation it was wonky so I probably crossed some wires. It is nice that we have this so explicit now so in the future it won't be so hand-wavy
| metricsEnabled, | ||
| { email: opts.to } | ||
| ), | ||
| link: this.linkBuilder.buildPasswordChangeRequiredLink( |
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.
Same question as above. Here it seems like maybe only 'link' is required?
| const links = { | ||
| supportUrl: this.linkBuilder.buildSupportLink(template, metricsEnabled), | ||
| privacyUrl: this.linkBuilder.buildPrivacyLink(template, metricsEnabled), | ||
| resetLink: this.linkBuilder.buildResetLink(template, metricsEnabled, { |
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.
Ditto
| OmitCommonLinks<TemplateData> & | ||
| OmitCommonLinks<verifyPrimary.TemplateData> & { | ||
| code: string; | ||
| service?: string; |
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 wonder if this should be ({ code:string } || { code: string, service:string, redirectTo:string, resule:string }). I think you had a similar comment during your review on my code. I am pretty sure if we have a service we want resume and redirectTo also.
| ...opts, | ||
| ...links, | ||
| }); | ||
| // explicitly override the `to` to ensure we send to the right email |
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.
👍
| metricsEnabled, | ||
| { email: opts.to } | ||
| ), | ||
| link: this.linkBuilder.buildVerifyEmailLink(template, metricsEnabled, { |
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.
Is this actually used?
| const from = cmsRpFromName | ||
| ? `${cmsRpFromName} <${this.mailerConfig.sender}>` | ||
| : this.mailerConfig.sender; | ||
| console.debug(`!!! From: ${from} !!!`); |
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.
Stray debug!
| const mailer: any = senders.email; | ||
| const accountEventManager = new AccountEventsManager(); | ||
|
|
||
| // setup for fxa-mailer, since this runs outside of the key_server context we |
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.
Is there a reason we couldn't hoist this to the top of the file? It seems a little wasteful to construct these everytime we run this function.
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 tried, but one of the required objects for constructing the FxaMailer were down here... Though as I say that I realize I can hoist that up too 🤦
| postalCode: '', | ||
| }, | ||
| }, | ||
| acceptLanguage: 'en/US', |
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.
The browser posts something like this: en-US,en;q=0.9. I think en is the safest fallback. Basically I'm a little unsure if the /US is valid?
We also have a locale column on the accounts table in the database. I think in this case we should use that to target the language that's most likely what the user is used to.
| new NodeRendererBindings({ | ||
| translations: { | ||
| basePath: join(__dirname, '../public/locales'), | ||
| ftlFileName: 'auth.ftl', |
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.
Thanks for making sure to grab this change!
Because:
This Commit:
Closes: FXA-12883
Checklist
Put an
xin the boxes that applyScreenshots (Optional)
Please attach the screenshots of the changes made in case of change in user interface.
Other information (Optional)
Any other information that is important to this pull request.