frontend password change functionality #57

lucyllewy · 2016-02-17T00:25:58Z

refactor clientManager.js to allow configuration parsing as a serparate
function.
refactor clientManager.js to add configuration writing function.
add new plugin for /setpass command.
add server.js changes to allow for new "profile" screen
add password change ui to "profile" screen
modify css, front-end shout.js and index.html to add "profile" ui
refactor client.js to use new clientManager functionality for saving
the configuration files

duplicates erming/shout#506

astorije · 2016-02-17T05:10:41Z

Hey @diddledan, thanks for this! (Cc @JocelynDelalande as he started reviewing this on Shout)

However, I see that maaaassive diff, I'm a bit scared considering the feature. In particular:

Is there a specific reason why you need to upgrade Bootstrap from 3.2 to 3.5?
I believe the client/index.html.orig file was committed here by accident. Could you confirm and remove please?

I believe once we clean this up a bit it will be much easier to review :-)

xPaw · 2016-02-17T08:30:51Z

client/index.html

+                                <input class="input" type="password" name="old_password" placeholder="Enter current password">
+                            </div>
+                            <div class="col-sm-12">
+                                <label aria-label="Enter desired new password">


Mixed spaces, use tabs.

JocelynDelalande · 2016-02-17T08:45:07Z

Thanks for careful review (I have to admit reviewing latest commits, I focused on UI), I got my +1 a bit too early on shout project, there is still work to be done, obviously

@diddledan be brave ;-)

lucyllewy · 2016-02-17T16:10:07Z

the replaced/upgraded bootstrap was done to add alert-box functionality which was missing from the previous bundle.

xPaw · 2016-02-17T16:14:10Z

Updating bootstrap shouldn't be part of your PR. As for alerts, I'm not fond of how alerts look in bootstrap, maybe using a separate library for the task would be better?

http://kamranahmed.info/toast
https://t4t5.github.io/sweetalert/

Having some alerts like that will come in handy later on when we will handle IRC errors and disconnections (e.g. show an alert when you get disconnected from a network)

JocelynDelalande · 2016-02-17T18:48:07Z

Updating bootstrap shouldn't be part of your PR.

Depends. As it's motivated, I would be okay with that update to be part of that PR, what do others think ?

BS is (trying to) follow semver, so this version jump is supposed to be backward compatible.

As for alerts, I'm not fond of how alerts look in bootstrap, maybe using a separate library for the task would be better?

http://kamranahmed.info/toast
https://t4t5.github.io/sweetalert/

Having some alerts like that will come in handy later on when we will handle IRC errors and disconnections (e.g. show an alert when you get disconnected from a network)

BS alerts are fine IMHO (I didn't said it's the best), it will still be time to rework alerts later, let's not make things too hard for this PR or it may never make it to master.

xPaw · 2016-02-17T18:52:45Z

Depends. As it's motivated, I would be okay with that update to be part of that PR, what do others think ?

In that case, can we get alerts added to the current bootstrap version to use? Would make for a simpler diff.

MaxLeiter · 2016-02-17T19:15:00Z

I think it should be its own PR, or at least a separate commit

astorije · 2016-02-18T04:11:46Z

First of all, @diddledan, this is great. I do believe it is not ready at the moment, but I am strongly in favor and will make sure we can get this to a mergeable state.
What is really cool is that the feature itself works, I can indeed change my password from the UI, which is great. This means that while we are striving to make this PR as awesome as we can, people can already run it locally if it's an absolute necessity for their client! And for that I say 👏 👏

Depends. As it's motivated, I would be okay with that update to be part of that PR, what do others think ?

I do not agree with this. Such an upgrade requires testing and therefore should be isolated. As a matter of fact, I noticed the following bug:

Before	After

This bug might not be the upgrade itself, but again by cumulating upgrades, refactors and features, it's very hard for us (at least for me) to see what's wrong in a PR with 1200+ changes.

And while this PR does upgrade Bootstrap from 3.2 to 3.5, as @xPaw stated, it also adds components that were removed using Bootstrap's customized builds, making things a bit harder.

As a matter of fact, if the extra 1k changes are simply to add that dismissible alert-box to display success/failure, I'd say we are doing something wrong.
My proposal is the following:

Let's use what we already have accessible in Bootstrap or the custom CSS or whatever so that gets in quickly. At the very least, if nothing can work, we can use a JS alert().
In the meantime, we design a notification/alert/flash/whatever-it's-called feature that looks more appropriate for the current design, that can be modified from a theme and that can be re-used by other parts of the project (this is going to be a crucial point in the package system, as such flash container will have its own API there).
Then we can update that part of the code when it's ready.
As @xPaw said, this is not a feature people are going to use every day, so a JS alert(), while definitely ugly and a temporary solution-until-better, does not throw me off. Again, we did not need such feature before, and we are going to reuse it, so let's take the time to design it right.
What do you think?

