adutra commented on PR #17601: URL: https://github.com/apache/iceberg/pull/17601#issuecomment-5344882171
@bharos thanks for flagging me here. TBH, I am not very supportive of this change 😅 First off, the code paths you are modifying are only activated when a _vended token exchange_ happens, that is, a session context or table context contains a subject token and a subject token type, and the auth manager then exchanges this token against another one, using the parent session's token as the actor token. To my best knowledge, only Trino is leveraging this today: https://github.com/trinodb/trino/blob/38406672349c33d4902bca7a5ebd380b6b382802/plugin/trino-iceberg/src/main/java/io/trino/plugin/iceberg/catalog/rest/TrinoRestCatalog.java#L484-L510 And even Trino is struggling with it. They may remove this code soon. Next, I believe you are conflating an initial token request using token exchange (which is a standard OAuth2 grant) with a _token refresh request_ using token exchange (which is an Iceberg idiosyncrasy). It's not because the initial token was obtained via token exchange that it should be refreshed via token exchange. > 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. Yes that is true. And I agree it's odd. But if we were to fix this properly, we would need to _replay the initial token exchange request_, not send a refresh request using Iceberg's non-standard token exchange flow (which IDPs cannot understand). (Small digression: the question of how to properly refresh an exchanged token is very complex: the exchanged token is bound to the subject token's own lifespan. In general, IDPs do not issue refresh tokens for exchanged tokens, which makes exchanged tokens kind of un-refreshable in practice.) Therefore, unless you have a concrete use case that requires this change, I think it just doesn't make sense anymore to change this part of the codebase. Generally speaking, I think we should stop updating `OAuthUtil` – unless it's a critical bugfix – and move our efforts to the AuthManager v2. Please help moving it forward, the first PR is here: #15703. -- 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]
