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

ENH: Clear authentication defaults with more fine-grained control #129

Closed
max-sixty opened this issue Feb 20, 2018 · 8 comments
Closed

ENH: Clear authentication defaults with more fine-grained control #129

max-sixty opened this issue Feb 20, 2018 · 8 comments

Comments

@max-sixty
Copy link
Contributor

We have about 220 lines of code which handles authentication - do we need that? Generally the answer to "do we need something we do a lot of" is 'Yes', but asking regardless

My prior is that we could:

  1. Check if creds are passed by the user
  2. Otherwise pass nothing through to the Google libraries, and let them manage the defaults

That would reduced the code we needed to maintain and conform to standards - Google have very reasonable defaults, and we make it harder for those to flow through - e.g. PANDAS_GBQ_CREDENTIALS_FILE is non-standard, every other implementation uses GOOGLE_APPLICATION_CREDENTIALS, so users need to have an additional setting to use this library

@tswast
Copy link
Collaborator

tswast commented Feb 21, 2018

I definitely think there is room for improvement here. My ideal flow would be:

  1. Use credentials as explicitly passed by the user.
    • Passing in the bytes of the JSON file as currently done for backwards compatibility.
    • Passing in a Credentials object from google-auth.
  2. Try to load cached user-auth credentials.
  3. Use service account from PANDAS_GBQ_CREDENTIALS_FILE env var. (Should we keep doing this?)
  4. Try to create default credentials using google-auth.
  5. Do three-legged OAuth to get user credentials.

Note that google-auth default credentials does not do three-legged OAuth, so I think that's an area where pandas-gbq will need to keep some auth code around.

Edit: Swapped items (1) and (2) so that explicitly passed credentials are always used first. Also, current behavior does default credentials before PANDAS_GBQ_CREDENTIALS_FILE. If we keep PANDAS_GBQ_CREDENTIALS_FILE around, we should prefer it to default credentials.

@max-sixty
Copy link
Contributor Author

And if we just didn't pass any creds along - we literally did nothing there - what would happen then? How much would the google.cloud.bigquery-like libraries do?

@tswast
Copy link
Collaborator

tswast commented Feb 21, 2018

The google-cloud-bigquery library just does (3) default credentials.

@max-sixty
Copy link
Contributor Author

OK, thanks. I'll ponder

@caiolopes
Copy link

caiolopes commented Feb 26, 2018

Just a small suggestion on this subject, I really enjoy how this library handles the authentication:

https://github.com/nithinmurali/pygsheets

It gives me more freedom than pandas-gbq right now.

It let the user pass a custom credentials object and decide where to save the oauth store instead of ~./config/pandas-gbq/bigquery_credentials.dat

pandas-gbq is giving me problems when running at appengine right now because it ignores that I set PANDAS_GBQ_CREDENTIALS_FILE and uses the default service account for my appengine app.

EDIT:

I saw now that pygsheets uses oauth2client which is deprecated, but anyway, I liked how it gives more control over the authentication.

@caiolopes
Copy link

For example, the method get_application_default_credentials gets my project's default service account at appengine first instead of the json file at PANDAS_GBQ_CREDENTIALS_FILE.

And the worst part is that it doesn't allow me to modify this with any parameter in the read_gbq, to_gbq.

@tswast
Copy link
Collaborator

tswast commented Feb 27, 2018

@caiolopes Yes, that's good feedback. I actually forgot about PANDAS_GBQ_CREDENTIALS_FILE. If we keep that around, it should definitely take precedent before default credentials.

I should also swap (1) and (2) in my above comment. If ever supplied a google-auth Credentials object, we should always use that.

@tswast tswast changed the title Does this package need to handle as much authentication? ENH: Clear authentication defaults with more fine-grained control Apr 3, 2018
@tswast tswast mentioned this issue Apr 7, 2018
9 tasks
@tswast
Copy link
Collaborator

tswast commented Apr 7, 2018

I have fleshed out a design that accounts for this feedback at #161. Closing this issue as a duplicate of that proposal.

@tswast tswast closed this as completed Apr 7, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants