From c7d21438df80eefc6c333648fea0769bdf7c00d4 Mon Sep 17 00:00:00 2001 From: Kit Westneat Date: Fri, 9 Mar 2018 10:15:19 -0500 Subject: [PATCH 1/8] 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 93e850ba3fd42e0aa963d1b901e2a80ab01ee80a Mon Sep 17 00:00:00 2001 From: Olly Dutton Date: Tue, 13 Mar 2018 12:16:59 +1100 Subject: [PATCH 2/8] Update UserDetailComment.js Ternary to display asset url if there's no asset title in admin comments --- client/coral-admin/src/components/UserDetailComment.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/coral-admin/src/components/UserDetailComment.js b/client/coral-admin/src/components/UserDetailComment.js index ba185a9df..931b4bdeb 100644 --- a/client/coral-admin/src/components/UserDetailComment.js +++ b/client/coral-admin/src/components/UserDetailComment.js @@ -76,7 +76,7 @@ class UserDetailComment extends React.Component {
- Story: {comment.asset.title} + Story: {comment.asset.title ? comment.asset.title : comment.asset.url} { {t('modqueue.moderate')} From bfb9ea1eea8823ec310ee218db4c4136739309c5 Mon Sep 17 00:00:00 2001 From: Olly Dutton Date: Tue, 13 Mar 2018 12:18:21 +1100 Subject: [PATCH 3/8] Update Comment.js Ternary to display asset url if there's no asset title in admin comments --- client/coral-admin/src/routes/Moderation/components/Comment.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/coral-admin/src/routes/Moderation/components/Comment.js b/client/coral-admin/src/routes/Moderation/components/Comment.js index f865908b2..85e68e25a 100644 --- a/client/coral-admin/src/routes/Moderation/components/Comment.js +++ b/client/coral-admin/src/routes/Moderation/components/Comment.js @@ -122,7 +122,7 @@ class Comment extends React.Component {
- Story: {comment.asset.title} + Story: {comment.asset.title ? comment.asset.title : comment.asset.url} {!currentAsset && ( {t('modqueue.moderate')} From 8e0c53ef3b76bff2af2232a42722d9a461ef01b9 Mon Sep 17 00:00:00 2001 From: Olly Dutton Date: Tue, 13 Mar 2018 15:33:46 +1100 Subject: [PATCH 4/8] Update UserDetailComment.js fix linting error --- client/coral-admin/src/components/UserDetailComment.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/coral-admin/src/components/UserDetailComment.js b/client/coral-admin/src/components/UserDetailComment.js index 931b4bdeb..2d39a7853 100644 --- a/client/coral-admin/src/components/UserDetailComment.js +++ b/client/coral-admin/src/components/UserDetailComment.js @@ -76,7 +76,8 @@ class UserDetailComment extends React.Component {
- Story: {comment.asset.title ? comment.asset.title : comment.asset.url} + Story:{' '} + {comment.asset.title ? comment.asset.title : comment.asset.url} { {t('modqueue.moderate')} From 6efda40e295fd0d0c8d3f40c99549057c4e64245 Mon Sep 17 00:00:00 2001 From: Olly Dutton Date: Tue, 13 Mar 2018 15:34:28 +1100 Subject: [PATCH 5/8] Update Comment.js fix linting error --- client/coral-admin/src/routes/Moderation/components/Comment.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/client/coral-admin/src/routes/Moderation/components/Comment.js b/client/coral-admin/src/routes/Moderation/components/Comment.js index 85e68e25a..f3378a234 100644 --- a/client/coral-admin/src/routes/Moderation/components/Comment.js +++ b/client/coral-admin/src/routes/Moderation/components/Comment.js @@ -122,7 +122,8 @@ class Comment extends React.Component {
- Story: {comment.asset.title ? comment.asset.title : comment.asset.url} + Story:{' '} + {comment.asset.title ? comment.asset.title : comment.asset.url} {!currentAsset && ( {t('modqueue.moderate')} From 1a8006e3a3194d65d8d252a28ec41c00daa4ada7 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 14 Mar 2018 14:55:24 -0600 Subject: [PATCH 6/8] 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); From bb1b265cae1bd8383c8f989a80e285c8689b5779 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 14 Mar 2018 15:12:38 -0600 Subject: [PATCH 7/8] lint --- client/coral-admin/src/components/UserDetailComment.js | 2 +- client/coral-admin/src/routes/Moderation/components/Comment.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/client/coral-admin/src/components/UserDetailComment.js b/client/coral-admin/src/components/UserDetailComment.js index 2d39a7853..a14aecf9f 100644 --- a/client/coral-admin/src/components/UserDetailComment.js +++ b/client/coral-admin/src/components/UserDetailComment.js @@ -76,7 +76,7 @@ class UserDetailComment extends React.Component {
- Story:{' '} + Story:{' '} {comment.asset.title ? comment.asset.title : comment.asset.url} { diff --git a/client/coral-admin/src/routes/Moderation/components/Comment.js b/client/coral-admin/src/routes/Moderation/components/Comment.js index f3378a234..637772c0f 100644 --- a/client/coral-admin/src/routes/Moderation/components/Comment.js +++ b/client/coral-admin/src/routes/Moderation/components/Comment.js @@ -122,7 +122,7 @@ class Comment extends React.Component {
- Story:{' '} + Story:{' '} {comment.asset.title ? comment.asset.title : comment.asset.url} {!currentAsset && ( From 8c7e8080c304a8a1679b21d95adb06ebc6e61601 Mon Sep 17 00:00:00 2001 From: Wyatt Johnson Date: Wed, 14 Mar 2018 15:13:44 -0600 Subject: [PATCH 8/8] added translations --- client/coral-admin/src/components/UserDetailComment.js | 3 ++- client/coral-admin/src/routes/Moderation/components/Comment.js | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/client/coral-admin/src/components/UserDetailComment.js b/client/coral-admin/src/components/UserDetailComment.js index a14aecf9f..206242058 100644 --- a/client/coral-admin/src/components/UserDetailComment.js +++ b/client/coral-admin/src/components/UserDetailComment.js @@ -76,7 +76,7 @@ class UserDetailComment extends React.Component {
- Story:{' '} + {t('common.story')}:{' '} {comment.asset.title ? comment.asset.title : comment.asset.url} { @@ -110,6 +110,7 @@ class UserDetailComment extends React.Component {
+ {/* TODO: translate string */} Contains Link diff --git a/client/coral-admin/src/routes/Moderation/components/Comment.js b/client/coral-admin/src/routes/Moderation/components/Comment.js index 637772c0f..efdcfbd79 100644 --- a/client/coral-admin/src/routes/Moderation/components/Comment.js +++ b/client/coral-admin/src/routes/Moderation/components/Comment.js @@ -122,7 +122,7 @@ class Comment extends React.Component {
- Story:{' '} + {t('common.story')}:{' '} {comment.asset.title ? comment.asset.title : comment.asset.url} {!currentAsset && ( @@ -157,6 +157,7 @@ class Comment extends React.Component {
+ {/* TODO: translate string */} Contains Link