Repository navigation
Conversation
|
Friendly ping on this PR — it's been open for three weeks now with no activity, so I wanted to check it hasn't slipped through the cracks. There is one mechanical blocker worth flagging: all 14 workflow runs are still Would it also be possible to point this at the right reviewer for the web-v2 area, and to assign issue #13356 to me if that helps track it? I noticed the issue was labeled Thanks for your time. |
…esponse body `loginApi` resolves to the response body, because the axios wrapper runs with `isTransformResponse: false`, so `refreshToken` reading `res.data` got `undefined` and the destructuring threw `TypeError: Cannot read properties of undefined (reading 'access_token')`. The scheduled refresh therefore never stored a new token, and the next request after expiry returned 401 and sent the user back to the login page. Read the token from `res`, as `loginAction` already does, and cover the refresh flow with a regression test. Generated-by: ZCode agent (deepseek-v4.1-flash model)
54037fc to
1901a6e
Compare
What changes were proposed in this pull request?
refreshToken(
web-v2/web/src/lib/store/auth/index.js), matchingloginActionin the same file.Why are the changes needed?
loginApiresolves to the response body:defHttpruns withisTransformResponse: false(web-v2/web/src/lib/utils/axios/index.js:344), and thatbranch of
transformResponseHookreturnsres.data(
web-v2/web/src/lib/utils/axios/index.js:64-66).refreshTokendestructuredres.data,which is
undefinedfor a body, so every scheduled refresh threwand no new token was stored. Once the token expired, the next request returned 401 and
the user was redirected to the login page.
Fix: #13356
Does this PR introduce any user-facing change?
Yes. With
gravitino.authenticator.oauth.provider=default, the scheduled token refreshnow stores the new token instead of failing, so an expired token no longer forces a
re-login.
How was this patch tested?
pnpm test(vitest, Node 20.19.0 as pinned by the build): 5 files / 56 tests pass.Before the fix the new test fails with
expected 'auth/refreshToken/rejected' to be 'auth/refreshToken/fulfilled'.pnpm lint(eslint src --max-warnings=0): passes.loginApimocked, ratherthan by hand against a live OAuth server, because the defect is in the response
handling rather than in the HTTP call itself.
A few notes for reviewers:
replied "you can go ahead, thank you" — so this PR is that fix. Happy to hold it if you would
rather assign it first.
the default-provider login (
app/login/components/DefaultLogin.js:43dispatchessetIntervalIdAction); the OIDC login path usesuserManagerand never reachesrefreshToken, so OIDC token handling is unaffected. The form's defaultgrant_type=client_credentialsalso means re-posting the stored params really doesreturn a fresh
access_tokenandexpires_in.res.datawas the only remaining misuse of this kind underweb-v2/web/src/lib: theonly other use is inside the axios wrapper, where returning
res.datais correct.vitest.config.js, even though that wouldfix the root cause for every future store test: it changes how all test files are
transformed. Mocking the request layer is the smaller change and follows the existing
convention that vitest tests mock their dependencies.
web-v2/web. The legacy UI still reads the wronglevel in
web/web/src/lib/store/auth/index.js:63; I left it out to keep the changesmall and will extend it here if you would rather fix both in one go.
branch-1.3contains the same code in both frontends, so this needs thebranch-1.3label if you want the automatic cherry-pick to pick it up.
web-v2/web/src/lib/store/auth/index.test.jsdid not load onmain:Failed to parse source for import analysis because the content contains invalid JS syntax ... File: src/lib/provider/session.js:164. The store reaches the request layer,which imports the JSX provider in
provider/session.js:159. Thevi.mockcalls addedhere keep these tests off that import path. No existing assertion was changed — the two
tests that were already in the file simply run again now.
web-v2's frontend unit tests are not run in CI, which is how the above wentunnoticed:
web-ui-tests.ymlfilters onweb/web/**only, and:web-v2:web:buildruns lint, prettier and the Next.js build but not
vitest. Worth a separate change ifyou agree.