Skip to content
This repository was archived by the owner on Nov 5, 2022. It is now read-only.

Make authentication optional for endpoints that don't require it? #9

Open
rpodcast opened this issue Sep 26, 2017 · 4 comments
Open

Comments

@rpodcast
Copy link

I've been inspired by the new RStudio community portal (powered by discourse) to leverage the discourse API for scraping posts and other information, and luckily I found your package! When reviewing the API documentation and doing some quick trial and error, I noticed that a few of the endpoints do not require an API key nor user name. Within discgolf I verified that the following functions could in fact run just as well without the key or user parameters specified: categories, category, category_latest_topics, category_top_topics, tag, topics_latest, topics_new, and topics_by. For these functions I made a slight modification (along with a new utility function) to bypass using the dc function if the user does not specify user and key. For example here's a revised version of topics_latest and the new function use_auth in action:

topics_latest <- function(url = NULL, key = NULL, user = NULL, ...){
  args <- NULL
  if (use_auth(key, user)) {
    args <- dc(list(api_key = check_key(key), api_username = check_user(user)))
  }
  disc_GET(check_url(url), "latest.json", args, ...)
}
use_auth <- function(key = NULL, user = NULL) {
  # if each of key or user are not null, then assume auth is needed
  tmp <- !is.null(key) & !is.null(user)
  return(tmp)
}

Does this sound like a reasonable approach? I'm happy to make updates and would be glad to send a PR if you find it useful.

@sckott
Copy link
Owner

sckott commented Sep 26, 2017

hi @Thercast , thanks for the issue.

the problem is that the functions check_key and check_user are allowing a user to pass in key and username as args, OR allow them to set those two things as env vars OR R options. The latter two of which then wouldn't pass in api key and username as args to function calls.

so your proposed setup i think wouldn't work when an api key and username are in fact required AND when the user has set key and username as either env vars or R options instead of passing in as args.

@rpodcast
Copy link
Author

Thanks for the feedback! If we can safely assume that the results returned for the aforementioned endpoints are the same with or without credentials, then it might make more sense to simply use disc_GET(check_url(url), endpt, args = NULL, ...) inside those relevant functions. Then we wouldn't need any other function to check if credentials were supplied. Does that sound like a decent approach?

@sckott
Copy link
Owner

sckott commented Sep 26, 2017

I checked to see what the Ruby gem does - it sends credentials if the user supplies them, even if the route doesn't need credentials. I agree with this for the primary reason that it's just less complex

I think the way forward is to not stop() on missing credentials, but just not pass them to the request if not passed as arg, not found in env vars, and not found in R options. For routes that don't need auth, it's all good, for routes that need auth we'll fail with message that auth needed, e,g.

on this request

https://discuss.ropensci.org/topics/private-messages/sckott.json?api_key=asdf&api_username=asdf

we get

{
   "errors": [
        "You are not permitted to view the requested resource."
    ],
    "error_type": "invalid_access"
}

not passing this on right now, but can add that

@sckott
Copy link
Owner

sckott commented Sep 27, 2017

i've pushed some changes, make sure to sync upstream before PR

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants