fix(LDAP): unblock a cached id from a deleted user or group #56292
Conversation
blizzz
requested review from
ArtificialOwl and
icewind1991
and removed request for
a team
|
/backport to stable32 |
blizzz
force-pushed
the
fix/noid/ldap-regenerate-deleted-user
branch
from
c9f26d9 to
bf0cc66
Compare
As of 155b750 we cache the user id and LDAP DN for a month, because they are highly requested, but rarely change and we have a working repair mechanism. But if an LDAP user is deleted, the information is still cached. So a user that is being regenerated with the same target user ID will get a _xxxx suffix, when the data is still in local cache. Signed-off-by: Arthur Schiwon <[email protected]>
blizzz
force-pushed
the
fix/noid/ldap-regenerate-deleted-user
branch
from
bf0cc66 to
5ce8e06
Compare
| $dude = $this->ncUserManager->get($intName); | ||
| $dude->getBackendClassName() === 'LDAP'; | ||
|
|
There was a problem hiding this comment.
| $dude = $this->ncUserManager->get($intName); | |
| $dude->getBackendClassName() === 'LDAP'; |
left-over?
| // DN might be cached if the user was deleted within last month. | ||
| // If it was a valid user we would have returned earlier at the | ||
| // $mapper->getNameByDN($fdn) call, which does not run against the cache. | ||
| || $this->ncUserManager->get($intName)?->getBackendClassName() === 'LDAP' |
There was a problem hiding this comment.
I do not understand the getNameByDN part of the comment.
I do not see what prevents collisions with this PR.
If I set username to a non-unique LDAP attribute, with this PR I will run into trouble no?
Would be way cleaner to either clear on ignore the cache in this code path.
There was a problem hiding this comment.
Yeah, this is the problem, it relies on certain behavior inside the Mappings class. More later.
If I set username to a non-unique LDAP attribute, with this PR I will run into trouble no?
No, I don't see why you would.
Would be way cleaner to either clear on ignore the cache in this code path.
I don't want to clear the whole cache, this is pointless for an edge case while the mechanism as a whole is quite effective!
The memcache uses the uid as key (we don't have it) and is not really searchable. So without making the structure bigger and more redundant, which should be avoided, we can make the mapper more complicated:
- remember all DNs that do not resolve in a name (runtime only)
- if we encounter a cache hit involving such a DN, reset the cache entry and discard the value
What I don't like about this approach is that cache logic and mechanism are spread along the mapping. But it is better than the current approach, which relies on certain behaviour in different classes (this gives me the stomach aches tbh).
The good thing is that wrong values don't hurt, they'll be repaired (as long as the cached DN is not-existing that is).
There was a problem hiding this comment.
If I set username to a non-unique LDAP attribute, with this PR I will run into trouble no?
No, I don't see why you would.
To me it looks like this change defeats the collision detection mechanism.
If a user already exists with the same uid, it will happily return it even if it’s not the same one. Or did I miss where there is a check that the existing user and the new one are the same?
There was a problem hiding this comment.
Not that still works:
Also works through web interface (with memcache in action).
Reason is that mapAndAnnounceIfApplicable() will return false due to database constraints (cf. AbstractMapping::map()).
There was a problem hiding this comment.
Nevertheless, would still tend to move it into the mapping. I don't like the reliance on inner workings of other modules, as stated in #56292 (comment).
Summary
As of 155b750 we cache the user id and LDAP DN for a month, because they are highly requested, but rarely change and we have a working repair mechanism.
But if an LDAP user is deleted, the information is still cached. So a user that is being regenerated with the same target user ID will get a _xxxx suffix, when the data is still in local cache.
The error case can be reproduced:
occ user:info 88ce7e0d-cd84-46ab-99c9-d49c1b2aba48shows info about that userocc ldap:reset-user 88ce7e0d-cd84-46ab-99c9-d49c1b2aba48occ user:info 88ce7e0d-cd84-46ab-99c9-d49c1b2aba48shows that the user does not existoc_ldap_user_mapping, you will find a mapping with a _1234 suffix.Checklist
3. to review, feature component)stable32)