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

Don't keep checking whether the access token that was just fetched is already expired #4941

Closed
sync-by-unito bot opened this issue Oct 6, 2021 · 7 comments · Fixed by #5013
Closed
Assignees

Comments

@sync-by-unito
Copy link

sync-by-unito bot commented Oct 6, 2021

realm/realm-js#3985
Likely what’s happening is that if the device time is more than 30 min in the future, it thinks its access token is expired, so it requests a new one. But that one will also (immediately) appear to be expired too, so it tries again, etc. in an infinite loop. 
To mitigate this, the client could check if the newly minted access token is already expired. If it is, then likely there's an unrecoverable issue.

@nirinchev
Copy link
Member

Rather than use the expires_at field as it is, we should probably calculate expiration date from the difference between expires_at and issued_at and add that to the local device time.

@jsflax
Copy link
Contributor

jsflax commented Oct 6, 2021

That doesn't sound right unless issued_at corresponds with the "incorrect" device time. Which if that's the case, fine, but I would have expected expires_at to also correspond with the "incorrect" device time.

@nirinchev
Copy link
Member

issued_at and expires_at are set by the server and should be "correct". The device should not try and validate tokens and instead trust the server - the expires_at check is just an optimization to proactively refresh the token and not something that we should be overly strict at enforcing. So if the device time is t0 and issued_at == t1 and expires_at == t2, we should assume that we should refresh the token before t0 + (t2 - t1) - 5 minutes or something.

@jsflax
Copy link
Contributor

jsflax commented Oct 6, 2021

Fixed by #4943.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Oct 7, 2021

➤ Josman Pérez Expósito commented:

Hello Team, do we have an ETA of when this is going to be fixed? I have seen that the pull request has been accepted and it seems to be pending the merge request. I just want to inform the customer of a potential date.
#4943

Thanks

@nirinchev
Copy link
Member

We don't have a timeline yet. We're working on it, but even when it is fixed, it needs to be included in an SDK release. It doesn't seem like an overly urgent issue though, so we won't due out-of-band releases specifically for it. Feel free to tell the customer to watch this issue for updates.

@sync-by-unito
Copy link
Author

sync-by-unito bot commented Oct 7, 2021

➤ Josman Pérez Expósito commented:

Great, thanks for the info [~nikola.irinchev]

@sync-by-unito sync-by-unito bot assigned RedBeard0531 and unassigned jsflax Oct 28, 2021
RedBeard0531 added a commit that referenced this issue Nov 1, 2021
Previously we would check if the access token we just got is still valid using
the local clock, and would stay in WaitingForAccessToken if we think it is
expired. Instead, assume that the just-acquired token is valid and go strait
to Active if we are in WaitingForAccessToken.

Fixes #4941.
@RedBeard0531 RedBeard0531 changed the title Check If the access token that was just fetched is already expired Don't keep checking whether the access token that was just fetched is already expired Nov 3, 2021
RedBeard0531 added a commit that referenced this issue Nov 3, 2021
Previously we would check if the access token we just got is still valid using
the local clock, and would stay in WaitingForAccessToken if we think it is
expired. Instead, assume that the just-acquired token is valid and go strait
to Active if we are in WaitingForAccessToken.

Fixes #4941.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants