Skip to content

Refactor user_ldap group membership cache and add check-group command#39446

Merged
come-nc merged 17 commits intomasterfrom
enh/user_ldap-refactor-group-membership-cache
Aug 10, 2023
Merged

Refactor user_ldap group membership cache and add check-group command#39446
come-nc merged 17 commits intomasterfrom
enh/user_ldap-refactor-group-membership-cache

Conversation

@come-nc
Copy link
Contributor

@come-nc come-nc commented Jul 18, 2023 •
edited
Loading

Summary

Refactor user_ldap group membership cache, to be able to easily check if a membership is known or not.
Add a command check-group which works like check-user, with the same --update option to force update from LDAP.

TODO

  • Figure out if indexes are needed on the new table
  • Improve command output (maybe use event listeners?)
  • Make sure command ignores cache for --update (currently not the case)

Checklist

github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 18, 2023
View reviewed changes
apps/user_ldap/lib/Db/GroupMembership.php
* @method void setGroupid(string $groupid)
* @method string getGroupid()
*/
class GroupMembership extends Entity {

Check notice

Code scanning / Psalm

PropertyNotSetInConstructor

Property OCA\User_LDAP\Db\GroupMembership::$id is not defined in constructor of OCA\User_LDAP\Db\GroupMembership or in any methods called in the constructor
apps/user_ldap/lib/Db/GroupMembership.php
*/
class GroupMembership extends Entity {
/** @var string */
protected $groupid;

Check notice

Code scanning / Psalm

PropertyNotSetInConstructor

Property OCA\User_LDAP\Db\GroupMembership::$groupid is not defined in constructor of OCA\User_LDAP\Db\GroupMembership or in any methods called in the constructor
apps/user_ldap/lib/Db/GroupMembership.php
protected $groupid;

/** @var string */
protected $userid;

Check notice

Code scanning / Psalm

PropertyNotSetInConstructor

Property OCA\User_LDAP\Db\GroupMembership::$userid is not defined in constructor of OCA\User_LDAP\Db\GroupMembership or in any methods called in the constructor
@come-nc come-nc force-pushed the enh/user_ldap-refactor-group-membership-cache branch from 173062a to 33ad29a Compare July 20, 2023 10:56
@come-nc come-nc self-assigned this Jul 20, 2023
@come-nc come-nc added this to the Nextcloud 28 milestone Jul 20, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 20, 2023
View reviewed changes
@come-nc come-nc force-pushed the enh/user_ldap-refactor-group-membership-cache branch from bce4a6e to bb584c2 Compare July 20, 2023 14:44
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 20, 2023
View reviewed changes
github-advanced-security[bot]
github-advanced-security bot found potential problems Jul 20, 2023
View reviewed changes
@come-nc
Copy link
Contributor Author

come-nc commented Jul 20, 2023 •
edited
Loading 

{
"level":3,
"message":"Table \"oc_ldap_group_membership\" has no primary key and therefor will not behave sane in clustered setups. This will throw an exception and not be installable in a future version of Nextcloud.",
}
  • Add primary key

@come-nc come-nc changed the title WIP - refactor user_ldap group membership cache and add check-group c… Refactor user_ldap group membership cache and add check-group command Jul 20, 2023
@come-nc come-nc force-pushed the enh/user_ldap-refactor-group-membership-cache branch from c6b9696 to c3c770a Compare August 1, 2023 10:23
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 1, 2023
View reviewed changes
apps/user_ldap/lib/Command/CheckGroup.php

protected function execute(InputInterface $input, OutputInterface $output): int {
$this->dispatcher->addListener(GroupCreatedEvent::class, fn ($event) => $this->onGroupCreatedEvent($event, $output));
$this->dispatcher->addListener(UserAddedEvent::class, fn ($event) => $this->onUserAddedEvent($event, $output));

Check notice

Code scanning / Psalm

ArgumentTypeCoercion

Argument 1 of OCA\User_LDAP\Command\CheckGroup::onUserAddedEvent expects OCP\Group\Events\UserAddedEvent, but parent type OCP\EventDispatcher\Event provided
apps/user_ldap/lib/Command/CheckGroup.php
protected function execute(InputInterface $input, OutputInterface $output): int {
$this->dispatcher->addListener(GroupCreatedEvent::class, fn ($event) => $this->onGroupCreatedEvent($event, $output));
$this->dispatcher->addListener(UserAddedEvent::class, fn ($event) => $this->onUserAddedEvent($event, $output));
$this->dispatcher->addListener(UserRemovedEvent::class, fn ($event) => $this->onUserRemovedEvent($event, $output));

Check notice

Code scanning / Psalm

ArgumentTypeCoercion

Argument 1 of OCA\User_LDAP\Command\CheckGroup::onUserRemovedEvent expects OCP\Group\Events\UserRemovedEvent, but parent type OCP\EventDispatcher\Event provided
@come-nc
Copy link
Contributor Author

come-nc commented Aug 1, 2023 •
edited
Loading

  • It only works if using full dn for non-mapped groups, would be better if using name works as well (for user it’s complicated but for groups it might be simpler to do that)
  • It does not seem to correctly fire the group created event when passed a full dn of a new group

@come-nc come-nc force-pushed the enh/user_ldap-refactor-group-membership-cache branch from c3c770a to 8c80749 Compare August 1, 2023 16:05
@come-nc come-nc added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 3, 2023
@come-nc come-nc requested a review from Altahrim August 3, 2023 11:59
github-advanced-security[bot]
github-advanced-security bot found potential problems Aug 3, 2023
View reviewed changes
apps/user_ldap/lib/Command/CheckGroup.php
}

protected function execute(InputInterface $input, OutputInterface $output): int {
$this->dispatcher->addListener(GroupCreatedEvent::class, fn ($event) => $this->onGroupCreatedEvent($event, $output));

Check notice

Code scanning / Psalm

ArgumentTypeCoercion

Argument 1 of OCA\User_LDAP\Command\CheckGroup::onGroupCreatedEvent expects OCP\Group\Events\GroupCreatedEvent, but parent type OCP\EventDispatcher\Event provided
@come-nc
Copy link
Contributor Author

come-nc commented Aug 3, 2023

The GroupCreatedEvent firing is not completely working, but since GroupDeletedEvent is not fired I’m letting that for a follow-up PR.

@come-nc come-nc marked this pull request as ready for review August 3, 2023 15:22
@come-nc come-nc requested a review from blizzz as a code owner August 3, 2023 15:22
@come-nc come-nc requested review from a team, icewind1991 and nfebe and removed request for a team August 3, 2023 15:23
Altahrim
Altahrim reviewed Aug 4, 2023
View reviewed changes
@come-nc
Copy link
Contributor Author

come-nc commented Aug 7, 2023

Note: I looked into using LazyUser for events, but the test expects the event to only fire for existing users, which makes sense I suppose, so I abandoned the idea.

@come-nc come-nc requested a review from Altahrim August 8, 2023 13:56
come-nc added 17 commits August 10, 2023 10:57
@come-nc
Refactor user_ldap group membership to use flat DB
d8142b6 
Move away from serialized arrays. Also use a QBMapper class for the new table.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Update autoloader mapping for user_ldap
d9b3680 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add group events to UpdateGroups
34fa413 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Move UpdateGroups methods to a service
2c19aac 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add check-group command
f9ed48e 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add missing primary key for ldap_group_membership
ec13f22 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix errors in UpdateGroupsService.php
1b102ca 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Always empty cache before updating a group
ce5a4e5 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add output to check-group --update command
7a14aa7 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix getKnownGroups return value
42448c0 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix check-group command for new groups
1026b21 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
[user_ldap] Small perf improvements suggestion from review
c33c40f 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Dispatch UserRemovedEvents for removed groups
ad1e487 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Removed GroupCreatedEvent/GroupDeletedEvent from UpdateGroupsService
7732de7 
This should be handled when mapping groups, not when registering their
 members. An empty group may still exist.

Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Use BIGINT for new table id field
5425f7d 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Fix check-group --update for deleted groups
b8a0954 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Add unique index for user_ldap group memberships
a080811 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc
Copy link
Contributor Author

come-nc commented Aug 10, 2023

Rebased and squashed

@come-nc come-nc force-pushed the enh/user_ldap-refactor-group-membership-cache branch from 1d07a76 to a080811 Compare August 10, 2023 08:57
@come-nc come-nc merged commit 7d01b96 into master Aug 10, 2023
@come-nc come-nc deleted the enh/user_ldap-refactor-group-membership-cache branch August 10, 2023 15:22
@come-nc come-nc added the pending documentation This pull request needs an associated documentation update label Aug 14, 2023
@come-nc come-nc mentioned this pull request Aug 28, 2023
Merged
This was referenced Sep 11, 2023
Merged
Merged
@come-nc come-nc removed the pending documentation This pull request needs an associated documentation update label Nov 13, 2023
This was referenced Feb 14, 2024
Closed
Closed
@come-nc come-nc mentioned this pull request Mar 28, 2024
Open
8 tasks
@joshtrichards joshtrichards mentioned this pull request Sep 23, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@AndyScherzinger AndyScherzinger AndyScherzinger approved these changes

@Altahrim Altahrim Altahrim approved these changes

@blizzz blizzz Awaiting requested review from blizzz blizzz is a code owner

@icewind1991 icewind1991 Awaiting requested review from icewind1991 icewind1991 is a code owner automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

Assignees

@come-nc come-nc

Labels

3. to review Waiting for reviews enhancement

Projects

None yet

Milestone

Nextcloud 28

Development

Successfully merging this pull request may close these issues.

Force sync a ldap GROUP through command line or API

3 participants

@come-nc @AndyScherzinger @Altahrim