From 0240df8d3a222faa9d30183e0beb1bb2a5e6cc0b 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 eeb401bda..bb7215971 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 3edf4ed64b7ee1ad2511fe3f31790fd9bd0f8817 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 1bae30502..518a9d693 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 6f248316f..a0a40f00c 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 d8e810841..a9e1f3e0a 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 8d93af769..f7784ad80 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 8aae04314efa02b4470e4a717958276ecdfcd22f 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 f28bff611..38966cff7 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]