iamoceans opened a new pull request, #13380:
URL: https://github.com/apache/gravitino/pull/13380
### What changes were proposed in this pull request?
- Read the OAuth token from the response body in `refreshToken`
(`web-v2/web/src/lib/store/auth/index.js`), matching `loginAction` in the
same file.
- Add a regression test for the refresh flow.
### Why are the changes needed?
`loginApi` resolves to the response body: `defHttp` runs with
`isTransformResponse: false`
(`web-v2/web/src/lib/utils/axios/index.js:344`), and that
branch of `transformResponseHook` returns `res.data`
(`web-v2/web/src/lib/utils/axios/index.js:64-66`). `refreshToken`
destructured `res.data`,
which is `undefined` for a body, so every scheduled refresh threw
```text
TypeError: Cannot read properties of undefined (reading 'access_token')
```
and 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 refresh
now 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.
- The refresh path is covered by dispatching the thunk with `loginApi`
mocked, rather
than 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:
- The issue is not assigned yet. I asked for it in the issue thread and
@xxubai, who reported it,
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 blast radius is exactly the reported scenario. The refresh timer is
only started by
the default-provider login (`app/login/components/DefaultLogin.js:43`
dispatches
`setIntervalIdAction`); the OIDC login path uses `userManager` and never
reaches
`refreshToken`, so OIDC token handling is unaffected. The form's default
`grant_type=client_credentials` also means re-posting the stored params
really does
return a fresh `access_token` and `expires_in`.
- `res.data` was the only remaining misuse of this kind under
`web-v2/web/src/lib`: the
only other use is inside the axios wrapper, where returning `res.data` is
correct.
- I deliberately did not add a JSX loader to `vitest.config.js`, even though
that would
fix 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.
- This PR deliberately touches only `web-v2/web`. The legacy UI still reads
the wrong
level in `web/web/src/lib/store/auth/index.js:63`; I left it out to keep
the change
small and will extend it here if you would rather fix both in one go.
- `branch-1.3` contains the same code in both frontends, so this needs the
`branch-1.3`
label if you want the automatic cherry-pick to pick it up.
- `web-v2/web/src/lib/store/auth/index.test.js` did not load on `main`:
`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`. The `vi.mock`
calls added
here 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
went
unnoticed: `web-ui-tests.yml` filters on `web/web/**` only, and
`:web-v2:web:build`
runs lint, prettier and the Next.js build but not `vitest`. Worth a
separate change if
you agree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
