Skip to content

Regression: LDAP User Data Sync not always working #23321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
casalsgh merged 6 commits into from
Oct 1, 2021
Merged

Regression: LDAP User Data Sync not always working #23321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
407f6c2
Regression: LDAP User Data not updating on login
pierre-lehnen-rc Sep 30, 2021
4ba2160
Close modal on background sync
pierre-lehnen-rc Sep 30, 2021
2718eeb
Replaced warning with an error
pierre-lehnen-rc Sep 30, 2021
5b2d9e1
Fixed email sync
pierre-lehnen-rc Sep 30, 2021
f44112b
Return promise
pierre-lehnen-rc Sep 30, 2021
e59395c
Review Items
pierre-lehnen-rc Oct 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 37 additions & 53 deletions app/importer/server/classes/ImportDataConverter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,9 @@ 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, addUserToDefaultChannels } 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<string, any>;
Expand Down Expand Up @@ -148,6 +148,26 @@ export class ImportDataConverter {
}
}

addUserEmails(updateData: Record<string, any>, userData: IImportUser, existingEmails: Array<IUserEmail>): void {
if (!userData.emails?.length) {
return;
}

const verifyEmails = Boolean(this.options.flagEmailsAsVerified);
const newEmailList: Array<IUserEmail> = [];

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<string, any>, userData: IImportUser): void {
if (!userData.services) {
return;
Expand All @@ -170,14 +190,6 @@ export class ImportDataConverter {
}
}

flagEmailsAsVerified(updateData: Record<string, any>, userData: IImportUser): void {
if (!this.options.flagEmailsAsVerified || !userData.emails.length) {
return;
}

updateData.$set['emails.$[].verified'] = true;
}

addCustomFields(updateData: Record<string, any>, userData: IImportUser): void {
if (!userData.customFields) {
return;
Expand All @@ -202,43 +214,40 @@ 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<string, any> = {
$set: {
roles: userData.roles || ['user'],
type: userData.type || 'user',
...userData.statusText && { statusText: userData.statusText },
...userData.bio && { bio: userData.bio },
...userData.name && { name: userData.name },
...userData.services?.ldap && { ldap: true },
...userData.avatarUrl && { _pendingAvatarUrl: userData.avatarUrl },
},
};

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);
}

updateUser(existingUser: IUser, userData: IImportUser): void {
userData._id = existingUser._id;
if (userData.utcOffset) {
Users.setUtcOffset(_id, userData.utcOffset);
}

this.updateUserId(userData._id, userData);
if (userData.name || userData.username) {
saveUserIdentity({ _id, name: userData.name, username: userData.username });
}

if (userData.importIds.length) {
this.addUserToCache(userData.importIds[0], existingUser._id, existingUser.username);
}

if (userData.avatarUrl) {
try {
Users.update({ _id: existingUser._id }, { $set: { _pendingAvatarUrl: userData.avatarUrl } });
} catch (error) {
this._logger.warn(`Failed to set ${ existingUser._id }'s avatar from url ${ userData.avatarUrl }`);
this._logger.error(error);
}
}
}

insertUser(userData: IImportUser): IUser {
Expand All @@ -253,35 +262,10 @@ 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);
}

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);
}
}
});

addUserToDefaultChannels(user, true);
return user;
}

Expand Down
2 changes: 2 additions & 0 deletions client/views/admin/settings/groups/LDAPGroupPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ function LDAPGroupPage({ _id, ...group }: ISetting): JSX.Element {
try {
await testConnection(undefined);
const confirmSync = async (): Promise<void> => {
closeModal();

try {
const { message } = await syncNow(undefined);
dispatchToastMessage({ type: 'success', message: t(message) });
Expand Down
4 changes: 2 additions & 2 deletions server/lib/ldap/Connection.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 });
Expand Down Expand Up @@ -394,7 +394,7 @@ export class LDAPConnection {
}

public async bindDN(dn: string, password: string): Promise<void> {
await new Promise<void>((resolve, reject) => {
return new Promise<void>((resolve, reject) => {
try {
this.client.bind(dn, password, (error) => {
if (error) {
Expand Down
22 changes: 12 additions & 10 deletions server/lib/ldap/Manager.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -205,19 +205,18 @@ export class LDAPManager {
private static async addLdapUser(ldapUser: ILDAPEntry, username: string | undefined, password: string | undefined, ldap: LDAPConnection): Promise<LDAPLoginResult> {
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<boolean>('LDAP_Login_Fallback') && typeof password === 'string' && password.trim() !== '') {
Accounts.setPassword(user._id, password, { logout: false });
Expand All @@ -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<boolean>('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<boolean>('LDAP_Update_Data_On_Login') ?? true);
logger.debug({ msg: 'Logging user in', syncData });
const updatedUser = (syncData && await this.syncUserForLogin(ldapUser, user)) || user;

Expand Down