feat(low-code): add profile assertion flow to oauth authenticator component#236
Conversation
|
/autofix
|
📝 WalkthroughWalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
What do you think about the new profile assertion feature? Do you see any potential use cases that we should consider? 😊 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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: 2
🧹 Nitpick comments (5)
airbyte_cdk/sources/declarative/auth/oauth.py (1)
Line range hint
1-229: Address the formatting issues reported by the pipeline.The pipeline indicates that the file requires formatting using Ruff formatter. Could you please run the formatter to ensure compliance with the project's style guidelines? Wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
unit_tests/sources/declarative/auth/test_oauth.py (1)
Line range hint
1-451: Address formatting issues as indicated by the pipeline.The pipeline reports that the file requires formatting using Ruff formatter. Could you please format the code accordingly? Wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
619-628: Consider enhancing the description foruse_profile_assertion.The current description for
use_profile_assertioncould be more detailed to help users understand when to enable this option. Perhaps providing an example or explaining the use case would be helpful. Wdyt?🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
Line range hint
1-2290: Address the formatting issues reported by the pipeline.The pipeline indicates that the file requires formatting using Ruff formatter. Could you please format the code accordingly? Wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1928-1929: LGTM! Clean implementation of profile assertion handling.The implementation follows the factory pattern consistently by using
_create_component_from_modelto handle the optional profile assertion. This maintains consistency with how other optional components are created.Hey, would you consider adding a comment explaining what a profile assertion is and when it's used? Something like "Profile assertions are used in OAuth JWT Assertion Profile flows (RFC 7523) to generate JWTs before requesting access tokens." wdyt?
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte_cdk/sources/declarative/auth/oauth.py(7 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(1 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(14 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(2 hunks)unit_tests/sources/declarative/auth/test_oauth.py(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[warning] File requires formatting using Ruff formatter
unit_tests/sources/declarative/auth/test_oauth.py
[warning] File requires formatting using Ruff formatter
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
[warning] File requires formatting using Ruff formatter
airbyte_cdk/sources/declarative/auth/oauth.py
[warning] File requires formatting using Ruff formatter
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
2002-2002: LGTM! Profile assertion properly passed to authenticator.The profile assertion is correctly passed to the OAuth authenticator constructor, maintaining the alphabetical order of parameters.
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting using Ruff formatter
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (2)
1255-1258: LGTM! The profile_assertion property is well defined.The property correctly references the JwtAuthenticator schema and aligns with RFC 7523 requirements for JWT assertion-based authentication.
Hey, would you consider adding an example to help developers understand how to use this property? Something like this might be helpful, wdyt?
profile_assertion: title: Profile Assertion description: The authenticator being used to authenticate the client authenticator. "$ref": "#/definitions/JwtAuthenticator" + examples: + - type: JwtAuthenticator + secret_key: "{{ config['jwt_secret'] }}" + algorithm: "RS256"
1259-1263: LGTM! The use_profile_assertion property is well structured.The boolean flag with a default value of false makes it backward compatible and the description clearly explains its purpose.
Consider adding an example to make it more developer-friendly? Maybe something like this, wdyt?
use_profile_assertion: title: Use Profile Assertion description: Enable using profile assertion as a flow for OAuth authorization. type: boolean default: false + examples: + - true + - "{{ config['enable_jwt_profile'] }}"
…ile-assertion' of github.com:airbytehq/airbyte-python-cdk into lazebnyi/extend-oauth-authenticator-component-with-profile-assertion
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
airbyte_cdk/sources/declarative/auth/oauth.py (1)
105-110:⚠️ Potential issueProperly evaluate
use_profile_assertionbefore using in conditionals.Since
use_profile_assertioncan be anInterpolatedBoolean,str, orbool, it's important to evaluate it using.eval(self.config)to ensure it resolves to a boolean value before using it in conditional statements. Otherwise, the conditionals may not behave as expected. Could you update the code to evaluateself.use_profile_assertionappropriately? Wdyt?Apply this diff to ensure
use_profile_assertionis properly evaluated:self.grant_type = InterpolatedString.create( "urn:ietf:params:oauth:grant-type:jwt-bearer" - if self.use_profile_assertion + if (isinstance(self.use_profile_assertion, InterpolatedBoolean) + and self.use_profile_assertion.eval(self.config)) + or self.use_profile_assertion is True else self.grant_type, parameters=parameters, )
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/auth/oauth.py (2)
126-131: Add type annotation forassertion_name.The
assertion_namefield is missing a type annotation. Since it's a constant string, should we addstras the type hint? Also, consider making it a class constant since it's a fixed value. Wdyt?- self.assertion_name = "assertion" + self.assertion_name: str = "assertion"Or as a class constant:
@dataclass class DeclarativeOauth2Authenticator(AbstractOauth2Authenticator, DeclarativeAuthenticator): + ASSERTION_NAME: str = "assertion" ... - self.assertion_name = "assertion" + self.assertion_name: str = self.ASSERTION_NAME
227-238: Add type hints to method calls inbuild_refresh_request_body.The method logic looks good, but there are type safety concerns with the untyped function calls. Should we add type hints to the called methods to improve type safety? Wdyt?
- def get_assertion_name(self) -> str: + def get_assertion_name(self) -> str: """Returns the name of the assertion parameter.""" return self.assertion_name - def get_assertion(self) -> str: + def get_assertion(self) -> str: """Returns the assertion token value.""" if self.profile_assertion is None: raise ValueError("profile_assertion is not set") return self.profile_assertion.token🧰 Tools
🪛 GitHub Actions: Linters
[error] 235-235: Call to untyped function "get_assertion_name" in typed context
[error] 235-235: Call to untyped function "get_assertion" in typed context
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
airbyte_cdk/sources/declarative/auth/oauth.py(7 hunks)
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/auth/oauth.py
[error] 82-82: Argument 1 to "create" of "InterpolatedString" has incompatible type "InterpolatedString | str | None"; expected "InterpolatedString | str"
[error] 86-86: Argument 1 to "create" of "InterpolatedString" has incompatible type "InterpolatedString | str | None"; expected "InterpolatedString | str"
[error] 221-221: Function is missing a return type annotation
[error] 224-224: Function is missing a return type annotation
[error] 225-225: Item "None" of "DeclarativeAuthenticator | None" has no attribute "token"
[error] 235-235: Call to untyped function "get_assertion_name" in typed context
[error] 235-235: Call to untyped function "get_assertion" in typed context
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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: Analyze (python)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/auth/oauth.py (3)
50-51: LGTM! Good type hints for optional client credentials.The type hints correctly reflect that these fields can be either
None,InterpolatedString, orstr. This change nicely supports both basic OAuth and profile assertion flows.
70-71: LGTM! Well-structured profile assertion attributes.The type hints are comprehensive, allowing for flexible configuration of the profile assertion flow. The
use_profile_assertionflag supports multiple types (InterpolatedBoolean,str,bool) which is great for declarative configuration.
144-154: LGTM! Clear and comprehensive validation logic.The validation checks and error messages are well-structured and descriptive. They properly handle both basic OAuth and profile assertion flows.
Maxime Carbonneau-Leclerc (maxi297)
left a comment
Maxime Carbonneau-Leclerc (maxi297)
left a comment
There was a problem hiding this comment.
Just a small question regarding the declarative interface. The rest makes sense to me if the tests with source-google-sheets are good
Aldo Gonzalez (aldogonzalez8)
left a comment
Aldo Gonzalez (aldogonzalez8)
left a comment
There was a problem hiding this comment.
These changes make sense to me. I would like a manifest test, but I don't want to block you on that as we already have a required implementation for this.
|
/autofix
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/http_logger.py (1)
48-48: LGTM! Nice formatting improvement.The spacing adjustment in the type ignore directive
# type: ignore[return-value]aligns better with PEP 484's type checking syntax. The comment also helpfully explains the type mismatch reason.Hey, while we're here, would you be interested in exploring ways to properly type this return value instead of using type: ignore? We could potentially define a proper JsonType or use TypedDict to make this more type-safe, wdyt? 🤔
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
1954-1954: Consider extracting the conditional evaluation logic to reduce duplication.The pattern
InterpolatedString.create(value, parameters=model.parameters or {}).eval(config) if value else valueis repeated for bothclient_idandclient_secret. What do you think about extracting this into a helper method? Something like:+ def _eval_if_present(self, value: Optional[str], parameters: Dict[str, Any], config: Config) -> Optional[str]: + return InterpolatedString.create(value, parameters=parameters or {}).eval(config) if value else value + def create_oauth_authenticator(...): - client_id=InterpolatedString.create(model.client_id, parameters=model.parameters or {}).eval(config) if model.client_id else model.client_id, + client_id=self._eval_if_present(model.client_id, model.parameters, config), - client_secret=InterpolatedString.create(model.client_secret, parameters=model.parameters or {}).eval(config) if model.client_secret else model.client_secret, + client_secret=self._eval_if_present(model.client_secret, model.parameters, config),This would make the code more DRY and easier to maintain. WDYT? 🤔
Also applies to: 1960-1960
🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting with Ruff formatter
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/auth/oauth.py(9 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(3 hunks)airbyte_cdk/sources/http_logger.py(1 hunks)airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py(5 hunks)
✅ Files skipped from review due to trivial changes (1)
- airbyte_cdk/sources/streams/http/requests_native_auth/oauth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- airbyte_cdk/sources/declarative/auth/oauth.py
🧰 Additional context used
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py
[warning] File requires formatting with Ruff formatter
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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 (2)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
1928-1932: LGTM! Clean implementation of profile assertion initialization.The initialization of
profile_assertionfollows the established pattern for optional components and handles the None case appropriately.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting with Ruff formatter
2006-2007: LGTM! Clean addition of profile assertion parameters.The addition of
profile_assertionanduse_profile_assertionparameters is well-placed and aligns with the PR's objective of supporting two-step authentication.🧰 Tools
🪛 GitHub Actions: Linters
[warning] File requires formatting with Ruff formatter
|
/autofix
|
…ile-assertion' of github.com:airbytehq/airbyte-python-cdk into lazebnyi/extend-oauth-authenticator-component-with-profile-assertion
Serhii Lazebnyi (lazebnyi)
deleted the
lazebnyi/extend-oauth-authenticator-component-with-profile-assertion
branch
3 participants
What
To authenticate in some connectors like Google Sheets, we need to pass two-step authentication. Before requesting an access token, the user needs to generate a JWT to authenticate the access token request with it. This is described in RFC 7523 - https://datatracker.ietf.org/doc/html/rfc7523.
How
We have extended the current OAuth implementation with new fields (e.g.
profile_assertion,use_profile_assertion) and methods to use OAuth JWT Assertion Profiles.Summary by CodeRabbit
New Features
Improvements
client_idandclient_secretoptional.Tests