Skip to content

[stable33] fix(local-controller): validate when trying adding member of type contact #2702

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

Open
backportbot wants to merge 2 commits into
base: stable33
Choose a base branch
from
+197 −77
Open

[stable33] fix(local-controller): validate when trying adding member of type contact #2702

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3510b40
fix(local-controller): validate when trying adding member of type con…
cristianscheid Jul 24, 2026
c6efac6
test: Add coverage for memberAdd and membersAdd methods
artonge Jul 31, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions lib/Controller/LocalController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -207,13 +207,12 @@ public function memberAdd(string $circleId, string $userId, int $type): DataResp
$this->permissionService->memberMustBeAtLeastModerator($memberUser);
}

// exception in Contact
if ($type === Member::TYPE_CONTACT) {
$currentUser = $this->federatedUserService->getCurrentUser();
if (!$this->configService->isLocalInstance($currentUser->getInstance())) {
throw new OCSException('works only from local instance', 404);
}

// prefix with current user id to scope contact lookup to this user
$userId = $currentUser->getUserId() . '/' . $userId;
}

Expand Down Expand Up @@ -252,6 +251,15 @@ public function membersAdd(string $circleId, array $members): DataResponse {
$userId = $this->get('id', $member);
$type = $this->getInt('type', $member);

if ($type === Member::TYPE_CONTACT) {
$currentUser = $this->federatedUserService->getCurrentUser();
if (!$this->configService->isLocalInstance($currentUser->getInstance())) {
throw new OCSException('works only from local instance', 404);
}
// prefix with current user id to scope contact lookup to this user
$userId = $currentUser->getUserId() . '/' . $userId;
}

if ($type === Member::TYPE_CIRCLE) {
$this->circleService->getCircle($userId);
}
Expand Down
262 changes: 187 additions & 75 deletions tests/unit/lib/Controller/LocalControllerTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@
use OCA\Circles\AppInfo\Application;
use OCA\Circles\Controller\LocalController;
use OCA\Circles\Model\Circle;
use OCA\Circles\Model\Probes\BasicProbe;
use OCA\Circles\Model\Probes\CircleProbe;
use OCA\Circles\Model\Member;
use OCA\Circles\Service\CircleService;
use OCA\Circles\Service\ConfigService;
use OCA\Circles\Service\FederatedUserService;
Expand All @@ -20,11 +19,10 @@
use OCA\Circles\Service\PermissionService;
use OCA\Circles\Service\SearchService;
use OCA\Circles\Tools\Traits\TDeserialize;
use OCP\AppFramework\Http\DataResponse;
use OCP\AppFramework\OCS\OCSException;
use OCP\IRequest;
use OCP\IUserManager;
use OCP\IUserSession;
use PHPUnit\Framework\MockObject\MockObject;
use Psr\Container\ContainerInterface;
use Test\TestCase;

/**
Expand All @@ -33,95 +31,209 @@
class LocalControllerTest extends TestCase {
use TDeserialize;

/** @var IRequest|MockObject */
private $request;
// suffix user ids with hash to avoid collision with existing users
private const TEST_USER_1 = 'circles-test-user-98cb5bac';
private const TEST_USER_2 = 'circles-test-user-bc412b0c';
private const TEST_USER_3 = 'circles-test-user-sd5468sd';
private const TEST_USER_4 = 'circles-test-user-32sds25s';

private ContainerInterface $container;
private Application $app;
private LocalController $localController;
private IUserManager $userManager;
private IUserSession $userSession;
private string $circleId = '';
private array $circlesToCleanup = [];
private static array $usersToCleanup = [];

public static function setUpBeforeClass(): void {
parent::setUpBeforeClass();

$app = new Application();
$userManager = $app->getContainer()->get(IUserManager::class);

foreach ([self::TEST_USER_1, self::TEST_USER_2, self::TEST_USER_3, self::TEST_USER_4] as $userId) {
$user = $userManager->get($userId);
if ($user === null) {
$userManager->createUser($userId, 'test-pwd');
self::$usersToCleanup[] = $userId;
}
}
}

/** @var IUserSession|MockObject */
private $userSession;
public function setUp(): void {
parent::setUp();

/** @var FederatedUserService|MockObject */
private $federatedUserService;
$this->app = new Application();
$this->container = $this->app->getContainer();
$this->userManager = $this->container->get(IUserManager::class);
$this->userSession = $this->container->get(IUserSession::class);

/** @var CircleService|MockObject */
private $circleService;
$user1 = $this->userManager->get(self::TEST_USER_1);
$this->userSession->setUser($user1);

/** @var MemberService|MockObject */
private $memberService;
$this->localController = new LocalController(
Application::APP_ID,
$this->container->get(IRequest::class),
$this->userSession,
$this->container->get(FederatedUserService::class),
$this->container->get(CircleService::class),
$this->container->get(MemberService::class),
$this->container->get(MembershipService::class),
$this->container->get(PermissionService::class),
$this->container->get(SearchService::class),
$this->container->get(ConfigService::class),
);

// Create a circle as TEST_USER_1 (owner)
$circleResult = $this->localController->create('test-circle')->getData();
$this->circlesToCleanup[] = $circleResult['id'];
$this->circleId = $circleResult['id'];
}

/** @var MembershipService|MockObject */
private $membershipService;
public static function tearDownAfterClass(): void {
parent::tearDownAfterClass();

/** @var SearchService|MockObject */
private $searchService;
$app = new Application();
$userManager = $app->getContainer()->get(IUserManager::class);

/** @var PermissionService|MockObject */
private $permissionService;
foreach (self::$usersToCleanup as $userId) {
$userManager->get($userId)?->delete();
}
}

/** @var ConfigService|MockObject */
private $configService;
protected function tearDown(): void {
parent::tearDown();

/** @var LocalController */
private $localController;
$circleService = $this->container->get(CircleService::class);

public function setUp(): void {
parent::setUp();
$this->request = $this->createMock(IRequest::class);
$this->userSession = $this->createMock(IUserSession::class);
$this->federatedUserService = $this->createMock(FederatedUserService::class);
$this->circleService = $this->createMock(CircleService::class);
$this->memberService = $this->createMock(MemberService::class);
$this->membershipService = $this->createMock(MembershipService::class);
$this->searchService = $this->createMock(SearchService::class);
$this->permissionService = $this->createMock(PermissionService::class);
$this->configService = $this->createMock(ConfigService::class);
$this->configService->expects($this->any())->method('getAppValueBool')->with(ConfigService::FRONTEND_ENABLED)->willReturn(true);
$this->localController = new LocalController(Application::APP_ID,
$this->request,
$this->userSession,
$this->federatedUserService,
$this->circleService,
$this->memberService,
$this->membershipService,
$this->permissionService,
$this->searchService,
$this->configService);
foreach ($this->circlesToCleanup as $circleId) {
try {
$circleService->destroy($circleId);
} catch (\Throwable) {
// continue cleanup
}
}
}

/**
* @dataProvider dataForCirclesList
*
* @param int $limit
* @param int $offset
* @return void
* @throws OCSException
*/
public function testCirclesList(int $limit, int $offset): void {
$probe = new CircleProbe();
$probe->filterHiddenCircles()
->filterBackendCircles()
->addDetail(BasicProbe::DETAILS_POPULATION)
->setItemsOffset($offset)
->setItemsLimit($limit);
$circle1 = new Circle();
$circle1->setName('Circle One');
$circle1->setSingleId('CircleOne');
$circle2 = new Circle();
$circle2->setName('Circle Two');
$circle2->setSingleId('CircleTwo');
$circle3 = new Circle();
$circle3->setName('Circle Three');
$circle3->setSingleId('CircleThree');
$circles = [$circle1, $circle2, $circle3];
$selectedCircles = array_slice($circles, $offset, $limit > 0 ? $limit : null);
$this->circleService->expects($this->once())->method('getCircles')->with($probe)->willReturn($selectedCircles);
$response = new DataResponse($this->serializeArray($selectedCircles));
$this->assertEquals($response, $this->localController->circles($limit, $offset));
public function testCirclesList(int $limit, int $offset, int $expectedCount): void {
$result1 = $this->localController->create('test-circle1')->getData();
$result2 = $this->localController->create('test-circle2')->getData();
$this->circlesToCleanup[] = $result1['id'];
$this->circlesToCleanup[] = $result2['id'];

$data = $this->localController->circles($limit, $offset)->getData();

$this->assertEquals(count($data), $expectedCount);
}

public static function dataForCirclesList(): array {
return [
[-1, 0],
[1, 1]
[-1, 0, 3],
[1, 1, 1],
[-1, 1, 2],
];
}

public function testMemberAdd(): void {
// Add TEST_USER_2 as a member
$memberResult = $this->localController->memberAdd($this->circleId, self::TEST_USER_2, Member::TYPE_USER)->getData();

// Verify the member was added
$this->assertNotNull($memberResult);
$this->assertEquals(self::TEST_USER_2, $memberResult['userId']);
$this->assertEquals($this->circleId, $memberResult['circleId']);
$this->assertEquals(Member::TYPE_USER, $memberResult['userType']);
}

public function testMemberAddPermissionDeniedForNonMember(): void {
// Switch to TEST_USER_2 (who is not a member)
$this->userSession->setUser($this->userManager->get(self::TEST_USER_2));

$response = $this->localController->memberAdd($this->circleId, self::TEST_USER_3, Member::TYPE_USER);
$this->assertEquals($response->getData()['message'], 'Insufficient permissions to perform this action');
}

public function testMemberAddPermissionDeniedForRegularMember(): void {
// Add TEST_USER_2 as a regular member
$this->localController->memberAdd($this->circleId, self::TEST_USER_2, Member::TYPE_USER);

// Switch to TEST_USER_2
$this->userSession->setUser($this->userManager->get(self::TEST_USER_2));

// TEST_USER_2 (regular member) tries to add another user
// This should fail because they're not a moderator
$response = $this->localController->memberAdd($this->circleId, self::TEST_USER_3, Member::TYPE_USER);
$this->assertEquals($response->getData()['message'], 'Insufficient permissions to perform this action');
}

public function testMemberAddAllowedForRegularMemberInFriendCircle(): void {
// Make it a friend circle (CFG_FRIEND = 128)
$this->localController->editConfig($this->circleId, Circle::CFG_FRIEND);

// Add TEST_USER_2 as a regular member
$this->localController->memberAdd($this->circleId, self::TEST_USER_2, Member::TYPE_USER);

// Switch to TEST_USER_2
$this->userSession->setUser($this->userManager->get(self::TEST_USER_2));

// TEST_USER_2 (regular member) tries to add TEST_USER_3
$result = $this->localController->memberAdd($this->circleId, self::TEST_USER_3, Member::TYPE_USER)->getData();

// Verify the member was added
$this->assertNotNull($result);
$this->assertEquals($result['userId'], self::TEST_USER_3);
$this->assertEquals($result['circleId'], $this->circleId);
}

public function testMemberAddAllowedForModerator(): void {
// Add TEST_USER_2 as a member
$memberResult = $this->localController->memberAdd($this->circleId, self::TEST_USER_2, Member::TYPE_USER)->getData();

// Promote TEST_USER_2 to moderator
$this->localController->memberLevel($this->circleId, $memberResult['id'], Member::LEVEL_MODERATOR);

// Switch to TEST_USER_2
$this->userSession->setUser($this->userManager->get(self::TEST_USER_2));

// TEST_USER_2 (moderator) tries to add TEST_USER_3
$result = $this->localController->memberAdd($this->circleId, self::TEST_USER_3, Member::TYPE_USER)->getData();

// Verify the member was added
$this->assertNotNull($result);
$this->assertEquals(self::TEST_USER_3, $result['userId']);
$this->assertEquals($this->circleId, $result['circleId']);
}

public function testMembersAddMultipleUsers(): void {
// Add multiple members at once
$members = [
['id' => self::TEST_USER_2, 'type' => Member::TYPE_USER],
['id' => self::TEST_USER_3, 'type' => Member::TYPE_USER],
];

$result = $this->localController->membersAdd($this->circleId, $members)->getData();

// Verify both members were added
$this->assertCount(2, $result);
$this->assertEquals($result[0]['userId'], self::TEST_USER_2);
$this->assertEquals($result[1]['userId'], self::TEST_USER_3);
}

public function testMembersAddPermissionDenied(): void {
// Switch to TEST_USER_2 (who is not a member)
$this->userSession->setUser($this->userManager->get(self::TEST_USER_2));

// Attempt to add multiple members without permission
$members = [
['id' => self::TEST_USER_3, 'type' => Member::TYPE_USER],
['id' => self::TEST_USER_4, 'type' => Member::TYPE_USER],
];

$response = $this->localController->membersAdd($this->circleId, $members);
$this->assertEquals($response->getData()['message'], 'Insufficient permissions to perform this action');
}
}