[user_ldap] Update profile from LDAP fields

[march42]

• Resolves: Import personal data from LDAP #32085

Summary

Implement feature, to update user profile data from LDAP attributes.
Add configuration items for LDAP attribute mapping (Administration - LDAP/AD integration - Advanced - User Profile Attributes)
Add methods for user profile properties to lib/private/User/User.php
Add connections for LDAP attributes to user profile properties

TODO

• Testing with various setting and scenario

Checklist

• Code is properly formatted

• Sign-off message is added to all commits

• Tests (unit, integration, api and/or acceptance) are included

• Screenshots before/after for front-end changes
  
  

• Documentation (manuals or wiki) has been updated or is not required
  EDIT: documentation PR 9614

• Backports requested where applicable (ex: critical bugfixes)

[come-nc]

The feature will be popular I think but the code needs to be refined.
Also we must be very careful about performance in user_ldap, this should have no performance impact when not used.

@Pytal Can you review this? It’s a lot related to the profile system.

[march42]

cleanup: remove unneeded unset

[march42]

I reworked the code:

• prepare and propagate an array with the profile data from LDAP and fetching user just once. Like @come-nc suggested.
• added Fediverse profile property
• did some cleanup and simplification
• fixed messages from psalm

I have found, that some commits are not correctly signed-off. Seems to be like all commits through GitHub Desktop. So I would probably best rebase the branch before you finally merge it. Any suggestions?

[come-nc]

See my last comment, please go through the accountmanager instead of adding methods to OCP\User.

Also, could you please detail how this will behave for multivalue fields from LDAP?

Regarding scope, if I understand correctly the admin can force a given scope for all profile data synced from LDAP?
What happens if the user then changes the scope?

Is it possible for the admin to leave the scope option empty to let users decide the scope of each information?

[come-nc]

I have found, that some commits are not correctly signed-off. Seems to be like all commits through GitHub Desktop. So I would probably best rebase the branch before you finally merge it. Any suggestions?

You can use rebase with --signoff option.
For instance git rebase HEAD~25 --signoff will add a signoff to the last 25 commits.

[march42]

Now, you got me a little pissed, because you were unspecific and I think I'm not supposed to read your mind. I'll be specific, so we could find an agreement on how to handle. First of all. I feel unappreciated, even unseen. I created my branch in May 2022, after issue 32085 was opened. see #32085 (comment) I invested time, to package my quick'n'dirty hack and pretty it up, before committing to GitHub. I updated the documentation, run numerous test and checked different scenarios. Nobody seemed to care, until February 2023, when I was asked to make a pull request. I don't need this to be integrated. I don't have any intentions on wasting my time or your time. I have two working solutions and patching after update is done in minutes. If this is unwanted or unappreciated, I'm happy with closing and deletion. This is your call to make. After your suggestions two weeks ago I took time, to rework the code and test on current version 25 and version 26 beta 3 and 4. For each configured profile property you will fetch the user again. You should prepare an array of profile values to apply and give that to your method, which could then fetch the user only once. > Are you sure all those proxy methods are needed? What prevents user_ldap from getting the account itself? I couldn't get the account directly, so I went that way, because that's how eMail and Quota are handled. I took time, to rewrite the code and even more time to test it and cleanup the coding. Now I have to go back and do it again, because your suggestions were too cautious phrased. I'm not very happy with this and I will have to make time for this next weekend. 1. Thanks for being specific about how to handle without touching OCP\IUser I appreciate this very much.

You should use |IAccountManager::getAccount($user)| to get the account, then|IAccount::setProperty| for each property, and then|IAccountManager::updateAccount($account)|.

2. What do you want me to do with those unset. I don't want you to be uncomfortable. I did, what I thought was intended. Looking at the original code of processAttributes, for example version 25 starting line 162, you can see a number of unset($attr) in lines 172, 192, 201, 232. I used that present code as guideline for my coding. I am pretty unaffected by wether or not those are present. I’m still not comfortable with those unset, we are not micromanaging memory anywhere else in our code like this. Like I said, You are doing it in your code exactly in the questioned processAttributes function. What should I do to make you comfortable? 3. I will take time, to ckeck and test this specifically and add more clearification to the documentation.

