Session refresh check issue

[peteruithoven]

We're running into the issue that after a while user get into a loop where superlogin-client keeps refreshing the session. We think there is something wrong with the checkRefresh() logic.
We hope the following illustration explains the issue.
The user logged in a while ago (on the issued date), he now is beyond refreshThreshold, in this case 30% of total potential session lifetime (issued till expires). (refreshThreshold: 0.3).
The session is refreshed, the issued date stays the same but the expires moves up.
After that refresh the current date is still beyond the refresh threshold, since this is calculated from the issued date until the expires date. This means that when checkRefresh is called again it will have to refresh again.

We noticed the issue because we perform a request after a successful refresh (when the refreshed event is triggered), that new request would trigger another refresh, triggering an infinite loop.

@colinskow, what do you think?

[peteruithoven]

Shouldn't the /refresh request also return a refreshed date, besides the current issued and expires dates? This way a client could calculate the threshold from that refreshed date until the expires date.

Or, should the issued date also be updated on a refresh?

[micky2be]

I see the problem. The issued value is the time the session has been created I believe.
So indeed pushing the expires date doesn't play well with the threshold.
Updating issued or creating refreshed could help.

Alternatively we could go around the problem on the frontend by using a locale date to compare with after a successful refresh.
I think I'll work on this solution but ideally I would like to avoid relying on client time.

[peteruithoven]

I would like to avoid relying on client time

You actually mean time here or client side in general? But we need the time anyway to know how far we are along the threshold?

[micky2be]

Client side time. So far we rely only on what the server give us for beginning/issued and end/expires and compare with client time to check for refresh or what not, which is fine.
Having to set the boundaries on the client though can bring some unwanted behavior (if the difference is way off), but should be fine in most cases.

I'll come up with something, no worries.
In the meantime you might wanna bug @colinskow for the backend side with your proposal.

[peteruithoven]

Awesome. I openend an issue at the superlogin: colinskow/superlogin#162. I'll try to look into superlogin's code what would need to happen to add that refreshed field.

[peteruithoven]

I've opened a pull request at superlogin: colinskow/superlogin#163

[micky2be]

What's the status on this?
Do you need me to make a new release?

[peteruithoven]

This fix seems to work, we've been using a fork with this fix for a while. This does however also requires a fix in superlogin, so we're also running a fork of superlogin. Superlogin-client will fall back to how it used to work if needed, so you can use this without an updated superlogin.

Releasing it sounds like a good idea.