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

Implement connector config dependency for OAuth consent URL #7983

Merged
merged 8 commits into from
Nov 17, 2021

Conversation

ChristopheDuong
Copy link
Contributor

@ChristopheDuong ChristopheDuong commented Nov 15, 2021

What

Closes #6971

How

  • Handle input configuration from connector and forward these values to oauth flow implementation
  • Validate Json configs against the different Json Schemas (input & output) for OAuth flows.

Recommended reading order

  1. airbyte-oauth/src/main/java/io/airbyte/oauth/BaseOAuth2Flow.java
  2. airbyte-oauth/src/main/java/io/airbyte/oauth/BaseOAuthFlow.java
  3. airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java
  4. airbyte-oauth/src/test/java/io/airbyte/oauth/flows/BaseOAuthFlowTest.java

@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 15, 2021 15:31 Inactive
@github-actions github-actions bot added area/platform issues related to the platform area/server labels Nov 15, 2021
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 15, 2021 16:29 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 15, 2021 16:36 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 15, 2021 17:31 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.

LGTM but can you add unit tests for the new functionality?

@@ -75,25 +77,61 @@ public BaseOAuth2Flow(final ConfigRepository configRepository,
}

@Override
@Deprecated
public String getSourceConsentUrl(final UUID workspaceId, final UUID sourceDefinitionId, final String redirectUrl)
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to keep this method, or should it be removed in this PR? (not sure what depends on it so just asking, if easy to remove here probably better)

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 removed it and added test to make sure the ones that need to be backward compatible can still be used without authConfigSpecification

@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 16, 2021 09:15 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 16, 2021 09:54 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 16, 2021 14:40 Inactive
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 16, 2021 16:30 Inactive
…sing oauth (#8018)

* Instance-wide params should only be injected if a connector will be using oauth

* Add test for nested parameters

* format code

* Cleanup code
@ChristopheDuong ChristopheDuong temporarily deployed to more-secrets November 17, 2021 18:35 Inactive
@ChristopheDuong ChristopheDuong merged commit 83c1959 into master Nov 17, 2021
@ChristopheDuong ChristopheDuong deleted the chris/oauth-consent-url branch November 17, 2021 19:07
schlattk pushed a commit to schlattk/airbyte that referenced this pull request Jan 4, 2022
…q#7983)

* Refactor OAuth consent flow with new API

* Use input connector configuration in getConsentURL for OAuth flow

* Instance-wide params should only be injected if a connector will be using oauth (airbytehq#8018)

* Add test for nested parameters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth Flow: new authSpecifications supporting field references from connector config as dependency
2 participants