-
Notifications
You must be signed in to change notification settings - Fork 10.3k
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.
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
[NEW] Allow ldap mapping of customFields #7614
[NEW] Allow ldap mapping of customFields #7614
Conversation
Extending ldap mapping functionality to also allow customFields synchronization.
@goiaba can you fix the conflict? |
@rodrigok done. |
const tmpLdapField = RocketChat.templateVarHandler(ldapField, ldapUser.object); | ||
const userFieldValue = _.reduce(userField.split('.'), (acc, el) => acc[el], user); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens when a subfield is undefined? Like customFields.name.first
and name
is undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An exception! :$ I'll fix it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigok I see you've already merged this into develop branch, but I think you were right regarding this part of the code. An exception would be thrown and it would not be properly handled. You were faster than me, I didn't have enough time to push the changes before your merge. I'd recommend replacing this:
const userFieldValue = _.reduce(userField.split('.'), (acc, el) => acc[el], user);
by this (or something more elegant...):
let userFieldValue;
try {
userFieldValue = _.reduce(userField.split('.'), (acc, el) => acc[el], user);
if (!userFieldValue) {
throw new Error();
}
} catch (err) {
logger.debug(`user attribute does not exist: ${ userField }`);
return;
}
What do you think @rodrigok?
userData.name = tmpLdapField; | ||
logger.debug(`user.name changed to: ${ tmpLdapField }`); | ||
if (tmpLdapField && userFieldValue !== tmpLdapField) { | ||
userData[userField] = tmpLdapField; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A field containing .
will generate an exception in MongoDB
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rodrigok Didn't get this one. Is it possible to have a field containing .
? Wouldn't MongoDB interpret it as a nested field?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@goiaba Is not possible to have a field containing .
, since you are creating an object like:
{
"name.first": "Rodrigo"
}
and it should be
{
name: {
first: 'rodrigo'
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my tests, if the variable userData contains the object { name: 'value1', 'customFields.secao-descricao': 'value2','customFields.secao-sigla': 'value3' }
, the object stored in the users collection, inside a user document is "customFields" : { "secao-sigla" : "value3", "secao-descricao" : "value2" }
. No exceptions occur.
We have other examples in the same code (sync.js), in the line 123, for instance: userData['services.ldap.id'] = uniqueId.value;
.
Maybe I still didn't understand what you're trying to show me. :-(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry @goiaba , I was totally wrong 😞
@rodrigok This code is buggy! It doesn't work if the customFields do not already exist (causing an exception that prevents user's login). My bad! I already fixed the problem and can open a new PR later. May I create a new PR to revert the merge? |
Any news ? Can't use ldap mapping with custom fileds with version 0.59.0 : departmentNumber is correctly set in my ldap entry
|
@magicbelette Maybe I missed this setting within the LDAP administration screen |
Extending ldap mapping functionality to also allow custom fields synchronization.
@RocketChat/core
Closes #4332
Example: