mansehajsingh commented on PR #8: URL: https://github.com/apache/polaris-tools/pull/8#issuecomment-2819719187
Hi @adutra! > In Polaris, the token exchange grant type is meant primarily for token refreshes. What is the use case here? Where is the subject token expected to come from? To be honest, writing this part of the implementation I was trying to keep a level of parity with the way authentication was implemented in `RESTSessionCatalog#newSession` in Iceberg, but [I see you've merged some changes recently to revamp that behaviour.](https://github.com/apache/iceberg/pull/12197) If we foresee no use case- even when external OAuth does land in OSS Polaris- to support flexible token exchange outside of token refresh with Polaris, then I have no qualms about removing this part from `AuthenticationSessionWrapper` and just supporting `client_credentials` and a preset bearer token. >What is the use case for the actor token, and where it is supposed to come from? Asking because the actor token [is not honored by OSS Polaris](https://github.com/apache/polaris/blob/6b76c39095d70cc1719c2093dcde6e2ebb5d4fa2/service/common/src/main/java/org/apache/polaris/service/auth/DefaultOAuth2ApiService.java#L68-L69), only vendor-specific products make usage of actor tokens, e.g. Tabular. Which makes me realize that we might be "sneaking in" some vendor-specific features here. I don't mind doing so, but I think they should be more clearly flagged as Snowflake-specific features. You're right- I had mislabelled this, I've updated this in the PR description and the comments. I had been confused looking at the implementation of Iceberg's `OAuth2Util#fromTokenExchange` as it passes the parent `AuthSession`'s `token` as an `actorToken` to `OAuth2Util#exchangeToken`. Taking another look at the implementation, the real reason why the token is needed is so that the parent session puts the existing token in the `Authorization` header since Polaris requires token exchange to be performed with the existing token in the `Authorization` header. Then, the headers are inherited from the parent session in `OAuth2Util#exchangeToken`. If we end up removing token exchange this may not even be relevant anymore. There are no vendor specific implementations I was trying to support here. > That's interesting, thanks for link 👍 I didn't know that Open Catalog already had support for external authentication, since OSS Polaris doesn't – but hopefully not for a long time: https://github.com/apache/polaris/pull/1397. We probably should make sure that whichever support is added here for external auth also works with OSS Polaris with an external IDP like Keycloak or Auth0. Agreed, are we okay to merge this PR ahead of time once it has gone through review and open up an issue to ensure that when external OAuth is finalized in Polaris we ensure that this external OAuth support is compatible? -- 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]
