Skip to content

Conversation

@lakelse
Copy link
Contributor

@lakelse lakelse commented Oct 31, 2022

When the client credential access token expiry is within the expiry margin, set it to None.

When the client credential access token expiry is within the expiry
margin, set it to None.
@simonrob
Copy link
Owner

simonrob commented Oct 31, 2022

Can you explain why this would be needed?

This condition is only currently encountered if there is no refresh token present, in which case either we automatically get a new token on expiry (CCG / ROPCG flow) or will need to prompt the user in TOKEN_EXPIRY_MARGIN seconds regardless. What is the benefit from invalidating the session early and prompting the user here?

(I can understand why it might be good to retain the existing token and prompt the user early, but that is not what this change does.)

@lakelse
Copy link
Contributor Author

lakelse commented Oct 31, 2022

Hey @simonrob, first of all, great work on this project!

So, I can only speak to my experience using the client credentials grant flow. When the access token falls into the condition evaluating the expiry, there's no refresh token of course... Then on to the 'elif' checking if the token expiry is less than the current_time... If it's not, the token is never decrypted. Perhaps instead it should be? Forgive my lack of understanding here but
I do know there's bug with the client credentials flow if the access token is within the expiry margin, it'll result in an error because it tries to use the encrypted access token.

@simonrob
Copy link
Owner

I do know there's bug with the client credentials flow if the access token is within the expiry margin, it'll result in an error because it tries to use the encrypted access token.

This is the key part – thanks for pointing this out. I would rather keep the token to use while it is still valid, but happy to accept a change that fixes use of the token within the expiry period (e.g., add an additional else block to decrypt)

@simonrob
Copy link
Owner

Actually, scratch that - the whole point of the expiry period is to refresh the token. I'll merge this request.

@simonrob simonrob merged commit fbf4546 into simonrob:main Oct 31, 2022
@lakelse
Copy link
Contributor Author

lakelse commented Oct 31, 2022

Thanks, Simon! It's a pleasure to make a contribution. I'm sure you've saved people untold hours.

@lakelse lakelse deleted the fix-client-credential-flow-access-token-within-expiry-margin branch October 31, 2022 20:31
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