feat(auth): add OIDC sign-out#44
Conversation
Add end_session_endpoint metadata and E2E coverage for sign-out via the mock OIDC server.
Test & Lint Summary
|
Summary
Test & Lint Summary
|
Summary
Test & Lint Summary
|
Restrict OIDC seed injection to the app origin and redirect end-session to the post-logout URI.
Summary
Test & Lint Summary
|
|
Requesting review — this PR adds |
noa-lucent
left a comment
noa-lucent
left a comment
There was a problem hiding this comment.
The core fix (end_session_endpoint in user-manager.ts) is correct and matches the issue specification exactly. The sign-out e2e tests cover both UI paths (user menu + settings page) and properly verify session cleanup. The sign-in-helper stabilization (origin guard + idempotency sentinel) is a sound improvement that prevents re-seeding after the sign-out redirect chain.
Left a few minor/nit comments:
- Extract the duplicated
waitForFunctionsession-check intooidc-helpers.ts - Remove deprecated
noWaitAfteroption - Make mock-server's JSON parse failure loud instead of silent
- Remove redundant null check in
UpdateUserhandler
None are blocking. Nice clean PR.
| const key = window.sessionStorage.key(i); | ||
| if (key && key.startsWith('oidc.user:')) return false; | ||
| } | ||
| return true; |
There was a problem hiding this comment.
[minor] The waitForFunction that scans sessionStorage for oidc.user: keys is duplicated in both tests. Extract it into a helper in oidc-helpers.ts (e.g. waitForOidcSessionCleared(page, timeout)) alongside the existing readOidcSession. This keeps the session-assertion logic in one place and avoids drift if the storage key prefix ever changes.
|
|
||
| await expect(page.getByTestId('user-menu-trigger')).toBeVisible({ timeout: 15000 }); | ||
| await page.getByTestId('user-menu-trigger').click(); | ||
| await page.getByTestId('user-menu-signout').click({ noWaitAfter: true }); |
There was a problem hiding this comment.
[nit] noWaitAfter has been deprecated since Playwright 1.50 and is a no-op in ^1.59.1 (this project's version). Remove it to avoid misleading readers into thinking it has an effect.
Same applies to line 28.
| if (!raw) return {}; | ||
| const contentType = req.headers['content-type'] ?? ''; | ||
| if (contentType.includes('application/json')) { | ||
| try { |
There was a problem hiding this comment.
[minor] Silent error suppression — if JSON.parse fails, the function returns {} and the mock happily processes a request with an empty body. For a test server this masks malformed payloads from test code, making failures harder to diagnose. Propagate the parse error (or return a 400) instead of silently falling back.
try {
return JSON.parse(raw);
} catch (err) {
sendText(res, 400, `Invalid JSON body: ${err.message}`);
// or just let it throw — the mock should be loud
}(This requires passing res in or restructuring slightly, but the principle stands: test infrastructure should fail loudly.)
| case 'GetMe': { | ||
| return sendJson(res, 200, { user: mapUser(defaultUser), clusterRole: defaultUser.clusterRole }); | ||
| } | ||
| case 'ListUsers': { |
There was a problem hiding this comment.
[nit] The second if (!user) guard (after users.has(identityId) already confirmed the key exists) is redundant. You can simplify:
case 'UpdateUser': {
const user = users.get(body.identityId);
if (!user) return sendText(res, 404, 'User not found');
// ...update fields...
}This removes the double-check without changing behavior.
3 participants
Summary
Testing
Fixes #41