From 4a59798da85086e0c792853ee2e52b49b46cd42a Mon Sep 17 00:00:00 2001 From: Rafael Tapia Date: Wed, 11 Oct 2023 12:05:51 -0300 Subject: [PATCH] fix: use setImmediate to handle username update (#30500) --- .changeset/wicked-jars-double.md | 5 + .../app/lib/server/functions/saveUser.js | 1 + .../lib/server/functions/saveUserIdentity.ts | 129 ++++++++++++----- apps/meteor/tests/end-to-end/api/09-rooms.js | 48 ++++--- .../server/users/saveUserIdentity.spec.ts | 134 ++++++++++++++++++ 5 files changed, 258 insertions(+), 59 deletions(-) create mode 100644 .changeset/wicked-jars-double.md create mode 100644 apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts diff --git a/.changeset/wicked-jars-double.md b/.changeset/wicked-jars-double.md new file mode 100644 index 000000000000..23deffe8606f --- /dev/null +++ b/.changeset/wicked-jars-double.md @@ -0,0 +1,5 @@ +--- +"@rocket.chat/meteor": patch +--- + +Handle the username update in the background diff --git a/apps/meteor/app/lib/server/functions/saveUser.js b/apps/meteor/app/lib/server/functions/saveUser.js index 42438be4ab7a..46bef4c7d1aa 100644 --- a/apps/meteor/app/lib/server/functions/saveUser.js +++ b/apps/meteor/app/lib/server/functions/saveUser.js @@ -366,6 +366,7 @@ export const saveUser = async function (userId, userData) { _id: userData._id, username: userData.username, name: userData.name, + updateUsernameInBackground: true, })) ) { throw new Meteor.Error('error-could-not-save-identity', 'Could not save user identity', { diff --git a/apps/meteor/app/lib/server/functions/saveUserIdentity.ts b/apps/meteor/app/lib/server/functions/saveUserIdentity.ts index 2eb360e150c6..34ca0ca246db 100644 --- a/apps/meteor/app/lib/server/functions/saveUserIdentity.ts +++ b/apps/meteor/app/lib/server/functions/saveUserIdentity.ts @@ -1,5 +1,7 @@ +import type { IUser } from '@rocket.chat/core-typings'; import { Messages, VideoConference, LivechatDepartmentAgents, Rooms, Subscriptions, Users } from '@rocket.chat/models'; +import { SystemLogger } from '../../../../server/lib/logger/system'; import { FileUpload } from '../../../file-upload/server'; import { _setRealName } from './setRealName'; import { _setUsername } from './setUsername'; @@ -11,7 +13,17 @@ import { validateName } from './validateName'; * @param {object} changes changes to the user */ -export async function saveUserIdentity({ _id, name: rawName, username: rawUsername }: { _id: string; name?: string; username?: string }) { +export async function saveUserIdentity({ + _id, + name: rawName, + username: rawUsername, + updateUsernameInBackground = false, +}: { + _id: string; + name?: string; + username?: string; + updateUsernameInBackground?: boolean; // TODO: remove this +}) { if (!_id) { return false; } @@ -48,46 +60,91 @@ export async function saveUserIdentity({ _id, name: rawName, username: rawUserna // if coming from old username, update all references if (previousUsername) { - if (usernameChanged && typeof rawUsername !== 'undefined') { - const fileStore = FileUpload.getStore('Avatars'); - const previousFile = await fileStore.model.findOneByName(previousUsername); - const file = await fileStore.model.findOneByName(username); - if (file) { - await fileStore.model.deleteFile(file._id); - } - if (previousFile) { - await fileStore.model.updateFileNameById(previousFile._id, username); - } - - await Messages.updateAllUsernamesByUserId(user._id, username); - await Messages.updateUsernameOfEditByUserId(user._id, username); - - const cursor = Messages.findByMention(previousUsername); - for await (const msg of cursor) { - const updatedMsg = msg.msg.replace(new RegExp(`@${previousUsername}`, 'ig'), `@${username}`); - await Messages.updateUsernameAndMessageOfMentionByIdAndOldUsername(msg._id, previousUsername, username, updatedMsg); - } - - await Rooms.replaceUsername(previousUsername, username); - await Rooms.replaceMutedUsername(previousUsername, username); - await Rooms.replaceUsernameOfUserByUserId(user._id, username); - await Subscriptions.setUserUsernameByUserId(user._id, username); - - await LivechatDepartmentAgents.replaceUsernameOfAgentByUserId(user._id, username); + const handleUpdateParams = { + username, + previousUsername, + rawUsername, + usernameChanged, + user, + name, + previousName, + rawName, + nameChanged, + }; + if (updateUsernameInBackground) { + setImmediate(async () => { + try { + await updateUsernameReferences(handleUpdateParams); + } catch (err) { + SystemLogger.error(err); + } + }); + } else { + await updateUsernameReferences(handleUpdateParams); } + } + + return true; +} - // update other references if either the name or username has changed - if (usernameChanged || nameChanged) { - // update name and fname of 1-on-1 direct messages - await Subscriptions.updateDirectNameAndFnameByName(previousUsername, rawUsername && username, rawName && name); +async function updateUsernameReferences({ + username, + previousUsername, + rawUsername, + usernameChanged, + user, + name, + previousName, + rawName, + nameChanged, +}: { + username: string; + previousUsername: string; + rawUsername?: string; + usernameChanged: boolean; + user: IUser; + name: string; + previousName: string | undefined; + rawName?: string; + nameChanged: boolean; +}): Promise { + if (usernameChanged && typeof rawUsername !== 'undefined') { + const fileStore = FileUpload.getStore('Avatars'); + const previousFile = await fileStore.model.findOneByName(previousUsername); + const file = await fileStore.model.findOneByName(username); + if (file) { + await fileStore.model.deleteFile(file._id); + } + if (previousFile) { + await fileStore.model.updateFileNameById(previousFile._id, username); + } - // update name and fname of group direct messages - await updateGroupDMsName(user); + await Messages.updateAllUsernamesByUserId(user._id, username); + await Messages.updateUsernameOfEditByUserId(user._id, username); - // update name and username of users on video conferences - await VideoConference.updateUserReferences(user._id, username || previousUsername, name || previousName); + const cursor = Messages.findByMention(previousUsername); + for await (const msg of cursor) { + const updatedMsg = msg.msg.replace(new RegExp(`@${previousUsername}`, 'ig'), `@${username}`); + await Messages.updateUsernameAndMessageOfMentionByIdAndOldUsername(msg._id, previousUsername, username, updatedMsg); } + + await Rooms.replaceUsername(previousUsername, username); + await Rooms.replaceMutedUsername(previousUsername, username); + await Rooms.replaceUsernameOfUserByUserId(user._id, username); + await Subscriptions.setUserUsernameByUserId(user._id, username); + + await LivechatDepartmentAgents.replaceUsernameOfAgentByUserId(user._id, username); } - return true; + // update other references if either the name or username has changed + if (usernameChanged || nameChanged) { + // update name and fname of 1-on-1 direct messages + await Subscriptions.updateDirectNameAndFnameByName(previousUsername, rawUsername && username, rawName && name); + + // update name and fname of group direct messages + await updateGroupDMsName(user); + + // update name and username of users on video conferences + await VideoConference.updateUserReferences(user._id, username || previousUsername, name || previousName); + } } diff --git a/apps/meteor/tests/end-to-end/api/09-rooms.js b/apps/meteor/tests/end-to-end/api/09-rooms.js index 5a534fe2674d..ed3c7eefb15b 100644 --- a/apps/meteor/tests/end-to-end/api/09-rooms.js +++ b/apps/meteor/tests/end-to-end/api/09-rooms.js @@ -4,6 +4,7 @@ import path from 'path'; import { expect } from 'chai'; import { after, afterEach, before, beforeEach, describe, it } from 'mocha'; +import { sleep } from '../../../lib/utils/sleep'; import { getCredentials, api, request, credentials } from '../../data/api-data.js'; import { sendSimpleMessage, deleteMessage } from '../../data/chat.helper'; import { imgURL } from '../../data/interactions.js'; @@ -1543,29 +1544,30 @@ describe('[Rooms]', function () { roomId = result.body.room.rid; }); - it('should update group name if user changes username', (done) => { - updateSetting('UI_Use_Real_Name', false).then(() => { - request - .post(api('users.update')) - .set(credentials) - .send({ - userId: testUser._id, - data: { - username: `changed.username.${testUser.username}`, - }, - }) - .end(() => { - request - .get(api('subscriptions.getOne')) - .set(credentials) - .query({ roomId }) - .end((err, res) => { - const { subscription } = res.body; - expect(subscription.name).to.equal(`rocket.cat,changed.username.${testUser.username}`); - done(); - }); - }); - }); + it('should update group name if user changes username', async () => { + await updateSetting('UI_Use_Real_Name', false); + await request + .post(api('users.update')) + .set(credentials) + .send({ + userId: testUser._id, + data: { + username: `changed.username.${testUser.username}`, + }, + }); + + // need to wait for the username update finish + await sleep(300); + + await request + .get(api('subscriptions.getOne')) + .set(credentials) + .query({ roomId }) + .send() + .expect((res) => { + const { subscription } = res.body; + expect(subscription.name).to.equal(`rocket.cat,changed.username.${testUser.username}`); + }); }); it('should update group name if user changes name', (done) => { diff --git a/apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts b/apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts new file mode 100644 index 000000000000..b91165fb3ca9 --- /dev/null +++ b/apps/meteor/tests/unit/server/users/saveUserIdentity.spec.ts @@ -0,0 +1,134 @@ +import { expect } from 'chai'; +import proxyquire from 'proxyquire'; +import sinon from 'sinon'; + +// Create stubs for dependencies +const stubs = { + findOneUserById: sinon.stub(), + updateUsernameAndMessageOfMentionByIdAndOldUsername: sinon.stub(), + updateUsernameOfEditByUserId: sinon.stub(), + updateAllUsernamesByUserId: sinon.stub(), + updateDirectNameAndFnameByName: sinon.stub(), + updateUserReferences: sinon.stub(), + setUsername: sinon.stub(), + setRealName: sinon.stub(), + validateName: sinon.stub(), + FileUpload: sinon.stub(), +}; + +const { saveUserIdentity } = proxyquire.noCallThru().load('../../../../app/lib/server/functions/saveUserIdentity', { + '@rocket.chat/models': { + Users: { + findOneById: stubs.findOneUserById, + }, + Messages: { + updateUsernameAndMessageOfMentionByIdAndOldUsername: stubs.updateUsernameAndMessageOfMentionByIdAndOldUsername, + updateUsernameOfEditByUserId: stubs.updateUsernameOfEditByUserId, + updateAllUsernamesByUserId: stubs.updateAllUsernamesByUserId, + }, + Subscriptions: { + updateDirectNameAndFnameByName: stubs.updateDirectNameAndFnameByName, + }, + VideoConference: { + updateUserReferences: stubs.updateUserReferences, + }, + }, + 'meteor/meteor': { + 'Meteor': sinon.stub(), + '@global': true, + }, + '../../../../app/file-upload/server': { + FileUpload: stubs.FileUpload, + }, + '../../../../app/lib/server/functions/setRealName': { + _setRealName: stubs.setRealName, + }, + '../../../../app/lib/server/functions/setUsername': { + _setUsername: stubs.setUsername, + }, + '../../../../app/lib/server/functions/updateGroupDMsName': { + updateGroupDMsName: sinon.stub(), + }, + '../../../../app/lib/server/functions/validateName': { + validateName: stubs.validateName, + }, +}); + +describe('Users - saveUserIdentity', () => { + beforeEach(() => { + // Reset stubs before each test + Object.values(stubs).forEach((stub) => stub.reset()); + }); + + it('should return false if _id is not provided', async () => { + const result = await saveUserIdentity({ _id: undefined }); + + expect(stubs.findOneUserById.called).to.be.false; + expect(result).to.be.false; + }); + + it('should return false if user does not exist', async () => { + stubs.findOneUserById.returns(undefined); + const result = await saveUserIdentity({ _id: 'valid_id' }); + + expect(stubs.findOneUserById.calledWith('valid_id')).to.be.true; + expect(result).to.be.false; + }); + + it('should return false if username is not allowed', async () => { + stubs.findOneUserById.returns({ username: 'oldUsername' }); + stubs.validateName.returns(false); + const result = await saveUserIdentity({ _id: 'valid_id', username: 'admin' }); + + expect(stubs.validateName.calledWith('admin')).to.be.true; + expect(result).to.be.false; + }); + + it('should return false if username is invalid or unavailable', async () => { + stubs.findOneUserById.returns({ username: 'oldUsername' }); + stubs.validateName.returns(true); + stubs.setUsername.returns(false); + const result = await saveUserIdentity({ _id: 'valid_id', username: 'invalidUsername' }); + + expect(stubs.validateName.calledWith('invalidUsername')).to.be.true; + expect(stubs.setUsername.calledWith('valid_id', 'invalidUsername', { username: 'oldUsername' })).to.be.true; + expect(result).to.be.false; + }); + + it("should not update the username if it's not changed", async () => { + stubs.findOneUserById.returns({ username: 'oldUsername', name: 'oldName' }); + stubs.validateName.returns(true); + stubs.setUsername.returns(true); + await saveUserIdentity({ _id: 'valid_id', username: 'oldUsername', name: 'oldName' }); + + expect(stubs.validateName.called).to.be.false; + expect(stubs.setUsername.called).to.be.false; + expect(stubs.updateUsernameOfEditByUserId.called).to.be.false; + expect(stubs.updateAllUsernamesByUserId.called).to.be.false; + expect(stubs.updateUsernameAndMessageOfMentionByIdAndOldUsername.called).to.be.false; + expect(stubs.updateDirectNameAndFnameByName.called).to.be.false; + expect(stubs.updateUserReferences.called).to.be.false; + }); + + it('should return false if _setName fails', async () => { + stubs.findOneUserById.returns({ name: 'oldName' }); + stubs.setRealName.returns(false); + const result = await saveUserIdentity({ _id: 'valid_id', name: 'invalidName' }); + + expect(stubs.setRealName.calledWith('valid_id', 'invalidName', { name: 'oldName' })).to.be.true; + expect(result).to.be.false; + }); + + it('should update Subscriptions and VideoConference if name changes', async () => { + stubs.findOneUserById.returns({ name: 'oldName', username: 'oldUsername' }); + stubs.setRealName.returns(true); + const result = await saveUserIdentity({ _id: 'valid_id', name: 'name', username: 'oldUsername' }); + + expect(stubs.setUsername.called).to.be.false; + expect(stubs.setRealName.called).to.be.true; + expect(stubs.updateUsernameOfEditByUserId.called).to.be.false; + expect(stubs.updateDirectNameAndFnameByName.called).to.be.true; + expect(stubs.updateUserReferences.called).to.be.true; + expect(result).to.be.true; + }); +});