Skip to content

Commit 917ced7

Browse files
nshirleynshirley
committed
fix(auth): Pass correct client/service name to mailer
Because: - The old mailer would internally call to oauth_info_service This Commit: - Fixes a few places that were missed to call oauth_info_service and pass the correct clientName to the templates - verifyLogin, and verifyLoginCode email templates are fixed Closes: FXA-13055
1 parent dace520 commit 917ced7

6 files changed

Lines changed: 64 additions & 12 deletions

File tree

‎packages/fxa-auth-server/lib/routes/emails.js‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ const validators = require('./validators');
1717
const { reportSentryError } = require('../sentry');
1818
const { emailsMatch, normalizeEmail } = require('fxa-shared').email.helpers;
1919
const { recordSecurityEvent } = require('./utils/security-event');
20+
const { OAuthClientInfoServiceName } = require('../senders/oauth_client_info');
2021
const EMAILS_DOCS = require('../../docs/swagger/emails-api').default;
2122
const DESCRIPTION = require('../../docs/swagger/shared/descriptions').default;
2223
const HEX_STRING = validators.HEX_STRING;
@@ -139,6 +140,7 @@ module.exports = (
139140
statsd
140141
) => {
141142
const fxaMailer = Container.get(FxaMailer);
143+
const oauthClientInfoService = Container.get(OAuthClientInfoServiceName);
142144

143145
const REMINDER_PATTERN = new RegExp(
144146
`^(?:${verificationReminders.keys.join('|')})$`
@@ -870,6 +872,7 @@ module.exports = (
870872
await request.emitMetricsEvent(`email.verification.resent`);
871873
} else {
872874
if (fxaMailer.canSend('verifyLogin')) {
875+
const clientInfo = await oauthClientInfoService.fetch(service);
873876
await fxaMailer.sendVerifyLoginEmail({
874877
...FxaMailerFormat.account(account),
875878
...(await FxaMailerFormat.metricsContext(request)),
@@ -879,7 +882,7 @@ module.exports = (
879882
...FxaMailerFormat.sync(service),
880883
code,
881884
service,
882-
clientName: 'Firefox',
885+
clientName: clientInfo.name,
883886
redirectTo: request.payload.redirectTo,
884887
resume: request.payload.resume,
885888
});

‎packages/fxa-auth-server/lib/routes/utils/signin.js‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,9 @@ const { RelyingPartyConfigurationManager } = require('@fxa/shared/cms');
1818
const requestHelper = require('./request_helper');
1919
const { FxaMailer } = require('../../senders/fxa-mailer');
2020
const { FxaMailerFormat } = require('../../senders/fxa-mailer-format');
21+
const {
22+
OAuthClientInfoServiceName,
23+
} = require('../../senders/oauth_client_info');
2124

2225
const BASE_36 = validators.BASE_36;
2326

@@ -52,6 +55,7 @@ module.exports = (
5255
: null;
5356

5457
const fxaMailer = Container.get(FxaMailer);
58+
const oauthClientInfoService = Container.get(OAuthClientInfoServiceName);
5559

5660
return {
5761
validators: {
@@ -436,7 +440,7 @@ module.exports = (
436440
code: emailCode,
437441
resume,
438442
redirectTo,
439-
service
443+
service,
440444
});
441445
} else {
442446
await mailer.sendVerifyEmail([], accountRecord, {
@@ -504,6 +508,7 @@ module.exports = (
504508
const geoData = request.app.geo;
505509
try {
506510
if (fxaMailer.canSend('verifyLogin')) {
511+
const clientInfo = await oauthClientInfoService.fetch(service);
507512
await fxaMailer.sendVerifyLoginEmail({
508513
...FxaMailerFormat.account(accountRecord),
509514
...(await FxaMailerFormat.metricsContext(request)),
@@ -512,7 +517,7 @@ module.exports = (
512517
...FxaMailerFormat.device(request),
513518
...FxaMailerFormat.sync(service),
514519
code: sessionToken.tokenVerificationId,
515-
clientName: 'Firefox',
520+
clientName: clientInfo.name,
516521
redirectTo: redirectTo,
517522
service: service,
518523
resume: resume,
@@ -562,6 +567,7 @@ module.exports = (
562567

563568
try {
564569
if (fxaMailer.canSend('verifyLoginCode')) {
570+
const clientInfo = await oauthClientInfoService.fetch(service);
565571
await fxaMailer.sendVerifyLoginCodeEmail({
566572
...FxaMailerFormat.account(accountRecord),
567573
...(await FxaMailerFormat.metricsContext(request)),
@@ -571,11 +577,13 @@ module.exports = (
571577
...FxaMailerFormat.sync(service),
572578
...FxaMailerFormat.cmsLogo(rpCmsConfig?.shared),
573579
...FxaMailerFormat.cmsRpInfo(rpCmsConfig),
574-
...FxaMailerFormat.cmsEmailSubject(rpCmsConfig?.VerifyLoginCodeEmail),
580+
...FxaMailerFormat.cmsEmailSubject(
581+
rpCmsConfig?.VerifyLoginCodeEmail
582+
),
575583
code,
576584
redirectTo,
577585
resume,
578-
serviceName: service,
586+
serviceName: clientInfo.name,
579587
});
580588
} else {
581589
const { timeZone } = request.app.geo;

‎packages/fxa-auth-server/test/local/routes/emails.js‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -365,6 +365,7 @@ describe('/recovery_email/status', () => {
365365
});
366366
const stripeHelper = mocks.mockStripeHelper();
367367
stripeHelper.hasActiveSubscription = sinon.fake.resolves(false);
368+
mocks.mockOAuthClientInfo();
368369
const accountRoutes = makeRoutes({
369370
config: config,
370371
db: mockDB,
@@ -382,6 +383,7 @@ describe('/recovery_email/status', () => {
382383
describe('invalid email', () => {
383384
let mockRequest;
384385
beforeEach(() => {
386+
mocks.mockOAuthClientInfo();
385387
mockRequest = mocks.mockRequest({
386388
credentials: {
387389
email: TEST_EMAIL_INVALID,
@@ -580,6 +582,9 @@ describe('/recovery_email/resend_code', () => {
580582
return Promise.resolve();
581583
});
582584
const mockMailer = mocks.mockMailer();
585+
mocks.mockOAuthClientInfo({
586+
fetch: sinon.stub().resolves({ name: 'Firefox' }),
587+
});
583588
const mockFxaMailer = mocks.mockFxaMailer();
584589
const mockMetricsContext = mocks.mockMetricsContext();
585590
const accountRoutes = makeRoutes({
@@ -750,6 +755,7 @@ describe('/recovery_email/verify_code', () => {
750755
};
751756
const mockDB = mocks.mockDB(dbData, dbErrors);
752757
const mockMailer = mocks.mockMailer();
758+
mocks.mockOAuthClientInfo();
753759
const mockFxaMailer = mocks.mockFxaMailer();
754760
const mockPush = mocks.mockPush();
755761
const mockCustoms = mocks.mockCustoms();
@@ -1132,6 +1138,7 @@ describe('/recovery_email', () => {
11321138
const mockCustoms = mocks.mockCustoms();
11331139

11341140
beforeEach(() => {
1141+
mocks.mockOAuthClientInfo();
11351142
mockRequest = mocks.mockRequest({
11361143
credentials: {
11371144
uid: uuid.v4({}, Buffer.alloc(16)).toString('hex'),
@@ -1196,6 +1203,7 @@ describe('/recovery_email', () => {
11961203
describe('/mfa/recovery_email/secondary/resend_code', () => {
11971204
let fxaMailer;
11981205
beforeEach(() => {
1206+
mocks.mockOAuthClientInfo();
11991207
fxaMailer = mocks.mockFxaMailer();
12001208
});
12011209
afterEach(() => {

‎packages/fxa-auth-server/test/local/routes/password.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,6 +73,7 @@ describe('/password', () => {
7373
// mailer mock must be done before route creation/require
7474
// otherwise it won't pickup the mock we define because
7575
// of module caching
76+
mocks.mockOAuthClientInfo();
7677
mockFxaMailer = mocks.mockFxaMailer();
7778
mockAccountEventsManager = mocks.mockAccountEventsManager();
7879
glean.resetPassword.emailSent.reset();

‎packages/fxa-auth-server/test/local/routes/utils/signin.js‎

Lines changed: 38 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -56,6 +56,7 @@ describe('checkPassword', () => {
5656
let customs, db, signinUtils;
5757

5858
beforeEach(() => {
59+
mocks.mockOAuthClientInfo();
5960
db = mocks.mockDB();
6061
customs = {
6162
v2Enabled: sinon.spy(() => true),
@@ -786,6 +787,7 @@ describe('sendSigninNotifications', () => {
786787
db = mocks.mockDB();
787788
log = mocks.mockLog();
788789
mailer = mocks.mockMailer();
790+
mocks.mockOAuthClientInfo();
789791
fxaMailer = mocks.mockFxaMailer();
790792
metricsContext = mocks.mockMetricsContext();
791793
request = mocks.mockRequest(defaultMockRequestData(log, metricsContext));
@@ -1149,7 +1151,7 @@ describe('sendSigninNotifications', () => {
11491151
uaOSVersion: '11',
11501152
},
11511153
code: 'tokenVerifyCode',
1152-
clientName: 'Firefox',
1154+
clientName: 'sync',
11531155
redirectTo: request.payload.redirectTo,
11541156
service: undefined,
11551157
resume: request.payload.resume,
@@ -1187,7 +1189,17 @@ describe('sendSigninNotifications', () => {
11871189
});
11881190

11891191
it('emits correct notifications when verificationMethod=email-2fa', () => {
1190-
return sendSigninNotifications(
1192+
const oauthClientInfoMock = mocks.mockOAuthClientInfo({
1193+
fetch: sinon.stub().resolves({ name: undefined }),
1194+
});
1195+
// Re-initialize sendSigninNotifications to pick up the new mock
1196+
const localSendSigninNotifications = makeSigninUtils({
1197+
log,
1198+
db,
1199+
mailer,
1200+
config,
1201+
}).sendSigninNotifications;
1202+
return localSendSigninNotifications(
11911203
request,
11921204
accountRecord,
11931205
sessionToken,
@@ -1212,6 +1224,8 @@ describe('sendSigninNotifications', () => {
12121224
serviceName: undefined,
12131225
});
12141226

1227+
assert.calledOnce(oauthClientInfoMock.fetch);
1228+
12151229
assert.calledTwice(log.flowEvent);
12161230
assert.calledWithMatch(log.flowEvent.getCall(0), {
12171231
event: 'account.login',
@@ -1271,10 +1285,12 @@ describe('sendSigninNotifications', () => {
12711285

12721286
describe('email verification sending', () => {
12731287
beforeEach(() => {
1288+
mocks.mockOAuthClientInfo();
12741289
sessionToken.tokenVerified = false;
12751290
});
12761291

12771292
it('passes service parameter correctly when request wantsKeys', () => {
1293+
mocks.mockOAuthClientInfo();
12781294
request.query.keys = true;
12791295
request.query.service = 'sync';
12801296
return sendSigninNotifications(
@@ -1290,16 +1306,26 @@ describe('sendSigninNotifications', () => {
12901306
});
12911307

12921308
it('passes service parameter correctly when service is undefined', () => {
1309+
const oauthClientInfoMock = mocks.mockOAuthClientInfo();
12931310
request.payload.service = undefined;
1294-
return sendSigninNotifications(
1311+
1312+
// Re-initialize sendSigninNotifications to pick up the new oauthClientInfoMock
1313+
const localSendSigninNotifications = makeSigninUtils({
1314+
log,
1315+
db,
1316+
mailer,
1317+
config,
1318+
}).sendSigninNotifications;
1319+
1320+
return localSendSigninNotifications(
12951321
request,
12961322
accountRecord,
12971323
sessionToken,
12981324
'email-otp'
12991325
).then(() => {
13001326
assert.calledOnce(fxaMailer.sendVerifyLoginCodeEmail);
1301-
const callArgs = fxaMailer.sendVerifyLoginCodeEmail.getCall(0).args[0];
1302-
assert.equal(callArgs.serviceName, undefined);
1327+
assert.calledOnce(oauthClientInfoMock.fetch);
1328+
assert.calledWith(oauthClientInfoMock.fetch, undefined);
13031329
});
13041330
});
13051331

@@ -1369,6 +1395,9 @@ describe('sendSigninNotifications', () => {
13691395
sessionToken.tokenVerified = false;
13701396
sessionToken.tokenVerificationId = 'tokenVerifyCode';
13711397
sessionToken.mustVerify = true;
1398+
mocks.mockOAuthClientInfo({
1399+
fetch: sinon.stub().resolves({ name: 'mockOauthClientName' }),
1400+
});
13721401
const rpCmsConfig = {
13731402
clientId: '00f00f',
13741403
shared: {
@@ -1422,7 +1451,7 @@ describe('sendSigninNotifications', () => {
14221451
entrypoint: 'testo',
14231452
redirectTo: req.payload.redirectTo,
14241453
resume: req.payload.resume,
1425-
serviceName: undefined,
1454+
serviceName: 'mockOauthClientName',
14261455
cmsRpClientId: rpCmsConfig.clientId,
14271456
cmsRpFromName: rpCmsConfig.shared?.emailFromName,
14281457
logoUrl: rpCmsConfig?.shared?.emailLogoUrl,
@@ -1531,6 +1560,7 @@ describe('createKeyFetchToken', () => {
15311560
createKeyFetchToken;
15321561

15331562
beforeEach(() => {
1563+
mocks.mockOAuthClientInfo();
15341564
db = mocks.mockDB();
15351565
password = {
15361566
unwrap: sinon.spy(() => Promise.resolve(Buffer.from('abcdef123456'))),
@@ -1602,6 +1632,7 @@ describe('getSessionVerificationStatus', () => {
16021632
let getSessionVerificationStatus;
16031633

16041634
beforeEach(() => {
1635+
mocks.mockOAuthClientInfo();
16051636
getSessionVerificationStatus = makeSigninUtils(
16061637
{}
16071638
).getSessionVerificationStatus;
@@ -1691,6 +1722,7 @@ describe('cleanupReminders', () => {
16911722
let cleanupReminders, mockCadReminders;
16921723

16931724
beforeEach(() => {
1725+
mocks.mockOAuthClientInfo();
16941726
mockCadReminders = mocks.mockCadReminders();
16951727
cleanupReminders = makeSigninUtils({
16961728
cadReminders: mockCadReminders,

‎packages/fxa-auth-server/test/mocks.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1215,7 +1215,7 @@ function mockFxaMailer(overrides) {
12151215

12161216
function mockOAuthClientInfo(overrides) {
12171217
const mock = {
1218-
fetch: sinon.stub().resolves('sync'),
1218+
fetch: sinon.stub().resolves({ name: 'sync' }),
12191219
...overrides,
12201220
};
12211221
Container.set(OAuthClientInfoServiceName, mock);

0 commit comments

Comments
 (0)