Skip to content

Commit f641ada

Browse files
vbudhramvbudhram
authored
Merge pull request #18382 from mozilla/fxa-11113
feat(security): Add security events for recovery phone, recovery codes
2 parents ceaa834 + d98af9c commit f641ada

11 files changed

Lines changed: 298 additions & 9 deletions

File tree

‎packages/db-migrations/databases/fxa/patches/patch-162-163.sql‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,14 @@
1+
SET NAMES utf8mb4 COLLATE utf8mb4_bin;
2+
3+
CALL assertPatchLevel('162');
4+
5+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_phone_send_code');
6+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_phone_setup_complete');
7+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_phone_signin_complete');
8+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_phone_signin_failed');
9+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_phone_removed');
10+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_codes_replaced');
11+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_codes_created');
12+
INSERT INTO securityEventNames(name) VALUES ('account.recovery_codes_signin_complete');
13+
14+
UPDATE dbMetadata SET value = '163' WHERE name = 'schema-patch-level';

‎packages/db-migrations/databases/fxa/patches/patch-163-162.sql‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
-- SET NAMES utf8mb4 COLLATE utf8mb4_bin;
2+
--
3+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_phone_send_code';
4+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_phone_setup_complete';
5+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_phone_signin_complete';
6+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_phone_signin_failed';
7+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_phone_removed';
8+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_codes_replaced';
9+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_codes_created';
10+
-- DELETE FROM securityEventNames WHERE name = 'account.recovery_codes_signin_complete';
11+
--
12+
-- UPDATE dbMetadata SET value = '162' WHERE name = 'schema-patch-level';

‎packages/db-migrations/databases/fxa/target-patch.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,3 @@
11
{
2-
"level": 162
2+
"level": 163
33
}

‎packages/fxa-auth-server/lib/routes/recovery-codes.js‎

Lines changed: 33 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,13 +11,15 @@ const { Container } = require('typedi');
1111
const RECOVERY_CODES_DOCS =
1212
require('../../docs/swagger/recovery-codes-api').default;
1313
const { BackupCodeManager } = require('@fxa/accounts/two-factor');
14+
const { AccountEventsManager } = require('../account-events');
1415

1516
const RECOVERY_CODE_SANE_MAX_LENGTH = 20;
1617

