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 multiple LDAP configuration support by fixing AccessFactory #37903

Merged
merged 3 commits into from
May 2, 2023

Conversation

come-nc
Copy link
Contributor

@come-nc come-nc commented Apr 24, 2023

Summary

PR #34772 broke the fix from PR #35070 because AccessFactory was reusing the same Manager instance for all access instances.

Checklist

It must not reuse the same OCA\User_LDAP\User\Manager instance for
 several Access instances.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added bug 3. to review Waiting for reviews labels Apr 24, 2023
@come-nc come-nc self-assigned this Apr 24, 2023
@come-nc come-nc requested a review from blizzz April 24, 2023 14:07
@come-nc come-nc added this to the Nextcloud 27 milestone Apr 24, 2023
@come-nc
Copy link
Contributor Author

come-nc commented Apr 24, 2023

/backport to stable26

@come-nc come-nc requested review from a team, ArtificialOwl and icewind1991 and removed request for a team April 24, 2023 14:09
@come-nc

This comment was marked as outdated.

@evilham
Copy link
Contributor

evilham commented Apr 26, 2023

Tested this patch, it solves the issue on NC 26

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc requested review from artonge and szaimen April 27, 2023 07:18
Copy link
Contributor

@szaimen szaimen left a comment

Choose a reason for hiding this comment

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

🐘

@@ -55,7 +53,7 @@ public function get(Connection $connection): Access {
return new Access(
$connection,
$this->ldap,
$this->userManager,
Server::get(Manager::class),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add a comment to explain why we do not use DI? It will help to not revert in the future.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Apr 27, 2023
@come-nc come-nc enabled auto-merge April 27, 2023 09:22
@come-nc come-nc disabled auto-merge May 2, 2023 15:10
@come-nc come-nc merged commit c995428 into master May 2, 2023
@come-nc come-nc deleted the fix/user_ldap-fix-multiple-ldap-support branch May 2, 2023 15:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Multiple Ldap doesn't working
5 participants