@diddledan, a few other things:

I noticed the emojis are still there, either because you didn't remove them all in the code, or because you didn't submit the updated grunted templates:
This PR also affects tooltips, that do not get hidden well anymore (until a click is emitted somewhere):
I am not in favor of adding a "My Profile" element to the menu. There are not many settings at the moment, they need to be designed (not in terms of UI but more broadly) and there is just one element in the profile at the moment. Can you move the password change to the settings instead please? Also, the user icon is already used to display a nick change for now.

That's all I got for now, but I will come back with more once we get to a smaller diff.

Keep up the good work, that's a great start!

JocelynDelalande · 2016-02-18T08:57:06Z

Thx to everyone for involvement :-)

@astorije showed that bootstrap adhesion to semver rule "no breaking change between two minors" is quite loose, so better be cautious.

As a matter of fact, if the extra 1k changes are simply to add that dismissible alert-box to display success/failure, I'd say we are doing something wrong.
My proposal is the following:
Let's use what we already have accessible in Bootstrap or the custom CSS or whatever so that gets in quickly. At the very least, if nothing can work, we can use a JS alert().

It turns out that there is no need to update bootstrap to get the alerts, it seems to have been present for a while.

Not to over-grow the scope of the PR, nor introduce breaking changes, I'd suggest the following :

remove the BS upgrade
modify the BS customized build as @xPaw suggested (better in a dedicated commit).

@xPaw @astorije @diddledan ok with that ?

In the meantime, we design a notification/alert/flash/whatever-it's-called feature that looks more appropriate for the current design, that can be modified from a theme and that can be re-used by other parts of the project (this is going to be a crucial point in the package system, as such flash container will have its own API there).
Then we can update that part of the code when it's ready. As @xPaw said, this is not a feature people are going to use every day, so a JS alert(), while definitely ugly and a temporary solution-until-better, does not throw me off. Again, we did not need such feature before, and we are going to reuse it, so let's take the time to design it right. What do you think?

I agree with your 2 stages approach : doing "best-effort" with what we have here, keeping it simple, and designing propper notification later, with global thinking.

JocelynDelalande · 2016-02-18T09:07:20Z

I am not in favor of adding a "My Profile" element to the menu. There are not many settings at the moment, they need to be designed (not in terms of UI but more broadly) and there is just one element in the profile at the moment. Can you move the password change to the settings instead please? Also, the user icon is already used to display a nick change for now.

Not strongly opinionated on this point. But I'm ok with the proposal to slip it into settings if others are, as it's a common practice, I think users will find it.

astorije · 2016-02-18T14:50:06Z

It turns out that there is no need to update bootstrap to get the alerts, it seems to have been present for a while.

I understand alerts have been removed for this very version of Bootstrap 3.2 we are shipping, using the customized build form I have mentioned earlier, which is why @diddledan added them, and which is why I am suggesting a dumb alert() at the moment.

modify the BS customized build as @xPaw suggested (better in a dedicated commit).
@xPaw @astorije @diddledan ok with that ?

I am not ok with this. As said, it's a bigger change that will impact the rest of the features and define some new UI, so it should not be decided as a side-effect of this. Really the alert() way, while the least pretty here, is the simplest and most straightforward/cross-browser solution atm. I am more than ok discussing this right after, but not blindly switching to Bootstrap default alerts without discussion/tests/mockups.

JocelynDelalande · 2016-02-18T15:07:24Z

I understand alerts have been removed for this very version of Bootstrap 3.2 we are shipping, using the customized build form I have mentioned earlier, which is why @diddledan added them, and which is why I am suggesting a dumb alert() at the moment.

Hm I think there is something I don't understand, let's state what I understand, and stop me where I'm wrong :

We are using a slimmed down custom build of bootstrap 3.2
This custom build doesn't include alert features
Can't we just add the alert feature to the custom build, staying at version 3.2 ?

astorije · 2016-02-18T15:27:43Z

We are using a slimmed down custom build of bootstrap 3.2
This custom build doesn't include alert features

Correct.

Can't we just add the alert feature to the custom build, staying at version 3.2 ?

We can, but should not as stated above. alert() is the way to go for this very PR.

lucyllewy · 2016-02-18T16:55:57Z

I've reverted the bootstrap, replaced the alerts with the window.alert() function, and moved the ui into settings thereby removing the "profile" page altogether. (the smilie faces should be gone now too!)

xPaw · 2016-02-18T17:03:54Z

