feat(auth): Use libs email in auth server #19952
Conversation
nshirley
force-pushed
the
FXA-12883
branch
4 times, most recently
from
b5529cc to
9ecdac7
Compare
| * @param opts | ||
| * @returns | ||
| */ | ||
| async sendVerifySecondaryCodeEmail( |
There was a problem hiding this comment.
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
| 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.
@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.
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.
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
| OmitCommonLinks<TemplateData> & | ||
| OmitCommonLinks<verifyPrimary.TemplateData> & { | ||
| code: string; | ||
| service?: string; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So, I tried to do this but it doesn't enforce the additional parameters when one is defined (i.e., adding in service doesn't then make redirectTo and resume required als), if that's what the intent was. And from what I can tell, we can't always guarantee that places that call this will have those available to them... maybe I can dig in more to check the routes, handlers, joi validation etc
| metricsEnabled, | ||
| { email: opts.to } | ||
| ), | ||
| link: this.linkBuilder.buildVerifyEmailLink(template, metricsEnabled, { |
There was a problem hiding this comment.
If I'm following the templates correctly then it does look like it's used in libs/accounts/email-renderer/src/templates/verificationReminderFirst/index.txt
Though, now that I'm looking at this - your comment may have been for a different line and I've since pushed so I don't know for sure...
| 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.
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.
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 🤦
There was a problem hiding this comment.
Oh, actually I just found the other issue - setting up the instance requires an authDb for boucnes, but that's an async function so there's not a great way to setup the fxa-mailer at the top of the module
| postalCode: '', | ||
| }, | ||
| }, | ||
| acceptLanguage: 'en/US', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I ended up fixing this to a better 'default' value but also trying to use the account.locale then falling back if it's not set for some reason
| country: 'United States', | ||
| city: 'Mountain View', | ||
| }, | ||
| device: { |
There was a problem hiding this comment.
I'm guessing uaDeviceType isn't actually used / needed? Just double checking we didn't miss something here...
There was a problem hiding this comment.
I just double checked through the partials that this email references/inherits from and I don't see a uaDeviceType, deviceType, or even type referenced anywhere,
nshirley
force-pushed
the
FXA-12883
branch
7 times, most recently
from
4ec9d74 to
f87de31
Compare
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
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.