diff --git a/appinfo/routes.php b/appinfo/routes.php index b8f39f3f..cabad721 100644 --- a/appinfo/routes.php +++ b/appinfo/routes.php @@ -35,6 +35,10 @@ // Core Settings API routes (minimal, for basic app functionality) ['name' => 'settings#index', 'url' => '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/api/settings', 'verb' => 'GET'], ['name' => 'settings#create', 'url' => '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/api/settings', 'verb' => 'POST'], + // Canonical AppHost write verb. `settings#create` (POST, above) is the + // legacy alias and delegates to `update()`; both are kept. + // @spec openspec/specs/method-decomposition/spec.md#requirement-settingscontroller-settings-crud-endpoints-req-decomp-013 + ['name' => 'settings#update', 'url' => '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/api/settings', 'verb' => 'PUT'], ['name' => 'settings#load', 'url' => '/api/settings/load', 'verb' => 'GET'], ['name' => 'settings#initialize', 'url' => '/api/settings/initialize', 'verb' => 'POST'], ['name' => 'settings#status', 'url' => '/api/settings/status', 'verb' => 'GET'], diff --git a/lib/Controller/SettingsController.php b/lib/Controller/SettingsController.php index 1a90eaf2..7e2c5bd6 100644 --- a/lib/Controller/SettingsController.php +++ b/lib/Controller/SettingsController.php @@ -294,15 +294,34 @@ public function index(): JSONResponse }//end index() /** - * Handle the post request to update settings. + * Write the app-configuration settings block read back by index(). + * + * This is the canonical AppHost write verb for `/api/settings` + * (`PUT /api/settings` → `settings#update`). It persists exactly the three + * sections `index()` surfaces through `SettingsService::getAllSettings()`: + * + * - `configuration` / `selectedRegister` → `SettingsService::updateSettings` + * - `userGroups.{generic,organizationAdmin,superUser}` → validated via + * `validateGroups`, then persisted through the matching setter + * - `emailSettings` → `SettingsService::updateEmailSettings` + * + * It deliberately does NOT absorb the controller's other configuration + * surfaces (general/sync/archimate/email/amef/voorzieningen/user-groups/ + * cronjob/eol-sync config, email templates, ArchiMate import-export, + * progress and statistics). Those are separate endpoints on their own URLs + * and keep their own handlers — this method is not a catch-all. + * + * Admin-only: no NoAdminRequired tag is declared, so Nextcloud's security + * middleware requires an administrator — the same posture as `create()`. + * Net privilege change is zero. * * @return JSONResponse JSON response containing the updated settings. * * @NoCSRFRequired * - * @spec openspec/specs/method-decomposition/spec.md + * @spec openspec/specs/method-decomposition/spec.md#requirement-settingscontroller-settings-crud-endpoints-req-decomp-013 */ - public function create(): JSONResponse + public function update(): JSONResponse { try { $data = $this->request->getParams(); @@ -326,6 +345,26 @@ public function create(): JSONResponse return new JSONResponse(['error' => $e->getMessage()], 500); }//end try + }//end update() + + /** + * Handle the post request to update settings. + * + * Legacy alias for `update()`, kept so `POST /api/settings` keeps behaving + * exactly as before. The auth attributes are repeated here on purpose: + * Nextcloud's middleware only evaluates the attributes of the method the + * router actually dispatched, so delegation does not inherit them. + * + * @return JSONResponse JSON response containing the updated settings. + * + * @NoCSRFRequired + * + * @spec openspec/specs/method-decomposition/spec.md#requirement-settingscontroller-settings-crud-endpoints-req-decomp-013 + */ + public function create(): JSONResponse + { + return $this->update(); + }//end create() /** diff --git a/tests/Unit/Controller/SettingsControllerCanonicalWriteTest.php b/tests/Unit/Controller/SettingsControllerCanonicalWriteTest.php new file mode 100644 index 00000000..acbc89f3 --- /dev/null +++ b/tests/Unit/Controller/SettingsControllerCanonicalWriteTest.php @@ -0,0 +1,316 @@ + + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit\Controller; + +use OCA\SoftwareCatalog\Controller\SettingsController; +use OCA\SoftwareCatalog\Service\SettingsService; +use OCP\AppFramework\Http\JSONResponse; +use OCP\IRequest; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; +use ReflectionClass; +use ReflectionMethod; + +/** + * Pins the canonical AppHost write verb on `/api/settings`. + * + * `PUT /api/settings` → `settings#update` is the canonical write in + * OpenRegister's AppHost dialect; `POST /api/settings` → `settings#create` + * is the legacy alias. SoftwareCatalog ships its own SettingsController, so + * AppHost's generic controller is never aliased in and the leaf owes both + * methods itself. Before this change `PUT /api/settings` answered 405. + * + * @spec openspec/specs/method-decomposition/spec.md#requirement-settingscontroller-settings-crud-endpoints-req-decomp-013 + */ +class SettingsControllerCanonicalWriteTest extends TestCase +{ + /** + * The canonical `/api/settings` methods, asserted item by item. + * + * @return array + */ + public static function canonicalMethodProvider(): array + { + return [ + 'index (GET)' => ['index'], + 'create (POST)' => ['create'], + 'update (PUT)' => ['update'], + ]; + } + + /** + * Each canonical method must exist and be publicly dispatchable. + * + * Asserted on the ITEM, not on the container: "the class exists" would + * pass with `update()` missing entirely. + * + * @dataProvider canonicalMethodProvider + */ + public function testCanonicalMethodExistsAndIsDispatchable(string $method): void + { + $reflection = new ReflectionClass(SettingsController::class); + + $this->assertTrue( + $reflection->hasMethod($method), + sprintf('SettingsController::%s() is missing — the route would 500 on dispatch.', $method) + ); + + $ref = $reflection->getMethod($method); + + $this->assertTrue( + $ref->isPublic(), + sprintf('SettingsController::%s() must be public to be dispatchable.', $method) + ); + $this->assertFalse( + $ref->isStatic(), + sprintf('SettingsController::%s() must not be static.', $method) + ); + $this->assertSame( + 0, + $ref->getNumberOfRequiredParameters(), + sprintf('SettingsController::%s() must take no required parameters.', $method) + ); + } + + /** + * Positive control: the scan above must actually have inspected methods. + * + * Without this, a provider that silently returned [] would make the whole + * class report green while checking nothing. + */ + public function testPositiveControlScanInspectedMethods(): void + { + $reflection = new ReflectionClass(SettingsController::class); + + $inspected = 0; + foreach (self::canonicalMethodProvider() as $case) { + if ($reflection->hasMethod($case[0]) === true) { + $inspected++; + } + } + + $this->assertGreaterThan(0, $inspected, 'Positive control: the canonical-method scan matched nothing.'); + $this->assertSame(3, $inspected, 'Positive control: expected all 3 canonical methods to be inspected.'); + } + + /** + * `update()` must carry the SAME auth posture as `create()`. + * + * Net privilege change must be zero: `create()` declares `@NoCSRFRequired` + * and deliberately NOT `@NoAdminRequired`, so the middleware demands an + * administrator. Copying `@NoAdminRequired` from a sibling READ method + * here would silently open instance-wide config to any logged-in user. + */ + public function testUpdateHasIdenticalAuthPostureToCreate(): void + { + $createDoc = (new ReflectionMethod(SettingsController::class, 'create'))->getDocComment(); + $updateDoc = (new ReflectionMethod(SettingsController::class, 'update'))->getDocComment(); + + $this->assertIsString($createDoc, 'create() must have a docblock.'); + $this->assertIsString($updateDoc, 'update() must have a docblock.'); + + foreach (['@NoCSRFRequired', '@PublicPage', '@NoAdminRequired'] as $tag) { + $this->assertSame( + str_contains($createDoc, $tag), + str_contains($updateDoc, $tag), + sprintf('Auth posture drift: %s differs between create() and update().', $tag) + ); + } + + // Pin the absolute posture too, so a future relaxation of BOTH + // methods cannot slip through the parity check above. + $this->assertStringNotContainsString( + '@NoAdminRequired', + $updateDoc, + 'update() writes instance-wide config and must stay admin-only.' + ); + $this->assertStringNotContainsString( + '@PublicPage', + $updateDoc, + 'update() must never be a public page.' + ); + + // Attribute form must not sneak the posture in either. + $updateAttrs = array_map( + static fn ($a) => $a->getName(), + (new ReflectionMethod(SettingsController::class, 'update'))->getAttributes() + ); + $this->assertNotContains('OCP\AppFramework\Http\Attribute\NoAdminRequired', $updateAttrs); + $this->assertNotContains('OCP\AppFramework\Http\Attribute\PublicPage', $updateAttrs); + } + + /** + * Build a controller with only the collaborators the write path touches. + * + * @param array $params The request params to serve. + */ + private function makeController(array $params, SettingsService $settingsService): SettingsController + { + $reflection = new ReflectionClass(SettingsController::class); + /** @var SettingsController $controller */ + $controller = $reflection->newInstanceWithoutConstructor(); + + $request = $this->createMock(IRequest::class); + $request->method('getParams')->willReturn($params); + + foreach ( + [ + 'request' => $request, + 'settingsService' => $settingsService, + 'logger' => $this->createMock(LoggerInterface::class), + ] as $name => $value + ) { + $prop = $reflection->getProperty($name); + $prop->setAccessible(true); + $prop->setValue($controller, $value); + } + + return $controller; + } + + /** + * `update()` persists exactly the three sections `index()` reads back. + */ + public function testUpdateWritesConfigurationUserGroupsAndEmailSettings(): void + { + $params = [ + 'configuration' => ['catalog' => 'main'], + 'userGroups' => ['generic' => ['users']], + 'emailSettings' => ['smtpHost' => 'mail.example.org'], + ]; + + $settingsService = $this->createMock(SettingsService::class); + $settingsService->expects($this->once()) + ->method('updateSettings') + ->willReturn(['configuration' => ['catalog' => 'main']]); + $settingsService->expects($this->once()) + ->method('validateGroups') + ->with(['users']) + ->willReturn(['valid' => ['users'], 'invalid' => []]); + $settingsService->expects($this->once()) + ->method('setGenericUserGroups') + ->with(['users']); + $settingsService->expects($this->once()) + ->method('updateEmailSettings') + ->with(['smtpHost' => 'mail.example.org']) + ->willReturn(['smtpHost' => 'mail.example.org']); + + $response = $this->makeController($params, $settingsService)->update(); + + $this->assertInstanceOf(JSONResponse::class, $response); + $this->assertSame(200, $response->getStatus()); + + $data = $response->getData(); + $this->assertTrue($data['success']); + $this->assertSame('Settings updated successfully', $data['message']); + $this->assertSame(['catalog' => 'main'], $data['data']['configuration']['configuration']); + $this->assertSame(['users'], $data['data']['userGroups']['generic']); + $this->assertSame(['smtpHost' => 'mail.example.org'], $data['data']['emailSettings']); + } + + /** + * `update()` must NOT absorb the other configuration surfaces. + * + * A body carrying only keys that belong to the dedicated endpoints + * (general/sync/cronjob/eol-sync config) must persist nothing. + */ + public function testUpdateIsNotACatchAllForOtherSettingsSurfaces(): void + { + $settingsService = $this->createMock(SettingsService::class); + $settingsService->expects($this->never())->method('updateSettings'); + $settingsService->expects($this->never())->method('setGenericUserGroups'); + $settingsService->expects($this->never())->method('updateEmailSettings'); + + $params = [ + 'catalogLocation' => '/somewhere', + 'syncTimeWindow' => '30', + 'cronjobs' => ['enabled' => true], + 'eolSync' => ['enabled' => true], + ]; + + $response = $this->makeController($params, $settingsService)->update(); + + $this->assertSame(200, $response->getStatus()); + $this->assertSame([], $response->getData()['data']); + } + + /** + * Invalid group names still short-circuit to 400 through `update()`. + */ + public function testUpdateReturns400OnInvalidGroupNames(): void + { + $settingsService = $this->createMock(SettingsService::class); + $settingsService->method('validateGroups') + ->willReturn(['valid' => [], 'invalid' => ['no-such-group']]); + $settingsService->expects($this->never())->method('setGenericUserGroups'); + + $response = $this->makeController( + ['userGroups' => ['generic' => ['no-such-group']]], + $settingsService + )->update(); + + $this->assertSame(400, $response->getStatus()); + $this->assertSame('Invalid generic group names provided', $response->getData()['error']); + } + + /** + * `create()` is a pure delegation to `update()` — identical payload. + */ + public function testCreateDelegatesToUpdateAndReturnsTheSamePayload(): void + { + $params = ['configuration' => ['catalog' => 'main']]; + + $makeService = function (): SettingsService { + $svc = $this->createMock(SettingsService::class); + $svc->method('updateSettings')->willReturn(['configuration' => ['catalog' => 'main']]); + return $svc; + }; + + $viaUpdate = $this->makeController($params, $makeService())->update(); + $viaCreate = $this->makeController($params, $makeService())->create(); + + $this->assertSame($viaUpdate->getStatus(), $viaCreate->getStatus()); + $this->assertSame($viaUpdate->getData(), $viaCreate->getData()); + } + + /** + * The delegation is structural, not copy-paste: `create()`'s body is a + * single `return $this->update();`. + */ + public function testCreateBodyIsASingleDelegationCall(): void + { + $ref = new ReflectionMethod(SettingsController::class, 'create'); + $lines = file($ref->getFileName()); + $body = implode( + '', + array_slice($lines, ($ref->getStartLine() - 1), ($ref->getEndLine() - $ref->getStartLine() + 1)) + ); + + $this->assertStringContainsString('return $this->update();', $body); + $this->assertStringNotContainsString( + 'updateConfigSettings', + $body, + 'create() must delegate, not duplicate the write logic.' + ); + } + + /** + * `update()` maps a service failure to the pre-existing 500 shape. + */ + public function testUpdateMapsExceptionsTo500(): void + { + $settingsService = $this->createMock(SettingsService::class); + $settingsService->method('updateSettings')->willThrowException(new \RuntimeException('boom')); + + $response = $this->makeController(['configuration' => ['a' => 'b']], $settingsService)->update(); + + $this->assertSame(500, $response->getStatus()); + $this->assertSame('boom', $response->getData()['error']); + } +} diff --git a/tests/Unit/SettingsRouteTableTest.php b/tests/Unit/SettingsRouteTableTest.php new file mode 100644 index 00000000..ec2a6e16 --- /dev/null +++ b/tests/Unit/SettingsRouteTableTest.php @@ -0,0 +1,167 @@ + + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\SoftwareCatalog\Tests\Unit; + +use OCA\SoftwareCatalog\Controller\SettingsController; +use PHPUnit\Framework\TestCase; +use ReflectionClass; + +/** + * Evaluates `appinfo/routes.php` and pins the canonical `/api/settings` verbs. + * + * The route table is EVALUATED, not grepped: a commented-out entry or a line + * inside a string would satisfy a grep while the router never sees it. + * + * @spec openspec/specs/method-decomposition/spec.md#requirement-settingscontroller-settings-crud-endpoints-req-decomp-013 + */ +class SettingsRouteTableTest extends TestCase +{ + /** + * The evaluated route entries. + * + * @return array> + */ + private function routes(): array + { + $table = require __DIR__.'/../../appinfo/routes.php'; + + $this->assertIsArray($table); + $this->assertArrayHasKey('routes', $table); + $this->assertIsArray($table['routes']); + + return $table['routes']; + } + + /** + * Find every entry matching a name/url/verb triple. + * + * @return array> + */ + private function match(array $routes, string $name, string $url, string $verb): array + { + return array_values( + array_filter( + $routes, + static fn ($r) => ($r['name'] ?? null) === $name + && ($r['url'] ?? null) === $url + && ($r['verb'] ?? null) === $verb + ) + ); + } + + /** + * The canonical AppHost triples on `/api/settings`. + * + * @return array + */ + public static function canonicalRouteProvider(): array + { + return [ + 'GET read' => ['settings#index', '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/api/settings', 'GET'], + 'POST legacy write' => ['settings#create', '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/api/settings', 'POST'], + 'PUT canonical write' => ['settings#update', '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/api/settings', 'PUT'], + ]; + } + + /** + * Each canonical route must be registered exactly once. + * + * @dataProvider canonicalRouteProvider + */ + public function testCanonicalRouteIsRegistered(string $name, string $url, string $verb): void + { + $hits = $this->match($this->routes(), $name, $url, $verb); + + $this->assertCount( + 1, + $hits, + sprintf('Expected exactly one route %s %s → %s, found %d.', $verb, $url, $name, count($hits)) + ); + } + + /** + * Positive control: the route table must be non-trivial. + * + * If `routes.php` returned an empty list the matcher above would report + * "not found" for the right reason, but a matcher bug that scanned + * nothing would look identical. This pins that routes were inspected. + */ + public function testPositiveControlRouteTableIsPopulated(): void + { + $routes = $this->routes(); + $inspected = count($routes); + + $this->assertGreaterThan(0, $inspected, 'Positive control: the route table evaluated to zero entries.'); + $this->assertGreaterThan( + 50, + $inspected, + 'Positive control: far fewer routes than expected — the table did not evaluate fully.' + ); + + $settingsRoutes = array_filter( + $routes, + static fn ($r) => str_starts_with((string) ($r['name'] ?? ''), 'settings#') + ); + $this->assertGreaterThan(0, count($settingsRoutes), 'Positive control: no settings# routes matched.'); + } + + /** + * Every route entry must target a method that actually exists (gate-14). + */ + public function testEverySettingsRouteTargetsAnExistingPublicMethod(): void + { + $reflection = new ReflectionClass(SettingsController::class); + $checked = 0; + + foreach ($this->routes() as $route) { + $name = (string) ($route['name'] ?? ''); + if (str_starts_with($name, 'settings#') === false) { + continue; + } + + $method = substr($name, strlen('settings#')); + $this->assertTrue( + $reflection->hasMethod($method), + sprintf('Route %s points at a nonexistent SettingsController::%s().', $name, $method) + ); + $this->assertTrue( + $reflection->getMethod($method)->isPublic(), + sprintf('Route %s targets non-public SettingsController::%s().', $name, $method) + ); + $checked++; + } + + $this->assertGreaterThan(0, $checked, 'Positive control: no settings# routes were checked.'); + } + + /** + * The PUT entry must sit before the SPA `/{path}` catch-all. + */ + public function testCanonicalWriteIsDeclaredBeforeTheSpaCatchAll(): void + { + $routes = $this->routes(); + + $updateIndex = null; + $catchAllIndex = null; + + foreach ($routes as $i => $route) { + if (($route['name'] ?? null) === 'settings#update' && ($route['verb'] ?? null) === 'PUT') { + $updateIndex = $i; + } + + if (($route['url'] ?? null) === '/{path}') { + $catchAllIndex = $i; + } + } + + $this->assertNotNull($updateIndex, 'settings#update PUT is not registered.'); + $this->assertNotNull($catchAllIndex, 'The SPA catch-all is missing — test premise is stale.'); + $this->assertLessThan($catchAllIndex, $updateIndex); + } +}