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

(de)serialize values for refresh tokens #29

Closed
wants to merge 3 commits into from

Conversation

korsvanloon
Copy link
Contributor

@korsvanloon korsvanloon commented Feb 21, 2024

When you have a federated token that contains values, the values got lost after refreshing.

This was especially a problem in systems that use values for specific logic.
For instance, using anonymous queries or authenticated queries based on an anonymous value.
The access token might be created for an authenticated user, but then after the refresh, the system thought it would be an anonymous token because the value was lost.

Copy link
Member

@mvantellingen mvantellingen left a comment

Choose a reason for hiding this comment

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

This PR changes the working of the module; might be best to discuss offline which problem needs to be solved and how that can be achieved

src/jwt.ts Outdated Show resolved Hide resolved
src/jwt.ts Outdated Show resolved Hide resolved
@@ -27,7 +27,7 @@ export class FederatedAuthPlugin<TContext extends FederatedTokenContext>

const rt = request.http?.headers.get("X-Refresh-Token");
if (rt) {
contextValue.federatedToken.loadRefreshToken(rt);
contextValue.federatedToken.deserializeRefreshToken(rt);
Copy link
Member

Choose a reason for hiding this comment

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

the loadRefreshToken was chosen since it 'loads' it, i would expect a deserializeRefreshToken() to just return the value

@mvantellingen
Copy link
Member

Perhaps also good to outline in the PR description what this PR fixes.

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