Also, could you please detail how this will behave for multivalue fields from LDAP?

I intended to take only the first value from multivalue fields. $value=$propertyValue[0]; But I haven't really checked it, because my only multivalue field is email and this is not handled with my code.

Regarding scope, if I understand correctly the admin can force a given scope for all profile data synced from LDAP?

Yes, admin choice is "must be set by user" or a forced scope for all data synced. I choose so, because I didn't like the default "Show to everyone" and wanted to be more restrictive and keep data private.

What happens if the user then changes the scope?

It gets overwritten on next change of data. IF the admin changes the data in LDAP, the scope would be changed for the changed data. I chose not to have the LDAP data cached in user settings, to exactly know what data was touched by user_ldap and what by the user self. This was too much performance impact.

Is it possible for the admin to leave the scope option empty to let users decide the scope of each information?

That's the default "must be set by user" setting and $scope='unset' I will try and make some time this weekend, to look into it again. Thanks for your suggestions and spending your time on this.

[come-nc]

Now, you got me a little pissed, because you were unspecific and I think I'm not supposed to read your mind. I'll be specific, so we could find an agreement on how to handle. First of all. I feel unappreciated, even unseen. I created my branch in May 2022, after issue 32085 was opened. see #32085 (comment) I invested time, to package my quick'n'dirty hack and pretty it up, before committing to GitHub. I updated the documentation, run numerous test and checked different scenarios. Nobody seemed to care, until February 2023, when I was asked to make a pull request.

I’m sorry you feel that way, that was not my intention of course.
I do not follow feature requests in the issues closely as we already have a lot on our plate. So I only became aware of this when the PR was open and I got pinged for a review.
I’m used to reviewing code from collegues and being picky about it, to try to merge the best code as possible. Sorry if that felt like too much complains, your work is appreciated of course. As I previously stated I think this feature will be welcomed by users.

I don't need this to be integrated. I don't have any intentions on wasting my time or your time. I have two working solutions and patching after update is done in minutes. If this is unwanted or unappreciated, I'm happy with closing and deletion. This is your call to make. After your suggestions two weeks ago I took time, to rework the code and test on current version 25 and version 26 beta 3 and 4. […] I took time, to rewrite the code and even more time to test it and cleanup the coding. Now I have to go back and do it again, because your suggestions were too cautious phrased. I'm not very happy with this and I will have to make time for this next weekend.

I am happy with the refactoring you did regarding fetching the user and I do not think it overlaps that much with going through the account intead of OCP\IUser.
Only the calls to $user->setProfileProperty will need to change on user_ldap side.

2. What do you want me to do with those unset. I don't want you to be uncomfortable. I did, what I thought was intended. Looking at the original code of processAttributes, for example version 25 starting line 162, you can see a number of unset($attr) in lines 172, 192, 201, 232. I used that present code as guideline for my coding. I am pretty unaffected by wether or not those are present. I’m still not comfortable with those unset, we are not micromanaging memory anywhere else in our code like this. Like I said, You are doing it in your code exactly in the questioned processAttributes function. What should I do to make you comfortable? 3. I will take time, to ckeck and test this specifically and add more clearification to the documentation.

Thank you for the pointers on processAttribute, that’s code from before I joined Nextcloud, I did not see much unset in the rest of the code.
I was just afraid by merging those unset of setting a precedent and having unsets added everywhere in the code. Because I do not understand why these local vars should be unset more than any other.
@blizzz Do you remember what is the rationale behind the unset calls in processAttribute?

Thank you for your details on scope handling, that sounds quite reasonable.

If this is eating too much of your time I can take it from here if your prefer.

[szaimen]

Merge?

[march42]

Merge?

If you're asking me, then yes.

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22

[come-nc]

There are open questions in nextcloud/documentation#9614