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(openIDConnect scheme): Check token expiration before id_token #1417

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

dygeenen
Copy link
Contributor

@dygeenen dygeenen commented Jan 7, 2022

During an Axios call, the token is not automatically refreshed because it relies on the tokenExpired return of the scheme check function, which first checks id_token which often expires at the same time, and therefore does not detect any problems.
By checking first the token expiration before the id_token, the problem is solved

Related issue :
#1370

During an Axios call, the token is not automatically refreshed because it relies on the tokenExpired return of the scheme check function, which first checks id_token which often expires at the same time, and therefore does not detect any problems.
By checking first the token expiration before the id_token, the problem is solved

Related issue : 
nuxt-community#1370
@bmulholland
Copy link
Contributor

Can you please add a summary of why this order is required as a comment in the code? Since we don't have tests, that's the only protection we have from making sure this doesn't break in future.

And speaking of, unless we add tests, there will be risk of this changing in future without awareness of the repercussions. Finding a way to test this change -- in this PR or another one -- would go a long way to ensuring stability. See also #1393

@bmulholland
Copy link
Contributor

Forgot to say -- thanks for the fix!

@tobyreid tobyreid mentioned this pull request Jan 31, 2022
@tobyreid
Copy link
Contributor

@dygeenen - nice spot, I'm using a local copy of this version in my project. Looks like the linting failed in this PR though - keen to see this merged!

@shollingsworth
Copy link
Contributor

dygeenen#1
This seems to fix the linting error.

@jedjebari
Copy link

@dygeenen : do you have some updates about this issue ?

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.

5 participants