From 9bb6ea12a8315024a05d564ff9d986563a43dcc7 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Sun, 9 Aug 2026 16:04:46 +0200 Subject: [PATCH] fix(auth): scope the contact-person read-outs to the caller's organisation (#459) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /api/contactpersonen/organisation/{organisationId}` was `@NoAdminRequired` with "is somebody logged in" as its only guard. `$organisationId` is a path parameter and was never compared to the caller's own organisation, so any authenticated user could read the contact persons of any organisation — and the response carries each contact's Nextcloud username, group membership and enabled/disabled state. Every sibling on the same controller that touches that data (`getUserInfo`, `getBulkUserInfo`, `updateUserGroups`, `disableUser`, `enableUser`) already refuses it to non-admins. - `checkOrganisationReadPermission()`: instance admins may read any organisation; everybody else only the organisation their own contactpersoon belongs to; a caller whose organisation cannot be resolved is refused. This mirrors `verifyCrossTenantScope()`, which already fails closed for writes. - The same guard is applied to the sibling route `getContactPersonsWithUserDetailsForOrganization`, named in the issue as having the same shape. - The per-record organisation is re-checked in PHP. The search filter is a bare top-level `organisation` key; whether OpenRegister reads that as an object property, as `@self` metadata, or ignores it is not visible from the call site, and an ignored filter returns an UNSCOPED result set that looks exactly like a scoped one. A record with no resolvable organisation is not served. - `resolveContactOrganisation()` now normalises the stored value. `organisatie` is declared as a related object in the register, so it can arrive as a nested envelope; comparing that raw against a plain UUID read as "different tenant" and denied legitimate members. - The enrichment loop reuses `buildUserInfoData()`, the shape the admin-gated siblings already return — three catalog group memberships rather than every GID the account holds. - `total` now counts what is actually returned instead of the unfiltered server-side total. Can-fail proof: reverting the controller to `origin/development` turns 4 of the 7 new tests red, including the item-level assertion — `victim@b.example` and `org-uuid-B` appear in the response body. The other 3 assert the legitimate surface still works and pass in both directions by design. phpcs lib/: 0 errors / 87 warnings, identical to origin/development. phpmd, psalm, phpstan clean. Unit suite 519 tests green. --- lib/Controller/ContactpersonenController.php | 202 ++++++++-- ...ersonenControllerOrganisationScopeTest.php | 352 ++++++++++++++++++ 2 files changed, 526 insertions(+), 28 deletions(-) create mode 100644 tests/Unit/Controller/ContactpersonenControllerOrganisationScopeTest.php diff --git a/lib/Controller/ContactpersonenController.php b/lib/Controller/ContactpersonenController.php index ba7eddd8..ee29d28b 100644 --- a/lib/Controller/ContactpersonenController.php +++ b/lib/Controller/ContactpersonenController.php @@ -169,6 +169,15 @@ public function __construct( /** * Get contactpersonen for an organisation with user status. * + * Authorization (GH#459): the response carries Nextcloud account data — + * username, full group membership and enabled/disabled state — for every + * contact of the requested organisation. That is the same payload + * {@see getUserInfo()} and {@see getBulkUserInfo()} refuse to non-admins, + * so the same bar applies here: an instance admin may read any + * organisation, anybody else may read only their OWN organisation, and a + * caller whose organisation cannot be resolved is refused. This mirrors + * {@see verifyCrossTenantScope()}, which already fails closed for writes. + * * @param string $organisationId The organisation ID. * * @return JSONResponse List of contactpersonen with user information. @@ -179,10 +188,19 @@ public function __construct( */ public function getContactpersonen(string $organisationId): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + $authError = $this->checkOrganisationReadPermission( + currentUser: $currentUser, + organisationId: $organisationId + ); + if ($authError !== null) { + return $authError; + } + try { // Get object service. $objectService = $this->container->get('OCA\OpenRegister\Service\ObjectService'); @@ -199,40 +217,39 @@ public function getContactpersonen(string $organisationId): JSONResponse $contactpersonen = $objectService->searchObjectsPaginated($searchParams); // Enhance with user information. + // + // GH#459 second finding: the filter above is a bare top-level + // `organisation` key. Whether OpenRegister treats that as an + // object-property filter, as `@self` metadata, or ignores it is + // not visible from here — and an ignored filter returns an + // UNSCOPED result set that looks exactly like a scoped one. The + // organisation of every returned record is therefore re-checked + // here, so a filter that fails to scope cannot leak. $enhancedContacts = []; foreach ($contactpersonen['results'] as $contactpersoon) { $contactData = $contactpersoon->getObject(); - $username = $contactData['username'] ?? null; - - $hasUser = empty($username) === false; - $userInfo = [ - 'hasUser' => $hasUser, - 'username' => $username, - 'groups' => [], - 'disabled' => false, - ]; - if (empty($username) === false) { - $user = $this->userManager->get($username); - if ($user !== null) { - $userGroups = $this->groupManager->getUserGroups($user); - $userInfo['groups'] = array_map( - function ($group) { - return $group->getGID(); - }, - $userGroups - ); - - // Get the disabled status from Nextcloud. - $userInfo['disabled'] = ($user->isEnabled() === false); - } + $contactOrg = $this->normaliseOrganisationRef(value: ($contactData['organisation'] ?? null)); + if ($contactOrg === null) { + $contactOrg = $this->normaliseOrganisationRef(value: ($contactData['organisatie'] ?? null)); } + // A record with no resolvable organisation is NOT served: an + // unattributed contact must not be presented as a member of + // the organisation the caller asked for. + if ($contactOrg === null || $contactOrg !== trim($organisationId)) { + continue; + } + + // The buildUserInfoData() shape is what the admin-gated + // getUserInfo()/getBulkUserInfo() already return: it reports + // only the three software-catalog group memberships instead of + // every GID the account holds, which is all this surface needs. $enhancedContacts[] = [ 'id' => $contactpersoon->getId(), 'uuid' => $contactpersoon->getUuid(), 'data' => $contactData, - 'user' => $userInfo, + 'user' => $this->buildUserInfoData(contactData: $contactData), ]; }//end foreach @@ -240,7 +257,7 @@ function ($group) { [ 'success' => true, 'contactpersonen' => $enhancedContacts, - 'total' => $contactpersonen['total'] ?? count($enhancedContacts), + 'total' => count($enhancedContacts), ] ); } catch (\Exception $e) { @@ -262,6 +279,110 @@ function ($group) { }//end try }//end getContactpersonen() + /** + * Check whether the caller may read the contact persons of an organisation. + * + * The contact-person read-outs carry Nextcloud account data (username, + * group membership, enabled state). Instance admins may read any + * organisation; everybody else may read only the organisation their own + * contactpersoon record belongs to. A caller whose organisation cannot be + * resolved is refused — the same fail-closed posture + * {@see verifyCrossTenantScope()} already applies to writes (GH#459). + * + * @param \OCP\IUser $currentUser The currently authenticated caller. + * @param string $organisationId The organisation the caller asked for. + * + * @return JSONResponse|null Forbidden response when refused, null when permitted. + * + * @spec openspec/specs/contactpersonen-api/spec.md + */ + private function checkOrganisationReadPermission(\OCP\IUser $currentUser, string $organisationId): ?JSONResponse + { + if ($this->groupManager->isAdmin($currentUser->getUID()) === true) { + return null; + } + + if (trim($organisationId) === '') { + return new JSONResponse( + ['success' => false, 'message' => 'Forbidden: an organisation is required'], + Http::STATUS_FORBIDDEN + ); + } + + $callerOrgUuid = null; + try { + $objectService = $this->container->get('OCA\OpenRegister\Service\ObjectService'); + $callerOrgUuid = $this->resolveContactOrganisation(objectService: $objectService, username: $currentUser->getUID()); + } catch (\Exception $e) { + $this->logger->warning( + 'ContactpersonenController: could not resolve the caller organisation, denying contact read', + ['callerUid' => $currentUser->getUID(), 'organisationId' => $organisationId, 'error' => $e->getMessage()] + ); + + return new JSONResponse( + ['success' => false, 'message' => 'Forbidden: organisation scope could not be verified'], + Http::STATUS_FORBIDDEN + ); + }//end try + + if ($callerOrgUuid === null || $callerOrgUuid !== trim($organisationId)) { + $this->logger->warning( + 'ContactpersonenController: cross-organisation contact read denied', + ['callerUid' => $currentUser->getUID(), 'callerOrg' => $callerOrgUuid, 'requestedOrg' => $organisationId] + ); + + return new JSONResponse( + ['success' => false, 'message' => 'Forbidden: you may only read contact persons of your own organisation'], + Http::STATUS_FORBIDDEN + ); + } + + return null; + + }//end checkOrganisationReadPermission() + + /** + * Normalise an organisation reference to a plain identifier string. + * + * Accepts a UUID string, or a nested related-object array carrying + * `uuid`, `id`, or an `@self` envelope with either of those. + * + * @param mixed $value The raw stored value. + * + * @return string|null The identifier, or null when there is none. + * + * @spec openspec/specs/contactpersonen-api/spec.md + */ + private function normaliseOrganisationRef(mixed $value): ?string + { + if (is_string($value) === true) { + $trimmed = trim($value); + if ($trimmed === '') { + return null; + } + + return $trimmed; + } + + if (is_array($value) === false) { + return null; + } + + foreach (['uuid', 'id', '@self'] as $key) { + if (array_key_exists($key, $value) === false) { + continue; + } + + $nested = $this->normaliseOrganisationRef(value: $value[$key]); + if ($nested !== null) { + return $nested; + } + } + + return null; + + }//end normaliseOrganisationRef() + /** * Convert a contactpersoon to a user account. * @@ -872,6 +993,12 @@ private function verifyCrossTenantScope(\OCP\IUser $currentUser, string $usernam /** * Resolve the organisation UUID for a user's contactpersoon. * + * The stored value is normalised: `organisatie` is declared as a related + * object in lib/Settings/softwarecatalogus_register.json, so it may arrive + * as a bare UUID string or as a nested envelope. Returning the raw value + * made a nested reference compare unequal to a plain UUID, which both this + * method's callers treat as "different tenant" (GH#459). + * * @param object $objectService The OpenRegister ObjectService. * @param string $username The username to look up. * @@ -890,7 +1017,13 @@ private function resolveContactOrganisation(object $objectService, string $usern } $data = $results['results'][0]->getObject(); - return $data['organisation'] ?? $data['organisatie'] ?? null; + + $ref = $this->normaliseOrganisationRef(value: ($data['organisation'] ?? null)); + if ($ref !== null) { + return $ref; + } + + return $this->normaliseOrganisationRef(value: ($data['organisatie'] ?? null)); }//end resolveContactOrganisation() @@ -963,6 +1096,10 @@ private function resolveCatalogGroupNames(\OCP\IUser $user): array * Returns all contact persons linked to a specific organization, * with their corresponding Nextcloud user details spliced in. * + * Same authorization bar as {@see getContactpersonen()} — this is the + * sibling route that returns the same account data for a caller-chosen + * organisation (GH#459). + * * @param string $organizationUuid The organization UUID. * * @return JSONResponse JSON response containing contact persons with user details. @@ -973,10 +1110,19 @@ private function resolveCatalogGroupNames(\OCP\IUser $user): array */ public function getContactPersonsWithUserDetailsForOrganization(string $organizationUuid): JSONResponse { - if ($this->userSession->getUser() === null) { + $currentUser = $this->userSession->getUser(); + if ($currentUser === null) { return new JSONResponse(['message' => 'Not authenticated'], Http::STATUS_UNAUTHORIZED); } + $authError = $this->checkOrganisationReadPermission( + currentUser: $currentUser, + organisationId: $organizationUuid + ); + if ($authError !== null) { + return $authError; + } + try { $this->logger->info( 'ContactpersonenController: Getting contact persons with user details for organization', diff --git a/tests/Unit/Controller/ContactpersonenControllerOrganisationScopeTest.php b/tests/Unit/Controller/ContactpersonenControllerOrganisationScopeTest.php new file mode 100644 index 00000000..65f3583a --- /dev/null +++ b/tests/Unit/Controller/ContactpersonenControllerOrganisationScopeTest.php @@ -0,0 +1,352 @@ + + * @copyright 2024 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://codeberg.org/Conduction/SoftwareCatalog + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Controller; + +use OCA\OpenRegister\Db\ObjectEntity; +use OCA\OpenRegister\Service\ObjectService; +use OCA\SoftwareCatalog\Controller\ContactpersonenController; +use OCA\SoftwareCatalog\Service\ContactpersoonService; +use OCA\SoftwareCatalog\Service\SettingsService; +use OCA\SoftwareCatalog\Service\SoftwareCatalogue\ContactPersonHandler; +use OCP\AppFramework\Http; +use OCP\IGroupManager; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserManager; +use OCP\IUserSession; +use OCP\Security\ISecureRandom; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * GH#459 — organisation scope on the contact-person read-outs. + * + * @category Test + * @package OCA\SoftwareCatalog\Tests\Unit\Controller + * @author Conduction b.v. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @version GIT: + * @link https://codeberg.org/Conduction/SoftwareCatalog + */ +class ContactpersonenControllerOrganisationScopeTest extends TestCase +{ + + private const CALLER_ORG = 'org-uuid-A'; + + private const FOREIGN_ORG = 'org-uuid-B'; + + private const FOREIGN_USERNAME = 'victim@b.example'; + + /** @var IUserManager|MockObject */ + private IUserManager|MockObject $userManager; + + /** @var IGroupManager|MockObject */ + private IGroupManager|MockObject $groupManager; + + /** @var IUserSession|MockObject */ + private IUserSession|MockObject $userSession; + + /** @var ContainerInterface|MockObject */ + private ContainerInterface|MockObject $container; + + /** @var ObjectService|MockObject */ + private ObjectService|MockObject $objectService; + + /** @var ContactpersoonService|MockObject */ + private ContactpersoonService|MockObject $contactSvc; + + /** @var LoggerInterface|MockObject */ + private LoggerInterface|MockObject $logger; + + private ContactpersonenController $controller; + + + /** + * Set up mocks and the controller instance. + * + * @return void + */ + protected function setUp(): void + { + parent::setUp(); + + $this->userManager = $this->createMock(IUserManager::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->objectService = $this->createMock(ObjectService::class); + $this->contactSvc = $this->createMock(ContactpersoonService::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->container = $this->createMock(ContainerInterface::class); + + $this->container + ->method('get') + ->with('OCA\OpenRegister\Service\ObjectService') + ->willReturn($this->objectService); + + $this->controller = new ContactpersonenController( + 'softwarecatalog', + $this->createMock(IRequest::class), + $this->createMock(SettingsService::class), + $this->createMock(ContactPersonHandler::class), + $this->contactSvc, + $this->userManager, + $this->groupManager, + $this->userSession, + $this->container, + $this->createMock(ISecureRandom::class), + $this->logger + ); + + }//end setUp() + + + /** + * Authenticate a caller with the given uid and admin flag. + * + * @param string $uid The caller uid. + * @param bool $isAdmin Whether the caller is an instance admin. + * + * @return void + */ + private function authenticate(string $uid, bool $isAdmin): void + { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + $this->userSession->method('getUser')->willReturn($user); + $this->groupManager->method('isAdmin')->willReturn($isAdmin); + + }//end authenticate() + + + /** + * Build a contactpersoon entity carrying an organisation and a username. + * + * @param string $organisation The organisation reference. + * @param string|null $username The Nextcloud username, when any. + * + * @return ObjectEntity + */ + private function makeContact(string $organisation, ?string $username=null): ObjectEntity + { + $data = ['organisatie' => $organisation]; + if ($username !== null) { + $data['username'] = $username; + } + + $entity = $this->createMock(ObjectEntity::class); + $entity->method('getObject')->willReturn($data); + $entity->method('getId')->willReturn(1); + $entity->method('getUuid')->willReturn('contact-uuid'); + + return $entity; + + }//end makeContact() + + + /** + * A non-admin asking for somebody else's organisation is refused. + * + * @return void + */ + public function testForeignOrganisationIsForbiddenForNonAdmin(): void + { + $this->authenticate(uid: 'plain@a.example', isAdmin: false); + + // The caller's own contactpersoon resolves to organisation A. + $this->objectService + ->method('searchObjectsPaginated') + ->willReturn(['results' => [$this->makeContact(organisation: self::CALLER_ORG)], 'total' => 1]); + + $response = $this->controller->getContactpersonen(self::FOREIGN_ORG); + + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + + }//end testForeignOrganisationIsForbiddenForNonAdmin() + + + /** + * A caller whose own organisation cannot be resolved is refused (fail closed). + * + * @return void + */ + public function testUnresolvableCallerOrganisationIsForbidden(): void + { + $this->authenticate(uid: 'orphan@example', isAdmin: false); + + $this->objectService + ->method('searchObjectsPaginated') + ->willReturn(['results' => [], 'total' => 0]); + + $response = $this->controller->getContactpersonen(self::CALLER_ORG); + + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + + }//end testUnresolvableCallerOrganisationIsForbidden() + + + /** + * A non-admin reading their OWN organisation still succeeds. + * + * @return void + */ + public function testOwnOrganisationIsAllowedForNonAdmin(): void + { + $this->authenticate(uid: 'plain@a.example', isAdmin: false); + + $this->objectService + ->method('searchObjectsPaginated') + ->willReturn(['results' => [$this->makeContact(organisation: self::CALLER_ORG)], 'total' => 1]); + + $response = $this->controller->getContactpersonen(self::CALLER_ORG); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertTrue($response->getData()['success']); + + }//end testOwnOrganisationIsAllowedForNonAdmin() + + + /** + * A record from another organisation is dropped from the body. + * + * This is the ITEM-level assertion: OpenRegister is made to return an + * UNSCOPED result set (the exact failure mode of a filter that does not + * scope), and the foreign username must not appear in the response. + * + * @return void + */ + public function testForeignRecordIsAbsentFromTheBodyWhenTheQueryDoesNotScope(): void + { + $this->authenticate(uid: 'admin', isAdmin: true); + + $mine = $this->makeContact(organisation: self::CALLER_ORG, username: 'me@a.example'); + $foreign = $this->makeContact(organisation: self::FOREIGN_ORG, username: self::FOREIGN_USERNAME); + + $this->objectService + ->method('searchObjectsPaginated') + ->willReturn(['results' => [$mine, $foreign], 'total' => 2]); + + $this->userManager->method('get')->willReturn(null); + + $response = $this->controller->getContactpersonen(self::CALLER_ORG); + $body = json_encode($response->getData()); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertStringNotContainsString( + self::FOREIGN_USERNAME, + (string) $body, + 'a contact belonging to another organisation leaked into the response body' + ); + $this->assertStringNotContainsString(self::FOREIGN_ORG, (string) $body); + $this->assertCount(1, $response->getData()['contactpersonen']); + $this->assertSame(1, $response->getData()['total']); + + }//end testForeignRecordIsAbsentFromTheBodyWhenTheQueryDoesNotScope() + + + /** + * An instance admin may read any organisation. + * + * @return void + */ + public function testAdminMayReadAnyOrganisation(): void + { + $this->authenticate(uid: 'admin', isAdmin: true); + + $this->objectService + ->method('searchObjectsPaginated') + ->willReturn(['results' => [$this->makeContact(organisation: self::FOREIGN_ORG)], 'total' => 1]); + + $this->userManager->method('get')->willReturn(null); + + $response = $this->controller->getContactpersonen(self::FOREIGN_ORG); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertCount(1, $response->getData()['contactpersonen']); + + }//end testAdminMayReadAnyOrganisation() + + + /** + * The sibling with-user-details route carries the same guard. + * + * @return void + */ + public function testSiblingWithUserDetailsRouteIsAlsoScoped(): void + { + $this->authenticate(uid: 'plain@a.example', isAdmin: false); + + $this->objectService + ->method('searchObjectsPaginated') + ->willReturn(['results' => [$this->makeContact(organisation: self::CALLER_ORG)], 'total' => 1]); + + // If the guard is absent the service is reached; it must not be. + $this->contactSvc + ->expects($this->never()) + ->method('getContactPersonsWithUserDetailsForOrganization'); + + $response = $this->controller->getContactPersonsWithUserDetailsForOrganization(self::FOREIGN_ORG); + + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + + }//end testSiblingWithUserDetailsRouteIsAlsoScoped() + + + /** + * A nested related-object organisation reference is understood. + * + * `organisatie` is declared as a related object in the register, so the + * stored value may be an envelope rather than a bare UUID. If the + * normaliser missed that shape, a legitimate member would be refused. + * + * @return void + */ + public function testNestedOrganisationReferenceIsResolved(): void + { + $this->authenticate(uid: 'plain@a.example', isAdmin: false); + + $entity = $this->createMock(ObjectEntity::class); + $entity->method('getObject')->willReturn(['organisatie' => ['@self' => ['uuid' => self::CALLER_ORG]]]); + $entity->method('getId')->willReturn(2); + $entity->method('getUuid')->willReturn('contact-uuid-2'); + + $this->objectService + ->method('searchObjectsPaginated') + ->willReturn(['results' => [$entity], 'total' => 1]); + + $this->userManager->method('get')->willReturn(null); + + $response = $this->controller->getContactpersonen(self::CALLER_ORG); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertCount(1, $response->getData()['contactpersonen']); + + }//end testNestedOrganisationReferenceIsResolved() + + +}//end class