do not trigger counting on LDAP #10610
Conversation
Noted ✍️. |
|
I think we should also just hide the counter in the frontend then, otherwise it could be confusing that the number differs from the number of listed users. Do we also have that slow counting issue with other user backends? Then we should aim for a more general approach like disable counting for all external user backends for example. |
|
@juliushaertl which number to do you refer to? in the sidebar, none will be shown. |
Yes, at least on my test instance it shows the number of local users is shown (for the everyone group). But that is a small design issue we can fix afterwards.
At least user_saml app doesn't implement the countUsers action on the UserBackend so it will not be triggered there. I'm fine with this for now. We can still find a more generic way if we encounter further issues with other backends. |
| @@ -43,6 +43,7 @@ | |||
| use OC\AppFramework\Http; | |||
| use OC\ForbiddenException; | |||
| use OC\Security\IdentityProof\Manager; | |||
| use OCA\User_LDAP\User_Proxy; | |||
There was a problem hiding this comment.
Not sure about this, it also seems to work with the user_ldap app enabled. Is it always available though autoloading or are there cases when this might fail?
There was a problem hiding this comment.
When I tested it, it worked for me with both user_ldap enabled and disabled. In the logic, the class is only referred to when user_ldap was found enabled.
There was a problem hiding this comment.
Ok fine by me then, worked here as well.
There was a problem hiding this comment.
@juliushaertl mind updating your review?
This should not be the case. The counting goes per general user manager, and it should not happen at all.
We should aim for a proper way, yes, it's just it is a bit late for 14 to introduce new Interfaces, e.g. |
|
There is an interface for groups. https://github.com/nextcloud/server/tree/master/lib/public/Group/Backend |
| $isLDAPUsed = (bool)array_reduce($this->userManager->getBackends(), function ($ldapFound, $backend) { | ||
| return $ldapFound || $backend instanceof User_Proxy; | ||
| }); | ||
| $this->userManager->getBackends(); |
There was a problem hiding this comment.
chilling around. thanks for the catch.
it's probably too many, degregading performance Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
blizzz
force-pushed
the
fix/noid/dontcountwithldap
branch
from
d83b05d to
1b74bfc
Compare
Expected:
Actual:
this fixes it.
(Yes, it also mixed OCA space into OC, it was already the case though 🙊 🙈 )