fix: (DeclarativeOAuthFlow) - allow DeclarativeOauth2Authenticator to use access_token directly when no token_refresh_endpoint or refresh_token values are provided.
#182
Conversation
…authentication-using-access-token-value
bazarnov
added
oauth
declarative_oauth
oauth_in_builder
and removed
bug
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request introduces modifications to the OAuth authentication mechanism in the Airbyte CDK. The primary changes include making the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Hey there! 👋 I noticed these changes look pretty neat. Wdyt about the new flexibility in token management? The sequence diagram might help visualize how the new 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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 (6)
airbyte_cdk/sources/declarative/auth/oauth.py (1)
99-101: Consider validating access_token_value.What do you think about adding validation to ensure access_token_value isn't an empty string when provided? This could prevent potential auth header issues, wdyt?
self._access_token: Optional[str] = ( - self.access_token_value if self.access_token_value else None + self.access_token_value if self.access_token_value and self.access_token_value.strip() else None )airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
57-66: Consider improving readability of the conditional logic.The current condition is a bit complex. What do you think about making it more explicit with intermediate variables, wdyt?
+ has_refresh_endpoint = self.get_token_refresh_endpoint() is not None + has_refresh_token = self.get_refresh_token() is not None + should_use_existing_token = not has_refresh_endpoint or (not has_refresh_token and self.access_token) token = ( self.access_token - if ( - not self.get_token_refresh_endpoint() - or not self.get_refresh_token() - and self.access_token - ) + if should_use_existing_token else self.get_access_token() )unit_tests/sources/declarative/auth/test_oauth.py (1)
132-150: Consider adding more test cases for edge scenarios.The current test covers the happy path well. What do you think about adding tests for:
- Empty string access_token_value
- None access_token_value
- Interaction between access_token_value and refresh_token
- Precedence when both token sources are available
This would help ensure robust behavior across all scenarios, wdyt?
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
1803-1804: Consider addressing the type ignore comment more robustlyInstead of using a type ignore comment, would it make more sense to properly type the
token_refresh_endpointparameter? This could involve updating the type hints in the model or the authenticator class. wdyt?
1838-1838: LGTM! Consider parameter orderingThe addition of
access_token_valuelooks good. Just wondering if we should group it closer to related authentication parameters likeclient_idandclient_secretfor better readability? wdyt?airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
504-509: LGTM! New access_token_value field looks goodThe field is well-defined with clear description and examples. Would you consider adding a note in the description about the precedence between
access_token_valueand the token refresh mechanism? For example, clarifying if providing this value will completely bypass the refresh flow, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
airbyte_cdk/sources/declarative/auth/oauth.py(2 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(2 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py(3 hunks)unit_tests/sources/declarative/auth/test_oauth.py(1 hunks)
🔇 Additional comments (5)
airbyte_cdk/sources/declarative/auth/oauth.py (2)
50-50: LGTM! The optional parameters enhance flexibility.
The changes allow for both dynamic token refresh and static token usage scenarios, which is a good improvement in flexibility.
Also applies to: 58-58
108-116: LGTM! Good error handling.
The None handling is implemented well, and the error message is clear and helpful.
airbyte_cdk/sources/streams/http/requests_native_auth/abstract_oauth.py (1)
210-210: LGTM! Consistent with implementation.
The return type change to Optional[str] aligns well with the implementation class.
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
1062-1067: LGTM! Well-documented schema addition
The new access_token_value property is well-defined with clear title, description, and helpful examples. The schema changes align perfectly with the implementation.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (1)
492-493: LGTM! Making token_refresh_endpoint optional
The change aligns perfectly with the PR objective to allow DeclarativeOauth2Authenticator to work without a token refresh endpoint.
What
Resolving:
How
token_refresh_endpointto beOptionalaccess_token_valueaccess_token_valueto be set inget_auth_header()test_get_auth_header_without_refresh_token_and_without_refresh_token_endpointSummary by CodeRabbit
Summary by CodeRabbit
New Features
token_refresh_endpointis now an optional parameter in the OAuth authentication process, allowing for greater flexibility.access_token_valueto specify an access token directly, bypassing the token refresh process.session_tokenfield for alternative authentication methods.Tests
access_token_valueis provided without atoken_refresh_endpointor refresh token.