Skip to content

Commit e00da7c

Browse files
dschomdschom
committed
bug(auth): Fix broken functional tests due to rate-limiting
Because: - We were tripping rate limits during functional tests This Commit: - Adds email to `customs.checkAuthenticated` - Fixes the verifySessionCode check. We can check on UID here, so we don't need to also check IP since the endpoint is protected by a session token.
1 parent 3003f93 commit e00da7c

15 files changed

Lines changed: 74 additions & 31 deletions

File tree

‎packages/fxa-auth-server/config/rate-limit-rules.txt‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,6 @@
6767
passwordForgotVerifyCode : email : 10 : 15 minutes : 15 minutes
6868
verifyRecoveryCode : ip : 10 : 10 minutes : 30 minutes
6969
verifyRecoveryCode : email : 10 : 15 minutes : 15 minutes
70-
verifySessionCode : ip : 10 : 10 minutes : 30 minutes
7170
verifySessionCode : uid : 10 : 15 minutes : 15 minutes
7271

7372
# Verify TOTP Code Limits

‎packages/fxa-auth-server/lib/customs.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ class CustomsClient {
117117
return this.handleCustomsResult(request, result);
118118
}
119119

120-
async checkAuthenticated(request, uid, action) {
120+
async checkAuthenticated(request, uid, email, action) {
121121
const checked = await this.checkV2(request, 'checkAuthenticated', action, {
122122
ip: request?.app?.clientAddress,
123123
uid,

‎packages/fxa-auth-server/lib/routes/devices-and-sessions.js‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -322,6 +322,7 @@ module.exports = (
322322
let { ttl } = request.payload;
323323
const credentials = request.auth.credentials;
324324
const uid = credentials.uid;
325+
const email = credentials.email;
325326
const sender = credentials.deviceId;
326327

327328
if (
@@ -331,7 +332,12 @@ module.exports = (
331332
throw new error.featureNotEnabled();
332333
}
333334

334-
await customs.checkAuthenticated(request, uid, 'invokeDeviceCommand');
335+
await customs.checkAuthenticated(
336+
request,
337+
uid,
338+
email,
339+
'invokeDeviceCommand'
340+
);
335341

336342
const targetDevice = await db.device(uid, target);
337343

@@ -468,6 +474,7 @@ module.exports = (
468474
const body = request.payload;
469475
const sessionToken = request.auth.credentials;
470476
const uid = sessionToken.uid;
477+
const email = sessionToken.email;
471478
const payload = body.payload;
472479
const endpointAction = body._endpointAction || 'devicesNotify';
473480

@@ -484,7 +491,7 @@ module.exports = (
484491
}
485492

486493
let [, deviceArray] = await Promise.all([
487-
customs.checkAuthenticated(request, uid, endpointAction),
494+
customs.checkAuthenticated(request, uid, email, endpointAction),
488495
request.app.devices,
489496
]);
490497

‎packages/fxa-auth-server/lib/routes/password.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,7 @@ module.exports = function (
108108
await customs.checkAuthenticated(
109109
request,
110110
sessionToken.uid,
111+
sessionToken.email,
111112
customs.v2Enabled()
112113
? 'authenticatedPasswordChange'
113114
: 'passwordChange'

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

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,7 +203,7 @@ module.exports = (
203203
log.begin('verifyRecoveryKey', request);
204204

205205
const sessionToken = request.auth.credentials;
206-
const { uid } = sessionToken;
206+
const { uid, email } = sessionToken;
207207

208208
try {
209209
if (sessionToken.tokenVerificationId) {
@@ -212,7 +212,12 @@ module.exports = (
212212

213213
// This route can let you check if a key is valid therefore we
214214
// rate limit it.
215-
await customs.checkAuthenticated(request, uid, 'getRecoveryKey');
215+
await customs.checkAuthenticated(
216+
request,
217+
uid,
218+
email,
219+
'getRecoveryKey'
220+
);
216221

217222
const { recoveryKeyId } = request.payload;
218223

@@ -278,10 +283,10 @@ module.exports = (
278283
handler: async function (request) {
279284
log.begin('getRecoveryKey', request);
280285

281-
const { uid } = request.auth.credentials;
286+
const { uid, email } = request.auth.credentials;
282287
const { recoveryKeyId } = request.params;
283288

284-
await customs.checkAuthenticated(request, uid, 'getRecoveryKey');
289+
await customs.checkAuthenticated(request, uid, email, 'getRecoveryKey');
285290

286291
const { recoveryData } = await db.getRecoveryKey(uid, recoveryKeyId);
287292

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

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@ export type Customs = {
4848
checkAuthenticated: (
4949
req: AuthRequest,
5050
uid: string,
51+
email: string,
5152
action: string
5253
) => Promise<void>;
5354
};
@@ -202,6 +203,7 @@ class RecoveryPhoneHandler {
202203
await this.customs.checkAuthenticated(
203204
request,
204205
uid,
206+
email,
205207
'recoveryPhoneSendResetPasswordCode'
206208
);
207209

@@ -285,6 +287,7 @@ class RecoveryPhoneHandler {
285287
await this.customs.checkAuthenticated(
286288
request,
287289
uid,
290+
email,
288291
'recoveryPhoneSendSetupCode'
289292
);
290293

@@ -381,6 +384,7 @@ class RecoveryPhoneHandler {
381384
await this.customs.checkAuthenticated(
382385
request,
383386
uid,
387+
email,
384388
'verifyRecoveryPhoneTotpCode'
385389
);
386390

@@ -645,6 +649,7 @@ class RecoveryPhoneHandler {
645649
await this.customs.checkAuthenticated(
646650
request,
647651
uid,
652+
email,
648653
'verifyRecoveryPhoneTotpCode'
649654
);
650655

@@ -665,10 +670,7 @@ class RecoveryPhoneHandler {
665670
}
666671

667672
if (success) {
668-
await this.db.verifyPasswordForgotTokenWithMethod(
669-
id,
670-
'totp-2fa'
671-
);
673+
await this.db.verifyPasswordForgotTokenWithMethod(id, 'totp-2fa');
672674

673675
await this.glean.resetPassword.recoveryPhoneCodeComplete(request);
674676

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

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -371,10 +371,15 @@ module.exports = function (
371371
const options = request.payload;
372372
const sessionToken = request.auth.credentials;
373373
const { code } = options;
374-
const { uid } = sessionToken;
374+
const { uid, email } = sessionToken;
375375
const devices = await request.app.devices;
376376

377-
await customs.checkAuthenticated(request, uid, 'verifySessionCode');
377+
await customs.checkAuthenticated(
378+
request,
379+
uid,
380+
email,
381+
'verifySessionCode'
382+
);
378383

379384
request.emitMetricsEvent('session.verify_code');
380385

@@ -618,10 +623,15 @@ module.exports = function (
618623
log.begin('Session.verify_push', request);
619624
const options = request.payload;
620625
const sessionToken = request.auth.credentials;
621-
const { uid } = sessionToken;
626+
const { uid, email } = sessionToken;
622627
const { code, tokenVerificationId } = options;
623628

624-
await customs.checkAuthenticated(request, uid, 'verifySessionCode');
629+
await customs.checkAuthenticated(
630+
request,
631+
uid,
632+
email,
633+
'verifySessionCode'
634+
);
625635
request.emitMetricsEvent('session.verify_push');
626636

627637
const device = await db.deviceFromTokenVerificationId(

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -346,6 +346,7 @@ module.exports = (
346346
await customs.checkAuthenticated(
347347
request,
348348
passwordForgotToken.uid,
349+
passwordForgotToken.email,
349350
'verifyTotpCode'
350351
);
351352

@@ -505,7 +506,7 @@ module.exports = (
505506
const sessionToken = request.auth.credentials;
506507
const { uid, email } = sessionToken;
507508

508-
await customs.checkAuthenticated(request, uid, 'verifyTotpCode');
509+
await customs.checkAuthenticated(request, uid, email, 'verifyTotpCode');
509510

510511
const token = await db.totpToken(sessionToken.uid);
511512
const sharedSecret = token.sharedSecret;

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,7 @@ module.exports = (
139139
await customs.checkAuthenticated(
140140
request,
141141
checkAuthenticatedUid,
142+
email,
142143
customs.v2Enabled ? 'authenticatedAccountLogin' : 'accountLogin'
143144
);
144145
} else {

‎packages/fxa-auth-server/test/local/customs.js‎

Lines changed: 13 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -511,6 +511,7 @@ describe('Customs', () => {
511511

512512
action = 'devicesNotify';
513513
const uid = 'foo';
514+
const email = '[email protected]';
514515

515516
function checkRequestBody(body) {
516517
assert.deepEqual(
@@ -540,42 +541,42 @@ describe('Customs', () => {
540541
.reply(200, '{"block":true,"retryAfter":10001}');
541542

542543
return customsWithUrl
543-
.checkAuthenticated(request, uid, action)
544+
.checkAuthenticated(request, uid, email, action)
544545
.then((result) => {
545546
assert.equal(
546547
result,
547548
undefined,
548549
'Nothing is returned when /checkAuthenticated succeeds - 1'
549550
);
550-
return customsWithUrl.checkAuthenticated(request, uid, action);
551+
return customsWithUrl.checkAuthenticated(request, uid, email, action);
551552
})
552553
.then((result) => {
553554
assert.equal(
554555
result,
555556
undefined,
556557
'Nothing is returned when /checkAuthenticated succeeds - 2'
557558
);
558-
return customsWithUrl.checkAuthenticated(request, uid, action);
559+
return customsWithUrl.checkAuthenticated(request, uid, email, action);
559560
})
560561
.then((result) => {
561562
assert.equal(
562563
result,
563564
undefined,
564565
'Nothing is returned when /checkAuthenticated succeeds - 3'
565566
);
566-
return customsWithUrl.checkAuthenticated(request, uid, action);
567+
return customsWithUrl.checkAuthenticated(request, uid, email, action);
567568
})
568569
.then((result) => {
569570
assert.equal(
570571
result,
571572
undefined,
572573
'Nothing is returned when /checkAuthenticated succeeds - 4'
573574
);
574-
return customsWithUrl.checkAuthenticated(request, uid, action);
575+
return customsWithUrl.checkAuthenticated(request, uid, email, action);
575576
})
576577
.then(() => {
577578
// request is blocked
578-
return customsWithUrl.checkAuthenticated(request, uid, action);
579+
return customsWithUrl.checkAuthenticated(request, uid, email, action);
579580
})
580581
.then(
581582
() => {
@@ -861,7 +862,12 @@ describe('Customs', () => {
861862
});
862863

863864
try {
864-
await customsWithUrl.checkAuthenticated(request, 'uid', action);
865+
await customsWithUrl.checkAuthenticated(
866+
request,
867+
'uid',
868+
'email',
869+
action
870+
);
865871
assert.fail('should have failed');
866872
} catch (err) {
867873
assert.isTrue(

0 commit comments

Comments
 (0)