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

Fix ldap:check-user method for newly created LDAP users #30938

Merged
merged 4 commits into from
Feb 17, 2022

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Jan 31, 2022

This completes #29837 by also ignoring cache for newly created users instead of only newly deleted users.

This replaces #29837 (and #29326 and #29451) and should fix #27948

Signed-off-by: Côme Chilliet come.chilliet@nextcloud.com

@come-nc come-nc added the 3. to review Waiting for reviews label Jan 31, 2022
@come-nc come-nc added this to the Nextcloud 24 milestone Jan 31, 2022
@come-nc come-nc self-assigned this Jan 31, 2022
@come-nc come-nc requested a review from blizzz January 31, 2022 13:54
@come-nc
Copy link
Contributor Author

come-nc commented Jan 31, 2022

@goyome Does this answer your problem? Can you test it?

@come-nc come-nc requested review from a team, nickvergessen and icewind1991 and removed request for a team February 1, 2022 08:53
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 requested a review from CarlSchwan February 3, 2022 16:12
@come-nc
Copy link
Contributor Author

come-nc commented Feb 3, 2022

/rebase

@nextcloud-command nextcloud-command force-pushed the fix/user_ldap-fix-check-ldap-new-user branch from 59b5abe to 58da2ea Compare February 3, 2022 16:23
@goyome
Copy link
Contributor

goyome commented Feb 3, 2022

Hi @come-nc , thanks for the investigation but it still doesn't work for me

php occ ldap:check-user --force --update colson2i
The given user is not a recognized LDAP user.

$this->backend->userExistsOnLDAP($uid, true) returns nothing while $this->backend->loginName2UserName($uid) seems to do the job…
Sadly, I can't explain why

@come-nc
Copy link
Contributor Author

come-nc commented Feb 3, 2022

@goyome Thank you for testing.

The thing is that ldap:check-user, as most occ user related commands, is expecting an internal nextcloud username, but a non-mapped yet user has none.
So in this case you need to give the LDAP DN:

$ add_ldap_user_in_docker.sh newuser3
adding new entry "cn=newuser3,ou=people,dc=planetexpress,dc=com"
$ docker exec /nextcloud_nextcloud_1 occ ldap:check-user "cn=newuser3,ou=people,dc=planetexpress,dc=com" --update
The user is still available on LDAP.
  entryuuid: 
    44d09de6-195c-103c-99ca-c3b0c166a522
  dn: 
    cn=newuser3,ou=people,dc=planetexpress,dc=com
  uid: 
    newuser3
  mail: 
    newuser3@planetexpress.com
  cn: 
    newuser3

An other option seem to be using a search first to trigger the mapping, but this only works if the search is not yet cached:

$ add_ldap_user_in_docker.sh newuser4
adding new entry "cn=newuser4,ou=people,dc=planetexpress,dc=com"
$ docker exec /nextcloud_nextcloud_1 occ ldap:search newuser
newuser1 (newuser1)
newuser2 (newuser2)
newuser3 (newuser3)
newuser4 (newuser4)
$ docker exec /nextcloud_nextcloud_1 occ ldap:check-user newuser4 --update
The user is still available on LDAP.
  entryuuid: 
    92d0c9a8-195c-103c-99cb-c3b0c166a522
  dn: 
    cn=newuser4,ou=people,dc=planetexpress,dc=com
  uid: 
    newuser4
  mail: 
    newuser4@planetexpress.com
  cn: 
    newuser4
$ add_ldap_user_in_docker.sh newuser5
adding new entry "cn=newuser5,ou=people,dc=planetexpress,dc=com"
$ docker exec /nextcloud_nextcloud_1 occ ldap:search newuser
newuser1 (newuser1)
newuser2 (newuser2)
newuser3 (newuser3)
newuser4 (newuser4)
$ docker exec /nextcloud_nextcloud_1 occ ldap:check-user newuser5 --update
The given user is not a recognized LDAP user.

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

goyome commented Feb 4, 2022

Thanks @come-nc, that was it.
I was using uid instead of dn but the help mislead me.

Usage:
  ldap:check-user [options] [--] <ocName>

Arguments:
  ocName                the user name as used in Nextcloud

Maybe you should specify that we have to use dn in the description of the --update option or use element of config (ldap_base_users and ldap_login_filter/ldap_expert_username_attr) to create the dn from the uid

@come-nc come-nc force-pushed the fix/user_ldap-fix-check-ldap-new-user branch from b08719f to 26b9ef5 Compare February 7, 2022 09:35
@come-nc
Copy link
Contributor Author

come-nc commented Feb 7, 2022

@blizzz Could you have review on this one, because in the end I had to change a bit fetching to make sure it worked with a user DN as for a non-yet-mapped user there is no owncloud_name yet.
It worked before the last commit already, but a bit by chance, because some functions are supposed to take an owncloud_name but are actually working with a dn as well. So I added an explicit DN fetch to make it clean and future proof.

come-nc and others added 4 commits February 17, 2022 09:58
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
…ser.php

Co-authored-by: Carl Schwan <carl@carlschwan.eu>

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the fix/user_ldap-fix-check-ldap-new-user branch from 26b9ef5 to 44680b5 Compare February 17, 2022 09:01
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit f5d1365 into master Feb 17, 2022
@PVince81 PVince81 deleted the fix/user_ldap-fix-check-ldap-new-user branch February 17, 2022 16:20
@come-nc
Copy link
Contributor Author

come-nc commented Feb 21, 2022

/backport to stable23

@come-nc
Copy link
Contributor Author

come-nc commented Feb 21, 2022

/backport to stable22

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.

Force sync a ldap user through command line or API
4 participants