GAds oauth demo #5996
GAds oauth demo #5996
Conversation
github-actions
bot
added
area/api
| @@ -1184,6 +1184,29 @@ paths: | |||
| $ref: "#/components/responses/NotFoundResponse" | |||
| "422": | |||
| $ref: "#/components/responses/InvalidInputResponse" | |||
| /v1/source_oauths/oauth_params/create: | |||
There was a problem hiding this comment.
these were just moved for grouping with other oauth endpoints. no actual changes happened here.
| @@ -69,15 +74,32 @@ public JsonNode injectDestinationOAuthParameters(UUID destinationDefinitionId, U | |||
| } | |||
| } | |||
|
|
|||
| private void injectJsonNode(ObjectNode config, ObjectNode fromConfig) { | |||
| @VisibleForTesting | |||
| void injectJsonNode(ObjectNode mainConfig, ObjectNode fromConfig) { | |||
There was a problem hiding this comment.
it was cumbersome to test this via the calling methods so I exposed it here. Open to changing this if we think it's a bad idea.
| public class GoogleAdsOauthFlow extends GoogleOAuthFlow { | ||
|
|
||
| public GoogleAdsOauthFlow(ConfigRepository configRepository) { | ||
| super(configRepository, "https://www.googleapis.com/auth/adwords"); |
There was a problem hiding this comment.
Btw the GoogleAnalytics scope URL is encoded as https%3A//. We should probably handle URL encoding of query params automatically in the abstract class...
There was a problem hiding this comment.
I introduce some additional code to handle these in #6018
| private final String consentUrl = "https://accounts.google.com/o/oauth2/v2/auth"; | ||
| private final String accessTokenUrl = "https://oauth2.googleapis.com/token"; |
There was a problem hiding this comment.
I'm curious to know why these were changed from constant strings to attributes to the class?
Is it because you expect in the future to override these values when extending the base class maybe? In that case, is it more clear to initialize these in the constructor (as it is done with googleQueryParameters?
There was a problem hiding this comment.
I initially had them that way but then realized it's probably not going to be injected separately by each connector. Moved it back to static
| public class GoogleAnalyticsOauthFlow extends GoogleOAuthFlow { | ||
|
|
||
| public GoogleAnalyticsOauthFlow(ConfigRepository configRepository) { | ||
| super(configRepository, "https%3A//www.googleapis.com/auth/analytics.readonly"); |
There was a problem hiding this comment.
| super(configRepository, "https%3A//www.googleapis.com/auth/analytics.readonly"); | |
| super(configRepository, "https://www.googleapis.com/auth/analytics.readonly"); |
| // TODO state should be randomly generated, and the 2nd step of oauth should verify its value | ||
| // matches the initially generated state value |
There was a problem hiding this comment.
In that case, the state value should be returned by the getConsentUrl and then, forwarded by the UI back to the completeOAuthFlow API endpoint?
There was a problem hiding this comment.
maybe just passing the full "consentUrl" in the call from the UI? but that requires storing some state on that side.
…hq/airbyte into sherif/oauth-google-analytics-v2
What
This branch contains working code for demoing oauth with Google Ads. It builds on the branch
chris/google-analytics-oauth(#5911).How
GoogleOauthFlowas a superclass to perform oauth for all google connectors. The main changes are: injectingscopefrom the child class, and allowing the child class to specify how to parse the instancewide config to get client ID and client secret.OauthConfigSupplier.injectConfigto handle merging nested configs. Previously it overwrote any keys. Now it merges any objects with their equivalent objects if they exist, prioritizing thefromConfig. This was needed to work with google ads as the connector nests its credentials inside the config.Recommended reading order
For points 1 & 2:
GoogleAnalyticsOauthFlow.javaGoogleAdsOauthFlow.javaGoogleOauthFlow.javaFor point 3:
OAuthConfigSupplier.java,OAuthConfigSupplierTest.javaImportant Learnings from this PR
It uncovered a few points for improvement: