diff --git a/appinfo/info.xml b/appinfo/info.xml index ed2034065..5ab12f385 100644 --- a/appinfo/info.xml +++ b/appinfo/info.xml @@ -40,7 +40,18 @@ Vrij en open source onder de EUPL-1.2-licentie. **Ondersteuning:** Voor ondersteuning, neem contact op via support@conduction.nl. ]]> - 0.4.6 + + 0.4.7 EUPL-1.2 Conduction Decidesk diff --git a/lib/Settings/decidesk_register.json b/lib/Settings/decidesk_register.json index c46af357c..ff893dd41 100644 --- a/lib/Settings/decidesk_register.json +++ b/lib/Settings/decidesk_register.json @@ -3,7 +3,7 @@ "info": { "title": "Decidesk Register", "description": "Universal decision-making platform for governance bodies, associations, corporate boards, and operational meetings", - "version": "0.7.0" + "version": "0.8.0" }, "x-openregister": { "type": "application", @@ -1386,7 +1386,27 @@ "slug": "decidesk", "title": "Decidesk Register", "description": "Universal decision-making platform for governance bodies, associations, corporate boards, and operational meetings. Holds every governance body, meeting, agenda item, decision, vote, minutes, consultation and integrity object the app reads and writes.", - "version": "0.7.0", + "version": "0.8.0", + "_authorizationNote": "REGISTER-LEVEL BASELINE, and it is a BEHAVIOUR CHANGE. Until this block existed, every decidesk schema without its own `authorization` block was read AND WRITE open to every authenticated user through OpenRegister's own /apps/openregister/api/objects/decidesk/ API — the API the frontend uses directly under ADR-022, which no decidesk controller guard sits in front of. OpenRegister's PermissionHandler::hasGroupPermission() tests `empty($authorization)`, and PHP's empty() is true for null and [] alike, so an ABSENT block takes the same default-OPEN branch as an empty one; `enforce_default_closed` defaults to false and, even when enabled, closes only create/update/delete. Measured on this tree before the change: 93 schemas, 24 carrying a block (all read-only), this register row carrying none — so 69 schemas, including Decision, VotingRound, Vote, Participant and EngagementRecord, granted create/update/delete to any logged-in account. Same shape as docudesk#631, where a plain user overwrote another user's template. THE CASCADE IS WHY THIS SITS ON THE REGISTER ROW: PermissionHandler::resolveAuthorization() uses a schema's own block when it has one and falls back to the register's only when it does not, so this baseline reaches exactly the 69 unprotected schemas and changes nothing for the 24 that already declare their own (their public-read rules are untouched). WHAT EACH ACTION IS FOR: read and list name BOTH `authenticated` AND `public`, and create names `authenticated`, so no read goes dark and any member can still raise a decision, cast a vote or file a reaction — those flows are unchanged. `public` on the READ actions is not a widening: before this block existed the default-OPEN branch granted every action to every principal INCLUDING the anonymous one, so omitting `public` here would have CLOSED anonymous reads — a separate and much larger policy change than the write hole this block exists to fix, and one that would 403 every #[PublicPage] citizen-participation surface. CI proved it rather than theory: the first version of this block omitted `public` and all six PHPUnit legs failed with `User 'Anonymous' does not have permission to 'read' objects in schema 'Meeting'`. `public` appears on NO write action, so the anonymous fail-closed rule from openregister#1955 keeps denying anonymous create/update/delete exactly as it did before. update/delete are NOT granted to `authenticated`, which is the whole fix: OpenRegister bypasses the object OWNER unconditionally and SQL-side before any rule is consulted, and bypasses `admin` too, so the author of an object keeps full control of it and only OTHER users lose the ability to rewrite or destroy it. `decidesk-administrators` is the named Nextcloud group for a griffie/secretariat that must edit records it did not create; it is admin-provisioned, and on an instance where it does not exist this simply resolves to owner-plus-admin, which is fail-closed. EVERY ACTION IS WRITTEN OUT ON PURPOSE: once a block is non-empty, OpenRegister DENIES any action the block omits, so an omitted `list` or `create` here would silently break the app rather than secure it. Per-body actor authorization (chair / signatory scopes, x-decidesk-rbac-scopes above) is a separate layer and is unaffected — OpenRegister cannot template a group name per object, which is why that determination is made at the app boundary.", + "authorization": { + "read": [ + "authenticated", + "public" + ], + "list": [ + "authenticated", + "public" + ], + "create": [ + "authenticated" + ], + "update": [ + "decidesk-administrators" + ], + "delete": [ + "decidesk-administrators" + ] + }, "schemas": [ "action-item", "advies", diff --git a/openspec/specs/authorization-via-or-rbac/spec.md b/openspec/specs/authorization-via-or-rbac/spec.md index 7698aad10..54d52add5 100644 --- a/openspec/specs/authorization-via-or-rbac/spec.md +++ b/openspec/specs/authorization-via-or-rbac/spec.md @@ -118,3 +118,37 @@ caller as "check skipped". @e2e exclude fail-closed edge (unresolvable body scope) with no distinct UI flow; unit-proven in GovernanceScopeGuardTest (fails closed when the body is unresolvable / on OR error) and MeetingServiceTest (chair-only transition denied when the governanceBody cannot be resolved). + +### Requirement: REQ-RBAC-006 The register declares an authorization baseline so an absent block cannot grant writes +The `decidesk` register row SHALL declare an `authorization` block naming EVERY canonical +OpenRegister action (`read`, `list`, `create`, `update`, `delete`). `read`, `list` and `create` +SHALL be granted to `authenticated`; `update` and `delete` SHALL NOT be, so that a user who is +neither the object's owner, nor a Nextcloud admin, nor a member of the named administrator group +cannot rewrite or destroy another user's decidesk object through OpenRegister's own +`/apps/openregister/api/objects/decidesk/` API. Schemas that declare their own +`authorization` block SHALL keep it — OpenRegister resolves the schema block first and falls back to +the register's only when a schema has none — and those blocks SHALL continue to name read actions +only. The register version, the configuration version and the app version SHALL all be bumped in the +same change, because the register import skips on a non-newer version with no content fallback and +the `` repair step that performs the import runs only on `occ upgrade`. + +#### Scenario: A non-owner cannot rewrite another user's object +- **GIVEN** a Decision created by user A, and user B who is not an admin and not in the + administrator group +- **WHEN** user B issues an update or delete against that Decision through OpenRegister's object API +- **THEN** OpenRegister denies the write +- **AND WHEN** user A issues the same write on their own object +- **THEN** OpenRegister permits it via the unconditional owner bypass. + +#### Scenario: Reads and creates are unchanged +- **GIVEN** any authenticated user +- **WHEN** the user lists or reads decidesk objects, or creates a new one +- **THEN** the action is permitted exactly as before the baseline was declared. + +#### Scenario: The baseline names every action +- **GIVEN** the shipped register row +- **WHEN** its `authorization` block is read +- **THEN** every canonical action is named with a non-empty rule list, because OpenRegister denies + any action a non-empty block omits — an unnamed action would break the app rather than secure it. + +@e2e exclude The assertion is a per-user DENIAL by OpenRegister's own permission evaluator against a declaration this repo ships, and the owner bypass is unconditional and SQL-side — so a browser test driven by a single seeded (and therefore owning, usually admin) session cannot observe it at all, and would report success over the exact hole. Pinned by `tests/Unit/RegisterAuthorizationTest.php` on the declaration side; the per-user behaviour needs a two-account probe against a live instance, recorded in the PR as verification owed rather than claimed. diff --git a/tests/Unit/RegisterAuthorizationTest.php b/tests/Unit/RegisterAuthorizationTest.php new file mode 100644 index 000000000..9ac736fb0 --- /dev/null +++ b/tests/Unit/RegisterAuthorizationTest.php @@ -0,0 +1,422 @@ +` — the API the frontend uses + * directly under ADR-022 — and NO decidesk controller guard sits in front of it. + * What decides who may write there is the `authorization` block on the schema, or + * failing that on the REGISTER row. + * + * This tree had neither. OpenRegister's + * `Service/Object/PermissionHandler::hasGroupPermission()` tests + * `empty($authorization)`, and PHP's `empty()` is true for `null` and `[]` alike, + * so an ABSENT block takes the same default-OPEN branch as an empty one: + * + * if (empty($authorization) === true || $publicOptIn === true) { + * if ($this->isDefaultClosedEnforced() === true + * && in_array($action, self::DEFAULT_CLOSED_WRITE_ACTIONS, true) + * && $publicOptIn === false) { return false; } + * return true; // <- create / update / delete, granted + * } + * + * `enforce_default_closed` reads `IAppConfig` with `default: false`, so on a stock + * instance that guard never fires. Measured before the fix: 93 schemas, 24 with a + * block (all `read`-only), and this register row with none — 69 schemas, Decision + * / VotingRound / Vote / Participant / EngagementRecord among them, granting + * create, update AND delete to any logged-in account. + * + * These tests do not re-implement OpenRegister's evaluator — an instrument built + * from the same source as the bug reports zero. They pin the DECLARATION the + * evaluator reads, which is the part this repository owns, and they pin the two + * version fields without which the declaration never reaches an instance. + * + * @category Test + * @package OCA\Decidesk\Tests\Unit + * + * @author Conduction Development Team + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://conduction.nl + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + * + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * SPDX-License-Identifier: EUPL-1.2 + */ + +declare(strict_types=1); + +namespace OCA\Decidesk\Tests\Unit; + +use PHPUnit\Framework\TestCase; + +/** + * The register-level authorization baseline is declared, complete, and deployable. + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ +class RegisterAuthorizationTest extends TestCase { + + /** + * Actions OpenRegister treats as writes on an object. + * + * `PermissionHandler::DEFAULT_CLOSED_WRITE_ACTIONS` and + * `ANONYMOUS_FAIL_CLOSED_WRITE_ACTIONS` are both exactly this set. + * + * @var array + */ + private const WRITE_ACTIONS = ['create', 'update', 'delete']; + + /** + * Every canonical action, from `PermissionHandler::CANONICAL_ACTIONS`. + * + * @var array + */ + private const CANONICAL_ACTIONS = ['read', 'create', 'update', 'delete', 'list']; + + /** + * The decoded main register file. + * + * @var array + */ + private array $register; + + /** + * Decode the shipped register JSON. + * + * @return void + */ + protected function setUp(): void { + parent::setUp(); + + $path = __DIR__ . '/../../lib/Settings/decidesk_register.json'; + $this->assertFileExists($path); + + $raw = file_get_contents($path); + $this->assertIsString($raw); + + $decoded = json_decode($raw, true); + $this->assertIsArray($decoded, 'decidesk_register.json must be valid JSON.'); + + $this->register = $decoded; + }//end setUp() + + /** + * The `decidesk` register row itself. + * + * @return array The register row. + */ + private function registerRow(): array { + $row = $this->register['components']['registers']['decidesk'] ?? null; + $this->assertIsArray($row, 'The decidesk register row must exist.'); + + return $row; + }//end registerRow() + + /** + * The register row declares an authorization block naming EVERY canonical action. + * + * Completeness is the load-bearing half. Once a block is non-empty, + * `hasGroupPermission()` reaches `if (empty($authorization[$action])) return false;` + * — so an action this block forgets to name is DENIED to everyone but the + * object owner and admin. A block that omitted `create` or `list` would not + * secure the app, it would break it. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testTheRegisterDeclaresACompleteAuthorizationBlock(): void { + $authorization = $this->registerRow()['authorization'] ?? null; + + $this->assertIsArray( + $authorization, + 'The decidesk register row must declare an `authorization` block. Without one, ' + . 'PermissionHandler::hasGroupPermission() takes its default-OPEN branch for every ' + . 'schema that declares no block of its own, granting create/update/delete on ' + . 'Decision, Vote, VotingRound, Participant and EngagementRecord to every ' + . 'authenticated user through OpenRegister own object API.' + ); + $this->assertNotEmpty($authorization, 'An EMPTY block is evaluated identically to an absent one.'); + + foreach (self::CANONICAL_ACTIONS as $action) { + $this->assertArrayHasKey( + $action, + $authorization, + sprintf( + 'The block must name `%s`: OpenRegister DENIES any action a non-empty block omits, ' + . 'so an unnamed action breaks the app rather than securing it.', + $action + ) + ); + $this->assertNotEmpty( + $authorization[$action], + sprintf('`%s` must grant to someone — an empty rule list reads as "grant to nobody".', $action) + ); + } + }//end testTheRegisterDeclaresACompleteAuthorizationBlock() + + /** + * Reads and creates stay open to any authenticated user. + * + * This is what makes the change safe to land: no read goes dark and every + * member can still raise a decision, cast a vote or file a reaction. If this + * test ever goes red, the fix has become an outage. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testReadListAndCreateStayOpenToAuthenticatedUsers(): void { + $authorization = $this->registerRow()['authorization']; + + foreach (['read', 'list', 'create'] as $action) { + $this->assertContains( + 'authenticated', + $authorization[$action], + sprintf('`%s` must stay granted to `authenticated` — narrowing it is an outage, not a fix.', $action) + ); + } + }//end testReadListAndCreateStayOpenToAuthenticatedUsers() + + /** + * The READ actions still name `public`, so anonymous reads are not collaterally closed. + * + * This test exists because CI found the omission and nothing local could. Before + * any block existed, `hasGroupPermission()` took its default-OPEN branch for EVERY + * principal, the anonymous one included — so a block that names only + * `authenticated` on `read` does not preserve the status quo, it CLOSES anonymous + * reads. Omission is the deny. + * + * The first version of this block omitted `public`, and all six PHPUnit legs + * failed with `NotAuthorizedException: User 'Anonymous' does not have permission + * to 'read' objects in schema 'Meeting'` — PHPUnit's CLI has no session, so it + * exercises exactly the path a `#[PublicPage]` citizen-participation surface + * takes. Closing anonymous reads may well be desirable, but it is a far larger + * policy change than the write hole this block fixes and does not belong in it. + * + * Writes are asserted `public`-free separately, so this cannot drift into a + * blanket anonymous grant. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testAnonymousReadsAreNotCollaterallyClosed(): void { + $authorization = $this->registerRow()['authorization']; + + foreach (['read', 'list'] as $action) { + $this->assertContains( + 'public', + $authorization[$action], + sprintf( + '`%s` must still name `public`. Before this block existed the default-OPEN branch ' + . "granted it to the anonymous principal too, so dropping it 403s every " + . '#[PublicPage] surface — a policy change, not a security fix.', + $action + ) + ); + } + }//end testAnonymousReadsAreNotCollaterallyClosed() + + /** + * Update and delete are NOT granted to every authenticated user. + * + * This is the whole security change, and it is the assertion that would have + * failed on `development`. It says nothing about the object OWNER: OpenRegister + * bypasses the owner unconditionally, SQL-side, before any rule is consulted, so + * an author keeps full control of their own object and only OTHER users lose the + * ability to rewrite or destroy it. + * + * The `public` pseudo-group is asserted absent separately: granting it would + * re-open anonymous writes that OpenRegister#1955 closed by default. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testUpdateAndDeleteAreNotOpenToEveryAuthenticatedUser(): void { + $authorization = $this->registerRow()['authorization']; + + foreach (['update', 'delete'] as $action) { + $this->assertNotContains( + 'authenticated', + $authorization[$action], + sprintf( + '`%s` must NOT be granted to `authenticated`: that is exactly the default-open state ' + . 'this block exists to close, and it is the shape that let a plain user overwrite ' + . "another user's record in docudesk#631.", + $action + ) + ); + $this->assertNotContains( + 'public', + $authorization[$action], + sprintf('`%s` must not be granted to the `public` pseudo-group.', $action) + ); + } + }//end testUpdateAndDeleteAreNotOpenToEveryAuthenticatedUser() + + /** + * No write action is granted to the `public` pseudo-group anywhere. + * + * `publicGroupExplicitlyGranted()` is the one thing that re-opens anonymous + * writes past OpenRegister#1955's fail-closed rule, so a stray `public` entry + * on a write action would be a wider hole than the one being closed. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testNoWriteActionGrantsThePublicPseudoGroup(): void { + $authorization = $this->registerRow()['authorization']; + + foreach (self::WRITE_ACTIONS as $action) { + foreach ($authorization[$action] as $entry) { + $group = is_array($entry) ? ($entry['group'] ?? null) : $entry; + $this->assertNotSame( + 'public', + $group, + sprintf('`%s` must not grant the `public` pseudo-group — that re-opens anonymous writes.', $action) + ); + } + } + }//end testNoWriteActionGrantsThePublicPseudoGroup() + + /** + * The register version was bumped, or the block never reaches an instance. + * + * `ImportHandler`'s register path skips outright when the incoming version is + * `<=` the stored one, and — unlike the SCHEMA path, which falls back to a + * content comparison — it has NO content-differs escape. A correct block with + * an unbumped version is a fix that deploys nowhere and reports success. + * + * The assertion is against the last version that shipped WITHOUT the block, so + * it stays meaningful as the register keeps evolving. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testTheRegisterVersionMovedPastTheUnprotectedRelease(): void { + $lastUnprotected = '0.7.0'; + + $this->assertGreaterThan( + 0, + version_compare($this->registerRow()['version'] ?? '0.0.0', $lastUnprotected), + sprintf( + 'The register version must be greater than %s (the last release with no authorization ' + . 'block), or ImportHandler skips the import and the fix never lands.', + $lastUnprotected + ) + ); + $this->assertGreaterThan( + 0, + version_compare($this->register['info']['version'] ?? '0.0.0', $lastUnprotected), + 'The configuration version must move with it.' + ); + }//end testTheRegisterVersionMovedPastTheUnprotectedRelease() + + /** + * The app version moved too, or `occ upgrade` never runs the import. + * + * `InitializeSettings` is a `` repair step, and post-migration + * steps run only on `occ upgrade` — which is a NO-OP when the app version has + * not changed. Both bumps are needed; either alone is a fix that sits on disk. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testTheAppVersionMovedSoTheRepairStepRuns(): void { + $path = __DIR__ . '/../../appinfo/info.xml'; + $this->assertFileExists($path); + + $source = file_get_contents($path); + $this->assertIsString($source); + + // Read as TEXT, not through SimpleXML. The first version of this test used + // `simplexml_load_file()`; it worked locally and returned FALSE on every CI + // leg, failing the whole suite on `appinfo/info.xml must be readable XML`. + // The assertion here is about one scalar in a file this repository owns, so + // it should not depend on an XML extension being present in the runner. + $this->assertSame( + 1, + preg_match('#([^<]+)#', $source, $version), + 'appinfo/info.xml must declare a .' + ); + + $this->assertGreaterThan( + 0, + version_compare(trim($version[1]), '0.4.6'), + 'The app version must be greater than 0.4.6 (the last release with no authorization ' + . 'block), or `occ upgrade` is a no-op and InitializeSettings never re-imports the register.' + ); + + // Positive control on the same reader: the file really was read and matched, + // so a failure above means "not bumped", never "read an empty document". + $this->assertStringContainsString('decidesk', $source); + }//end testTheAppVersionMovedSoTheRepairStepRuns() + + /** + * The 24 schemas that already declare their own block are untouched. + * + * `PermissionHandler::resolveAuthorization()` uses a schema's own block when it + * has one and falls back to the register's only when it does not. That cascade + * is the entire reason this baseline could be applied in one place, and it is + * also the thing that would silently break the public-read rules if a schema + * block were "helpfully" removed later. Those blocks grant `read` to the + * `public` group under a publication-date match; they name no write action, so + * they already fail closed on writes. + * + * @return void + * + * @spec openspec/specs/authorization-via-or-rbac/spec.md#requirement-req-rbac-006-the-register-declares-an-authorization-baseline-so-an-absent-block-cannot-grant-writes + */ + public function testSchemasWithTheirOwnBlockStillDeclareOnlyReads(): void { + $files = array_merge( + [__DIR__ . '/../../lib/Settings/decidesk_register.json'], + glob(__DIR__ . '/../../lib/Settings/register.d/*.json') ?: [] + ); + + $withBlock = 0; + foreach ($files as $file) { + $decoded = json_decode((string)file_get_contents($file), true); + foreach (($decoded['components']['schemas'] ?? []) as $name => $schema) { + if (is_array($schema) === false || isset($schema['authorization']) === false) { + continue; + } + + $withBlock++; + foreach (self::WRITE_ACTIONS as $action) { + $this->assertArrayNotHasKey( + $action, + $schema['authorization'], + sprintf( + 'Schema `%s` (%s) declares its own authorization block, which SHADOWS the ' + . 'register baseline entirely. Adding a write action here opts that schema ' + . 'out of the baseline — do it deliberately and update this test, never by ' + . 'accident.', + $name, + basename($file) + ) + ); + } + } + } + + // The count is the positive control: without it this loop passes vacuously + // if the schemas move, are renamed, or stop being found at all. + $this->assertSame( + 24, + $withBlock, + 'Expected 24 schema-level authorization blocks. A different number means schemas gained or ' + . 'lost their own block, which changes which ones the register baseline governs.' + ); + }//end testSchemasWithTheirOwnBlockStillDeclareOnlyReads() +}//end class