hashgraph
Multi-session user login impossible: cross-context (API+UI) refresh token invalidation
Problem description
This is a bug which has been frustrating the heck out of me that is related to the 2.18.0 release, and I can finally articulate what is going on!
Consider, a situation where you are in parallel experimenting/testing with guardian on the UI and API (or different browsers)
As per the new authentication measures in 2.18.0 there is a hard requirement for the generation of a refresh token before the access token can be created.
There is an issue where that is problematic.
If you are generating new refresh tokens through the API (or potentially in a different browser) the old refresh token loses the ability to generate new access tokens.
A screenshot from the policies view:
This basically happens under the conditions:
- Multiple refresh tokens are generated under different context
- The last refresh token has context to the user object
- (For UI token) After default access token timeout of 60 seconds a new access token can't be generated as the previous refresh token has been overwritten.
This is because of these lines
https://github.com/hashgraph/guardian/blob/main/auth-service/src/api/account-service.ts#L156
This is exacerbated due to this line:
https://github.com/hashgraph/guardian/blob/main/auth-service/src/api/account-service.ts#L185
As the filter is looking for both the username, and the refresh token id (that may not exist on the user object due to it being overwritten)
Step to reproduce
Steps to reproduce the behavior:
- UI Login as user
- Use API to login as same user (or potentially different browser)
- Wait 60 seconds
- See DID missing error
Expected behavior
Looking into the code, it seems that the refresh token should last for a year, this is fine as it is configurable, but losing login context (or a user potentially feeling they lost all their data) isn't great UX.
In terms of code behaviour, I would presume that this change would fix the issue:
const user = await new DataBaseHelper(User).findOne({refreshToken: decryptedToken.id, username: decryptedToken.name});
to
const user = await new DataBaseHelper(User).findOne({username: decryptedToken.name});
The reason why this might be okay, is that the expire at decoding happens on the line above, so a refresh token, would last for the period of time by default.
As this is authentication related, it requires review from more people.
More context
I am unsure to whether this could be related to this issue https://github.com/hashgraph/guardian/issues/3450, as the lock up of the system could be related to a invalid access, token that is in an infinite loop state.
It could be more appropriate to examine if there are any other authentication libraries that are more industry standard in terms of sec and auth, or perhaps we will reach a state where email/pass auth isn't even required for individual guardian instances.
Hi @mattsmithies, thank you for the detailed description - much appreciated!
Currently Guardian internals do not support multiple simultaneous user sessions, as this can result (if such a user is not being careful) in complex workflow issues. So this session 'kick out' functionality is there by design, however what is clear from your description is that it is currently poorly done. Guardian should indicate to the user that they have been disconnected because another session was established, and redirected to the login page. We will address this as a matter of urgency.
It is definitely possible to make Guardian workflow engine multi-session safe if there is a business need for it. This would be a project, and thus needs to be discussed and prioritised as per the usual procedure.
Maybe I am misremembering, but Guardian never allowed multiple simultaneous user sessions anyway? We also came across this behaviour while doing some manual testing (as our application uses Guardian API exclusively but we sometimes check the UI); I agree that the user should be made aware that they have been disconnected and redirected to the login page.
