bharos opened a new pull request, #17601: URL: https://github.com/apache/iceberg/pull/17601
Fixes #17600. A session created by exchanging a subject token is built through `fromTokenResponse`, which copies the parent configuration with `AuthConfig.builder().from(parent.config())`. That inherits both `credential` and `exchangeEnabled` from the catalog session. `OAuth2Util.refreshToken` branches only on `exchangeEnabled`. So when `token-exchange-enabled` is `false`, an exchanged session refreshes itself with the parent's client credential and receives a token for the catalog client. The session keeps working, but it no longer represents the exchanged subject. This pins `exchangeEnabled` to `true` for sessions built by `fromTokenExchange`, so how a session was obtained decides how it is renewed. #13809 added the flag to control how credential derived sessions refresh, and that behaviour is unchanged. Two points worth calling out for review: 1. This makes `exchangeEnabled` on a child session record how the session was obtained, rather than only reflecting what was configured. An alternative is a separate field on `AuthConfig` for provenance, at the cost of a wider change. I went with the smaller one, happy to switch. 2. The pinned value propagates one level further, because `RESTSessionCatalog` passes the contextual session as the parent of table sessions and `fromAccessToken` also copies the parent config. A vended credential table session under an exchanged session therefore re-exchanges its own token instead of falling back to the client credential. That looks like the correct behaviour for the same reason, but it is a behaviour change beyond the contextual session itself. Not addressed here: `fromAccessToken` inherits the parent credential in the same way, so the bearer token path carries a similar hazard. Left for a separate change to keep this one reviewable. Testing: added `exchangedSessionRefreshesByExchangeWhenExchangeIsDisabled` to `TestOAuth2Util`. It fails without the production change. No existing test reaches this path, since the two tests in `TestRESTCatalog` that disable token exchange never create a session through `fromTokenExchange`. --- **AI Disclosure** - Model: Claude Opus 5 - Platform/Tool: GitHub Copilot - Human Oversight: fully reviewed - Prompt Summary: Investigate why an exchanged REST catalog session loses its identity after refresh, confirm the behaviour on main, and produce a minimal fix with a regression test. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
