Skip to content

feat: add auth functions and add arg to covidcast #79

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

Merged
merged 22 commits into from
Apr 17, 2023
Merged

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Apr 5, 2023

An implementation of API keys:

  • API key getter function
  • API key setting recipes in README.md
  • add auth arg to covidcast call params
  • needs some testing

@dshemetov
Copy link
Contributor Author

dshemetov commented Apr 13, 2023

Our API supports auth via three approaches:

  1. requests parameter api_key,
  2. HTTP Basic authentication: pass a header with "Authentication: Basic [user:pass]" (in our case user is "epidata" and pass is api key)
  3. HTTP Bearer authentication: pass a header with "Authentication: Bearer [pass]" (pass is api key)

I won't implement option 1, so we don't pollute the user's request URLs and reduce chance of them sharing their API key. The latter two seem equivalent, so I'll implement the (2), since that's implicit in httr::authenticate.

* remove set_auth and update README recipes instead
* change API key prio to env var first, then option
* use httr::authenticate to set query auth key
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Looks nice! Have a bunch of little questions/suggestions. Main things:

  • add mention of query complexity limit
  • typo & tweaks in warning message
  • testing and CI stuff

@brookslogan
Copy link
Contributor

question: Are the old-epidata-endpoint auth token arguments clearly different from the new api key? We might need to go through and say something like "an {endpoint name}-specific authentication token (not the same thing as the newer, system-wide API key)".

@dshemetov
Copy link
Contributor Author

Yes, they are different. Agreed.

@dshemetov dshemetov merged commit dbeaa37 into dev Apr 17, 2023
@dshemetov dshemetov deleted the ds/api-keys branch April 17, 2023 22:43
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