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

Add auth config to oauth api #7798

Merged
merged 8 commits into from
Nov 15, 2021
Merged

Add auth config to oauth api #7798

merged 8 commits into from
Nov 15, 2021

Conversation

ChristopheDuong
Copy link
Contributor

@ChristopheDuong ChristopheDuong commented Nov 9, 2021

What

New OAuth flows will need values from the connector config in #6971

How

Alteernative to #7678

This API changes adds an "optional" auth config parameter to the API endpoint to be potentially used during oauth flows (depends on flow implementation)

Context

  • When FE calls the completeOauth API endpoint, a auth config is built with values extracted from a partial connector config if any fields is used as input fields to the oauth flow
  • once oauth flow is completed, the backend returns the oauth outputs inside the auth config. The Front-end would have to merge this config back to the connector config where it needs to (as declared in the rootObject in auth spec object).
  • Once merged, FE can continue with the modified connectionConfig to SourceCreate when clicking on "setup connection"

@github-actions github-actions bot added area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/server labels Nov 9, 2021
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 9, 2021 19:49 Inactive
Copy link
Contributor

@sherifnada sherifnada left a comment

Choose a reason for hiding this comment

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

I like this approach a lot. It's very well-scoped to the inputs required for oauth. I'd go for this one over #7678

@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 10, 2021 12:44 Inactive
@sherifnada sherifnada added this to the ConnCore, Nov 17 2021 milestone Nov 10, 2021
Copy link
Contributor

@jamakase jamakase left a comment

Choose a reason for hiding this comment

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

Have some questions:

  1. How will it handle airbyte_secret fields?
  2. I believe we don't yet have file upload, but what if we have it at some point, won't it break this endpoint? Though if we upload file initially and then just send ids - it should be fine. But we need to consider such a case in advance.

I understand that this prop is just additional and we are free to send everything we want here, but the problem is that we can't send exact connectorConfiguration property as it may be not valid yet, so there is a possibility that we will send some temp frontend state.

@ChristopheDuong
Copy link
Contributor Author

Have some questions:
I understand that this prop is just additional and we are free to send everything we want here, but the problem is that we can't send exact connectorConfiguration property as it may be not valid yet, so there is a possibility that we will send some temp frontend state.

In this PR, we changed our minds and instead of sending connectorConfiguration, we'll be restricting to build a new config object authInputConfiguration with only fields that are needed by the OAuth flow as described in its Input Specification node.

@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 12, 2021 10:05 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 12, 2021 15:51 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 12, 2021 17:50 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 12, 2021 18:39 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 12, 2021 19:16 Inactive
Copy link
Contributor

@cgardens cgardens left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for the short PR with just the iface change!

* Change protocol for new OAuthConfigSpecification

* Add examples

* Add protocol object to api too

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
@github-actions github-actions bot added area/protocol CDK Connector Development Kit labels Nov 15, 2021
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 15, 2021 11:51 Inactive
@ChristopheDuong ChristopheDuong merged commit cdb476e into master Nov 15, 2021
@ChristopheDuong ChristopheDuong deleted the chris/oauth-api-2 branch November 15, 2021 11:56
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 15, 2021 11:57 Inactive
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
* Change OAuth API

* Change protocol for new OAuth Spec (airbytehq#7827)

* Add examples

* Add protocol object to api too

Co-authored-by: Sherif A. Nada <snadalive@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/api Related to the api area/documentation Improvements or additions to documentation area/platform issues related to the platform area/protocol area/server CDK Connector Development Kit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants