From da1e29e542b42c537b7974ef477ea0eec3b5a3b5 Mon Sep 17 00:00:00 2001 From: Ruben van der Linde Date: Wed, 21 Oct 2026 10:38:57 +0200 Subject: [PATCH] docs(openspec): access follows membership, a $memberOf match operator Assisted-by: ClaudeCode:claude-opus-5-5 --- .../access-follows-membership/design.md | 137 ++++++++++++++++++ .../access-follows-membership/proposal.md | 91 ++++++++++++ .../specs/rbac-scopes/spec.md | 62 ++++++++ .../access-follows-membership/tasks.md | 18 +++ 4 files changed, 308 insertions(+) create mode 100644 openspec/changes/access-follows-membership/design.md create mode 100644 openspec/changes/access-follows-membership/proposal.md create mode 100644 openspec/changes/access-follows-membership/specs/rbac-scopes/spec.md create mode 100644 openspec/changes/access-follows-membership/tasks.md diff --git a/openspec/changes/access-follows-membership/design.md b/openspec/changes/access-follows-membership/design.md new file mode 100644 index 0000000000..78173d6181 --- /dev/null +++ b/openspec/changes/access-follows-membership/design.md @@ -0,0 +1,137 @@ +# Design: access-follows-membership + +Read at openregister development d9a2f9df6b. + +## Context + +- Authorization rules are lists per action of group ids and conditional + rules `{ group | user, match }`. `match` maps an object property to a + literal, a dynamic token (`$userId`, `$user.`, `$organisation`, + `$now`) or an operator object (`$eq`, `$ne`, `$gt`, `$gte`, `$lt`, `$lte`, + `$in`, `$nin`, `$contains`, `$exists`). +- The match is interpreted in three places, and all three call one token + resolver, `ConditionMatcher::resolveDynamicValue()`: + - the QueryBuilder list path, `MagicRbacHandler::applyRbacFilters()` → + `buildPropertyCondition()` (used by `MagicSearchHandler::searchObjects()` + for both the page and `_count`); + - the raw-SQL list path, `MagicRbacHandler::buildRbacConditionsSql()` → + `buildPropertyConditionSql()` (UNION searches, related-row access, the + row predicate); + - the PHP verdict, `ConditionMatcher::objectMatchesConditions()`, used by + `PermissionHandler::hasGroupPermission()`, `MagicRbacHandler::hasPermission()`, + deny rules and `PropertyRbacHandler`. +- `$in` with an empty list is the impossible predicate on both SQL paths and + `false` in `OperatorEvaluator`. +- A precedent reads another schema to decide authorization: + `DepartmentMatrixPersonValues` reads the caller's person object with + `_rbac: false, _multitenancy: false`, caches per user and source for the + request, and re-checks the user match in PHP (`collect()`). + +## D-1: one operator, resolved where the tokens are resolved + +`$memberOf` is an operator inside a property's operator object. +`ConditionMatcher::resolveDynamicValue()` replaces it with +`$in: `. Nothing downstream learns a new word: both SQL +emitters and `OperatorEvaluator` already handle `$in`, including the empty +list. This is the ADR-011 shape: a new operator is added to the shared +resolver, not to each interpreter, so list and find cannot drift. + +If the same operator object also carries `$in`, the two lists are +intersected. Other operators beside it (`$ne`, `$exists`) still apply, AND. + +## D-2: a materialised set, not a correlated subquery + +The set is read in PHP once and sent as an `IN (...)` list. A subquery +(`t.administration_id IN (SELECT m.administration_id FROM m +WHERE m.user_id = ...)`) was considered and rejected: + +- the PHP verdict needs the set anyway, so a subquery would be a second + implementation of the same rule, which is the list-versus-find divergence + ADR-011 exists to prevent; +- the membership table's name, its column names, its soft-delete and + validity semantics would have to be re-expressed in SQL on PostgreSQL and + MariaDB, inside a predicate that is also used in UNION searches across + tables; +- the set is small in every known use: a bookkeeper belongs to a handful of + administrations. + +Both ways put the predicate in the `WHERE` clause, so `LIMIT`/`OFFSET` and +`COUNT(*)` are computed over the admitted rows only. The count is exact. + +The read is capped at 500 membership rows per caller and operand. Past the +cap the set is truncated and a warning is logged. Truncation can only hide +rows, never admit them. + +## D-3: the membership read bypasses the membership schema's own rules + +The set is read with `_rbac: false, _multitenancy: false`. A bookkeeper who +may not list `AdministrationMembership` still has memberships, and the +authorization block that names the schema is written by the register author, +who already decides who reads what. Reading with the caller's rights would +make the rule depend on a second rule and, for a schema whose rules use +`$memberOf` on itself, recurse. + +The user match is checked again in PHP on every row, as +`DepartmentMatrixPersonValues::collect()` does: a filter the search layer did +not understand comes back as no filter, and every member's administration +would become the caller's. + +## D-4: the operand + +| key | required | meaning | +|---|---|---| +| `schema` | yes | slug or id of the membership schema | +| `register` | no | slug or id of its register; default: the first register holding the schema | +| `property` | yes | membership field whose values form the set (scalar or array) | +| `match` | no, default `userId` | membership field holding the caller's Nextcloud uid | +| `where` | no | static filters on membership fields: a literal (strict equality) or an operator object, evaluated by `OperatorEvaluator` | +| `current` | no | `{ "from": , "until": }`: a row counts when `from` is empty or on or before today and `until` is empty or on or after today, compared on the `Y-m-d` date | + +`match` and `property` reuse the names of the department matrix's +`userSource`, which reads the same kind of record. + +## D-5: everything that goes wrong denies + +| situation | set | outcome | +|---|---|---| +| no caller (anonymous) | `[]` | `$in: []`, no row | +| operand not an object, `schema` or `property` missing, `where` or `current` malformed | unresolvable | the property resolves to `null`, which every interpreter already treats as deny (`1 = 0` in SQL) | +| schema or register not found, read throws | `[]`, warning logged | no row | +| caller has no membership rows | `[]` | no row | + +An admin is unaffected: admins bypass RBAC before any match is read. + +## D-6: per-request cache + +`MemberOfResolver` keeps `uid + canonical JSON of the operand → set` for the +life of the service, which Nextcloud's container gives one request. The key +carries the uid because `ObjectService::runAs()` can change the subject inside +a request. A long-running process (a background job) shares one instance +across its work, so a membership written during that run is seen on the next +run; that is the same staleness the department matrix accepts. + +## D-7: interaction with multitenancy + +A conditional rule whose match names a field other than `_organisation` +lifts the organisation filter for the callers it admits +(`MagicRbacHandler::hasConditionalRulesBypassingMultitenancy()`). A +`$memberOf` rule does the same: the membership is the tenancy. A schema that +wants both adds `"_organisation": "$organisation"` beside it in the same +match. + +## Declarative-vs-imperative decision + +Declarative. The rule lives in the schema's authorization block (ADR-031); +the only code is the engine's resolver, in OpenRegister (ADR-023 Rule 1). +shillinq's `AdministrationContextService` keeps its job of choosing the +active administration for the UI; it stops being the place that decides +which rows a bookkeeper may read. + +## Risks + +- A typo in the operand denies silently to the user. Mitigated by the + warning in the log; a save-time validator for match operands is a separate + change, since none exists for the other operators either. +- Slug ambiguity: a schema slug that exists in two registers. Name the + `register`, or use ids. +- A very large membership set makes a long `IN` list. The cap bounds it. diff --git a/openspec/changes/access-follows-membership/proposal.md b/openspec/changes/access-follows-membership/proposal.md new file mode 100644 index 0000000000..423f3a3b7d --- /dev/null +++ b/openspec/changes/access-follows-membership/proposal.md @@ -0,0 +1,91 @@ +--- +kind: code +depends_on: [] +--- + +# Proposal: access-follows-membership + +## Summary + +A bookkeeper in shillinq may read the rows of the administrations they are a +member of, and no others. The membership is a record: an +`AdministrationMembership` row that says user `anna` has role `boekhouder` in +administration `adm-1` from 1 January. OpenRegister's authorization rules can +compare an object field with the caller's user id, groups, organisation or +the current time, but not with values held in the caller's rows of another +schema. So the isolation cannot be declared today, and shillinq enforces it +in its own service layer, which ADR-023 Rule 1 forbids and which list queries +through the plain object API do not pass through at all. + +This change adds one match operator, `$memberOf`. It admits an object when +the value of one of its fields is in the set of values that the caller's rows +in a membership schema hold. The same need exists wherever an app has a +tenant-like scope that is not a Nextcloud organisation: a case team, a +school, a project, a board. + +## What changes + +- A `match` property MAY take `{ "$memberOf": { ... } }`. The operand names + the membership schema (and optionally its register), the membership field + whose values form the set, the membership field that holds the caller's + user id, optional static filters (for a role) and an optional validity + window. +- The operator is resolved by the one token resolver the whole RBAC stack + shares (`ConditionMatcher::resolveDynamicValue()`), into `{ "$in": [ ... ] }`. + So the list query, the count, the single-object verdict, property-level + rules and deny rules all read the same set, and the list predicate sits in + the SQL `WHERE`, where pagination and counts are computed. +- The caller's set is read once per request per caller and per operand. +- Every failure denies: no caller, a malformed operand, a membership schema + that cannot be found or read. Nothing is admitted by an error. + +## Example + +```json +"authorization": { + "read": [ + { + "group": "shillinq_bookkeeper", + "match": { + "administrationId": { + "$memberOf": { + "schema": "administration-membership", + "register": "shillinq", + "property": "administrationId", + "match": "userId", + "where": { "role": { "$in": ["boekhouder", "controller"] } }, + "current": { "from": "validFrom", "until": "validUntil" } + } + } + } + } + ] +} +``` + +## Out of scope + +- `$notMemberOf`. Nobody has asked for exclusion by membership. +- Dynamic tokens inside `where`. The filters are literals; the caller is + bound by `match`. +- A membership that itself follows a membership (nested `$memberOf`). +- A management UI for the operator. Schemas declare it in their register + JSON, as every other match rule. + +## Impact + +- New `lib/Service/Rbac/MemberOfResolver.php`. +- `lib/Service/ConditionMatcher.php` resolves `$memberOf`. +- No change to `MagicRbacHandler` or `PermissionHandler`: both already + delegate token resolution to `ConditionMatcher`. +- No migration. Existing rules are unchanged. +- Consumers: shillinq (administration isolation). Other apps with + tenant-like scopes can adopt it without code. + +## ADR fit + +- ADR-023 Rule 1: data RBAC is OpenRegister's job, and a gap is fixed in + OpenRegister. This is that fix; shillinq can drop its own guard. +- ADR-031: the behaviour is declared in the schema's authorization block, not + written as an app service. +- ADR-011 (OpenRegister): one resolver, so list and find cannot disagree. diff --git a/openspec/changes/access-follows-membership/specs/rbac-scopes/spec.md b/openspec/changes/access-follows-membership/specs/rbac-scopes/spec.md new file mode 100644 index 0000000000..59c98b9a1f --- /dev/null +++ b/openspec/changes/access-follows-membership/specs/rbac-scopes/spec.md @@ -0,0 +1,62 @@ +# rbac-scopes + +## ADDED Requirements + +### Requirement: A match can admit by the caller's membership records + +A `match` property MAY take an operator object `{ "$memberOf": }`. +The operand SHALL name a membership schema (`schema`, required), optionally +its register (`register`), the membership field whose values form the set +(`property`, required), the membership field that holds the caller's user id +(`match`, default `userId`), optional static filters (`where`) and an +optional validity window (`current: { from, until }`). + +OpenRegister SHALL admit an object for that rule only when the object's value +for the property is in the set of `property` values held by the caller's +membership rows that pass `where` and `current`. The set SHALL be resolved by +`ConditionMatcher::resolveDynamicValue()` into `$in`, so the list query, its +count, the single-object verdict, property-level rules and deny rules read +the same set. The membership rows SHALL be read without the membership +schema's own RBAC and multitenancy, and the user match SHALL be re-checked on +every row read. + +#### Scenario: a bookkeeper lists only the rows of their administrations + +- **GIVEN** a schema `boeking` with `read: [{ "group": "shillinq_bookkeeper", "match": { "administrationId": { "$memberOf": { "schema": "administration-membership", "property": "administrationId" } } } }]` +- **AND** user `anna` in group `shillinq_bookkeeper` has membership rows for `adm-1` and `adm-2` +- **AND** the `boeking` table holds rows of `adm-1`, `adm-2` and `adm-3` +- **WHEN** `anna` lists `boeking` with `_limit` and `_page` +- **THEN** every returned row has `administrationId` `adm-1` or `adm-2` +- **AND** the total count equals the number of `adm-1` and `adm-2` rows +- **AND** the pages together hold exactly those rows +- @e2e exclude {engine primitive; proven against SQLite in MagicRbacMemberOfListTest, task 2.2} + +#### Scenario: a single read of another administration's row is refused + +- **GIVEN** the same schema and memberships +- **WHEN** `anna` reads a `boeking` row of `adm-3` +- **THEN** the per-object check refuses it, as the list excludes it +- @e2e exclude {engine primitive; covered by ConditionMatcherMemberOfTest, task 2.1} + +#### Scenario: the role filter and validity window narrow the set + +- **GIVEN** the operand has `where: { "role": { "$in": ["boekhouder"] } }` and `current: { "from": "validFrom", "until": "validUntil" }` +- **AND** `anna` is `inkijker` in `adm-1`, `boekhouder` in `adm-2` until yesterday, and `boekhouder` in `adm-4` with no end date +- **WHEN** the set is resolved for `anna` +- **THEN** it is `["adm-4"]` +- @e2e exclude {engine primitive; covered by MemberOfResolverTest, task 1.1} + +#### Scenario: whatever cannot be resolved denies + +- **GIVEN** a `$memberOf` operand without `schema`, or naming a schema that does not exist, or a caller who is not signed in +- **WHEN** the rule is evaluated on a list or a single object +- **THEN** the rule admits no object +- **AND** the list query carries an impossible predicate rather than dropping the condition +- @e2e exclude {engine primitive; covered by MemberOfResolverTest and MagicRbacMemberOfListTest} + +#### Scenario: the set is read once per request + +- **GIVEN** a list of 50 rows evaluated under a `$memberOf` rule +- **WHEN** the list and the per-object checks of the same request run +- **THEN** the membership rows are read once for that caller and operand +- @e2e exclude {engine primitive; covered by MemberOfResolverTest, task 1.1} diff --git a/openspec/changes/access-follows-membership/tasks.md b/openspec/changes/access-follows-membership/tasks.md new file mode 100644 index 0000000000..aa4045f946 --- /dev/null +++ b/openspec/changes/access-follows-membership/tasks.md @@ -0,0 +1,18 @@ +# Tasks: access-follows-membership + +## 1. The membership set + +- [ ] 1.1 `MemberOfResolver::valuesFor(mixed $operand, ?string $userId): ?array` per design D-2 to D-6. Verify: `tests/Unit/Service/Rbac/MemberOfResolverTest.php` covers the caller's values, another user's rows ignored even when the search returns them, the `where` role filter, the `current` window (future, past, open-ended), array-valued `property`, an anonymous caller, each malformed operand, a schema that cannot be found, and that a second call with the same caller and operand does not read again while another caller does. + +## 2. The operator + +- [ ] 2.1 `ConditionMatcher::resolveDynamicValue()` turns `$memberOf` into `$in`, intersects with a sibling `$in`, and resolves the whole property to `null` when the operand is unresolvable. Verify: `tests/Unit/Service/ConditionMatcherMemberOfTest.php` for the single-object verdict: a member's administration is admitted, another administration is refused, an unresolvable operand refuses. +- [ ] 2.2 List query proof. Verify: `tests/Unit/Db/MagicMapper/MagicRbacMemberOfListTest.php` runs the raw-SQL predicate a list builds (`MagicSearchHandler::buildWhereConditionsSql()` over the real `MagicRbacHandler` and `ConditionMatcher`) against an SQLite table holding rows of three administrations: the caller sees only their two administrations' rows, `COUNT(*)` equals that number, the pages under `LIMIT`/`OFFSET` add up to it, and a caller with no membership gets zero. The QueryBuilder path (`applyRbacFilters()`) is asserted to emit an `in` over the same set. + +## 3. Docs + +- [ ] 3.1 Document `$memberOf` beside the other match operators in `docs/`. + +Acceptance: +- `composer check:strict` green on the last branch. +- The single-object verdict and the list agree for the same caller and rows (2.1 and 2.2 use the same fixtures).