client/index.html

@@ -265,6 +265,36 @@ <h1 class="title">Settings</h1>
 									Enable notification for all messages
 								</label>
 							</div>
+							<% if (!public) { %>
+							<div id="change-password">
+								<form action="">


Add method="post"

astorije · 2016-02-18T17:09:32Z

I've reverted the bootstrap, replaced the alerts with the window.alert() function, and moved the ui into settings thereby removing the "profile" page altogether. (the smilie faces should be gone now too!)

Now that's a much nicer diff, I'll do a review when I am outside of work :-)

... Buuuuuut you are going to hate me if not already :-)
I just remember that we already had a feedback for a form (only negative, red feedbak for now, so you could reuse one of the green colors from the UI, if there is no colors set for positive feedback)! It's faaaar from perfect and definitely needs thinking and design, but it's there, and using the same mechanism would allow for more consistency:

Again, will make a bigger review later :-)

JocelynDelalande · 2016-02-18T17:35:02Z

client/index.html

@@ -1,4 +1,4 @@
-<!doctype html>
+<!doctype html>


what changed exactly here ? is it github tripping ?

it looks like I accidentally saved the file with a UTF-8 BOM identifier. I'll try to remove it in a mo

xPaw · 2016-02-21T10:28:28Z

src/client.js

@@ -46,18 +45,20 @@ var inputs = [
 	"quit",
 	"raw",
 	"services",
+	"setpass",


Did you forget to remove this?

I did. oops :-)

xPaw · 2016-02-21T10:34:46Z

Here's a question: Should the token be re-generated when the password is changed? This would invalidate all sessions, and require them to re-login (re-send new token to current session) with the new password the next time they try to use your account.

astorije · 2016-02-22T05:33:35Z

@xPaw, while I fully agree with you, here are my 2 cents:

I believe this PR should make it to master without this addition, that we can add later on.
I notice this is not the first time you mention something related to the session token. Do you think it would be worth relying on a third-party library for user management, like passport or anything else (if so, we need to list pros/cons to decide)? It seems to do gazillion things we do not need, but I thinking overall. Maybe I am completely wrong, or maybe this is not something we can do short-term, but I'd be happy to have your opinion.

@diddledan, would you mind rebasing with master and fixing what was given in comments (CSS class, labels and setpass) so we can proceed with the reviews? :-)

Thanks a lot for this, and hang in there, it's getting super close!

xPaw · 2016-02-22T10:07:57Z

I notice this is not the first time you mention something related to the session token. Do you think it would be worth relying on a third-party library for user management, like passport or anything else (if so, we need to list pros/cons to decide)? It seems to do gazillion things we do not need, but I thinking overall. Maybe I am completely wrong, or maybe this is not something we can do short-term, but I'd be happy to have your opinion.

That might be worth looking into.

lucyllewy · 2016-02-22T12:59:12Z

I believe I've fixed all the issues most-recently raised (sr-only labels, extra "setpass" autocomplete reference now removed, and class names/styles) I've also rebased against master

astorije · 2016-02-23T05:51:08Z

client/js/lounge.js

@@ -734,7 +759,7 @@ $(function() {
 	});

 	var windows = $("#windows");
-	var forms = $("#sign-in, #connect");
+	var forms = $("#sign-in, #connect, #change-password");


[Off-topic of this PR]
I have no idea what's going on here, but this seems fishy. We might want to look into it and its usefulness when we clean up that file.

AFAICT this is used to wire-up the on-submit event handlers on each of the three elements with those IDs, which happen to be <form> elements (the elements are selected separately as there's commas between each ID, and therefore will return a JQuery object referencing each of the elements with IDs sign-in, connect, and change-password). The event handler we wire-up onto this resultant set of elements calls event.preventDefault() which stops the form submittal from reloading the page. The form is submitted through the websocket or XHR instead?

astorije · 2016-02-23T06:03:30Z

Hi @diddledan,

When I'm trying to change the password now, I get no success or error message, could you take a look? :-/
Also, Travis CI breaks on CSS linting, do you mind fixing this too?

Not that it will hold off my +1, but I left you a comment in Client.prototype.setPassword, trying to fully understand things.

I should be all set after that :-)

astorije · 2016-02-23T06:04:05Z

@YaManicKill, do you mind taking second review after @diddledan has fixed what's currently broken?

AlMcKinlay · 2016-02-23T08:58:45Z

Yeah, I'll take a look in the next day or 2.

astorije · 2016-02-23T23:51:43Z

Ok, then I think the next minor release (maybe tonight if I have time) will get out without this then. Hopefully @diddledan gets some spare time to put the final touch to this before the next next minor release gets out :-)

