determined icon indicating copy to clipboard operation
determined copied to clipboard

fix: Check auth validity before setting isAuthenticated

Open julian-determined-ai opened this issue 11 months ago • 2 comments

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:

  1. Check if the user `isAuthenticated"
  2. 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

  1. Ensure that you are logged out of MLDE.
  2. Navigate to the login page.
  3. Using devtools set a value for the global/auth-token
  4. Add a ?redirect query to the login url such as login?redirect=/lore/
  5. Visit the URL in (4).
  6. 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

julian-determined-ai avatar Mar 06 '24 21:03 julian-determined-ai

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...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

netlify[bot] avatar Mar 06 '24 21:03 netlify[bot]

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:

... and 320 files with indirect coverage changes

codecov[bot] avatar Mar 06 '24 21:03 codecov[bot]