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 connector config to oauth api #7678

Closed
wants to merge 1 commit into from
Closed

Conversation

ChristopheDuong
Copy link
Contributor

@ChristopheDuong ChristopheDuong commented Nov 5, 2021

What

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

How

Alteernative to #7798

This API changes adds an "optional" connector config parameter to the API endpoint:

Context

  • When FE calls the completeOauth API endpoint, a partial connectionConfig (that the user is trying to create, as if it was the SourceCreate API but not exhaustive yet) can be passed.
  • If any fields is used as input fields to the oauth flow, the backend can refer to these values directly in the connectionConfig.
  • once oauth flow is completed, the backend will copy the oauth outputs back inside the connector config where it needs to (in the rootObject) along with placeholder masked instance-wide params and return the modified connectionConfig. The merge of connectionConfig + oauth outputs is therefore not needed in the front end anymore.
  • so after that call the FE can just forward the new 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 5, 2021
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 5, 2021 17:19 Inactive
Comment on lines +3310 to +3312
oauthOutput:
type: object
additionalProperties: true # Oauth parameters like refresh/access token etc.. will be different per API so we don't specify them in advance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I would expect this requires some small tweak to do in the front-end code to keep it compatible with the API change?
(the completeOauthResponse is now nested under oauthOutput)

cc @jamakase

@@ -3296,9 +3302,24 @@ components:
description: The query parameters present in the redirect URL after a user granted consent e.g auth code
type: object
additionalProperties: true # Oauth parameters like code, state, etc.. will be different per API so we don't specify them in advance
CompleteOauthResponse:
connectionConfiguration:
Copy link
Contributor

Choose a reason for hiding this comment

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

How come we are passing in connectionConfiguration and not an oauth specific configuration like we discussed in the spec?

Copy link
Contributor Author

@ChristopheDuong ChristopheDuong Nov 8, 2021

Choose a reason for hiding this comment

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

if we pass the connectionConfiguration (which includes the authConfiguraton as a child node btw)

The backend can do the "seeding" to the connectionConfiguration and return the modified config back to the frontend.

We can also go read the "input" fields such as shop_id directly in the connectionConfiguration.connectorSpecification which would be a "partial" connectionConfig being entered in the UI by the user when the oauth flow is being triggered.

(Note the authConfiguraton could also be retrieved directly from the spec.json from a standardSourceConnectionDefinitionId too, so we wouldn't need to pass it in the API call either)

If the "seeding" is to be done in the UI instead, then I can rename this field to authConfiguration yes
and that'd be extra work to do in the Front-end to handle the input/output from the authConfiguration to merge back into connectionConfiguration
see the thread https://docs.google.com/document/d/1hjyu7hDqXPT1vUtofT7BzkZxDkD5ScpOI3xNAFVlcTs/edit?disco=AAAAQTBC0OQ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The second option where the "seeding" is done by the UI would be from this other alternative PR: #7798 (then we can close and not merge this one)

@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 9, 2021 16:47 Inactive
@swyxio swyxio deleted the chris/oauth-api branch October 12, 2022 18:14
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/server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants