Skip to content

Commit 25c17cf

Browse files
authored
Merge pull request AppFlowy-IO#154 from AppFlowy-IO/fix_flaky_login
fix: Fixes three critical race conditions that caused redirect loops …
2 parents be6ae46 + 7e1d3c8 commit 25c17cf

File tree

8 files changed

+448
-38
lines changed

8 files changed

+448
-38
lines changed

.github/workflows/integration-test.yml

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -2,17 +2,42 @@ name: E2E Tests
22

33
on:
44
push:
5-
branches: [main, develop]
5+
branches: [main]
66
pull_request:
7-
branches: [main, develop]
7+
branches: [main]
88
workflow_dispatch:
99

1010
env:
1111
CLOUD_VERSION: latest-amd64
1212

1313
jobs:
14-
run_e2e_tests:
14+
# Run different test categories in parallel for 6x speedup
15+
test:
1516
runs-on: ubuntu-latest
17+
strategy:
18+
fail-fast: false # Don't cancel other jobs if one fails
19+
matrix:
20+
# Group tests by directory/feature for parallel execution
21+
test-group:
22+
- name: "auth"
23+
spec: "cypress/e2e/auth/**/*.cy.ts"
24+
description: "Authentication (OAuth, OTP, Password, Login/Logout)"
25+
- name: "editor"
26+
spec: "cypress/e2e/editor/**/*.cy.ts"
27+
description: "Document editing and formatting"
28+
- name: "database"
29+
spec: "cypress/e2e/database/**/*.cy.ts"
30+
description: "Database and grid operations"
31+
- name: "page"
32+
spec: "cypress/e2e/page/**/*.cy.ts"
33+
description: "Page management (create, delete, share, publish)"
34+
- name: "chat"
35+
spec: "cypress/e2e/chat/**/*.cy.ts"
36+
description: "AI chat features"
37+
- name: "account-space-user"
38+
spec: "cypress/e2e/{account,space,user,app}/**/*.cy.ts"
39+
description: "Account, Space, User, and App tests"
40+
name: "${{ matrix.test-group.name }}"
1641
steps:
1742
- name: Checkout code
1843
uses: actions/checkout@v4
@@ -113,8 +138,9 @@ jobs:
113138
exit 1
114139
)
115140
116-
# Run HTTP API integration tests
141+
# Run HTTP API integration tests (only once, in auth group)
117142
- name: Run HTTP API integration tests
143+
if: matrix.test-group.name == 'auth'
118144
run: |
119145
echo "Running HTTP API integration tests..."
120146
pnpm run test:http-api
@@ -144,17 +170,17 @@ jobs:
144170
sleep 2
145171
done' && echo "✓ Preview server is ready" || (echo "❌ Preview server failed to start" && exit 1)
146172
147-
- name: Run tests
173+
- name: Run ${{ matrix.test-group.name }} tests
148174
run: |
149175
pnpm cypress run \
150-
--spec 'cypress/e2e/**/*.cy.ts' \
176+
--spec '${{ matrix.test-group.spec }}' \
151177
--config video=false
152178
153-
- name: Upload artifacts
154-
if: always()
179+
- name: Upload artifacts (${{ matrix.test-group.name }})
180+
if: failure()
155181
uses: actions/upload-artifact@v4
156182
with:
157-
name: test-results
183+
name: test-results-${{ matrix.test-group.name }}
158184
path: |
159185
cypress/screenshots/**
160186
cypress/logs/**

cypress/e2e/auth/oauth-login.cy.ts

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -485,5 +485,192 @@ describe('OAuth Login Flow', () => {
485485
cy.log('[STEP 5] Redirect loop prevention test passed - token persisted and no redirect occurred');
486486
});
487487
});
488+
489+
describe('Old Token Race Condition', () => {
490+
it('should clear old expired token before processing OAuth callback', () => {
491+
const oldExpiredToken = 'old-expired-token-' + uuidv4();
492+
const oldRefreshToken = 'old-expired-refresh-' + uuidv4();
493+
const newAccessToken = 'new-oauth-token-' + uuidv4();
494+
const newRefreshToken = 'new-oauth-refresh-' + uuidv4();
495+
const mockUserId = uuidv4();
496+
const mockWorkspaceId = uuidv4();
497+
498+
cy.log('[TEST START] Testing old expired token race condition');
499+
500+
cy.log('[SETUP] Pre-populate localStorage with expired token');
501+
cy.window().then((win) => {
502+
// Set old expired token (expired 1 hour ago)
503+
win.localStorage.setItem('token', JSON.stringify({
504+
access_token: oldExpiredToken,
505+
refresh_token: oldRefreshToken,
506+
expires_at: Math.floor(Date.now() / 1000) - 3600, // Expired!
507+
user: {
508+
id: mockUserId,
509+
email: 'old@example.com',
510+
},
511+
}));
512+
});
513+
514+
// Mock refresh endpoint to FAIL for old token, SUCCESS for new token
515+
cy.intercept('POST', `${gotrueUrl}/token?grant_type=refresh_token`, (req) => {
516+
const { body } = req;
517+
518+
if (body.refresh_token === oldRefreshToken) {
519+
// Old token refresh should fail
520+
req.reply({
521+
statusCode: 400,
522+
body: {
523+
error: 'invalid_grant',
524+
error_description: 'Refresh token is invalid or expired',
525+
},
526+
});
527+
} else if (body.refresh_token === newRefreshToken) {
528+
// New token refresh should succeed
529+
req.reply({
530+
statusCode: 200,
531+
body: {
532+
access_token: newAccessToken,
533+
refresh_token: newRefreshToken,
534+
expires_at: Math.floor(Date.now() / 1000) + 3600,
535+
user: {
536+
id: mockUserId,
537+
email: 'test@example.com',
538+
email_confirmed_at: new Date().toISOString(),
539+
created_at: new Date().toISOString(),
540+
updated_at: new Date().toISOString(),
541+
},
542+
},
543+
});
544+
} else {
545+
// Unknown token
546+
req.reply({ statusCode: 400, body: { error: 'unknown_token' } });
547+
}
548+
}).as('refreshToken');
549+
550+
// Mock verify for NEW token only
551+
cy.intercept('GET', `${apiUrl}/api/user/verify/${newAccessToken}`, {
552+
statusCode: 200,
553+
body: {
554+
code: 0,
555+
data: { is_new: false },
556+
message: 'Success',
557+
},
558+
}).as('verifyNewToken');
559+
560+
// Mock verify for OLD token (should NOT be called if fix is working)
561+
cy.intercept('GET', `${apiUrl}/api/user/verify/${oldExpiredToken}`, {
562+
statusCode: 401,
563+
body: {
564+
code: 401,
565+
message: 'Token expired',
566+
},
567+
}).as('verifyOldToken');
568+
569+
// Mock workspace endpoints
570+
cy.intercept('GET', `${apiUrl}/api/user/workspace`, {
571+
statusCode: 200,
572+
body: {
573+
code: 0,
574+
data: {
575+
user_profile: { uuid: mockUserId },
576+
visiting_workspace: {
577+
workspace_id: mockWorkspaceId,
578+
workspace_name: 'My Workspace',
579+
icon: '',
580+
created_at: Date.now().toString(),
581+
database_storage_id: '',
582+
owner_uid: 1,
583+
owner_name: 'Test User',
584+
member_count: 1,
585+
},
586+
workspaces: [
587+
{
588+
workspace_id: mockWorkspaceId,
589+
workspace_name: 'My Workspace',
590+
icon: '',
591+
created_at: Date.now().toString(),
592+
database_storage_id: '',
593+
owner_uid: 1,
594+
owner_name: 'Test User',
595+
member_count: 1,
596+
},
597+
],
598+
},
599+
message: 'Success',
600+
},
601+
}).as('getUserWorkspaceInfo');
602+
603+
cy.intercept('GET', `${apiUrl}/api/user/profile*`, {
604+
statusCode: 200,
605+
body: {
606+
code: 0,
607+
data: {
608+
uid: 1,
609+
uuid: mockUserId,
610+
email: 'test@example.com',
611+
name: 'Test User',
612+
metadata: {},
613+
encryption_sign: null,
614+
latest_workspace_id: mockWorkspaceId,
615+
updated_at: Date.now(),
616+
},
617+
message: 'Success',
618+
},
619+
}).as('getCurrentUser');
620+
621+
// Step 1: Simulate OAuth callback with NEW tokens
622+
cy.log('[STEP 1] Simulating OAuth callback with NEW tokens (old expired token in localStorage)');
623+
const callbackUrl = `${baseUrl}/auth/callback#access_token=${newAccessToken}&refresh_token=${newRefreshToken}&expires_at=${Math.floor(Date.now() / 1000) + 3600
624+
}&token_type=bearer`;
625+
626+
cy.visit(callbackUrl, { failOnStatusCode: false });
627+
cy.wait(2000);
628+
629+
// Step 2: Verify NEW token was used for verification (not old token)
630+
cy.log('[STEP 2] Verifying NEW token is used for verification');
631+
cy.wait('@verifyNewToken').then((interception) => {
632+
expect(interception.response?.statusCode).to.equal(200);
633+
cy.log('[SUCCESS] verifyToken called with NEW token (old token was cleared first)');
634+
});
635+
636+
// Step 3: Verify refresh was called with NEW token (not old expired token)
637+
cy.log('[STEP 3] Verifying refresh called with NEW token');
638+
cy.wait('@refreshToken').then((interception) => {
639+
const requestBody = interception.request.body;
640+
641+
expect(requestBody.refresh_token).to.equal(newRefreshToken);
642+
cy.log('[SUCCESS] refreshToken called with NEW token (not old expired token)');
643+
});
644+
645+
// Step 4: Verify we're redirected to /app (not /login due to token invalidation)
646+
cy.log('[STEP 4] Verifying successful redirect to /app');
647+
cy.url({ timeout: 15000 }).should('include', '/app');
648+
cy.url().should('not.include', '/login');
649+
650+
// Step 5: Verify NEW token is saved (old token replaced)
651+
cy.log('[STEP 5] Verifying NEW token is saved in localStorage');
652+
cy.window().then((win) => {
653+
const token = win.localStorage.getItem('token');
654+
655+
expect(token).to.exist;
656+
const tokenData = JSON.parse(token || '{}');
657+
658+
expect(tokenData.access_token).to.equal(newAccessToken);
659+
expect(tokenData.refresh_token).to.equal(newRefreshToken);
660+
// Old token should be completely replaced
661+
expect(tokenData.access_token).to.not.equal(oldExpiredToken);
662+
expect(tokenData.refresh_token).to.not.equal(oldRefreshToken);
663+
cy.log('[SUCCESS] NEW token saved, old token replaced');
664+
});
665+
666+
// Step 6: Wait to ensure no redirect loop occurs
667+
cy.log('[STEP 6] Verifying no redirect loop (session not invalidated)');
668+
cy.wait(3000);
669+
cy.url().should('include', '/app');
670+
cy.url().should('not.include', '/login');
671+
672+
cy.log('[TEST COMPLETE] Old token race condition handled correctly - old token cleared before OAuth processing');
673+
});
674+
});
488675
});
489676

src/application/services/js-services/http/http_api.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -223,6 +223,9 @@ export function initAPIService(config: AFCloudConfig) {
223223
const token = getTokenParsed();
224224

225225
if (!token) {
226+
console.debug('[initAPIService][request] no token found, sending request without auth header', {
227+
url: config.url,
228+
});
226229
return config;
227230
}
228231

@@ -237,6 +240,10 @@ export function initAPIService(config: AFCloudConfig) {
237240

238241
access_token = newToken?.access_token || '';
239242
} catch (e) {
243+
console.warn('[initAPIService][request] refresh token failed, marking token invalid', {
244+
url: config.url,
245+
message: (e as Error)?.message,
246+
});
240247
invalidToken();
241248
return config;
242249
}
@@ -262,6 +269,7 @@ export function initAPIService(config: AFCloudConfig) {
262269
const token = getTokenParsed();
263270

264271
if (!token) {
272+
console.warn('[initAPIService][response] 401 without token, emitting invalid token');
265273
invalidToken();
266274
return response;
267275
}
@@ -271,6 +279,10 @@ export function initAPIService(config: AFCloudConfig) {
271279
try {
272280
await refreshToken(refresh_token);
273281
} catch (e) {
282+
console.warn('[initAPIService][response] refresh on 401 failed, emitting invalid token', {
283+
message: (e as Error)?.message,
284+
url: response.config?.url,
285+
});
274286
invalidToken();
275287
}
276288
}
@@ -286,6 +298,10 @@ export async function signInWithUrl(url: string) {
286298
const gotrueError = parseGoTrueErrorFromUrl(url);
287299

288300
if (gotrueError) {
301+
console.warn('[signInWithUrl] GoTrue error detected in callback URL', {
302+
code: gotrueError.code,
303+
message: gotrueError.message,
304+
});
289305
// GoTrue returned an error, reject with parsed error
290306
return Promise.reject({
291307
code: gotrueError.code,
@@ -298,6 +314,7 @@ export async function signInWithUrl(url: string) {
298314
const hash = urlObj.hash;
299315

300316
if (!hash) {
317+
console.warn('[signInWithUrl] No hash found in callback URL');
301318
return Promise.reject('No hash found');
302319
}
303320

@@ -306,15 +323,35 @@ export async function signInWithUrl(url: string) {
306323
const refresh_token = params.get('refresh_token');
307324

308325
if (!accessToken || !refresh_token) {
326+
console.warn('[signInWithUrl] Missing tokens in callback hash', {
327+
hasAccessToken: !!accessToken,
328+
hasRefreshToken: !!refresh_token,
329+
});
309330
return Promise.reject({
310331
code: -1,
311332
message: 'No access token or refresh token found',
312333
});
313334
}
314335

336+
// CRITICAL: Clear old token BEFORE processing new OAuth tokens
337+
// This prevents axios interceptor from trying to auto-refresh the old expired token
338+
// during verifyToken() API call, which would cause a race condition where:
339+
// 1. verifyToken() makes API call with NEW token in URL
340+
// 2. Axios interceptor sees OLD token in localStorage, tries to refresh it
341+
// 3. Old token refresh fails → invalidToken() called → session invalidated
342+
// 4. Meanwhile, OAuth flow is trying to save NEW token → conflicts with invalidation
343+
// By clearing the old token first, we ensure axios interceptor skips auto-refresh
344+
const hadOldToken = !!localStorage.getItem('token');
345+
346+
if (hadOldToken) {
347+
console.debug('[signInWithUrl] Clearing old token before processing OAuth callback to prevent race condition');
348+
localStorage.removeItem('token');
349+
}
350+
315351
try {
316352
await verifyToken(accessToken);
317353
} catch (e) {
354+
console.warn('[signInWithUrl] Verify token failed', { message: (e as Error)?.message });
318355
return Promise.reject({
319356
code: -1,
320357
message: 'Verify token failed',
@@ -324,6 +361,7 @@ export async function signInWithUrl(url: string) {
324361
try {
325362
await refreshToken(refresh_token);
326363
} catch (e) {
364+
console.warn('[signInWithUrl] Refresh token failed', { message: (e as Error)?.message });
327365
return Promise.reject({
328366
code: -1,
329367
message: 'Refresh token failed',

src/application/services/js-services/index.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ import {
3434
UploadTemplatePayload,
3535
} from '@/application/template.type';
3636
import {
37+
AccessLevel,
3738
CreateFolderViewPayload,
3839
CreatePagePayload,
3940
CreateSpacePayload,
@@ -52,10 +53,9 @@ import {
5253
UpdateSpacePayload,
5354
UpdateWorkspacePayload,
5455
UploadPublishNamespacePayload,
55-
WorkspaceMember,
56-
YjsEditorKey,
5756
ViewIconType,
58-
AccessLevel
57+
WorkspaceMember,
58+
YjsEditorKey
5959
} from '@/application/types';
6060
import { applyYDoc } from '@/application/ydoc/apply';
6161

0 commit comments

Comments
 (0)