fix: unify RBAC condition matching onto ConditionMatcher (#1336)

[rjzondervan]

Summary

• Collapses PermissionHandler::evaluateMatchConditions and MagicRbacHandler's private PHP-side matcher onto the shared ConditionMatcher service, so schema-level RBAC now honours the full operator set ($eq/$ne/$gt/$gte/$lt/$lte/$in/$nin/$exists) and dynamic variables ($organisation/$userId/$now) that the SQL row-level filter and property-level RBAC already supported.
• Fixes three list↔find parity bugs surfaced by the originating OpenCatalogi PublicationsController::attachments report.
• Closes the parity gap the rbac-scopes spec already required but the schema-level implementation was not delivering.

Refs #1336.

What changed

1. Schema-level matcher unified onto ConditionMatcher

• PermissionHandler::evaluateMatchConditions() deleted; hasGroupPermission() now delegates via an @self.organisation envelope fold. hasPermission() no longer fetches activeOrganisation separately — ConditionMatcher resolves $organisation itself.
• MagicRbacHandler::hasPermission() rewritten to delegate; 7 private PHP-side helpers removed (objectMatchesConditions, objectPropertyMatchesCondition, valueMatchesOperator, singleOperatorMatches, comparisonOperatorMatches, arrayOperatorMatches, existsOperatorMatches). SQL emitter path (applyRbacFilters, buildRbacConditionsSql, buildMatchConditions, buildPropertyCondition, ...) untouched.
• MagicMapper::initializeHandlers resolves ConditionMatcher from the container when constructing MagicRbacHandler.

2. Null-valued properties follow SQL three-valued logic

OperatorEvaluator used raw PHP <=/>=/</> without null guards. null <= "<datetime>" in PHP returned true (coerces null to "", which is lex-less than any non-empty string), while SQL NULL <= X correctly returned NULL → filtered out. Now:

Operator	Behaviour when value or operand is null
$gt / $gte / $lt / $lte	returns false if either side is null
$in	false if value is null
$nin	false if value is null (was true)
$ne with non-null operand	false if value is null (was true)
$eq: null	unchanged — backward-compat "match missing field" escape hatch
$exists	unchanged — the canonical null-aware operator

3. $now aligned to SQL-native format

ConditionMatcher::resolveDynamicValue now emits Y-m-d H:i:s (was 'c' = 2026-04-24T14:43:49+00:00). For text/JSON columns storing dates, the raw lex comparison diverged at the T-vs-space position, causing list/find drift. Aligned to MagicRbacHandler's format, which also matches DateTimeNormalizer's input format.

Out of scope (follow-up work)

• Schema::hasPermission() / Schema::evaluateMatchConditions() in lib/Db/Schema.php is a fourth duplicate of the same equality-only matcher. It has test coverage in BasicCrudTest.php but zero production callers — logged as §3.7 in tasks.md for a separate cleanup.
• OpenCatalogi PublicationsController::show _rbac:false, double-find in ::attachments, FileService::getFiles RBAC integration, and the "User 'Anonymous' does not have permission" exception wording are consumer-side concerns — will be handled in separate PRs against opencatalogi.

Test plan

• Unit — PermissionHandlerRbacTest 24 tests including 14 new ones (10 delegation tests with mocked ConditionMatcher + 4 real-wired scenarios using real ConditionMatcher + OperatorEvaluator).
• Unit — new MagicRbacHandlerTest with 14 tests covering admin/owner bypass, simple rules, conditional rules, $in, $userId, and delegation shape.
• Unit — OperatorEvaluatorTest +11 null-handling tests (6 started as failing, all 39 now pass).
• Unit — ConditionMatcherTest unchanged, 21 tests still passing.
• Integration — tests/Service/ObjectHandlersIntegrationTest.php had 5 tests calling the removed evaluateMatchConditions; removed and replaced with a pointer comment.
• Full RBAC suite: 102 tests / 148 assertions, all green.
• composer phpcs — clean (auto-fixed 8, 2 manual docblock shortenings).
• composer phpmd — clean for changed files (added @SuppressWarnings(PHPMD.NPathComplexity) on MagicRbacHandler::hasPermission — inlined rule dispatch intentionally).
• composer psalm — clean for changed files (48 unrelated pre-existing DeletionAnalysis errors remain).
• composer phpstan — clean for changed files.
• Manual smoke (needs dev env) — OpenCatalogi PublicationsController::attachments with a publication schema using { "read": [{ "group": "public", "match": { "publishedAt": { "$lte": "$now" } } }] }: past-dated publication with attachment returns 200; future-dated returns 404 (not 500 with "User 'Anonymous'"); publishedAt: null returns 404.
• Manual smoke — composite publicatiedatum + depublicatiedatum rule (covered by testCompositePublishedAndNotDepublishedRule as an end-to-end PHP test, but worth running against a real database for list-find parity).
• Manual smoke — OpenRegister UI (localhost:3030) with a logged-in user querying a schema using $userId match: results user-scoped in both list and detail views.

🤖 Generated with Claude Code

[bbrands02]

Critical issue:

• OperatorEvaluator still treats an unknown operator as allowed by returning true. That means a malformed rule like ['publishedAt' => ['$foo' => 'bar']] can pass PHP-side RBAC. The SQL path does not appear to behave the same way: unknown operators return null and can result in no condition / deny behavior. So the PR still leaves a list-vs-find drift and it is fail-open on the PHP side

[github-actions]

Quality Report — ConductionNL/openregister @ 60cd59e

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 599/599
PHPUnit					❌
Newman					❌
Playwright					⏭️

---

Quality workflow — 2026-04-24 09:22 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

Two merge blockers (resolved-relation unwrapping regression and incomplete $in null guard) need to be addressed. The consolidation direction is correct, the SQL emitter is verified untouched, and the admin/owner short-circuits and envelope fold all check out. Fix the two blockers and this is ready to ship.

[rjzondervan]

Review response

Thanks for the sharp review — addressed in 6c958a3c1.

🔴 Blockers — fixed

1. Resolved-relation unwrapping (Wilco)
Added ConditionMatcher::unwrapResolvedRelation(), called from singleConditionMatches before any comparison branch. Arrays with an id key are reduced to the id (mirroring the deleted evaluateMatchConditions fallback); arrays without an id key pass through unchanged. Extracted as a helper to keep singleConditionMatches under PHPMD's NPath/cyclomatic thresholds.

2. $in null-handling (Wilco)
operatorIn now has an explicit if ($value === null) return false guard. Previously it happened to work for the common case (in_array(null, ['a', 'b'], true) is false), but in_array(null, [null, 'x'], true) returns true — diverging from SQL NULL IN (NULL, 'x') → NULL → filter out. Added a regression test (testInAgainstNullValueWithNullInOperandArrayStillReturnsFalse).

3. Unknown operator fail-closed (bbrands02)
OperatorEvaluator::applySingleOperator's default switch arm now returns false and logs a warning. The SQL path already dropped unknown-operator conditions and denied; PHP was fail-open. Flipped the existing testUnknownOperatorReturnsTrueAndLogsWarning test to the new contract.

🟡 Non-blocking concerns — addressed

• $ne: null SQL divergence documented. New requirement in rbac-scopes/spec.md: "Divergences from strict SQL three-valued logic (documented)" with explicit scenarios for both $eq: null (escape hatch) and $ne: null (PHP-sensible, SQL-denies-all). The parity goal is no longer oversold.
• Schema::hasPermission + ::evaluateMatchConditions got @deprecated tags pointing to PermissionHandler::hasPermission / MagicRbacHandler::applyRbacFilters with a clear "scheduled for follow-up cleanup" note. The §3.7 follow-up is now visible in-file.
• Real-wired integration tests added. Your concern 3 was tightly coupled to blocker 1 — proved by the fact that the new testResolvedRelationUnwrappingViaRealConditionMatcher (real ConditionMatcher + OperatorEvaluator, no mocks) would have caught the regression. Also added testUnknownOperatorFailsClosedViaRealConditionMatcher for bbrands02's case.
• Minor: envelope + precedence — comment added in PermissionHandler::hasGroupPermission explaining that existing @self.organisation wins over the separately-passed $objectOrganisation, matching pre-unification semantics.
• Minor: MagicRbacHandler docblock — tightened to distinguish local string-rule dispatch (intentionally kept in-class, needs no operator vocabulary) from the delegated match: branch. The "do not reintroduce" wording no longer oversells.

Not touched (pre-existing, flagged by reviewer as follow-up)

• SQL emitter at MagicRbacHandler::buildComparisonOperatorConditionSql emits col = NULL for $eq: null. Confirmed pre-existing, mirror-image of the drift this PR is fixing. Should be a separate issue — happy to open one, just let me know.

CHANGELOG

Three new bullets under Unreleased/Fixed covering the substantive code fixes (unwrap, unknown-operator, $in null). Aligns user-facing notes with what ships.

Quality

All RBAC tests green: 108 tests / 155 assertions. composer phpcs / phpmd clean for touched files. (PHPUnit and Newman CI failures are pre-existing and being addressed in a separate stream — not from this PR.)

Ready for another look. 🙏

[github-actions]

Quality Report — ConductionNL/openregister @ 60ae6e6

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	✅
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		✅
stylelint		✅
composer			✅	✅ 147/147
npm			✅	✅ 599/599
PHPUnit					❌
Newman					❌
Playwright					⏭️

---

Quality workflow — 2026-04-24 10:06 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

Both blockers addressed — unwrapResolvedRelation() in ConditionMatcher and the operatorIn null guard are in and tested. The $ne: null SQL divergence is documented in the spec rather than code-fixed, which is the right call given $exists is the canonical operator. @deprecated annotations added to Schema::hasPermission/evaluateMatchConditions, the + union precedence comment is in place, and the MagicRbacHandler docblock is accurate. 108 tests / 155 assertions green. Ready to merge.