- refactor clientManager.js to allow configuration parsing as a serparate function. - refactor clientManager.js to add configuration writing function. - add server.js changes to allow for new password-change functionality - add password change ui to "settings" screen - refactor client.js to use new clientManager functionality for saving the configuration files

astorije · 2016-02-27T23:24:46Z

Alright, excellent work @diddledan, I am finally 👍
👏

Off to a second review now, I don't think it should be blocking as other folks have already taken a look at it!

xPaw · 2016-02-27T23:33:21Z

Looks fine to me code wise.

maxpoulin64 · 2016-02-28T01:07:02Z

Yep, I think we finally can merge this one. 👍 👏

frontend password change functionality

JocelynDelalande · 2016-03-01T21:07:46Z

Thaaaat is nice, thanks everyone :-), and congrats @diddledan !

lucyllewy mentioned this pull request Feb 17, 2016

front-end-password-change capability erming/shout#506

Open

astorije added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Feb 17, 2016

astorije self-assigned this Feb 17, 2016

astorije added the Type: Security Security concern or PRs that must be reviewed with extra care regarding security. label Feb 17, 2016

xPaw reviewed Feb 17, 2016
View reviewed changes

lucyllewy force-pushed the frontend-password-change branch from 237a0e8 to f327a4f Compare February 17, 2016 16:04

lucyllewy force-pushed the frontend-password-change branch from f327a4f to c3d99a1 Compare February 18, 2016 16:54

xPaw reviewed Feb 18, 2016
View reviewed changes

JocelynDelalande reviewed Feb 18, 2016
View reviewed changes

lucyllewy force-pushed the frontend-password-change branch from c3d99a1 to e370bdd Compare February 18, 2016 18:08

xPaw reviewed Feb 21, 2016
View reviewed changes

astorije mentioned this pull request Feb 22, 2016

[Discussion] Towards better settings (and per-channel settings) #85

Open

lucyllewy force-pushed the frontend-password-change branch from 94f1ac6 to 9d6a262 Compare February 22, 2016 12:58

astorije reviewed Feb 23, 2016
View reviewed changes

lucyllewy force-pushed the frontend-password-change branch from 9d6a262 to b79a918 Compare February 26, 2016 18:38

astorije added the second review needed label Feb 27, 2016

maxpoulin64 added a commit that referenced this pull request Feb 28, 2016

Merge pull request #57 from diddledan/frontend-password-change

1d8667e

frontend password change functionality

maxpoulin64 merged commit 1d8667e into thelounge:master Feb 28, 2016

maxpoulin64 removed the second review needed label Feb 28, 2016

astorije mentioned this pull request Feb 29, 2016

Fix complete crash when refreshing a public instance #125

Merged

maxpoulin64 mentioned this pull request Mar 8, 2016

Auto complete fix for nicks #168

Closed

maxpoulin64 mentioned this pull request Mar 20, 2016

Saving user config merges current config in #211

Closed

xPaw mentioned this pull request Apr 2, 2016

Fix not being able to remove networks from user config #233

Merged

astorije mentioned this pull request Jul 6, 2016

Passwords as a per-user thing, not a per-server thing. erming/shout#677

Closed

MaxLeiter mentioned this pull request Oct 3, 2016

Change password in web interface erming/shout#359

Closed

astorije added this to the 1.3.0 milestone Apr 1, 2017

frontend password change functionality #57

frontend password change functionality #57

Conversation

lucyllewy commented Feb 17, 2016

astorije commented Feb 17, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JocelynDelalande commented Feb 17, 2016

lucyllewy commented Feb 17, 2016

xPaw commented Feb 17, 2016

JocelynDelalande commented Feb 17, 2016

xPaw commented Feb 17, 2016

MaxLeiter commented Feb 17, 2016

astorije commented Feb 18, 2016

JocelynDelalande commented Feb 18, 2016

JocelynDelalande commented Feb 18, 2016

astorije commented Feb 18, 2016

JocelynDelalande commented Feb 18, 2016

astorije commented Feb 18, 2016

lucyllewy commented Feb 18, 2016

Choose a reason for hiding this comment

astorije commented Feb 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xPaw commented Feb 21, 2016

astorije commented Feb 22, 2016

xPaw commented Feb 22, 2016

lucyllewy commented Feb 22, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astorije commented Feb 23, 2016

astorije commented Feb 23, 2016

AlMcKinlay commented Feb 23, 2016

astorije commented Feb 23, 2016

astorije commented Feb 27, 2016

xPaw commented Feb 27, 2016

maxpoulin64 commented Feb 28, 2016

JocelynDelalande commented Mar 1, 2016