Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The fetch hook leaves other authenticated admin UI requests using global fetch with Bearer authorization, causing 401 failures when auto mode obtains a DPoP-bound token.
1 open issue
1 file not reviewed
js/pnpm-lock.yaml: Generated file
What changed in this PR
Adds DPoP support to the Admin and Account Consoles through the shared Keycloak adapter, addressing #54675 while retaining Bearer fallback when the server does not advertise DPoP.
Changes:
- Upgrade
keycloak-jsto^26.2.5and enable DPoP auto mode. - Route Account Console API requests through
keycloak.fetch. - Add a configurable fetch transport to the admin client and connect it to the Admin Console.
| File | Description |
|---|---|
js/pnpm-lock.yaml |
Lock keycloak-js to 26.2.5. |
js/libs/ui-shared/src/context/KeycloakContext.tsx |
Enable DPoP auto mode during initialization. |
js/libs/ui-shared/package.json |
Upgrade the shared UI adapter dependency. |
js/libs/keycloak-admin-client/src/utils/fetchWithError.ts |
Accept an optional fetch implementation. |
js/libs/keycloak-admin-client/src/resources/clientsV2.ts |
Pass custom fetch to the experimental API transport. |
js/libs/keycloak-admin-client/src/resources/agent.ts |
Use configured fetch for resource requests. |
js/libs/keycloak-admin-client/src/client.ts |
Store and expose custom fetch configuration. |
js/apps/admin-ui/src/admin-client.tsx |
Configure the admin client to use keycloak.fetch. |
js/apps/admin-ui/package.json |
Upgrade the Admin Console adapter dependency. |
js/apps/account-ui/src/api/request.ts |
Send account API requests through keycloak.fetch. |
js/apps/account-ui/package.json |
Upgrade the Account Console adapter dependency. |
Review effort: Balanced
7f1d8e6 to
91a9f15
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Capturing the default fetch breaks existing auth-endpoint tests by bypassing their mocks, and custom transport injection lacks regression coverage for v1/v2 calls, request options, and default-fetch fallback.
3 open issues
1 file not reviewed
js/pnpm-lock.yaml: Generated file
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
DPoP affects authentication in both consoles, but keycloak-js auto-mode and fetch internals are outside this snapshot, leaving HTTPS proof handling and insecure-HTTP fallback dependent on browser/server integration validation.
No open issues 🎉
3 resolved since last review
1 file not reviewed
js/pnpm-lock.yaml: Generated file
Review effort: Balanced
|
@ssilvert @mposolda could you take a look when you have a chance? Review focus
Closes #42813 and #42814. Copilot comments are addressed; CI was still running last I checked. |
There was a problem hiding this comment.
🟡 Changes recommended
The fetch wrapper drops existing Authorization and Content-Type headers from prebuilt Request inputs when init.headers is absent, breaking authenticated requests.
1 open issue
1 file not reviewed
js/pnpm-lock.yaml: Generated file
Review effort: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Default-on DPoP changes authentication in both consoles, but mocked transport tests do not validate proof generation, concurrent token refresh, or insecure-HTTP fallback, leaving these paths dependent on browser/server integration testing.
No open issues 🎉
1 resolved since last review
1 file not reviewed
js/pnpm-lock.yaml: Generated file
Review effort: Balanced
There was a problem hiding this comment.
Thanks for this, Erik. The approach looks reasonable to me, but the Admin UI E2E job is failing reproducibly, so I'm requesting changes until that is sorted out.
Blocker: Admin UI E2E (chromium) fails on every completed run
All three completed runs of this PR fail with nearly the same set of tests:
| Run | Head | Result |
|---|---|---|
| 42007180606 | 5d651a7 | 99 failed |
| 42207074059 | ce9abb8 | 99 failed |
| 42616051744 | 46304ef | 100 failed |
The same specs dominate each time: autentication/flows, users/main, realm-settings/oid4vci-attributes, client-scope/main, plus autentication/policies, autentication/policies-ciba, client-scope/mappers and sessions/sign-out. The symptom is tables and forms that never load (e.g. locator('table tbody').first() never becomes visible in flows.spec.ts:46).
This does not look inherited from main: the most recent chromium failure there is an apt/Playwright browser install error, and the run before it was green.
One pattern that may help narrow it down: the failing specs I sampled sign in with a deep link, login(page, { to: … }), while the passing ones I sampled (realm-roles/main, groups/list) use plain login(page) and navigate afterwards. I have not found the root cause — the server log artifact shows nothing relevant, and I have not gone through the Playwright trace.
Other points
-
No real DPoP coverage. All the new tests mock
fetch. Nothing asserts that a proof is actually sent or that the console token is bound. An E2E or integration check would have value here, given that this turns DPoP on by default for both consoles. -
Upgrade note. Once the console token is DPoP-bound, the server rejects it when presented as plain
Bearer. Console extensions or custom themes that callfetchwithkeycloak.tokendirectly would start getting 401s. I think this needs an entry in the upgrading guide. -
setConfigbehaviour change in@keycloak/keycloak-admin-client. It used to resetrequestOptionswhen the property was omitted; now it keeps the previous value, and neitherrequestOptionsnorfetchcan be cleared through it. That is probably the better behaviour, but it is a change to a public API and should be called out in the description. -
createKeycloakFetchrewrites anyAuthorizationheader to the console token, not only a stale console Bearer token. Fine for the current callers, but surprising for a helper exported fromui-shared. Consider limiting it toBearer …values, or documenting the behaviour.
The earlier Copilot findings (unconverted call sites, default fetch captured at construction, missing transport tests) look addressed in the current head. The remaining raw fetch calls in QuickTheme.tsx and fetchContent.ts are unauthenticated resource loads, so they are fine as they are.
|
Addressed the review feedback: Admin UI E2E blocker — root cause was keycloak-js 26.2.5 restoring the application hash after login with Upgrade note — documented DPoP-bound console tokens and the need for extensions to use
|
There was a problem hiding this comment.
🟡 Changes recommended
The hash assignment creates a spurious Back destination and discards forward history when refreshing a deep link; the router should instead be notified with a popstate event.
1 open issue
1 file not reviewed
js/pnpm-lock.yaml: Generated file
Review effort: Balanced
| window.history.replaceState( | ||
| window.history.state, | ||
| "", | ||
| `${window.location.pathname}${window.location.search}`, | ||
| ); | ||
| window.location.hash = hash; |
Use keycloak-js dpopConfig and keycloak.fetch so console API calls can send DPoP-bound tokens when the server advertises support. Closes keycloak#42813 Closes keycloak#42814 Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Pass adminClient.getFetch() to direct fetchWithError call sites so Bearer-authorized admin UI helpers attach DPoP proofs when needed. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Avoid capturing global fetch in the admin client constructor so later stubs still apply, and add regression coverage for constructor/setConfig injection on v1 and v2 calls. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Concurrent token refreshes could make keycloak.fetch skip the Bearer→DPoP upgrade and send DPoP-bound tokens as Bearer, breaking Admin UI E2E. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
Refactor headers initialization to handle cases with input as Request. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> Signed-off-by: Erik Jan de Wit <edewit@redhat.com> Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
…back. Re-apply the hash restored by keycloak-js via replaceState so createHashRouter observes deep links, limit Authorization resync to Bearer tokens, and document DPoP-bound console tokens plus setConfig behaviour. Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
aea6c8b to
712fed4
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Assigning location.hash after replaceState adds a hashless console entry to navigation history, causing Back to return to the console root instead of the preceding page.
1 file not reviewed
js/pnpm-lock.yaml: Generated file
Review effort: Balanced
Closes #42813
Closes #42814
Summary
keycloak-jsto^26.2.5and enabledpopConfig: { mode: "auto" }in the sharedKeycloakProviderkeycloak.fetch@keycloak/keycloak-admin-clientto use a custom fetch implementation and wire the Admin Console throughkeycloak.fetchcreateHashRouter(keycloak-js restores the hash viareplaceState, which does not notify the router)Note on
setConfigKeycloakAdminClient.setConfig()now keeps the previousrequestOptionsandfetchwhen those properties are omitted, instead of resettingrequestOptionstoundefined. Neither can be cleared throughsetConfigonce set.