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

Add OAuth support #51

Closed
ledell opened this issue Jun 14, 2019 · 14 comments
Closed

Add OAuth support #51

ledell opened this issue Jun 14, 2019 · 14 comments
Assignees

Comments

@ledell
Copy link
Member

ledell commented Jun 14, 2019

Here's an announcement from meetup.com. We need to add support for OAuth, deprecate api_key (I guess we should not remove it from the functions because it will break people's code but we should tell people that it's not active anymore), update the docs and re-test everything. :-(

Changes to our API access

  • API Keys will be replaced by OAuth: We will be removing API keys on August 15, 2019 and requiring you to authenticate with OAuth.
  • Move to OAuth soon for continued free API access: Until August 15, members will be able to apply for OAuth access free of charge. After August 15, anyone who wants to apply for API access through OAuth will need to have a Meetup Pro account in order to do so.
  • We’re removing API version 2: Before August 15, you will need to switch to API version 3. API version 2 will no longer be available. You can find more information on our API updates and answers to common questions on our Help Center.
@ledell
Copy link
Member Author

ledell commented Jun 14, 2019

If I understand this correctly, this means that anyone who wants to use our package will have to either have applied for free OAuth access by Aug 15 or be a meetup.com pro member? That's NOT GOOD!

Here is the sign-up link: https://secure.meetup.com/meetup_api/oauth_consumers/create/

@ledell ledell changed the title Check that meetup.com API changes don't break our package Add OAuth support and deprecate API key methods Jun 14, 2019
@GregSutcliffe
Copy link
Contributor

If anyone wants to join forces, I'm looking into it this week - I'm hoping that adapting the examples in httr won't be too hard to do...

@gdequeiroz
Copy link
Member

@GregSutcliffe: Should we make a list of things that need to be changed?

@GregSutcliffe
Copy link
Contributor

@gdequeiroz seems reasonable. I think we can just implement a token-getting process as shown in httr and migrate the api_key code to use it, plus some docs, but I'm still poking around at the moment.

@GregSutcliffe
Copy link
Contributor

GregSutcliffe commented Jun 20, 2019

OK, so as an extremely hacky first try, I've got get_events() working with an OAuth token. You can see the code in this Gist - I'll clean it up and send a PR when it's... less headache-inducing :). The key message is that it's probably not a lot of code to change (the gist is only ~25 lines of change, although I didn't rename api_key yet...)

With a basic test completed, here's what I think needs to happen:

  • Provide a workflow to generate the token - this is currently in generate_oauth_token.R in the gist.
  • The .get_api_key function could prompt the user for OAuth key/secret and generate a token... otherwise it probably just needs to change to a check that there is a token in scope, and bail if not.
  • Do we want to provide a generic key/secret in the project (similar to what @jennybc did for googlesheets3)?
    • This allows users to get started right away, but I'm not sure what the implications are for the project maintainers (I think it would need a basic privacy policy in place here somewhere...).
    • If yes, the above step can be optional for advanced users, and the basic user just needs to do a web login and grant permissions to the default app.

Once the token is cached, we need to modify .quick_fetch and .fetch_results to use the token - this seems fairly trivial - but are there calls made not using these helpers? I didn't see any but I didn't look too hard yet...

Once OAuth is working, we can check to see if there's any v2 API calls being made, since that's also being deprecated 🙄

@gdequeiroz thoughts?

@GregSutcliffe
Copy link
Contributor

Spending a few moments thinking about this, it seems like:

Do we want to provide a generic key/secret in the project

Is a good possible solution to:

@ledell - If I understand this correctly, this means that anyone who wants to use our package will have to either have applied for free OAuth access by Aug 15 or be a meetup.com pro member? That's NOT GOOD!

If we set up meetupr to have a default OAuth consumer before August 15th, people will still have access to the API, at least until they force the whole thing over to Pro accounts only ;)

@ledell
Copy link
Member Author

ledell commented Jun 20, 2019

