This repository has been archived by the owner on Feb 22, 2023. It is now read-only.
Frontend tokens #1569
Merged
Frontend tokens #1569
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Storybook and Tailwind configuration previews: Ready Storybook: https://wordpress.github.io/openverse-frontend/_preview/1569 Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again. You can check the GitHub pages deployment action list to see the current status of the deployments. |
openverse-bot
added
💻 aspect: code
|
Size Change: -83.6 kB (-6%) ✅ Total Size: 1.35 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Interesting approach! I kind of wish it was possible to do this without relying on Nuxt context. Is there a reason we need to use it? I have to be honest I still don't really understand the point of Vue and Nuxt context stuff. Especially with how hacky and awkward it is to use them from the composition API, which we'll hopefully be able to move away from.
The other question I have is how the token will get refreshed when it expires?
|
|
If we put the client key and secret into the // ~/plugins/api-authentication.server.ts
import type { Plugin, Context } from '@nuxt/types'
interface State {
apiKey: string
expiration: Date
}
const local: { state: State | null } = {
state: null,
}
const getApiKey = async (ctx: Context): State => {
/* ... */
}
const expiresSoon = (s: State): boolean => {
/* ... */
}
const apiAuthentication: Plugin = async (ctx, inject) => {
if (!local.state || expiresSoon(local.state)) {
local.state = await getApiKey(ctx)
}
inject('openverseApiKey', local.state.apiKey)
} |
|
It just sucks to have to have the |
|
With the code above I suggested I think https://www.npmjs.com/package/async-lock or the like might be helpful for that, basically for the same use-case they mention in the docs, just not with Redis. https://www.npmjs.com/package/async-mutex might be simpler and appears to be more widely used. |
|
@sarayourfriend I tried this with a server-side plugin and it was refreshing the access token on every request. It seems to me that plugins are reinitialised on every request and so their state always starts as a blank one. We will need to store this key outside of Nuxt (in Redis, memcached or Node-Cache etc.) so that it can be persisted on the server outside of the Nuxt lifecycle. |
This is odd. I can reproduce it in the dev build but in the production build it works as expected (like a normal node module resolution). I'll have some changes up soon. |
|
Changes pushed. They work if you do the following:
You can try the |
|
Unit tests are currently failing due to missing API token so we'll have to sort out a way around that. Pushing the token into context makes the component code that relies on it a lot simpler. No need to do conditional checks or handle asynchrony. I'll come back and continue working on this later today. |
|
This PR is ready for review now and the description has been updated with up-to-date testing instructions. You can also visit the |
obulat
reviewed
Jul 28, 2022
| /** | ||
| * the schema of the API service | ||
| */ | ||
| export interface ApiService { |
krysal
reviewed
Jul 28, 2022
dhruvkb
commented
Jul 28, 2022
There was a problem hiding this comment.
I ran this locally and the "happy flow" seems to be working pretty well. I'm currently trying to understand how we could test these events:
- access tokens expiring
- token fetch failing (perhaps invalid credentials)
- requests being blocked till the token is refreshed
|
I think we could test all of those using the axios mock adapter. Thanks for pointing them out. I will write those tests. |
krysal
approved these changes
Jul 28, 2022
There was a problem hiding this comment.
This works well on my end, even the docker build on mac with some extra packages. I have some questions, but nothing worth blocking, I believe 👏
Perhaps when the token expires we can force a hard refresh at the next request, so the request is done server side, and then the token gets refreshed? It's not ideal but could be a solution in the interim.
| ], | ||
| css: ['~/styles/tailwind.css', '~/assets/fonts.css', '~/styles/accent.css'], | ||
| head, | ||
| env, | ||
| env, // TODO: Replace with `publicRuntimeConfig` |
There was a problem hiding this comment.
Is this comment valid? When will we have to replace it?
There was a problem hiding this comment.
I don't know when we would change this. @dhruvkb what did you have in mind? I think it should be a separate issue. Can you cut an issue for this if so?
Comment on lines
+146
to
+155
| handleTab(event: KeyboardEvent, element: string) { | ||
| if (this.showScrollButton.value && element !== 'scroll-button') { | ||
| return | ||
| } | ||
| focusIn(document.getElementById('__layout'), Focus.First) | ||
| }, | ||
| fetchMedia(...args: unknown[]) { | ||
| const mediaStore = useMediaStore(this.$pinia) | ||
| return mediaStore.fetchMedia(...args) | ||
| }, |
There was a problem hiding this comment.
Why did these functions change to the Option API?
There was a problem hiding this comment.
Because you can't return functions in setup and also use Options API for some things or else Nuxt will raise the "cannot stringify unbound function" error in the console. See #1237 for where we've fixed this before.
I don't I understand the suggestion or what problem it is trying to solve. The token requests always happen on the server side and the token is never sent to the client (this would be unsafe). When the token is about to expire, we pre-emptively request a new one and all other requests that come in will wait until the new token is retrieved. @krysal I've also dismissed your review because I made some non-trivial changes to the logic and I want to make sure the additional documentation in the code is reviewed and comprehensible. Can you re-review with an eye towards that? I've also added extensive unit tests, some of which are rather complicated due to juggling asynchrony in the tests themselves. @dhruvkb please review the tests and let me know if they are sufficient. If anyone can think of additional tests to write, please let me know. |
obulat
approved these changes
Aug 5, 2022
sarayourfriend
force-pushed
the
frontend_tokens
branch
from
cbceeb6 to
24184fb
Compare
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes
Fixes #1558 by @sarayourfriend
Description
This PR adds a mechanism to send authenticated requests when an API key has been provided (and unauthenticated ones) when not.
This means that API calls during SSR will be authenticated, and not throttled, but API calls from the client (subsequent calls) will be throttled as an authenticated user because they won't have any API key set.
Testing Instructions
TESTING_GUIDELINES.mdfor setting up your local environment with API authentication tokens after registering an app on your local dev server or on staging.Furthermore, test using the docker image:
Make sure to visit the docker-running service at
localhost:8443and not the IP address that nuxt spits out. That IP address is internal to the docker network. For some reason it still lets some requests through but most of them will fail (including static assets). If you stick tolocalhost:8443it works fine though.When making requests to the docker image, you will see more than one API token being generated now in the logs, depending on the number of CPUs docker has access to. That is because pm2 runs a worker per CPU it finds. If you're using docker desktop and it is not configured with multiple CPUs then you will not see this behavior. Please enable at least 2 CPUs so that you can test this!
Checklist
Update index.md).main) or a parent feature branch.Developer Certificate of Origin
Developer Certificate of Origin