Skip to content

Commit 0bbf8f5

Browse files
committed
task(settings): Debounce sending of code on click and render
Because: - Due to mobile issue the previous debounce technique might still have issues This Commit: - Caches record of when MFA OTP code requests occur - Uses record to determine whether or not to debounce - Also handles case where some one hammers the 'resend' button
1 parent 9b1a847 commit 0bbf8f5

5 files changed

Lines changed: 149 additions & 28 deletions

File tree

packages/fxa-settings/src/components/Settings/MfaGuard/error-boundary.test.tsx

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,21 +5,23 @@
55
import React from 'react';
66
import { render, screen } from '@testing-library/react';
77
import { MfaErrorBoundary } from './error-boundary';
8-
import { JwtTokenCache } from '../../../lib/cache';
8+
import { JwtTokenCache, MfaOtpRequestCache } from '../../../lib/cache';
99

1010
const mockScope = 'test';
1111
const mockSessionToken = 'session-xyz';
1212
const mockJwt = 'jwt-123';
1313

1414
describe('MfaErrorBoundary', () => {
15-
let removeSpy: jest.SpyInstance;
15+
let removeJwtSpy: jest.SpyInstance;
16+
let removeOtpSpy: jest.SpyInstance;
1617

1718
beforeEach(() => {
18-
removeSpy = jest.spyOn(JwtTokenCache, 'removeToken');
19+
removeJwtSpy = jest.spyOn(JwtTokenCache, 'removeToken');
20+
removeOtpSpy = jest.spyOn(MfaOtpRequestCache, 'remove');
1921
});
2022

2123
afterEach(() => {
22-
removeSpy.mockReset();
24+
removeJwtSpy.mockReset();
2325
});
2426

2527
it('renders children when no error occurs', () => {
@@ -72,6 +74,7 @@ describe('MfaErrorBoundary', () => {
7274
);
7375

7476
expect(screen.getByText('fallback')).toBeInTheDocument();
75-
expect(removeSpy).toHaveBeenCalledWith(mockSessionToken, mockScope);
77+
expect(removeJwtSpy).toHaveBeenCalledWith(mockSessionToken, mockScope);
78+
expect(removeOtpSpy).toHaveBeenCalledWith(mockSessionToken, mockScope);
7679
});
7780
});

packages/fxa-settings/src/components/Settings/MfaGuard/error-boundary.tsx

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@
44

55
import { Component, ReactNode } from 'react';
66
import { MfaScope } from '../../../lib/types';
7-
import { JwtTokenCache } from '../../../lib/cache';
7+
import { JwtTokenCache, MfaOtpRequestCache } from '../../../lib/cache';
88

99
/**
1010
* Error Boundary Implementation.
@@ -58,6 +58,10 @@ export class MfaErrorBoundary extends Component<
5858
this.props.sessionToken,
5959
this.props.requiredScope
6060
);
61+
MfaOtpRequestCache.remove(
62+
this.props.sessionToken,
63+
this.props.requiredScope
64+
);
6165
} else {
6266
// Causes error to bubble up to the next error boundary
6367
throw error;

packages/fxa-settings/src/components/Settings/MfaGuard/index.test.tsx

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -3,11 +3,11 @@
33
* file, You can obtain one at http://mozilla.org/MPL/2.0/. */
44

55
import React from 'react';
6-
import { screen, waitFor } from '@testing-library/react';
6+
import { act, screen, waitFor } from '@testing-library/react';
77
import userEvent from '@testing-library/user-event';
88
import { mockAppContext, renderWithRouter } from '../../../models/mocks';
99
import { MfaGuard } from './index';
10-
import { JwtTokenCache } from '../../../lib/cache';
10+
import { JwtTokenCache, MfaOtpRequestCache } from '../../../lib/cache';
1111
import { AuthUiErrors } from '../../../lib/auth-errors/auth-errors';
1212
import { AppContext } from '../../../models';
1313

@@ -56,9 +56,8 @@ async function submitCode(otp: string = mockOtp) {
5656

5757
describe('MfaGuard', () => {
5858
beforeEach(() => {
59-
if (JwtTokenCache.hasToken(mockSessionToken, mockScope)) {
60-
JwtTokenCache.removeToken(mockSessionToken, mockScope);
61-
}
59+
JwtTokenCache.removeToken(mockSessionToken, mockScope);
60+
MfaOtpRequestCache.remove(mockSessionToken, mockScope);
6261
jest.clearAllMocks();
6362
});
6463

@@ -127,7 +126,7 @@ describe('MfaGuard', () => {
127126
it('clears error banner on input change', async () => {
128127
renderWithRouter(
129128
<AppContext.Provider value={mockAppContext()}>
130-
<MfaGuard requiredScope={mockScope}>
129+
<MfaGuard requiredScope={mockScope} debounceIntervalMs={0}>
131130
<div>secured</div>
132131
</MfaGuard>
133132
</AppContext.Provider>
@@ -149,7 +148,7 @@ describe('MfaGuard', () => {
149148
it('shows resend success banner and hides error banner on resend success', async () => {
150149
renderWithRouter(
151150
<AppContext.Provider value={mockAppContext()}>
152-
<MfaGuard requiredScope={mockScope}>
151+
<MfaGuard requiredScope={mockScope} debounceIntervalMs={0}>
153152
<div>secured</div>
154153
</MfaGuard>
155154
</AppContext.Provider>
@@ -179,7 +178,7 @@ describe('MfaGuard', () => {
179178
it('shows error banner and hide success banner on resend error', async () => {
180179
renderWithRouter(
181180
<AppContext.Provider value={mockAppContext()}>
182-
<MfaGuard requiredScope={mockScope}>
181+
<MfaGuard requiredScope={mockScope} debounceIntervalMs={0}>
183182
<div>secured</div>
184183
</MfaGuard>
185184
</AppContext.Provider>
@@ -209,7 +208,7 @@ describe('MfaGuard', () => {
209208

210209
renderWithRouter(
211210
<AppContext.Provider value={mockAppContext()}>
212-
<MfaGuard requiredScope={mockScope}>
211+
<MfaGuard requiredScope={mockScope} debounceIntervalMs={0}>
213212
<div>secured</div>
214213
</MfaGuard>
215214
</AppContext.Provider>
@@ -226,7 +225,11 @@ describe('MfaGuard', () => {
226225

227226
renderWithRouter(
228227
<AppContext.Provider value={mockAppContext()}>
229-
<MfaGuard requiredScope={mockScope} onDismissCallback={mockOnDismiss}>
228+
<MfaGuard
229+
requiredScope={mockScope}
230+
onDismissCallback={mockOnDismiss}
231+
debounceIntervalMs={0}
232+
>
230233
<div>secured</div>
231234
</MfaGuard>
232235
</AppContext.Provider>
@@ -236,4 +239,39 @@ describe('MfaGuard', () => {
236239

237240
expect(mockOnDismiss).toHaveBeenCalledTimes(1);
238241
});
242+
243+
it('debounces OTP resend requests', async () => {
244+
renderWithRouter(
245+
<AppContext.Provider value={mockAppContext()}>
246+
<MfaGuard requiredScope={mockScope} debounceIntervalMs={100}>
247+
<div>secured</div>
248+
</MfaGuard>
249+
</AppContext.Provider>
250+
);
251+
252+
// Should be debounced! The dialog just rendered and a code went out...
253+
await userEvent.click(
254+
screen.getByRole('button', { name: 'Email new code.' })
255+
);
256+
await act(async () => {
257+
await new Promise((r) => setTimeout(r, 101));
258+
});
259+
260+
await userEvent.click(
261+
screen.getByRole('button', { name: 'Email new code.' })
262+
);
263+
// Should be debounced! The resend request above was just clicked...
264+
await userEvent.click(
265+
screen.getByRole('button', { name: 'Email new code.' })
266+
);
267+
268+
await act(async () => {
269+
await new Promise((r) => setTimeout(r, 101));
270+
});
271+
await userEvent.click(
272+
screen.getByRole('button', { name: 'Email new code.' })
273+
);
274+
275+
expect(mockAuthClient.mfaRequestOtp).toHaveBeenCalledTimes(3);
276+
});
239277
});

packages/fxa-settings/src/components/Settings/MfaGuard/index.tsx

Lines changed: 31 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import React, {
66
ReactNode,
77
useCallback,
88
useEffect,
9-
useRef,
109
useState,
1110
useSyncExternalStore,
1211
} from 'react';
@@ -22,6 +21,7 @@ import {
2221
import Modal from '../ModalMfaProtected';
2322
import {
2423
JwtTokenCache,
24+
MfaOtpRequestCache,
2525
sessionToken as getSessionToken,
2626
} from '../../../lib/cache';
2727
import { MfaScope } from '../../../lib/types';
@@ -39,26 +39,23 @@ export const MfaGuard = ({
3939
children,
4040
requiredScope,
4141
onDismissCallback = async () => {},
42+
debounceIntervalMs = 3000,
4243
}: {
4344
children: ReactNode;
4445
requiredScope: MfaScope;
4546
onDismissCallback?: () => Promise<void>;
47+
debounceIntervalMs?: number;
4648
}) => {
4749
// Let errors be handled by error boundaries in async contexts
4850
const handleError = useErrorHandler();
49-
5051
const config = useConfig();
51-
52-
const hasSentConfirmationCode = useRef(false);
53-
5452
const [localizedErrorBannerMessage, setLocalizedErrorBannerMessage] =
5553
useState<string | undefined>(undefined);
5654

5755
const [resendCodeLoading, setResendCodeLoading] = useState(false);
5856
const [showResendSuccessBanner, setShowResendSuccessBanner] = useState(false);
5957

6058
const resetStates = useCallback(() => {
61-
hasSentConfirmationCode.current = false;
6259
setLocalizedErrorBannerMessage(undefined);
6360
setShowResendSuccessBanner(false);
6461
}, []);
@@ -94,21 +91,34 @@ export const MfaGuard = ({
9491
throw new Error('Invalid state. Missing jwt cache.');
9592
}
9693

94+
const debounce = useCallback(
95+
(limitInMs: number) => {
96+
const lastRequest = MfaOtpRequestCache.get(sessionToken, requiredScope);
97+
return lastRequest != null && Date.now() - lastRequest < limitInMs;
98+
},
99+
[sessionToken, requiredScope]
100+
);
101+
97102
// Modal Setup
98103
useEffect(() => {
99104
(async () => {
100105
// To avoid requesting multiple OTPs on mount
101-
if (
102-
hasSentConfirmationCode.current ||
103-
JwtTokenCache.hasToken(sessionToken, requiredScope)
104-
) {
106+
if (JwtTokenCache.hasToken(sessionToken, requiredScope)) {
105107
return;
106108
}
109+
110+
// Avoid bombarding the user with emails just because they open
111+
// the dialog
112+
const limitInMs = config.mfa.otp.expiresInMinutes * 60 * 1000;
113+
if (debounce(limitInMs)) {
114+
return;
115+
}
116+
107117
try {
108-
hasSentConfirmationCode.current = true;
118+
MfaOtpRequestCache.set(sessionToken, requiredScope);
109119
await authClient.mfaRequestOtp(sessionToken, requiredScope);
110120
} catch (err) {
111-
hasSentConfirmationCode.current = false;
121+
MfaOtpRequestCache.remove(sessionToken, requiredScope);
112122
if (err.code === 401) {
113123
handleError(err);
114124
return;
@@ -127,6 +137,8 @@ export const MfaGuard = ({
127137
alertBar,
128138
ftlMsgResolver,
129139
onDismiss,
140+
config.mfa.otp.expiresInMinutes,
141+
debounce,
130142
]);
131143

132144
const onSubmitOtp = async (code: string) => {
@@ -151,12 +163,19 @@ export const MfaGuard = ({
151163
};
152164

153165
const handleResendCode = async () => {
166+
// Stop users from hammering the resend button...
167+
if (debounce(debounceIntervalMs)) {
168+
return;
169+
}
170+
154171
setResendCodeLoading(true);
155172
try {
173+
MfaOtpRequestCache.set(sessionToken, requiredScope);
156174
await authClient.mfaRequestOtp(sessionToken, requiredScope);
157175
setLocalizedErrorBannerMessage(undefined);
158176
setShowResendSuccessBanner(true);
159177
} catch (err) {
178+
MfaOtpRequestCache.remove(sessionToken, requiredScope);
160179
setShowResendSuccessBanner(false);
161180
if (err.code === 401) {
162181
handleError(err);

packages/fxa-settings/src/lib/cache.ts

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -404,3 +404,60 @@ export class JwtNotFoundError extends Error {
404404
super(message);
405405
}
406406
}
407+
408+
/**
409+
* Manages OTP request cache
410+
*
411+
* The main purpose of this cache is to track when we email users
412+
* MFA OTP codes. This info can then be used to debounce sends.
413+
*/
414+
export class MfaOtpRequestCache {
415+
/** Key where data is held in persistent storage */
416+
private static readonly storageKey = 'mfa_otp_requests';
417+
418+
/** Internal state, access is protected by getters / setters below. */
419+
private static _state?: Record<string, number>;
420+
421+
/** Gets the current state with backing in persistent storage */
422+
private static get state(): Record<string, number> {
423+
if (this._state != null) {
424+
return this._state;
425+
}
426+
427+
// Fallback to stored state, if stored state is invalid, then
428+
// assume fresh slate, and create new state object
429+
this._state = storage.get(this.storageKey);
430+
if (this._state == null) {
431+
this._state = {};
432+
}
433+
434+
return this._state;
435+
}
436+
437+
private static set state(val: Record<string, number>) {
438+
this._state = val;
439+
this.store();
440+
}
441+
442+
static getKey(sessionToken: string, scope: MfaScope) {
443+
return `${sessionToken}-${scope}`;
444+
}
445+
446+
private static store() {
447+
storage.set(this.storageKey, this._state);
448+
}
449+
450+
static set(sessionToken: string, requiredScope: MfaScope) {
451+
this.state[this.getKey(sessionToken, requiredScope)] = Date.now();
452+
this.store();
453+
}
454+
455+
static remove(sessionToken: string, requiredScope: MfaScope) {
456+
delete this.state[this.getKey(sessionToken, requiredScope)];
457+
this.store();
458+
}
459+
460+
static get(sessionToken: string, requiredScope: MfaScope) {
461+
return this.state[this.getKey(sessionToken, requiredScope)];
462+
}
463+
}

0 commit comments

Comments
 (0)