From c7d21438df80eefc6c333648fea0769bdf7c00d4 Mon Sep 17 00:00:00 2001 From: Kit Westneat Date: Fri, 9 Mar 2018 10:15:19 -0500 Subject: [PATCH 1/2] checks for and mitigates username collisions with new external users --- services/users.js | 33 +++++++++++++++++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/services/users.js b/services/users.js index f5319122c..36ee38780 100644 --- a/services/users.js +++ b/services/users.js @@ -1,7 +1,7 @@ const uuid = require('uuid'); const bcrypt = require('bcryptjs'); const errors = require('../errors'); -const { some, merge } = require('lodash'); +const { some, merge, random } = require('lodash'); const { ROOT_URL } = require('../config'); const { jwt: JWT_SECRET } = require('../secrets'); const debug = require('debug')('talk:services:users'); @@ -346,6 +346,35 @@ class UsersService { return username.replace(/ /g, '_').replace(/[^a-zA-Z_]/g, ''); } + /** + * Creates the initial username for an external account. Searches to make + * sure username not already used. Adds a random number if username already + * in use. + */ + static async getInitialUsername(username) { + let MAX_ATTEMPTS = 10; + let END_NUMBER_MAX = 99999; + + let castedName = UsersService.castUsername(username); + let testName = castedName; + let existingUserWithName; + + for (let i = 0; i < MAX_ATTEMPTS; i++) { + existingUserWithName = await UserModel.findOne({ + lowercaseUsername: testName.toLowerCase(), + }); + + if (!existingUserWithName) { + return testName; + } + + let endNumber = random(0, END_NUMBER_MAX); + testName = castedName + '_' + endNumber; + } + + throw new Error('cannot find free name after ' + MAX_ATTEMPTS); + } + /** * Finds a user given a social profile and if the user does not exist, creates * them. @@ -368,7 +397,7 @@ class UsersService { // User does not exist and need to be created. // Create an initial username for the user. - let username = UsersService.castUsername(displayName); + let username = await UsersService.getInitialUsername(displayName); // The user was not found, lets create them! user = new UserModel({ From 1a8006e3a3194d65d8d252a28ec41c00daa4ada7 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 14 Mar 2018 14:55:24 -0600 Subject: [PATCH 2/2] improved db query support, added tests --- services/users.js | 71 ++++++++++++++++++++++++++--------- test/server/services/users.js | 13 +++++++ 2 files changed, 66 insertions(+), 18 deletions(-) diff --git a/services/users.js b/services/users.js index 36ee38780..aa488bbbc 100644 --- a/services/users.js +++ b/services/users.js @@ -1,7 +1,7 @@ const uuid = require('uuid'); const bcrypt = require('bcryptjs'); const errors = require('../errors'); -const { some, merge, random } = require('lodash'); +const { difference, sample, some, merge, random } = require('lodash'); const { ROOT_URL } = require('../config'); const { jwt: JWT_SECRET } = require('../secrets'); const debug = require('debug')('talk:services:users'); @@ -352,27 +352,62 @@ class UsersService { * in use. */ static async getInitialUsername(username) { - let MAX_ATTEMPTS = 10; - let END_NUMBER_MAX = 99999; + const MAX_ATTEMPTS = 10; + const END_NUMBER_MAX = 99999; + const GROUP_ATTEMPTS = 50; - let castedName = UsersService.castUsername(username); - let testName = castedName; - let existingUserWithName; + // Cast the original username. + const castedName = UsersService.castUsername(username); + const lowercaseUsername = castedName.toLowerCase(); - for (let i = 0; i < MAX_ATTEMPTS; i++) { - existingUserWithName = await UserModel.findOne({ - lowercaseUsername: testName.toLowerCase(), - }); - - if (!existingUserWithName) { - return testName; - } - - let endNumber = random(0, END_NUMBER_MAX); - testName = castedName + '_' + endNumber; + // Try to see if our first guess has been taken. + const existingUserWithName = await UserModel.findOne({ + lowercaseUsername, + }); + if (!existingUserWithName) { + return castedName; } - throw new Error('cannot find free name after ' + MAX_ATTEMPTS); + // Our first username was taken, lets try to find a non-taken name. + for (let i = 0; i < MAX_ATTEMPTS; i++) { + // Generate `GROUP_ATTEMPTS` guesses for the username. + const usernameGuesses = Array.from(Array(GROUP_ATTEMPTS)).map( + () => `${castedName}_${random(0, END_NUMBER_MAX)}` + ); + + // Map them all to lowercase. + const lowercaseUsernameGuesses = usernameGuesses.map(guess => + guess.toLowerCase() + ); + + // See if any of these users aren't taken already. + const existingUsernames = (await UserModel.find( + { + lowercaseUsername: { $in: lowercaseUsernameGuesses }, + }, + { lowercaseUsername: 1 } + )).map(({ lowercaseUsername }) => lowercaseUsername); + if (existingUsernames.length === lowercaseUsernameGuesses.length) { + // The number of found users is the same as the number of username + // guesses, aka, all the usernames are taken. + continue; + } + + // At least one of the usernames wasn't taken! Let's filter this to only + // include unused usernames and grab one random entry from the list. + const foundLowercaseUsernameIndex = lowercaseUsernameGuesses.indexOf( + sample(difference(lowercaseUsernameGuesses, existingUsernames)) + ); + + // Now we get the uppercase version of that string. + return usernameGuesses[foundLowercaseUsernameIndex]; + } + + throw new Error( + 'cannot find free name after ' + + (MAX_ATTEMPTS * GROUP_ATTEMPTS + 1) + + ' tries' + ); } /** diff --git a/test/server/services/users.js b/test/server/services/users.js index 1676f517b..06ba3813e 100644 --- a/test/server/services/users.js +++ b/test/server/services/users.js @@ -58,6 +58,19 @@ describe('services.UsersService', () => { }); }); + describe('#getInitialUsername', () => { + it('should find the first result when there is no conflict', async () => { + const username = await UsersService.getInitialUsername( + 'TheGreatSockmonster' + ); + expect(username).to.equal('TheGreatSockmonster'); + }); + it('should find a first result when there is a conflict', async () => { + const username = await UsersService.getInitialUsername('Sockmonster'); + expect(username).to.match(/Sockmonster_[0-9]+/); + }); + }); + describe('#findPublicByIdArray()', () => { it('should find an array of users from an array of ids', async () => { const ids = mockUsers.map(user => user.id);