From 376cb79105c3d5fdb58d6d4efc4fec30205d1bea 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 35e952e63..d0f5c261c 100644 --- a/lib/Db/CoreQueryBuilder.php +++ b/lib/Db/CoreQueryBuilder.php @@ -1327,8 +1327,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 b14ac9ea4ac9a5420f0afacc6a8de472fff17517 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 9dbcfa76c..4c4c0d908 100644 --- a/lib/CirclesManager.php +++ b/lib/CirclesManager.php @@ -89,6 +89,7 @@ class CirclesManager { /** @var CirclesQueryHelper */ private $circlesQueryHelper; + private bool $forceSync = false; /** * CirclesManager constructor. @@ -172,7 +173,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 { @@ -183,7 +185,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); } @@ -244,6 +247,7 @@ public function startOccSession( public function stopSession(): void { $this->federatedUserService->unsetCurrentUser(); $this->federatedUserService->bypassCurrentUserCondition(false); + $this->forceSync = false; } @@ -318,7 +322,7 @@ public function createCircle( * @throws UnknownRemoteException */ public function destroyCircle(string $singleId): void { - $this->circleService->destroy($singleId); + $this->circleService->destroy($singleId, $this->forceSync); } @@ -446,7 +450,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); @@ -495,7 +499,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 8443f67c9..762a5c5a2 100644 --- a/lib/Model/Federated/FederatedEvent.php +++ b/lib/Model/Federated/FederatedEvent.php @@ -114,6 +114,8 @@ class FederatedEvent implements JsonSerializable { /** @var int */ private $bypass = 0; + private bool $forceSync = false; + /** * FederatedEvent constructor. @@ -572,6 +574,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 475ed968d..f31d22a23 100644 --- a/lib/Service/CircleService.php +++ b/lib/Service/CircleService.php @@ -241,6 +241,7 @@ public function create( /** * @param string $circleId + * @param bool $forceSync * * @return array * @throws CircleNotFoundException @@ -255,13 +256,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 2bf3e7752..dc6cbaf22 100644 --- a/lib/Service/FederatedEventService.php +++ b/lib/Service/FederatedEventService.php @@ -384,7 +384,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 138816ba0..f3a3a2cb8 100644 --- a/lib/Service/MemberService.php +++ b/lib/Service/MemberService.php @@ -212,7 +212,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()); @@ -224,6 +224,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); @@ -281,6 +282,7 @@ function (FederatedUser $federatedUser) use ($patron) { /** * @param string $memberId + * @param bool $forceSync * * @return array * @throws FederatedEventException @@ -294,7 +296,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, @@ -304,6 +306,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 166b300c64e02432854beb3eff6c737613820608 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 | 165 ++++++++++++++++++ .../lib/Controller/AdminControllerTest.php | 2 +- .../lib/Controller/LocalControllerTest.php | 2 +- 4 files changed, 181 insertions(+), 21 deletions(-) create mode 100644 tests/unit/CirclesManagerTest.php diff --git a/tests/phpunit.xml b/tests/phpunit.xml index 678e29a81..016512980 100644 --- a/tests/phpunit.xml +++ b/tests/phpunit.xml @@ -1,20 +1,15 @@ - - - - . - - - - ../appinfo - ../lib - - - - - - - - - - + + + + . + + + + + + + ../appinfo + ../lib + + diff --git a/tests/unit/CirclesManagerTest.php b/tests/unit/CirclesManagerTest.php new file mode 100644 index 000000000..041714f30 --- /dev/null +++ b/tests/unit/CirclesManagerTest.php @@ -0,0 +1,165 @@ +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 fd9fe1273..a3bac6e0f 100644 --- a/tests/unit/lib/Controller/AdminControllerTest.php +++ b/tests/unit/lib/Controller/AdminControllerTest.php @@ -125,7 +125,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 d0fdec353..2b3b2a64f 100644 --- a/tests/unit/lib/Controller/LocalControllerTest.php +++ b/tests/unit/lib/Controller/LocalControllerTest.php @@ -134,7 +134,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]