Skip to content

Commit 2901752

Browse files
vbudhramvbudhram
authored
Merge pull request #20358 from mozilla/investigate-basket
fix(auth): emit notifyAttachedServices for passwordless and linked-accounts
2 parents ed4693b + 8571684 commit 2901752

5 files changed

Lines changed: 248 additions & 51 deletions

File tree

‎packages/fxa-auth-server/lib/routes/linked-accounts.spec.ts‎

Lines changed: 61 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,25 @@ describe('/linked_account', () => {
285285
glean.thirdPartyAuth.googleRegComplete,
286286
mockRequest
287287
);
288+
289+
// Should emit SNS verified + login + profileDataChange events so
290+
// Basket/Braze learn about the new account.
291+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
292+
(call: any[]) => call[0]
293+
);
294+
expect(notifyEvents).toContain('verified');
295+
expect(notifyEvents).toContain('login');
296+
expect(notifyEvents).toContain('profileDataChange');
297+
sinon.assert.calledWithMatch(
298+
mockLog.notifyAttachedServices,
299+
'verified',
300+
mockRequest,
301+
sinon.match({
302+
email: mockGoogleUser.email,
303+
uid: UID,
304+
service: 'sync',
305+
})
306+
);
288307
});
289308

290309
it('should link existing fxa account and new google account and return session', async () => {
@@ -315,6 +334,15 @@ describe('/linked_account', () => {
315334
mockRequest,
316335
{ reason: 'linking' }
317336
);
337+
338+
// Should emit SNS login + profileDataChange but NOT verified
339+
// (the account already existed).
340+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
341+
(call: any[]) => call[0]
342+
);
343+
expect(notifyEvents).not.toContain('verified');
344+
expect(notifyEvents).toContain('login');
345+
expect(notifyEvents).toContain('profileDataChange');
318346
});
319347

320348
it('should return session with valid google id token', async () => {
@@ -338,6 +366,12 @@ describe('/linked_account', () => {
338366
expect(mockDB.createSessionToken.calledOnce).toBe(true);
339367
expect(result.uid).toBe(UID);
340368
expect(result.sessionToken).toBeTruthy();
369+
370+
// Re-login: login event only, no verified, no profileDataChange.
371+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
372+
(call: any[]) => call[0]
373+
);
374+
expect(notifyEvents).toEqual(['login']);
341375
sinon.assert.calledOnceWithExactly(
342376
glean.thirdPartyAuth.googleLoginComplete,
343377
mockRequest
@@ -516,6 +550,14 @@ describe('/linked_account', () => {
516550
glean.thirdPartyAuth.appleRegComplete,
517551
mockRequest
518552
);
553+
554+
// Should emit SNS verified + login + profileDataChange events.
555+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
556+
(call: any[]) => call[0]
557+
);
558+
expect(notifyEvents).toContain('verified');
559+
expect(notifyEvents).toContain('login');
560+
expect(notifyEvents).toContain('profileDataChange');
519561
});
520562

521563
it('should link existing fxa account and new apple account and return session', async () => {
@@ -544,6 +586,14 @@ describe('/linked_account', () => {
544586
mockRequest,
545587
{ reason: 'linking' }
546588
);
589+
590+
// New link on existing account: login + profileDataChange, no verified.
591+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
592+
(call: any[]) => call[0]
593+
);
594+
expect(notifyEvents).not.toContain('verified');
595+
expect(notifyEvents).toContain('login');
596+
expect(notifyEvents).toContain('profileDataChange');
547597
});
548598

549599
it('should return session with valid apple id token', async () => {
@@ -571,6 +621,12 @@ describe('/linked_account', () => {
571621
glean.thirdPartyAuth.appleLoginComplete,
572622
mockRequest
573623
);
624+
625+
// Re-login: login event only.
626+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
627+
(call: any[]) => call[0]
628+
);
629+
expect(notifyEvents).toEqual(['login']);
574630
});
575631
});
576632
});
@@ -912,11 +968,7 @@ describe('/linked_account', () => {
912968
mockLog.debug,
913969
'Revoked 1 third party sessions for user fxauid'
914970
);
915-
sinon.assert.calledWithExactly(
916-
mockDB.deleteLinkedAccount,
917-
UID,
918-
'google'
919-
);
971+
sinon.assert.calledWithExactly(mockDB.deleteLinkedAccount, UID, 'google');
920972
});
921973

