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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 17 commits into from
Aug 10, 2023
Merged

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

merged 17 commits into from
Aug 10, 2023

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/Group_Proxy.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Migration/Version1190Date20230706134108.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Migration/Version1190Date20230706134108.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Command/CheckGroup.php Fixed Show fixed Hide fixed
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
apps/user_ldap/lib/Db/GroupMembershipMapper.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
apps/user_ldap/lib/Jobs/UpdateGroups.php Fixed Show fixed Hide fixed
@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
@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 Fixed Show fixed Hide fixed
apps/user_ldap/lib/Command/CheckGroup.php Fixed Show fixed Hide fixed
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
apps/user_ldap/lib/Db/GroupMembershipMapper.php Fixed Show fixed Hide fixed
@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
@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 <[email protected]>
@come-nc
Update autoloader mapping for user_ldap
d9b3680 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Add group events to UpdateGroups
34fa413 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Move UpdateGroups methods to a service
2c19aac 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Add check-group command
f9ed48e 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Add missing primary key for ldap_group_membership
ec13f22 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Fix errors in UpdateGroupsService.php
1b102ca 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Always empty cache before updating a group
ce5a4e5 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Add output to check-group --update command
7a14aa7 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Fix getKnownGroups return value
42448c0 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Fix check-group command for new groups
1026b21 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
[user_ldap] Small perf improvements suggestion from review
c33c40f 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Dispatch UserRemovedEvents for removed groups
ad1e487 
Signed-off-by: Côme Chilliet <[email protected]>
@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 <[email protected]>
@come-nc
Use BIGINT for new table id field
5425f7d 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Fix check-group --update for deleted groups
b8a0954 
Signed-off-by: Côme Chilliet <[email protected]>
@come-nc
Add unique index for user_ldap group memberships
a080811 
Signed-off-by: Côme Chilliet <[email protected]>
@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 was automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe nfebe was 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