Make usernames case-insensitive when logging in

[ashwinikammar]

Fixes #2943

What is the fix:
For a new user, a profile will be created in thelounge and every time the same profile will be loaded irrespective of the username case.
For an existing user, one of his/her profile will be loaded irrespective of the username case. Further, duplicate profiles of the user will be removed.

[xPaw]

Is LDAP case-insensitive? Why only fix it for LDAP, and not in The Lounge overall?

My proposed fix would be to simply lowercase searching here:

thelounge/src/clientManager.js

Lines 36 to 38 in 9bb0b02

	ClientManager.prototype.findClient = function (name) {
	return this.clients.find((u) => u.name === name);
	};

That way it works outside of LDAP, and does not require a config option.

[xPaw]

Can you undo the signin/config change and squash please?

[xPaw]

@McInkay what do you reckon about this? It allows the files on disk to stay in whatever capitalization, but login will be case-insensitive.

Unsure how we should (if we even need to) handle same usernames with different casing on case-sensitive file systems.

[ashwinikammar]

Hi @xPaw, Can you please have a look at the latest changes

[xPaw]

Yeah that looks fine, just need to give it some testing (see my comment above).

Did you verify that this works on LDAP?

[ashwinikammar]

Yes I have tested with LDAP as well, it works fine

[AlMcKinlay]

what do you reckon about this? It allows the files on disk to stay in whatever capitalization, but login will be case-insensitive.

Sounds generally sensible to me.

Unsure how we should (if we even need to) handle same usernames with different casing on case-sensitive file systems.

Maybe we need to add a check to startup to see if there are any duplicated names just with case sensitivity differences.

We can deprecate this until the next major version, just skipping this code for now if we find one, and just refusing to start up from the next major version? I suspect this won't affect many people, but we probably need to do it properly.

[xPaw]

We can do a loop after this code to find any duplicates that are case insensitive here:

thelounge/src/clientManager.js

Lines 41 to 43 in 3194777

	const users = this.getUsers();
	if (users.length === 0) {

We can deprecate this until the next major version, just skipping this code for now if we find one

I say we just print an error for user that has duplicates and not load it (remove the duplicate entry from users array).

[ashwinikammar]

Hi @xPaw ,
Do you want me to proceed with the approach you mentioned?

[xPaw]

Should be fine.

[xPaw]

Double loop may get more expensive than it needs to be.

You can use a Set to avoid the inner loop entirely. Like so:

Lowercase the name and check whether its in that set, if it is, then remove it from the user list and print a message, otherwise add it to set and continue.

If you loop from the end, then you can splice out the user from the array while iterating.

[ashwinikammar]

hi @xPaw
I have avoided using for loop in my latest change. Please have a look
Also, it would be helpful if you can suggest the changes to solve the build error for node 12.x on windows latest.

[xPaw]

The updated code still essentially loops multiple times, just in a functional manner.

Here's a diff of what I had in mind, could you apply that to your PR (and squash it so its a single commit).

diff --git a/src/clientManager.js b/src/clientManager.js
index ca1f55b6..942d31ff 100644
--- a/src/clientManager.js
+++ b/src/clientManager.js
@@ -34,18 +34,39 @@ ClientManager.prototype.init = function (identHandler, sockets) {
 };
 
 ClientManager.prototype.findClient = function (name) {
-	return this.clients.find((u) => u.name === name);
+	name = name.toLowerCase();
+	return this.clients.find((u) => u.name.toLowerCase() === name);
 };
 
 ClientManager.prototype.loadUsers = function () {
-	const users = this.getUsers();
+	let users = this.getUsers();
 
 	if (users.length === 0) {
 		log.info(
 			`There are currently no users. Create one with ${colors.bold("thelounge add <name>")}.`
 		);
+		return;
 	}
 
+	const alreadySeenUsers = new Set();
+	users = users.filter((user) => {
+		user = user.toLowerCase();
+
+		if (alreadySeenUsers.has(user)) {
+			log.error(
+				`There is more than one user named "${colors.bold(
+					user
+				)}". Usernames are now case insensitive, duplicate users will not load.`
+			);
+
+			return false;
+		}
+
+		alreadySeenUsers.add(user);
+
+		return true;
+	});
+
 	// This callback is used by Auth plugins to load users they deem acceptable
 	const callbackLoadUser = (user) => {
 		this.loadUser(user);

Also, it would be helpful if you can suggest the changes to solve the build error for node 12.x on windows latest.

Looks like a temporary issue.

[xPaw]

Thanks for applying the change.

I also thought that we need to make sure user is a string before calling findClient, can you apply this too:

diff --git a/src/server.js b/src/server.js
index ba1ab417..699cabb8 100644
--- a/src/server.js
+++ b/src/server.js
@@ -803,6 +803,10 @@ function performAuthentication(data) {
 		return;
 	}
 
+	if (typeof data.user !== "string") {
+		return;
+	}
+
 	const authCallback = (success) => {
 		// Authorization failed
 		if (!success) {

Could you please squash your commits into one after doing so? https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

Sorry for the trouble.

[ashwinikammar]

Sure, i'll add that check and squash all commits into one.

[ashwinikammar]

After adding below changes, users with the integers as username can also login because the numbers sent through input fields are considered as strings. Any thoughts on this?

Thanks for applying the change.

I also thought that we need to make sure user is a string before calling findClient, can you apply this too:

diff --git a/src/server.js b/src/server.js
index ba1ab417..699cabb8 100644
--- a/src/server.js
+++ b/src/server.js
@@ -803,6 +803,10 @@ function performAuthentication(data) {
 		return;
 	}
 
+	if (typeof data.user !== "string") {
+		return;
+	}
+
 	const authCallback = (success) => {
 		// Authorization failed
 		if (!success) {

Could you please squash your commits into one after doing so? https://stackoverflow.com/questions/5189560/squash-my-last-x-commits-together-using-git

Sorry for the trouble.

[xPaw]

After adding below changes, users with the integers as username can also login because the numbers sent through input fields are considered as strings.

Sure, numeric usernames are valid. And the input field being sent through websocket would remain a string. That check is to verify we can call toLowerCase on it.

[ashwinikammar]

After adding below changes, users with the integers as username can also login because the numbers sent through input fields are considered as strings.

Sure, numeric usernames are valid. And the input field being sent through websocket would remain a string. That check is to verify we can call toLowerCase on it.

okay got it! thank you

[xPaw]

Looks good now, just need to give it some testing.