user_ldap icon indicating copy to clipboard operation
user_ldap copied to clipboard

inGroup and getUserGroups have different cache keys and implementations

Open PVince81 opened this issue 9 years ago • 11 comments

I'd expect Group_LDAP::inGroup to simply use Group_LDAP::getUserGroups or at least use the same cache keys.

This means that it could happen that the cache contains slightly different values, leading to bugs like https://github.com/owncloud/core/issues/26683 where one API which uses getUserGroups() sees that the user is member of a group, but inGroup says the user isn't.

@jvillafanez @owncloud/ldap

PVince81 avatar Nov 28 '16 17:11 PVince81

From what I see the cache would be memcache

PVince81 avatar Nov 28 '16 17:11 PVince81

I tried to force clear partially the cache and found out that inGroup also called _groupMembers which itself stores the same information in yet another cache key "_groupMembers" . $dnGroup

PVince81 avatar Nov 28 '16 17:11 PVince81

and "inGroup-members" ...

PVince81 avatar Nov 28 '16 17:11 PVince81

Here are steps to reproduce, it is a bit difficult because it needs a special sharing scenario: https://github.com/owncloud/core/issues/26683#issuecomment-263346186

It is likely that we can find other easier code path that would trigger inGroup calls.

PVince81 avatar Nov 28 '16 18:11 PVince81

Also observed on a real env where groupExists would return false but getUserGroups still returns the membership... Broke some stuff with sharing.

PVince81 avatar Dec 23 '16 11:12 PVince81

We need to rewrite this caching thing to make more sense and only cache a single info once.

PVince81 avatar Dec 23 '16 11:12 PVince81

@IljaN I'd like you to have a look at this.

You can setup a test LDAP docker using these scripts: https://github.com/owncloud/administration/tree/master/ldap-testing (start.sh then run the zombie spawner scripts).

See inside the scripts for the LDAP credentials.

You'll also need to setup a memcache in ownCloud. See https://github.com/owncloud/core/issues/26683#issuecomment-263346186 for steps to reproduce.

Let me know if you need more info about the setup, how to setup LDAP or details about this issue. Thanks!

PVince81 avatar Jan 18 '17 09:01 PVince81

Postponed until https://github.com/owncloud/core/issues/23558 is done. Memcache usage will be obsoleted by the introduction of the central user+group account table"

IljaN avatar Jan 25 '17 11:01 IljaN

Due to the nature of how caching is done here using hashing keys, it is not possible to cache everything only in a single location because we can't look up / search hashed values. (ex: find all groups in which user1 is in)

PVince81 avatar Feb 15 '17 10:02 PVince81

The user account table is in 10.0 but we'll also need a group table to be able to resolve this.

PVince81 avatar Apr 11 '17 14:04 PVince81

@jvillafanez is the new adjusted LDAP impl still using memcache for anything ?

PVince81 avatar May 17 '17 06:05 PVince81