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

Allow user to supply their own oauth app #130

Merged
merged 5 commits into from Apr 19, 2017

Conversation

@jarodmeng
Copy link
Contributor

commented Aug 5, 2016

No description provided.

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2016

Related to #127

@craigcitro

This comment has been minimized.

Copy link
Collaborator

commented Aug 5, 2016

This seems totally reasonable -- @hadley may have feelings about option naming?

@hadley

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

I'd rather have one interface - i.e. options or set_access_cred(), not both.

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2016

@hadley I only went for the options route and didn't modify set_access_cred(). The option is for user to supply their own oauth app to be used in oauth2.0_token().

@hadley

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

I get that - but why as an option instead of just exposing set_access_cred() so you can do whatever you want?

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2016

Mainly for convenience. To supply their own oauth app outside of get_access_cred(), user would need to 1) create an endpoint; 2) create an oauth app; 3) call oauth2.0_token with the endpoint, app and scope to get a token; 4) call set_access_cred() with the token. Using options inside get_access_cred() means user only needs to do 2) and put it in a global option.

@hadley

This comment has been minimized.

Copy link
Member

commented Aug 5, 2016

Or we could pull that bit out of get_access_cred() and provide a set_oauth2.0_cred() or similar with more parameters.

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Aug 6, 2016

@hadley I added a set_oauth2.0_cred() which uses bigqr as default oauth app, but allows user to supply their own. get_access_cred() calls set_oauth2.0_cred() in the oauth dance.

@jarodmeng jarodmeng force-pushed the jarodmeng:patch_auth branch from daf5626 to df9a2de Aug 6, 2016

Allow user to supply their own oauth app
Add set_oauth2.0_cred() function and use it in get_access_cred()

Return token from bq_env in get_access_cred()

@jarodmeng jarodmeng force-pushed the jarodmeng:patch_auth branch from df9a2de to c268d5f Aug 7, 2016

@hadley

This comment has been minimized.

Copy link
Member

commented Apr 18, 2017

Are you still interested in this PR? If so, I'd really appreciate a merge/rebase and a bullet in NEWS.

R/auth.r Outdated

#' @rdname get_access_cred
#' @export
set_oauth2.0_cred <- function(app = bigqr) {

This comment has been minimized.

Copy link
@hadley

hadley Apr 18, 2017

Member

Oh @app needs documenting too. What sort of object should it be?

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Apr 18, 2017

I vaguely recall something funky about the PR. I will take another look at the approach and rebase.

@jarodmeng

This comment has been minimized.

Copy link
Contributor Author

commented Apr 19, 2017

@hadley I rebased the PR on rstats-db:master, made some small changes and added a bullet to NEWS. I tried it on my machine using my own oauth app and it worked. LMK if there's anything I missed. Thanks!

@@ -7,6 +7,8 @@

* fixed for dplyr 0.6.0 compatibility

* `set_oauth2.0_cred()` allows user to supply their own Google OAuth application when setting credentials (#130, @jarodmeng)

This comment has been minimized.

Copy link
@hadley

hadley Apr 19, 2017

Member

It doesn't matter here, but in the future, can you please add bullets at the top, not the bottom?

@hadley hadley merged commit 87dade7 into r-dbi:master Apr 19, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Copy link
Member

commented Apr 19, 2017

Thanks!

Zsedo pushed a commit to Zsedo/bigrquery that referenced this pull request Jun 26, 2017

Allow user to supply their own oauth app (r-dbi#130)
* Allow user to supply their own oauth app

Add set_oauth2.0_cred() function and use it in get_access_cred()

Return token from bq_env in get_access_cred()

* Change default value of app for set_oauth2.0_cred() in function

* Improve get_access_cred() documentation

* Add a bullet to NEWS about set_oauth2.0_cred()
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.