Skip to content

Conversation

@blizzz
Copy link
Member

@blizzz blizzz commented Oct 29, 2025

Summary

The migration path is hard and unexpected. We can follow-up with a soft setup check instead for a while, but needs to be discussed.

Checklist

The migration path is hard and unexpected. We can follow-up with a soft
setup check instead for a while, but needs to be discussed.

Signed-off-by: Arthur Schiwon <[email protected]>
@blizzz blizzz added this to the Nextcloud 33 milestone Oct 29, 2025
@blizzz blizzz requested review from artonge and come-nc October 29, 2025 17:22
@blizzz blizzz requested a review from a team as a code owner October 29, 2025 17:22
@blizzz blizzz added the bug label Oct 29, 2025
@blizzz blizzz requested review from salmart-dev and yemkareems and removed request for a team October 29, 2025 17:22
@blizzz blizzz added 3. to review Waiting for reviews feature: ldap labels Oct 29, 2025
@blizzz
Copy link
Member Author

blizzz commented Oct 29, 2025

/backport to stable32

@blizzz blizzz merged commit 5060d64 into master Oct 30, 2025
197 of 200 checks passed
@blizzz blizzz deleted the fix/55613/drop-hard-base-checks branch October 30, 2025 13:56
@hamza221
Copy link
Contributor

the private function checkBasesAreValid is still there and not being used anywhere else

@blizzz
Copy link
Member Author

blizzz commented Oct 30, 2025

the private function checkBasesAreValid is still there and not being used anywhere else

might still be useful, going to check with @come-nc next week.

@come-nc
Copy link
Contributor

come-nc commented Nov 3, 2025

@blizzz So I checked the code a bit more and ldapBase is actually almost never used, apart from counting objects to test its value, and as default value for the other bases in the wizard. So it could even be removed and replaced by a test field.

I guess it’s fine to simply revert the check for now as bad values in this field are not important in the end. We should just make sure it’s never used as the actual base of the LDAP in the future.

Ideally we should inverstigate why a failing check caused so much trouble, especially the memory limit issues are concerning. But also I think most people failed to understand what the error meant and that the base field is actually multi-valued.

@blizzz
Copy link
Member Author

blizzz commented Nov 3, 2025

@blizzz So I checked the code a bit more and ldapBase is actually almost never used, apart from counting objects to test its value, and as default value for the other bases in the wizard. So it could even be removed and replaced by a test field.

I guess it’s fine to simply revert the check for now as bad values in this field are not important in the end. We should just make sure it’s never used as the actual base of the LDAP in the future.

👍 Yeah, as said, it is part of the cozy-easy-setup for non-LDAP-knowledgable people and basically copied over as user and group tree quite soonish. I don't think it was ever used for anything else?

Ideally we should inverstigate why a failing check caused so much trouble, especially the memory limit issues are concerning. But also I think most people failed to understand what the error meant and that the base field is actually multi-valued.

Though I was curious I did not have time too look into what the check was doing. Gut feeling said it looked like recursing endlessly, did not really look into it though. But struck me odd.

@nextcloud-bot nextcloud-bot mentioned this pull request Jan 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Nextcloud 32 breaks LDAP for some overlay directory schemas

6 participants