feat(OAuth): allow for access_token without expiration from the API #324
Conversation
📝 WalkthroughWalkthroughThis PR introduces changes to token management in the OAuth authentication components. In Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant D as DeclarativeOauth2Authenticator
C->>D: get_token_expiry_date()
D->>D: _has_access_token_been_initialized()
alt Token not initialized
D-->>C: Return AirbyteDateTime(datetime.min)
else Token initialized
D-->>C: Return computed token expiry date
end
sequenceDiagram
participant C as Caller
participant A as AbstractOauth2Authenticator
C->>A: _parse_token_expiration_date(value)
alt Value not provided and token valid
A->>A: get_token_expiry_date()
A-->>C: Return current token expiry date
else
A-->>C: Parse and return provided expiration value
end
Possibly related PRs
Suggested labels
Suggested reviewers
What do you think about these suggestions? Would you like to include any additional reviewers or labels? ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/auth/oauth.py (1)
234-237: Consider adding a docstring to explain the fallback behavior, wdyt?The logic to return
datetime.minwhen access token is not initialized is a significant behavior that should be documented.def get_token_expiry_date(self) -> AirbyteDateTime: + """ + Returns the token expiry date. If the access token has not been initialized, + returns the minimum datetime value to ensure token refresh. + """ if not self._has_access_token_been_initialized(): return AirbyteDateTime.from_datetime(datetime.min) return self._token_expiry_date # type: ignore # _token_expiry_date is an AirbyteDateTime. It is never None despite what mypy thinks
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/auth/oauth.py(2 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(1 hunks)unit_tests/sources/declarative/auth/test_oauth.py(2 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
unit_tests/sources/declarative/auth/test_oauth.py
[warning] 1-1: File would be reformatted. Please format the code according to the style guidelines.
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/auth/oauth.py (2)
6-6: LGTM!The import of
datetimeis correctly added alongsidetimedeltato support the new functionality.
239-241: LGTM!The helper method is well-named and follows the single responsibility principle.
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
264-266: LGTM!The added condition elegantly handles the case where no expiry value is provided but the current token is still valid. The comment clearly explains the logic.
unit_tests/sources/declarative/auth/test_oauth.py (2)
304-304: LGTM!The addition of
access_token_valueparameter to the test is consistent with the changes in the implementation.
317-343: LGTM!The new test case thoroughly verifies the behavior when there's no access token but a future expiry date. The test:
- Sets up the authenticator with a future expiry date
- Mocks the HTTP interaction
- Verifies both the token value and expiry date
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
unit_tests/sources/declarative/auth/test_oauth.py (1)
317-344: LGTM! Would you consider adding more assertions to make the test more robust?The test effectively verifies the core functionality. Perhaps we could make it even more thorough by adding assertions for:
- The request headers to ensure proper authentication
- The grant type in the request body
- Edge cases around the expiry date
What do you think? 🤔
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/auth/test_oauth.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
unit_tests/sources/declarative/auth/test_oauth.py (1)
304-304: LGTM! Good addition of access_token_value parameter.This change aligns well with testing token initialization scenarios.
maxi297
deleted the
maxi297/oauth_flow_without_api_provided_expiration
branch
What
See https://github.com/airbytehq/airbyte-enterprise/pull/82#discussion_r1943603994 for more details
How
Allow for initializing an access_token while provided a date in the future for refresh.
Note that if the token_expiry_date is not far enough in the future, the connector might refresh the token on every call. It is the responsibility of the connector to put a date that is far enough in the future to avoid this.
Summary by CodeRabbit
Bug Fixes
Tests