[Low Code CDK] configurable oauth request payload #13993
Conversation
| @@ -27,13 +27,15 @@ def __init__( | |||
| refresh_token: str, | |||
| scopes: List[str] = None, | |||
| refresh_access_token_headers: Mapping[str, Any] = None, | |||
| refresh_request_body: Mapping[str, Any] = None, | |||
There was a problem hiding this comment.
/auth/oauth.py is marked as deprecated so I wasn't sure if I needed to update this flow. Other recent PRs update both files so I did the same.
| @@ -52,6 +53,8 @@ def _create_subcomponent(self, v, kwargs, config): | |||
| v["options"] = self._merge_dicts(kwargs.get("options", dict()), v.get("options", dict())) | |||
|
|
|||
| return self.create_component(v, config)() | |||
| elif isinstance(v, dict): | |||
| return InterpolatedMapping(v).eval(config=config, kwargs=kwargs) | |||
There was a problem hiding this comment.
The existing code only supports interpolating new class components, but does not currently support standalone dictionary interpolation. Since we first check if there is a class_name, for component mappings, we'll skip over and just return the uninterpolated map. For example, this wouldn't work:
authenticator:
refresh_request_body:
grant_type: "refresh_token"
client_secret: "{{ config['client_secret'] }}"
redirect_uri: "{{ config['redirect_uri'] }}
One caveat with this code is that the standalone dictionary doesn't support options at this level because we don't merge existing and current level options. We could potentially add in something like: self._merge_dicts(kwargs.get("options", dict()), v.get("options", dict())), but I think we'd want to discourage nesting options into the standalone dictionary. It's just confusing from a DX standpoint.
There was a problem hiding this comment.
on second thought, this introduces kind of a concerning side effect to our component generation. For most of our existing components, we interpolate at run time, this change to the factory forces it at parse/instantiation time for all components, which we don't want...
I think this all comes about since we're reusing the existing Oauth2Authenticator shared by declarative and normal integrations. I'd have to shove an interpolator and configs/kwargs/options into the class for get_refresh_request_body() to interpolate like I'd expect it to. I'll think on this a little bit more.
There was a problem hiding this comment.
@brianjlai would it make sense for the factory to create an InterpolatedMapping instead of a standard map when there is no class_name so we can always do interpolation?
| from requests.auth import AuthBase | ||
|
|
||
|
|
||
| class Oauth2Authenticator(AuthBase): |
There was a problem hiding this comment.
as per some offline discussion, we're now going to have a separate declarative oauth authenticator. Its annoying that we have a declarative oauth authenticator that looks roughly similar to the existing CDK one. But at least this can support runtime interpolation w/o shoe horning it into the existing implementation.
| } | ||
|
|
||
|
|
||
| class TestOauth2Authenticator: |
There was a problem hiding this comment.
This is a rough copy of the existing unit tests to ensure we retain the same feature set, but w/ a few extra test variations that rely on inputs that require interpolation.
brianjlai
changed the title
|
100% since this affects the existing connector workflow a little bit |
| from airbyte_cdk.sources.streams.http.requests_native_auth.abstract_oauth import AbstractOauth2Authenticator | ||
|
|
||
|
|
||
| class DeclarativeOauth2Authenticator(AbstractOauth2Authenticator): |
There was a problem hiding this comment.
should we move this to the declarative package so the inter-module dependencies only go in one direction?
There was a problem hiding this comment.
good point. i didn't really consider that, but I think it makes sense to keep interpolation out of existing cdk code as much as possible. will move this
|
/test connector=connectors/source-hubspot
Build PassedTest summary info: |
|
/test connector=connectors/source-slack
|
|
/test connector=connectors/source-linnworks
Build FailedTest summary info: |
|
/test connector=connectors/source-square
Build FailedTest summary info: |
|
/test connector=connectors/source-strava
|
|
/test connector=connectors/source-github
Build PassedTest summary info: |
|
/test connector=connectors/source-salesforce
Build PassedTest summary info: |
|
/test connector=connectors/source-stripe
Build PassedTest summary info: |
|
/test connector=connectors/source-chargebee
Build PassedTest summary info: |
|
I wasn't super convinced that we run SAT on a PR against the current CDK changes (similar to how we had that gap in source acceptance tests before). But either way, I did some sanity checks against a couple of the connectors that use the new |
What
Adds the ability to configure additional fields thats can go into the request body made when refreshing tokens as part of the oauth flow.
How
Uses the existing Oauth2Authenticator class and adds an additional config field called
refresh_request_bodywhich is a mapping of fields to values that will go into the outbound request. In the event of a conflict between oauth language names likeclient_idorrefresh_tokenwe will defer to the existing value instead of the override.Recommended reading order
requests_native_auth/oauth.pythonfactory.py🚨 User Impact 🚨
Are there any breaking changes? What is the end result perceived by the user? If yes, please merge this PR with the 🚨🚨 emoji so changelog authors can further highlight this if needed.
Pre-merge Checklist
Expand the relevant checklist and delete the others.
New Connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampledocs/integrations/README.mdairbyte-integrations/builds.mdAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereUpdating a connector
Community member or Airbyter
airbyte_secret./gradlew :airbyte-integrations:connectors:<name>:integrationTest.README.mdbootstrap.md. See description and examplesdocs/integrations/<source or destination>/<name>.mdincluding changelog. See changelog exampleAirbyter
If this is a community PR, the Airbyte engineer reviewing this PR is responsible for the below items.
/test connector=connectors/<name>command is passing/publishcommand described hereConnector Generator
-scaffoldin their name) have been updated with the latest scaffold by running./gradlew :airbyte-integrations:connector-templates:generator:testScaffoldTemplatesthen checking in your changesTests
Unit
Put your unit tests output here.
Integration
Put your integration tests output here.
Acceptance
Put your acceptance tests output here.