determined
determined copied to clipboard
fix: Check auth validity before setting isAuthenticated
Description
There is bug in GenAI currently where a user gets stuck in an infinite redirect loop, this fixes the issue.
You can see the bug in action below: https://github.com/determined-ai/determined/assets/103522725/7375fff4-bdc6-4c78-a6f0-5717080cf21f
The ultimate cause of the bug is that if you look at the logic in the sign in page we:
- Check if the user `isAuthenticated"
- Check if there is a redirect, if there is we redirect the user.
However the issue is that in (1) the isAuthenticated
can be set to true without the user actually being a valid and authenticated user. This causes an infinite login bug in GenAI since we use the MLDE
login page for authentication also.
In order to preserve all current log in and auth related behavior, this PR introduces an extra check to ensure the user is authenticated if an auth-token
is present. If not, then we handle the error using out handleError
function. This set up ensures that unauthenticated users will not be improperly redirected.
Test Plan
- Ensure that you are logged out of MLDE.
- Navigate to the login page.
- Using devtools set a value for the
global/auth-token
- Add a
?redirect
query to the login url such aslogin?redirect=/lore/
- Visit the URL in (4).
- Ensure that you are not redirected, and you instead immediately shown a "loading" message and then the login page again.
Commentary (optional)
Checklist
- [x] Changes have been manually QA'd
- [x] User-facing API changes need the "User-facing API Change" label.
- [x] Release notes should be added as a separate file under
docs/release-notes/
. See Release Note for details. - [x] Licenses should be included for new code which was copied and/or modified from any external code.
Ticket
Deploy Preview for determined-ui ready!
Name | Link |
---|---|
Latest commit | a4090404e04a4ffb6d5a052503025187198ee741 |
Latest deploy log | https://app.netlify.com/sites/determined-ui/deploys/65e8ee5fc4e216000869fc9f |
Deploy Preview | https://deploy-preview-8967--determined-ui.netlify.app |
Preview on mobile | Toggle QR Code...Use your smartphone camera to open QR code link. |
To edit notification comments on pull requests, go to your Netlify site configuration.
Codecov Report
Attention: Patch coverage is 16.66667%
with 10 lines
in your changes are missing coverage. Please review.
Project coverage is 42.60%. Comparing base (
191a144
) to head (a409040
). Report is 5 commits behind head on main.
Additional details and impacted files
@@ Coverage Diff @@
## main #8967 +/- ##
==========================================
- Coverage 47.37% 42.60% -4.77%
==========================================
Files 1162 842 -320
Lines 176134 136791 -39343
Branches 2237 2236 -1
==========================================
- Hits 83448 58286 -25162
+ Misses 92528 78347 -14181
Partials 158 158
Flag | Coverage Δ | |
---|---|---|
harness | ? |
|
web | 42.54% <16.66%> (-0.01%) |
:arrow_down: |
Flags with carried forward coverage won't be shown. Click here to find out more.
Files | Coverage Δ | |
---|---|---|
webui/react/src/hooks/useAuthCheck.ts | 37.34% <16.66%> (-1.12%) |
:arrow_down: |