fix(user_ldap): sync users even when no display name is set

[eyJhb]

• Resolves: New ldap users can't login: LDAP Login: Could not get user object for DN ... Maybe the LDAP entry has no set display name attribute? #39751

Summary

Removes the check for display name when doing batchApplyUserAttributes, as that is no longer a bug anymore (it was first introduced in owncloud/core#20804 ). This works well in combination with #46114 , which also fixes the enforcement of display name + it fixes logic that was applied for both users and groups, even though it only applied to groups.

I think this PR ,as well as the previous linked one, we will be able to close this issue #39751 , which is largely misconfiguration, however, there is no need for enforcing display name anymore.

Opening this in the hope of getting someone smarter to say if this is a good/bad idea. Spent most of a day debugging this, and the codebase is still very large to navigate for me.

I'm thinking @come-nc will have some insight this area :) Hopefully tagging is not frowned upon.

TODO

• Should be created as a backbort, as fix(mail): Log hint when mail_from_address may be wrong #46040 also has.

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
• Backports requested where applicable (ex: critical bugfixes)

[provokateurin]

Unfortunately I have no idea about LDAP 🙈

[come-nc]

I’m not against this per-se but as far as I know display name is mandatory on purpose historically.
For instance see getFilterForUserCount, only users with a display name are counted in user count.

I’m not sure what good it does to merge this, since having a badly configured displayName will cause trouble in other places, no?

[eyJhb]

Yeah, other places should ideally be changed as well. But I don't think display name is required, based on when you create a new user, it isn't even required there.

I ran the patch above + the patch you made (somewhat), and didn't really encounter any issues. Bet generally, the weird requirement that display name is required, doesn't seem to be no more. And even if it is, it fails silently all over the place.

[come-nc]

Would need @blizzz input for the historic there, but then if we allow it we would need to allow it everywhere and count these users in the user count as well.

[eyJhb]

That would make sense yeah. I haven't looked at the other auth providers, so I have no clue how they work. My initial reason for having to investigate this, was that I had somehow made users which had a display name, but then I have removed the display name for LDAP.

This gives a broken state on Netxcloud, as it will refuse to sync with ldap for the users with no display name (even though they are IN the system), e.g. only half of my users had the ldap quota updated. So surely, even this change would be nice to have in such cases :) - I should note, that I didn't experience any issues when using this PR, but then again, I've only played around with the LDAP codebase, and I have nowhere near the insight into Nextcloud like you all have.

I see I had initially misread this PR #46114 , as I can see it now enforces the use of display name even further than before. From my testing, it was generally the function dn2ocname which would result in a usermanager->get($uid), to return no user, if no display name was set. This became apparent for me when I was using OIDC, and it would refuse to find the user here https://github.com/pulsejet/nextcloud-oidc-login/blob/master/lib/Service/LoginService.php#L245

I really appreciate you all taking the time to discuss this, and the fast responses on the matter. No matter the outcome :)

[blizzz]

Would need @blizzz input for the historic there, but then if we allow it we would need to allow it everywhere and count these users in the user count as well.

This is THE most reliable (generic) way to filter out system accounts 🙊

I would say, do it with a config flag only, but really I am not convinced it fixes a bug, rather hides configuration mistakes?

[eyJhb]

Would need @blizzz input for the historic there, but then if we allow it we would need to allow it everywhere and count these users in the user count as well.

This is THE most reliable (generic) way to filter out system accounts 🙊

I would say, do it with a config flag only, but really I am not convinced it fixes a bug, rather hides configuration mistakes?

I understand that it is one way to filter away the system accounts, but shouldn't that be up to the user to configure that correctly with the filter options provided? Instead what this does, is have a kind of hidden requirement that isn't obvious until deep-diving into the code. If this is how it should be intended to work, then there need to be more logging, so that it is much easier to debug and find, instead of somewhat just "silently" failing to do what I expected at least, sync my users (and also in my case, only syncing half? Because it had somewhat synced some of the users initially??).

If it is a bug or not, is very much up in the air atm. as it can be view as both things. I'm not knee-deep in the Nextcloud code, but for me it seems like there is no requirement anymore for display name, and that the user should manage themselves if the filter based on the display name themselves :)

[blizzz]

Instead what this does, is have a kind of hidden requirement that isn't obvious until deep-diving into the code.

I don't want to say to read the documentation, but it is actually stated there 😅 : https://docs.nextcloud.com/server/latest/admin_manual/configuration_user/user_auth_ldap.html#note

If this is how it should be intended to work, then there need to be more logging, so that it is much easier to debug and find, instead of somewhat just "silently" failing to do what I expected at least, sync my users (and also in my case, only syncing half? Because it had somewhat synced some of the users initially??)

It is logged with debug level: https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/Access.php#L582

[eyJhb]

I don't want to say to read the documentation, but it is actually stated there 😅 : https://docs.nextcloud.com/server/latest/admin_manual/configuration_user/user_auth_ldap.html#note

Yeah, I found it there after quite some looking around (not the documentation, the source code :p). I actually thought I had mentioned that originally in here, but most likely it was my draft comment on the issue instead. I will say, I never looked at the documentation initially, as the interactive setup (LDAP) in the UI is quite intuitive, and I never considered that such a limitation would be put into place :D (but that will teach me to assume things)

It is logged with debug level: https://github.com/nextcloud/server/blob/master/apps/user_ldap/lib/Access.php#L582

Would that include already synced users? E.g. if the batch sync function is called (the one I patch here), would that log line come out? (it somewhat feels more of a warning than debug as well) For some reason I can't get debug logs out (unrelated).

But I have nothing against keeping it as is, but I think it's a limitation that was put into place a long time ago, when there were other limitations of Nextcloud than there is now. And it would be much cleaner, if the administrator filtered on display name, instead of handling it behind the scenes. But then again, I'm not the one who will end up doing support on it in the future, or when users start showing up complaining about all their system users suddenly showing up 😅 (and in the end, it will take more than JUST what I made now. A couple of more queries should be changed, and it should be ensured that it doesn't break anything e.g. larger scope than what I just showed).

EDIT: I don't think we need to drag this out further, I'm happy with the replies I've gotten. So I think @blizzz can say the final words, and we can close this PR (which in hindsight maybe should have been an issue), or work towards what we want. My goal is not to waste your time in here, and hopefully it's not viewed as such ! :)

[blizzz]

I never looked at the documentation initially, as the interactive setup (LDAP) in the UI is quite intuitive, and I never considered that such a limitation would be put into place :D (but that will teach me to assume things)

😊 We take that as both a compliment, and pointing out our requirement as a hint in the configuration as good suggestion for an improvement! Thanks!

Would that include already synced users?

This would be logged again and again when a candidate cannot be mapped; but if the record "looses" its displayname afterwards, then not anymore i think 🤔

So I think @blizzz can say the final words, and we can close this PR (which in hindsight maybe should have been an issue), or work towards what we want. My goal is not to waste your time in here, and hopefully it's not viewed as such ! :)

Not viewed as such, thank you for your time and effort!

[eyJhb]

Equally thank you for your time in reviewing this, and patiently answering my questions and comments. Highly appreciated! Feels like a very welcoming community. Keep up the good work! (10/10 would return again, if I find something to contribute with :) )

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!

(If you believe you should not receive this message, you can add yourself to the blocklist.)