-
Notifications
You must be signed in to change notification settings - Fork 687
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
Make sure users with wrong tokens are locked out instead of crashing the app #570
Conversation
I had this same issue, and just tested this out, and it looks like it is working. I'll let it bake for a day or so and comment if it does not fix the issue. |
@@ -286,10 +286,10 @@ function auth(data) { | |||
} | |||
} else { | |||
client = manager.findClient(data.user, data.token); | |||
var signedIn = data.token && data.token === client.config.token; | |||
var signedIn = data.token && client && data.token === client.config.token; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like an odd fix. How does this entire else
branch work if client
is not found?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you follow the rest of the code path in this case, when findClient
returns false it treats the user as failing his authentication.
There is one place in authFunction
(called by authFunction(client, data.user, data.password, authCallback);
) which does make use of client
, but it's wrapped around a try
/catch
so it's "safe".
I recognize that this is the cheapest solution to make this work though, do you have any suggestion to improve this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You actually removed this client
check here: cf64cb0#diff-e6a5b42b2f7a26c840607370aed5301a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whaaaat, why did I do that?!
Either way, that other change could have happened as well, I think...
Make sure users with wrong tokens are locked out instead of crashing the app
Fixes #569, see issue for more details.
That issue only appears when a wrong token stored in
localStorage
, so users could login fine in private browsing for example.@pugabear let me know if this fixes your issue.