Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a redirect loop issue between the checkAuth and logout methods in the APISIX OIDC auth provider by simplifying the redirect mechanism and removing the isRedirecting guard flag.
Changes:
- Removed the
isRedirectingflag andsetTimeoutdelay mechanism from checkAuth and checkError methods - Changed logout to perform manual redirects via
window.location.hrefinstead of returning a redirect URL - Updated code formatting to use single quotes and 2-space indentation (via prettier configuration)
- Bumped package version from 1.2.0 to 1.2.1
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/ra-apisix-oidc/src/apisixOidcAuthProvider.ts | Refactored auth flow to fix redirect loop by removing isRedirecting flag, simplifying redirects, and reformatting code |
| packages/ra-apisix-oidc/package.json | Version bump to 1.2.1 and formatting updates |
| package-lock.json | Version update (inconsistent with package.json) |
| .prettierrc.js | Changed tab width from 4 to 2 spaces |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| logout: async () => { | ||
| const accessToken = storage.getItem("access_token"); | ||
| const accessToken = storage.getItem('access_token'); | ||
| if (!accessToken) { | ||
| return Promise.resolve(); | ||
| return Promise.reject(); | ||
| } | ||
| storage.removeItem("access_token"); | ||
| return Promise.resolve(logoutURL); | ||
| storage.removeItem('access_token'); | ||
| window.location.href = logoutURL; | ||
| return Promise.reject(); | ||
| }, |
There was a problem hiding this comment.
The logout method has changed its behavior: (1) It now rejects when no access token exists (previously resolved), and (2) It manually redirects via window.location.href and then rejects (previously returned the logout URL as a resolved value for react-admin to handle). While these changes may be part of fixing the reported loop issue, consider the implications: rejecting when already logged out may cause unnecessary error handling, and manual redirects with rejection prevent react-admin from cleaning up state. Verify through testing that this approach works correctly with react-admin's logout flow and doesn't cause unexpected error messages or UI states.
| checkError: error => { | ||
| if (error.status === 401) { | ||
| storage.removeItem("access_token"); | ||
| if (!isRedirecting) { | ||
| isRedirecting = true; | ||
| saveCurrentLocation(storage); | ||
| setTimeout(() => { | ||
| window.location.href = loginURL; | ||
| }, 100); | ||
| } | ||
| return Promise.reject({ logoutUser: false }); | ||
| storage.removeItem('access_token'); | ||
| saveCurrentLocation(storage); | ||
| window.location.href = loginURL; | ||
| return Promise.reject(); | ||
| } | ||
| return Promise.resolve(); | ||
| }, | ||
| checkAuth: async () => { | ||
| const accessToken = storage.getItem("access_token"); | ||
| const accessToken = storage.getItem('access_token'); | ||
| if (!accessToken) { | ||
| if (!isRedirecting) { | ||
| isRedirecting = true; | ||
| saveCurrentLocation(storage); | ||
| setTimeout(() => { | ||
| window.location.href = loginURL; | ||
| }, 100); | ||
| } | ||
| return Promise.reject({ redirectTo: false }); | ||
| saveCurrentLocation(storage); | ||
| window.location.href = loginURL; | ||
| return Promise.reject(); |
There was a problem hiding this comment.
Both checkAuth and checkError now perform synchronous redirects via window.location.href and return Promise.reject() without parameters. The previous implementation used special rejection parameters like { redirectTo: false } and { logoutUser: false } to control react-admin's redirect behavior, and included an isRedirecting flag to prevent concurrent redirects. While removing these mechanisms may fix the reported loop issue, verify through testing that: (1) react-admin doesn't attempt its own redirects after the manual window.location.href assignment, (2) multiple simultaneous failures don't cause multiple redirect attempts, and (3) the user experience is smooth without the 100ms setTimeout delay that was previously used.
| semi: true, | ||
| singleQuote: true, | ||
| tabWidth: 4, | ||
| tabWidth: 2, |
There was a problem hiding this comment.
Changing the tabWidth from 4 to 2 is a project-wide formatting change that will affect all files when they are reformatted. This appears to be applied to the apisixOidcAuthProvider.ts file in this PR, but it's a broader configuration change. Consider whether this formatting change should be applied consistently across the entire codebase in a separate PR to avoid mixing formatting changes with functional changes, or ensure all files have been reformatted consistently.
| tabWidth: 2, | |
| tabWidth: 4, |
2 participants
The lib was looping between checkAuth and logout.