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

Support BigQuery OAuth using a refresh token and client secrets #2805

Merged
merged 4 commits into from
Oct 9, 2020

Conversation

drewbanin
Copy link
Contributor

@drewbanin drewbanin commented Oct 1, 2020

resolves #2344
resolves #2802

Description

This PR adds support for the bearer connection method on BQ. When this method is specified, a client_id, client_secret, refresh_token and token_uri must also be provided. Given these secrets, dbt will mint access tokens to authorize BigQuery queries.

TODO

  • Should we rename bearer to oauth-secrets as the auth method?
  • Write tests
  • Changelog entry

Checklist

  • I have signed the CLA
  • I have run this code in development and it appears to resolve the stated issue
  • This PR includes tests, or tests are not required/relevant for this PR
  • I have updated the CHANGELOG.md and added information about my change to the "dbt next" section.

@cla-bot cla-bot bot added the cla:yes label Oct 1, 2020
@@ -165,6 +179,16 @@ def get_bigquery_credentials(cls, profile_credentials):
details = profile_credentials.keyfile_json
return creds.from_service_account_info(details, scopes=cls.SCOPE)

elif method == BigQueryConnectionMethod.BEARER:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jtcohen6 do you have any thoughts / opinions on what we call this auth method?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't mind bearer as long as it reflects what BQ calls this (right?). I prefer it to oauth-secrets

Copy link
Contributor Author

@drewbanin drewbanin Oct 1, 2020

Choose a reason for hiding this comment

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

^ it's actually different than the method described in that link, which is kind of why i do not want to call this bearer! Bearer auth actually feels more to me like the change described in #2802 :D

After some more thought, I think we should roll with refresh-token on this one

Copy link
Contributor

@jtcohen6 jtcohen6 Oct 1, 2020

Choose a reason for hiding this comment

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

QED. Let's call this something else. oauth-refresh-token?

Edit: refresh-token also works, we both got there

@heisencoder
Copy link
Contributor

I'm pretty excited about this feature! Let me know if you need any help with an integration test case (whether that's feasible). Currently, the BIGQUERY_SERVICE_ACCOUNT_JSON is passed in as an environment variable, so maybe a similar variable could be something like BIGQUERY_REFRESH_TOKEN_JSON, which would contain the parameters to the GoogleCredentials.Credentials method.

@drewbanin drewbanin marked this pull request as ready for review October 9, 2020 14:16
@@ -51,19 +54,30 @@ class BigQueryConnectionMethod(StrEnum):
OAUTH = 'oauth'
SERVICE_ACCOUNT = 'service-account'
SERVICE_ACCOUNT_JSON = 'service-account-json'
OAUTH_SECRETS = 'oauth-secrets'
Copy link
Contributor

Choose a reason for hiding this comment

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

Were you going to rename this to refresh-token?

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 decided not to! I actually updated this PR to also accept an access token directly via the token configuration. So, it's kind of just a grab bag of oauth auth methods. Google lets you supply:

  • a client id / client secret / refresh token / token uri OR
  • a standalone access token

The access token is going to expire after ~60 mins (not well defined), but it was a one-line change and feels like a reasonable enough use case to support. You buy it?

Copy link
Contributor

Choose a reason for hiding this comment

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

gotcha — does that mean that this resolves #2802 as well?

Copy link
Contributor

@jtcohen6 jtcohen6 left a comment

Choose a reason for hiding this comment

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

neat!

@drewbanin drewbanin merged commit 7203825 into dev/kiyoshi-kuromiya Oct 9, 2020
@kwigley kwigley deleted the feature/bigquery-oauth-token branch February 5, 2021 17:20
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.

[BigQuery] Raw token authentication method Support BigQuery oauth using a bearer token
5 participants