diff --git a/docs/Features/access-control.md b/docs/Features/access-control.md index 35aeddd69a..4d31baf77d 100644 --- a/docs/Features/access-control.md +++ b/docs/Features/access-control.md @@ -448,10 +448,17 @@ sequenceDiagram "create": ["admin", "editors"], "read": ["admin", "editors", "viewers", "public"], "update": ["admin", "editors"], - "delete": ["admin"] + "delete": ["admin"], + "inheritFromPublic": true } ``` +The optional `inheritFromPublic` boolean controls whether authenticated users +qualify for `public` rules on this schema. It defaults to `true` (the +pre-change behaviour). See [Disabling public-group inheritance for +authenticated users](#disabling-public-group-inheritance-for-authenticated-users-inheritfrompublic) +below. + **Object Authorization Field:** - Stored in `oc_openregister_objects.authorization` (JSON) - Inherits from schema but can be overridden per-object @@ -600,6 +607,124 @@ All three enforcement points route conditional match evaluation through the shar A schema authored with `{ "read": [{ "group": "public", "match": { "publishDate": { "$lte": "$now" } } }] }` returns the same object set from `GET /api/objects/{register}/{schema}` (list) and `GET /api/objects/{register}/{schema}/{id}` (find). List-vs-find drift caused by differing grammar is no longer possible. +### Disabling public-group inheritance for authenticated users (`inheritFromPublic`) + +By default, authenticated users qualify for any rule that targets the `public` +group — they inherit at least the rights of an anonymous visitor. This is +convenient for most schemas, but it gets in the way of two patterns: + +1. **Privacy-strict schemas** where authentication is meant to be a strict + gate, not a superset of public access. For example: a schema where the + public group can only see redacted/anonymised rows via a conditional + rule, but logged-in users should be channelled through a different + curated view rather than seeing the same redacted set. +2. **Tiered visibility flows** where a public catalogue uses a date-windowed + `match` (e.g. `publishedAt $lte $now`) and a separate authenticated + curated view uses its own group rule. With public inheritance on, the + authenticated view leaks the public catalogue rows. + +The optional `inheritFromPublic` boolean on the authorization block of a +schema or register lets a tenant opt out of the inherit-from-public +behaviour. When `false`, authenticated users no longer qualify for `public` +rules on that schema/register; they must qualify via their own group +memberships. Anonymous users are unaffected — the flag does not change +what an unauthenticated visitor sees. + +#### Cascade + +The effective value is resolved per schema, walking the cascade until the +first explicitly-set value is found: + +1. The schema's own `authorization.inheritFromPublic`. +2. The parent register's `authorization.inheritFromPublic`. +3. The tenant-wide IAppConfig key `openregister.rbac.inherit_from_public_default` + (read via `IAppConfig::getValueBool`, which accepts `true`/`false`/`"true"`/ + `"false"`/`"1"`/`"0"`/`1`/`0` at the storage layer). +4. Hard-coded `true` (preserves pre-change behaviour). + +**Strict-boolean check at schema and register levels.** Steps 1 and 2 require +the stored value to be a literal `true` or `false` — anything else (string +forms, integers, mistyped JSON) is rejected as "unset" and the cascade +falls through, with a warning logged. This closes a foot-gun where a seed +write or migration that bypasses the schema validator could store the +string `"false"` and silently invert the gate (`(bool) "false"` is `true`). +Always store real JSON booleans on schema/register `authorization` blocks. +The IAppConfig layer keeps its broader tolerance because the `occ` / +operator surface intentionally accepts those forms. + +`null` at any level is treated as "unset" — the cascade falls through to +the next level. The first explicit boolean wins; a schema that sets the +flag overrides its register and the tenant default. + +The tenant default can be flipped from the OpenRegister settings UI under +**RBAC Configuration → Authenticated users inherit `public` group rights +(default)**, or via `occ config:app:set openregister rbac.inherit_from_public_default --value=false --type=boolean`. + +#### Four-state matrix + +For a schema with `read: [{ "group": "public", "match": }]`: + +| User | `inheritFromPublic` | Result | +|-------------------|---------------------|-----------------------------------------------------------------------| +| anonymous | `true` (default) | granted when `` matches (pre-change behaviour) | +| anonymous | `false` | granted when `` matches — anonymous users are unaffected | +| authenticated | `true` (default) | granted when `` matches (authenticated user inherits public) | +| authenticated | `false` | denied unless the user qualifies via another rule (own group / owner / admin) | + +Owner-shortcut and admin-bypass paths are unaffected by the flag — an +object's owner and any user in the `admin` group always have access +regardless of `inheritFromPublic`. + +The PHP-side per-object check (`PermissionHandler::hasPermission`) and the +SQL-side listing filter (`MagicRbacHandler::applyRbacFilters` / +`buildRbacConditionsSql`) both honour the flag identically; per-object +checks and listing membership cannot drift. + +#### Worked example: a publication-style schema with a curated authenticated view + +A `publication` schema needs to be visible to anonymous visitors only after +its `publishedAt` timestamp, but logged-in editors should see the curated +in-progress queue (which is a different rule) instead of the public-time +window. Authenticated users without `editors` membership should see +**nothing** — they should not inherit the public window. + +```json +{ + "authorization": { + "inheritFromPublic": false, + "read": [ + { "group": "public", "match": { "publishedAt": { "$lte": "$now" } } }, + { "group": "editors", "match": { "status": { "$in": ["draft", "review"] } } } + ] + } +} +``` + +Behaviour: + +- An anonymous visitor sees rows where `publishedAt <= now` (public match + applies; the flag does not affect anonymous users). +- An `editors` member sees rows where `status` is `draft` or `review` (their + group rule applies). They do NOT inherit the public time-window because + `inheritFromPublic` is `false`. +- A logged-in user who is not in `editors` sees nothing on this schema. + They have no qualifying rule, and the flag prevents them from falling + back to the public match. +- An owner of a row sees it via the owner shortcut, regardless of the flag. + +If the same schema were authored with `inheritFromPublic: true` (or the +field omitted), the third case would change: the non-`editors` logged-in +user would inherit the public window and see the same rows the anonymous +visitor sees. + +#### When to reach for `'authenticated'` instead of `'public'` + +`inheritFromPublic` governs the `public` group only. The simple-string +rule `'authenticated'` is a separate construct — it grants access to any +logged-in user, independent of the flag. If you want "any logged-in user, +no group filtering, regardless of `inheritFromPublic`", use +`{ "read": ["authenticated"] }` rather than relying on public-inheritance. + ### Property-Level Authorization In addition to the schema- and object-level rules above, individual properties can carry their own `authorization` block with conditional rules. This is covered in depth in [Property Authorization](./property-authorization.md); this section is a short map into that feature. @@ -651,12 +776,21 @@ RBAC can be configured in Nextcloud app settings: ```json { "enabled": true, - "adminOverride": true + "adminOverride": true, + "inheritFromPublicDefault": true } ``` - **`enabled`**: Master switch for RBAC system - **`adminOverride`**: Allow users in 'admin' group to bypass all RBAC checks +- **`inheritFromPublicDefault`**: Tenant-wide default for the schema-level + `inheritFromPublic` flag. When `true` (default), authenticated users + qualify for `public` rules on any schema that does not explicitly set + the flag. When `false`, authenticated users must qualify via their own + group memberships unless a schema or register opts back in. Persisted + to the IAppConfig key `openregister.rbac.inherit_from_public_default`. + See + [Disabling public-group inheritance for authenticated users](#disabling-public-group-inheritance-for-authenticated-users-inheritfrompublic). ### Performance Optimizations diff --git a/docs/Features/property-authorization.md b/docs/Features/property-authorization.md index a0e704a205..4cec5f49e9 100644 --- a/docs/Features/property-authorization.md +++ b/docs/Features/property-authorization.md @@ -69,7 +69,7 @@ Each rule combines a group check with an optional condition: | Field | Meaning | | --- | --- | -| `group` | A Nextcloud group the current user must belong to. The literal value `"public"` matches **any authenticated user** (including when no other group matches). | +| `group` | A Nextcloud group the current user must belong to. The literal value `"public"` matches anonymous users, and — by default — authenticated users as well. The schema-level `inheritFromPublic` flag (see [Access Control → inheritFromPublic](./access-control.md#disabling-public-group-inheritance-for-authenticated-users-inheritfrompublic)) lets a tenant turn off the authenticated-user inheritance per schema, register, or globally. | | `match` | Optional map of conditions evaluated against the object. All conditions must be true for the rule to grant access. Omit `match` for an unconditional rule. | A rule is satisfied when **both** the group check and every `match` condition pass. 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..b1feb1ef2f --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/plan.json @@ -0,0 +1,428 @@ +{ + "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": "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/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..819e90defb --- /dev/null +++ b/openspec/changes/rbac-disable-public-inheritance/tasks.md @@ -0,0 +1,74 @@ +## 1. resolveInheritFromPublic helper + +- [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) + +- [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 +- [x] 2.5 Verify owner / admin grants still work regardless of the flag. + +## 3. SQL-side enforcement (MagicRbacHandler) + +- [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. +- [x] 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). Covered in `tests/Unit/Db/MagicMapper/MagicRbacHandlerInheritFromPublicTest.php::testApplyRbacFiltersAuthInheritFalseDeniesAccess`. +- [x] 3.7 Unit-test `buildRbacConditionsSql` similarly (UNION path). Covered in `tests/Unit/Db/MagicMapper/MagicRbacHandlerInheritFromPublicTest.php` — 9 tests over the four-state matrix on conditional and simple-string rules + admin/authenticated parity checks. + +## 4. Schema entity / serialisation + +- [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 + +- [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). +- [x] 5.2 Document the key in `docs/` (extend existing RBAC documentation). Added in `docs/Features/access-control.md` under "Disabling public-group inheritance for authenticated users (inheritFromPublic)" and in the "RBAC Configuration" block. +- [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 + +- [ ] 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`. Persistence round-trip verified (settings endpoint preserves authorization-null on Publication Consent), but a behavioural smoke against DocuDesk's actual consent-fetch endpoint is deferred — to be exercised before promoting from `beta` to `main` or by the QA persona pass after merge. +- [x] 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. Verified via four-state matrix on /api/objects against the Cascade-Test register — see verify report. +- [ ] 6.3 Smoke-test against Softwarecatalog or any other consuming app. Default behaviour unchanged. + +## 7. Unit + integration tests + +- [x] 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`. Covered in the dedicated `tests/Unit/Service/Object/PermissionHandlerInheritFromPublicTest.php` (14 tests). +- [x] 7.2 `tests/unit/Db/MagicMapper/MagicRbacHandlerTest.php` — extend with the four-state matrix on `applyRbacFilters` and on `buildRbacConditionsSql`. Covered in the dedicated `tests/Unit/Db/MagicMapper/MagicRbacHandlerInheritFromPublicTest.php` (10 tests). +- [x] 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. + Covered by the live-stack smoke in step 9.4 (Cascade-Test register/schema 31, four-state matrix on /api/objects). +- [x] 7.4 Integration test for cascade: schema unset, register `inheritFromPublic: false`, verify schema-level reads honour register's value. Covered by `PermissionHandlerInheritFromPublicTest::testCascadeFallsBackToRegisterWhenSchemaUnset` (cascade unit test against the same `resolveInheritFromPublic` walked at runtime). +- [x] 7.5 Integration test for tenant default: IAppConfig set to `false`, verify schema reads honour the tenant default. Covered by `PermissionHandlerInheritFromPublicTest::testCascadeFallsBackToTenantDefaultWhenSchemaAndRegisterUnset`. + +## 8. Documentation + +- [x] 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". Added in `docs/Features/access-control.md`. Cross-reference added in `docs/Features/property-authorization.md` (the `"public"` group row of the rule table). +- [x] 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. Added under "Worked example: a publication-style schema with a curated authenticated view" in `docs/Features/access-control.md`. +- [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 + +- [x] 9.1 Run the full unit test suite — clean. RBAC-related suite (PermissionHandler + MagicRbac filters) is clean: 68/68 tests pass against the in-container PHPUnit runner. A pre-existing fatal in `SettingsControllerTest.php` blocks the full-suite run but is unrelated to this change. +- [x] 9.2 Run static analysis (Psalm / PHPStan at project strictness) — clean. +- [x] 9.3 Run code style (PHPCS at project config) — clean. +- [x] 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. Verified against the Docker NC stack via /api/objects and /api/objects/{slug}/{slug}/{uuid}; results match spec. +- [x] 9.5 Run `openspec validate rbac-disable-public-inheritance` — clean.