922974
it('handles credentials changed event', async () => {
@@ -967,11 +1019,7 @@ describe('/linked_account', () => {
9671019
mockLog.debug,
9681020
'Revoked 1 third party sessions for user fxauid'
9691021
);
970-
sinon.assert.calledWithExactly(
971-
mockDB.deleteLinkedAccount,
972-
UID,
973-
'google'
974-
);
1022+
sinon.assert.calledWithExactly(mockDB.deleteLinkedAccount, UID, 'google');
9751023
});
9761024

9771025
it('handles account enabled event', async () => {
@@ -1204,9 +1252,7 @@ describe('/linked_account', () => {
12041252
},
12051253
],
12061254
});
1207-
mockDB.getLinkedAccount = sinon.spy(() =>
1208-
Promise.resolve({ uid: UID })
1209-
);
1255+
mockDB.getLinkedAccount = sinon.spy(() => Promise.resolve({ uid: UID }));
12101256
const mockConfig = {
12111257
appleAuthConfig: { clientId: 'OooOoo', teamId: 'teamId' },
12121258
};
@@ -1288,11 +1334,7 @@ describe('/linked_account', () => {
12881334
mockLog.debug,
12891335
'Revoked 1 third party sessions for user fxauid'
12901336
);
1291-
sinon.assert.calledWithExactly(
1292-
mockDB.deleteLinkedAccount,
1293-
UID,
1294-
'apple'
1295-
);
1337+
sinon.assert.calledWithExactly(mockDB.deleteLinkedAccount, UID, 'apple');
12961338
sinon.assert.calledWithExactly(
12971339
statsd.increment,
12981340
'handleAppleSET.processed.consent-revoked'
@@ -1319,11 +1361,7 @@ describe('/linked_account', () => {
13191361
mockLog.debug,
13201362
'Revoked 1 third party sessions for user fxauid'
13211363
);
1322-
sinon.assert.calledWithExactly(
1323-
mockDB.deleteLinkedAccount,
1324-
UID,
1325-
'apple'
1326-
);
1364+
sinon.assert.calledWithExactly(mockDB.deleteLinkedAccount, UID, 'apple');
13271365
sinon.assert.calledWithExactly(
13281366
statsd.increment,
13291367
'handleAppleSET.processed.account-delete'

‎packages/fxa-auth-server/lib/routes/linked-accounts.ts‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import isA from 'joi';
1919
import DESCRIPTION from '../../docs/swagger/shared/descriptions';
2020
import { AppError as error } from '@fxa/accounts/errors';
2121
import { schema as METRICS_CONTEXT_SCHEMA } from '../metrics/context';
22+
import { notifyAttachedServicesForAccountSession } from './utils/account';
2223
import {
2324
getGooglePublicKey,
2425
getApplePublicKey,
@@ -351,6 +352,13 @@ export class LinkedAccountHandler {
351352
const name = idToken.name;
352353

353354
let accountRecord;
355+
// Tracks which of the three third-party auth paths we took so the
356+
// SNS notification block after the if/else can emit the right events.
357+
let linkedAccountFlow:
358+
| 'new-link-existing-account'
359+
| 'new-account'
360+
| 'existing-linked-account'
361+
| undefined;
354362
const linkedAccountRecord = await this.db.getLinkedAccount(
355363
userid,
356364
provider
@@ -433,6 +441,7 @@ export class LinkedAccountHandler {
433441
flowBeginTime,
434442
service,
435443
});
444+
linkedAccountFlow = 'new-link-existing-account';
436445
} catch (err) {
437446
this.log.trace(
438447
'Account.login.sendPostAddLinkedAccountNotification.error',
@@ -503,6 +512,7 @@ export class LinkedAccountHandler {
503512
uid: accountRecord.uid,
504513
reason: provider === 'google' ? 'google' : 'apple',
505514
});
515+
linkedAccountFlow = 'new-account';
506516
}
507517
} else {
508518
// This is an existing user and existing FxA user
@@ -531,6 +541,7 @@ export class LinkedAccountHandler {
531541
uid: accountRecord.uid,
532542
reason: provider === 'google' ? 'google' : 'apple',
533543
});
544+
linkedAccountFlow = 'existing-linked-account';
534545
}
535546

536547
let verificationMethod,
@@ -562,6 +573,29 @@ export class LinkedAccountHandler {
562573

563574
const sessionToken = await this.db.createSessionToken(sessionTokenOptions);
564575

576+
// Mirror the SNS notifications that AccountHandler.createAccount
577+
// fires. Placed after createSessionToken so db.sessions already
578+
// includes the new session. A new third-party link on an existing
579+
// account counts as a profile change (the link was added).
580+
const isNewAccount = linkedAccountFlow === 'new-account';
581+
const deviceCount = isNewAccount
582+
? 1
583+
: (await this.db.sessions(accountRecord.uid)).length;
584+
await notifyAttachedServicesForAccountSession({
585+
log: this.log,
586+
request,
587+
account: {
588+
uid: accountRecord.uid,
589+
email: accountRecord.primaryEmail.email,
590+
locale: accountRecord.locale,
591+
},
592+
service,
593+
deviceCount,
594+
isNewAccount,
595+
emailVerified: true,
596+
profileChanged: linkedAccountFlow === 'new-link-existing-account',
597+
});
598+
565599
return {
566600
uid: sessionToken.uid,
567601
sessionToken: sessionToken.data,

‎packages/fxa-auth-server/lib/routes/passwordless.spec.ts‎

Lines changed: 43 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -79,10 +79,14 @@ jest.mock('./utils/security-event', () => ({
7979
// we simply reassign the stub per-test.
8080
let getOptionalCmsEmailConfigStub: sinon.SinonStub;
8181

82-
jest.mock('./utils/account', () => ({
83-
getOptionalCmsEmailConfig: (...args: any[]) =>
84-
getOptionalCmsEmailConfigStub(...args),
85-
}));
82+
jest.mock('./utils/account', () => {
83+
const actual = jest.requireActual('./utils/account');
84+
return {
85+
...actual,
86+
getOptionalCmsEmailConfig: (...args: any[]) =>
87+
getOptionalCmsEmailConfigStub(...args),
88+
};
89+
});
8690

8791
// ---------------------------------------------------------------------------
8892
// Route factory (mirrors the Mocha `makeRoutes`)
@@ -142,11 +146,7 @@ function makeRoutes(options: any = {}) {
142146
return passwordlessRoutes(log, db, config, customs, glean, statsd, redis);
143147
}
144148

145-
function runTest(
146-
route: any,
147-
request: any,
148-
assertions?: (result: any) => void
149-
) {
149+
function runTest(route: any, request: any, assertions?: (result: any) => void) {
150150
return new Promise((resolve, reject) => {
151151
try {
152152
return route.handler(request).then(resolve, reject);
@@ -382,9 +382,7 @@ describe('/account/passwordless/confirm_code', () => {
382382

383383
expect(mockCustoms.check.callCount).toBe(2);
384384
expect(mockCustoms.check.args[0][2]).toBe('passwordlessVerifyOtp');
385-
expect(mockCustoms.check.args[1][2]).toBe(
386-
'passwordlessVerifyOtpPerDay'
387-
);
385+
expect(mockCustoms.check.args[1][2]).toBe('passwordlessVerifyOtpPerDay');
388386

389387
expect(mockOtpManagerIsValid.callCount).toBe(1);
390388
expect(mockOtpManagerIsValid.args[0][0]).toBe(TEST_EMAIL);
@@ -406,6 +404,22 @@ describe('/account/passwordless/confirm_code', () => {
406404
expect(result.verified).toBe(true);
407405
expect(result.authAt).toBe(1234567890);
408406
expect(result.isNewAccount).toBe(true);
407+
408+
// Should emit SNS verified + login + profileDataChange events so
409+
// Basket/Braze learn about the new passwordless account. Missing
410+
// these calls was the root cause of the basket regression (FXA-13416).
411+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
412+
(call: any[]) => call[0]
413+
);
414+
expect(notifyEvents).toContain('verified');
415+
expect(notifyEvents).toContain('login');
416+
expect(notifyEvents).toContain('profileDataChange');
417+
sinon.assert.calledWithMatch(
418+
mockLog.notifyAttachedServices,
419+
'verified',
420+
mockRequest,
421+
sinon.match({ email: TEST_EMAIL, uid })
422+
);
409423
});
410424
});
411425

@@ -426,6 +440,7 @@ describe('/account/passwordless/confirm_code', () => {
426440
lastAuthAt: () => 1234567890,
427441
})
428442
);
443+
mockDB.sessions = sinon.spy(() => Promise.resolve([{}, {}, {}]));
429444

430445
return runTest(route, mockRequest, (result) => {
431446
expect(mockOtpManagerIsValid.callCount).toBe(1);
@@ -441,6 +456,19 @@ describe('/account/passwordless/confirm_code', () => {
441456

442457
expect(result.uid).toBe(uid);
443458
expect(result.isNewAccount).toBe(false);
459+
460+
// Existing account: login event only, no verified, no
461+
// profileDataChange. deviceCount should come from db.sessions.
462+
const notifyEvents = mockLog.notifyAttachedServices.args.map(
463+
(call: any[]) => call[0]
464+
);
465+
expect(notifyEvents).toEqual(['login']);
466+
sinon.assert.calledWithMatch(
467+
mockLog.notifyAttachedServices,
468+
'login',
469+
mockRequest,
470+
sinon.match({ email: TEST_EMAIL, uid, deviceCount: 3 })
471+
);
444472
});
445473
});
446474

@@ -1876,11 +1904,7 @@ describe('existing passwordless accounts bypass flag and allowlist', () => {
18761904
},
18771905
},
18781906
});
1879-
const route = getRoute(
1880-
routes,
1881-
'/account/passwordless/send_code',
1882-
'POST'
1883-
);
1907+
const route = getRoute(routes, '/account/passwordless/send_code', 'POST');
18841908
mockDB.accountRecord = sinon.spy(() =>
18851909
Promise.resolve({
18861910
uid,
@@ -1910,11 +1934,7 @@ describe('existing passwordless accounts bypass flag and allowlist', () => {
19101934
},
19111935
},
19121936
});
1913-
const route = getRoute(
1914-
routes,
1915-
'/account/passwordless/send_code',
1916-
'POST'
1917-
);
1937+
const route = getRoute(routes, '/account/passwordless/send_code', 'POST');
19181938
mockDB.accountRecord = sinon.spy(() =>
19191939
Promise.resolve({
19201940
uid,
@@ -1944,11 +1964,7 @@ describe('existing passwordless accounts bypass flag and allowlist', () => {
19441964
},
19451965
},
19461966
});
1947-
const route = getRoute(
1948-
routes,
1949-
'/account/passwordless/send_code',
1950-
'POST'
1951-
);
1967+
const route = getRoute(routes, '/account/passwordless/send_code', 'POST');
19521968
mockDB.accountRecord = sinon.spy(() =>
19531969
Promise.reject(error.unknownAccount())
19541970
);

0 commit comments

Comments
 (0)