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

Client credential grant made possible #388

Closed
wants to merge 3 commits into from

Conversation

cderv
Copy link
Contributor

@cderv cderv commented Jul 31, 2016

As explained in #384 , Oauth2 credential grant just need an access token request and then is not possible with httr oauth2 mechanism because authorization request is made at every call.

In this PR, I just add an option to skip the part with an authorization request in order to make use of API with Oauth2 credential grant possible.

Optional arg without_auth_req is set to FALSE by default to keep everyting else the same and not break anything.


This change is Reviewable

client_id = app$key,
client_secret = app$secret,
# redirect_uri = redirect_uri,
grant_type = "client_credentials")
Copy link
Contributor Author

@cderv cderv Dec 16, 2016

Choose a reason for hiding this comment

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

As tested in another use case with Yelp API (see #384), we may not have to use this if solution to set grant_type as grant_type = "client_credentials" could be pass manually by users through user_params = list(grant_type = "client_credentials").
However, there is still code = code that we do not need in a client credentials grant type

Copy link
Member

Choose a reason for hiding this comment

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

I think this block could be made simpler by setting code <- NULL if client_credentials.

Then you could write:

req_params <- list(
  client_id = app$key,
  redirect_uri = redirect_uri,
  grant_type = if (client_credentials) "client_credentials" else "authorization_code",
  code = code
)

Copy link
Contributor Author

@cderv cderv Jul 28, 2017

Choose a reason for hiding this comment

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

For client credential, there is no redirect_uri too. I need to put something like that for this to work :

  req_params <- list(
    client_id = app$key,
    redirect_uri = if (client_credentials) NULL else redirect_uri,
    grant_type = if (client_credentials) "client_credentials" else "authorization_code",
    code = code
  )

code <- NULL by previous code if client_credentials (authorization phase is then skipped)

@hadley
Copy link
Member

hadley commented Jul 24, 2017

Rather than to continue overloading oauth2.0_token with more arguments, it might be better to create a new function. Which of the existing arguments to oauth2.0_token also apply to client credential grants?

@cderv
Copy link
Contributor Author

cderv commented Jul 27, 2017

Client credential grant do not have an authorize phase. So no authorize url to visit for a obtaining a code. It this part of init_oauth2.0 that need to be optional for this kind of grant. In the issue discussion, @jennybc gave an example where she needed to put a dummy authorize url and blank response to skip this part and get the token. It is why I added a new arg to skip this part like use_basic_auth or use_oob.

I can get a token using

rte_app <- oauth_app("opendatarte", key = Sys.getenv("RTE_ID"),
                      secret = Sys.getenv("RTE_SECRET"))

I need to put a dummy authorize for this to work but It does not require one, like for the yelp example

rte_endpoint <-
  oauth_endpoint(NULL,
                 authorize = "https://digital.iservices.rte-france.com/token/oauth/",
                 access = "https://digital.iservices.rte-france.com/token/oauth/")

then I need use_oob = T to have the message in the console and not open a browser window. I need to hit enter interactively for the oauth dance to continue and obtaining the token. This API uses basic auth so I need use_basic_auth = T and I need to precise the grant type.

token <- oauth2.0_token(rte_endpoint, rte_app,
                        user_params = list(grant_type = "client_credentials"),
                        use_basic_auth = T, use_oob = T)

This work to obtain the token.

I do not know for the Yelp API but in my case I do not have a refresh endpoint for the token.

so this give an error

token$refresh()
# Error: Refresh token not available

I needed to define a new R6 Class for this to work asking for a new token using init_oauth2.0 for credential grant. May be another issue but related to this grant.

TokenDataRTE <- R6::R6Class("TokenDataRTE", inherit = httr:::Token2.0, list(
  can_refresh = function() {
    TRUE
  },
  refresh = function() {
    cred <- httr::init_oauth2.0(
      endpoint = self$endpoint,
      app = self$app,
      user_params = self$params$user_params,
      use_basic_auth = self$params$use_basic_auth,
      without_auth_req = self$params$without_auth_req
    )
    if (is.null(cred)) {
      httr:::remove_cached_token(self)
    } else {
      self$credentials <- cred
      self$cache()
      .state$token$credentials <- cred
    }
    self
  }))

Does it get you more insight on this case ?
How should I modify this PR ? or write another one ?

@hadley
Copy link
Member

hadley commented Jul 27, 2017

I think the main question is whether we want to do a major refactoring now, or wait until httr2. It's probably better to wait, and reconsider the API from the ground-up after a careful reading of RFC 6749.

For now, lets make the argument client_credentials, defaulting to FALSE. Can you please also tweak the documentation for oauth_endpoint saying to set authorize to NULL if not needed.

@cderv
Copy link
Contributor Author

cderv commented Jul 28, 2017

OK. Thanks. I see where you want me to go. I worked on something locally. It is working I think. However, should I rebase the PR first ? or make a new one? What are your advices on an old PR with conflicts ?

@hadley
Copy link
Member

hadley commented Jul 28, 2017

Unless you're pretty familiar with git, it's probably just easier to start with a fresh branch and PR.

@cderv
Copy link
Contributor Author

cderv commented Jul 28, 2017

I did a rebase with the current branch, I did handle merge conflict but I need to force push. not sure how a PR discussion respond to that.

So, I began a new branch. It is in my repo. will make a PR when it is ready. Still need to tweak the documentation and add bullet NEWS. Will do this when I will come back Sunday.

Thanks for your help.

@hadley
Copy link
Member

hadley commented Jul 28, 2017

Sounds great - thanks! I'll close this PR and look forward to your new one.

@hadley hadley closed this Jul 28, 2017
@cderv cderv mentioned this pull request Jul 30, 2017
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

Successfully merging this pull request may close these issues.

2 participants