-
Notifications
You must be signed in to change notification settings - Fork 41
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
Openverse: Add endpoint to proxy search requests with authentication #382
Conversation
a953cbf
to
61b4bec
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The suggestions in this review are a little much, but I've made the changes with the idea that this API is likely to get abused heavily at times, and as I don't know how long the oauth tokens last, I've assumed that it's likely that requests will come in during the token refresh window.
These changes in short are:
- Allows requests to be made using the "stale"/almost-expiring token while it's being refreshed
- Limits the token refreshing to happening with hopefully 1, but maybe more, processes. I'm unsure what would happen if multiple requests refreshed the token at the same time (Would one be invalidated by the other? etc) but mostly to avoid causing all requests to request new tokens during the refresh window.
- Avoids more issues with the OV api being unavailable (API error, or network issue), by not constantly hitting the upstream API in those cases for the same search (imagine a refresh type scenario)
- Adds a more friendly human-facing error in the event of a search API HTTP failure in production, and avoids adding the errors as translation strings (when they ideally should not be shown to end-users - and those debugging kind of probably need english errors rather than Arabic)
- Bumps the timeout from 5s to 15s as network & load issues can cause varying latency, it'd be better to wait a few extra seconds than abort while receiving a large response.
Other than that, I see no issues in the rest of the PR or way of doing it.
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Outdated
Show resolved
Hide resolved
public_html/wp-content/plugins/pattern-creator/includes/openverse-client.php
Show resolved
Hide resolved
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
…rse-client.php Co-authored-by: Dion Hulse <[email protected]>
Thanks for the improvements! Definitely worth making it less prone to abuse now, rather than after something happens 🙂 The tokens should stay valid for 10 hours, but there's no guarantee (that's not a documented value). This updated refresh flow makes a lot of sense. And even if an invalid token is used in the
Seems like generating new tokens does not invalidate old tokens, so it should be OK if someone's using the previous token while the refresh happens.
Good point - I've gotten conflicting advice on this across projects in the past (where other devs will be the ones seeing the errors), but untranslated makes sense for these. |
See #375 — This PR adds an API endpoint to proxy the Openverse requests through WP, so that we can add authentication & some light caching. The OV API uses OAuth to bypass rate limiting, so we need to be able to generate and use an oauth token when making requests. Additionally, the requests are cached for an hour, to prevent excessive use of the OV API & make things a little faster for folks making repeated searches.
This will need two new secrets for the consumer ID & secret.
How to test the changes in this Pull Request:
I use Insomnia to test API changes, but you can also request via curl.
Get the secrets, search for "Pattern Directory Openverse Oauth2". Set these in a new file
mu-plugins/1-sandbox.php
(since 0-sandbox is tracked in git).http://localhost:8888/wp-json/wporg/v1/openverse/search
, with the parametersearch
& any value you want to search for{ title, url, thumbnail }
, and you should see two headersX-WP-Total
,X-WP-TotalPages
.yarn wp-env run cli "wp option get ov-oauth-token"
Here's a command for curl, you'll just want to add your auth info (or change
get_items_permissions_check
):