-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Core: REST Catalog OAuth2 Support Refresh Token Flow #12362
base: main
Are you sure you want to change the base?
Conversation
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2. Closes apache#12196
Rebased to the latest main HEAD at this moment. Added test is Could someone please review it? Thanks! |
// Catalog headers are used to send requests to the Catalog REST endpoint. | ||
Map<String, String> catalogHeaders = | ||
ImmutableMap.of("Authorization", "Bearer client-credentials-token:sub=catalog"); | ||
// Basic headers are used to send requests to the oauth2 server enepoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a typo here, will fix it with other changes based on comments.
Hi @nika-qubit thank you so much for starting working on this, it's been a while that I wanted to tackle this myself. However I am not sure I agree with the direction taken in this PR: First off, refresh tokens should not be exposed as a client configuration option, for a few reasons:
Secondly, I would argue that since the primary grant type is always Let me expand on that: The That's why the RFC 6749 explicitly states that "a refresh token SHOULD NOT be included" in responses to the Auth0, for instance, does not issue refresh tokens for the So I would suggest instead:
Does that make sense? \cc @danielcweeks @nastra |
@adutra Thanks for the detailed explanation. A little background of why I was adding the refresh token flow: I'm trying to make the auth package work with Google OAuth 2.0. Since the client is not the (cloud) resource owner initially, a user consent is always needed from the very beginning.
Based on my knowledge, Google OAuth 2.0 does not support the From my perspective, both point 2 ("authorization code flow") and point 3 ("refresh token flow") send requests to the auth server with the client credential info with each of their own additional code/token without user interaction. Do you think I should start looking for Google's auth endpoints that do support the "token_exchange" grant type? A side question is that an access token usually expires in an hour. Does it mean if we set the option, the REST client can only work for that amount of time? |
@nika-qubit thanks for the detailed context! Indeed, Google OAuth 2.0 uses the so-called "Authorization Code" flow for authentication. Iceberg REST unfortunately does not have support for this flow. I understand and share your pain here, but imho the right way to proceed is to provide Iceberg REST with support for this flow natively. Just implementing support for refreshing tokens, as you did in this PR, imho is not going to solve this problem since you still need to pass the refresh token manually. Instead, we need the Iceberg REST client to handle that transparently, which would include redirecting the user to their browser for authentication whenever required. But this is a complex flow. The good news is, I've been working on this for quite a while. The plan would be as follows:
So, how urgent is this for you? Can you wait a few weeks more? |
If you set the |
Supported refreshing token using the refresh token flow in addition to token exchange flow for OAuth2.
Closes #12196