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

If the login creates a similar token we should just return the same token without an abort operation #2129

Closed
npalaska opened this issue Mar 4, 2021 · 4 comments · Fixed by #2144
Labels
API Of and relating to application programming interfaces to services and functions enhancement Server
Milestone

Comments

@npalaska
Copy link
Member

npalaska commented Mar 4, 2021

Right now we abort the operation if the similar token gets generated because of an instantaneous second login attempt which in reality is very unlikely to happen. However, if this to happen we should just return the same previous token instead of abort operation.

@npalaska npalaska added API Of and relating to application programming interfaces to services and functions enhancement Server labels Mar 4, 2021
@portante
Copy link
Member

Right now we abort the operation if the similar token gets generated because of an instantaneous second login attempt which in reality is very unlikely to happen.

What is the definition of "similar"? Is there some function that is comparing the tokens to determine similarity? Or do you mean identical.

However, if this to happen we should just return the same previous token instead of abort operation.

This sentence seems to indicate a case of where the generated tokens are the same. We need to make sure there is no way to generate the same token.

@npalaska
Copy link
Member Author

What is the definition of "similar"? Is there some function that is comparing the tokens to determine similarity? Or do you mean identical.

Yes Identical is the right word. I observed it happening during the unit tests randomly because of the instantaneous second login attempt. The tokens table in our database is defined in such a way that if it gets another token thats identical to the any of the stored tokens, it will raise the integrityError and we do check that in our code. However, if an already logged-in user tries to create another login token very instantaneously, we compare the new token to one provided in the authorization header, if they match we abort the request (since they are identical) However, we discussed that its a wrong work flow we should just return the same token instead of aborting the request in the case of logged in user.

However, if this to happen we should just return the same previous token instead of abort operation.

This sentence seems to indicate a case of where the generated tokens are the same. We need to make sure there is no way to generate the same token.

Yes indeed

@npalaska npalaska linked a pull request Mar 18, 2021 that will close this issue
@webbnh
Copy link
Member

webbnh commented Mar 19, 2021

However, if this to happen we should just return the same previous token instead of abort operation.

This sentence seems to indicate a case of where the generated tokens are the same. We need to make sure there is no way to generate the same token.

Yes indeed

If it is the same user in both cases, and if both tokens have the same expiration time, where is the problem in having the two tokens be identical, @portante?

@webbnh
Copy link
Member

webbnh commented Mar 19, 2021

Dave pointed out a potential problem where the second of two logged-in sessions which have the same token (by whatever happenstance) runs into problems when the first session logs out, which might happen during automated testing.

I don't think that this will be an issue in the case of real clients using the server, because tokens are expected/intended to be short-duration and will require periodic renewal during a session, so, if we implement logout as a token expiration, the second session will simply renew the token and continue as normal.

For automated testing, this is an edge case which we'll have to deal with. I think there are several options.

@portante portante removed the Backlog label Oct 30, 2021
@portante portante added this to the v0.71 milestone May 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API Of and relating to application programming interfaces to services and functions enhancement Server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants