From d84a19ac5cb47c9cdf355610b87ebc21080c44b5 Mon Sep 17 00:00:00 2001 From: Jonas Date: Mon, 18 Nov 2024 18:05:09 +0100 Subject: [PATCH 1/3] fix(CoreQueryBuilder): Use correct member entry for circle as initiator Before, the member entry that matched the `singleId` of the user and `level = 9` was used as initiator, which in practice means the member entry for the internal user circle of the user most of the time. Instead, we want to use the member entry that matches `singleId` of the user and `circleId` of the circle in question. This fixes the wrong `level` being set for `initiator` when calling `circleProbe()` with `DataProbe::INITIATOR`. Fixes: #1757 Signed-off-by: Jonas --- lib/Db/CoreQueryBuilder.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/Db/CoreQueryBuilder.php b/lib/Db/CoreQueryBuilder.php index 95549a870..0d3450cb8 100644 --- a/lib/Db/CoreQueryBuilder.php +++ b/lib/Db/CoreQueryBuilder.php @@ -1307,8 +1307,8 @@ public function completeProbeWithInitiator( ->leftJoin( $alias, CoreRequestBuilder::TABLE_MEMBER, $aliasInitiator, $expr->andX( - $expr->eq($aliasInitiator . '.circle_id', $helperAlias . '.' . $field), - $this->exprLimitInt('level', Member::LEVEL_OWNER, $aliasInitiator) + $expr->eq($aliasInitiator . '.circle_id', $alias . '.unique_id'), + $expr->eq($aliasInitiator . '.' . $field, $helperAlias . '.inheritance_first'), ) ); // From 95db9d9c9dcd32496be111533e927e945f4d608d Mon Sep 17 00:00:00 2001 From: Jonas Date: Wed, 20 Nov 2024 10:34:31 +0100 Subject: [PATCH 2/3] feat(CirclesManager): Add flag to enforce synchronous event execution Required for testing (where only one php thread is available), but as well for clients that require the event to be excecuted immediately. Signed-off-by: Jonas --- lib/CirclesManager.php | 14 +++++++++----- lib/Model/Federated/FederatedEvent.php | 11 +++++++++++ lib/Service/CircleService.php | 4 +++- lib/Service/FederatedEventService.php | 2 +- lib/Service/MemberService.php | 7 +++++-- 5 files changed, 29 insertions(+), 9 deletions(-) diff --git a/lib/CirclesManager.php b/lib/CirclesManager.php index 02524836c..78031d2f8 100644 --- a/lib/CirclesManager.php +++ b/lib/CirclesManager.php @@ -69,6 +69,7 @@ class CirclesManager { /** @var CirclesQueryHelper */ private $circlesQueryHelper; + private bool $forceSync = false; /** * CirclesManager constructor. @@ -152,7 +153,8 @@ public function getLocalFederatedUser(string $userId): FederatedUser { * @throws InvalidIdException * @throws FederatedUserException */ - public function startSession(?FederatedUser $federatedUser = null): void { + public function startSession(?FederatedUser $federatedUser = null, bool $forceSync = false): void { + $this->forceSync = $forceSync; if (is_null($federatedUser)) { $this->federatedUserService->initCurrentUser(); } else { @@ -163,7 +165,8 @@ public function startSession(?FederatedUser $federatedUser = null): void { /** * */ - public function startSuperSession(): void { + public function startSuperSession(bool $forceSync = false): void { + $this->forceSync = $forceSync; $this->federatedUserService->unsetCurrentUser(); $this->federatedUserService->bypassCurrentUserCondition(true); } @@ -224,6 +227,7 @@ public function startOccSession( public function stopSession(): void { $this->federatedUserService->unsetCurrentUser(); $this->federatedUserService->bypassCurrentUserCondition(false); + $this->forceSync = false; } @@ -298,7 +302,7 @@ public function createCircle( * @throws UnknownRemoteException */ public function destroyCircle(string $singleId): void { - $this->circleService->destroy($singleId); + $this->circleService->destroy($singleId, $this->forceSync); } @@ -426,7 +430,7 @@ public function flagAsAppManaged(string $circleId, bool $enabled = true): void { * @throws UnknownRemoteException */ public function addMember(string $circleId, FederatedUser $federatedUser): Member { - $outcome = $this->memberService->addMember($circleId, $federatedUser); + $outcome = $this->memberService->addMember($circleId, $federatedUser, $this->forceSync); $member = new Member(); $member->import($outcome); @@ -475,7 +479,7 @@ public function levelMember(string $memberId, int $level): Member { * @throws UnknownRemoteException */ public function removeMember(string $memberId): void { - $this->memberService->removeMember($memberId); + $this->memberService->removeMember($memberId, $this->forceSync); } diff --git a/lib/Model/Federated/FederatedEvent.php b/lib/Model/Federated/FederatedEvent.php index 3a78f4005..0a3370613 100644 --- a/lib/Model/Federated/FederatedEvent.php +++ b/lib/Model/Federated/FederatedEvent.php @@ -94,6 +94,8 @@ class FederatedEvent implements JsonSerializable { /** @var int */ private $bypass = 0; + private bool $forceSync = false; + /** * FederatedEvent constructor. @@ -552,6 +554,15 @@ public function canBypass(int $flag): bool { return (($this->bypass & $flag) !== 0); } + public function forceSync(bool $forceSync): self { + $this->forceSync = $forceSync; + return $this; + } + + public function isForceSync(): bool { + return $this->forceSync; + } + /** * @param array $data diff --git a/lib/Service/CircleService.php b/lib/Service/CircleService.php index a2abfa116..eec67317a 100644 --- a/lib/Service/CircleService.php +++ b/lib/Service/CircleService.php @@ -221,6 +221,7 @@ public function create( /** * @param string $circleId + * @param bool $forceSync * * @return array * @throws CircleNotFoundException @@ -235,13 +236,14 @@ public function create( * @throws RequestBuilderException * @throws UnknownRemoteException */ - public function destroy(string $circleId): array { + public function destroy(string $circleId, bool $forceSync = false): array { $this->federatedUserService->mustHaveCurrentUser(); $circle = $this->getCircle($circleId); $event = new FederatedEvent(CircleDestroy::class); $event->setCircle($circle); + $event->forceSync($forceSync); $this->federatedEventService->newEvent($event); return $event->getOutcome(); diff --git a/lib/Service/FederatedEventService.php b/lib/Service/FederatedEventService.php index bbe3c6bc0..2f7594875 100644 --- a/lib/Service/FederatedEventService.php +++ b/lib/Service/FederatedEventService.php @@ -364,7 +364,7 @@ private function confirmSharedItem(FederatedEvent $event, IFederatedItem $item): * @param IFederatedItem $item */ private function configureEvent(FederatedEvent $event, IFederatedItem $item) { - if ($item instanceof IFederatedItemAsyncProcess) { + if ($item instanceof IFederatedItemAsyncProcess && !$event->isForceSync()) { $event->setAsync(true); } if ($item instanceof IFederatedItemLimitedToInstanceWithMembership) { diff --git a/lib/Service/MemberService.php b/lib/Service/MemberService.php index e26248d44..29ce38f41 100644 --- a/lib/Service/MemberService.php +++ b/lib/Service/MemberService.php @@ -192,7 +192,7 @@ public function getMembers(string $circleId): array { * @throws InvalidIdException * @throws SingleCircleNotFoundException */ - public function addMember(string $circleId, FederatedUser $federatedUser): array { + public function addMember(string $circleId, FederatedUser $federatedUser, bool $forceSync = false): array { $this->federatedUserService->mustHaveCurrentUser(); $circle = $this->circleRequest->getCircle($circleId, $this->federatedUserService->getCurrentUser()); @@ -204,6 +204,7 @@ public function addMember(string $circleId, FederatedUser $federatedUser): array $event = new FederatedEvent(SingleMemberAdd::class); $event->setCircle($circle); $event->setMember($member); + $event->forceSync($forceSync); $this->federatedEventService->newEvent($event); @@ -261,6 +262,7 @@ function (FederatedUser $federatedUser) use ($patron) { /** * @param string $memberId + * @param bool $forceSync * * @return array * @throws FederatedEventException @@ -274,7 +276,7 @@ function (FederatedUser $federatedUser) use ($patron) { * @throws UnknownRemoteException * @throws RequestBuilderException */ - public function removeMember(string $memberId): array { + public function removeMember(string $memberId, bool $forceSync = false): array { $this->federatedUserService->mustHaveCurrentUser(); $member = $this->memberRequest->getMemberById( $memberId, @@ -284,6 +286,7 @@ public function removeMember(string $memberId): array { $event = new FederatedEvent(MemberRemove::class); $event->setCircle($member->getCircle()); $event->setMember($member); + $event->forceSync($forceSync); $this->federatedEventService->newEvent($event); From 23e90f6202c42b9426476efd55b98ebb7114ee3f Mon Sep 17 00:00:00 2001 From: Jonas Date: Tue, 19 Nov 2024 14:48:57 +0100 Subject: [PATCH 3/3] test(phpunit): Add basic phpunit integration tests for CirclesManager Signed-off-by: Jonas --- tests/phpunit.xml | 33 ++-- tests/unit/CirclesManagerTest.php | 162 ++++++++++++++++++ .../lib/Controller/AdminControllerTest.php | 2 +- .../lib/Controller/LocalControllerTest.php | 2 +- 4 files changed, 178 insertions(+), 21 deletions(-) create mode 100644 tests/unit/CirclesManagerTest.php diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 3df457589..062b37156 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -1,24 +1,19 @@ - + - - - . - - - - ../appinfo - ../lib - - - - - - - - - - + + + . + + + + + + + ../appinfo + ../lib + + diff --git a/tests/unit/CirclesManagerTest.php b/tests/unit/CirclesManagerTest.php new file mode 100644 index 000000000..c10c55721 --- /dev/null +++ b/tests/unit/CirclesManagerTest.php @@ -0,0 +1,162 @@ +circleName = sha1(uniqId(mt_rand(), true)); + + // Create test user + $userManager = \OC::$server->get(IUserManager::class); + if (!$userManager->userExists($this->userId)) { + $user = $userManager->createUser($this->userId, $this->userId); + } else { + $user = $userManager->get($this->userId); + } + + // Create test group and add user + $groupManager = \OC::$server->get(IGroupManager::class); + if (!$groupManager->groupExists($this->groupId)) { + $group = $groupManager->createGroup($this->groupId); + $group->addUser($user); + } + + $this->circlesManager = \OC::$server->get(CirclesManager::class); + + } + + // Start user session as user (default: test user) + private function startSession(?string $userId = null): FederatedUser { + if (!$userId) { + $userId = $this->userId; + } + $federatedUser = $this->circlesManager->getLocalFederatedUser($userId); + $this->circlesManager->startSession($federatedUser, true); + return $federatedUser; + } + + public function testCreateCircle(): void { + $federatedUser = $this->startSession(); + + // Created circle has properties + $circle = $this->circlesManager->createCircle($this->circleName); + $this->assertEquals($this->circleName, $circle->getName()); + $this->assertEquals($this->circleName, $circle->getDisplayName()); + $this->assertEquals($this->circleName, $circle->getSanitizedName()); + $this->assertEquals($federatedUser->getSingleId(), $circle->getOwner()->getSingleId()); + $this->assertEquals($federatedUser->getSingleId(), $circle->getInitiator()->getSingleId()); + + // Created circle returned by probeCircle() + $circles = $this->circlesManager->probeCircles(); + $this->assertCount(1, array_filter($circles, function (Circle $c) { return $c->getName() === $this->circleName; })); + + // Destroyed circle not returned by probeCircle() + $this->circlesManager->destroyCircle($circle->getSingleId()); + $circles = $this->circlesManager->probeCircles(); + $this->assertCount(0, array_filter($circles, function (Circle $c) { return $c->getName() === $this->circleName; })); + } + + public function testGetCirclesWithInitiator(): void { + // Create circle as user 'admin' and add test user as member + $this->startSession('admin'); + $adminCircle = $this->circlesManager->createCircle($this->circleName); + $this->circlesManager->addMember($adminCircle->getSingleId(), $this->circlesManager->getLocalFederatedUser($this->userId)); + + // Get circles as test user + $federatedUser = $this->startSession(); + $circles = $this->circlesManager->getCircles(); + $circle = null; + foreach ($circles as $c) { + if ($c->getSingleId() === $adminCircle->getSingleId()) { + $circle = $c; + } + } + + // Initiator of probed circle has correct properties + $this->assertEquals($federatedUser->getSingleId(), $circle->getInitiator()->getSingleId()); + $this->assertEquals(1, $circle->getInitiator()->getLevel()); + + // Destroy circle as user 'admin' + $this->startSession('admin'); + $this->circlesManager->destroyCircle($adminCircle->getSingleId()); + } + + public function testProbeCirclesWithInitiator(): void { + // Create circle as user 'admin' and add test user as member + $this->startSession('admin'); + $adminCircle = $this->circlesManager->createCircle($this->circleName); + $this->circlesManager->addMember($adminCircle->getSingleId(), $this->circlesManager->getLocalFederatedUser($this->userId)); + + // Probe circles as test user + $federatedUser = $this->startSession(); + $dataProbe = new DataProbe(); + $dataProbe->add(DataProbe::INITIATOR); + $circles = $this->circlesManager->probeCircles(null, $dataProbe); + $circle = null; + foreach ($circles as $c) { + if ($c->getSingleId() === $adminCircle->getSingleId()) { + $circle = $c; + } + } + + // Initiator of probed circle has correct properties + $this->assertEquals($federatedUser->getSingleId(), $circle->getInitiator()->getSingleId()); + $this->assertEquals(1, $circle->getInitiator()->getLevel()); + + // Destroy circle as user 'admin' + $this->startSession('admin'); + $this->circlesManager->destroyCircle($adminCircle->getSingleId()); + } + + public function testProbeCirclesWithInitiatorAsGroupMember(): void { + // Create circle as user 'admin' and add test group as member + $this->startSession('admin'); + $adminCircle = $this->circlesManager->createCircle($this->circleName); + $federatedGroup = $this->circlesManager->getFederatedUser($this->groupId, Member::TYPE_GROUP); + $this->circlesManager->addMember($adminCircle->getSingleId(), $federatedGroup); + + // Probe circles as test user + $federatedUser = $this->startSession(); + $dataProbe = new DataProbe(); + $dataProbe->add(DataProbe::INITIATOR); + $circles = $this->circlesManager->probeCircles(null, $dataProbe); + $circle = null; + foreach ($circles as $c) { + if ($c->getSingleId() === $adminCircle->getSingleId()) { + $circle = $c; + } + } + + // Initiator of probed circle has correct properties + $this->assertEquals($federatedGroup->getSingleId(), $circle->getInitiator()->getSingleId()); + $this->assertEquals(1, $circle->getInitiator()->getLevel()); + + // Destroy circle as user 'admin' + $this->startSession('admin'); + $this->circlesManager->destroyCircle($adminCircle->getSingleId()); + } +} diff --git a/tests/unit/lib/Controller/AdminControllerTest.php b/tests/unit/lib/Controller/AdminControllerTest.php index 9a2cb007f..a992a6a22 100644 --- a/tests/unit/lib/Controller/AdminControllerTest.php +++ b/tests/unit/lib/Controller/AdminControllerTest.php @@ -108,7 +108,7 @@ public function testCirclesList(int $limit, int $offset): void { $this->assertEquals($response, $this->adminController->circles('an-user-id', $limit, $offset)); } - public function dataForCirclesList(): array { + public static function dataForCirclesList(): array { return [ [-1, 0], [1, 1] diff --git a/tests/unit/lib/Controller/LocalControllerTest.php b/tests/unit/lib/Controller/LocalControllerTest.php index 0ad9cea15..188e93f13 100644 --- a/tests/unit/lib/Controller/LocalControllerTest.php +++ b/tests/unit/lib/Controller/LocalControllerTest.php @@ -117,7 +117,7 @@ public function testCirclesList(int $limit, int $offset): void { $this->assertEquals($response, $this->localController->circles($limit, $offset)); } - public function dataForCirclesList(): array { + public static function dataForCirclesList(): array { return [ [-1, 0], [1, 1]