@GregSutcliffe

though I didn't rename api_key yet...)

We have two options here to not break existing users' code:

  1. We can remove api_key and can add the ... everywhere to swallow this unused param in "old" meetupr code.
  2. We can leave api_key and mark it as deprecated, and add a new param, oauth_token.

are there calls made not using these helpers

No, we always use the helpers, so all you have to do is modify them.

Do we want to provide a generic key/secret in the project?

Yes, this is a great idea. I will create a new meetup account specifically for this purpose and generate a key for that account that we can use.

we can check to see if there's any v2 API calls being made

I think all of our calls are v3, but good to double check.

Thanks!

@ledell
Copy link
Member Author

ledell commented Jun 21, 2019

@GregSutcliffe Some guidelines on using the API:

Protect any keys used to access the Meetup API and any data you receive via the Meetup API against unauthorized access, use or disclosure, and immediately report security issues to [email protected].

  • Also registration requires a phone number... I assume you signed up since you tested some code already. Did they do any authentication of the phone number (like sending a text)?

@GregSutcliffe
Copy link
Contributor

GregSutcliffe commented Jun 21, 2019

@ledell Thanks for the feedback. I agree with deprecating rather than outright removing, for now at least. The api_key functionality will work fine until Aug 15th, so we probably shouldn't remove it yet - but deprecating it so that users (who may not have seen the Meetup announcement, somehow) get a warning in time seems a plan. A second release after Aug 15th that removes it is then pretty trivial, I guess.

I don't think they used the phone number I provided (I actually put in my work desk phone, which since I work remotely, isn't connected to anything....). The process went through in milliseconds, so it seems automated.

As for protecting user data, since the callback URL is localhost I don't see how we can store any user data... :)

I'll take a shot at a decent PR and get that submitted. Hopefully today, we'll see how it goes.

@GregSutcliffe
Copy link
Contributor

@ledell also (and I'm sure you know this, but just to be sure), the redirect URL is specified on the Meetup side, so when you register the app, remember to use http://localhost:1410/ (with trailing slash) as the value.

@jennybc
Copy link
Collaborator

jennybc commented Jun 21, 2019

If you're looking for design inspiration, I recorded the preferred OAuth design for gargle/googledrive/bigrquery/googlesheets4 in this vignette:

https://gargle.r-lib.org/articles/gargle-auth-in-client-package.html

You won't be using gargle (which is Google specific), but the overall design of how to do auth is pretty general I think and could be adapted.

@GregSutcliffe
Copy link
Contributor

@jennybc thanks! I've already got a working solution based on your older code for googlesheets3 which I adapted to meetup_auth this morning - but I like how that link allows for falling back to API keys cleanly (my solution is rather more clunky). I'll take a detailed look through and see what it would entail to switch over...

@jennybc
Copy link
Collaborator

jennybc commented Jun 21, 2019

@GregSutcliffe Yeah the approach described in the linked gargle vignette is the current evolutionary state of patterns already in use in that whole set of packages, even going back to googlesheets. It's just gradually gotten cleaner as it gets refined in each package and, since they are all Google APIs, it finally got centralized in gargle. But it's all part of a single evolving design pattern.

GregSutcliffe added a commit to GregSutcliffe/meetupr that referenced this issue Jun 21, 2019
GregSutcliffe added a commit to GregSutcliffe/meetupr that referenced this issue Jun 21, 2019
GregSutcliffe added a commit to GregSutcliffe/meetupr that referenced this issue Jun 21, 2019
GregSutcliffe added a commit to GregSutcliffe/meetupr that referenced this issue Aug 23, 2019
@ledell
Copy link
Member Author

ledell commented Aug 23, 2019

We're done here! Thanks again @GregSutcliffe.

@ledell ledell closed this as completed Aug 23, 2019
@ledell ledell changed the title Add OAuth support and deprecate API key methods Add OAuth support Aug 23, 2019
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

8 participants