1718
module.exports = (log, db, config, customs, mailer, glean) => {
1819
const codeConfig = config.recoveryCodes;
1920
const RECOVERY_CODE_COUNT = (codeConfig && codeConfig.count) || 8;
2021
const backupCodeManager = Container.get(BackupCodeManager);
22+
const accountEventsManager = Container.get(AccountEventsManager);
2123

2224
// Validate backup authentication codes
2325
const recoveryCodesSchema = validators.recoveryCodes(
@@ -41,7 +43,11 @@ module.exports = (log, db, config, customs, mailer, glean) => {
4143
async handler(request) {
4244
log.begin('replaceRecoveryCodes', request);
4345

44-
const { authenticatorAssuranceLevel, uid } = request.auth.credentials;
46+
const {
47+
authenticatorAssuranceLevel,
48+
uid,
49+
id: sessionTokenId,
50+
} = request.auth.credentials;
4551

4652
// Since TOTP and backup authentication codes go hand in hand, you should only be
4753
// able to replace backup authentication codes in a TOTP verified session.
@@ -54,6 +60,13 @@ module.exports = (log, db, config, customs, mailer, glean) => {
5460
RECOVERY_CODE_COUNT
5561
);
5662

63+
accountEventsManager.recordSecurityEvent(db, {
64+
name: 'account.recovery_codes_replaced',
65+
uid,
66+
ipAddr: request.app.clientAddress,
67+
tokenId: sessionTokenId,
68+
});
69+
5770
const account = await db.account(uid);
5871
const { acceptLanguage, clientAddress: geo, ua } = request.app;
5972

@@ -94,7 +107,11 @@ module.exports = (log, db, config, customs, mailer, glean) => {
94107
async handler(request) {
95108
log.begin('updateRecoveryCodes', request);
96109

97-
const { authenticatorAssuranceLevel, uid } = request.auth.credentials;
110+
const {
111+
authenticatorAssuranceLevel,
112+
uid,
113+
id: sessionTokenId,
114+
} = request.auth.credentials;
98115

99116
// Since TOTP and backup authentication codes go hand in hand, you should only be
100117
// able to replace backup authentication codes in a TOTP verified session.
@@ -120,6 +137,13 @@ module.exports = (log, db, config, customs, mailer, glean) => {
120137
uid,
121138
});
122139

140+
accountEventsManager.recordSecurityEvent(db, {
141+
name: 'account.recovery_codes_created',
142+
uid,
143+
ipAddr: request.app.clientAddress,
144+
tokenId: sessionTokenId,
145+
});
146+
123147
log.info('account.recoveryCode.replaced', { uid });
124148

125149
await request.emitMetricsEvent('recoveryCode.replaced', { uid });
@@ -241,6 +265,13 @@ module.exports = (log, db, config, customs, mailer, glean) => {
241265
await request.emitMetricsEvent('recoveryCode.verified', { uid });
242266
glean.login.recoveryCodeSuccess(request, { uid });
243267

268+
accountEventsManager.recordSecurityEvent(db, {
269+
name: 'account.recovery_codes_signin_complete',
270+
uid,
271+
ipAddr: request.app.clientAddress,
272+
tokenId,
273+
});
274+
244275
return { remaining };
245276
},
246277
},

‎packages/fxa-auth-server/lib/routes/recovery-phone.ts‎

Lines changed: 47 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import { E164_NUMBER } from './validators';
2323
import AppError from '../error';
2424
import Localizer from '../l10n';
2525
import NodeRendererBindings from '../senders/renderer/bindings-node';
26+
import { AccountEventsManager } from '../account-events';
2627

2728
const { Container } = require('typedi');
2829

@@ -43,6 +44,7 @@ export type Customs = {
4344
class RecoveryPhoneHandler {
4445
private readonly recoveryPhoneService: RecoveryPhoneService;
4546
private readonly accountManager: AccountManager;
47+
private readonly accountEventsManager: AccountEventsManager;
4648
private readonly localizer: Localizer;
4749

4850
constructor(
@@ -54,6 +56,7 @@ class RecoveryPhoneHandler {
5456
) {
5557
this.recoveryPhoneService = Container.get(RecoveryPhoneService);
5658
this.accountManager = Container.get(AccountManager);
59+
this.accountEventsManager = Container.get(AccountEventsManager);
5760
this.localizer = new Localizer(new NodeRendererBindings());
5861
}
5962

@@ -86,8 +89,11 @@ class RecoveryPhoneHandler {
8689
};
8790

8891
async sendCode(request: AuthRequest) {
89-
const { uid, email } = request.auth
90-
.credentials as SessionTokenAuthCredential;
92+
const {
93+
uid,
94+
email,
95+
id: sessionTokenId,
96+
} = request.auth.credentials as SessionTokenAuthCredential;
9197

9298
if (!email) {
9399
throw AppError.invalidToken();
@@ -134,6 +140,14 @@ class RecoveryPhoneHandler {
134140
if (success) {
135141
this.log.info('account.recoveryPhone.signinSendCode.success', { uid });
136142
await this.glean.twoStepAuthPhoneCode.sent(request);
143+
144+
this.accountEventsManager.recordSecurityEvent(this.db, {
145+
name: 'account.recovery_phone_send_code',
146+
uid,
147+
ipAddr: request.app.clientAddress,
148+
tokenId: sessionTokenId,
149+
});
150+
137151
return { status: RecoveryPhoneStatus.SUCCESS };
138152
}
139153

@@ -308,6 +322,14 @@ class RecoveryPhoneHandler {
308322
uid,
309323
}
310324
);
325+
326+
this.accountEventsManager.recordSecurityEvent(this.db, {
327+
name: 'account.recovery_phone_setup_complete',
328+
uid,
329+
ipAddr: request.app.clientAddress,
330+
tokenId: sessionTokenId,
331+
});
332+
311333
return {
312334
phoneNumber,
313335
nationalFormat,
@@ -324,6 +346,13 @@ class RecoveryPhoneHandler {
324346
} else {
325347
this.log.info('account.recoveryPhone.phoneSignin.success', { uid });
326348

349+
this.accountEventsManager.recordSecurityEvent(this.db, {
350+
name: 'account.recovery_phone_signin_complete',
351+
uid,
352+
ipAddr: request.app.clientAddress,
353+
tokenId: sessionTokenId,
354+
});
355+
327356
try {
328357
await this.mailer.sendPostSigninRecoveryPhoneEmail(
329358
account.emails,
@@ -354,11 +383,19 @@ class RecoveryPhoneHandler {
354383
return { status: RecoveryPhoneStatus.SUCCESS };
355384
}
356385

386+
this.accountEventsManager.recordSecurityEvent(this.db, {
387+
name: 'account.recovery_phone_signin_failed',
388+
uid,
389+
ipAddr: request.app.clientAddress,
390+
tokenId: sessionTokenId,
391+
});
392+
357393
throw AppError.invalidOrExpiredOtpCode();
358394
}
359395

360396
async destroy(request: AuthRequest) {
361-
const { uid } = request.auth.credentials as unknown as { uid: string };
397+
const sessionToken = request.auth.credentials as SessionTokenAuthCredential;
398+
const { uid } = sessionToken;
362399

363400
let success = false;
364401
try {
@@ -392,6 +429,13 @@ class RecoveryPhoneHandler {
392429
const { acceptLanguage, geo, ua } = request.app;
393430

394431
try {
432+
this.accountEventsManager.recordSecurityEvent(this.db, {
433+
name: 'account.recovery_phone_removed',
434+
uid,
435+
ipAddr: request.app.clientAddress,
436+
tokenId: sessionToken.id,
437+
});
438+
395439
await this.mailer.sendPostRemoveRecoveryPhoneEmail(
396440
account.emails,
397441
account,

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

Lines changed: 37 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ const mocks = require('../../mocks');
1111
const error = require('../../../lib/error');
1212
const { Container } = require('typedi');
1313
const { BackupCodeManager } = require('@fxa/accounts/two-factor');
14+
const { AccountEventsManager } = require('../../../lib/account-events');
1415

1516
let log, db, customs, routes, route, request, requestOptions, mailer, glean;
1617
const TEST_EMAIL = '[email protected]';
@@ -45,7 +46,9 @@ describe('backup authentication codes', () => {
4546
const mockBackupCodeManager = {
4647
getCountForUserId: sandbox.fake(),
4748
};
48-
49+
const mockAccountEventsManager = {
50+
recordSecurityEvent: sandbox.fake(),
51+
};
4952
beforeEach(() => {
5053
log = mocks.mockLog();
5154
customs = mocks.mockCustoms();
@@ -71,10 +74,11 @@ describe('backup authentication codes', () => {
7174
},
7275
};
7376
Container.set(BackupCodeManager, mockBackupCodeManager);
77+
Container.set(AccountEventsManager, mockAccountEventsManager);
7478
});
7579

7680
afterEach(() => {
77-
sandbox.restore();
81+
sandbox.reset();
7882
});
7983

8084
describe('GET /recoveryCodes', () => {
@@ -91,6 +95,16 @@ describe('backup authentication codes', () => {
9195
8,
9296
'called with backup authentication code count'
9397
);
98+
assert.calledOnceWithExactly(
99+
mockAccountEventsManager.recordSecurityEvent,
100+
db,
101+
{
102+
name: 'account.recovery_codes_replaced',
103+
uid: UID,
104+
ipAddr: '63.245.221.32',
105+
tokenId: undefined,
106+
}
107+
);
94108
});
95109
});
96110

@@ -122,6 +136,16 @@ describe('backup authentication codes', () => {
122136
['123'],
123137
'called with backup authentication codes'
124138
);
139+
assert.calledOnceWithExactly(
140+
mockAccountEventsManager.recordSecurityEvent,
141+
db,
142+
{
143+
name: 'account.recovery_codes_created',
144+
uid: UID,
145+
ipAddr: '63.245.221.32',
146+
tokenId: undefined,
147+
}
148+
);
125149
});
126150
});
127151

@@ -174,6 +198,17 @@ describe('backup authentication codes', () => {
174198
const args = mailer.sendLowRecoveryCodesEmail.args[0];
175199
assert.lengthOf(args, 3);
176200
assert.equal(args[2].numberRemaining, 1);
201+
202+
assert.calledOnceWithExactly(
203+
mockAccountEventsManager.recordSecurityEvent,
204+
db,
205+
{
206+
name: 'account.recovery_codes_signin_complete',
207+
uid: UID,
208+
ipAddr: '63.245.221.32',
209+
tokenId: undefined,
210+
}
211+
);
177212
});
178213

179214
it('should rate-limit attempts to use a backup authentication code via customs', () => {

0 commit comments

Comments
 (0)