New OAuth2Manager #2244
New OAuth2Manager #2244
Conversation
There was a problem hiding this comment.
Generally LGTM, a few nit comments. Lets get #2055 merged and then rebase this PR
| - `noop`: No authentication (no Authorization header sent). | ||
| - `basic`: HTTP Basic authentication. | ||
| - `oauth2`: OAuth2 client credentials flow. | ||
| - `legacyoauth2`: Legacy OAuth2 client credentials flow (Deprecated and will be removed in PyIceberg 1.0.0) |
There was a problem hiding this comment.
nit: i think this becomes a little confusing. legacyoauth2 is a fallback mechanism. i.e. when the auth: block is not provided. i think we should call this out
There was a problem hiding this comment.
That makes sense - I'm fine with leaving legacyoauth2 completely out of this section and relying on the existing Authentication Options to explain those configurations
| return self._token | ||
|
|
||
|
|
||
| class OAuth2AuthManager(AuthManager): |
There was a problem hiding this comment.
do we have any tests for LegacyOAuth2AuthManager? do we want OAuth2AuthManager to be feature parity in this first release?
There was a problem hiding this comment.
i dont see credential, resource, and audience
iceberg-python/mkdocs/docs/configuration.md
Lines 368 to 371 in 4cac691
There was a problem hiding this comment.
we are using client_id and client_secret instead in the current implementation, as opposed to credential. This is also currently in draft mode, and I intend to review OAuth2 spec a little bit more in depth and other industry standard implementations before finalizing the implementation.
| @@ -187,3 +289,4 @@ def create(cls, class_or_name: str, config: Dict[str, Any]) -> AuthManager: | |||
| AuthManagerFactory.register("noop", NoopAuthManager) | |||
| AuthManagerFactory.register("basic", BasicAuthManager) | |||
| AuthManagerFactory.register("legacyoauth2", LegacyOAuth2AuthManager) | |||
| AuthManagerFactory.register("oauth2", OAuth2AuthManager) | |||
There was a problem hiding this comment.
nit: is it a good idea to call AuthManagerFactory.register directly in the file? Is it better if its encapsulated in a function?
Im worry about import automatically running this code
There was a problem hiding this comment.
My understanding is that with Python import the module's code is executed only once, and subsequent imports instructs Python to retrieve the loaded module object from sys.modules unless the application owner intentionaly reloads the module. The registration is also currently idempotent
|
i took the liberty to merge main :) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull Request Overview
This PR introduces a new OAuth2Manager implementation that more closely follows RFC6749 standards, replacing the legacy OAuth2 implementation. The new implementation includes proactive token refresh capabilities based on expiration times.
- Adds a new OAuth2AuthManager with RFC6749-compliant implementation
- Introduces thread-safe OAuth2TokenProvider for token management with automatic refresh
- Updates documentation to reflect the new oauth2 auth type and deprecates legacyoauth2
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pyiceberg/catalog/rest/auth.py | Implements OAuth2TokenProvider and OAuth2AuthManager classes with thread-safe token refresh |
| tests/catalog/test_rest.py | Adds integration test for the new oauth2 authentication type |
| mkdocs/docs/configuration.md | Updates documentation to include oauth2 configuration and marks legacyoauth2 as deprecated |
| raise ValueError( | ||
| "The expiration time of the Token must be provided by the Server in the Access Token Response in `expires_in` field, or by the PyIceberg Client." | ||
| ) | ||
| self._expires_at = time.time() + expires_in - self.refresh_margin |
There was a problem hiding this comment.
I think it makes more sense to use time.monotonic() here
There was a problem hiding this comment.
Fantastic suggestion @jayceslesar - thanks!
|
|
||
| def get_token(self) -> str: | ||
| with self._lock: | ||
| if not self._token or time.time() >= self._expires_at: |
| @@ -1646,6 +1646,40 @@ def test_rest_catalog_with_unsupported_auth_type() -> None: | |||
| assert "Could not load AuthManager class for 'unsupported'" in str(e.value) | |||
|
|
|||
|
|
|||
| def test_rest_catalog_with_oauth2_auth_type(requests_mock: Mocker) -> None: | |||
There was a problem hiding this comment.
It would be nice to have a test where fetching the token does not return a 200 or where refreshing the token does not return a 200.
That would allow us to verify that the non-happy paths work as intended.
Rationale for this change
New OAuth2Manager implementation that makes use of AuthManager and more closely follows https://datatracker.ietf.org/doc/html/rfc6749 recommendations.
Proactively refreshes the access token by checking the expiration.
Are these changes tested?
Yes, both in unit and integration tests.
Are there any user-facing changes?
No, this is a new feature.