Skip to content

Commit

Permalink
Merge pull request #1763 from nextcloud/backport/1758/stable28
Browse files Browse the repository at this point in the history
[stable28] fix(CoreQueryBuilder): Use correct member entry for circle as initiator
  • Loading branch information
mejo- authored Nov 25, 2024
2 parents 7c8b2d1 + 166b300 commit b4cd662
Show file tree
Hide file tree
Showing 10 changed files with 212 additions and 32 deletions.
14 changes: 9 additions & 5 deletions lib/CirclesManager.php
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@ class CirclesManager {
/** @var CirclesQueryHelper */
private $circlesQueryHelper;

private bool $forceSync = false;

/**
* CirclesManager constructor.
Expand Down Expand Up @@ -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 {
Expand All @@ -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);
}
Expand Down Expand Up @@ -244,6 +247,7 @@ public function startOccSession(
public function stopSession(): void {
$this->federatedUserService->unsetCurrentUser();
$this->federatedUserService->bypassCurrentUserCondition(false);
$this->forceSync = false;
}


Expand Down Expand Up @@ -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);
}


Expand Down Expand Up @@ -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);

Expand Down Expand Up @@ -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);
}


Expand Down
4 changes: 2 additions & 2 deletions lib/Db/CoreQueryBuilder.php
Original file line number Diff line number Diff line change
Expand Up @@ -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'),
)
);
//
Expand Down
11 changes: 11 additions & 0 deletions lib/Model/Federated/FederatedEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,8 @@ class FederatedEvent implements JsonSerializable {
/** @var int */
private $bypass = 0;

private bool $forceSync = false;


/**
* FederatedEvent constructor.
Expand Down Expand Up @@ -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
Expand Down
4 changes: 3 additions & 1 deletion lib/Service/CircleService.php
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,7 @@ public function create(

/**
* @param string $circleId
* @param bool $forceSync
*
* @return array
* @throws CircleNotFoundException
Expand All @@ -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();
Expand Down
2 changes: 1 addition & 1 deletion lib/Service/FederatedEventService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
7 changes: 5 additions & 2 deletions lib/Service/MemberService.php
Original file line number Diff line number Diff line change
Expand Up @@ -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());

Expand All @@ -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);

Expand Down Expand Up @@ -281,6 +282,7 @@ function (FederatedUser $federatedUser) use ($patron) {

/**
* @param string $memberId
* @param bool $forceSync
*
* @return array
* @throws FederatedEventException
Expand All @@ -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,
Expand All @@ -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);

Expand Down
33 changes: 14 additions & 19 deletions tests/phpunit.xml
Original file line number Diff line number Diff line change
@@ -1,20 +1,15 @@
<?xml version="1.0" encoding="utf-8" ?>
<phpunit bootstrap="bootstrap.php" convertDeprecationsToExceptions="true">
<testsuite name="circles">
<directory suffix='Test.php'>.</directory>
</testsuite>
<filter>
<whitelist>
<directory suffix=".php">../appinfo</directory>
<directory suffix=".php">../lib</directory>
</whitelist>
</filter>
<logging>
<log type="coverage-clover" target="clover.xml"/>
<!--<log type="coverage-text" target="php://stdout" showUncoveredFiles="false"/>-->
</logging>
<listeners>
<listener class="OCA\Circles\Tests\Env" file="TestSuiteListener.php"/>
</listeners>

<?xml version="1.0" encoding="utf-8"?>
<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" bootstrap="bootstrap.php" xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/10.5/phpunit.xsd" cacheDirectory=".phpunit.cache">
<testsuite name="circles">
<directory suffix="Test.php">.</directory>
</testsuite>
<logging>
<!--<log type="coverage-text" target="php://stdout" showUncoveredFiles="false"/>-->
</logging>
<source>
<include>
<directory suffix=".php">../appinfo</directory>
<directory suffix=".php">../lib</directory>
</include>
</source>
</phpunit>
165 changes: 165 additions & 0 deletions tests/unit/CirclesManagerTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,165 @@
<?php
/**
* SPDX-FileCopyrightText: 2024 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\Circles\Tests;

use OCA\Circles\CirclesManager;
use OCA\Circles\Model\Circle;
use OCA\Circles\Model\FederatedUser;
use OCA\Circles\Model\Member;
use OCA\Circles\Model\Probes\DataProbe;
use OCP\IGroupManager;
use OCP\IUserManager;
use Test\TestCase;

/**
* @group DB
*/
class CirclesManagerTest extends TestCase {
private CirclesManager $circlesManager;
private string $userId = 'circles-testuser';
private string $groupId = 'circles-testgroup';
private string $circleName;

public function setUp(): void {
parent::setUp();

// Random circle name
$this->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());
}
}
2 changes: 1 addition & 1 deletion tests/unit/lib/Controller/AdminControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down
2 changes: 1 addition & 1 deletion tests/unit/lib/Controller/LocalControllerTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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]
Expand Down

0 comments on commit b4cd662

Please sign in to comment.