From 407f6c2358d455f7f7b81dc6a0357187b557fa8b Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Thu, 30 Sep 2021 16:20:35 -0300 Subject: [PATCH 1/6] Regression: LDAP User Data not updating on login --- .../server/classes/ImportDataConverter.ts | 52 +++++++------------ app/lib/server/functions/saveUserIdentity.js | 5 +- server/lib/ldap/Manager.ts | 22 ++++---- 3 files changed, 35 insertions(+), 44 deletions(-) diff --git a/app/importer/server/classes/ImportDataConverter.ts b/app/importer/server/classes/ImportDataConverter.ts index c1adb2323cc61..7384cb112b9b9 100644 --- a/app/importer/server/classes/ImportDataConverter.ts +++ b/app/importer/server/classes/ImportDataConverter.ts @@ -9,7 +9,7 @@ import { IImportChannel } from '../../../../definition/IImportChannel'; import { IConversionCallbacks } from '../definitions/IConversionCallbacks'; import { IImportUserRecord, IImportChannelRecord, IImportMessageRecord } from '../../../../definition/IImportRecord'; import { Users, Rooms, Subscriptions, ImportData } from '../../../models/server'; -import { generateUsernameSuggestion, insertMessage } from '../../../lib/server'; +import { generateUsernameSuggestion, insertMessage, saveUserIdentity } from '../../../lib/server'; import { setUserActiveStatus } from '../../../lib/server/functions/setUserActiveStatus'; import { IUser } from '../../../../definition/IUser'; import type { Logger } from '../../../../server/lib/logger/Logger'; @@ -210,7 +210,6 @@ export class ImportDataConverter { type: userData.type || 'user', ...userData.statusText && { statusText: userData.statusText }, ...userData.bio && { bio: userData.bio }, - ...userData.name && { name: userData.name }, ...userData.services?.ldap && { ldap: true }, }, }; @@ -220,27 +219,35 @@ export class ImportDataConverter { this.addUserImportId(updateData, userData); this.flagEmailsAsVerified(updateData, userData); Users.update({ _id }, updateData); - } - - updateUser(existingUser: IUser, userData: IImportUser): void { - userData._id = existingUser._id; - this.updateUserId(userData._id, userData); + if (userData.utcOffset) { + Users.setUtcOffset(_id, userData.utcOffset); + } - if (userData.importIds.length) { - this.addUserToCache(userData.importIds[0], existingUser._id, existingUser.username); + if (userData.name || userData.username) { + saveUserIdentity({ _id, name: userData.name, username: userData.username, joinDefaultChannelsSilenced: true }); } if (userData.avatarUrl) { try { - Users.update({ _id: existingUser._id }, { $set: { _pendingAvatarUrl: userData.avatarUrl } }); + Users.update({ _id }, { $set: { _pendingAvatarUrl: userData.avatarUrl } }); } catch (error) { - this._logger.warn(`Failed to set ${ existingUser._id }'s avatar from url ${ userData.avatarUrl }`); + this._logger.warn(`Failed to set ${ _id }'s avatar from url ${ userData.avatarUrl }`); this._logger.error(error); } } } + updateUser(existingUser: IUser, userData: IImportUser): void { + userData._id = existingUser._id; + + this.updateUserId(userData._id, userData); + + if (userData.importIds.length) { + this.addUserToCache(userData.importIds[0], existingUser._id, existingUser.username); + } + } + insertUser(userData: IImportUser): IUser { const password = `${ Date.now() }${ userData.name || '' }${ userData.emails.length ? userData.emails[0].toUpperCase() : '' }`; const userId = userData.emails.length ? Accounts.createUser({ @@ -260,28 +267,7 @@ export class ImportDataConverter { this.addUserToCache(userData.importIds[0], user._id, userData.username); } - Meteor.runAsUser(userId, () => { - Meteor.call('setUsername', userData.username, { joinDefaultChannelsSilenced: true }); - if (userData.name) { - Users.setName(userId, userData.name); - } - - this.updateUserId(userId, userData); - - if (userData.utcOffset) { - Users.setUtcOffset(userId, userData.utcOffset); - } - - if (userData.avatarUrl) { - try { - Users.update({ _id: userId }, { $set: { _pendingAvatarUrl: userData.avatarUrl } }); - } catch (error) { - this._logger.warn(`Failed to set ${ userId }'s avatar from url ${ userData.avatarUrl }`); - this._logger.error(error); - } - } - }); - + this.updateUserId(userId, userData); return user; } diff --git a/app/lib/server/functions/saveUserIdentity.js b/app/lib/server/functions/saveUserIdentity.js index 6b7559648dda8..267d333e6b868 100644 --- a/app/lib/server/functions/saveUserIdentity.js +++ b/app/lib/server/functions/saveUserIdentity.js @@ -4,12 +4,13 @@ import { Messages, Rooms, Subscriptions, LivechatDepartmentAgents, Users } from import { FileUpload } from '../../../file-upload/server'; import { updateGroupDMsName } from './updateGroupDMsName'; import { validateName } from './validateName'; +import { addUserToDefaultChannels } from './addUserToDefaultChannels'; /** * * @param {object} changes changes to the user */ -export function saveUserIdentity({ _id, name: rawName, username: rawUsername }) { +export function saveUserIdentity({ _id, name: rawName, username: rawUsername, joinDefaultChannelsSilenced }) { if (!_id) { return false; } @@ -72,6 +73,8 @@ export function saveUserIdentity({ _id, name: rawName, username: rawUsername }) // update name and fname of group direct messages updateGroupDMsName(user); } + } else { + addUserToDefaultChannels(user, joinDefaultChannelsSilenced ?? false); } return true; diff --git a/server/lib/ldap/Manager.ts b/server/lib/ldap/Manager.ts index 17ec55c9c46ce..184d01bb9f519 100644 --- a/server/lib/ldap/Manager.ts +++ b/server/lib/ldap/Manager.ts @@ -205,19 +205,18 @@ export class LDAPManager { private static async addLdapUser(ldapUser: ILDAPEntry, username: string | undefined, password: string | undefined, ldap: LDAPConnection): Promise { const user = await this.syncUserForLogin(ldapUser, undefined, username); - this.onLogin(ldapUser, user, password, ldap, true); - if (user) { - return { - userId: user._id, - }; - } - } - - private static onLogin(ldapUser: ILDAPEntry, user: IUser | undefined, password: string | undefined, ldap: LDAPConnection, isNewUser: boolean): void { if (!user) { return; } + this.onLogin(ldapUser, user, password, ldap, true); + + return { + userId: user._id, + }; + } + + private static onLogin(ldapUser: ILDAPEntry, user: IUser, password: string | undefined, ldap: LDAPConnection, isNewUser: boolean): void { logger.debug('running onLDAPLogin'); if (settings.get('LDAP_Login_Fallback') && typeof password === 'string' && password.trim() !== '') { Accounts.setPassword(user._id, password, { logout: false }); @@ -233,7 +232,10 @@ export class LDAPManager { throw new Meteor.Error('LDAP-login-error', `LDAP Authentication succeeded, but there's already an existing user with provided username [${ user.username }] in Mongo.`); } - const syncData = settings.get('LDAP_Update_Data_On_Login') ?? true; + // If we're merging an ldap user with a local user, then we need to sync the data even if 'update data on login' is off. + const forceUserSync = !user.ldap; + + const syncData = forceUserSync || (settings.get('LDAP_Update_Data_On_Login') ?? true); logger.debug({ msg: 'Logging user in', syncData }); const updatedUser = (syncData && await this.syncUserForLogin(ldapUser, user)) || user; From 4ba2160821c7581783a056da9de042c5e67155b3 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Thu, 30 Sep 2021 17:00:22 -0300 Subject: [PATCH 2/6] Close modal on background sync --- client/views/admin/settings/groups/LDAPGroupPage.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/client/views/admin/settings/groups/LDAPGroupPage.tsx b/client/views/admin/settings/groups/LDAPGroupPage.tsx index 0d2a30bf27d7f..94a4b59273a15 100644 --- a/client/views/admin/settings/groups/LDAPGroupPage.tsx +++ b/client/views/admin/settings/groups/LDAPGroupPage.tsx @@ -50,6 +50,8 @@ function LDAPGroupPage({ _id, ...group }: ISetting): JSX.Element { try { await testConnection(undefined); const confirmSync = async (): Promise => { + closeModal(); + try { const { message } = await syncNow(undefined); dispatchToastMessage({ type: 'success', message: t(message) }); From 2718eebb7579727b267a04c852e1d7091d173898 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Thu, 30 Sep 2021 17:00:30 -0300 Subject: [PATCH 3/6] Replaced warning with an error --- server/lib/ldap/Connection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/ldap/Connection.ts b/server/lib/ldap/Connection.ts index 37f319dd2d9f2..e3cccaf148f7d 100644 --- a/server/lib/ldap/Connection.ts +++ b/server/lib/ldap/Connection.ts @@ -173,7 +173,7 @@ export class LDAPConnection { } searchOptions.filter = new this.ldapjs.filters.OrFilter({ filters }); } else { - searchLogger.warn('Unique Identifier Field is not configured.'); + throw new Error('Unique Identifier Field is not configured.'); } searchLogger.info({ msg: 'Searching by id', id }); From 5b2d9e1b5193c2e72d7287561d3b4f3cae49c667 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Thu, 30 Sep 2021 18:56:26 -0300 Subject: [PATCH 4/6] Fixed email sync --- .../server/classes/ImportDataConverter.ts | 51 ++++++++++--------- 1 file changed, 28 insertions(+), 23 deletions(-) diff --git a/app/importer/server/classes/ImportDataConverter.ts b/app/importer/server/classes/ImportDataConverter.ts index 7384cb112b9b9..cbddade8655e7 100644 --- a/app/importer/server/classes/ImportDataConverter.ts +++ b/app/importer/server/classes/ImportDataConverter.ts @@ -11,7 +11,7 @@ import { IImportUserRecord, IImportChannelRecord, IImportMessageRecord } from '. import { Users, Rooms, Subscriptions, ImportData } from '../../../models/server'; import { generateUsernameSuggestion, insertMessage, saveUserIdentity } from '../../../lib/server'; import { setUserActiveStatus } from '../../../lib/server/functions/setUserActiveStatus'; -import { IUser } from '../../../../definition/IUser'; +import { IUser, IUserEmail } from '../../../../definition/IUser'; import type { Logger } from '../../../../server/lib/logger/Logger'; type IRoom = Record; @@ -148,6 +148,26 @@ export class ImportDataConverter { } } + addUserEmails(updateData: Record, userData: IImportUser, existingEmails: Array): void { + if (!userData.emails?.length) { + return; + } + + const verifyEmails = Boolean(this.options.flagEmailsAsVerified); + const newEmailList: Array = []; + + for (const email of userData.emails) { + const verified = verifyEmails || existingEmails.find((ee) => ee.address === email)?.verified || false; + + newEmailList.push({ + address: email, + verified, + }); + } + + updateData.$set.emails = newEmailList; + } + addUserServices(updateData: Record, userData: IImportUser): void { if (!userData.services) { return; @@ -170,14 +190,6 @@ export class ImportDataConverter { } } - flagEmailsAsVerified(updateData: Record, userData: IImportUser): void { - if (!this.options.flagEmailsAsVerified || !userData.emails.length) { - return; - } - - updateData.$set['emails.$[].verified'] = true; - } - addCustomFields(updateData: Record, userData: IImportUser): void { if (!userData.customFields) { return; @@ -202,7 +214,11 @@ export class ImportDataConverter { subset(userData.customFields, 'customFields'); } - updateUserId(_id: string, userData: IImportUser): void { + updateUser(existingUser: IUser, userData: IImportUser): void { + const { _id } = existingUser; + + userData._id = _id; + // #ToDo: #TODO: Move this to the model class const updateData: Record = { $set: { @@ -217,7 +233,7 @@ export class ImportDataConverter { this.addCustomFields(updateData, userData); this.addUserServices(updateData, userData); this.addUserImportId(updateData, userData); - this.flagEmailsAsVerified(updateData, userData); + this.addUserEmails(updateData, userData, existingUser.emails || []); Users.update({ _id }, updateData); if (userData.utcOffset) { @@ -236,12 +252,6 @@ export class ImportDataConverter { this._logger.error(error); } } - } - - updateUser(existingUser: IUser, userData: IImportUser): void { - userData._id = existingUser._id; - - this.updateUserId(userData._id, userData); if (userData.importIds.length) { this.addUserToCache(userData.importIds[0], existingUser._id, existingUser.username); @@ -260,14 +270,9 @@ export class ImportDataConverter { joinDefaultChannelsSilenced: true, }); - userData._id = userId; const user = Users.findOneById(userId, {}); + this.updateUser(user, userData); - if (user && userData.importIds.length) { - this.addUserToCache(userData.importIds[0], user._id, userData.username); - } - - this.updateUserId(userId, userData); return user; } From f44112b57d56e49b830de08975c2dd349923e73a Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Thu, 30 Sep 2021 19:21:25 -0300 Subject: [PATCH 5/6] Return promise --- server/lib/ldap/Connection.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/lib/ldap/Connection.ts b/server/lib/ldap/Connection.ts index e3cccaf148f7d..811e554a34e49 100644 --- a/server/lib/ldap/Connection.ts +++ b/server/lib/ldap/Connection.ts @@ -394,7 +394,7 @@ export class LDAPConnection { } public async bindDN(dn: string, password: string): Promise { - await new Promise((resolve, reject) => { + return new Promise((resolve, reject) => { try { this.client.bind(dn, password, (error) => { if (error) { From e59395cda04fbf1d2efecacd4dc4fe8cd07038a7 Mon Sep 17 00:00:00 2001 From: Pierre Lehnen Date: Fri, 1 Oct 2021 14:03:54 -0300 Subject: [PATCH 6/6] Review Items --- .../server/classes/ImportDataConverter.ts | 15 ++++----------- app/lib/server/functions/saveUserIdentity.js | 5 +---- 2 files changed, 5 insertions(+), 15 deletions(-) diff --git a/app/importer/server/classes/ImportDataConverter.ts b/app/importer/server/classes/ImportDataConverter.ts index cbddade8655e7..9f09a44d49bd6 100644 --- a/app/importer/server/classes/ImportDataConverter.ts +++ b/app/importer/server/classes/ImportDataConverter.ts @@ -9,7 +9,7 @@ import { IImportChannel } from '../../../../definition/IImportChannel'; import { IConversionCallbacks } from '../definitions/IConversionCallbacks'; import { IImportUserRecord, IImportChannelRecord, IImportMessageRecord } from '../../../../definition/IImportRecord'; import { Users, Rooms, Subscriptions, ImportData } from '../../../models/server'; -import { generateUsernameSuggestion, insertMessage, saveUserIdentity } from '../../../lib/server'; +import { generateUsernameSuggestion, insertMessage, saveUserIdentity, addUserToDefaultChannels } from '../../../lib/server'; import { setUserActiveStatus } from '../../../lib/server/functions/setUserActiveStatus'; import { IUser, IUserEmail } from '../../../../definition/IUser'; import type { Logger } from '../../../../server/lib/logger/Logger'; @@ -227,6 +227,7 @@ export class ImportDataConverter { ...userData.statusText && { statusText: userData.statusText }, ...userData.bio && { bio: userData.bio }, ...userData.services?.ldap && { ldap: true }, + ...userData.avatarUrl && { _pendingAvatarUrl: userData.avatarUrl }, }, }; @@ -241,16 +242,7 @@ export class ImportDataConverter { } if (userData.name || userData.username) { - saveUserIdentity({ _id, name: userData.name, username: userData.username, joinDefaultChannelsSilenced: true }); - } - - if (userData.avatarUrl) { - try { - Users.update({ _id }, { $set: { _pendingAvatarUrl: userData.avatarUrl } }); - } catch (error) { - this._logger.warn(`Failed to set ${ _id }'s avatar from url ${ userData.avatarUrl }`); - this._logger.error(error); - } + saveUserIdentity({ _id, name: userData.name, username: userData.username }); } if (userData.importIds.length) { @@ -273,6 +265,7 @@ export class ImportDataConverter { const user = Users.findOneById(userId, {}); this.updateUser(user, userData); + addUserToDefaultChannels(user, true); return user; } diff --git a/app/lib/server/functions/saveUserIdentity.js b/app/lib/server/functions/saveUserIdentity.js index 267d333e6b868..6b7559648dda8 100644 --- a/app/lib/server/functions/saveUserIdentity.js +++ b/app/lib/server/functions/saveUserIdentity.js @@ -4,13 +4,12 @@ import { Messages, Rooms, Subscriptions, LivechatDepartmentAgents, Users } from import { FileUpload } from '../../../file-upload/server'; import { updateGroupDMsName } from './updateGroupDMsName'; import { validateName } from './validateName'; -import { addUserToDefaultChannels } from './addUserToDefaultChannels'; /** * * @param {object} changes changes to the user */ -export function saveUserIdentity({ _id, name: rawName, username: rawUsername, joinDefaultChannelsSilenced }) { +export function saveUserIdentity({ _id, name: rawName, username: rawUsername }) { if (!_id) { return false; } @@ -73,8 +72,6 @@ export function saveUserIdentity({ _id, name: rawName, username: rawUsername, jo // update name and fname of group direct messages updateGroupDMsName(user); } - } else { - addUserToDefaultChannels(user, joinDefaultChannelsSilenced ?? false); } return true;