Bmoric/add fetch source schema in oauth #19392

benmoriceau · 2022-11-14T19:43:59Z

What

Address https://github.com/airbytehq/oncall/issues/999. If in the oAuth config alreay exist in the DB, and is not updated, we are using it.

…-fetch-source-schema-in-oauth

benmoriceau · 2022-11-14T22:30:16Z

Trying to test it on my local at the moment.

airbyte-commons/build.gradle

airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java

lmossman

Had a few comments. Most important is the last one, where I ask about if we are using the right OAuth field in the connector spec

lmossman · 2022-11-14T22:33:02Z

airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java

    this.configRepository = configRepository;
    this.oAuthImplementationFactory = new OAuthImplementationFactory(configRepository, httpClient);
    this.trackingClient = trackingClient;
+    this.secretsRepositoryReader = secretsRepositoryReader;
  }

  public OAuthConsentRead getSourceOAuthConsent(final SourceOauthConsentRequest sourceDefinitionIdRequestBody)


This wasn't introduced by this PR, but this parameter should probably be named something like sourceOauthConsentRequestBody instead of sourceDefinitionIdRequestBody. Same for the destination method

lmossman · 2022-11-14T22:36:39Z

airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java

+    Jsons.deserializeToStringMap(oAuthInputConfigurationFromInput)
+        .forEach((k, v) -> {
+          if (AirbyteSecretConstants.SECRETS_MASK.equals(v)) {
+            result.put(k, oAuthInputConfigurationFromDB.get(k).textValue());


Are we guaranteed that oAuthInputConfigurationFromDB will always have an entry for each key here? If not, this could result in an NPE, so we should probably handle that case explicitly

I don't know. I will add a check in case if it is null.

lmossman · 2022-11-14T22:43:15Z

airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java

+        List<String> fieldsToGet =
+            buildJsonPathFromOAuthFlowInitParameters(spec.getAuthSpecification().getOauth2Specification().getOauthFlowInitParameters());


Why are we pulling these fields from the spec.getAuthSpecification()? Isn't that the old/legacy way of configuring OAuth?

The if-statement above which does OAuthConfigSupplier.hasOAuthConfigSpecification(spec) is checking if advancedAuth is set on the spec, which is the new way of configuring OAuth. So it seems like we shouldn't be accessing the legacy auth config fields here right?

You're right, Unfortunatelly spec.getAdvancedAuth().getOauthConfigSpecification().getOauthUserInputFromConnectorConfigSpecification() is a Json object. I will have to parse it based on the description...

benmoriceau · 2022-11-14T23:01:58Z

moving back to draft in order to address comments

lmossman

One more small comment, otherwise this looks fine to me

lmossman · 2022-11-15T01:40:32Z

airbyte-server/src/main/java/io/airbyte/server/handlers/OAuthHandler.java

@@ -195,6 +237,19 @@ public void setDestinationInstancewideOauthParams(final SetInstancewideDestinati
    configRepository.writeDestinationOAuthParam(param);
  }

+  private JsonNode getOAuthInputConfigurationForConsent(final ConnectorSpecification spec,
+                                                    final JsonNode hydratedSourceConnectionConfiguration,
+                                                    final JsonNode destinationDefinitionIdRequestBody) {


nit: rename this last parameter to oAuthInputConfiguration, since this current parameter name is not really quite right

This reverts commit a968078.

…9512)" This reverts commit f08b84f.

* Revert "Revert "Bmoric/add fetch source schema in oauth (#19392)" (#19512)" This reverts commit f08b84f. * Fix complete OAuth * Use var

* Update inputs * PR comments and autogen files * Fix oAuth retry * Handle null * Add missing ; * PR comments * use advanced oAuth * PR comments * More PR comments * Format * Avoid conflict in json extract * Format

This reverts commit a968078.

* Revert "Revert "Bmoric/add fetch source schema in oauth (#19392)" (#19512)" This reverts commit f08b84f. * Fix complete OAuth * Use var

benmoriceau added 3 commits November 10, 2022 14:52

Update inputs

b05453c

PR comments and autogen files

06d6c4d

Fix oAuth retry

0e49686

octavia-squidington-iv 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 14, 2022

benmoriceau added 2 commits November 14, 2022 11:47

Handle null

7dff0bf

Merge branch 'master' of github.com:airbytehq/airbyte into bmoric/add…

ec199d0

…-fetch-source-schema-in-oauth

octavia-squidington-iv removed area/api Related to the api area/documentation Improvements or additions to documentation labels Nov 14, 2022

Add missing ;

3d29990

benmoriceau temporarily deployed to more-secrets November 14, 2022 19:51 Inactive

benmoriceau requested review from lmossman, jdpgrailsdev, alovew, colesnodgrass, girarda and gosusnp November 14, 2022 22:29

benmoriceau marked this pull request as ready for review November 14, 2022 22:29

gosusnp requested changes Nov 14, 2022

View reviewed changes

lmossman reviewed Nov 14, 2022

View reviewed changes

PR comments

796c3ec

benmoriceau temporarily deployed to more-secrets November 14, 2022 22:47 Inactive

use advanced oAuth

ddac0ec

benmoriceau temporarily deployed to more-secrets November 14, 2022 23:47 Inactive

PR comments

35f500a

benmoriceau temporarily deployed to more-secrets November 14, 2022 23:58 Inactive

lmossman reviewed Nov 15, 2022

View reviewed changes

Format

fce3c2b

benmoriceau temporarily deployed to more-secrets November 15, 2022 16:55 Inactive

Merge branch 'master' into bmoric/add-fetch-source-schema-in-oauth

cdd5837

benmoriceau temporarily deployed to more-secrets November 15, 2022 18:36 Inactive

Avoid conflict in json extract

2dc776a

benmoriceau temporarily deployed to more-secrets November 15, 2022 19:38 Inactive

benmoriceau temporarily deployed to more-secrets November 15, 2022 19:40 Inactive

Format

46f0054

benmoriceau temporarily deployed to more-secrets November 15, 2022 22:28 Inactive

benmoriceau temporarily deployed to more-secrets November 15, 2022 23:16 Inactive

Merge branch 'master' into bmoric/add-fetch-source-schema-in-oauth

a416ff6

benmoriceau temporarily deployed to more-secrets November 16, 2022 16:16 Inactive

benmoriceau temporarily deployed to more-secrets November 16, 2022 16:17 Inactive

Merge branch 'master' into bmoric/add-fetch-source-schema-in-oauth

ca32646

benmoriceau temporarily deployed to more-secrets November 16, 2022 21:41 Inactive

benmoriceau merged commit a968078 into master Nov 16, 2022

benmoriceau deleted the bmoric/add-fetch-source-schema-in-oauth branch November 16, 2022 22:30

benmoriceau added a commit that referenced this pull request Nov 16, 2022

Revert "Bmoric/add fetch source schema in oauth (#19392)"

bfa79ec

This reverts commit a968078.

benmoriceau mentioned this pull request Nov 16, 2022

Revert "Bmoric/add fetch source schema in oauth" #19512

Merged

benmoriceau added a commit that referenced this pull request Nov 16, 2022

Revert "Bmoric/add fetch source schema in oauth (#19392)" (#19512)

f08b84f

This reverts commit a968078.

benmoriceau added a commit that referenced this pull request Nov 16, 2022

Revert "Revert "Bmoric/add fetch source schema in oauth (#19392)" (#1…

e081faa

…9512)" This reverts commit f08b84f.

github-actions bot mentioned this pull request Nov 17, 2022

Bump helm chart version reference to 0.40.44 #19546

Merged

benmoriceau added a commit that referenced this pull request Nov 17, 2022

Revert "Revert "Bmoric/add fetch source schema in oauth"" (#19513)

90350c1

* Revert "Revert "Bmoric/add fetch source schema in oauth (#19392)" (#19512)" This reverts commit f08b84f. * Fix complete OAuth * Use var

octavia-squidington-iii mentioned this pull request Nov 18, 2022

Bump Airbyte version from 0.40.18 to 0.40.19 #19579

Merged

akashkulk pushed a commit that referenced this pull request Dec 2, 2022

Revert "Bmoric/add fetch source schema in oauth (#19392)" (#19512)

e68dd89

This reverts commit a968078.

akashkulk pushed a commit that referenced this pull request Dec 2, 2022

Revert "Revert "Bmoric/add fetch source schema in oauth"" (#19513)

414518f

* Revert "Revert "Bmoric/add fetch source schema in oauth (#19392)" (#19512)" This reverts commit f08b84f. * Fix complete OAuth * Use var

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bmoric/add fetch source schema in oauth #19392

Bmoric/add fetch source schema in oauth #19392

benmoriceau commented Nov 14, 2022

benmoriceau commented Nov 14, 2022

lmossman left a comment

lmossman Nov 14, 2022

benmoriceau Nov 14, 2022

lmossman Nov 14, 2022

benmoriceau Nov 14, 2022

benmoriceau Nov 14, 2022

lmossman Nov 14, 2022

benmoriceau Nov 14, 2022

benmoriceau Nov 14, 2022

benmoriceau commented Nov 14, 2022

lmossman left a comment

lmossman Nov 15, 2022

benmoriceau Nov 15, 2022

		List<String> fieldsToGet =
		buildJsonPathFromOAuthFlowInitParameters(spec.getAuthSpecification().getOauth2Specification().getOauthFlowInitParameters());

Bmoric/add fetch source schema in oauth #19392

Bmoric/add fetch source schema in oauth #19392

Conversation

benmoriceau commented Nov 14, 2022

What

benmoriceau commented Nov 14, 2022

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benmoriceau commented Nov 14, 2022

lmossman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment