airbytehq / airbyte

The leading data integration platform for ETL / ELT data pipelines from APIs, databases & files to data warehouses, data lakes & data lakehouses. Both self-hosted and Cloud-hosted.
https://airbyte.com
Other
16.14k stars 4.13k forks source link

`DeclarativeOauth2Authenticator` doesn't support refreshed refresh_token value #18035

Closed imrehg closed 1 year ago

imrehg commented 2 years ago

Environment

Current Behavior

I'm implementing a low-code connector using OAuth 2.0 (following this docs), and the source has an OAuth 2.0 flow where the refresh_token is single-use. When the access_token is refreshed, one has to also save the returned (new) refresh_token, otherwise won't be able do another refresh later on. See that source's docs:

Refreshing an access token will invalidate the previous token, if it is still valid. Refreshing is a one-time operation.

As much as I see, airbyte_cdk.sources.streams.http.requests_native_auth.abstract_oauth.AbstractOauth2Authenticator doesn't support that flow, and thus I end up in the connector a successful connection test, but after that unable to run any sync jobs the way AirByte checks connectivity starting with the refresh_token all the time.

Expected Behavior

If the access token request sends back a new refresh token, update the stored value with that one. Potentially being able to define a refresh_token_name.

Logs

Hard to know what logs to include here, happy to add if anything specific requested.

Steps to Reproduce

Just one example, I'm not sure whether there are other sources with similar OAuth 2.0 behaviour that could be used.

For Monzo specifically:

  1. Create a new configuration-based connector, and use the spec.yaml and monzo.yaml for it.
  2. Follow the steps in the Monzo docs to create a new personal application, with type confidential to get refresh tokens
  3. Get a refresh token following the flow (needs a bit of manual play, unfortunately, at the moment)
  4. Set up the connector in AirByte, connector test will succed, but retesting again or running a sync job will fail.
# spec.yaml
documentationUrl: https://example.com
connectionSpecification:
  $schema: http://json-schema.org/draft-07/schema#
  title: Monzo Spec
  type: object
  required:
    - client_id
    - client_secret
    - refresh_token
  additionalProperties: true
  properties:
    client_id:
      type: string
      description: Client ID
      airbyte_secret: true
    client_secret:
      type: string
      description: Client Secret
      airbyte_secret: true
    refresh_token:
      type: string
      description: Refresh Token
      airbyte_secret: true
# monzo.yaml
version: "0.1.0"

definitions:
  selector:
    extractor:
      field_pointer: []
  requester:
    url_base: "https://api.monzo.com"
    http_method: "GET"
    authenticator:
      type: "OAuthAuthenticator"
      token_refresh_endpoint: "https://api.monzo.com/oauth2/token"
      client_id: "{{ config['client_id'] }}"
      client_secret: "{{ config['client_secret'] }}"
      refresh_token: "{{ config['refresh_token'] }}"
  retriever:
    record_selector:
      $ref: "*ref(definitions.selector)"
    paginator:
      type: NoPagination
    requester:
      $ref: "*ref(definitions.requester)"
  base_stream:
    retriever:
      $ref: "*ref(definitions.retriever)"
  whoami_stream:
    retriever:
      $ref: "*ref(definitions.retriever)"
    $options:
      name: "whoami"
      path: "/ping/whoami"

streams:
  - "*ref(definitions.whoami_stream)"

check:
  stream_names:
    - "whoami"

Are you willing to submit a PR?

Yes.

marcosmarxm commented 2 years ago

@girarda and @brianjlai can you take a look?

@imrehg do you need any help to make the contribution?

girarda commented 2 years ago

Unfortunately, there are no ways to update refresh tokens at the moment. This is something the team is working on this quarter https://github.com/airbytehq/airbyte/issues/3990

imrehg commented 2 years ago

Thanks @girarda, that's very useful! @marcosmarxm I took a look and would have some ideas how to approach this, but it might not align well with what the team's already doing. So I might look but not necessarily change anything to not duplicate work, unless of course you have specific guidance to still contribute and avoid that.

Also realised that it is likely more of a feature request than a bug, but probably pretty borderline. Thanks for the updates on this!

girarda commented 1 year ago

closing because single use refresh tokens are supported (using Overwrite config with refresh token response)

marcosmarxm commented 1 year ago

@girarda do we have example in our docs explaining how to use it?

girarda commented 1 year ago

Yes, @marcosmarxm

For the connector builder: https://docs.airbyte.com/connector-development/connector-builder-ui/authentication#update-refresh-token-from-authentication-response