[#3755] improvement(client-python): Support OAuth2TokenProvider for Python client #4011
Conversation
|
@xloya can you please help to review this? |
Sure, will take a look later. |
| from gravitino.auth.auth_constants import AuthConstants | ||
|
|
||
|
|
||
| class OAuth2TokenProvider(AuthDataProvider): |
There was a problem hiding this comment.
It seems that do not have docs, please add some docs for the new classes which have docs in Java.
| if data: | ||
| request_data = urlencode(data.to_dict()).encode() | ||
| headers = { | ||
| "Content-Type": "application/x-www-form-urlencoded", |
There was a problem hiding this comment.
Can we manage these uniformly as static variables?
There was a problem hiding this comment.
Added as static variables of HTTPClient
noidname01
force-pushed
the
python-oauth
branch
2 times, most recently
from
5b0fb82 to
255cabd
Compare
| OAUTH_PORT = 1082 | ||
|
|
||
|
|
||
| class TestOAuth2TokenProvider(unittest.TestCase): |
There was a problem hiding this comment.
It's better to add an expired token test case, WDYT?
There was a problem hiding this comment.
Actually the last testcase is the test for expired token, but it does not so obvious.
https://github.com/noidname01/gravitino/blob/255cabdb882228f81de1a1907d34fba31cf5b351/clients/client-python/tests/unittests/auth/test_oauth2_token_provider.py#L65
Theold_access_token will be loaded when DefaultOAuth2TokenProvider initializing, then the old_access_token will be replaced with new_access_token by calling get_token_data()
There was a problem hiding this comment.
I see. I think we also need to test abnormal cases, such as using an expired token to access the Server again, which should be rejected. I am not sure whether it is convenient to test the expired token to access the Server in a unit test. If it is not convenient, I think you can add a test case for authentication failure when using an expired token to access the Server in the integration tests.
There was a problem hiding this comment.
Yeah, it seems it's better to test in integration tests because it needs to interact with server.
noidname01
force-pushed
the
python-oauth
branch
2 times, most recently
from
ea69828 to
8f8b5db
Compare
|
@noidname01 do you need to update the code base on #4012 ? |
Yes, I think so. |
|
@noidname01 Do you have time continue on this? |
@jerryshao Sorry for the inactiveness recently, I will finish this PR tonight |
|
Thanks a lot, appreciated. @noidname01 |
|
@jerryshao I have done with rebasing and some fix, please review again. |
| JWT_EXPIRE = "exp" | ||
|
|
||
|
|
||
| class DefaultOAuth2TokenProvider(OAuth2TokenProvider): |
There was a problem hiding this comment.
The file name is strange that 2 doesn't equal to "to", oauth2 is a auth name.
There was a problem hiding this comment.
Ah yes, I know but I miss to modify this file name.
| @@ -50,3 +50,4 @@ def test_auth_provider(self): | |||
| token[len(AuthConstants.AUTHORIZATION_BASIC_HEADER) :] | |||
| ).decode("utf-8") | |||
| self.assertEqual(f"{user}:dummy", token_string) | |||
| os.environ["GRAVITINO_USER"] = "" | |||
There was a problem hiding this comment.
I guess to fix this issue: #4136 .
There was a problem hiding this comment.
@noidname01 How about set it to the origin value, rather than set it to empty string?
There was a problem hiding this comment.
@noidname01 How about set it to the origin value, rather than set it to empty string?
Sure, fixed!
|
Have you tested this using a real OAuth server? |
|
Overall looks good to me, I think you need create an issue to track that add some integration tests for the truly Oauth2 auth. |
No, I haven't. I plan to do this in another PR. I've created #4208 to track. |
What changes were proposed in this pull request?
OAuth2TokenProviderandDefaultOAuth2TokenProviderinclient-pythonWhy are the changes needed?
Fix: #3755, #4136
Does this PR introduce any user-facing change?
No
How was this patch tested?
Add UT and tested by
./gradlew clients:client-python:unittest