From dcabed8da2a0b0054c999f449c60491300de6d00 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Fri, 7 Aug 2026 08:20:56 +0200 Subject: [PATCH] fix(federation): delete the orphaned publish wrapper and test the seam that ships MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Gate 57 (orphaned-write-capability) named `FederationService::publishEntryForFederation()` — zero non-test production callers. Tracing the callers showed the capability is NOT missing, which is the opposite of what a dead write-capability usually means. `PublicationService::publish()` is live through `PublicationController::publish()`, routed as `publication#publish` (`PUT /api/publication/{objectType}/{uuid}/publish`, appinfo/routes.php:195). The federation wrapper was a pass-through to that same service call, minus two things the routed path has: * the per-object `authorizeEntry()` IDOR guard (ADR-005), and * the optional ISO-8601 `$when` moment — its signature was `(string $objectType, string $uuid)`, so a scheduled publication was unreachable through it. So it was not an unwired capability waiting for a caller; it was a second, weaker publish seam that nothing used. Wiring a route to it would have duplicated a live capability and widened the auth surface. It is deleted, along with its private `getPublicationService()` helper and the now-unused import. The two unit tests that named it were its ONLY callers anywhere in the repo. They are replaced by tests on the seam that actually ships — there was no `PublicationControllerTest` at all, so the live publish path and its IDOR guard were untested: * an admin publishes and `$when` is FORWARDED to the service; * a non-owning aanbod-beheerder gets 403 and `publish()` is never reached; * a peer-sourced (federated mirror) entry is refused even for an admin. Verified both directions, in a disposable nextcloud:32-apache container (PHP 8.3, the app bind-mounted at custom_apps/ so the deployed code is the code under test): | | result | |---|---| | gate 57 before | 1 finding — publishEntryForFederation | | gate 57 after | 0 findings | | new tests, guard intact | 3/3 pass (17/17 across both files) | | new tests, guard removed + `$when` dropped (mutant) | 3/3 FAIL | The mutant run is the positive control: all three assertions are about the guard and the forwarded argument, not about the mock. openspec/specs/federated-catalog-sync/spec.md named the deleted wrapper as the publish seam. It now names `PublicationController::publish()` and states that federation does not own a second publish entry point — the requirement's real invariant (visibility enforced by the OpenRegister public RBAC read gate on `publicatiedatum`) is unchanged and still satisfied. phpcs: 0 errors on both changed lib/ files. --- lib/Service/Federation/FederationService.php | 66 ++--- openspec/specs/federated-catalog-sync/spec.md | 11 +- .../Controller/PublicationControllerTest.php | 225 ++++++++++++++++++ tests/Unit/Service/FederationServiceTest.php | 61 +---- 4 files changed, 262 insertions(+), 101 deletions(-) create mode 100644 tests/Unit/Controller/PublicationControllerTest.php diff --git a/lib/Service/Federation/FederationService.php b/lib/Service/Federation/FederationService.php index 2e9d5378..6b53932c 100644 --- a/lib/Service/Federation/FederationService.php +++ b/lib/Service/Federation/FederationService.php @@ -26,7 +26,6 @@ namespace OCA\SoftwareCatalog\Service\Federation; -use OCA\SoftwareCatalog\Service\PublicationService; use OCA\SoftwareCatalog\Service\SettingsService; use OCP\App\IAppManager; use Psr\Container\ContainerInterface; @@ -223,52 +222,27 @@ public function announce(): array } }//end announce() - /** - * Make a local catalog entry visible to the federation by PUBLISHING it — - * i.e. set its `publicatiedatum` (the live OR RBAC publish gate) via the - * PublicationService. Only entries past their publicatiedatum are exposed to - * anonymous federation reads through the OpenCatalogi/OpenRegister public - * read surface; drafts (no publicatiedatum) never leave the instance. - * - * This is the publication-visibility leg of federation: it reuses the exact - * same `{group:public, match:{publicatiedatum:{$lte:$now}}}` rule that - * governs anonymous open-data reads, so one publish model serves both. The - * live cross-instance pull/merge remains deferred (needs a two-instance - * testbed) — see the @spec'd federated-catalog-sync subscription leg. - * - * @param string $objectType The publishable catalog object type. - * @param string $uuid The entry uuid. - * - * @return array{ok:bool, reason:string} Result. - * - * @spec openspec/specs/federated-catalog-sync/spec.md - */ - public function publishEntryForFederation(string $objectType, string $uuid): array - { - $publication = $this->getPublicationService(); - if ($publication === null) { - return ['ok' => false, 'reason' => 'PublicationService unavailable']; - } - - $result = $publication->publish($objectType, $uuid); - return ['ok' => $result['ok'], 'reason' => $result['reason']]; - }//end publishEntryForFederation() - - /** - * Get the open-data PublicationService (lazy, via the container) — federation - * reuses the same publicatiedatum publish gate as anonymous open data. - * - * @return PublicationService|null The service, or null when unavailable. + /* + * NO federation-specific publish entry point lives here, deliberately. + * + * `publishEntryForFederation()` used to sit at this spot: a pass-through to + * `PublicationService::publish()` with ZERO production callers (only two + * unit tests that constructed FederationService and called it directly). + * Gate 57 (orphaned-write-capability) named it, and tracing the callers + * showed the capability itself is NOT missing — it is live through + * `PublicationController::publish()`, routed as `publication#publish` + * (`PUT /api/publication/{objectType}/{uuid}/publish`, appinfo/routes.php), + * which calls the SAME `PublicationService::publish()` behind a per-object + * `authorizeEntry()` IDOR guard and additionally honours the optional + * ISO-8601 `$when` argument the wrapper silently dropped. + * + * Federation therefore has nothing of its own to publish: it consumes the + * result of that one publish model, because visibility is enforced by the + * OpenRegister public RBAC read gate + * `{group:public, match:{publicatiedatum:{$lte:$now}}}` — the same rule that + * governs anonymous open-data reads. Re-adding a second, guardless publish + * seam here would duplicate a live capability and weaken it. */ - private function getPublicationService(): ?PublicationService - { - try { - return $this->container->get(PublicationService::class); - } catch (\Throwable $e) { - $this->logger->error('[Federation] PublicationService unavailable', ['error' => $e->getMessage()]); - return null; - } - }//end getPublicationService() /** * Discover peer catalogs from the configured directory. diff --git a/openspec/specs/federated-catalog-sync/spec.md b/openspec/specs/federated-catalog-sync/spec.md index e61162a6..e6d48282 100644 --- a/openspec/specs/federated-catalog-sync/spec.md +++ b/openspec/specs/federated-catalog-sync/spec.md @@ -42,9 +42,14 @@ future (and not superseded by a `depublicatiedatum`). Visibility SHALL be enforced by the OpenRegister/OpenCatalogi public RBAC read gate `{group:public, match:{publicatiedatum:{$lte:$now}}}` (NOT the removed `@self.published` predicate) — the app SHALL NOT implement its own anonymous -filtering. Publishing an entry to the federation SHALL set `publicatiedatum` via -`FederationService::publishEntryForFederation()` (delegating to the shared -`PublicationService`). +filtering. Publishing an entry to the federation SHALL set `publicatiedatum` +through the app's single publish seam — `PublicationController::publish()` +(route `publication#publish`, `PUT /api/publication/{objectType}/{uuid}/publish`) +delegating to `PublicationService::publish()`, behind that controller's +per-object ownership guard. Federation SHALL NOT own a second publish entry +point: there is one publish model and federation consumes its result, because +the visibility rule above is the same rule that governs anonymous open-data +reads. #### Scenario: Published entry is visible to a peer diff --git a/tests/Unit/Controller/PublicationControllerTest.php b/tests/Unit/Controller/PublicationControllerTest.php new file mode 100644 index 00000000..0724f9fd --- /dev/null +++ b/tests/Unit/Controller/PublicationControllerTest.php @@ -0,0 +1,225 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * @link https://codeberg.org/Conduction/SoftwareCatalog + * + * @spec openspec/specs/open-data-publishing/spec.md + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Controller; + +use OCA\SoftwareCatalog\Controller\PublicationController; +use OCA\SoftwareCatalog\Service\PublicationService; +use OCP\AppFramework\Http; +use OCP\IConfig; +use OCP\IGroup; +use OCP\IGroupManager; +use OCP\IRequest; +use OCP\IUser; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * Test class for PublicationController. + */ +class PublicationControllerTest extends TestCase +{ + /** + * The publication service double. + * + * @var PublicationService|MockObject + */ + private PublicationService|MockObject $publicationService; + + /** + * The user session double. + * + * @var IUserSession|MockObject + */ + private IUserSession|MockObject $userSession; + + /** + * The group manager double. + * + * @var IGroupManager|MockObject + */ + private IGroupManager|MockObject $groupManager; + + /** + * The config double (per-user organisation lookup). + * + * @var IConfig|MockObject + */ + private IConfig|MockObject $config; + + /** + * Build the collaborator doubles. + * + * @return void + */ + protected function setUp(): void + { + parent::setUp(); + $this->publicationService = $this->createMock(PublicationService::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->config = $this->createMock(IConfig::class); + + }//end setUp() + + /** + * Build the controller under test. + * + * @return PublicationController The controller. + */ + private function controller(): PublicationController + { + return new PublicationController( + $this->createMock(IRequest::class), + $this->userSession, + $this->groupManager, + $this->config, + $this->publicationService, + $this->createMock(LoggerInterface::class) + ); + + }//end controller() + + /** + * Sign a user in and put them in the given groups. + * + * @param string $uid The uid. + * @param array $groups The group ids. + * + * @return void + */ + private function signIn(string $uid, array $groups): void + { + $user = $this->createMock(IUser::class); + $user->method('getUID')->willReturn($uid); + $this->userSession->method('getUser')->willReturn($user); + + $groupMocks = []; + foreach ($groups as $gid) { + $group = $this->createMock(IGroup::class); + $group->method('getGID')->willReturn($gid); + $groupMocks[] = $group; + } + + $this->groupManager->method('getUserGroups')->willReturn($groupMocks); + + }//end signIn() + + /** + * An admin publishes, and the optional ISO-8601 moment reaches the service. + * + * The wrapper this test replaces had the signature + * `publishEntryForFederation(string, string)` and could not express `$when` + * at all, so a scheduled publication was unreachable through it. + * + * @return void + */ + public function testAdminPublishForwardsTheOptionalWhenMoment(): void + { + $this->signIn(uid: 'alice', groups: ['admin']); + $this->publicationService->method('isPublishableType')->willReturn(true); + $this->publicationService->method('resolveEntry')->willReturn( + ['data' => ['_organisation' => 'org-1']] + ); + + $this->publicationService->expects($this->once()) + ->method('publish') + ->with('dienst', 'uuid-9', '2026-09-01T00:00:00+00:00') + ->willReturn( + [ + 'ok' => true, + 'reason' => 'scheduled', + 'publicatiedatum' => '2026-09-01T00:00:00+00:00', + ] + ); + + $response = $this->controller()->publish( + objectType: 'dienst', + uuid: 'uuid-9', + when: '2026-09-01T00:00:00+00:00' + ); + + $this->assertSame(Http::STATUS_OK, $response->getStatus()); + $this->assertTrue($response->getData()['ok']); + + }//end testAdminPublishForwardsTheOptionalWhenMoment() + + /** + * A beheerder from another organisation is refused, and the service is + * never reached — the refusal is server-side, before any write. + * + * @return void + */ + public function testNonOwnerBeheerderIsRefusedAndNeverReachesTheService(): void + { + $this->signIn(uid: 'bob', groups: ['aanbod-beheerder']); + $this->publicationService->method('isPublishableType')->willReturn(true); + $this->publicationService->method('resolveEntry')->willReturn( + ['data' => ['_organisation' => 'org-1']] + ); + $this->config->method('getUserValue')->willReturn('org-2'); + + $this->publicationService->expects($this->never())->method('publish'); + + $response = $this->controller()->publish(objectType: 'dienst', uuid: 'uuid-9'); + + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + + }//end testNonOwnerBeheerderIsRefusedAndNeverReachesTheService() + + /** + * A peer-sourced (federated mirror) entry is read-only locally, even for an + * admin — publishing it would republish another instance's record as ours. + * + * @return void + */ + public function testPeerSourcedEntryCannotBePublishedLocally(): void + { + $this->signIn(uid: 'alice', groups: ['admin']); + $this->publicationService->method('isPublishableType')->willReturn(true); + $this->publicationService->method('resolveEntry')->willReturn( + ['data' => ['_source' => ['instance' => 'https://peer.example.org']]] + ); + + $this->publicationService->expects($this->never())->method('publish'); + + $response = $this->controller()->publish(objectType: 'dienst', uuid: 'uuid-9'); + + $this->assertSame(Http::STATUS_FORBIDDEN, $response->getStatus()); + + }//end testPeerSourcedEntryCannotBePublishedLocally() +}//end class diff --git a/tests/Unit/Service/FederationServiceTest.php b/tests/Unit/Service/FederationServiceTest.php index e3cb95a3..18ca44c3 100644 --- a/tests/Unit/Service/FederationServiceTest.php +++ b/tests/Unit/Service/FederationServiceTest.php @@ -122,59 +122,16 @@ public function testAnnounceNoopWhenDisabled(): void $this->assertSame('federation disabled', $result['reason']); }//end testAnnounceNoopWhenDisabled() - /** - * The federation publication leg delegates to PublicationService — i.e. it - * PUBLISHES the entry (sets publicatiedatum, the live OR RBAC gate) so - * federated anonymous reads can see it. No bespoke published predicate. - * - * @return void - */ - public function testPublishEntryForFederationDelegatesToPublicationService(): void - { - $container = $this->createMock(ContainerInterface::class); - $appManager = $this->createMock(IAppManager::class); - $appManager->method('getInstalledApps')->willReturn(['softwarecatalog', 'opencatalogi']); - $config = $this->createMock(FederationConfig::class); - $logger = $this->createMock(LoggerInterface::class); - - $publication = $this->createMock(\OCA\SoftwareCatalog\Service\PublicationService::class); - $publication->expects($this->once()) - ->method('publish') - ->with('dienst', 'uuid-9') - ->willReturn(['ok' => true, 'reason' => 'published', 'publicatiedatum' => '2024-01-01T00:00:00+00:00']); - - $container->method('get') - ->with(\OCA\SoftwareCatalog\Service\PublicationService::class) - ->willReturn($publication); - - $service = new FederationService($container, $appManager, $config, new FederationMerger(), $this->createMock(SettingsService::class), $logger); - $result = $service->publishEntryForFederation('dienst', 'uuid-9'); - - $this->assertTrue($result['ok']); - $this->assertSame('published', $result['reason']); - }//end testPublishEntryForFederationDelegatesToPublicationService() - - /** - * The federation publication leg degrades cleanly (no throw) when the - * PublicationService cannot be resolved. - * - * @return void + /* + * The two testPublishEntryForFederation* tests that stood here were the ONLY + * callers of FederationService::publishEntryForFederation() anywhere in the + * repo — they constructed the service and invoked a method no production + * code path could reach. Both method and tests are gone; the publication + * leg they claimed to cover is exercised where it actually ships, in + * tests/Unit/Controller/PublicationControllerTest.php (controller -> + * PublicationService::publish, plus the per-object IDOR guard) and + * tests/Unit/Service/PublicationServiceTest.php. */ - public function testPublishEntryForFederationDegradesWithoutPublicationService(): void - { - $container = $this->createMock(ContainerInterface::class); - $appManager = $this->createMock(IAppManager::class); - $appManager->method('getInstalledApps')->willReturn(['softwarecatalog']); - $config = $this->createMock(FederationConfig::class); - $logger = $this->createMock(LoggerInterface::class); - $container->method('get')->willThrowException(new \RuntimeException('no service')); - - $service = new FederationService($container, $appManager, $config, new FederationMerger(), $this->createMock(SettingsService::class), $logger); - $result = $service->publishEntryForFederation('dienst', 'uuid-9'); - - $this->assertFalse($result['ok']); - $this->assertSame('PublicationService unavailable', $result['reason']); - }//end testPublishEntryForFederationDegradesWithoutPublicationService() /** * discoverPeers returns an empty list (no throw) when OpenCatalogi missing.