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

frontend password change functionality #57

Merged
merged 1 commit into from
Feb 28, 2016
Merged

frontend password change functionality #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions client/css/style.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1116,6 +1116,19 @@ button,
line-height: 1.8;
}

#settings #change-password .error,
#settings #change-password .success {
margin-bottom: 1em;
}

#settings #change-password .error {
color: #e74c3c;
}

#settings #change-password .success {
color: #2ecc40;
Copy link
Member

Choose a reason for hiding this comment

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

  • Why the margin-bottom for the error but not the success message? It doesn't seem necessary for none I think, but I can be wrong.
  • .error.success? :-)

Copy link
Author

Choose a reason for hiding this comment

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

to the first point, why a margin-bottom on .error but not .error.success: the second rule will inherit from the first so there is no reason to override the margin-bottom with an identical value when it is already applied by inheritance.

.error.success: that's a semantic issue I guess, but we can rename .error if need be.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it seems very messy to have elements that are both .error and .success. You should use one or the other, and if you do need a common class, you could call it .feedback. If it's just for common CSS properties though, .error, .success { /* ... */ } will do just fine.

}

#form {
background: #eee;
border-top: 1px solid #ddd;
Expand Down
25 changes: 25 additions & 0 deletions client/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -265,6 +265,31 @@ <h2>Notifications</h2>
Enable notification for all messages
</label>
</div>
<% if (!public) { %>
<div id="change-password">
<form action="" method="post">
<div class="col-sm-12">
<h2>Change password</h2>
</div>
<div class="col-sm-12">
<label for="old_password" class="sr-only">Enter current password</label>
<input type="password" name="old_password" class="input" placeholder="Enter current password">
</div>
<div class="col-sm-12">
<label for="new_password" class="sr-only">Enter desired new password</label>
<input type="password" name="new_password" class="input" placeholder="Enter desired new password">
</div>
<div class="col-sm-12">
<label for="verify_password" class="sr-only">Repeat new password</label>
<input type="password" name="verify_password" class="input" placeholder="Repeat new password">
</div>
<div class="col-sm-12 feedback"></div>
<div class="col-sm-12">
<button type="submit" class="btn">Change password</button>
</div>
</form>
</div>
<% } %>
<div class="col-sm-12">
<h2>About The Lounge</h2>
</div>
Expand Down
29 changes: 28 additions & 1 deletion client/js/lounge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,31 @@ $(function() {
.show();
});

socket.on("change-password", function(data) {
var passwordForm = $("#change-password");
if (data.error || data.success) {
var message = data.success ? data.success : data.error;
var feedback = passwordForm.find(".feedback");

if (data.success) {
feedback.addClass("success").removeClass("error");
} else {
feedback.addClass("error").removeClass("success");
}

feedback.text(message).show();
feedback.closest("form").one("submit", function() {
feedback.hide();
});
}
passwordForm
.find("input")
.val("")
.end()
.find(".btn")
.prop("disabled", false);
});

