-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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 refresh tokens (#2126) #2141
Conversation
9d1df6c
to
bf2c414
Compare
Add a little webserver that spits out a refresh token Add tests + notes on test support
bf2c414
to
d5c24b0
Compare
Rebasing this against 0.15.3 made appveyor think it was relevant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks excellent! Two minor comments here - happy to discuss both of them further!
result = requests.post(token_url, headers=headers, data=data) | ||
result_json = result.json() | ||
if 'access_token' not in result_json: | ||
raise DatabaseException(f'Did not get a token: {result_json}') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you know what kind of contents would be returned in result_json
here? I'm just worried that this could print sensitive-ish info out to the console, like a client id or client secret. If there's a chance that could happen, we should probably stringify the json and mask out creds if we can!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually got error messages about invalid client IDs. I don't think it returns sensitive information, it was actually very annoying to debug because of that (which is good/normal security practice for an endpoint like this):
Encountered an error:
Database Error
Did not get a token: {'data': None, 'message': 'This is an invalid client.', 'code': None, 'success': False, 'error': 'invalid_client'}
from test.integration.base import DBTIntegrationTest, use_profile | ||
|
||
|
||
class TestSnowflakeOauth(DBTIntegrationTest): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we make this test optional? It should definitely run in our test env, but I think some folks might think twice about creating security integrations in their own snowflake accounts to run these tests :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How optional? Opt-in, or opt-out?
resolves #2126
Description
Support refresh tokens by requesting client_id, client_secret, and a refresh token
Add a little webserver that spits out a refresh token given a client ID + client secret
Add tests
You can opt-out of running the new tests by exporting
DBT_INTEGRATION_TEST_SNOWFLAKE_OAUTH_DISABLED=1
into your test environment.Checklist
or tests are not required/relevant for this PRCHANGELOG.md
and added information about my change to the "dbt next" section.