Skip to content

Commit 23fa7c2

Browse files
smarcetsmarcet
committed
fix(refresh-token): fix retry exhaustion bug, dropzone accept callback, and response.ok handling
* Fix retryWithBackoff exhaustion guard (attempt === maxRetries was dead code after loop bound change to < maxRetries; errors were silently swallowed, function returned undefined instead of throwing). * Fix Dropzone accept callback to call done() on error path, preventing files from getting stuck in an indeterminate state. * Use response.ok instead of checking individual status codes, closing a gap where 4xx codes other than 400 fell through unhandled. * Add ok property to existing test mocks to match the new response.ok check. * Export retryWithBackoff and add 5 unit tests covering retry count, auth error short-circuit, transient recovery, and exponential backoff. * Add defensive check for missing access_token in refresh response.
1 parent 2c4c42f commit 23fa7c2

3 files changed

Lines changed: 110 additions & 25 deletions

File tree

‎src/components/inputs/upload-input-v2/dropzone.js‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ export class DropzoneJS extends React.Component {
6666
if (!e.message || !e.message.startsWith(AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR)) {
6767
initLogOut();
6868
}
69+
done(e.message || 'Auth error');
6970
return;
7071
}
7172
if (options.maxFiles && options.maxFiles < (this.state.files.length + this.props.uploadCount)) {

‎src/components/security/__tests__/methods.test.js‎

Lines changed: 84 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import {
33
AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR,
44
} from '../constants';
55

6-
import { refreshAccessToken } from '../methods';
6+
import { refreshAccessToken, retryWithBackoff } from '../methods';
77

88
// Mock utils/methods imports used by security/methods
99
jest.mock('../../../utils/methods', () => ({
@@ -79,6 +79,7 @@ describe('refreshAccessToken', () => {
7979

8080
it('should return tokens on successful response', async () => {
8181
const mockResponse = {
82+
ok: true,
8283
status: 200,
8384
json: jest.fn().mockResolvedValue({
8485
access_token: 'new-access-token',
@@ -102,6 +103,7 @@ describe('refreshAccessToken', () => {
102103

103104
it('should throw AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR on HTTP 400', async () => {
104105
const mockResponse = {
106+
ok: false,
105107
status: 400,
106108
statusText: 'Bad Request',
107109
};
@@ -116,6 +118,7 @@ describe('refreshAccessToken', () => {
116118

117119
it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 500', async () => {
118120
const mockResponse = {
121+
ok: false,
119122
status: 500,
120123
statusText: 'Internal Server Error',
121124
};
@@ -130,6 +133,7 @@ describe('refreshAccessToken', () => {
130133

131134
it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 502', async () => {
132135
const mockResponse = {
136+
ok: false,
133137
status: 502,
134138
statusText: 'Bad Gateway',
135139
};
@@ -142,6 +146,7 @@ describe('refreshAccessToken', () => {
142146

143147
it('should throw AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR on HTTP 503', async () => {
144148
const mockResponse = {
149+
ok: false,
145150
status: 503,
146151
statusText: 'Service Unavailable',
147152
};
@@ -184,6 +189,7 @@ describe('refreshAccessToken', () => {
184189

185190
it('should call setSessionClearingState(true) only on HTTP 400', async () => {
186191
const mockResponse = {
192+
ok: false,
187193
status: 400,
188194
statusText: 'Bad Request',
189195
};
@@ -201,6 +207,7 @@ describe('refreshAccessToken', () => {
201207

202208
it('should include status in error message for 5xx', async () => {
203209
const mockResponse = {
210+
ok: false,
204211
status: 503,
205212
statusText: 'Service Unavailable',
206213
};
@@ -219,3 +226,79 @@ describe('refreshAccessToken', () => {
219226
.toThrow(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: Failed to fetch`);
220227
});
221228
});
229+
230+
describe('retryWithBackoff', () => {
231+
232+
it('should return on first success without retrying', async () => {
233+
jest.useRealTimers();
234+
const fn = jest.fn().mockResolvedValue('ok');
235+
236+
const result = await retryWithBackoff(fn, 3, 1);
237+
238+
expect(result).toBe('ok');
239+
expect(fn).toHaveBeenCalledTimes(1);
240+
});
241+
242+
it('should retry network errors up to maxRetries then throw', async () => {
243+
jest.useRealTimers();
244+
const networkError = new Error('network down');
245+
const fn = jest.fn().mockRejectedValue(networkError);
246+
247+
await expect(retryWithBackoff(fn, 3, 1))
248+
.rejects
249+
.toThrow('network down');
250+
251+
expect(fn).toHaveBeenCalledTimes(3);
252+
});
253+
254+
it('should succeed after transient failures', async () => {
255+
jest.useRealTimers();
256+
const fn = jest.fn()
257+
.mockRejectedValueOnce(new Error('transient'))
258+
.mockRejectedValueOnce(new Error('transient'))
259+
.mockResolvedValue('recovered');
260+
261+
const result = await retryWithBackoff(fn, 5, 1);
262+
263+
expect(result).toBe('recovered');
264+
expect(fn).toHaveBeenCalledTimes(3);
265+
});
266+
267+
it('should not retry auth errors (HTTP 400)', async () => {
268+
jest.useRealTimers();
269+
const authError = new Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: 400 - Bad Request`);
270+
const fn = jest.fn().mockRejectedValue(authError);
271+
272+
await expect(retryWithBackoff(fn, 5, 1))
273+
.rejects
274+
.toThrow(AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR);
275+
276+
expect(fn).toHaveBeenCalledTimes(1);
277+
});
278+
279+
it('should apply exponential backoff delays', async () => {
280+
jest.useRealTimers();
281+
const setTimeoutSpy = jest.spyOn(global, 'setTimeout');
282+
283+
const fn = jest.fn()
284+
.mockRejectedValueOnce(new Error('fail 1'))
285+
.mockRejectedValueOnce(new Error('fail 2'))
286+
.mockResolvedValue('ok');
287+
288+
const baseDelay = 100;
289+
const result = await retryWithBackoff(fn, 5, baseDelay);
290+
291+
expect(result).toBe('ok');
292+
expect(fn).toHaveBeenCalledTimes(3);
293+
294+
// Extract the delay arguments from setTimeout calls made by retryWithBackoff
295+
const retryDelays = setTimeoutSpy.mock.calls
296+
.map(call => call[1])
297+
.filter(delay => delay >= baseDelay);
298+
299+
// 100 * 2^0 = 100ms, 100 * 2^1 = 200ms
300+
expect(retryDelays).toEqual([100, 200]);
301+
302+
setTimeoutSpy.mockRestore();
303+
});
304+
});

‎src/components/security/methods.js‎

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import moment from "moment-timezone";
1414
import request from 'superagent/lib/client';
1515
import SuperTokensLock from 'browser-tabs-lock';
1616
import Cookies from 'js-cookie'
17+
1718
let http = request;
1819

1920
/**
@@ -143,7 +144,7 @@ export const getAuthUrl = (
143144
* @param idToken
144145
* @returns {*}
145146
*/
146-
export const getLogoutUrl = (idToken= null) => {
147+
export const getLogoutUrl = (idToken = null) => {
147148
let baseUrl = getOAuth2IDPBaseUrl();
148149
let oauth2ClientId = getOAuth2ClientId();
149150
let url = URI(`${baseUrl}/oauth2/end-session`);
@@ -162,7 +163,7 @@ export const getLogoutUrl = (idToken= null) => {
162163
"state": state,
163164
}
164165

165-
if(idToken)
166+
if (idToken)
166167
queryParams.id_token_hint = idToken;
167168

168169
return url.query(queryParams);
@@ -228,7 +229,7 @@ export const emitAccessToken = async (code, backUrl = null) => {
228229
let redirectUri = getAuthCallback();
229230
let pkce = JSON.parse(getFromLocalStorage(PKCE, true));
230231

231-
if(!pkce)
232+
if (!pkce)
232233
throw Error(AUTH_ERROR_MISSING_PKCE_PARAM);
233234

234235
if (backUrl != null)
@@ -266,7 +267,7 @@ export const emitAccessToken = async (code, backUrl = null) => {
266267
export const MAX_RETRIES = 5;
267268
export const BACKOFF_BASE_MS = 1000;
268269

269-
const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => {
270+
export const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACKOFF_BASE_MS) => {
270271
for (let attempt = 0; attempt <= maxRetries; attempt++) {
271272
try {
272273
return await fn();
@@ -276,7 +277,7 @@ const retryWithBackoff = async (fn, maxRetries = MAX_RETRIES, baseDelayMs = BACK
276277
throw err;
277278
}
278279
// network/5xx errors are retryable — retry unless exhausted
279-
if (attempt === maxRetries) {
280+
if (attempt === maxRetries - 1) {
280281
throw err;
281282
}
282283
const delay = baseDelayMs * Math.pow(2, attempt);
@@ -338,13 +339,12 @@ const _getAccessToken = async () => {
338339
* @returns {Promise<*|undefined>}
339340
*/
340341
export const getAccessToken = async () => {
341-
if(navigator?.locks){
342+
if (navigator?.locks) {
342343
return await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => {
343344
console.log(`openstack-uicore-foundation::Security::methods::getAccessToken web lock api`, lock);
344345
return await _getAccessToken();
345346
});
346-
}
347-
else {
347+
} else {
348348
if (
349349
await retryPromise(
350350
() => Lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, GET_TOKEN_SILENTLY_LOCK_KEY_TIMEOUT),
@@ -383,13 +383,12 @@ const _clearAccessToken = () => {
383383

384384
export const clearAccessToken = async () => {
385385
// see https://developer.mozilla.org/en-US/docs/Web/API/Web_Locks_API
386-
if(navigator?.locks){
386+
if (navigator?.locks) {
387387
await navigator.locks.request(GET_TOKEN_SILENTLY_LOCK_KEY, async lock => {
388388
console.log(`openstack-uicore-foundation::Security::methods::clearAccessToken web lock api`, lock);
389389
_clearAccessToken();
390390
});
391-
}
392-
else {
391+
} else {
393392
if (
394393
await retryPromise(
395394
() => Lock.acquireLock(GET_TOKEN_SILENTLY_LOCK_KEY, GET_TOKEN_SILENTLY_LOCK_KEY_TIMEOUT),
@@ -401,8 +400,7 @@ export const clearAccessToken = async () => {
401400
} finally {
402401
await Lock.releaseLock(GET_TOKEN_SILENTLY_LOCK_KEY);
403402
}
404-
}
405-
else{
403+
} else {
406404
// error on locking
407405
throw Error(AUTH_ERROR_LOCK_ACQUIRE_ERROR);
408406
}
@@ -437,20 +435,24 @@ export const refreshAccessToken = async (refresh_token) => {
437435
throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${networkError.message}`);
438436
}
439437

440-
if (response.status === 400) {
438+
if (!response.ok) {
439+
console.log(`refreshAccessToken server error: ${response.status} - ${response.statusText}`);
440+
if (response.status >= 500) {
441+
// server error — transient, should be retried
442+
throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${response.status} - ${response.statusText}`);
443+
}
441444
// token is genuinely revoked — this is a real auth error
442445
setSessionClearingState(true);
443446
throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: ${response.status} - ${response.statusText}`);
444447
}
445448

446-
if (response.status >= 500) {
447-
// server error — transient, should be retried
448-
console.log(`refreshAccessToken server error: ${response.status} - ${response.statusText}`);
449-
throw Error(`${AUTH_ERROR_REFRESH_TOKEN_NETWORK_ERROR}: ${response.status} - ${response.statusText}`);
450-
}
451-
452449
const json = await response.json();
453450
let {access_token, refresh_token: new_refresh_token, expires_in, id_token} = json;
451+
// Defensively ensure we never propagate an undefined access token.
452+
if (!access_token) {
453+
setSessionClearingState(true);
454+
throw Error(`${AUTH_ERROR_REFRESH_TOKEN_REQUEST_ERROR}: missing access_token in refresh response`);
455+
}
454456
return {access_token, refresh_token: new_refresh_token, expires_in, id_token}
455457
}
456458

@@ -478,9 +480,8 @@ export const storeAuthInfo = (accessToken, expiresIn, refreshToken = null, idTok
478480

479481
if (idToken) {
480482
authInfo[ID_TOKEN] = idToken;
481-
Cookies.set(ID_TOKEN, idToken, { secure: true, sameSite: 'Lax' });
482-
}
483-
else{
483+
Cookies.set(ID_TOKEN, idToken, {secure: true, sameSite: 'Lax'});
484+
} else {
484485
Cookies.remove(ID_TOKEN);
485486
}
486487

@@ -563,7 +564,7 @@ export const validateIdToken = (idToken, issuer, audience) => {
563564
});
564565

565566
let storedNonce = getFromLocalStorage(NONCE, true);
566-
if(!storedNonce)
567+
if (!storedNonce)
567568
throw Error(AUTH_ERROR_MISSING_NONCE_PARAM);
568569

569570
let jwt = verifier.decode(idToken);

0 commit comments

Comments
 (0)