socket.on("init", function(data) {
if (data.networks.length === 0) {
$("#footer").find(".connect").trigger("click");
Expand Down Expand Up @@ -734,7 +759,7 @@ $(function() {
});

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

Choose a reason for hiding this comment

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

[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.

Copy link
Author

Choose a reason for hiding this comment

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

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?


windows.on("show", "#sign-in", function() {
var self = $(this);
Expand All @@ -761,6 +786,8 @@ $(function() {
.end();
if (form.closest(".window").attr("id") === "connect") {
event = "conn";
} else if (form.closest("div").attr("id") === "change-password") {
event = "change-password";
}
var values = {};
$.each(form.serializeArray(), function(i, obj) {
Expand Down
51 changes: 20 additions & 31 deletions src/client.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
var _ = require("lodash");
var Chan = require("./models/chan");
var crypto = require("crypto");
var fs = require("fs");
var identd = require("./identd");
var log = require("./log");
var net = require("net");
Expand Down Expand Up @@ -51,14 +50,15 @@ var inputs = [
"whois"
];

function Client(sockets, name, config) {
function Client(manager, name, config) {
_.merge(this, {
activeChannel: -1,
config: config,
id: id++,
name: name,
networks: [],
sockets: sockets
sockets: manager.sockets,
manager: manager
});
var client = this;
crypto.randomBytes(48, function(err, buf) {
Expand Down Expand Up @@ -221,6 +221,21 @@ Client.prototype.connect = function(args) {
});
};

Client.prototype.setPassword = function(hash) {
var client = this;
client.manager.updateUser(client.name, {password:hash});
// re-read the hash off disk to ensure we use whatever is saved. this will
// prevent situations where the password failed to save properly and so
// a restart of the server would forget the change and use the old
// password again.
var user = client.manager.readUserConfig(client.name);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I understand that line and comment: if the password was not saved properly (so, not saved), why do we need to re-read the config, since the password will be unchanged? Also, doesn't restarting the server trigger a new config file read?

Copy link
Author

Choose a reason for hiding this comment

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

my thinking was to reload the entire user object from disk rather than just tweak the in-memory so that we are verified to be using the canonical configuration rather than a version which might be different to what's saved to disk.

Copy link
Member

Choose a reason for hiding this comment

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

I see, I'm good with that for now :-)

if (user.password === hash) {
client.config.password = hash;
return true;
}
return false;
};

Client.prototype.input = function(data) {
var client = this;
var text = data.text.trim();
Expand Down Expand Up @@ -353,9 +368,6 @@ Client.prototype.save = function(force) {
return;
}

var name = this.name;
var path = Helper.HOME + "/users/" + name + ".json";

var networks = _.map(
this.networks,
function(n) {
Expand All @@ -364,29 +376,6 @@ Client.prototype.save = function(force) {
);

var json = {};
fs.readFile(path, "utf-8", function(err, data) {
if (err) {
console.log(err);
return;
}

try {
json = JSON.parse(data);
json.networks = networks;
} catch (e) {
console.log(e);
return;
}

fs.writeFile(
path,
JSON.stringify(json, null, " "),
{mode: "0777"},
function(err) {
if (err) {
console.log(err);
}
}
);
});
json.networks = networks;
client.manager.updateUser(client.name, json);
};
52 changes: 46 additions & 6 deletions src/clientManager.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,18 +29,14 @@ ClientManager.prototype.loadUsers = function() {

ClientManager.prototype.loadUser = function(name) {
try {
var json = fs.readFileSync(
Helper.HOME + "/users/" + name + ".json",
"utf-8"
);
json = JSON.parse(json);
var json = this.readUserConfig(name);
} catch (e) {
console.log(e);
return;
}
if (!this.findClient(name)) {
this.clients.push(new Client(
this.sockets,
this,
name,
json
));
Expand Down Expand Up @@ -93,6 +89,50 @@ ClientManager.prototype.addUser = function(name, password) {
return true;
};

ClientManager.prototype.updateUser = function(name, opts) {
var users = this.getUsers();
if (users.indexOf(name) === -1) {
return false;
}
if (typeof opts === "undefined") {
return false;
}
var path = Helper.HOME + "/users/" + name + ".json";
var user = {};

try {
user = this.readUserConfig(name);
_.merge(user, opts);
} catch (e) {
console.log(e);
return;
}

fs.writeFileSync(
path,
JSON.stringify(user, null, " "),
{mode: "0777"},
Copy link
Member

Choose a reason for hiding this comment

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

[Off-topic of this PR]
I never noticed the file was written with mode 0777 before, it's pretty sad...

Copy link
Member

Choose a reason for hiding this comment

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

We should have 0600 or at least 0644 here, but that's a thing to fix in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ouch, that's bad...

function(err) {
if (err) {
console.log(err);
}
}
);
return true;
};

ClientManager.prototype.readUserConfig = function(name) {
var users = this.getUsers();
if (users.indexOf(name) === -1) {
return false;
}
var path = Helper.HOME + "/users/" + name + ".json";
var user = {};
var data = fs.readFileSync(path, "utf-8");
user = JSON.parse(data);
return user;
};

ClientManager.prototype.removeUser = function(name) {
var users = this.getUsers();
if (users.indexOf(name) === -1) {
Expand Down
45 changes: 45 additions & 0 deletions src/server.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,51 @@ function init(socket, client, token) {
client.connect(data);
}
);
if (!config.public) {
socket.on(
"change-password",
function(data) {
var old = data.old_password;
var p1 = data.new_password;
var p2 = data.verify_password;
if (typeof old === "undefined" || old === "") {
socket.emit("change-password", {
error: "Please enter your current password"
});
return;
}
if (typeof p1 === "undefined" || p1 === "") {
socket.emit("change-password", {
error: "Please enter a new password"
});
return;
}
if (p1 !== p2) {
socket.emit("change-password", {
error: "Both new password fields must match"
});
return;
}
if (!bcrypt.compareSync(old || "", client.config.password)) {
socket.emit("change-password", {
error: "The current password field does not match your account password"
});
return;
}
var salt = bcrypt.genSaltSync(8);
var hash = bcrypt.hashSync(p1, salt);
if (client.setPassword(hash)) {
socket.emit("change-password", {
success: "Successfully updated your password"
});
return;
}
socket.emit("change-password", {
error: "Failed to update your password"
});
}
);
}
socket.on(
"open",
function(data) {
Expand Down