Fix login as non ldap user

[icewind1991]

cc @blizzz

[mention-bot]

@icewind1991, thanks for your PR! By analyzing the annotation information on this pull request, we identified @blizzz, @MorrisJobke and @dschmidt to be potential reviewers

[MorrisJobke]

The login works for me even without this change. Could you please comment what this change will address?

[icewind1991]

I'm not sure about the exact reason but when trying to make a dav request for a non ldap user (while having ldap users configured) $user ends up being null which currently breaks login

[MorrisJobke]

@blizzz Any comment on this?

[blizzz]

I am bewildered, need to verify.

[blizzz]

@icewind1991 I cannot verify the issue

$ cadaver http://nc.zara/master/remote.php/webdav
Authentication required for Nextcloud on server `nc.zara':
Username: master
Password: 
dav:/master/remote.php/webdav/> ls
Listing collection `/master/remote.php/webdav/': succeeded.
        welcome.txt                          164  Aug 19 12:26

do you have detailed reproduction steps?

[icewind1991]

No, it probably depends on the specific ldap setup but I don't have enough ldap knowledge to know why

[blizzz]

What might happened here is that the provided loginname resulted into an LDAP result when looking up a user for the provided loginname. Then there are only few cases were it could fail, e.g. there is no displayname set on LDAP for that user (should result in a log line like No or empty name for $dn) or a new mapping would not succeed (includes trying alternate names), would result in a log line like Could not create unique name for $dn.

But because we do not check for null in that case it does not gracefully deny login for that LDAP user, but ends up with an error.

The check could be reverted to only look for OCA\User_LDAP\User resulting in just one condition, the effect would be the same however. Typically, the result would be a user one so changing it accordingly might be a bit of micro optimization.

[MorrisJobke]

@icewind1991 Do you still have an instance where this is reproducible? Otherwise I would close this, because we can't reproduce it locally. Also fixing symptoms is not that good.

[MorrisJobke]

No response yet and this doesn't seem to be reproucible -> closing

[MorrisJobke]

@icewind1991 feel free to reopen once you know more about this issue 😉