Skip to content
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

Add command to synchronize specific ldap user #29326

Closed
wants to merge 3 commits into from

Conversation

goyome
Copy link
Contributor

@goyome goyome commented Oct 19, 2021

Add a new command to force sync LDAP user through cli.
Adress issue #27948

Signed-off-by: Guillaume COLSON <guillaume.colson@univ-lorraine.fr>
apps/user_ldap/lib/Command/SyncUser.php Outdated Show resolved Hide resolved
apps/user_ldap/lib/Command/SyncUser.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan added the 2. developing Work in progress label Oct 20, 2021
@CarlSchwan CarlSchwan requested review from blizzz and come-nc October 20, 2021 14:10
@CarlSchwan CarlSchwan added this to the Nextcloud 24 milestone Oct 20, 2021
Copy link
Member

@blizzz blizzz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see inline comment

apps/user_ldap/lib/Command/SyncUser.php Outdated Show resolved Hide resolved
@goyome
Copy link
Contributor Author

goyome commented Oct 21, 2021

I've tried to add dependency injection. It works well on my test instance.
Thanks again for your help.

goyome and others added 2 commits October 22, 2021 14:22
Co-authored-by: Carl Schwan <carl@carlschwan.eu>
Signed-off-by: Guillaume COLSON <guillaume.colson@univ-lorraine.fr>
Signed-off-by: Guillaume COLSON <guillaume.colson@univ-lorraine.fr>

$connection = $access->getConnection();

$loginName = $access->escapeFilterPart($input->getArgument('uid'));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this will not work, because the user id is not necessarily (and not by default) a valid login name.

Oh, btw, why is occ ldap:check-user --update $uid not sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi,

Thanks for following this PR.
The case I'm trying to address is when a new user appears in LDAP and needs to be synchronize in NC.
I want to speed up the process of creation of the account.

occ ldap:check-user --update $uid doesn't synchronize to NC such users.
I get the following message
The given user is not a recognized LDAP user.
I assume that this script searches for the user in NC database and then update its attribute.

What I want is to search in LDAP(using the defined filter) the username passed in parameters and then

  • If the user exists and is not present in NC, I want to create it <- that's what I want to do
  • If it exists in the LDAP and is present in NC database, its attributes are updated
  • If it doesn't exist, then print an error message

Concerning the first sentence in your comment, I believe (tell me if it's not the case) that my description is not accurate.
In fact, if the uid passed in parameter (toto in the example below) is not a valid login name, you get an error message indicating this fact.

php occ ldap:syncuser  toto
No user found with uid : toto

Maybe I should change the name of the command by ldap:create-user or ldap:add-user to be more explicit.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi goyome,

thanks for this valuable input. Based on this, i would like to suggest following:

  1. Rename the argument in configure() from uid to loginName. uid is typically used in Nextcloud to refer to the user id, while the login name can be one or more arbitrary LDAP attributes and some servers don't use uid. That way it is more clear for both users as well as developers to what to expect.

  2. $access = $this->backend->getLDAPAccess($uid); in the current code may give you the wrong instancess of Access since $uid is most likely unknown. This affects setups with more than one LDAP server configured. The User_Proxy $backend offers you a handy method however: $uid = $backend->loginName2UserName($loginName). It pulls yet unknown users, too.

  3. With loginName2UserName() from step 2 you can drop the manual fetchListOfUsers because this is already done, and the user imported. The returnd $uid is false when no user was found, otherwise contains the user id in Nextcloud.

  4. Use the $uid from above to get the Access instance only for updating the user. It'S duplicated from check-user --update though. The logic from CheckUser::updateUser() might be split into a Trait and reused.

  5. For the output use $backend->getDisplayName($uid), this fixes also the hardcoded cn that might or might not be present.

  6. About naming of the command, i would strongly refrain from create-user or add-user as you might think it would create the user on the LDAP server. My suggestion would be find-user.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the time taken to help me understand how to improve the code.
I'm gonna update the PR soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After testing this new approach, I'm inclined to modify check-user to add a new option (--map or --create) which will allow to map new user from ldap.
I tested it on my instance and it seems to be a better option.

What would be the best way to propose this modification ? I would be inclined to do a new PR and cancel this one as I'm doing it all over again. What do you think about that ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, you can go this way. Point out this PR in the new one to have a reference, should somebody ever want to look up things. I would prefer --fetch or your suggested --map as flag name, I am afraid create may cause confusion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2. developing Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants