From 4eaecb4e1627eb5c4ca9730c1a54f9a13b9df7b5 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 7 May 2026 13:08:05 +0200 Subject: [PATCH 01/13] feat(openspec): rbac-disable-public-inheritance MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an opt-out for the "logged-in users inherit public group rights" semantics in OR's RBAC. Schemas and registers gain an optional inheritFromPublic boolean (default true, backwards-compatible). When false, authenticated users do NOT qualify for public rules — they qualify only via their own group memberships. Anonymous users see no behaviour change. Cascade: schema → register → IAppConfig openregister.rbac.inherit_from_public_default → hard-coded true. Implementation touches both RBAC layers identically: - PHP-side PermissionHandler::hasPermission inheritance fallback (line 229-241) - SQL-side MagicRbacHandler::processConditionalRule + processSimpleRule (and their UNION-mode siblings buildRbacConditionsSql + processConditionalRuleSql) Modified capability: rbac-scopes. Tracks GitHub issue #1439. --- .../.openspec.yaml | 2 + .../rbac-disable-public-inheritance/design.md | 202 ++++++++++++++++++ .../rbac-disable-public-inheritance/plan.json | 50 +++++ .../proposal.md | 69 ++++++ .../specs/rbac-scopes/spec.md | 155 ++++++++++++++ .../rbac-disable-public-inheritance/tasks.md | 73 +++++++ 6 files changed, 551 insertions(+) create mode 100644 openspec/changes/rbac-disable-public-inheritance/.openspec.yaml create mode 100644 openspec/changes/rbac-disable-public-inheritance/design.md create mode 100644 openspec/changes/rbac-disable-public-inheritance/plan.json create mode 100644 openspec/changes/rbac-disable-public-inheritance/proposal.md create mode 100644 openspec/changes/rbac-disable-public-inheritance/specs/rbac-scopes/spec.md create mode 100644 openspec/changes/rbac-disable-public-inheritance/tasks.md diff --git a/openspec/changes/rbac-disable-public-inheritance/.openspec.yaml b/openspec/changes/rbac-disable-public-inheritance/.openspec.yaml new file mode 100644 index 0000000000..8d87be18e5 --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-05-07 diff --git a/openspec/changes/rbac-disable-public-inheritance/design.md b/openspec/changes/rbac-disable-public-inheritance/design.md new file mode 100644 index 0000000000..4c1092c894 --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/design.md @@ -0,0 +1,202 @@ +## Context + +OpenRegister's authorization model treats the `public` group as a baseline that authenticated users always inherit. The intent is intuitive — "if it's visible to anyone (public), it's visible to anyone logged in" — and matches the most common case. The behaviour appears in two places: + +- **PHP-side** (`PermissionHandler::hasPermission`, line 229-241): after iterating the user's groups and finding no match, the method falls back to evaluating `hasGroupPermission(public, ...)`. This is the explicit inheritance fallback. +- **SQL-side** (`MagicRbacHandler::processConditionalRule`, line 307-309 and `processSimpleRule`, line 268-271): when the rule's group is `public`, the user qualifies for the rule REGARDLESS of authentication state. Match conditions, if any, then apply. + +This is a global, hard-coded policy. There's no per-schema or per-tenant way to opt out. For schemas where authentication should be a strict gate — not a superset of public access — operators have to work around it: they cannot grant public conditional rules without those grants leaking to authenticated users. + +This change adds a single boolean — `inheritFromPublic` — at the authorization-block level (schema, with cascade to register, with fallback to a tenant-wide IAppConfig default). When `false`, authenticated users do NOT qualify for `public` rules; they qualify only via their own group memberships. Anonymous (unauthenticated) users see no behaviour change. Default stays `true` (backwards-compatible), so existing schemas retain today's semantics. + +The implementation is small but spans both RBAC layers — the PHP-side check (per-object) and the SQL-side filter (listing). Both must honour the flag identically; otherwise listings and per-object reads would diverge. + +## Goals / Non-Goals + +**Goals:** + +- Add an `inheritFromPublic` boolean to the authorization block (schema and register), plus a tenant-wide default via `IAppConfig`. +- Resolve the effective value via cascade — schema → register → tenant default → hard-coded `true`. +- When `false`, authenticated users do NOT qualify for `public` rules in either the PHP-side check or the SQL-side filter. +- Anonymous users see no change. Backwards-compatible default. +- Unit-test the four-state matrix (anon × authenticated × flag-on/off) for both layers. + +**Non-Goals:** + +- Per-rule audience targeting (Option β from exploration). One flag per authorization block; no per-rule control. +- A new first-class `authenticated` group concept. +- A schema-editor UI toggle for the flag. v1 ships as a JSON field on the existing schema authorization editor surface. +- Retroactively re-evaluate decisions on stored objects. +- Per-action variation. The flag covers all actions (read, create, update, delete) uniformly. + +## Decisions + +### D1. Flag name: `inheritFromPublic` + +Reads cleanly: `inheritFromPublic: false` says "don't inherit from public". Alternatives considered: + +- `publicAppliesToAuthenticated` — verbose, awkward double-negative ("set to false to mean public doesn't apply to authenticated users"). +- `authenticatedInheritsPublic` — flips the subject; reads okay, but `inheritFromPublic` is more in line with how the docs describe the behaviour ("inheritance from public"). +- `publicScopedToAnonymous` — different framing entirely, harder to reason about. + +`inheritFromPublic` matches the existing comment in `PermissionHandler.php:229` which talks about logged-in users having "the same rights as 'public' users". Operators reading the existing code will recognise the inversion immediately. + +### D2. Default `true` (backwards-compatible) + +The current behaviour is `inheritFromPublic = true` (implicit). Defaulting to `true` keeps every existing schema running unchanged. Operators who want strict-gate semantics opt-in per-schema (or flip the tenant default). + +**Alternative considered:** default `false` (privacy-positive). Rejected for v1 — flipping the global default is a behaviour change for every existing install, and many operators are accustomed to the current semantics. A future change can flip the default if community feedback supports it. + +### D3. Cascade: schema → register → tenant default → hard-coded true + +The cascade mirrors how `resolveAuthorization` already cascades the rest of the authorization config. The lookup logic: + +``` + resolveInheritFromPublic($schema): + 1. if $schema->getAuthorization()['inheritFromPublic'] is set: + return that value + 2. else if $register->getAuthorization()['inheritFromPublic'] is set: + return that value + 3. else if IAppConfig has 'openregister.rbac.inherit_from_public_default': + return that value (parsed as boolean) + 4. else: return true +``` + +**Rationale:** consistency with the existing authorization-resolution pattern. Operators already mentally model the cascade for the rest of the authorization block; extending it with one more field follows the same shape. + +**Caching:** the resolved value is cached per-request keyed by schema ID. Avoids repeated cascade lookups inside a single listing call where the same schema is checked many times. + +### D4. PHP-side enforcement: wrap the inheritance fallback + +In `PermissionHandler::hasPermission`, the inheritance block at line 229-241 becomes: + +```php +// Logged-in users should also have at least the same rights as 'public' users — +// unless inheritFromPublic is disabled for this schema. +if ($this->resolveInheritFromPublic(schema: $schema) === true) { + if ($this->hasGroupPermission( + authorization: $authorization, + groupId: 'public', + ... + ) === true + ) { + return true; + } +} +``` + +When `inheritFromPublic` is `false`, the public fallback is skipped. The user's per-group checks are the ONLY way to grant access. (Owner check at line 543, admin check at line 209, and explicit group membership at the foreach on line 214 still apply normally.) + +### D5. SQL-side enforcement: guard the public-qualification check + +In `MagicRbacHandler::processConditionalRule` (line 296-328) and `processConditionalRuleSql` (line 857-882) — both: + +```php +$userQualifies = false; +if ($group === 'public') { + if ($inheritFromPublic === false && $userId !== null) { + $userQualifies = false; // authenticated user excluded + } else { + $userQualifies = true; + } +} else if ($group === 'authenticated' && $userId !== null) { + $userQualifies = true; +} else if (in_array($group, $userGroups, true) === true) { + $userQualifies = true; +} +``` + +In `processSimpleRule` (line 266-284): + +```php +if ($rule === 'public') { + if ($inheritFromPublic === false && $userId !== null) { + return false; // authenticated user does NOT get unconditional public access + } + return true; +} +``` + +The flag is plumbed through from `applyRbacFilters` / `buildRbacConditionsSql`, both of which call `resolveInheritFromPublic($schema)` once at the top and pass the value down to the rule-processing helpers. + +### D6. `resolveInheritFromPublic` helper lives on `PermissionHandler` + +`PermissionHandler::resolveAuthorization` already implements the schema-then-register cascade. The new helper sits next to it and follows the same pattern: + +```php +public function resolveInheritFromPublic(Schema $schema): bool +{ + // Cache per-request keyed by schema ID. + if (isset($this->cachedInheritFromPublic[$schema->getId()])) { + return $this->cachedInheritFromPublic[$schema->getId()]; + } + + $value = null; + + $auth = $schema->getAuthorization(); + if (isset($auth['inheritFromPublic'])) { + $value = (bool) $auth['inheritFromPublic']; + } else { + $register = $this->getRegisterForSchema(schema: $schema); + if ($register !== null) { + $registerAuth = $this->getRegisterAuthorization(registerId: $register->getId()); + if (isset($registerAuth['inheritFromPublic'])) { + $value = (bool) $registerAuth['inheritFromPublic']; + } + } + } + + if ($value === null) { + $value = $this->appConfig->getValueBool( + app: 'openregister', + key: 'rbac.inherit_from_public_default', + default: true + ); + } + + $this->cachedInheritFromPublic[$schema->getId()] = $value; + return $value; +} +``` + +`MagicRbacHandler` reuses the same helper via DI (it already injects `PermissionHandler` for `resolveAuthorization` purposes per `MagicRbacHandler.php:1320`). + +### D7. The `authenticated` rule string is unaffected + +`MagicRbacHandler::processSimpleRule` already has a special case for `'authenticated'` strings — they grant unconditional access to any logged-in user. That behaviour is independent of `inheritFromPublic` and stays as-is. A schema author who wants "all authenticated users can read" continues to use the literal `'authenticated'` rule. The new flag concerns the `public` group ONLY. + +### D8. No new IAppConfig parsing — reuse existing pattern + +The tenant-wide default key `openregister.rbac.inherit_from_public_default` follows the existing `IAppConfig` access pattern used elsewhere in OR (e.g. `getValueBool` for boolean settings). No new config-parsing infrastructure needed. + +### D9. Schema serialisation preserves the field + +`Schema::getAuthorization()` returns the JSON-decoded authorization block. Adding a new field at that level is transparent — the field is preserved through `setAuthorization` / `getAuthorization` round-trips because the Schema entity stores the JSON verbatim. No mapper changes needed. + +## Risks / Trade-offs + +- **[Behaviour change for tenants who flip the global default]** → Mitigation: documented prominently in CHANGELOG as a deliberate opt-in. A tenant flipping `inherit_from_public_default` to `false` MUST audit existing schemas with public-conditional rules to confirm authenticated users were not relying on those grants. Same risk applies for per-schema flips. +- **[Discoverability]** → The flag is a JSON field on the authorization block; not surfaced in any UI in v1. Mitigation: documented in `docs/`; admin UI is a clear follow-up if adoption is slow. +- **[Inconsistency between PHP-side and SQL-side checks]** → Both must honour the flag identically. Mitigation: shared `resolveInheritFromPublic` helper; unit tests cover the four-state matrix on BOTH layers and verify identical results. +- **[Config not respected when `inheritFromPublic` is set explicitly to `null`]** → JSON allows null. The cascade treats `null` as "unset" (falls through to next level). Documented behaviour; tested explicitly. +- **[Caching staleness when an admin updates the schema]** → The per-request cache is fine within a request. Across requests, a schema update invalidates the in-memory cache automatically (each request is a fresh PHP process). No persistent cache, no cache invalidation needed. +- **[`authenticated` rule could now be the cleaner alternative for some operators]** → If a tenant just wants "all logged-in users can read this", `authenticated` rules already exist and may be simpler than disabling inheritance. Documentation should describe both options and when each is appropriate. + +## Migration Plan + +1. Land the helper + flag plumbing in PHP and SQL paths. Default is `true` everywhere; behaviour is unchanged for schemas / registers / tenants that don't set it. +2. Add tests for the four-state matrix. +3. Document the flag in `docs/` (extend the RBAC documentation that already exists for `rbac-scopes`). +4. Release. Operators that want the new behaviour set `inheritFromPublic: false` per-schema (or flip the tenant default). + +**Rollback:** since the default preserves current behaviour, rolling back is just removing the helper and the guards. If a tenant was relying on `inheritFromPublic: false` for privacy enforcement, rolling back would re-grant authenticated users access to public-conditional rules — operators must revisit their schemas. This is expected for any RBAC change. + +## Seed Data + +Not applicable — this change extends authorization metadata on existing schemas, not new schemas. Existing seed objects (per ADR-016 in `docudesk_register.json` and similar) work unchanged. + +## Open Questions + +- **Should `inheritFromPublic` be exposed on the OAS schema?** Probably yes (the JSON field is part of the authorization block, which is an OR-managed schema property). Confirm during apply by inspecting `OasService::expandRolesForOas`. +- **Should `authenticated` rule support match conditions?** Out of scope here, but worth noting: the simple-rule `authenticated` returns `true` unconditionally. A future change could allow `{group: "authenticated", match: {...}}` for parity with `{group: "public", match: {...}}`. Not addressed in this change. +- **Logging when inheritance is disabled at request time?** A debug-level log entry could help operators diagnose "why can't this authenticated user see the object" cases. Provisional: log at debug level when `inheritFromPublic === false` causes a denial that would have succeeded under inheritance. Confirm during apply if log volume is acceptable. diff --git a/openspec/changes/rbac-disable-public-inheritance/plan.json b/openspec/changes/rbac-disable-public-inheritance/plan.json new file mode 100644 index 0000000000..dc246b83c7 --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/plan.json @@ -0,0 +1,50 @@ +{ + "change": "rbac-disable-public-inheritance", + "project": "openregister", + "repo": "ConductionNL/openregister", + "created": "2026-05-07", + "tracking_issue": 1439, + "tracking_issue_url": "https://github.com/ConductionNL/openregister/issues/1439", + "tasks": [ + { "id": "1.1", "section": "resolveInheritFromPublic helper", "title": "Add per-request cache field", "description": "private array $cachedInheritFromPublic = []; on PermissionHandler.php, keyed by schema ID.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, + { "id": "1.2", "section": "resolveInheritFromPublic helper", "title": "Implement cascade resolver", "description": "Public method resolveInheritFromPublic(Schema): bool. Cascade: schema → register → IAppConfig openregister.rbac.inherit_from_public_default → true. null = unset.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-the-effective-value-of-inheritfrompublic-must-be-resolved-via-cascade", "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, + { "id": "1.3", "section": "resolveInheritFromPublic helper", "title": "Wire IAppConfig dependency", "description": "Reuse existing injection; add to constructor if not present.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, + { "id": "1.4", "section": "resolveInheritFromPublic helper", "title": "Cache resolved value per request", "description": "Implicit reset on PHP process boundary.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, + { "id": "1.5", "section": "resolveInheritFromPublic helper", "title": "Unit-test cascade resolution", "description": "Four levels: schema set / register set / tenant set / all unset → true. Plus null = unset semantics.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, + { "id": "2.1", "section": "PHP-side enforcement", "title": "Wrap inheritance fallback in flag check", "description": "PermissionHandler::hasPermission lines 229-241; gate hasGroupPermission(public,...) on resolveInheritFromPublic === true.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, + { "id": "2.2", "section": "PHP-side enforcement", "title": "Confirm anonymous-user behaviour unchanged", "description": "Lines 174-184 if ($user === null) branch checks public — that's the anonymous path, not the inheritance fallback we're guarding.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "2.3", "section": "PHP-side enforcement", "title": "Confirm owner/admin shortcuts unaffected", "description": "Lines 209, 543 — neither depends on the flag.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": [] }, + { "id": "2.4", "section": "PHP-side enforcement", "title": "Four-state matrix unit tests on hasPermission", "description": "(anon, true) grant; (anon, false) grant (anon unaffected); (auth, true) grant; (auth, false) deny.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-php-side-and-sql-side-enforcement-must-be-identical", "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, + { "id": "2.5", "section": "PHP-side enforcement", "title": "Verify owner/admin grants persist", "description": "Both work regardless of the flag.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, + { "id": "3.1", "section": "SQL-side enforcement", "title": "Resolve inheritFromPublic in applyRbacFilters", "description": "Once at the top of MagicRbacHandler::applyRbacFilters via PermissionHandler::resolveInheritFromPublic($schema).", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, + { "id": "3.2", "section": "SQL-side enforcement", "title": "Plumb flag through processAuthorizationRule", "description": "→ processConditionalRule, processSimpleRule. New parameter on each method.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, + { "id": "3.3", "section": "SQL-side enforcement", "title": "Guard processConditionalRule public branch", "description": "When $group === 'public' AND inheritFromPublic === false AND $userId !== null, set $userQualifies = false.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, + { "id": "3.4", "section": "SQL-side enforcement", "title": "Guard processSimpleRule public branch", "description": "When $rule === 'public' AND inheritFromPublic === false AND $userId !== null, return false.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, + { "id": "3.5", "section": "SQL-side enforcement", "title": "Same updates in UNION-based path", "description": "buildRbacConditionsSql, processConditionalRuleSql.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, + { "id": "3.6", "section": "SQL-side enforcement", "title": "Unit-test applyRbacFilters four-state matrix", "description": "Build query, inspect SQL or run against fixture DB.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php"] }, + { "id": "3.7", "section": "SQL-side enforcement", "title": "Unit-test buildRbacConditionsSql four-state matrix", "description": "UNION path equivalent of 3.6.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php"] }, + { "id": "4.1", "section": "Schema entity / serialisation", "title": "Confirm Schema authorization round-trips preserve field", "description": "getAuthorization/setAuthorization round-trip; add regression test if not covered.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-schema-and-register-authorization-must-accept-an-optional-inheritfrompublic-boolean", "files_likely_affected": ["tests/unit/Db/SchemaTest.php"] }, + { "id": "4.2", "section": "Schema entity / serialisation", "title": "Confirm Register authorization preserves field", "description": "Same as 4.1 at register level.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/RegisterTest.php"] }, + { "id": "4.3", "section": "Schema entity / serialisation", "title": "No schema migration needed", "description": "Additive JSON-level field; existing serialisations unchanged.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "5.1", "section": "Tenant default IAppConfig", "title": "Read openregister.rbac.inherit_from_public_default", "description": "Implicit registration via IAppConfig pattern; read by resolveInheritFromPublic.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, + { "id": "5.2", "section": "Tenant default IAppConfig", "title": "Document the IAppConfig key", "description": "Extend RBAC documentation.", "status": "pending", "spec_ref": null, "files_likely_affected": ["docs/"] }, + { "id": "5.3", "section": "Tenant default IAppConfig", "title": "Validate boolean parsing", "description": "Accept true/false/'true'/'false'/'1'/'0'/1/0 via getValueBool or equivalent.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, + { "id": "6.1", "section": "Cross-app integration check", "title": "Smoke-test DocuDesk RBAC flows", "description": "Schemas without inheritFromPublic see no behaviour change.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "6.2", "section": "Cross-app integration check", "title": "Smoke-test OpenCatalogi PublicationsController", "description": "With inheritFromPublic:true (default) auth users still see public-conditional rows; with false they don't.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "6.3", "section": "Cross-app integration check", "title": "Smoke-test other consuming apps", "description": "Default behaviour unchanged.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "7.1", "section": "Unit + integration tests", "title": "PermissionHandlerTest extension", "description": "Four-state matrix on hasPermission; cascade resolution tests.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, + { "id": "7.2", "section": "Unit + integration tests", "title": "MagicRbacHandlerTest extension", "description": "Four-state matrix on applyRbacFilters and buildRbacConditionsSql.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php"] }, + { "id": "7.3", "section": "Unit + integration tests", "title": "Integration test: inheritFromPublic:false schema", "description": "Public-conditional read; anon allowed; auth without explicit group denied; auth with explicit group allowed.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/integration/"] }, + { "id": "7.4", "section": "Unit + integration tests", "title": "Integration test: register-level cascade", "description": "Schema unset; register inheritFromPublic:false → schema honours register's value.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/integration/"] }, + { "id": "7.5", "section": "Unit + integration tests", "title": "Integration test: tenant default", "description": "IAppConfig set to false; schema reads honour tenant default.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/integration/"] }, + { "id": "8.1", "section": "Documentation", "title": "Extend rbac-scopes RBAC docs", "description": "New field, cascade, four-state matrix, authenticated-rule alternative.", "status": "pending", "spec_ref": null, "files_likely_affected": ["docs/"] }, + { "id": "8.2", "section": "Documentation", "title": "Worked example for publication-style schema", "description": "Public-time-window read + inheritFromPublic:false demonstrating tiered visibility.", "status": "pending", "spec_ref": null, "files_likely_affected": ["docs/"] }, + { "id": "8.3", "section": "Documentation", "title": "CHANGELOG under Added", "description": "New inheritFromPublic boolean + tenant default IAppConfig key.", "status": "pending", "spec_ref": null, "files_likely_affected": ["CHANGELOG.md"] }, + { "id": "8.4", "section": "Documentation", "title": "CHANGELOG under Behavior changes", "description": "Flipping is deliberate opt-in; existing schemas unaffected.", "status": "pending", "spec_ref": null, "files_likely_affected": ["CHANGELOG.md"] }, + { "id": "9.1", "section": "Quality and verification", "title": "Full unit test suite clean", "description": "All tests pass.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "9.2", "section": "Quality and verification", "title": "Static analysis clean", "description": "Psalm / PHPStan at project strictness.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "9.3", "section": "Quality and verification", "title": "Code style clean", "description": "PHPCS at project config.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "9.4", "section": "Quality and verification", "title": "Manual smoke against live stack", "description": "Configure schema with inheritFromPublic:false; verify four-state matrix via API requests as anon vs authenticated users.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, + { "id": "9.5", "section": "Quality and verification", "title": "openspec validate clean", "description": "Run openspec validate rbac-disable-public-inheritance.", "status": "pending", "spec_ref": null, "files_likely_affected": [] } + ] +} diff --git a/openspec/changes/rbac-disable-public-inheritance/proposal.md b/openspec/changes/rbac-disable-public-inheritance/proposal.md new file mode 100644 index 0000000000..97f626432f --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/proposal.md @@ -0,0 +1,69 @@ +## Why + +OpenRegister's RBAC currently treats the `public` group as universally inclusive: every read rule that targets `public` is also evaluated for authenticated users. This is the explicit "logged-in users should also have at least the same rights as 'public' users" semantics in `PermissionHandler::hasPermission` (line 229-241) and the matching qualification logic in `MagicRbacHandler::processConditionalRule` (`if ($group === 'public') $userQualifies = true`). It models authentication as a strict superset of anonymous access. + +That superset is the right default for most schemas — if a document is publicly visible, a logged-in colleague should also see it. But there are real cases where it's wrong: + +- **Tiered visibility flows.** A tenant wants public users to see a public catalogue (with date-windowed visibility, redactions applied), but logged-in users to access a different curated view that does NOT include the public catalogue's contents — e.g. a "draft / staging" register where logged-in users see drafts and the public sees nothing. With inheritance enabled, the public's empty result set is a non-issue, but with inheritance enabled and a public rule that grants visibility under conditions, logged-in users get those conditional grants too — even when the tenant explicitly wants them gated by their own group memberships. + +- **Privacy-strict schemas.** A schema where access is meant to be earned through explicit group membership only — anonymous via the public surface, authenticated only via their assigned groups. Inheritance dilutes that model: every public rule's grant cascades to authenticated users automatically. + +This change adds an opt-out: an `inheritFromPublic` boolean on the authorization block (schema-level, with register-level cascade and tenant-wide default). When `false`, authenticated users do NOT qualify for `public` rules — they must qualify via their own group memberships. The default stays `true` (current behaviour) so existing schemas are unaffected. + +## What Changes + +- **NEW:** `authorization.inheritFromPublic` (boolean, default `true`) field on schema and register authorization blocks. When `false`, the `public` group's rules apply ONLY to anonymous (unauthenticated) users; authenticated users must qualify through their own group memberships. +- **NEW:** Tenant-wide IAppConfig key `openregister.rbac.inherit_from_public_default` (boolean, default `true`). Used as the default when neither the schema's nor the register's authorization block specifies `inheritFromPublic` explicitly. Tenants can flip the global default to `false` for privacy-strict installs. +- **MODIFIED:** `PermissionHandler::hasPermission` — the inheritance fallback at line 229-241 (the "Logged-in users should also have at least the same rights as 'public' users" block) is wrapped in a check on the resolved `inheritFromPublic` flag. Skipped when `false`. +- **MODIFIED:** `MagicRbacHandler::processConditionalRule` and `processSimpleRule` — the `if ($group === 'public') $userQualifies = true` (and the simple-string `if ($rule === 'public') return true`) gain a guard: when `inheritFromPublic` is `false` AND the user is authenticated, the public rule does NOT qualify. Anonymous users see no behaviour change. +- **MODIFIED:** `MagicRbacHandler::processConditionalRuleSql` and the matching simple-rule path in the UNION code — same guard. +- **NEW:** `resolveInheritFromPublic(Schema $schema): bool` helper on `PermissionHandler` (or a similar central place) that resolves the effective flag using the cascade: schema → register → tenant default. +- **NO breaking change.** Default is `true` everywhere, mirroring today's behaviour. Schemas / registers / tenants that don't set the flag see no change. + +### Cascade + +Effective `inheritFromPublic` is resolved per request as: + +``` + schema.authorization.inheritFromPublic (if set) + ↓ else + register.authorization.inheritFromPublic (if set) + ↓ else + IAppConfig['openregister.rbac.inherit_from_public_default'] (default true) +``` + +The cascade matches how the rest of the authorization config cascades from schema → register today (per `PermissionHandler::resolveAuthorization`). + +### Out of scope + +- **Per-rule audience flags** (e.g. `audience: "anonymous"` on individual rules). Considered as Option β during exploration; rejected in favour of the simpler schema-level flag (Option α). A per-rule flavor can be added as a follow-up if a real use case appears. +- **A new "authenticated" group concept**. The change DOES NOT introduce an `authenticated` group as a first-class authorization target. (`MagicRbacHandler::processSimpleRule` already recognises the literal string `'authenticated'`, but extending that — e.g. with conditional `'authenticated'` rules with `match` blocks — is out of scope.) +- **Retroactive permission audits**. Existing access decisions on stored objects are not re-evaluated; the flag only affects future RBAC checks. +- **Front-end / admin UI for setting the flag**. v1 surfaces it as a JSON field on the schema / register authorization block, settable via the existing schema editor JSON view. A dedicated UI toggle is a follow-up. +- **Per-action override** (e.g. `inheritFromPublic` differing for read vs write). One flag covers all actions. + +## Capabilities + +### New Capabilities + +(none — this change extends an existing capability rather than introducing a new one.) + +### Modified Capabilities + +- `rbac-scopes`: the schema/register authorization block gains an optional `inheritFromPublic` boolean. The PHP-side and SQL-side public-group qualification logic honours the flag — when `false`, authenticated users do NOT qualify for `public` rules. Default `true` (unchanged behaviour). + +## Impact + +- **Code (openregister):** + - `lib/Db/Schema.php` (or wherever schema authorization is parsed) — accept the new `inheritFromPublic` field; preserve through serialisation. + - `lib/Service/Object/PermissionHandler.php` — add `resolveInheritFromPublic(Schema $schema): bool` helper using the cascade. Wrap the line 229-241 inheritance fallback in a check on this flag. + - `lib/Db/MagicMapper/MagicRbacHandler.php` — change `processConditionalRule`, `processConditionalRuleSql`, `processSimpleRule` (and any sibling methods) to accept and respect the flag. Plumb it through from `applyRbacFilters` and `buildRbacConditionsSql`, which resolve it once at the top of the method. + - Admin settings — surface the tenant default in the existing OR settings UI (where `IAppConfig` keys are exposed). v1 may ship as IAppConfig-only with admin UI follow-up. +- **API contract:** Schema authorization JSON gains an optional `inheritFromPublic` field. Additive, non-breaking. Existing schemas that don't set it retain today's inheriting behaviour. Authorization JSON serialisation includes the field when present. +- **Cross-app:** + - **DocuDesk**, **OpenCatalogi**, **Softwarecatalog**, **Procest**, **Pipelinq**, **ZaakAfhandelApp** — all consumers of OR's RBAC see no change for schemas that don't set `inheritFromPublic`. Tenants that flip the global default WILL see a behaviour change for any schema with public rules — this is documented in the CHANGELOG as a deliberate opt-in. + - The OpenCatalogi PublicationsController (the path that surfaced the original use case) automatically benefits — once a publication schema sets `inheritFromPublic: false`, authenticated users seeing publicly-time-windowed objects will be filtered the same way anonymous users are. +- **Privacy / compliance:** Strengthens the privacy-by-design knob set. Tenants with strict authorisation requirements (Wob/Woo with separate authenticated workflows, employee-only registers with public summaries) gain a clean way to gate authenticated access without restructuring their authorization rules. +- **Performance:** Per-RBAC-check overhead is one additional flag lookup (cached per-request). Negligible. +- **Tests:** Unit tests for `resolveInheritFromPublic` cascade behaviour. Unit tests for `hasPermission` covering the four states (anon × inherit-on/off, authenticated × inherit-on/off). SQL-side tests for `applyRbacFilters` with both flag values. Integration test against a schema with `inheritFromPublic: false` to confirm authenticated users don't see public-conditional rows. +- **Migration:** None. Field is additive with a backwards-compatible default. Existing serialised authorization blocks deserialise without modification. diff --git a/openspec/changes/rbac-disable-public-inheritance/specs/rbac-scopes/spec.md b/openspec/changes/rbac-disable-public-inheritance/specs/rbac-scopes/spec.md new file mode 100644 index 0000000000..ffc2a93b16 --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/specs/rbac-scopes/spec.md @@ -0,0 +1,155 @@ +--- +status: draft +--- + +# RBAC Scopes — Delta for Disable-Public-Inheritance Flag + +This delta extends the existing `rbac-scopes` capability with an opt-out flag for the "logged-in users inherit `public` group rights" semantics. Adds an `inheritFromPublic` boolean (default `true`) at the authorization-block level of schemas and registers, plus a tenant-wide IAppConfig default. When `false`, authenticated users do NOT qualify for `public` rules; they qualify only via their own group memberships. + +## ADDED Requirements + +### Requirement: Schema and register authorization MUST accept an optional `inheritFromPublic` boolean + +Schema and register authorization blocks MUST accept an optional `inheritFromPublic` field, boolean. The default value (when the field is absent or `null`) MUST be resolved via the cascade documented below. Existing schemas and registers that do not set the field MUST behave identically to before this change (default `true`). + +#### Scenario: Schema authorization without inheritFromPublic preserves pre-change behaviour + +- **GIVEN** a schema whose authorization block has no `inheritFromPublic` field +- **AND** the register's authorization block also has no `inheritFromPublic` field +- **AND** the tenant default IAppConfig key is unset +- **WHEN** RBAC checks run +- **THEN** the effective `inheritFromPublic` value is `true` (the pre-change behaviour) +- **AND** authenticated users qualify for `public` rules as they did before + +#### Scenario: Schema sets inheritFromPublic explicitly + +- **GIVEN** a schema whose authorization block contains `"inheritFromPublic": false` +- **WHEN** RBAC checks run +- **THEN** the effective value is `false` for that schema +- **AND** authenticated users do NOT qualify for `public` rules on that schema + +#### Scenario: Schema authorization round-trip preserves the field + +- **GIVEN** a schema saved with `"inheritFromPublic": false` in its authorization block +- **WHEN** the schema is fetched and re-serialised via `Schema::getAuthorization()` +- **THEN** the returned array contains `inheritFromPublic` with value `false` +- **AND** the JSON serialisation includes the field + +### Requirement: The effective value of `inheritFromPublic` MUST be resolved via cascade + +The cascade order MUST be: schema's authorization → register's authorization → tenant-wide `IAppConfig` key `openregister.rbac.inherit_from_public_default` → hard-coded `true`. The first explicitly-set value wins. `null` MUST be treated as "unset" (cascade falls through to the next level). + +#### Scenario: Cascade falls back to register when schema has no value + +- **GIVEN** a schema whose authorization has NO `inheritFromPublic` field +- **AND** the parent register's authorization has `"inheritFromPublic": false` +- **WHEN** the resolver is called for that schema +- **THEN** the resolved value is `false` + +#### Scenario: Cascade falls back to tenant default when neither schema nor register sets it + +- **GIVEN** schema and register without the field +- **AND** IAppConfig `openregister.rbac.inherit_from_public_default` is set to `false` +- **WHEN** the resolver is called +- **THEN** the resolved value is `false` + +#### Scenario: Cascade falls back to hard-coded true when nothing is set + +- **GIVEN** no schema, register, or tenant default is set +- **WHEN** the resolver is called +- **THEN** the resolved value is `true` + +#### Scenario: Schema explicit value wins over register and tenant + +- **GIVEN** schema sets `"inheritFromPublic": true` +- **AND** register sets `"inheritFromPublic": false` +- **AND** tenant default is `false` +- **WHEN** the resolver is called +- **THEN** the resolved value is `true` (schema wins) + +#### Scenario: null is treated as "unset" + +- **GIVEN** schema authorization contains `"inheritFromPublic": null` +- **AND** register sets `"inheritFromPublic": false` +- **WHEN** the resolver is called +- **THEN** the cascade continues past the schema; the resolved value is `false` (from register) + +### Requirement: When `inheritFromPublic` is `false`, authenticated users MUST NOT qualify for `public` rules + +When the resolved `inheritFromPublic` is `false`, the PHP-side `PermissionHandler::hasPermission` MUST NOT fall back to `hasGroupPermission(public, ...)` for authenticated users; it MUST return only the result of evaluating the user's own group memberships (plus owner / admin checks). Anonymous users see no behaviour change — the public-fallback path was never used for them in the first place. + +The SQL-side filter (`MagicRbacHandler::applyRbacFilters` and `buildRbacConditionsSql`) MUST equivalently exclude `public`-grouped rules from contributing conditions for authenticated users. The simple-string `'public'` rule MUST NOT grant unconditional access to authenticated users when the flag is `false`. Conditional `{group: "public", match: ...}` rules MUST NOT add their match conditions to the WHERE clause for authenticated users when the flag is `false`. + +#### Scenario: Authenticated user is denied when inheritance is off and only public has access + +- **GIVEN** a schema with `inheritFromPublic: false` and authorization `read: [{group: "public", match: }]` +- **AND** an authenticated user `alice` not a member of any group named in any rule +- **AND** an object that satisfies the public match +- **WHEN** alice attempts to read the object +- **THEN** access is denied +- **AND** the SQL filter excludes the object from listings for alice + +#### Scenario: Anonymous user is granted when public match passes (regardless of flag) + +- **GIVEN** the same schema as above +- **WHEN** an anonymous (unauthenticated) request reads the object +- **THEN** access is granted (the public match is satisfied) +- **AND** the SQL filter includes the object + +#### Scenario: Authenticated user with explicit group membership is still granted + +- **GIVEN** a schema with `inheritFromPublic: false` and authorization `read: [{group: "public", match: ...}, "editors"]` +- **AND** an authenticated user `bob` in the `editors` group +- **WHEN** bob attempts to read the object +- **THEN** access is granted (via the explicit `editors` rule) +- **AND** the SQL filter includes the object for bob + +#### Scenario: Owner check is unaffected by the flag + +- **GIVEN** a schema with `inheritFromPublic: false` and `read: [{group: "public", match: ...}]` +- **AND** an authenticated user `carol` who is the owner of an object +- **WHEN** carol attempts to read the object +- **THEN** access is granted via the owner shortcut, regardless of the flag + +#### Scenario: Admin user is unaffected by the flag + +- **GIVEN** a schema with `inheritFromPublic: false` +- **AND** an authenticated user in the `admin` group +- **WHEN** the admin reads any object on this schema +- **THEN** access is granted via the admin bypass, regardless of the flag + +### Requirement: When `inheritFromPublic` is `true` (or unset), behaviour MUST be identical to before this change + +For any schema, register, or tenant where the resolved `inheritFromPublic` is `true` (the default), authenticated users MUST continue to qualify for `public` rules exactly as they did before this change. The new code paths MUST NOT introduce any behavioural drift for schemas that don't opt out. + +#### Scenario: Pre-change schema is unaffected + +- **GIVEN** a schema with no `inheritFromPublic` field anywhere in its cascade +- **AND** an authenticated user +- **AND** an object satisfying a public match rule +- **WHEN** the user attempts to read the object +- **THEN** access is granted +- **AND** the result is identical to pre-change behaviour + +### Requirement: The simple-string `'authenticated'` rule MUST be unaffected by the flag + +The existing `'authenticated'` simple-rule string (recognised by `MagicRbacHandler::processSimpleRule` line 274-276) grants unconditional access to any logged-in user. This behaviour MUST be unchanged by `inheritFromPublic`. The flag concerns the `public` group only. + +#### Scenario: 'authenticated' rule still grants access when public inheritance is disabled + +- **GIVEN** a schema with `inheritFromPublic: false` and `read: ["authenticated"]` +- **AND** an authenticated user +- **WHEN** the user attempts to read +- **THEN** access is granted (via the `authenticated` rule, independent of the flag) + +### Requirement: PHP-side and SQL-side enforcement MUST be identical + +For any combination of (user state, flag value, authorization rules, object data), the result of `PermissionHandler::hasPermission` (per-object check) MUST agree with whether the SQL filter (`MagicRbacHandler::applyRbacFilters`) would include the object in a listing. The two layers MUST NOT diverge. + +#### Scenario: Per-object check and listing filter agree across the four-state matrix + +- **GIVEN** a schema with `read: [{group: "public", match: }]` +- **AND** the four states: (anon, authenticated) × (inheritFromPublic true, false) +- **AND** an object satisfying the public match +- **WHEN** the per-object `hasPermission` check runs AND the listing endpoint runs +- **THEN** for each of the four states, the per-object check's boolean result matches the listing's include/exclude decision for that object diff --git a/openspec/changes/rbac-disable-public-inheritance/tasks.md b/openspec/changes/rbac-disable-public-inheritance/tasks.md new file mode 100644 index 0000000000..9b2ca84af9 --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/tasks.md @@ -0,0 +1,73 @@ +## 1. resolveInheritFromPublic helper + +- [ ] 1.1 Add `private array $cachedInheritFromPublic = [];` field on `lib/Service/Object/PermissionHandler.php` for per-request caching keyed by schema ID. +- [ ] 1.2 Add public method `resolveInheritFromPublic(Schema $schema): bool` implementing the cascade: schema authorization → register authorization → IAppConfig `openregister.rbac.inherit_from_public_default` → hard-coded `true`. Treat `null` as "unset" — cascade falls through. +- [ ] 1.3 Wire the IAppConfig dependency. `PermissionHandler` already injects `$config` (or equivalent); reuse if so, else add to constructor. +- [ ] 1.4 Cache the resolved value per request keyed by schema ID. Reset implicitly per request (PHP process boundary). +- [ ] 1.5 Unit-test the cascade across the four levels (schema set, register set, tenant set, all unset → true). Plus the `null = unset` semantics. + +## 2. PHP-side enforcement (PermissionHandler::hasPermission) + +- [ ] 2.1 In `lib/Service/Object/PermissionHandler.php` lines 229-241, wrap the inheritance fallback (`hasGroupPermission(public, ...)` after the user-group foreach) in a check on `resolveInheritFromPublic($schema)`. When `false`, skip the fallback entirely. +- [ ] 2.2 Confirm anonymous-user behaviour at lines 174-184 is unchanged (the `if ($user === null)` branch already only checks public; this isn't the inheritance fallback we're guarding). +- [ ] 2.3 Confirm owner / admin shortcuts (lines 209, 543) are unaffected by the flag. +- [ ] 2.4 Unit-test `hasPermission` for the four-state matrix on this layer: + - (anon, true) → public match passes → grant + - (anon, false) → public match passes → grant (anon unaffected by flag) + - (auth, true) → public match passes (no other group) → grant + - (auth, false) → public match passes (no other group) → DENY +- [ ] 2.5 Verify owner / admin grants still work regardless of the flag. + +## 3. SQL-side enforcement (MagicRbacHandler) + +- [ ] 3.1 In `lib/Db/MagicMapper/MagicRbacHandler.php::applyRbacFilters` (line 132), resolve `inheritFromPublic` once at the top via `PermissionHandler::resolveInheritFromPublic($schema)` (already injected via DI per line 1320). +- [ ] 3.2 Pass the resolved flag into `processAuthorizationRule` → `processConditionalRule` and `processSimpleRule` as a new parameter. +- [ ] 3.3 Update `processConditionalRule` (lines 296-328): when `$group === 'public'` AND `inheritFromPublic === false` AND `$userId !== null`, set `$userQualifies = false` (skip the rule for authenticated users). +- [ ] 3.4 Update `processSimpleRule` (lines 266-284): when `$rule === 'public'` AND `inheritFromPublic === false` AND `$userId !== null`, return `false` (no unconditional access for authenticated users). +- [ ] 3.5 Same updates in the UNION-based path: `buildRbacConditionsSql` (line 758), `processConditionalRuleSql` (line 857), and the simple-rule path used by it. +- [ ] 3.6 Unit-test `applyRbacFilters` for the four-state matrix on this layer (build a query, inspect generated SQL or run against a fixture DB). +- [ ] 3.7 Unit-test `buildRbacConditionsSql` similarly (UNION path). + +## 4. Schema entity / serialisation + +- [ ] 4.1 Confirm `Schema::getAuthorization()` and `Schema::setAuthorization()` preserve the `inheritFromPublic` field through round-trips (the authorization is stored as JSON; the field is preserved automatically). Add a regression test if not already covered. +- [ ] 4.2 Confirm `Register::getAuthorization()` similarly preserves the field at the register level. +- [ ] 4.3 No schema migration needed — the field is a JSON-level addition with default `true`. + +## 5. Tenant default IAppConfig + +- [ ] 5.1 The IAppConfig key `openregister.rbac.inherit_from_public_default` is read by `resolveInheritFromPublic` (task 1.2). No registration step needed (IAppConfig keys are implicit). +- [ ] 5.2 Document the key in `docs/` (extend existing RBAC documentation). +- [ ] 5.3 Validate that boolean parsing accepts `true`, `false`, `"true"`, `"false"`, `"1"`, `"0"`, `1`, `0` (use `getValueBool` or equivalent helper). + +## 6. Cross-app integration check + +- [ ] 6.1 Smoke-test against DocuDesk's existing RBAC-using flows (consent records, etc.). Confirm no behavioural change for schemas that don't set `inheritFromPublic`. +- [ ] 6.2 Smoke-test against OpenCatalogi's PublicationsController (the path that surfaced the original use case). Confirm: with `inheritFromPublic: true` (default), authenticated users still see public-conditional rows; with `inheritFromPublic: false`, they don't. +- [ ] 6.3 Smoke-test against Softwarecatalog or any other consuming app. Default behaviour unchanged. + +## 7. Unit + integration tests + +- [ ] 7.1 `tests/unit/Service/Object/PermissionHandlerTest.php` — extend with the four-state matrix (anon × authenticated × flag-on/off) on `hasPermission`; cascade resolution tests for `resolveInheritFromPublic`. +- [ ] 7.2 `tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php` — extend with the four-state matrix on `applyRbacFilters` and on `buildRbacConditionsSql`. +- [ ] 7.3 Integration test (functional or Newman): a schema with `inheritFromPublic: false` and a public-conditional read rule; verify that: + - Anonymous request lists the object (public match passes). + - Authenticated request without explicit group membership does NOT list the object. + - Authenticated request with explicit group membership in another rule DOES list the object. +- [ ] 7.4 Integration test for cascade: schema unset, register `inheritFromPublic: false`, verify schema-level reads honour register's value. +- [ ] 7.5 Integration test for tenant default: IAppConfig set to `false`, verify schema reads honour the tenant default. + +## 8. Documentation + +- [ ] 8.1 Extend the canonical `rbac-scopes` documentation (in `docs/` or wherever the RBAC docs live) with the new `inheritFromPublic` field — its purpose, the cascade, the four-state matrix, the `authenticated` rule alternative for "all logged-in users". +- [ ] 8.2 Add a worked example: a publication-style schema with public-time-window read AND `inheritFromPublic: false`, demonstrating that authenticated users without explicit group access don't see the time-windowed content. +- [ ] 8.3 CHANGELOG entry under "Added": new `inheritFromPublic` boolean on schema/register authorization; tenant default IAppConfig key. +- [ ] 8.4 CHANGELOG entry under "Behavior changes" — note that flipping the tenant default OR setting `inheritFromPublic: false` per-schema is a deliberate opt-in; existing schemas that don't set it are unaffected. + +## 9. Quality and verification + +- [ ] 9.1 Run the full unit test suite — clean. +- [ ] 9.2 Run static analysis (Psalm / PHPStan at project strictness) — clean. +- [ ] 9.3 Run code style (PHPCS at project config) — clean. +- [ ] 9.4 Manual smoke against a live stack: configure a schema with `inheritFromPublic: false` and a public-conditional read rule; verify the four-state matrix manually via API requests as anonymous vs authenticated users. +- [ ] 9.5 Run `openspec validate rbac-disable-public-inheritance` — clean. From 3c05b62f480bf15c11acc907b5322d49113703d9 Mon Sep 17 00:00:00 2001 From: Robert Zondervan Date: Thu, 7 May 2026 13:32:11 +0200 Subject: [PATCH 02/13] feat(rbac): implement inheritFromPublic flag (#1439) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an opt-out for the implicit "logged-in users inherit public group rights" semantics. Schemas (and registers, via cascade) gain an optional inheritFromPublic boolean, default true (preserves pre-change behaviour). Cascade: schema.authorization.inheritFromPublic → register.authorization.inheritFromPublic → IAppConfig openregister.rbac.inherit_from_public_default → hard-coded true null is treated as "unset" — cascade falls through. PermissionHandler: - new constructor dep IAppConfig - new public resolveInheritFromPublic(Schema): bool with per-request cache - hasPermission line 229-241 inheritance fallback now gated on the flag MagicRbacHandler: - resolveInheritFromPublic(Schema) helper delegating to PermissionHandler via existing container DI - applyRbacFilters resolves the flag once at the top, plumbs through processAuthorizationRule → processConditionalRule + processSimpleRule - same plumbing in the UNION-mode path: buildRbacConditionsSql → processAuthorizationRuleSql → processConditionalRuleSql, and the shared processSimpleRule - the per-object hasPermission method (separate from PermissionHandler's) also gated identically Behaviour: - inheritFromPublic = true (default): unchanged from pre-change. - inheritFromPublic = false + anonymous user: still qualifies for public. - inheritFromPublic = false + authenticated user: does NOT qualify for public rules; only own-group / owner / admin grants apply. Tests: - new PermissionHandlerInheritFromPublicTest covers cascade resolution (4 levels + null=unset semantics) and the four-state matrix on hasPermission, plus owner/admin shortcut invariance. - existing PermissionHandlerRbacTest updated for new constructor sig. Quality: - PHPCS clean on touched files (auto-fix + manual passes). - PHPStan clean. - Psalm clean. - openspec validate clean. Deferred (tracked in tasks.md as not-yet-checked): - 3.6, 3.7: SQL-side unit tests (need fixture DB). - 6.x: cross-app smoke tests against running stacks. - 7.3-7.5: integration tests against running services. - 8.1, 8.2: docs extension + worked example. - 9.1, 9.4: full unit suite (PHPUnit needs the NC docker bootstrap) + manual live-stack smoke. Closes (partially) #1439. --- CHANGELOG.md | 6 + lib/Db/MagicMapper/MagicRbacHandler.php | 188 ++++- lib/Service/Object/PermissionHandler.php | 89 ++- .../rbac-disable-public-inheritance/plan.json | 458 ++++++++++- .../rbac-disable-public-inheritance/tasks.md | 50 +- ...PermissionHandlerInheritFromPublicTest.php | 551 ++++++++++++++ .../Object/PermissionHandlerRbacTest.php | 719 +++++++++++------- 7 files changed, 1675 insertions(+), 386 deletions(-) create mode 100644 tests/Unit/Service/Object/PermissionHandlerInheritFromPublicTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 12a8f72eb0..cecfd4b329 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ ## Unreleased +### Added +- **`inheritFromPublic` flag on schema and register authorization.** Schemas (and registers, via cascade) can now opt out of the implicit "logged-in users inherit `public` group rights" behaviour. When `inheritFromPublic: false` is set on the authorization block, authenticated users no longer qualify for `public` rules — they qualify only via their own group memberships. Anonymous users are unaffected. The cascade is `schema → register → IAppConfig openregister.rbac.inherit_from_public_default → hard-coded true`; `null` is treated as "unset". Default stays `true`, so existing schemas behave identically to before. Both the PHP-side check (`PermissionHandler::hasPermission`) and the SQL-side filter (`MagicRbacHandler::applyRbacFilters` / `buildRbacConditionsSql`) honour the flag identically. Useful for tiered visibility flows (public catalogue with date-windowed visibility plus a separate authenticated curated view) and privacy-strict schemas where authentication is meant to be a strict gate, not a superset of public access. ([#1439](https://github.com/ConductionNL/openregister/issues/1439)) + +### Behavior changes +- **`inheritFromPublic` defaults to `true` everywhere.** Setting `inheritFromPublic: false` on a schema, on its parent register, or via the tenant-wide `IAppConfig` key `openregister.rbac.inherit_from_public_default` is a deliberate opt-in. Tenants that flip the global default MUST audit existing schemas with public-conditional rules to confirm authenticated users were not relying on those grants. ([#1439](https://github.com/ConductionNL/openregister/issues/1439)) + ### Breaking Changes - **`@self.files` on rendered objects is now opt-in for full file metadata.** By default, `@self.files` is a lightweight list of integer file IDs (`[123, 456, 789]`). Consumers that need full file metadata (`id`, `path`, `title`, `accessUrl`, `downloadUrl`, `type`, `extension`, `size`, `hash`, `published`, `modified`, `labels`) MUST add `_extend[]=@self.files` (or the equivalent shorthand `_extend[]=_files`) to their request. The change applies to **every** consumer of OpenRegister's render output, including `show` endpoints in dependent apps (e.g. opencatalogi `/publications/{catalogSlug}/{id}`). Migration is a one-line query parameter addition. The previous behavior — full metadata always served on show, no metadata on list — caused asymmetric responses across endpoints and paid the file-lookup cost on every show response regardless of need. The new contract is symmetric across show and list endpoints (both emit `@self.files` as IDs by default; both accept `_extend[]=@self.files` for full metadata) and is documented under the `files-render-extension` capability. **Note:** Using `_extend[]=@self.files` (or `_files`) on **list** endpoints is heavily discouraged because it triggers per-row file/tag lookups (N+1 queries scaling with page size) and will result in degraded performance. Use it only when full file metadata is genuinely required for every row. **SOLR limitation:** on SOLR/index-backed list endpoints, `_extend[]=@self.files` is not yet supported; the lightweight ID list is always returned and the response carries `@self.extend_unsupported: ["@self.files"]` so consumers can detect the mismatch programmatically. Use the database-backed path when full file metadata is required on lists. diff --git a/lib/Db/MagicMapper/MagicRbacHandler.php b/lib/Db/MagicMapper/MagicRbacHandler.php index 07e1638249..cb3dee72d9 100644 --- a/lib/Db/MagicMapper/MagicRbacHandler.php +++ b/lib/Db/MagicMapper/MagicRbacHandler.php @@ -172,6 +172,9 @@ public function applyRbacFilters( return; } + // Resolve the inheritFromPublic flag once (schema → register → IAppConfig → true). + $inheritFromPublic = $this->resolveInheritFromPublic(schema: $schema); + // Build the RBAC filter conditions. $conditions = []; @@ -186,7 +189,8 @@ public function applyRbacFilters( qb: $qb, rule: $rule, userGroups: $userGroups, - userId: $userId + userId: $userId, + inheritFromPublic: $inheritFromPublic ); if ($ruleCondition === true) { @@ -223,10 +227,11 @@ public function applyRbacFilters( /** * Process a single authorization rule * - * @param IQueryBuilder $qb Query builder - * @param mixed $rule Authorization rule (string or array) - * @param array $userGroups User's group IDs - * @param string|null $userId Current user ID + * @param IQueryBuilder $qb Query builder + * @param mixed $rule Authorization rule (string or array) + * @param array $userGroups User's group IDs + * @param string|null $userId Current user ID + * @param bool $inheritFromPublic Whether authenticated users qualify for `public` rules. * * @return mixed True if unconditional access, SQL expression for conditional, null/false if no access */ @@ -234,16 +239,28 @@ private function processAuthorizationRule( IQueryBuilder $qb, mixed $rule, array $userGroups, - ?string $userId + ?string $userId, + bool $inheritFromPublic=true ): mixed { // Simple rule: just a group name string. if (is_string($rule) === true) { - return $this->processSimpleRule(rule: $rule, userGroups: $userGroups, userId: $userId); + return $this->processSimpleRule( + rule: $rule, + userGroups: $userGroups, + userId: $userId, + inheritFromPublic: $inheritFromPublic + ); } // Conditional rule: object with 'group' and optional 'match'. if (is_array($rule) === true && isset($rule['group']) === true) { - return $this->processConditionalRule(qb: $qb, rule: $rule, userGroups: $userGroups, userId: $userId); + return $this->processConditionalRule( + qb: $qb, + rule: $rule, + userGroups: $userGroups, + userId: $userId, + inheritFromPublic: $inheritFromPublic + ); } // Invalid rule format. @@ -257,16 +274,26 @@ private function processAuthorizationRule( /** * Process a simple (unconditional) authorization rule * - * @param string $rule Group name - * @param array $userGroups User's group IDs - * @param string|null $userId Current user ID + * @param string $rule Group name + * @param array $userGroups User's group IDs + * @param string|null $userId Current user ID + * @param bool $inheritFromPublic Whether authenticated users qualify for the `public` rule. * * @return bool True if user has access, false otherwise */ - private function processSimpleRule(string $rule, array $userGroups, ?string $userId): bool - { - // 'public' grants access to anyone, including unauthenticated users. + private function processSimpleRule( + string $rule, + array $userGroups, + ?string $userId, + bool $inheritFromPublic=true + ): bool { + // 'public' grants access to anonymous users, and to authenticated users + // when inheritFromPublic is true (the default — preserves pre-change semantics). if ($rule === 'public') { + if ($inheritFromPublic === false && $userId !== null) { + return false; + } + return true; } @@ -286,10 +313,11 @@ private function processSimpleRule(string $rule, array $userGroups, ?string $use /** * Process a conditional authorization rule * - * @param IQueryBuilder $qb Query builder - * @param array $rule Rule with 'group' and optional 'match' - * @param array $userGroups User's group IDs - * @param string|null $userId Current user ID + * @param IQueryBuilder $qb Query builder + * @param array $rule Rule with 'group' and optional 'match' + * @param array $userGroups User's group IDs + * @param string|null $userId Current user ID + * @param bool $inheritFromPublic Whether authenticated users qualify for `public` rules. * * @return mixed True if unconditional access, SQL expression for conditional, false if no access */ @@ -297,7 +325,8 @@ private function processConditionalRule( IQueryBuilder $qb, array $rule, array $userGroups, - ?string $userId + ?string $userId, + bool $inheritFromPublic=true ): mixed { $group = $rule['group']; $match = $rule['match'] ?? null; @@ -305,8 +334,13 @@ private function processConditionalRule( // Check if user qualifies for this group. $userQualifies = false; if ($group === 'public') { - // Public group means anyone can access, including unauthenticated users. - $userQualifies = true; + // Public group qualifies anonymous users, and authenticated users only + // when inheritFromPublic is true (default — preserves pre-change semantics). + if ($inheritFromPublic === false && $userId !== null) { + $userQualifies = false; + } else { + $userQualifies = true; + } } else if ($group === 'authenticated' && $userId !== null) { $userQualifies = true; } else if (in_array($group, $userGroups, true) === true) { @@ -696,6 +730,11 @@ public function hasPermission( return true; } + // Resolve the inheritFromPublic flag once (schema → register → IAppConfig → true). + // When false, authenticated users do NOT qualify for `public` rules. + $inheritFromPublic = $this->resolveInheritFromPublic(schema: $schema); + $publicQualifies = ($inheritFromPublic === true || $userId === null); + // Process each rule. // // Deduplication note (ADR-011): @@ -707,7 +746,15 @@ public function hasPermission( foreach ($rules as $rule) { // Simple string rule: direct group match. if (is_string($rule) === true) { - if ($rule === 'public' || in_array($rule, $userGroups, true) === true) { + if ($rule === 'public') { + if ($publicQualifies === true) { + return true; + } + + continue; + } + + if (in_array($rule, $userGroups, true) === true) { return true; } @@ -716,8 +763,14 @@ public function hasPermission( // Conditional rule: array with 'group' and optional 'match'. if (is_array($rule) === true && isset($rule['group']) === true) { - $group = $rule['group']; - $userQualifies = ($group === 'public' || in_array($group, $userGroups, true) === true); + $group = $rule['group']; + + if ($group === 'public') { + $userQualifies = $publicQualifies; + } else { + $userQualifies = in_array($group, $userGroups, true); + } + if ($userQualifies === false) { continue; } @@ -787,6 +840,9 @@ public function buildRbacConditionsSql(Schema $schema, string $action='read'): a return ['bypass' => true, 'conditions' => []]; } + // Resolve the inheritFromPublic flag once (schema → register → IAppConfig → true). + $inheritFromPublic = $this->resolveInheritFromPublic(schema: $schema); + // Build the RBAC filter conditions. $conditions = []; @@ -801,7 +857,8 @@ public function buildRbacConditionsSql(Schema $schema, string $action='read'): a $ruleResult = $this->processAuthorizationRuleSql( rule: $rule, userGroups: $userGroups, - userId: $userId + userId: $userId, + inheritFromPublic: $inheritFromPublic ); if ($ruleResult === true) { @@ -822,22 +879,37 @@ public function buildRbacConditionsSql(Schema $schema, string $action='read'): a /** * Process a single authorization rule for raw SQL output. * - * @param mixed $rule Authorization rule (string or array). - * @param array $userGroups User's group IDs. - * @param string|null $userId Current user ID. + * @param mixed $rule Authorization rule (string or array). + * @param array $userGroups User's group IDs. + * @param string|null $userId Current user ID. + * @param bool $inheritFromPublic Whether authenticated users qualify for `public` rules. * * @return mixed True if unconditional access, SQL string for conditional, false if no access. */ - private function processAuthorizationRuleSql(mixed $rule, array $userGroups, ?string $userId): mixed - { + private function processAuthorizationRuleSql( + mixed $rule, + array $userGroups, + ?string $userId, + bool $inheritFromPublic=true + ): mixed { // Simple rule: just a group name string. if (is_string($rule) === true) { - return $this->processSimpleRule(rule: $rule, userGroups: $userGroups, userId: $userId); + return $this->processSimpleRule( + rule: $rule, + userGroups: $userGroups, + userId: $userId, + inheritFromPublic: $inheritFromPublic + ); } // Conditional rule: object with 'group' and optional 'match'. if (is_array($rule) === true && isset($rule['group']) === true) { - return $this->processConditionalRuleSql(rule: $rule, userGroups: $userGroups, userId: $userId); + return $this->processConditionalRuleSql( + rule: $rule, + userGroups: $userGroups, + userId: $userId, + inheritFromPublic: $inheritFromPublic + ); } return false; @@ -846,23 +918,32 @@ private function processAuthorizationRuleSql(mixed $rule, array $userGroups, ?st /** * Process a conditional authorization rule for raw SQL output. * - * @param array $rule Rule with 'group' and optional 'match'. - * @param array $userGroups User's group IDs. - * @param string|null $userId Current user ID. + * @param array $rule Rule with 'group' and optional 'match'. + * @param array $userGroups User's group IDs. + * @param string|null $userId Current user ID. + * @param bool $inheritFromPublic Whether authenticated users qualify for `public` rules. * * @return mixed True if unconditional access, SQL string for conditional, false if no access. - * - * @psalm-suppress UnusedParam $userId reserved for user-specific match conditions in future RBAC rules */ - private function processConditionalRuleSql(array $rule, array $userGroups, ?string $userId): mixed - { + private function processConditionalRuleSql( + array $rule, + array $userGroups, + ?string $userId, + bool $inheritFromPublic=true + ): mixed { $group = $rule['group']; $match = $rule['match'] ?? null; // Check if user qualifies for this group. $userQualifies = false; if ($group === 'public') { - $userQualifies = true; + // Public group qualifies anonymous users, and authenticated users only + // when inheritFromPublic is true (default — preserves pre-change semantics). + if ($inheritFromPublic === false && $userId !== null) { + $userQualifies = false; + } else { + $userQualifies = true; + } } else if (in_array($group, $userGroups, true) === true) { $userQualifies = true; } @@ -1328,4 +1409,31 @@ private function resolveSchemaAuthorization(Schema $schema): ?array return $schema->getAuthorization(); } }//end resolveSchemaAuthorization() + + /** + * Resolve the effective `inheritFromPublic` flag for a schema. + * + * Delegates to PermissionHandler::resolveInheritFromPublic() which walks the + * cascade (schema → register → IAppConfig → true). Falls back to true (the + * pre-change behaviour) if PermissionHandler is unavailable. + * + * @param Schema $schema The schema to resolve the flag for. + * + * @return bool The effective inheritFromPublic value. + * + * @spec openspec/changes/rbac-disable-public-inheritance/specs/rbac-scopes/spec.md#requirement-the-effective-value-of-inheritfrompublic-must-be-resolved-via-cascade + */ + private function resolveInheritFromPublic(Schema $schema): bool + { + try { + $permissionHandler = $this->container->get(PermissionHandler::class); + return $permissionHandler->resolveInheritFromPublic($schema); + } catch (\Throwable $e) { + $this->logger->debug( + message: '[MagicRbacHandler] PermissionHandler unavailable, defaulting inheritFromPublic to true', + context: ['file' => __FILE__, 'line' => __LINE__, 'error' => $e->getMessage()] + ); + return true; + } + }//end resolveInheritFromPublic() }//end class diff --git a/lib/Service/Object/PermissionHandler.php b/lib/Service/Object/PermissionHandler.php index 68c8a29d8e..247df9a440 100644 --- a/lib/Service/Object/PermissionHandler.php +++ b/lib/Service/Object/PermissionHandler.php @@ -35,6 +35,7 @@ use OCA\OpenRegister\Db\SchemaMapper; use OCA\OpenRegister\Db\MagicMapper; use OCA\OpenRegister\Service\ConditionMatcher; +use OCP\IAppConfig; use OCP\IUserSession; use OCP\IUserManager; use OCP\IGroupManager; @@ -87,6 +88,18 @@ class PermissionHandler */ private array $cachedRegisterConfig = []; + /** + * Per-request cache for resolved `inheritFromPublic` flag per schema. + * + * Maps schema ID to its resolved boolean value (cascade: schema → register + * → IAppConfig → hard-coded true). Avoids repeated cascade walks within a + * single request when the same schema is checked many times (e.g. listing + * filter + per-object follow-up). + * + * @var array + */ + private array $cachedInheritFromPublic = []; + /** * PermissionHandler constructor. * @@ -96,6 +109,7 @@ class PermissionHandler * @param SchemaMapper $schemaMapper Mapper for schema operations. * @param MagicMapper $objectEntityMapper Mapper for object entity operations. * @param ConditionMatcher $conditionMatcher Shared PHP-side match evaluator (ADR-011). + * @param IAppConfig $appConfig Tenant configuration for the inheritFromPublic default. * @param LoggerInterface $logger Logger for permission auditing. * @param ContainerInterface $container Container for lazy loading services. * @@ -108,6 +122,7 @@ public function __construct( private readonly SchemaMapper $schemaMapper, private readonly MagicMapper $objectEntityMapper, private readonly ConditionMatcher $conditionMatcher, + private readonly IAppConfig $appConfig, private readonly LoggerInterface $logger, private readonly ContainerInterface $container ) { @@ -226,8 +241,11 @@ public function hasPermission( } }//end foreach - // Logged-in users should also have at least the same rights as 'public' users. - if ($this->hasGroupPermission( + // Logged-in users should also have at least the same rights as 'public' users — + // unless inheritFromPublic is disabled for this schema (cascade: schema → register + // → IAppConfig openregister.rbac.inherit_from_public_default → true). + if ($this->resolveInheritFromPublic(schema: $schema) === true + && $this->hasGroupPermission( authorization: $authorization, groupId: 'public', action: $action, @@ -662,6 +680,73 @@ public function resolveAuthorization(Schema $schema): ?array return null; }//end resolveAuthorization() + /** + * Resolve the effective `inheritFromPublic` flag for a schema. + * + * Cascade (first explicitly-set value wins): + * 1. schema's authorization.inheritFromPublic + * 2. register's authorization.inheritFromPublic + * 3. IAppConfig key `openregister.rbac.inherit_from_public_default` + * 4. hard-coded `true` (preserves pre-change behaviour) + * + * `null` is treated as "unset" — the cascade falls through. + * + * Result is cached per request, keyed by schema ID, to avoid repeated + * cascade walks when the same schema is checked many times (listing + * filter + per-object follow-up). + * + * @param Schema $schema The schema to resolve the flag for. + * + * @return bool The effective inheritFromPublic value. + * + * @spec openspec/changes/rbac-disable-public-inheritance/specs/rbac-scopes/spec.md#requirement-the-effective-value-of-inheritfrompublic-must-be-resolved-via-cascade + */ + public function resolveInheritFromPublic(Schema $schema): bool + { + $schemaId = $schema->getId(); + if ($schemaId !== null && array_key_exists($schemaId, $this->cachedInheritFromPublic) === true) { + return $this->cachedInheritFromPublic[$schemaId]; + } + + $resolved = null; + + // Step 1: schema-level authorization. + $auth = $schema->getAuthorization(); + if (is_array($auth) === true && array_key_exists('inheritFromPublic', $auth) === true && $auth['inheritFromPublic'] !== null) { + $resolved = (bool) $auth['inheritFromPublic']; + } + + // Step 2: register-level authorization. + if ($resolved === null) { + $register = $this->getRegisterForSchema(schema: $schema); + if ($register !== null) { + $registerAuth = $this->getRegisterAuthorization(registerId: $register->getId()); + if (is_array($registerAuth) === true + && array_key_exists('inheritFromPublic', $registerAuth) === true + && $registerAuth['inheritFromPublic'] !== null + ) { + $resolved = (bool) $registerAuth['inheritFromPublic']; + } + } + } + + // Step 3: tenant-wide IAppConfig default. + if ($resolved === null) { + $resolved = $this->appConfig->getValueBool( + app: 'openregister', + key: 'rbac.inherit_from_public_default', + default: true + ); + } + + if ($schemaId !== null) { + $this->cachedInheritFromPublic[$schemaId] = $resolved; + } + + return $resolved; + + }//end resolveInheritFromPublic() + /** * Get the parent register for a schema. * diff --git a/openspec/changes/rbac-disable-public-inheritance/plan.json b/openspec/changes/rbac-disable-public-inheritance/plan.json index dc246b83c7..b1feb1ef2f 100644 --- a/openspec/changes/rbac-disable-public-inheritance/plan.json +++ b/openspec/changes/rbac-disable-public-inheritance/plan.json @@ -6,45 +6,423 @@ "tracking_issue": 1439, "tracking_issue_url": "https://github.com/ConductionNL/openregister/issues/1439", "tasks": [ - { "id": "1.1", "section": "resolveInheritFromPublic helper", "title": "Add per-request cache field", "description": "private array $cachedInheritFromPublic = []; on PermissionHandler.php, keyed by schema ID.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, - { "id": "1.2", "section": "resolveInheritFromPublic helper", "title": "Implement cascade resolver", "description": "Public method resolveInheritFromPublic(Schema): bool. Cascade: schema → register → IAppConfig openregister.rbac.inherit_from_public_default → true. null = unset.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-the-effective-value-of-inheritfrompublic-must-be-resolved-via-cascade", "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, - { "id": "1.3", "section": "resolveInheritFromPublic helper", "title": "Wire IAppConfig dependency", "description": "Reuse existing injection; add to constructor if not present.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, - { "id": "1.4", "section": "resolveInheritFromPublic helper", "title": "Cache resolved value per request", "description": "Implicit reset on PHP process boundary.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, - { "id": "1.5", "section": "resolveInheritFromPublic helper", "title": "Unit-test cascade resolution", "description": "Four levels: schema set / register set / tenant set / all unset → true. Plus null = unset semantics.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, - { "id": "2.1", "section": "PHP-side enforcement", "title": "Wrap inheritance fallback in flag check", "description": "PermissionHandler::hasPermission lines 229-241; gate hasGroupPermission(public,...) on resolveInheritFromPublic === true.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, - { "id": "2.2", "section": "PHP-side enforcement", "title": "Confirm anonymous-user behaviour unchanged", "description": "Lines 174-184 if ($user === null) branch checks public — that's the anonymous path, not the inheritance fallback we're guarding.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "2.3", "section": "PHP-side enforcement", "title": "Confirm owner/admin shortcuts unaffected", "description": "Lines 209, 543 — neither depends on the flag.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": [] }, - { "id": "2.4", "section": "PHP-side enforcement", "title": "Four-state matrix unit tests on hasPermission", "description": "(anon, true) grant; (anon, false) grant (anon unaffected); (auth, true) grant; (auth, false) deny.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-php-side-and-sql-side-enforcement-must-be-identical", "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, - { "id": "2.5", "section": "PHP-side enforcement", "title": "Verify owner/admin grants persist", "description": "Both work regardless of the flag.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, - { "id": "3.1", "section": "SQL-side enforcement", "title": "Resolve inheritFromPublic in applyRbacFilters", "description": "Once at the top of MagicRbacHandler::applyRbacFilters via PermissionHandler::resolveInheritFromPublic($schema).", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, - { "id": "3.2", "section": "SQL-side enforcement", "title": "Plumb flag through processAuthorizationRule", "description": "→ processConditionalRule, processSimpleRule. New parameter on each method.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, - { "id": "3.3", "section": "SQL-side enforcement", "title": "Guard processConditionalRule public branch", "description": "When $group === 'public' AND inheritFromPublic === false AND $userId !== null, set $userQualifies = false.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, - { "id": "3.4", "section": "SQL-side enforcement", "title": "Guard processSimpleRule public branch", "description": "When $rule === 'public' AND inheritFromPublic === false AND $userId !== null, return false.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, - { "id": "3.5", "section": "SQL-side enforcement", "title": "Same updates in UNION-based path", "description": "buildRbacConditionsSql, processConditionalRuleSql.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Db/MagicMapper/MagicRbacHandler.php"] }, - { "id": "3.6", "section": "SQL-side enforcement", "title": "Unit-test applyRbacFilters four-state matrix", "description": "Build query, inspect SQL or run against fixture DB.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php"] }, - { "id": "3.7", "section": "SQL-side enforcement", "title": "Unit-test buildRbacConditionsSql four-state matrix", "description": "UNION path equivalent of 3.6.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php"] }, - { "id": "4.1", "section": "Schema entity / serialisation", "title": "Confirm Schema authorization round-trips preserve field", "description": "getAuthorization/setAuthorization round-trip; add regression test if not covered.", "status": "pending", "spec_ref": "rbac-scopes/spec.md#requirement-schema-and-register-authorization-must-accept-an-optional-inheritfrompublic-boolean", "files_likely_affected": ["tests/unit/Db/SchemaTest.php"] }, - { "id": "4.2", "section": "Schema entity / serialisation", "title": "Confirm Register authorization preserves field", "description": "Same as 4.1 at register level.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/RegisterTest.php"] }, - { "id": "4.3", "section": "Schema entity / serialisation", "title": "No schema migration needed", "description": "Additive JSON-level field; existing serialisations unchanged.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "5.1", "section": "Tenant default IAppConfig", "title": "Read openregister.rbac.inherit_from_public_default", "description": "Implicit registration via IAppConfig pattern; read by resolveInheritFromPublic.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, - { "id": "5.2", "section": "Tenant default IAppConfig", "title": "Document the IAppConfig key", "description": "Extend RBAC documentation.", "status": "pending", "spec_ref": null, "files_likely_affected": ["docs/"] }, - { "id": "5.3", "section": "Tenant default IAppConfig", "title": "Validate boolean parsing", "description": "Accept true/false/'true'/'false'/'1'/'0'/1/0 via getValueBool or equivalent.", "status": "pending", "spec_ref": null, "files_likely_affected": ["lib/Service/Object/PermissionHandler.php"] }, - { "id": "6.1", "section": "Cross-app integration check", "title": "Smoke-test DocuDesk RBAC flows", "description": "Schemas without inheritFromPublic see no behaviour change.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "6.2", "section": "Cross-app integration check", "title": "Smoke-test OpenCatalogi PublicationsController", "description": "With inheritFromPublic:true (default) auth users still see public-conditional rows; with false they don't.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "6.3", "section": "Cross-app integration check", "title": "Smoke-test other consuming apps", "description": "Default behaviour unchanged.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "7.1", "section": "Unit + integration tests", "title": "PermissionHandlerTest extension", "description": "Four-state matrix on hasPermission; cascade resolution tests.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Service/Object/PermissionHandlerTest.php"] }, - { "id": "7.2", "section": "Unit + integration tests", "title": "MagicRbacHandlerTest extension", "description": "Four-state matrix on applyRbacFilters and buildRbacConditionsSql.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php"] }, - { "id": "7.3", "section": "Unit + integration tests", "title": "Integration test: inheritFromPublic:false schema", "description": "Public-conditional read; anon allowed; auth without explicit group denied; auth with explicit group allowed.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/integration/"] }, - { "id": "7.4", "section": "Unit + integration tests", "title": "Integration test: register-level cascade", "description": "Schema unset; register inheritFromPublic:false → schema honours register's value.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/integration/"] }, - { "id": "7.5", "section": "Unit + integration tests", "title": "Integration test: tenant default", "description": "IAppConfig set to false; schema reads honour tenant default.", "status": "pending", "spec_ref": null, "files_likely_affected": ["tests/integration/"] }, - { "id": "8.1", "section": "Documentation", "title": "Extend rbac-scopes RBAC docs", "description": "New field, cascade, four-state matrix, authenticated-rule alternative.", "status": "pending", "spec_ref": null, "files_likely_affected": ["docs/"] }, - { "id": "8.2", "section": "Documentation", "title": "Worked example for publication-style schema", "description": "Public-time-window read + inheritFromPublic:false demonstrating tiered visibility.", "status": "pending", "spec_ref": null, "files_likely_affected": ["docs/"] }, - { "id": "8.3", "section": "Documentation", "title": "CHANGELOG under Added", "description": "New inheritFromPublic boolean + tenant default IAppConfig key.", "status": "pending", "spec_ref": null, "files_likely_affected": ["CHANGELOG.md"] }, - { "id": "8.4", "section": "Documentation", "title": "CHANGELOG under Behavior changes", "description": "Flipping is deliberate opt-in; existing schemas unaffected.", "status": "pending", "spec_ref": null, "files_likely_affected": ["CHANGELOG.md"] }, - { "id": "9.1", "section": "Quality and verification", "title": "Full unit test suite clean", "description": "All tests pass.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "9.2", "section": "Quality and verification", "title": "Static analysis clean", "description": "Psalm / PHPStan at project strictness.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "9.3", "section": "Quality and verification", "title": "Code style clean", "description": "PHPCS at project config.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "9.4", "section": "Quality and verification", "title": "Manual smoke against live stack", "description": "Configure schema with inheritFromPublic:false; verify four-state matrix via API requests as anon vs authenticated users.", "status": "pending", "spec_ref": null, "files_likely_affected": [] }, - { "id": "9.5", "section": "Quality and verification", "title": "openspec validate clean", "description": "Run openspec validate rbac-disable-public-inheritance.", "status": "pending", "spec_ref": null, "files_likely_affected": [] } + { + "id": "1.1", + "section": "resolveInheritFromPublic helper", + "title": "Add per-request cache field", + "description": "private array $cachedInheritFromPublic = []; on PermissionHandler.php, keyed by schema ID.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Service/Object/PermissionHandler.php" + ] + }, + { + "id": "1.2", + "section": "resolveInheritFromPublic helper", + "title": "Implement cascade resolver", + "description": "Public method resolveInheritFromPublic(Schema): bool. Cascade: schema \u2192 register \u2192 IAppConfig openregister.rbac.inherit_from_public_default \u2192 true. null = unset.", + "status": "done", + "spec_ref": "rbac-scopes/spec.md#requirement-the-effective-value-of-inheritfrompublic-must-be-resolved-via-cascade", + "files_likely_affected": [ + "lib/Service/Object/PermissionHandler.php" + ] + }, + { + "id": "1.3", + "section": "resolveInheritFromPublic helper", + "title": "Wire IAppConfig dependency", + "description": "Reuse existing injection; add to constructor if not present.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Service/Object/PermissionHandler.php" + ] + }, + { + "id": "1.4", + "section": "resolveInheritFromPublic helper", + "title": "Cache resolved value per request", + "description": "Implicit reset on PHP process boundary.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Service/Object/PermissionHandler.php" + ] + }, + { + "id": "1.5", + "section": "resolveInheritFromPublic helper", + "title": "Unit-test cascade resolution", + "description": "Four levels: schema set / register set / tenant set / all unset \u2192 true. Plus null = unset semantics.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "tests/unit/Service/Object/PermissionHandlerTest.php" + ] + }, + { + "id": "2.1", + "section": "PHP-side enforcement", + "title": "Wrap inheritance fallback in flag check", + "description": "PermissionHandler::hasPermission lines 229-241; gate hasGroupPermission(public,...) on resolveInheritFromPublic === true.", + "status": "done", + "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", + "files_likely_affected": [ + "lib/Service/Object/PermissionHandler.php" + ] + }, + { + "id": "2.2", + "section": "PHP-side enforcement", + "title": "Confirm anonymous-user behaviour unchanged", + "description": "Lines 174-184 if ($user === null) branch checks public \u2014 that's the anonymous path, not the inheritance fallback we're guarding.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "2.3", + "section": "PHP-side enforcement", + "title": "Confirm owner/admin shortcuts unaffected", + "description": "Lines 209, 543 \u2014 neither depends on the flag.", + "status": "done", + "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", + "files_likely_affected": [] + }, + { + "id": "2.4", + "section": "PHP-side enforcement", + "title": "Four-state matrix unit tests on hasPermission", + "description": "(anon, true) grant; (anon, false) grant (anon unaffected); (auth, true) grant; (auth, false) deny.", + "status": "done", + "spec_ref": "rbac-scopes/spec.md#requirement-php-side-and-sql-side-enforcement-must-be-identical", + "files_likely_affected": [ + "tests/unit/Service/Object/PermissionHandlerTest.php" + ] + }, + { + "id": "2.5", + "section": "PHP-side enforcement", + "title": "Verify owner/admin grants persist", + "description": "Both work regardless of the flag.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "tests/unit/Service/Object/PermissionHandlerTest.php" + ] + }, + { + "id": "3.1", + "section": "SQL-side enforcement", + "title": "Resolve inheritFromPublic in applyRbacFilters", + "description": "Once at the top of MagicRbacHandler::applyRbacFilters via PermissionHandler::resolveInheritFromPublic($schema).", + "status": "done", + "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", + "files_likely_affected": [ + "lib/Db/MagicMapper/MagicRbacHandler.php" + ] + }, + { + "id": "3.2", + "section": "SQL-side enforcement", + "title": "Plumb flag through processAuthorizationRule", + "description": "\u2192 processConditionalRule, processSimpleRule. New parameter on each method.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Db/MagicMapper/MagicRbacHandler.php" + ] + }, + { + "id": "3.3", + "section": "SQL-side enforcement", + "title": "Guard processConditionalRule public branch", + "description": "When $group === 'public' AND inheritFromPublic === false AND $userId !== null, set $userQualifies = false.", + "status": "done", + "spec_ref": "rbac-scopes/spec.md#requirement-when-inheritfrompublic-is-false-authenticated-users-must-not-qualify-for-public-rules", + "files_likely_affected": [ + "lib/Db/MagicMapper/MagicRbacHandler.php" + ] + }, + { + "id": "3.4", + "section": "SQL-side enforcement", + "title": "Guard processSimpleRule public branch", + "description": "When $rule === 'public' AND inheritFromPublic === false AND $userId !== null, return false.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Db/MagicMapper/MagicRbacHandler.php" + ] + }, + { + "id": "3.5", + "section": "SQL-side enforcement", + "title": "Same updates in UNION-based path", + "description": "buildRbacConditionsSql, processConditionalRuleSql.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Db/MagicMapper/MagicRbacHandler.php" + ] + }, + { + "id": "3.6", + "section": "SQL-side enforcement", + "title": "Unit-test applyRbacFilters four-state matrix", + "description": "Build query, inspect SQL or run against fixture DB.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php" + ] + }, + { + "id": "3.7", + "section": "SQL-side enforcement", + "title": "Unit-test buildRbacConditionsSql four-state matrix", + "description": "UNION path equivalent of 3.6.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php" + ] + }, + { + "id": "4.1", + "section": "Schema entity / serialisation", + "title": "Confirm Schema authorization round-trips preserve field", + "description": "getAuthorization/setAuthorization round-trip; add regression test if not covered.", + "status": "done", + "spec_ref": "rbac-scopes/spec.md#requirement-schema-and-register-authorization-must-accept-an-optional-inheritfrompublic-boolean", + "files_likely_affected": [ + "tests/unit/Db/SchemaTest.php" + ] + }, + { + "id": "4.2", + "section": "Schema entity / serialisation", + "title": "Confirm Register authorization preserves field", + "description": "Same as 4.1 at register level.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "tests/unit/Db/RegisterTest.php" + ] + }, + { + "id": "4.3", + "section": "Schema entity / serialisation", + "title": "No schema migration needed", + "description": "Additive JSON-level field; existing serialisations unchanged.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "5.1", + "section": "Tenant default IAppConfig", + "title": "Read openregister.rbac.inherit_from_public_default", + "description": "Implicit registration via IAppConfig pattern; read by resolveInheritFromPublic.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Service/Object/PermissionHandler.php" + ] + }, + { + "id": "5.2", + "section": "Tenant default IAppConfig", + "title": "Document the IAppConfig key", + "description": "Extend RBAC documentation.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "docs/" + ] + }, + { + "id": "5.3", + "section": "Tenant default IAppConfig", + "title": "Validate boolean parsing", + "description": "Accept true/false/'true'/'false'/'1'/'0'/1/0 via getValueBool or equivalent.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "lib/Service/Object/PermissionHandler.php" + ] + }, + { + "id": "6.1", + "section": "Cross-app integration check", + "title": "Smoke-test DocuDesk RBAC flows", + "description": "Schemas without inheritFromPublic see no behaviour change.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "6.2", + "section": "Cross-app integration check", + "title": "Smoke-test OpenCatalogi PublicationsController", + "description": "With inheritFromPublic:true (default) auth users still see public-conditional rows; with false they don't.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "6.3", + "section": "Cross-app integration check", + "title": "Smoke-test other consuming apps", + "description": "Default behaviour unchanged.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "7.1", + "section": "Unit + integration tests", + "title": "PermissionHandlerTest extension", + "description": "Four-state matrix on hasPermission; cascade resolution tests.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "tests/unit/Service/Object/PermissionHandlerTest.php" + ] + }, + { + "id": "7.2", + "section": "Unit + integration tests", + "title": "MagicRbacHandlerTest extension", + "description": "Four-state matrix on applyRbacFilters and buildRbacConditionsSql.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php" + ] + }, + { + "id": "7.3", + "section": "Unit + integration tests", + "title": "Integration test: inheritFromPublic:false schema", + "description": "Public-conditional read; anon allowed; auth without explicit group denied; auth with explicit group allowed.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "tests/integration/" + ] + }, + { + "id": "7.4", + "section": "Unit + integration tests", + "title": "Integration test: register-level cascade", + "description": "Schema unset; register inheritFromPublic:false \u2192 schema honours register's value.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "tests/integration/" + ] + }, + { + "id": "7.5", + "section": "Unit + integration tests", + "title": "Integration test: tenant default", + "description": "IAppConfig set to false; schema reads honour tenant default.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "tests/integration/" + ] + }, + { + "id": "8.1", + "section": "Documentation", + "title": "Extend rbac-scopes RBAC docs", + "description": "New field, cascade, four-state matrix, authenticated-rule alternative.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "docs/" + ] + }, + { + "id": "8.2", + "section": "Documentation", + "title": "Worked example for publication-style schema", + "description": "Public-time-window read + inheritFromPublic:false demonstrating tiered visibility.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [ + "docs/" + ] + }, + { + "id": "8.3", + "section": "Documentation", + "title": "CHANGELOG under Added", + "description": "New inheritFromPublic boolean + tenant default IAppConfig key.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "CHANGELOG.md" + ] + }, + { + "id": "8.4", + "section": "Documentation", + "title": "CHANGELOG under Behavior changes", + "description": "Flipping is deliberate opt-in; existing schemas unaffected.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [ + "CHANGELOG.md" + ] + }, + { + "id": "9.1", + "section": "Quality and verification", + "title": "Full unit test suite clean", + "description": "All tests pass.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "9.2", + "section": "Quality and verification", + "title": "Static analysis clean", + "description": "Psalm / PHPStan at project strictness.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "9.3", + "section": "Quality and verification", + "title": "Code style clean", + "description": "PHPCS at project config.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "9.4", + "section": "Quality and verification", + "title": "Manual smoke against live stack", + "description": "Configure schema with inheritFromPublic:false; verify four-state matrix via API requests as anon vs authenticated users.", + "status": "pending", + "spec_ref": null, + "files_likely_affected": [] + }, + { + "id": "9.5", + "section": "Quality and verification", + "title": "openspec validate clean", + "description": "Run openspec validate rbac-disable-public-inheritance.", + "status": "done", + "spec_ref": null, + "files_likely_affected": [] + } ] } diff --git a/openspec/changes/rbac-disable-public-inheritance/tasks.md b/openspec/changes/rbac-disable-public-inheritance/tasks.md index 9b2ca84af9..79c3d9c035 100644 --- a/openspec/changes/rbac-disable-public-inheritance/tasks.md +++ b/openspec/changes/rbac-disable-public-inheritance/tasks.md @@ -1,44 +1,44 @@ ## 1. resolveInheritFromPublic helper -- [ ] 1.1 Add `private array $cachedInheritFromPublic = [];` field on `lib/Service/Object/PermissionHandler.php` for per-request caching keyed by schema ID. -- [ ] 1.2 Add public method `resolveInheritFromPublic(Schema $schema): bool` implementing the cascade: schema authorization → register authorization → IAppConfig `openregister.rbac.inherit_from_public_default` → hard-coded `true`. Treat `null` as "unset" — cascade falls through. -- [ ] 1.3 Wire the IAppConfig dependency. `PermissionHandler` already injects `$config` (or equivalent); reuse if so, else add to constructor. -- [ ] 1.4 Cache the resolved value per request keyed by schema ID. Reset implicitly per request (PHP process boundary). -- [ ] 1.5 Unit-test the cascade across the four levels (schema set, register set, tenant set, all unset → true). Plus the `null = unset` semantics. +- [x] 1.1 Add `private array $cachedInheritFromPublic = [];` field on `lib/Service/Object/PermissionHandler.php` for per-request caching keyed by schema ID. +- [x] 1.2 Add public method `resolveInheritFromPublic(Schema $schema): bool` implementing the cascade: schema authorization → register authorization → IAppConfig `openregister.rbac.inherit_from_public_default` → hard-coded `true`. Treat `null` as "unset" — cascade falls through. +- [x] 1.3 Wire the IAppConfig dependency. `PermissionHandler` already injects `$config` (or equivalent); reuse if so, else add to constructor. +- [x] 1.4 Cache the resolved value per request keyed by schema ID. Reset implicitly per request (PHP process boundary). +- [x] 1.5 Unit-test the cascade across the four levels (schema set, register set, tenant set, all unset → true). Plus the `null = unset` semantics. ## 2. PHP-side enforcement (PermissionHandler::hasPermission) -- [ ] 2.1 In `lib/Service/Object/PermissionHandler.php` lines 229-241, wrap the inheritance fallback (`hasGroupPermission(public, ...)` after the user-group foreach) in a check on `resolveInheritFromPublic($schema)`. When `false`, skip the fallback entirely. -- [ ] 2.2 Confirm anonymous-user behaviour at lines 174-184 is unchanged (the `if ($user === null)` branch already only checks public; this isn't the inheritance fallback we're guarding). -- [ ] 2.3 Confirm owner / admin shortcuts (lines 209, 543) are unaffected by the flag. -- [ ] 2.4 Unit-test `hasPermission` for the four-state matrix on this layer: +- [x] 2.1 In `lib/Service/Object/PermissionHandler.php` lines 229-241, wrap the inheritance fallback (`hasGroupPermission(public, ...)` after the user-group foreach) in a check on `resolveInheritFromPublic($schema)`. When `false`, skip the fallback entirely. +- [x] 2.2 Confirm anonymous-user behaviour at lines 174-184 is unchanged (the `if ($user === null)` branch already only checks public; this isn't the inheritance fallback we're guarding). +- [x] 2.3 Confirm owner / admin shortcuts (lines 209, 543) are unaffected by the flag. +- [x] 2.4 Unit-test `hasPermission` for the four-state matrix on this layer: - (anon, true) → public match passes → grant - (anon, false) → public match passes → grant (anon unaffected by flag) - (auth, true) → public match passes (no other group) → grant - (auth, false) → public match passes (no other group) → DENY -- [ ] 2.5 Verify owner / admin grants still work regardless of the flag. +- [x] 2.5 Verify owner / admin grants still work regardless of the flag. ## 3. SQL-side enforcement (MagicRbacHandler) -- [ ] 3.1 In `lib/Db/MagicMapper/MagicRbacHandler.php::applyRbacFilters` (line 132), resolve `inheritFromPublic` once at the top via `PermissionHandler::resolveInheritFromPublic($schema)` (already injected via DI per line 1320). -- [ ] 3.2 Pass the resolved flag into `processAuthorizationRule` → `processConditionalRule` and `processSimpleRule` as a new parameter. -- [ ] 3.3 Update `processConditionalRule` (lines 296-328): when `$group === 'public'` AND `inheritFromPublic === false` AND `$userId !== null`, set `$userQualifies = false` (skip the rule for authenticated users). -- [ ] 3.4 Update `processSimpleRule` (lines 266-284): when `$rule === 'public'` AND `inheritFromPublic === false` AND `$userId !== null`, return `false` (no unconditional access for authenticated users). -- [ ] 3.5 Same updates in the UNION-based path: `buildRbacConditionsSql` (line 758), `processConditionalRuleSql` (line 857), and the simple-rule path used by it. +- [x] 3.1 In `lib/Db/MagicMapper/MagicRbacHandler.php::applyRbacFilters` (line 132), resolve `inheritFromPublic` once at the top via `PermissionHandler::resolveInheritFromPublic($schema)` (already injected via DI per line 1320). +- [x] 3.2 Pass the resolved flag into `processAuthorizationRule` → `processConditionalRule` and `processSimpleRule` as a new parameter. +- [x] 3.3 Update `processConditionalRule` (lines 296-328): when `$group === 'public'` AND `inheritFromPublic === false` AND `$userId !== null`, set `$userQualifies = false` (skip the rule for authenticated users). +- [x] 3.4 Update `processSimpleRule` (lines 266-284): when `$rule === 'public'` AND `inheritFromPublic === false` AND `$userId !== null`, return `false` (no unconditional access for authenticated users). +- [x] 3.5 Same updates in the UNION-based path: `buildRbacConditionsSql` (line 758), `processConditionalRuleSql` (line 857), and the simple-rule path used by it. - [ ] 3.6 Unit-test `applyRbacFilters` for the four-state matrix on this layer (build a query, inspect generated SQL or run against a fixture DB). - [ ] 3.7 Unit-test `buildRbacConditionsSql` similarly (UNION path). ## 4. Schema entity / serialisation -- [ ] 4.1 Confirm `Schema::getAuthorization()` and `Schema::setAuthorization()` preserve the `inheritFromPublic` field through round-trips (the authorization is stored as JSON; the field is preserved automatically). Add a regression test if not already covered. -- [ ] 4.2 Confirm `Register::getAuthorization()` similarly preserves the field at the register level. -- [ ] 4.3 No schema migration needed — the field is a JSON-level addition with default `true`. +- [x] 4.1 Confirm `Schema::getAuthorization()` and `Schema::setAuthorization()` preserve the `inheritFromPublic` field through round-trips (the authorization is stored as JSON; the field is preserved automatically). Add a regression test if not already covered. +- [x] 4.2 Confirm `Register::getAuthorization()` similarly preserves the field at the register level. +- [x] 4.3 No schema migration needed — the field is a JSON-level addition with default `true`. ## 5. Tenant default IAppConfig -- [ ] 5.1 The IAppConfig key `openregister.rbac.inherit_from_public_default` is read by `resolveInheritFromPublic` (task 1.2). No registration step needed (IAppConfig keys are implicit). +- [x] 5.1 The IAppConfig key `openregister.rbac.inherit_from_public_default` is read by `resolveInheritFromPublic` (task 1.2). No registration step needed (IAppConfig keys are implicit). - [ ] 5.2 Document the key in `docs/` (extend existing RBAC documentation). -- [ ] 5.3 Validate that boolean parsing accepts `true`, `false`, `"true"`, `"false"`, `"1"`, `"0"`, `1`, `0` (use `getValueBool` or equivalent helper). +- [x] 5.3 Validate that boolean parsing accepts `true`, `false`, `"true"`, `"false"`, `"1"`, `"0"`, `1`, `0` (use `getValueBool` or equivalent helper). ## 6. Cross-app integration check @@ -61,13 +61,13 @@ - [ ] 8.1 Extend the canonical `rbac-scopes` documentation (in `docs/` or wherever the RBAC docs live) with the new `inheritFromPublic` field — its purpose, the cascade, the four-state matrix, the `authenticated` rule alternative for "all logged-in users". - [ ] 8.2 Add a worked example: a publication-style schema with public-time-window read AND `inheritFromPublic: false`, demonstrating that authenticated users without explicit group access don't see the time-windowed content. -- [ ] 8.3 CHANGELOG entry under "Added": new `inheritFromPublic` boolean on schema/register authorization; tenant default IAppConfig key. -- [ ] 8.4 CHANGELOG entry under "Behavior changes" — note that flipping the tenant default OR setting `inheritFromPublic: false` per-schema is a deliberate opt-in; existing schemas that don't set it are unaffected. +- [x] 8.3 CHANGELOG entry under "Added": new `inheritFromPublic` boolean on schema/register authorization; tenant default IAppConfig key. +- [x] 8.4 CHANGELOG entry under "Behavior changes" — note that flipping the tenant default OR setting `inheritFromPublic: false` per-schema is a deliberate opt-in; existing schemas that don't set it are unaffected. ## 9. Quality and verification - [ ] 9.1 Run the full unit test suite — clean. -- [ ] 9.2 Run static analysis (Psalm / PHPStan at project strictness) — clean. -- [ ] 9.3 Run code style (PHPCS at project config) — clean. +- [x] 9.2 Run static analysis (Psalm / PHPStan at project strictness) — clean. +- [x] 9.3 Run code style (PHPCS at project config) — clean. - [ ] 9.4 Manual smoke against a live stack: configure a schema with `inheritFromPublic: false` and a public-conditional read rule; verify the four-state matrix manually via API requests as anonymous vs authenticated users. -- [ ] 9.5 Run `openspec validate rbac-disable-public-inheritance` — clean. +- [x] 9.5 Run `openspec validate rbac-disable-public-inheritance` — clean. diff --git a/tests/Unit/Service/Object/PermissionHandlerInheritFromPublicTest.php b/tests/Unit/Service/Object/PermissionHandlerInheritFromPublicTest.php new file mode 100644 index 0000000000..bed9a4197f --- /dev/null +++ b/tests/Unit/Service/Object/PermissionHandlerInheritFromPublicTest.php @@ -0,0 +1,551 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @version GIT: + * + * @link https://www.OpenRegister.app + * + * @spec openspec/changes/rbac-disable-public-inheritance/tasks.md + */ + +declare(strict_types=1); + +namespace Unit\Service\Object; + +use OCA\OpenRegister\Db\Register; +use OCA\OpenRegister\Db\RegisterMapper; +use OCA\OpenRegister\Db\Schema; +use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Db\MagicMapper; +use OCA\OpenRegister\Service\ConditionMatcher; +use OCA\OpenRegister\Service\Object\PermissionHandler; +use OCP\IAppConfig; +use OCP\IGroupManager; +use OCP\IUser; +use OCP\IUserManager; +use OCP\IUserSession; +use PHPUnit\Framework\TestCase; +use PHPUnit\Framework\MockObject\MockObject; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; + +/** + * Tests for PermissionHandler::resolveInheritFromPublic and the inheritance gating + * inside hasPermission introduced by the rbac-disable-public-inheritance change. + * + * Covers: + * - The four-level cascade (schema → register → IAppConfig → hard-coded true) + * - `null = unset` semantics + * - The four-state matrix on hasPermission: (anon, auth) × (inheritFromPublic true, false) + * - Owner / admin shortcuts unaffected by the flag + */ +class PermissionHandlerInheritFromPublicTest extends TestCase +{ + + /** + * Subject under test. + * + * @var PermissionHandler + */ + private PermissionHandler $handler; + + /** + * Mock user session. + * + * @var IUserSession&MockObject + */ + private IUserSession&MockObject $userSession; + + /** + * Mock user manager. + * + * @var IUserManager&MockObject + */ + private IUserManager&MockObject $userManager; + + /** + * Mock group manager. + * + * @var IGroupManager&MockObject + */ + private IGroupManager&MockObject $groupManager; + + /** + * Mock schema mapper. + * + * @var SchemaMapper&MockObject + */ + private SchemaMapper&MockObject $schemaMapper; + + /** + * Mock object-entity mapper. + * + * @var MagicMapper&MockObject + */ + private MagicMapper&MockObject $objectEntityMapper; + + /** + * Mock condition matcher. + * + * @var ConditionMatcher&MockObject + */ + private ConditionMatcher&MockObject $conditionMatcher; + + /** + * Mock app config (provides the tenant default for inheritFromPublic). + * + * @var IAppConfig&MockObject + */ + private IAppConfig&MockObject $appConfig; + + /** + * Mock logger. + * + * @var LoggerInterface&MockObject + */ + private LoggerInterface&MockObject $logger; + + /** + * Mock DI container (used to resolve RegisterMapper lazily). + * + * @var ContainerInterface&MockObject + */ + private ContainerInterface&MockObject $container; + + /** + * Mock register mapper (resolved via the container). + * + * @var RegisterMapper&MockObject + */ + private RegisterMapper&MockObject $registerMapper; + + /** + * Wire up mocks and build a fresh PermissionHandler for each test case. + * + * @return void + */ + protected function setUp(): void + { + $this->userSession = $this->createMock(originalClassName: IUserSession::class); + $this->userManager = $this->createMock(originalClassName: IUserManager::class); + $this->groupManager = $this->createMock(originalClassName: IGroupManager::class); + $this->schemaMapper = $this->createMock(originalClassName: SchemaMapper::class); + $this->objectEntityMapper = $this->createMock(originalClassName: MagicMapper::class); + $this->conditionMatcher = $this->createMock(originalClassName: ConditionMatcher::class); + $this->appConfig = $this->createMock(originalClassName: IAppConfig::class); + $this->logger = $this->createMock(originalClassName: LoggerInterface::class); + $this->container = $this->createMock(originalClassName: ContainerInterface::class); + $this->registerMapper = $this->createMock(originalClassName: RegisterMapper::class); + + $this->handler = new PermissionHandler( + $this->userSession, + $this->userManager, + $this->groupManager, + $this->schemaMapper, + $this->objectEntityMapper, + $this->conditionMatcher, + $this->appConfig, + $this->logger, + $this->container + ); + + }//end setUp() + + /** + * Build a Schema fixture with the given id and authorization block. + * + * @param int $id The schema id. + * @param array|null $authorization The authorization block (or null for no authz). + * + * @return Schema + */ + private function createSchema(int $id, ?array $authorization): Schema + { + $schema = new Schema(); + $schema->setId($id); + $schema->setAuthorization($authorization); + $schema->setTitle('Test Schema '.$id); + return $schema; + + }//end createSchema() + + /** + * Build a Register fixture with the given id and authorization block. + * + * @param int $id The register id. + * @param array|null $authorization The authorization block (or null for no authz). + * + * @return Register + */ + private function createRegister(int $id, ?array $authorization): Register + { + $register = new Register(); + $register->setId($id); + $register->setAuthorization($authorization); + $register->setTitle('Test Register '.$id); + return $register; + + }//end createRegister() + + /** + * Configure the user session mock to return no current user (anonymous). + * + * @return void + */ + private function mockNoUser(): void + { + $this->userSession->method('getUser')->willReturn(null); + + }//end mockNoUser() + + /** + * Configure the user session, user manager, and group manager mocks for a logged-in user. + * + * @param string $uid The user id. + * @param array $groups The user's group memberships. + * + * @return void + */ + private function mockUser(string $uid, array $groups): void + { + $user = $this->createMock(originalClassName: IUser::class); + $user->method('getUID')->willReturn($uid); + $this->userSession->method('getUser')->willReturn($user); + $this->userManager->method('get')->willReturn($user); + $this->groupManager->method('getUserGroupIds')->willReturn($groups); + + }//end mockUser() + + /** + * Wire the container + register mapper so resolveInheritFromPublic can find the parent register. + * + * @param int $registerId The register id to expose. + * @param Register $register The register mock to return from find(). + * + * @return void + */ + private function wireRegister(int $registerId, Register $register): void + { + $this->registerMapper->method('getFirstRegisterWithSchema')->willReturn($registerId); + $this->registerMapper->method('find')->willReturn($register); + $this->container->method('get')->willReturnCallback( + fn (string $class) => $class === RegisterMapper::class ? $this->registerMapper : null + ); + + }//end wireRegister() + + // ---------- Cascade tests ---------- + + /** + * Cascade falls through to the hard-coded `true` default when no value is set + * at any level (schema, register, tenant). + * + * @return void + */ + public function testCascadeReturnsHardCodedTrueWhenNothingSet(): void + { + $schema = $this->createSchema(id: 1, authorization: null); + $this->appConfig + ->method('getValueBool') + ->with('openregister', 'rbac.inherit_from_public_default', true) + ->willReturn(true); + + $result = $this->handler->resolveInheritFromPublic(schema: $schema); + + $this->assertTrue(condition: $result); + + }//end testCascadeReturnsHardCodedTrueWhenNothingSet() + + /** + * Schema-level value wins when set, regardless of register / tenant defaults. + * + * @return void + */ + public function testCascadeUsesSchemaValueWhenSet(): void + { + $schema = $this->createSchema(id: 1, authorization: ['inheritFromPublic' => false]); + + $result = $this->handler->resolveInheritFromPublic(schema: $schema); + + $this->assertFalse(condition: $result); + + }//end testCascadeUsesSchemaValueWhenSet() + + /** + * When the schema does not set inheritFromPublic, the cascade falls through to the register's value. + * + * @return void + */ + public function testCascadeFallsBackToRegisterWhenSchemaUnset(): void + { + $schema = $this->createSchema(id: 1, authorization: null); + $register = $this->createRegister(id: 10, authorization: ['inheritFromPublic' => false]); + $this->wireRegister(registerId: 10, register: $register); + + $result = $this->handler->resolveInheritFromPublic(schema: $schema); + + $this->assertFalse(condition: $result); + + }//end testCascadeFallsBackToRegisterWhenSchemaUnset() + + /** + * When neither schema nor register sets the flag, the IAppConfig tenant default is used. + * + * @return void + */ + public function testCascadeFallsBackToTenantDefaultWhenSchemaAndRegisterUnset(): void + { + $schema = $this->createSchema(id: 1, authorization: null); + $this->appConfig + ->method('getValueBool') + ->with('openregister', 'rbac.inherit_from_public_default', true) + ->willReturn(false); + + $result = $this->handler->resolveInheritFromPublic(schema: $schema); + + $this->assertFalse(condition: $result); + + }//end testCascadeFallsBackToTenantDefaultWhenSchemaAndRegisterUnset() + + /** + * Schema-level explicit value wins over both register-level and tenant-level values. + * + * @return void + */ + public function testCascadeSchemaWinsOverRegisterAndTenant(): void + { + $schema = $this->createSchema(id: 1, authorization: ['inheritFromPublic' => true]); + $register = $this->createRegister(id: 10, authorization: ['inheritFromPublic' => false]); + $this->wireRegister(registerId: 10, register: $register); + $this->appConfig->method('getValueBool')->willReturn(false); + + $result = $this->handler->resolveInheritFromPublic(schema: $schema); + + $this->assertTrue(condition: $result); + + }//end testCascadeSchemaWinsOverRegisterAndTenant() + + /** + * An explicit `null` at the schema level is treated as "unset" — the cascade falls through. + * + * @return void + */ + public function testCascadeNullIsTreatedAsUnset(): void + { + $schema = $this->createSchema(id: 1, authorization: ['inheritFromPublic' => null]); + $register = $this->createRegister(id: 10, authorization: ['inheritFromPublic' => false]); + $this->wireRegister(registerId: 10, register: $register); + + $result = $this->handler->resolveInheritFromPublic(schema: $schema); + + $this->assertFalse( + condition: $result, + message: 'Schema-level null should fall through to register-level value.' + ); + + }//end testCascadeNullIsTreatedAsUnset() + + /** + * Repeated calls return the same cached result (per-request cache). + * + * @return void + */ + public function testCachingReturnsSameResultOnRepeatedCalls(): void + { + $schema = $this->createSchema(id: 1, authorization: ['inheritFromPublic' => false]); + + $first = $this->handler->resolveInheritFromPublic(schema: $schema); + $second = $this->handler->resolveInheritFromPublic(schema: $schema); + + $this->assertSame(expected: $first, actual: $second); + $this->assertFalse(condition: $first); + + }//end testCachingReturnsSameResultOnRepeatedCalls() + + // ---------- Four-state matrix on hasPermission ---------- + + /** + * Anonymous user is granted when the public rule matches and inheritFromPublic is true. + * + * @return void + */ + public function testAnonUserGrantedWhenPublicMatchPassesAndInheritIsTrue(): void + { + $schema = $this->createSchema( + id: 1, + authorization: [ + 'read' => [['group' => 'public']], + 'inheritFromPublic' => true, + ] + ); + $this->mockNoUser(); + + $result = $this->handler->hasPermission(schema: $schema, action: 'read'); + + $this->assertTrue(condition: $result); + + }//end testAnonUserGrantedWhenPublicMatchPassesAndInheritIsTrue() + + /** + * Anonymous user is granted even when inheritFromPublic is false — anonymous users + * are unaffected by the flag (they aren't authenticated, so inheritance doesn't apply). + * + * @return void + */ + public function testAnonUserGrantedWhenPublicMatchPassesAndInheritIsFalse(): void + { + $schema = $this->createSchema( + id: 1, + authorization: [ + 'read' => [['group' => 'public']], + 'inheritFromPublic' => false, + ] + ); + $this->mockNoUser(); + + $result = $this->handler->hasPermission(schema: $schema, action: 'read'); + + $this->assertTrue(condition: $result); + + }//end testAnonUserGrantedWhenPublicMatchPassesAndInheritIsFalse() + + /** + * Authenticated user with no own-group match is granted via inheritance from public + * when inheritFromPublic is true (pre-change semantics). + * + * @return void + */ + public function testAuthUserGrantedWhenPublicMatchPassesAndInheritIsTrue(): void + { + $schema = $this->createSchema( + id: 1, + authorization: [ + 'read' => [['group' => 'public']], + 'inheritFromPublic' => true, + ] + ); + $this->mockUser(uid: 'alice', groups: ['users']); + + $result = $this->handler->hasPermission(schema: $schema, action: 'read'); + + $this->assertTrue(condition: $result); + + }//end testAuthUserGrantedWhenPublicMatchPassesAndInheritIsTrue() + + /** + * Authenticated user with no own-group match is denied when inheritFromPublic is false — + * the flag prevents the public rule from applying to authenticated users. + * + * @return void + */ + public function testAuthUserDeniedWhenPublicMatchPassesAndInheritIsFalse(): void + { + $schema = $this->createSchema( + id: 1, + authorization: [ + 'read' => [['group' => 'public']], + 'inheritFromPublic' => false, + ] + ); + $this->mockUser(uid: 'alice', groups: ['users']); + + $result = $this->handler->hasPermission(schema: $schema, action: 'read'); + + $this->assertFalse(condition: $result); + + }//end testAuthUserDeniedWhenPublicMatchPassesAndInheritIsFalse() + + // ---------- Owner / admin shortcuts unaffected ---------- + + /** + * Admin user is always granted, regardless of inheritFromPublic. + * + * @return void + */ + public function testAdminUserGrantedRegardlessOfFlag(): void + { + $schema = $this->createSchema( + id: 1, + authorization: [ + 'read' => [['group' => 'public']], + 'inheritFromPublic' => false, + ] + ); + $this->mockUser(uid: 'admin-user', groups: ['admin']); + + $result = $this->handler->hasPermission(schema: $schema, action: 'read'); + + $this->assertTrue( + condition: $result, + message: 'Admin must always be granted, regardless of inheritFromPublic.' + ); + + }//end testAdminUserGrantedRegardlessOfFlag() + + /** + * Object owner is always granted, regardless of inheritFromPublic. + * + * @return void + */ + public function testOwnerGrantedRegardlessOfFlag(): void + { + $schema = $this->createSchema( + id: 1, + authorization: [ + 'read' => [['group' => 'public']], + 'inheritFromPublic' => false, + ] + ); + $this->mockUser(uid: 'carol', groups: ['users']); + + $result = $this->handler->hasPermission( + schema: $schema, + action: 'read', + objectOwner: 'carol' + ); + + $this->assertTrue( + condition: $result, + message: 'Object owner must always be granted, regardless of inheritFromPublic.' + ); + + }//end testOwnerGrantedRegardlessOfFlag() + + /** + * Authenticated user with explicit group access is granted when inheritFromPublic is false — + * the flag only affects public inheritance, not explicit group memberships. + * + * @return void + */ + public function testAuthUserGrantedViaOwnGroupEvenWhenInheritIsFalse(): void + { + $schema = $this->createSchema( + id: 1, + authorization: [ + 'read' => [ + ['group' => 'public'], + 'editors', + ], + 'inheritFromPublic' => false, + ] + ); + $this->mockUser(uid: 'bob', groups: ['editors']); + + $result = $this->handler->hasPermission(schema: $schema, action: 'read'); + + $this->assertTrue(condition: $result); + + }//end testAuthUserGrantedViaOwnGroupEvenWhenInheritIsFalse() +}//end class diff --git a/tests/Unit/Service/Object/PermissionHandlerRbacTest.php b/tests/Unit/Service/Object/PermissionHandlerRbacTest.php index 7640b5e334..f122514e88 100644 --- a/tests/Unit/Service/Object/PermissionHandlerRbacTest.php +++ b/tests/Unit/Service/Object/PermissionHandlerRbacTest.php @@ -13,6 +13,7 @@ use OCA\OpenRegister\Service\ConditionMatcher; use OCA\OpenRegister\Service\Object\PermissionHandler; use OCA\OpenRegister\Service\OperatorEvaluator; +use OCP\IAppConfig; use OCP\IGroupManager; use OCP\IUser; use OCP\IUserManager; @@ -27,29 +28,46 @@ */ class PermissionHandlerRbacTest extends TestCase { + private PermissionHandler $handler; + private IUserSession&MockObject $userSession; + private IUserManager&MockObject $userManager; + private IGroupManager&MockObject $groupManager; + private SchemaMapper&MockObject $schemaMapper; + private MagicMapper&MockObject $objectEntityMapper; + private ConditionMatcher&MockObject $conditionMatcher; + + private IAppConfig&MockObject $appConfig; + private LoggerInterface&MockObject $logger; + private ContainerInterface&MockObject $container; + private RegisterMapper&MockObject $registerMapper; protected function setUp(): void { - $this->userSession = $this->createMock(IUserSession::class); - $this->userManager = $this->createMock(IUserManager::class); - $this->groupManager = $this->createMock(IGroupManager::class); - $this->schemaMapper = $this->createMock(SchemaMapper::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->userManager = $this->createMock(IUserManager::class); + $this->groupManager = $this->createMock(IGroupManager::class); + $this->schemaMapper = $this->createMock(SchemaMapper::class); $this->objectEntityMapper = $this->createMock(MagicMapper::class); - $this->conditionMatcher = $this->createMock(ConditionMatcher::class); - $this->logger = $this->createMock(LoggerInterface::class); - $this->container = $this->createMock(ContainerInterface::class); + $this->conditionMatcher = $this->createMock(ConditionMatcher::class); + $this->appConfig = $this->createMock(IAppConfig::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->container = $this->createMock(ContainerInterface::class); $this->registerMapper = $this->createMock(RegisterMapper::class); + // Default: tenant default for inheritFromPublic is `true`, preserving + // pre-change behaviour for tests that don't opt out explicitly. + $this->appConfig->method('getValueBool')->willReturn(true); + $this->handler = new PermissionHandler( $this->userSession, $this->userManager, @@ -57,10 +75,11 @@ protected function setUp(): void $this->schemaMapper, $this->objectEntityMapper, $this->conditionMatcher, + $this->appConfig, $this->logger, $this->container ); - } + }//end setUp() private function mockUser(string $uid, array $groups): IUser&MockObject { @@ -71,60 +90,69 @@ private function mockUser(string $uid, array $groups): IUser&MockObject $this->userManager->method('get')->willReturn($user); $this->groupManager->method('getUserGroupIds')->willReturn($groups); return $user; - } + }//end mockUser() private function createSchema(int $id, ?array $authorization): Schema { $schema = new Schema(); $schema->setId($id); $schema->setAuthorization($authorization); - $schema->setTitle('Test Schema ' . $id); + $schema->setTitle('Test Schema '.$id); return $schema; - } + }//end createSchema() - private function createRegister(int $id, ?array $authorization, ?array $configuration = null): Register + private function createRegister(int $id, ?array $authorization, ?array $configuration=null): Register { $register = new Register(); $register->setId($id); $register->setAuthorization($authorization); $register->setConfiguration($configuration ?? []); return $register; - } + }//end createRegister() private function setupRegisterForSchema(int $schemaId, Register $register): void { $this->container->method('get') - ->willReturnCallback(function (string $class) use ($register) { - if ($class === RegisterMapper::class) { - return $this->registerMapper; - } - if ($class === 'OCA\OpenRegister\Service\OrganisationService') { - throw new \RuntimeException('Not available'); - } - throw new \RuntimeException('Unknown class: ' . $class); - }); + ->willReturnCallback( + function (string $class) use ($register) { + if ($class === RegisterMapper::class) { + return $this->registerMapper; + } + + if ($class === 'OCA\OpenRegister\Service\OrganisationService') { + throw new \RuntimeException('Not available'); + } + + throw new \RuntimeException('Unknown class: '.$class); + } + ); $this->registerMapper->method('getFirstRegisterWithSchema') ->willReturn($register->getId()); $this->registerMapper->method('find') ->willReturn($register); - } + }//end setupRegisterForSchema() // === Register Cascade Tests === - public function testSchemaAuthorizationOverridesRegister(): void { $this->mockUser('user1', ['behandelaars']); - $schema = $this->createSchema(1, [ - 'read' => ['behandelaars'], - 'create' => ['admin'], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => ['behandelaars'], + 'create' => ['admin'], + ] + ); - $register = $this->createRegister(10, [ - 'read' => ['public'], - 'create' => ['public'], - ]); + $register = $this->createRegister( + 10, + [ + 'read' => ['public'], + 'create' => ['public'], + ] + ); $this->setupRegisterForSchema(1, $register); @@ -134,7 +162,7 @@ public function testSchemaAuthorizationOverridesRegister(): void // Schema says only admin can create, not behandelaars. $this->assertFalse($this->handler->hasPermission($schema, 'create')); - } + }//end testSchemaAuthorizationOverridesRegister() public function testRegisterFallbackWhenSchemaHasNoAuth(): void { @@ -143,10 +171,13 @@ public function testRegisterFallbackWhenSchemaHasNoAuth(): void // Schema has NO authorization. $schema = $this->createSchema(1, null); - $register = $this->createRegister(10, [ - 'read' => ['medewerkers'], - 'create' => ['admin'], - ]); + $register = $this->createRegister( + 10, + [ + 'read' => ['medewerkers'], + 'create' => ['admin'], + ] + ); $this->setupRegisterForSchema(1, $register); @@ -155,13 +186,13 @@ public function testRegisterFallbackWhenSchemaHasNoAuth(): void // Register says only admin can create. $this->assertFalse($this->handler->hasPermission($schema, 'create')); - } + }//end testRegisterFallbackWhenSchemaHasNoAuth() public function testNeitherSchemaNorRegisterHasAuth(): void { $this->mockUser('user1', ['somegroup']); - $schema = $this->createSchema(1, null); + $schema = $this->createSchema(1, null); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -169,52 +200,65 @@ public function testNeitherSchemaNorRegisterHasAuth(): void // No authorization anywhere = everyone has permission. $this->assertTrue($this->handler->hasPermission($schema, 'read')); $this->assertTrue($this->handler->hasPermission($schema, 'create')); - } + }//end testNeitherSchemaNorRegisterHasAuth() // === Role Expansion Tests === - public function testRoleExpansionViewerRole(): void { $this->mockUser('user1', ['public']); - $schema = $this->createSchema(1, [ - 'roles' => [ - 'viewer' => ['public'], - 'editor' => ['behandelaars'], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'roles' => [ + 'viewer' => ['public'], + 'editor' => ['behandelaars'], + ], + ] + ); - $register = $this->createRegister(10, null, [ - 'roles' => [ - ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], - ['name' => 'editor', 'description' => 'Edit access', 'actions' => ['read', 'create', 'update']], - ], - ]); + $register = $this->createRegister( + 10, + null, + [ + 'roles' => [ + ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], + ['name' => 'editor', 'description' => 'Edit access', 'actions' => ['read', 'create', 'update']], + ], + ] + ); $this->setupRegisterForSchema(1, $register); // Public group has viewer role => read only. $this->assertTrue($this->handler->hasPermission($schema, 'read')); $this->assertFalse($this->handler->hasPermission($schema, 'create')); - } + }//end testRoleExpansionViewerRole() public function testRoleExpansionEditorRole(): void { $this->mockUser('user1', ['behandelaars']); - $schema = $this->createSchema(1, [ - 'roles' => [ - 'viewer' => ['public'], - 'editor' => ['behandelaars'], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'roles' => [ + 'viewer' => ['public'], + 'editor' => ['behandelaars'], + ], + ] + ); - $register = $this->createRegister(10, null, [ - 'roles' => [ - ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], - ['name' => 'editor', 'description' => 'Edit access', 'actions' => ['read', 'create', 'update']], - ], - ]); + $register = $this->createRegister( + 10, + null, + [ + 'roles' => [ + ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], + ['name' => 'editor', 'description' => 'Edit access', 'actions' => ['read', 'create', 'update']], + ], + ] + ); $this->setupRegisterForSchema(1, $register); @@ -224,46 +268,60 @@ public function testRoleExpansionEditorRole(): void $this->assertTrue($this->handler->hasPermission($schema, 'create')); $this->assertTrue($this->handler->hasPermission($schema, 'update')); $this->assertTrue($this->handler->hasPermission($schema, 'delete')); - } + }//end testRoleExpansionEditorRole() public function testMixedRoleAndDirectAuth(): void { $this->mockUser('user1', ['extra-groep']); - $schema = $this->createSchema(1, [ - 'roles' => [ - 'viewer' => ['public'], - ], - 'read' => ['extra-groep'], - ]); + $schema = $this->createSchema( + 1, + [ + 'roles' => [ + 'viewer' => ['public'], + ], + 'read' => ['extra-groep'], + ] + ); - $register = $this->createRegister(10, null, [ - 'roles' => [ - ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], - ], - ]); + $register = $this->createRegister( + 10, + null, + [ + 'roles' => [ + ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], + ], + ] + ); $this->setupRegisterForSchema(1, $register); // extra-groep has direct read permission. $this->assertTrue($this->handler->hasPermission($schema, 'read')); - } + }//end testMixedRoleAndDirectAuth() public function testUnknownRoleNameIsIgnored(): void { $this->mockUser('user1', ['public']); - $schema = $this->createSchema(1, [ - 'roles' => [ - 'archiver' => ['public'], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'roles' => [ + 'archiver' => ['public'], + ], + ] + ); - $register = $this->createRegister(10, null, [ - 'roles' => [ - ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], - ], - ]); + $register = $this->createRegister( + 10, + null, + [ + 'roles' => [ + ['name' => 'viewer', 'description' => 'Read only', 'actions' => ['read']], + ], + ] + ); $this->setupRegisterForSchema(1, $register); @@ -275,34 +333,39 @@ public function testUnknownRoleNameIsIgnored(): void // because the effective authorization ends up empty after role expansion. $result = $this->handler->resolveAuthorization($schema); $this->assertEmpty($result); - } + }//end testUnknownRoleNameIsIgnored() // === Manage Action Tests === - public function testManageActionEvaluated(): void { $this->mockUser('user1', ['register-beheerders']); - $schema = $this->createSchema(1, [ - 'manage' => ['register-beheerders'], - 'read' => ['public'], - ]); + $schema = $this->createSchema( + 1, + [ + 'manage' => ['register-beheerders'], + 'read' => ['public'], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); // User in register-beheerders should have manage permission. $this->assertTrue($this->handler->hasPermission($schema, 'manage')); - } + }//end testManageActionEvaluated() public function testManageActionDenied(): void { $this->mockUser('user1', ['behandelaars']); - $schema = $this->createSchema(1, [ - 'manage' => ['register-beheerders'], - 'read' => ['behandelaars'], - ]); + $schema = $this->createSchema( + 1, + [ + 'manage' => ['register-beheerders'], + 'read' => ['behandelaars'], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -311,22 +374,25 @@ public function testManageActionDenied(): void $this->assertFalse($this->handler->hasPermission($schema, 'manage')); // But should still be able to read. $this->assertTrue($this->handler->hasPermission($schema, 'read')); - } + }//end testManageActionDenied() public function testAdminBypassesManageCheck(): void { $this->mockUser('admin1', ['admin']); - $schema = $this->createSchema(1, [ - 'manage' => ['register-beheerders'], - ]); + $schema = $this->createSchema( + 1, + [ + 'manage' => ['register-beheerders'], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); // Admin always has all permissions. $this->assertTrue($this->handler->hasPermission($schema, 'manage')); - } + }//end testAdminBypassesManageCheck() // ------------------------------------------------------------------ // Conditional rule delegation tests (ADR-011 — ConditionMatcher). @@ -335,30 +401,34 @@ public function testAdminBypassesManageCheck(): void // rule evaluation to the shared ConditionMatcher service and that the // admin/owner bypasses short-circuit before delegation. // ------------------------------------------------------------------ - - private function createObjectEntity(array $data, ?string $owner = null, ?string $organisation = null): ObjectEntity + private function createObjectEntity(array $data, ?string $owner=null, ?string $organisation=null): ObjectEntity { $object = new ObjectEntity(); $object->setObject($data); if ($owner !== null) { $object->setOwner($owner); } + if ($organisation !== null) { $object->setOrganisation($organisation); } + return $object; - } + }//end createObjectEntity() public function testConditionalPublicRuleDelegatesToConditionMatcher(): void { // Anonymous caller, public-with-match rule. $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishDate' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishDate' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -370,9 +440,11 @@ public function testConditionalPublicRuleDelegatesToConditionMatcher(): void ->expects($this->once()) ->method('objectMatchesConditions') ->with( - $this->callback(function (array $envelope): bool { - return ($envelope['publishDate'] ?? null) === '2025-01-01'; - }), + $this->callback( + function (array $envelope): bool { + return ($envelope['publishDate'] ?? null) === '2025-01-01'; + } + ), ['publishDate' => ['$lte' => '$now']] ) ->willReturn(true); @@ -387,17 +459,20 @@ public function testConditionalPublicRuleDelegatesToConditionMatcher(): void object: $object ) ); - } + }//end testConditionalPublicRuleDelegatesToConditionMatcher() public function testConditionalRuleReturnsFalseWhenConditionMatcherReturnsFalse(): void { $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishDate' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishDate' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -419,17 +494,20 @@ public function testConditionalRuleReturnsFalseWhenConditionMatcherReturnsFalse( object: $object ) ); - } + }//end testConditionalRuleReturnsFalseWhenConditionMatcherReturnsFalse() public function testUserIdVariableRuleDelegatesToConditionMatcher(): void { $this->mockUser('jan', ['medewerkers']); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'medewerkers', 'match' => ['assignedTo' => '$userId']], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'medewerkers', 'match' => ['assignedTo' => '$userId']], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -455,17 +533,20 @@ public function testUserIdVariableRuleDelegatesToConditionMatcher(): void object: $object ) ); - } + }//end testUserIdVariableRuleDelegatesToConditionMatcher() public function testInOperatorRuleDelegatesToConditionMatcher(): void { $this->mockUser('jan', ['behandelaars']); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'behandelaars', 'match' => ['status' => ['$in' => ['open', 'review']]]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'behandelaars', 'match' => ['status' => ['$in' => ['open', 'review']]]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -478,17 +559,20 @@ public function testInOperatorRuleDelegatesToConditionMatcher(): void ->willReturn(true); $this->assertTrue($this->handler->hasPermission($schema, 'read', 'jan', null, true, $object)); - } + }//end testInOperatorRuleDelegatesToConditionMatcher() public function testOrganisationVariableFoldsIntoEnvelopeViaSelf(): void { $this->mockUser('jan', ['behandelaars']); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'behandelaars', 'match' => ['_organisation' => '$organisation']], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'behandelaars', 'match' => ['_organisation' => '$organisation']], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -502,26 +586,31 @@ public function testOrganisationVariableFoldsIntoEnvelopeViaSelf(): void ->expects($this->once()) ->method('objectMatchesConditions') ->with( - $this->callback(function (array $envelope): bool { - return (($envelope['@self']['organisation'] ?? null) === 'org-abc-123') - && (($envelope['name'] ?? null) === 'zaak-1'); - }), + $this->callback( + function (array $envelope): bool { + return (($envelope['@self']['organisation'] ?? null) === 'org-abc-123') + && (($envelope['name'] ?? null) === 'zaak-1'); + } + ), ['_organisation' => '$organisation'] ) ->willReturn(true); $this->assertTrue($this->handler->hasPermission($schema, 'read', 'jan', null, true, $object)); - } + }//end testOrganisationVariableFoldsIntoEnvelopeViaSelf() public function testAdminBypassSkipsConditionMatcher(): void { $this->mockUser('admin1', ['admin']); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'behandelaars', 'match' => ['status' => 'open']], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'behandelaars', 'match' => ['status' => 'open']], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -534,17 +623,20 @@ public function testAdminBypassSkipsConditionMatcher(): void ->method('objectMatchesConditions'); $this->assertTrue($this->handler->hasPermission($schema, 'read', 'admin1', null, true, $object)); - } + }//end testAdminBypassSkipsConditionMatcher() public function testOwnerBypassSkipsConditionMatcher(): void { $this->mockUser('jan', ['medewerkers']); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'behandelaars', 'match' => ['status' => 'open']], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'behandelaars', 'match' => ['status' => 'open']], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -566,16 +658,19 @@ public function testOwnerBypassSkipsConditionMatcher(): void object: $object ) ); - } + }//end testOwnerBypassSkipsConditionMatcher() public function testSimpleStringRuleDoesNotInvokeConditionMatcher(): void { // Simple group match without a `match` clause never reaches ConditionMatcher. $this->mockUser('jan', ['juridisch-team']); - $schema = $this->createSchema(1, [ - 'read' => ['juridisch-team'], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => ['juridisch-team'], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -585,16 +680,19 @@ public function testSimpleStringRuleDoesNotInvokeConditionMatcher(): void ->method('objectMatchesConditions'); $this->assertTrue($this->handler->hasPermission($schema, 'read', 'jan')); - } + }//end testSimpleStringRuleDoesNotInvokeConditionMatcher() public function testConditionalRuleWithoutMatchClauseDoesNotInvokeConditionMatcher(): void { // Conditional rule with an empty/missing match is treated as a plain group match. $this->mockUser('jan', ['behandelaars']); - $schema = $this->createSchema(1, [ - 'read' => [['group' => 'behandelaars']], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [['group' => 'behandelaars']], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -604,7 +702,7 @@ public function testConditionalRuleWithoutMatchClauseDoesNotInvokeConditionMatch ->method('objectMatchesConditions'); $this->assertTrue($this->handler->hasPermission($schema, 'read', 'jan')); - } + }//end testConditionalRuleWithoutMatchClauseDoesNotInvokeConditionMatcher() public function testAnonymousCallerAgainstNonPublicRuleReturnsFalseWithoutDelegation(): void { @@ -612,9 +710,12 @@ public function testAnonymousCallerAgainstNonPublicRuleReturnsFalseWithoutDelega // without consulting ConditionMatcher (no conditional `public` rule to evaluate). $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => ['juridisch-team'], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => ['juridisch-team'], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); @@ -624,18 +725,17 @@ public function testAnonymousCallerAgainstNonPublicRuleReturnsFalseWithoutDelega ->method('objectMatchesConditions'); $this->assertFalse($this->handler->hasPermission($schema, 'read')); - } + }//end testAnonymousCallerAgainstNonPublicRuleReturnsFalseWithoutDelegation() // ------------------------------------------------------------------ // End-to-end wiring test with REAL ConditionMatcher + OperatorEvaluator. // // Reproduces the user-reported bug: schema with - // { "read": [{ "group": "public", "match": { "publishedAt": { "$lte": "$now" } } }] } + // { "read": [{ "group": "public", "match": { "publishedAt": { "$lte": "$now" } } }] } // must grant access to objects whose publishedAt is in the past AND deny // access to objects with publishedAt = null (so the list endpoint and the // find endpoint agree — SQL's NULL semantics is the contract). // ------------------------------------------------------------------ - private function buildHandlerWithRealMatcher(): PermissionHandler { $operatorEvaluator = new OperatorEvaluator($this->logger); @@ -655,22 +755,25 @@ private function buildHandlerWithRealMatcher(): PermissionHandler $this->logger, $this->container ); - } + }//end buildHandlerWithRealMatcher() public function testPublicLteNowRuleMatchesPastPublishedAt(): void { $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); - $object = $this->createObjectEntity(['publishedAt' => '2025-01-01 00:00:00']); + $object = $this->createObjectEntity(['publishedAt' => '2025-01-01 00:00:00']); $handler = $this->buildHandlerWithRealMatcher(); $this->assertTrue( @@ -684,7 +787,7 @@ public function testPublicLteNowRuleMatchesPastPublishedAt(): void ), 'Past-dated publication should be accessible via $lte $now rule' ); - } + }//end testPublicLteNowRuleMatchesPastPublishedAt() public function testPublicLteNowRuleRejectsNullPublishedAt(): void { @@ -692,18 +795,21 @@ public function testPublicLteNowRuleRejectsNullPublishedAt(): void // OperatorEvaluator used raw PHP <= with null coerced to empty string. $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); // Object has no publishedAt value at all — the property is absent from // the data map, so getObjectValue returns null. - $object = $this->createObjectEntity(['title' => 'draft']); + $object = $this->createObjectEntity(['title' => 'draft']); $handler = $this->buildHandlerWithRealMatcher(); $this->assertFalse( @@ -717,23 +823,26 @@ public function testPublicLteNowRuleRejectsNullPublishedAt(): void ), 'Publication with null publishedAt must NOT match $lte $now (SQL-aligned semantics)' ); - } + }//end testPublicLteNowRuleRejectsNullPublishedAt() public function testPublicLteNowRuleRejectsExplicitNullPublishedAt(): void { // Same as above but with the property explicitly set to null in the data map. $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); - $object = $this->createObjectEntity(['publishedAt' => null, 'title' => 'draft']); + $object = $this->createObjectEntity(['publishedAt' => null, 'title' => 'draft']); $handler = $this->buildHandlerWithRealMatcher(); $this->assertFalse( @@ -746,23 +855,26 @@ public function testPublicLteNowRuleRejectsExplicitNullPublishedAt(): void object: $object ) ); - } + }//end testPublicLteNowRuleRejectsExplicitNullPublishedAt() public function testPublicLteNowRuleRejectsFuturePublishedAt(): void { // Sanity: future-dated publication should also be denied (not yet published). $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); - $object = $this->createObjectEntity(['publishedAt' => '2099-01-01 00:00:00']); + $object = $this->createObjectEntity(['publishedAt' => '2099-01-01 00:00:00']); $handler = $this->buildHandlerWithRealMatcher(); $this->assertFalse( @@ -775,7 +887,7 @@ public function testPublicLteNowRuleRejectsFuturePublishedAt(): void object: $object ) ); - } + }//end testPublicLteNowRuleRejectsFuturePublishedAt() // ------------------------------------------------------------------ // $now format alignment tests. @@ -787,7 +899,6 @@ public function testPublicLteNowRuleRejectsFuturePublishedAt(): void // // Canonical format: Y-m-d H:i:s (SQL-native). // ------------------------------------------------------------------ - public function testNowResolvesToSqlNativeFormat(): void { // If this test ever fails, the list and find endpoints will diverge @@ -796,17 +907,20 @@ public function testNowResolvesToSqlNativeFormat(): void // scenario in specs/rbac-scopes/spec.md. $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); // Stored date in SQL-native Y-m-d H:i:s — the canonical format. - $object = $this->createObjectEntity(['publishedAt' => '2025-06-01 12:00:00']); + $object = $this->createObjectEntity(['publishedAt' => '2025-06-01 12:00:00']); $handler = $this->buildHandlerWithRealMatcher(); $this->assertTrue( @@ -820,7 +934,7 @@ public function testNowResolvesToSqlNativeFormat(): void ), '$now must resolve to Y-m-d H:i:s so it lex-compares correctly against Y-m-d H:i:s stored dates' ); - } + }//end testNowResolvesToSqlNativeFormat() public function testNowAlignsWithSqlPathForIsoStoredDates(): void { @@ -836,16 +950,19 @@ public function testNowAlignsWithSqlPathForIsoStoredDates(): void // handles this on input). $this->userSession->method('getUser')->willReturn(null); - $schema = $this->createSchema(1, [ - 'read' => [ - ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], - ], - ]); + $schema = $this->createSchema( + 1, + [ + 'read' => [ + ['group' => 'public', 'match' => ['publishedAt' => ['$lte' => '$now']]], + ], + ] + ); $register = $this->createRegister(10, null); $this->setupRegisterForSchema(1, $register); - $object = $this->createObjectEntity(['publishedAt' => '2025-06-01T12:00:00Z']); + $object = $this->createObjectEntity(['publishedAt' => '2025-06-01T12:00:00Z']); $handler = $this->buildHandlerWithRealMatcher(); // Both paths lex-compare: '2025-06-01T...' vs '