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 option to fetch specific user from LDAP #29451

Closed
wants to merge 3 commits into from

Conversation

goyome
Copy link
Contributor

@goyome goyome commented Oct 27, 2021

Signed-off-by: Guillaume COLSON guillaume.colson@univ-lorraine.fr
Address issue #27948
This PR is meant to replace #29326 after discussing about it with @blizzz

Signed-off-by: Guillaume COLSON <guillaume.colson@univ-lorraine.fr>
@szaimen szaimen added the 3. to review Waiting for reviews label Oct 27, 2021
@szaimen szaimen requested review from blizzz, CarlSchwan, a team and juliusknorr and removed request for a team October 27, 2021 08:04
@szaimen szaimen added this to the Nextcloud 24 milestone Oct 27, 2021
;
}

protected function execute(InputInterface $input, OutputInterface $output): int {
try {
$uid = $input->getArgument('ocName');
$this->isAllowed($input->getOption('force'));
if ($input->getOption('fetch')) {
$uid = $this->backend->loginName2UserName($uid);
Copy link
Member

Choose a reason for hiding this comment

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

check whether loginName2UserName() returned false – this is when the user was not found. Probably best to exit with a message in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case, we fall back to the default behavior.
As the user isn't mapped, the next call to $this->confirmUserIsMapped($uid); will trigger the error
The given user is not a recognized LDAP user.

I thought that it would be enough. What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

Since --fetch has to be proactively used, my expectancy was that it really and only tries to fetch the user from LDAP first. The fallback might be undesired here (even if it does not hurt). Exiting with a new return code (2) is the best way to deal with this imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a commit to implement what you suggested.

Signed-off-by: Guillaume COLSON <guillaume.colson@univ-lorraine.fr>
Signed-off-by: Guillaume COLSON <guillaume.colson@univ-lorraine.fr>
@come-nc
Copy link
Contributor

come-nc commented Dec 21, 2021

I think this is obsolete because of #29837 which is already merged.
@goyome Can you confirm?

@goyome
Copy link
Contributor Author

goyome commented Jan 12, 2022

@come-nc Thanks for your improvement and sorry for the late reply. I had few times to make the appropriate tests.

I tried to fetch a new ldap user with
php occ ldap:check-user --update new_user
but the script stop with the error
The given user is not a recognized LDAP user.
which is returned by the function $this->confirmUserIsMapped($uid); which is executed before $exists = $this->backend->userExistsOnLDAP($uid, true);

Anyway, I tried to move the call to confirmUserIsMapped after the update but no changes.

Maybe you should add a call to $this->backend->loginName2UserName($uid) in the update call instead of adding the new fetch option but to me the functionnality needs to be implemented in one way or the other.

@come-nc
Copy link
Contributor

come-nc commented Jan 13, 2022

@goyome I think I tested check-ldap for newly-deleted users but not for newly-created users.

When I find time I will look into fixing this as well.

@goyome
Copy link
Contributor Author

goyome commented Jan 13, 2022

Thanks a lot 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants