fix(rest): accept oauth2-server-uri for the OAuth token endpoint#1369
fix(rest): accept oauth2-server-uri for the OAuth token endpoint#1369tanmayrauth wants to merge 3 commits into
Conversation
Read the portable oauth2-server-uri property used by Java, PyIceberg and iceberg-rust as the OAuth2 token endpoint, so a catalog config that works in those clients also configures iceberg-go. The existing rest.authorization-url key is kept as a compatibility alias, and oauth2-server-uri takes precedence when both are set. toProps now serializes the endpoint under the portable key. Closes apache#1346
Reuse parseAuthURL for oauth2-server-uri so it rejects scheme-less and host-less values like rest.authorization-url does, and serialize the endpoint under both keys so consumers reading the legacy alias keep working.
tanmayrauth
force-pushed
the
fix-1346-oauth2-server-uri
branch
from
2f9de19 to
cbe819d
Compare
zeroshade
left a comment
zeroshade
left a comment
There was a problem hiding this comment.
Thanks for adding oauth2-server-uri support. I found one precedence issue that should be fixed before merge:
- The merged configuration can now contain a client-provided oauth2-server-uri and a server override rest.authorization-url. Because fromProps globally prefers oauth2-server-uri over rest.authorization-url after the maps have already been merged, the client value wins and the server override is ignored. REST config overrides should apply after defaults and client properties. Please preserve layer precedence and only resolve the oauth2-server-uri/rest.authorization-url alias within a single source layer, with a regression test for WithAuthURI(A) plus /config override rest.authorization-url=B.
| } | ||
|
|
||
| // oauth2-server-uri is the portable, preferred key and takes precedence over | ||
| // the rest.authorization-url alias when both are set. |
There was a problem hiding this comment.
This global alias precedence loses REST config layer precedence after fetchConfig merges defaults, client props, and server overrides. If the client supplied WithAuthURI(A), toProps now adds oauth2-server-uri=A, and a server /config override of rest.authorization-url=B leaves both keys in the merged map. This switch then chooses A, so the server override is ignored. Please resolve the oauth2-server-uri/rest.authorization-url alias within each source layer before merging, or otherwise preserve defaults → client → overrides precedence, and add a regression test for WithAuthURI(A) plus a server rest.authorization-url=B override.
There was a problem hiding this comment.
Good catch, thanks — you're right that the global alias resolution broke layer precedence.
Instead of resolving oauth2-server-uri/rest.authorization-url globally after the merge, I now collapse the alias within each source layer before merging in fetchConfig. A new resolveAuthURLAlias helper
canonicalizes each layer to the single oauth2-server-uri key (oauth2-server-uri still wins over the alias within a layer), so the defaults → client → overrides map merge preserves precedence naturally. A server
rest.authorization-url override now correctly wins over a client-provided oauth2-server-uri.
Added TestFetchConfigAuthURLOverridePrecedence covering exactly the WithAuthURI(A) + /config override rest.authorization-url=B case — asserts B wins. fromProps's existing precedence switch is unchanged since it's still correct for the single-layer newCatalogFromProps path.
Resolve the oauth2-server-uri/rest.authorization-url alias within each config layer before merging so a server rest.authorization-url override is no longer shadowed by a client-provided oauth2-server-uri.
3 participants
Read the portable oauth2-server-uri property used by Java, PyIceberg and iceberg-rust as the OAuth2 token endpoint, so a catalog config that works in those clients also configures iceberg-go. The existing rest.authorization-url key is kept as a compatibility alias, and oauth2-server-uri takes precedence when both are set. toProps now serializes the endpoint under the portable key.
Closes #1346