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

Fix awaiting of token refresh request before making any further API requests #20383

Merged
merged 4 commits into from
Nov 10, 2023

Conversation

azrikahar
Copy link
Contributor

@azrikahar azrikahar commented Nov 10, 2023

Overview

Reproduction

in all the recordings below, I have artificially added a 5 seconds delay for /auth/refresh API endpoint just to reproduce this issue easier & show the "queueing" clearer. Particularly adding the following code:

await new Promise((resolve) => setTimeout(resolve, 5000));

right before this line:

return next();

as well as setting ACCESS_TOKEN_TTL="45s" to make the access token expire faster.

Admittedly I also had to use Edge's sleeping tab function to prevent the constant periodic token refresh in the background, I believe an alternative way to test this would be to disable said periodic token refresh by removing the setTimeout here:

directus/app/src/auth.ts

Lines 129 to 131 in 09edc03

if (response.data.data.expires <= 2100000000) {
refreshTimeout = setTimeout(() => refresh(), response.data.data.expires - 10000);
}

Context

Currently there is a logic added by #8827 to await token refresh request before any further requests are fired:

directus/app/src/api.ts

Lines 28 to 36 in 09edc03

return new Promise((resolve) => {
if (config.url && config.url === '/auth/refresh') {
queue.pause();
resolve(requestConfig);
queue.start();
} else {
queue.add(() => resolve(requestConfig));
}
});

However the resolve(requestConfig) isn't really a blocking code, so queue.start() gets calls within milliseconds after queue.pause(), as if it was never paused at all. Here's a look at all the requests firing even when the /auth/refresh request is still pending:

msedge_sAvkqicBkq.mp4

The first commit c9f517c of this PR makes sure requests are queued after the refresh request completes:

msedge_mITAvK3uK1.mp4

However as seen in the video, they are still failing. This is because when they were queued, they were still using the old token, so they will still fail. This is why the second commit f436985 of this PR is meant to make it so they will use the new token:

directus/app/src/auth.ts

Lines 120 to 121 in 3f3db2c

// Add the header to the API handler for every request
api.defaults.headers.common['Authorization'] = `Bearer ${accessToken}`;

and make sure it's updated in the queued requestConfig when p-queue does call the arrow function:

msedge_gHqS5cN301.mp4

Scope

What's changed:

  • Ensure the previous "pause all requests while token is refreshing" to work properly now
  • Made sure requests that are queued will use the new token rather than the token that they were using when they were added to the queue

Potential Risks / Drawbacks

  • It should be fixing a logic that was intended by wasn't working, so I'm not sure are there new risks added here
  • Tiny drawback would be requests are technically ever so slightly slower than before since they need to wait for /auth/refresh to finish, even if the old access token has a long valid duration.

Review Notes / Questions

None at the moment


Fixes #18016

@azrikahar azrikahar requested review from a team and DanielBiegler and removed request for a team November 10, 2023 14:21
Copy link

changeset-bot bot commented Nov 10, 2023

🦋 Changeset detected

Latest commit: 3f3db2c

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@directus/app Patch
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

app/src/api.ts Outdated Show resolved Hide resolved
Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Nice work 🚀

@paescuj paescuj merged commit fc1b4ca into main Nov 10, 2023
4 checks passed
@paescuj paescuj deleted the fix/18016 branch November 10, 2023 15:44
@github-actions github-actions bot added this to the Next Release milestone Nov 10, 2023
@paescuj paescuj self-assigned this Nov 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

App doesn't properly refresh the session once the tab gets focus again
2 participants