Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extend low code OAuthAuthenticator with token refresh capabilities #26966

Merged
merged 15 commits into from
Jun 7, 2023

Conversation

flash1293
Copy link
Contributor

@flash1293 flash1293 commented Jun 2, 2023

What

Related to #26342

Allow a user to specify a refresh token updater object as part of the OAuthAuthenticator low code component:

    authenticator:
      type: OAuthAuthenticator
      client_id: "some_client_id"
      client_secret: "some_client_secret"
      token_refresh_endpoint: "https://api.sendgrid.com/v3/auth"
      refresh_token: "{{ config['apikey'] }}"
      refresh_request_body:
        body_field: "yoyoyo"
        interpolated_body_field: "{{ config['apikey'] }}"
      refresh_token_updater:
        refresh_token_name: "the_refresh_token"
        refresh_token_config_path:
          - apikey

If specified, the refresh token, access token and access token expiry time returned from the authentication response is written back to the config object. This is important if a refresh token can only be used once.

How

  • Add refresh_token_updater object to the existing oauth configuration
  • If set, instantiate a SingleUseRefreshTokenOAuth2Authenticator instead

Following changes were necessary in the CDK class:

  • Replace config id and client config paths params with regular string params (resolve in factory) - if not set they still grab it from the config automatically (required to adapt the xero connector because it uses custom config paths - see Update CDK for source-xero #27007)
  • Add expiry format if it's not a number of seconds for feature compatibility as the regular oauth authenticator already allowed this
  • Do not fail if config values are not available

On the low code CDK side, the jinja interpolation happens in the factory at creation time.

Changes to quickbooks and removing the existing low code component will happen on a separate PR.

Long term, it's planned to merge the functionality in a single class, this has been split out of the initial phase to reduce risk and scope.

🚨 User Impact 🚨

SingleUseRefreshTokenOAuth2Authenticator interface does not support custom config paths anymore for client id and client secret, needs to be adapted by resolving them before creating an instance.

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues CDK Connector Development Kit connectors/source/xero labels Jun 2, 2023
@octavia-squidington-iii octavia-squidington-iii removed the area/connectors Connector related issues label Jun 5, 2023
@airbytehq airbytehq deleted a comment from github-actions bot Jun 5, 2023
@flash1293 flash1293 marked this pull request as ready for review June 5, 2023 09:28
@flash1293 flash1293 requested a review from a team as a code owner June 5, 2023 09:28
@flash1293 flash1293 requested a review from girarda June 5, 2023 09:28
Copy link
Contributor

@girarda girarda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! 🚢

self.access_token = new_access_token
self.set_refresh_token(new_refresh_token)
self.set_token_expiry_date(new_token_expiry_date)
emit_configuration_as_airbyte_control_message(self._connector_config)
return self.access_token

def refresh_access_token(self) -> Tuple[str, int, str]:
def refresh_access_token(self) -> Tuple[str, str, str]:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is fine because the cast is done in get_new_token_expiry_date

@flash1293
Copy link
Contributor Author

flash1293 commented Jun 6, 2023

@girarda Could you take another quick look? Adjusted as discussed in grooming: Make work with empty config values (remove validation and do not fail on empty config values)

The discussed removal of the direct SingleUseRefreshTokenOAuthAuthenticator in low code will be done in a follow-up PR: #27053

Tested two affected connectors below to make sure things still work

@flash1293
Copy link
Contributor Author

flash1293 commented Jun 6, 2023

/test connector=connectors/source-quickbooks local_cdk=1

🕑 connectors/source-quickbooks https://github.com/airbytehq/airbyte/actions/runs/5187304011
✅ connectors/source-quickbooks https://github.com/airbytehq/airbyte/actions/runs/5187304011
Python tests coverage:

Name                              Stmts   Miss  Cover
-----------------------------------------------------
source_quickbooks/__init__.py         3      0   100%
source_quickbooks/components.py      36      4    89%
source_quickbooks/source.py           4      1    75%
-----------------------------------------------------
TOTAL                                43      5    88%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:598: The previous and actual discovered catalogs are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:695: This tests currently leads to too much failures. We need to fix the connectors at scale first.
================== 38 passed, 3 skipped in 458.07s (0:07:38) ===================

@flash1293
Copy link
Contributor Author

flash1293 commented Jun 6, 2023

/test connector=connectors/source-gitlab local_cdk=1

🕑 connectors/source-gitlab https://github.com/airbytehq/airbyte/actions/runs/5188221032
✅ connectors/source-gitlab https://github.com/airbytehq/airbyte/actions/runs/5188221032
Python tests coverage:

Name                        Stmts   Miss  Cover
-----------------------------------------------
source_gitlab/__init__.py       2      0   100%
source_gitlab/source.py        61      3    95%
source_gitlab/streams.py      281     15    95%
-----------------------------------------------
TOTAL                         344     18    95%

Build Passed

Test summary info:

=========================== short test summary info ============================
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:100: The previous and actual specifications are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:598: The previous and actual discovered catalogs are identical.
SKIPPED [1] ../usr/local/lib/python3.9/site-packages/connector_acceptance_test/tests/test_core.py:695: This tests currently leads to too much failures. We need to fix the connectors at scale first.
================== 43 passed, 3 skipped in 550.29s (0:09:10) ===================

@girarda girarda self-requested a review June 6, 2023 14:33
@flash1293 flash1293 merged commit b34fb00 into master Jun 7, 2023
17 checks passed
@flash1293 flash1293 deleted the flash1293/unified-single-use branch June 7, 2023 08:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants