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

source-xero: enable on Cloud and make OAuth Declarative #54127

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

topefolorunso
Copy link
Collaborator

What

Allows users to bring their oauth apps for authentication. This removes the restriction on the number of OAuth users Airbyte's app can have. Resolves #42126

How

Achieved by making the OAuth declarative as per doc

Review guide

  1. manifest.yaml

User Impact

Users will only be able to use OAuth2 method to authenticate connector.

Can this PR be safely reverted and rolled back?

  • YES 💚
  • NO ❌

Copy link

vercel bot commented Feb 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
airbyte-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Feb 19, 2025 8:00pm

@octavia-squidington-iii octavia-squidington-iii added area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/xero labels Feb 16, 2025
Copy link
Collaborator

@bazarnov bazarnov left a comment

Choose a reason for hiding this comment

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

Nice! I'd like to see some changes on this PR just to make sure we are fully covered with the oAuth implementation here.

Q:

  • how did we test the OAuth Flow ? Did we?

@topefolorunso
Copy link
Collaborator Author

topefolorunso commented Feb 19, 2025

Need guidance on how to test this @bazarnov. Tried using ci in local but got ValueError(\nValueError: Jinja macro has undeclared variables: {'client_id_value', 'client_secret_value'}.

cc: @natikgadzhi

@bazarnov
Copy link
Collaborator

bazarnov commented Feb 19, 2025

@topefolorunso

Please, try to replace yours base_requester with this one:

base_requester:
    type: HttpRequester
    url_base: https://api.xero.com/api.xro/2.0/
    authenticator:
      type: OAuthAuthenticator
      client_id: "{{ config['client_id'] }}"
      client_secret: "{{ config['client_secret'] }}"
      grant_type: refresh_token
      expires_in_name: expires_in
      refresh_request_body: {}
      token_refresh_endpoint: https://identity.xero.com/connect/token
      refresh_token: "{{ config['client_refresh_token'] }}"
      token_expiry_date: "{{ config['token_expiry_date'] }}"
      refresh_request_headers:
          Authorization: "Basic {{ (config['client_id'] ~ ':' ~ config['client_secret']) | base64encode }}"
      refresh_token_updater:
        refresh_token_name: refresh_token
        refresh_token_config_path:
          - client_refresh_token
        access_token_config_path:
          - client_access_token
        token_expiry_date_config_path:
          - token_expiry_date

and the spec.advanced_auth with this one:

advanced_auth:
    auth_flow_type: oauth2.0
    oauth_config_specification:
      oauth_connector_input_specification:
        consent_url: https://login.xero.com/identity/connect/authorize?response_type=code&{{client_id_param}}&{{scope_param}}&{{redirect_uri_param}}
        scope: offline_access accounting.transactions.read accounting.reports.read accounting.budgets.read accounting.reports.tenninetynine.read accounting.journals.read accounting.settings.read accounting.contacts.read accounting.attachments.read assets.read files.read projects.read openid
        access_token_url: https://identity.xero.com/connect/token
        access_token_headers:
          Authorization: "Basic {{ (client_id_value ~ ':' ~ client_secret_value) | b64encode }}"
        access_token_params:
          grant_type: authorization_code
          code: "{{ auth_code_value }}"
          redirect_uri: "{{ redirect_uri_value }}"
        extract_output:
          - refresh_token
          - token_expiry_date
      complete_oauth_output_specification:
        required:
          - refresh_token
          - token_expiry_date
        properties:
          access_token:
            type: string
            path_in_connector_config:
              - client_access_token
          refresh_token:
            type: string
            path_in_connector_config:
              - refresh_token
          token_expiry_date:
            type: string
            path_in_connector_config:
              - token_expiry_date
      complete_oauth_server_input_specification:
        required:
          - client_id
          - client_secret
        properties:
          client_id:
            type: string
          client_secret:
            type: string
      complete_oauth_server_output_specification:
        required:
          - client_id
          - client_secret
        properties:
          client_id:
            type: string
            path_in_connector_config:
              - client_id
          client_secret:
            type: string
            path_in_connector_config:
              - client_secret

The reason the path_in_oauth_response is removed for the manifest-only connector like so, is that the Connector Builder doesn't support this for now, but the extract_output declaration only, unfortunately; so in order to make this source forkable for the rest of the world it should be 100% compatible with Connector Builder.

Once replaced, please check it works for the Connector Builder (OAuthFlow + TestRead), it's expected to have the TestRead works with refresh_token, obtaining the access_token at the OAuthAuthenticator level.

@topefolorunso
Copy link
Collaborator Author

@bazarnov I guess the issue now is getting the access_token and refresh_token in the first place. I should expect to see an authorization screen, right? But nothing pops up. Or am I missing something?

@topefolorunso
Copy link
Collaborator Author

image

image

@topefolorunso
Copy link
Collaborator Author

Notice that the refresh_token parameter is empty in the request

@topefolorunso
Copy link
Collaborator Author

What's the easiest way to test the auth flow please?

@bazarnov
Copy link
Collaborator

Did you provide the testing values correctly?

@topefolorunso
Copy link
Collaborator Author

Yes, that's cliend_id and client_secret right?

@bazarnov
Copy link
Collaborator

Please make sure you're not logged in to any xero accounts during the tests, you have to proceed with the OAuthFlow first, then test read. If you have an active account you've logged in, make sure you test using the correct one or use private browsing instead

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/connectors Connector related issues area/documentation Improvements or additions to documentation connectors/source/xero
Projects
None yet
Development

Successfully merging this pull request may close these issues.

source-xero: enable on Cloud and disable OAuth.
3 participants