docs(openspec): access follows membership, a $memberOf match operator

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.<prop>`, `$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: <the caller's set>`. 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 <membership table> 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": <field>, "until": <field> }`: 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.

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.

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": <operand> }`.
+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}

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).