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.

Sign up for GitHub

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

Jump to bottom

Do not lose authentication token when the connection gets lost #369

Merged
merged 1 commit into from
Jun 19, 2016
Merged

Do not lose authentication token when the connection gets lost #369

merged 1 commit into from
Jun 19, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented May 31, 2016

No description provided.

@xPaw xPaw added the Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors. label May 31, 2016
@xPaw xPaw added this to the 2.0.0 milestone May 31, 2016
xPaw
xPaw reviewed May 31, 2016
View reviewed changes
client/js/lounge.js
login.find(".btn").prop("disabled", false);
var token = window.localStorage.getItem("token");
if (token) {
window.localStorage.removeItem("token");
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual offender. I chose to never delete the token even on failed auths to show the "authorisation failed" message to the user until they relogin.

@maxpoulin64
Copy link
Member

Looks all good to me! 👍

@astorije astorije self-assigned this Jun 6, 2016
@astorije
Copy link
Member

Testing this, I hit a weird bug. To reproduce:

  • Start the server and go to the login page
  • Enter the wrong password and submit
  • An "Authentication failed" message appears. Reload the page and it disappears.
  • Enter the right password and submit
  • Restart the server and go to the login page
  • The "Authentication failed" message appears again, even if not submitting anything. Reloading the page again and again will keep the message.

I am going to review #370 instead of looking further into this. I don't think it's worth fixing this in an isolated way considering it has to do with restarting the server.

@xPaw
Copy link
Member Author

xPaw commented Jun 13, 2016 •
edited
Loading

@astorije That was actually intentional, I mentioned it in IRC. However considering it keeps doing that after entering a wrong password, I should remove the token when that happens.

EDIT: It will remove stored token on a failed authentication.

@astorije
Copy link
Member

So, I definitely hit the bug many times on mobile, but I cannot consistently reproduce it on laptop right now. That being said, code looks fine, @maxpoulin64 said he tested a bit more than just reading the code, and I tested that previous behavior didn't happen (i.e. that we have iso-functionality with what we had before PR) so I'm 👍.
This is definitely a fix that will be live-tested in a prerelease. Good job @xPaw, it was an annoying bug!

@astorije astorije merged commit 75c578c into master Jun 19, 2016
@astorije astorije deleted the xpaw/fix-losing-auth branch June 19, 2016 02:35
matburnham pushed a commit to matburnham/lounge that referenced this pull request Sep 6, 2017
@astorije
Merge pull request thelounge#369 from thelounge/xpaw/fix-losing-auth
6458427 
Do not lose authentication token when the connection gets lost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@astorije astorije

@maxpoulin64 maxpoulin64

Labels
Type: Bug Issues that report and PRs that solve any defects that cause unexpected behaviors.
Projects
None yet
Milestone
2.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@xPaw @maxpoulin64 @astorije