diff --git a/lib/AppInfo/Application.php b/lib/AppInfo/Application.php index 005e590330..e7a16d9fcd 100644 --- a/lib/AppInfo/Application.php +++ b/lib/AppInfo/Application.php @@ -729,6 +729,12 @@ function () { // driver-level 500 an unresolvable column name used to produce. $context->registerMiddleware(\OCA\OpenRegister\Middleware\UnknownMetadataFieldMiddleware::class); + // Register the RelatedRowFilterMiddleware: maps + // RelatedRowFilterNotSupportedException — a `_related`/`_referenced` + // filter on a multi-schema facet route that cannot honour it — onto an + // HTTP 400 with the exception's message, not an uncaught 500. + $context->registerMiddleware(\OCA\OpenRegister\Middleware\RelatedRowFilterMiddleware::class); + // Register the MaintenanceModeMiddleware (admin-operations-console // D-6): while maintenance mode holds, every controller but the // operations console is refused with the administered message, so the diff --git a/lib/Controller/FlowRunController.php b/lib/Controller/FlowRunController.php index cdee5f46cf..fe07254012 100644 --- a/lib/Controller/FlowRunController.php +++ b/lib/Controller/FlowRunController.php @@ -29,6 +29,7 @@ namespace OCA\OpenRegister\Controller; use DateTime; +use DateTimeZone; use OCA\OpenRegister\Db\AuditFlowAttribution; use OCA\OpenRegister\Db\FlowRun; use OCA\OpenRegister\Db\FlowRunMapper; @@ -254,7 +255,7 @@ public function summary(): JSONResponse { } if ($since === null) { - $since = new DateTime('-7 days'); + $since = (new DateTime('-7 days'))->setTimezone(new DateTimeZone('UTC')); } $flowIds = array_slice($this->flowIdsParam(), 0, 200); @@ -329,7 +330,11 @@ private function sinceParam(): DateTime|false|null { } try { - return new DateTime(trim($raw)); + // 🔴 NORMALISED TO UTC BEFORE IT IS BOUND. The mapper binds the moment + // with PARAM_DATETIME_MUTABLE, which formats the object's own wall + // clock without converting, and the `created` column is UTC: + // `12:00:00+02:00` was compared as 12:00:00, two hours off the instant. + return (new DateTime(trim($raw)))->setTimezone(new DateTimeZone('UTC')); } catch (Throwable) { return false; } diff --git a/lib/Controller/ViewsController.php b/lib/Controller/ViewsController.php index 21e1af8fdf..dfbdc6cb69 100644 --- a/lib/Controller/ViewsController.php +++ b/lib/Controller/ViewsController.php @@ -216,9 +216,8 @@ private function refusalFor(InvalidArgumentException $e): JSONResponse { /** * Refuse an update that changes fields this caller does not own. * - * Answers a response to RETURN, or null when the update may proceed. The - * refusal NAMES the fields, because the message a member needs is which - * field was refused rather than that something was. + * Answers a response to RETURN, or null when the update may proceed. The refusal + * NAMES the fields, because a member needs to know which field was refused. * * Only fields whose VALUE changes are judged: the edit screen sends the * whole view. The caller resolves the view with requireReachableView(). @@ -226,6 +225,7 @@ private function refusalFor(InvalidArgumentException $e): JSONResponse { * @param View $view The stored view. * @param string $userId The caller. * @param array $data The request body. + * @param array|null $savedAs The body as the save will write it, when that differs (PUT writes `isPublic` false when omitted). * * @return JSONResponse|null The refusal, or null when allowed. * @@ -234,7 +234,7 @@ private function refusalFor(InvalidArgumentException $e): JSONResponse { * @spec openspec/specs/saved-search-views/spec.md * @spec openspec/changes/view-alert-recipients/specs/saved-search-views/spec.md */ - private function refuseForbiddenViewFields(View $view, string $userId, array $data): ?JSONResponse { + private function refuseForbiddenViewFields(View $view, string $userId, array $data, ?array $savedAs=null): ?JSONResponse { // Only keys that name a view property are judged, so a `_limit` or // routing key on the body cannot refuse an update a member may make. $fields = array_intersect_key( @@ -261,7 +261,7 @@ private function refuseForbiddenViewFields(View $view, string $userId, array $da ); if ($refused === []) { - $this->viewers->assertMayNameRecipients(view: $view->jsonSerialize(), reach: $this->viewers->reachOf(userId: $userId), data: $data); + $this->viewers->assertMayNameRecipients(view: $view->jsonSerialize(), reach: $this->viewers->reachOf(userId: $userId), data: ($savedAs ?? $data)); return null; } @@ -592,7 +592,7 @@ public function update(string $id): JSONResponse { // an administrator are unaffected: `mayAdminister()` answers true // for both and the guard returns null. $stored = $this->requireReachableView(id: $id, userId: $userId); - $refusal = $this->refuseForbiddenViewFields(view: $stored, userId: $userId, data: $data); + $refusal = $this->refuseForbiddenViewFields(view: $stored, userId: $userId, data: $data, savedAs: ($data + ['isPublic' => false])); if ($refusal !== null) { return $refusal; } diff --git a/lib/Db/MagicMapper.php b/lib/Db/MagicMapper.php index 56eeb8b8fe..41f8a85c82 100644 --- a/lib/Db/MagicMapper.php +++ b/lib/Db/MagicMapper.php @@ -69,6 +69,8 @@ use OCA\OpenRegister\Service\FieldEncryptionHandler; use OCA\OpenRegister\Service\Object\DraftStatusPolicy; use OCA\OpenRegister\Service\Object\ObjectEventGate; +use OCA\OpenRegister\Service\Query\PropertyReadGuard; +use OCA\OpenRegister\Service\Query\RelatedRowAccess; use OCA\OpenRegister\Service\Query\RelatedRowQueryApplier; use OCA\OpenRegister\Service\SettingsService; use OCA\OpenRegister\Support\QueryLimit; @@ -611,12 +613,21 @@ private function initializeHandlers(): void { // Assembled by hand rather than resolved from the container, for the // same reason the cache handler above is not: the container would walk // MagicMapper → applier → MagicTableHandler → MagicMapper and recurse. + // The applier also answers the property read question for the search + // handler's nested-path filters: a filter that reaches through a + // reference and one on a path inside an object property are the same + // oracle on a field the caller may not read. + $relatedRowAccess = new RelatedRowAccess( + rbac: $this->rbacHandler, + logger: $this->logger, + guard: new PropertyReadGuard(container: $this->container, logger: $this->logger) + ); $relatedRowApplier = new RelatedRowQueryApplier( schemaMapper: $this->schemaMapper, tableHandler: $this->tableHandler, - rbacHandler: $this->rbacHandler, db: $this->db, - logger: $this->logger + logger: $this->logger, + access: $relatedRowAccess ); $this->searchHandler = new MagicSearchHandler( @@ -1676,6 +1687,7 @@ private function countUnionBatch(array $query, array $registerSchemaPairs): int * UNION ALL is faster but has limitations: * - All tables must exist * - No complex aggregations + * - No related-row filters (`_related`, `_referenced`) * - Simpler query structure * * @param array $query Search query parameters. @@ -1690,6 +1702,14 @@ private function shouldUseUnionQuery(array $query): bool { return false; } + // 🔴 `_related` and `_referenced` are EXISTS subqueries that only the + // QueryBuilder path (the sequential one) builds. The raw-SQL UNION builder + // refuses them, which on this route would end in an uncaught exception + // (HTTP 500), so a query carrying either takes the sequential path. + if (array_key_exists('_related', $query) === true || array_key_exists('_referenced', $query) === true) { + return false; + } + // UNION ALL is safe for simple searches. return true; }//end shouldUseUnionQuery() @@ -7704,6 +7724,9 @@ static function (array $first, array $second): int { * * @return list The folder file ids. * + * @throws RuntimeException When a table could not be read: a sum over the tables that could be read would + * be too low and look measured, so the whole measurement fails (and is logged). + * * @spec openspec/changes/organisation-usage-measured/specs/organisation-usage-measured/spec.md */ public function findFolderIdsByOrganisation(string $organisationUuid): array { @@ -7711,6 +7734,7 @@ public function findFolderIdsByOrganisation(string $organisationUuid): array { $folderCol = 't.' . self::METADATA_PREFIX . 'folder'; $deletedCol = 't.' . self::METADATA_PREFIX . 'deleted'; $folderIds = []; + $failed = []; foreach (array_keys($this->discoverMagicTables()) as $fullTableName) { $bareTableName = substr($fullTableName, strlen($this->getTablePrefix())); @@ -7730,10 +7754,32 @@ public function findFolderIdsByOrganisation(string $organisationUuid): array { $result->closeCursor(); } catch (\Exception $e) { - continue; - } + // 🔴 NEVER SKIPPED IN SILENCE. A table that fails (a missing + // _folder/_organisation column, a lock, a timeout) drops its folders + // from the sum, and the caller would show a measured-looking number + // that is too low. The service contract is "a figure that cannot be + // measured is null, never 0", so this fails after the scan, naming + // every table it could not read. + $this->logger->warning( + message: '[MagicMapper] findFolderIdsByOrganisation: could not read a table', + context: [ + 'file' => __FILE__, + 'line' => __LINE__, + 'table' => $fullTableName, + 'organisation' => $organisationUuid, + 'error' => $e->getMessage(), + ] + ); + $failed[] = $fullTableName; + }//end try }//end foreach + if ($failed !== []) { + throw new RuntimeException( + 'The folders of organisation ' . $organisationUuid . ' could not be read from: ' . implode(', ', $failed) + ); + } + return array_keys($folderIds); }//end findFolderIdsByOrganisation() diff --git a/lib/Db/MagicMapper/MagicFacetHandler.php b/lib/Db/MagicMapper/MagicFacetHandler.php index eede34e3a1..aa42605236 100644 --- a/lib/Db/MagicMapper/MagicFacetHandler.php +++ b/lib/Db/MagicMapper/MagicFacetHandler.php @@ -433,6 +433,9 @@ function ($key) { * @param array $query The search query with filters and facet config. * * @return array Merged facet results across all tables. + * + * @throws \OCA\OpenRegister\Exception\RelatedRowFilterNotSupportedException When the query carries `_related` or `_referenced`. + * * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value */ public function getSimpleFacetsUnion(array $tableConfigs, array $query): array { @@ -442,6 +445,11 @@ public function getSimpleFacetsUnion(array $tableConfigs, array $query): array { return []; } + // A `_related`/`_referenced` filter cannot be honoured in these raw-SQL + // arms. Refuse HERE, before the per-facet queries, whose own catch blocks + // would turn the refusal into empty buckets with only a log line. + $this->searchHandler?->refuseRelatedRowFilters(query: $query); + // Extract facet configuration. $facetConfig = $query['_facets'] ?? []; if (empty($facetConfig) === true) { diff --git a/lib/Db/MagicMapper/MagicRbacHandler.php b/lib/Db/MagicMapper/MagicRbacHandler.php index 175e8b2e54..e016a575d3 100644 --- a/lib/Db/MagicMapper/MagicRbacHandler.php +++ b/lib/Db/MagicMapper/MagicRbacHandler.php @@ -1672,7 +1672,7 @@ private function deniesHere( * @spec openspec/changes/query-related-schema-rows/specs/zoeken-filteren/spec.md */ public function buildRbacPredicateForAlias(Schema $schema, string $alias, string $action = 'read', ?string $tableName = null): string { - if (preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $alias) !== 1) { + if (preg_match('/^[A-Za-z_][A-Za-z0-9_]*\z/', $alias) !== 1) { throw new InvalidArgumentException( sprintf('\'%s\' is not a table alias an access predicate may be built for.', $alias) ); diff --git a/lib/Db/MagicMapper/MagicSearchHandler.php b/lib/Db/MagicMapper/MagicSearchHandler.php index 6287594d93..f1dae6e060 100644 --- a/lib/Db/MagicMapper/MagicSearchHandler.php +++ b/lib/Db/MagicMapper/MagicSearchHandler.php @@ -51,6 +51,7 @@ use OCA\OpenRegister\Db\WatcherMapper; use OCA\OpenRegister\Service\Query\RelatedRowQueryApplier; use OCA\OpenRegister\Exception\EncryptedFieldFilterException; +use OCA\OpenRegister\Exception\RelatedRowFilterNotSupportedException; use OCA\OpenRegister\Exception\UnknownMetadataFieldException; use OCA\OpenRegister\Service\DateTimeNormalizer; use OCA\OpenRegister\Service\LanguageService; @@ -614,9 +615,100 @@ private function applyRelatedRowFilters(IQueryBuilder $qb, array $query, ?int $r $register = new Register(); $register->setId($registerId); - $this->relatedRows->apply(qb: $qb, query: $query, register: $register, outerAlias: 't'); + $this->relatedRows->apply( + qb: $qb, + query: $query, + register: $register, + organisationScopeFor: fn (Schema $related, ?int $relatedRegisterId): array => $this->organisationScopeForRelated( + query: $query, + related: $related, + registerId: $relatedRegisterId + ) + ); }//end applyRelatedRowFilters() + /** + * Refuse a query carrying `_related` or `_referenced` where it cannot be honoured. + * + * 🔴 REFUSE, DO NOT DROP. The related-row filters are written for the + * QueryBuilder path (applyRelatedRowFilters). Underscore keys are skipped by + * the raw-SQL builder, so a cross-table UNION search or UNION facet carrying + * one answered the UNFILTERED set to a deliberately narrow question, and its + * counts disagreed with the single-schema facet for the same query. + * + * @param array $query The request query. + * + * @return void + * + * @throws RelatedRowFilterNotSupportedException When the query carries either key. + * + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md + */ + public function refuseRelatedRowFilters(array $query): void { + foreach (['_related', '_referenced'] as $key) { + if (array_key_exists($key, $query) === true) { + throw new RelatedRowFilterNotSupportedException( + message: sprintf( + '\'%s\' filters are not supported on a query across several schemas; narrow the query to one schema. ' + . 'Answering without the filter would return rows it was meant to exclude.', + $key + ) + ); + } + } + }//end refuseRelatedRowFilters() + + /** + * The organisation scope a related-row subquery must apply to the related table. + * + * The SAME decision the list path takes for a table (`multitenancyApplies()`, + * the cascade widening, `resolveOrganizationScope()`), made for the RELATED + * schema with the request's own flags, so the subquery cannot consult rows + * the list of that table would not return. `SCOPE_ALL` when the boundary does + * not apply (system context, admin bypass, an RBAC rule that waives it). + * + * @param array $query The request query, for `_rbac` and `_multitenancy`. + * @param Schema $related The related schema. + * @param int|null $registerId The register the related table lives in. + * + * @return array{mode: string, uuids: array} The decided scope. + * + * @SuppressWarnings(PHPMD.StaticAccess) admitOrganisationless() is a pure transform of the decided scope. + * + * @spec openspec/specs/rbac-scopes/spec.md + */ + private function organisationScopeForRelated(array $query, Schema $related, ?int $registerId): array { + $_rbac = $this->flagFromQuery(value: ($query['_rbac'] ?? true)); + $multitenancyExplicit = $this->isExplicitlyTrue(value: ($query['_multitenancy_explicit'] ?? false)); + $resolved = $this->resolveMultitenancyFlag( + _multitenancy: $this->flagFromQuery(value: ($query['_multitenancy'] ?? true)), + multitenancyExplicit: $multitenancyExplicit, + schema: $related + ); + + $applies = $this->multitenancyApplies( + schema: $related, + _rbac: $_rbac, + _multitenancy: $resolved, + multitenancyExplicit: $multitenancyExplicit + ); + if ($applies === false) { + return ['mode' => MagicOrganizationHandler::SCOPE_ALL, 'uuids' => []]; + } + + $scope = $this->organizationHandler->resolveOrganizationScope( + adminBypassEnabled: $this->organizationHandler->isAdminOverrideEnabled(), + registerId: $registerId, + schemaId: $related->getId() + ); + + if ($this->organisationlessRowsAdmitted(schema: $related, _rbac: $_rbac, multitenancyExplicit: $multitenancyExplicit) === true) { + $scope = MagicOrganizationHandler::admitOrganisationless(scope: $scope); + } + + return $scope; + }//end organisationScopeForRelated() + /** * Apply metadata, object-field and ID filters to the query. * @@ -784,10 +876,12 @@ private function applyRelationFieldFilters(IQueryBuilder $qb, array $query): voi * @return string[] Array of SQL WHERE conditions (without leading AND/WHERE). * * @throws UnknownMetadataFieldException When a `@self` key names no metadata column. + * @throws RelatedRowFilterNotSupportedException When the query carries `_related` or `_referenced`, which this raw-SQL builder cannot honour. * * @spec openspec/specs/zoeken-filteren/spec.md#requirement-self-metadata-filters-support-comparison-operators * @spec openspec/changes/object-level-sharing-and-private-scope/specs/private-object-scope/spec.md#requirement-the-private-principal-is-honoured-identically-on-every-enforcement-path * @spec openspec/changes/magic-table-metadata-self-healing/specs/object-lifecycle/spec.md#requirement-a-table-that-cannot-be-healed-degrades-quietly-req-mth-002 + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md */ public function buildWhereConditionsSql( array $query, @@ -796,7 +890,8 @@ public function buildWhereConditionsSql( ?int $registerId = null, ?string $tableName = null, ): array { - $conditions = []; + $this->refuseRelatedRowFilters(query: $query); + // Get connection for value quoting through QueryBuilder. $qb = $this->db->getQueryBuilder(); $connection = $qb->getConnection(); @@ -812,7 +907,6 @@ public function buildWhereConditionsSql( $_rbac = $query['_rbac'] ?? true; $conditions = array_merge( - $conditions, $this->lifecycleConditionsSql( query: $query, includeDeleted: $includeDeleted, schema: $schema, missingColumns: $this->missingMetadataColumns(tableName: $tableName) @@ -1764,6 +1858,13 @@ private function buildObjectFilterConditionsSql( [$query, $nestedFilters] = $this->nestedPaths->split(filters: $query, properties: $properties); foreach ($nestedFilters as $filter) { + // A filter on a path inside an object property is an oracle on that + // property: one the caller may not read matches nothing. + if ($this->relatedRows->mayReadProperty(schema: $schema, property: $filter['root']) === false) { + $conditions[] = '1=0'; + continue; + } + $expression = $this->nestedPaths->expression( column: $this->quoteIdentifier(name: $this->sanitizeColumnName(name: $filter['root']), isPostgres: $isPostgres), path: $filter['path'], @@ -2905,7 +3006,7 @@ private function applyObjectFilters(IQueryBuilder $qb, array $filters, Schema $s $isPostgres = $this->isPostgresPlatform(); [$filters, $nestedFilters] = $this->nestedPaths->split(filters: $filters, properties: $properties ?? []); - $this->applyNestedPathFilters(qb: $qb, nestedFilters: $nestedFilters, properties: $properties ?? [], isPostgres: $isPostgres); + $this->applyNestedPathFilters(qb: $qb, nestedFilters: $nestedFilters, properties: $properties ?? [], isPostgres: $isPostgres, schema: $schema); foreach ($filters as $field => $value) { // Check if this field exists as a column in the schema. @@ -3193,6 +3294,7 @@ private function jsonArrayCondition(IQueryBuilder $qb, string $columnName, mixed * @param list, value: mixed}> $nestedFilters The path filters from NestedPathFilter::split(). * @param array $properties The schema's properties. * @param bool $isPostgres Whether the platform is PostgreSQL. + * @param Schema $schema The schema, for the property read rule. * * @return void * @@ -3201,12 +3303,20 @@ private function jsonArrayCondition(IQueryBuilder $qb, string $columnName, mixed * @spec openspec/changes/nested-property-filters/specs/zoeken-filteren/spec.md#requirement-a-filter-can-name-a-field-inside-an-object-property * @SuppressWarnings(PHPMD.StaticAccess) NullFilter::conditionsFor() is the one reader of `orNull`, shared by every filter path. */ - private function applyNestedPathFilters(IQueryBuilder $qb, array $nestedFilters, array $properties, bool $isPostgres): void { + private function applyNestedPathFilters(IQueryBuilder $qb, array $nestedFilters, array $properties, bool $isPostgres, Schema $schema): void { foreach ($nestedFilters as $filter) { if (($properties[$filter['root']]['x-openregister-encrypted'] ?? false) === true) { throw new EncryptedFieldFilterException(property: $filter['root']); } + // A filter on a path inside an object property is an oracle on that + // property: one the caller may not read matches nothing. + if ($this->relatedRows->mayReadProperty(schema: $schema, property: $filter['root']) === false) { + $this->ignoredFilters[] = $filter['root'] . '.' . implode('.', $filter['path']); + $qb->andWhere('1 = 0'); + continue; + } + $expression = $this->nestedPaths->expression( column: 't.' . $this->sanitizeColumnName(name: $filter['root']), path: $filter['path'], diff --git a/lib/Db/MagicMapper/NestedPathFilter.php b/lib/Db/MagicMapper/NestedPathFilter.php index a1a18b7e2d..801ec7c056 100644 --- a/lib/Db/MagicMapper/NestedPathFilter.php +++ b/lib/Db/MagicMapper/NestedPathFilter.php @@ -54,8 +54,12 @@ class NestedPathFilter { /** * A path segment: a property name, nothing that could leave the JSON path. + * + * Ends in `\z`, not `$`: `$` also matches before a trailing newline, so + * `a\n` passed and reached the JSON path literal, which MariaDB rejects (a + * 500 instead of the refusal this class intends). */ - private const SEGMENT = '/^[A-Za-z0-9_\-]+$/'; + private const SEGMENT = '/^[A-Za-z0-9_\-]+\z/'; /** * Split a filter map into the filters on columns and the filters on a path inside an object property. diff --git a/lib/Exception/RelatedRowFilterNotSupportedException.php b/lib/Exception/RelatedRowFilterNotSupportedException.php new file mode 100644 index 0000000000..a922d18eaf --- /dev/null +++ b/lib/Exception/RelatedRowFilterNotSupportedException.php @@ -0,0 +1,39 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Exception; + +use InvalidArgumentException; + +/** + * A related-row filter on a route that would drop it. + * + * Dropping it answers the unfiltered set to a deliberately narrow question, so + * the route refuses; the middleware turns the refusal into an HTTP 400. + * + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md + */ +class RelatedRowFilterNotSupportedException extends InvalidArgumentException { +}//end class diff --git a/lib/Listener/ViewAlertCrossedListener.php b/lib/Listener/ViewAlertCrossedListener.php index 029ab004b4..7f98cca355 100644 --- a/lib/Listener/ViewAlertCrossedListener.php +++ b/lib/Listener/ViewAlertCrossedListener.php @@ -40,6 +40,7 @@ use OCA\OpenRegister\Event\ViewAlertCrossedEvent; use OCA\OpenRegister\Service\Notification\EmailSender; use OCA\OpenRegister\Service\Notification\NotificationRecipientResolver; +use OCA\OpenRegister\Service\Rbac\ViewerReachResolver; use OCA\OpenRegister\Service\View\ViewAlert; use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; @@ -95,6 +96,7 @@ class ViewAlertCrossedListener implements IEventListener { * @param IFactory $l10nFactory For each recipient's own language in an email. * @param IUserManager $userManager To read a recipient's language. * @param LoggerInterface $logger For recipients and channels nothing could deliver to. + * @param ViewerReachResolver $viewers Re-checks at send time that a recipient may still read the view. */ public function __construct( private readonly NotificationRecipientResolver $recipients, @@ -103,6 +105,7 @@ public function __construct( private readonly IFactory $l10nFactory, private readonly IUserManager $userManager, private readonly LoggerInterface $logger, + private readonly ViewerReachResolver $viewers, ) { }//end __construct() @@ -130,7 +133,7 @@ public function handle(Event $event): void { 'threshold' => $alert->threshold, ]; - $uids = $this->resolve(viewId: $parameters['viewId'], alert: $alert); + $uids = $this->onlyThoseWhoMayRead(view: $view->jsonSerialize(), uids: $this->resolve(viewId: $parameters['viewId'], alert: $alert)); foreach ($alert->channels as $channel) { if ($channel === self::CHANNEL_NOTIFICATION) { $this->notifyAll(uids: $uids, parameters: $parameters); @@ -152,6 +155,46 @@ public function handle(Event $event): void { } }//end handle() + /** + * Drop, and log, every recipient who may no longer read the view. + * + * 🔴 RECIPIENTS ARE JUDGED WHEN THE ALERT IS SAVED, AND REACH CHANGES AFTER. + * An owner removes a share or un-publishes the view, and the recipient who + * could read it at save time keeps receiving its name and count by + * notification and email. So the question is asked again at the moment of + * delivery, with the same rule the save used (the owner, an administrator, + * or whoever the view's shares and `isPublic` reach). An unreadable + * membership drops the recipient: not telling someone is the safe error. + * + * @param array $view The serialised view that crossed. + * @param array $uids The verified recipients. + * + * @return array The recipients who still read it. + * + * @spec openspec/changes/view-alert-recipients/specs/saved-search-views/spec.md + */ + private function onlyThoseWhoMayRead(array $view, array $uids): array { + $readers = []; + $dropped = []; + foreach ($uids as $uid) { + if ($this->viewers->recipientReads(view: $view, recipient: self::USER_PREFIX . $uid) === true) { + $readers[] = $uid; + continue; + } + + $dropped[] = $uid; + } + + if ($dropped !== []) { + $this->logger->warning( + '[ViewAlertCrossedListener] View {view} alert skipped recipients who may no longer read the view: {recipients}', + ['view' => (string)($view['uuid'] ?? ''), 'recipients' => implode(', ', $dropped)] + ); + } + + return $readers; + }//end onlyThoseWhoMayRead() + /** * The verified uids an alert names, with every name that resolved to nobody logged. * diff --git a/lib/Middleware/RelatedRowFilterMiddleware.php b/lib/Middleware/RelatedRowFilterMiddleware.php new file mode 100644 index 0000000000..c3c73d3d0b --- /dev/null +++ b/lib/Middleware/RelatedRowFilterMiddleware.php @@ -0,0 +1,63 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Middleware; + +use Exception; +use OCA\OpenRegister\Exception\RelatedRowFilterNotSupportedException; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\JSONResponse; +use OCP\AppFramework\Http\Response; +use OCP\AppFramework\Middleware; + +/** + * A `_related`/`_referenced` filter on a multi-schema facet (or search) route is + * refused, not dropped. This answers that refusal with a 400 and the exception's + * message instead of the opaque 500 an uncaught exception produces. + * + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md + */ +class RelatedRowFilterMiddleware extends Middleware { + + /** + * Map RelatedRowFilterNotSupportedException onto a 400 response. + * + * @param Controller $controller The controller that was executing. + * @param string $methodName The controller method that was executing. + * @param Exception $exception The exception that was thrown. + * + * @return Response The 400 response. + * + * @throws Exception Rethrows every other exception. + * + * @SuppressWarnings(PHPMD.UnusedFormalParameter) The signature is the Middleware contract. + * + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md + */ + public function afterException(Controller $controller, string $methodName, Exception $exception): Response { + if ($exception instanceof RelatedRowFilterNotSupportedException === false) { + throw $exception; + } + + return new JSONResponse(data: ['error' => $exception->getMessage()], statusCode: 400); + }//end afterException() +}//end class diff --git a/lib/Service/File/TaggingHandler.php b/lib/Service/File/TaggingHandler.php index 692ba1011e..906f352771 100644 --- a/lib/Service/File/TaggingHandler.php +++ b/lib/Service/File/TaggingHandler.php @@ -184,7 +184,7 @@ private function findOrCreateTag(string $tagName): ISystemTag { $allTags = $this->systemTagManager->getAllTags(visibilityFilter: null, nameSearchPattern: $tagName); foreach ($allTags as $tag) { if ($tag->getName() === $tagName) { - return $tag; + return $this->hideInternalTag(tag: $tag); } } @@ -203,7 +203,7 @@ private function findOrCreateTag(string $tagName): ISystemTag { $allTags = $this->systemTagManager->getAllTags(visibilityFilter: null, nameSearchPattern: $tagName); foreach ($allTags as $tag) { if ($tag->getName() === $tagName) { - return $tag; + return $this->hideInternalTag(tag: $tag); } } @@ -211,6 +211,49 @@ private function findOrCreateTag(string $tagName): ISystemTag { }//end try }//end findOrCreateTag() + /** + * Hide an existing internal `object:` tag from users, when it is still visible or assignable. + * + * 🔴 CREATION IS NOT THE ONLY WAY AN OBJECT TAG EXISTS. A tag created before + * the hidden/not-assignable default, or first created by a user in the Files + * tag UI under the same name, keeps `userVisible` and `userAssignable` true, + * so the "out of every user's tag list" outcome held for new installs only. + * Every attach goes through here, so the tag is repaired the next time it is + * touched. A tag that is never attached again is not reached; hiding those + * needs an upgrade step. Best effort: a refused update is logged and the tag + * is still used, because losing the file link is worse than a visible tag. + * + * @param ISystemTag $tag The tag that was found. + * + * @return ISystemTag The same tag. + */ + private function hideInternalTag(ISystemTag $tag): ISystemTag { + if (str_starts_with($tag->getName(), self::OBJECT_TAG_PREFIX) === false) { + return $tag; + } + + if ($tag->isUserVisible() === false && $tag->isUserAssignable() === false) { + return $tag; + } + + try { + $this->systemTagManager->updateTag( + tagId: $tag->getId(), + newName: $tag->getName(), + userVisible: false, + userAssignable: false, + color: $tag->getColor() + ); + } catch (Exception $e) { + $this->logger->warning( + message: '[TaggingHandler] Could not hide the internal tag ' . $tag->getName() . ': ' . $e->getMessage(), + context: ['file' => __FILE__, 'line' => __LINE__] + ); + } + + return $tag; + }//end hideInternalTag() + /** * Get tags for a file. * diff --git a/lib/Service/Flow/FlowStepReport.php b/lib/Service/Flow/FlowStepReport.php index 999cc61425..874d2391ff 100644 --- a/lib/Service/Flow/FlowStepReport.php +++ b/lib/Service/Flow/FlowStepReport.php @@ -50,6 +50,19 @@ class FlowStepReport { */ public const MAX_LINES = 50; + /** + * The most characters one line's message keeps. + * + * The count cap alone does not bound the log: fifty lines of a megabyte each + * are fifty megabytes per step, kept for months. + */ + public const MAX_MESSAGE_LENGTH = 500; + + /** + * What replaces the end of a message cut at {@see self::MAX_MESSAGE_LENGTH}. + */ + public const TRUNCATION_MARKER = '… [truncated]'; + /** * The levels a line may carry. */ @@ -94,7 +107,9 @@ public function put(string $key, mixed $value): void { * `error`; anything else is recorded as `info`. At most * {@see self::MAX_LINES} lines are kept per step, and the last one then * says how many were left out, so a node writing in a loop cannot grow - * the log without bound. + * the log without bound. A message longer than + * {@see self::MAX_MESSAGE_LENGTH} characters is cut and ends in + * {@see self::TRUNCATION_MARKER}, so one large string cannot grow it either. * * @param string $level The line's level. * @param string $message What happened, in plain words. @@ -127,11 +142,30 @@ public function line(string $level, string $message): void { $lines[] = [ 'at' => $this->now(), 'level' => $level, - 'message' => $message, + 'message' => $this->bounded(message: $message), ]; $this->detail['lines'] = $lines; }//end line() + /** + * The message, cut to {@see self::MAX_MESSAGE_LENGTH} characters with a marker when it is longer. + * + * The result, marker included, is at most MAX_MESSAGE_LENGTH characters. + * + * @param string $message The message as the node wrote it. + * + * @return string The bounded message. + */ + private function bounded(string $message): string { + if (mb_strlen($message) <= self::MAX_MESSAGE_LENGTH) { + return $message; + } + + $keep = (self::MAX_MESSAGE_LENGTH - mb_strlen(self::TRUNCATION_MARKER)); + + return mb_substr($message, 0, $keep) . self::TRUNCATION_MARKER; + }//end bounded() + /** * The current moment as ISO 8601 with milliseconds, UTC. * diff --git a/lib/Service/Query/PropertyReadGuard.php b/lib/Service/Query/PropertyReadGuard.php new file mode 100644 index 0000000000..99e8c274c0 --- /dev/null +++ b/lib/Service/Query/PropertyReadGuard.php @@ -0,0 +1,104 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Service\Query; + +use OCA\OpenRegister\Db\Schema; +use OCA\OpenRegister\Service\PropertyRbacHandler; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; +use Throwable; + +/** + * A filter on a property is a boolean oracle on its value. + * + * Whether a row matches `field = x` tells the caller what the field holds, one + * guess at a time, whether or not the response ever contains the field. So the + * property read rule has to hold for a property a filter NAMES, not only for + * one a response returns. Facets and aggregates already withhold a property the + * caller cannot read; this is the same question asked for the filters that + * reach through a reference or into an object property. + * + * The question is asked the way the facet path asks it: the property read rule + * with an EMPTY object, because a filter is not about one record. A rule that + * depends on the record therefore does not admit the filter, which is the safe + * direction. It FAILS CLOSED: when the handler cannot be resolved or throws, + * the property is treated as unreadable. + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ +class PropertyReadGuard { + + /** + * The guard. + * + * @param ContainerInterface|null $container Resolves the property read rule lazily (it is built after the query layer). + * @param LoggerInterface $logger The logger. + */ + public function __construct( + private readonly ?ContainerInterface $container, + private readonly LoggerInterface $logger, + ) { + }//end __construct() + + /** + * Whether the caller may read a property of a schema at all. + * + * @param Schema $schema The schema the property belongs to. + * @param string $property The property name. + * + * @return bool False when the property read rule withholds it, or cannot be evaluated. + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ + public function mayRead(Schema $schema, string $property): bool { + if ($schema->hasPropertyAuthorization() === false) { + // Nothing on this schema is governed at property level. + return true; + } + + if ($this->container === null) { + $this->logger->warning( + message: '[PropertyReadGuard] No container to resolve the property read rule; treating the property as unreadable', + context: ['file' => __FILE__, 'line' => __LINE__, 'property' => $property] + ); + return false; + } + + try { + $rbac = $this->container->get(PropertyRbacHandler::class); + + return $rbac->canReadProperty(schema: $schema, property: $property, object: []); + } catch (Throwable $e) { + $this->logger->warning( + message: '[PropertyReadGuard] Could not check the property read rule; treating the property as unreadable', + context: [ + 'file' => __FILE__, + 'line' => __LINE__, + 'property' => $property, + 'exception' => $e->getMessage(), + ] + ); + return false; + } + }//end mayRead() +}//end class diff --git a/lib/Service/Query/RelatedRowAccess.php b/lib/Service/Query/RelatedRowAccess.php new file mode 100644 index 0000000000..c0855d7bba --- /dev/null +++ b/lib/Service/Query/RelatedRowAccess.php @@ -0,0 +1,186 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Service\Query; + +use InvalidArgumentException; +use OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler; +use OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler; +use OCA\OpenRegister\Db\Schema; +use OCP\DB\QueryBuilder\IQueryBuilder; +use Psr\Log\LoggerInterface; + +/** + * The row predicate says which rows of the related schema may be consulted. It + * says nothing about which of their FIELDS may be matched on, and it carries no + * organisation term. This adds both: the property read rule for every field a + * block names, and the organisation boundary for the related alias. + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ +class RelatedRowAccess { + + /** + * Decides whether the caller may read a property. + * + * @var PropertyReadGuard + */ + private readonly PropertyReadGuard $guard; + + /** + * Constructor. + * + * @param MagicRbacHandler $rbac Builds the related schema's row predicate. + * @param LoggerInterface $logger The logger, for the default guard. + * @param PropertyReadGuard|null $guard The property read guard; defaults to one without a container, which fails closed on a governed schema. + */ + public function __construct(private readonly MagicRbacHandler $rbac, LoggerInterface $logger, ?PropertyReadGuard $guard = null) { + $this->guard = ($guard ?? new PropertyReadGuard(container: null, logger: $logger)); + }//end __construct() + + /** + * Whether the caller may read a property of a schema. + * + * @param Schema $schema The schema. + * @param string $property The property. + * + * @return bool False when it is withheld, or the rule cannot be evaluated. + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ + public function mayReadProperty(Schema $schema, string $property): bool { + return $this->guard->mayRead(schema: $schema, property: $property); + }//end mayReadProperty() + + /** + * The related rows' access predicate: the RBAC row rule AND the organisation boundary. + * + * 🔴 THE ORGANISATION BOUNDARY IS A SEPARATE TERM FROM THE ROW RULE. The row + * predicate carries no `_organisation` term (on a schema with no + * authorization block it admits every row), so a subquery that stopped there + * matched rows of another organisation: a caller who writes a foreign row's + * uuid into their own object could probe that row's fields, and foreign rows + * pointing at the caller's object satisfied `_related`. The outer list would + * never return those rows, so the subquery must not consult them. + * + * The SCOPE is decided by the caller of the list path (one decision, shared + * with the outer query); this only renders it for the related alias. A mode + * it does not know matches nothing. + * + * @param IQueryBuilder $qb The query, which receives the organisation parameters. + * @param Schema $schema The related schema. + * @param string $alias The related row's alias. + * @param string $tableName The bare related table, for the row predicate. + * @param array{mode: string, uuids: array} $scope The decided organisation scope. + * + * @return string The combined predicate. + * + * @spec openspec/specs/rbac-scopes/spec.md + */ + public function predicate(IQueryBuilder $qb, Schema $schema, string $alias, string $tableName, array $scope): string { + $rbacPredicate = $this->rbac->buildRbacPredicateForAlias(schema: $schema, alias: $alias, action: 'read', tableName: $tableName); + $column = $alias . '._organisation'; + $mode = ($scope['mode'] ?? null); + + if ($mode === MagicOrganizationHandler::SCOPE_ALL) { + return $rbacPredicate; + } + + if ($mode === MagicOrganizationHandler::SCOPE_NULL_ONLY) { + return '(' . $rbacPredicate . ') AND ' . $column . ' IS NULL'; + } + + $uuids = array_values(array_filter( + ($scope['uuids'] ?? []), + static fn ($uuid): bool => is_string($uuid) === true && $uuid !== '' + )); + $scoped = [MagicOrganizationHandler::SCOPE_IN, MagicOrganizationHandler::SCOPE_IN_OR_NULL]; + if (in_array($mode, $scoped, true) === false || $uuids === []) { + // SCOPE_NONE, an unknown mode, or "in these organisations" naming none. + return '(' . $rbacPredicate . ') AND 1 = 0'; + } + + $placeholders = []; + foreach ($uuids as $position => $uuid) { + $name = sprintf('%s_org%d', $alias, $position); + $placeholders[] = ':' . $name; + $qb->setParameter($name, $uuid); + } + + $term = $column . ' IN (' . implode(', ', $placeholders) . ')'; + if ($mode === MagicOrganizationHandler::SCOPE_IN_OR_NULL) { + $term = '(' . $term . ' OR ' . $column . ' IS NULL)'; + } + + return '(' . $rbacPredicate . ') AND ' . $term; + }//end predicate() + + /** + * Refuse a block that names a field that is not a readable property of the related schema. + * + * 🔴 TWO REFUSALS, ONE MESSAGE. A field must be a PROPERTY of the related + * schema, not merely a well-formed name: a magic table also has metadata + * columns (`_owner`, `_organisation`, `_authorization`, `_status`), and a + * `gt`/`lt` on those discloses object-level sharing metadata of readable rows + * by binary search, while an unknown name reached the database as an unknown + * column (HTTP 500 instead of the 400 the parser answers elsewhere). + * + * 🔴 A CONDITION IS A BOOLEAN ORACLE ON THE FIELD IT NAMES. The related + * schema's ROW predicate decides which rows may be consulted; it says + * nothing about which of their fields. Without this a caller who cannot read + * `salary` can still binary-search it with `gt`/`lt`. The refusal is worded + * like an unknown property on purpose, so a refused field cannot be told + * apart from one that does not exist. + * + * Covers every property of the related schema the SQL reads: each condition + * field and, for `_related`, the foreign key the join compares (for + * `_referenced` the foreign key is a property of the outer schema, which this + * class is not given). + * + * @param RelatedRowFilter $filter The parsed block. + * @param Schema $schema The related schema. + * + * @return void + * + * @throws InvalidArgumentException When a named field is not a property of the schema, or is withheld from the caller. + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ + public function assertReadable(RelatedRowFilter $filter, Schema $schema): void { + $fields = array_column($filter->conditions, 'field'); + if ($filter->direction !== RelatedRowFilter::DIRECTION_FORWARD) { + $fields[] = $filter->foreignKey; + } + + $properties = $schema->getProperties(); + foreach ($fields as $field) { + $field = (string)$field; + if (array_key_exists($field, $properties) === false + || $this->guard->mayRead(schema: $schema, property: $field) === false + ) { + throw new InvalidArgumentException( + sprintf('\'%s\' is not a property of schema \'%s\' that a related-row filter can match on.', $field, $filter->schema) + ); + } + } + }//end assertReadable() +}//end class diff --git a/lib/Service/Query/RelatedRowExistsClause.php b/lib/Service/Query/RelatedRowExistsClause.php index 0d1d1be5aa..e18b0fbf88 100644 --- a/lib/Service/Query/RelatedRowExistsClause.php +++ b/lib/Service/Query/RelatedRowExistsClause.php @@ -23,6 +23,9 @@ namespace OCA\OpenRegister\Service\Query; use InvalidArgumentException; +use OCP\IDBConnection; +use Psr\Log\LoggerInterface; +use Throwable; /** * One `EXISTS (...)` clause per related-row filter, per engine. @@ -494,10 +497,16 @@ private function joinCondition( /** * The magic-table column a property is stored in. * - * A magic table names its columns in snake case (`propertyDefinition` is - * `property_definition`, see MagicMapper's column naming), so a property - * name written as the schema spells it must be converted before it can be - * a column. A name already in snake case is unchanged. + * Applies the rule that names the columns (`MagicMapper::sanitizeColumnName`): + * camelCase to snake_case, lower-cased, any character outside `[a-z0-9_]` + * replaced by an underscore, runs of underscores collapsed, trailing + * underscores removed (`propertyDefinition` is `property_definition`, a name + * already in snake case is unchanged). The earlier one-line conversion + * skipped the replacement and collapse steps, so a property those steps map + * differently was filtered on a column that does not exist, or refused. The + * `col_` prefix for a leading digit is NOT applied, because the rule that + * creates the columns never applies it either (its guard compares + * `preg_match()` to `false`); such a name is refused by quoteIdentifier(). * * @param string $property The property name. * @@ -506,7 +515,12 @@ private function joinCondition( * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md */ private function columnName(string $property): string { - return strtolower((string)preg_replace('/([a-z0-9])([A-Z])/', '$1_$2', $property)); + $name = strtolower((string)preg_replace('/([a-z0-9])([A-Z])/', '$1_$2', $property)); + $name = (string)preg_replace('/[^a-z0-9_]/', '_', $name); + $name = (string)preg_replace('/_+/', '_', $name); + + // The 64-character cap the read path applies (MySQL's identifier limit). + return substr(rtrim($name, '_'), 0, 64); }//end columnName() /** @@ -525,7 +539,7 @@ private function columnName(string $property): string { * @throws InvalidArgumentException When the name is not a plain identifier. */ private function quoteIdentifier(string $name): string { - if (preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $name) !== 1) { + if (preg_match('/^[A-Za-z_][A-Za-z0-9_]*\z/', $name) !== 1) { throw new InvalidArgumentException( sprintf('\'%s\' is not a column name a related-row filter may reference.', $name) ); @@ -572,4 +586,40 @@ private function jsonField(string $engine, string $alias, string $field): string sprintf('No related-row SQL for engine \'%s\'.', $engine) ); }//end jsonField() + + /** + * Which engine the SQL must be written for. + * + * Read off the LIVE CONNECTION rather than configured separately, because a + * second source of truth for the engine is a second thing that can be wrong + * about it, and being wrong would surface as a syntax error in production + * and nowhere else. + * + * The detection matches `MagicRbacHandler::isPostgres()` deliberately, + * including its fallback: when the platform cannot be read, both default to + * MariaDB syntax. Two different guesses would put MariaDB JSON functions + * and Postgres operators in the same statement. + * + * @param IDBConnection $db The connection, read for its platform. + * @param LoggerInterface $logger The logger. + * + * @return string The engine. + * + * @spec openspec/changes/query-related-schema-rows/specs/zoeken-filteren/spec.md + */ + public function engineFor(IDBConnection $db, LoggerInterface $logger): string { + try { + $platform = $db->getDatabasePlatform(); + if (stripos(get_debug_type($platform), 'PostgreSQL') !== false) { + return RelatedRowExistsClause::ENGINE_POSTGRES; + } + } catch (Throwable $e) { + $logger->warning( + message: '[RelatedRowExistsClause] Could not read the database platform; defaulting to MariaDB syntax', + context: ['file' => __FILE__, 'line' => __LINE__, 'exception' => $e->getMessage()] + ); + } + + return RelatedRowExistsClause::ENGINE_MARIADB; + }//end engineFor() }//end class diff --git a/lib/Service/Query/RelatedRowQueryApplier.php b/lib/Service/Query/RelatedRowQueryApplier.php index 60b34b5516..1765c10494 100644 --- a/lib/Service/Query/RelatedRowQueryApplier.php +++ b/lib/Service/Query/RelatedRowQueryApplier.php @@ -23,7 +23,6 @@ namespace OCA\OpenRegister\Service\Query; use InvalidArgumentException; -use OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler; use OCA\OpenRegister\Db\MagicMapper\MagicTableHandler; use OCA\OpenRegister\Db\Register; use OCA\OpenRegister\Db\Schema; @@ -31,7 +30,6 @@ use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; use Psr\Log\LoggerInterface; -use Throwable; /** * The one place a `_related` filter becomes SQL on a real query. @@ -56,19 +54,34 @@ class RelatedRowQueryApplier { * * @param SchemaMapper $schemaMapper The schema lookup. * @param MagicTableHandler $tableHandler Resolves a register and schema to their table. - * @param MagicRbacHandler $rbacHandler Builds the access predicate for the related rows. * @param IDBConnection $db The connection, read for its platform. * @param LoggerInterface $logger The logger. + * @param RelatedRowAccess $access The access rules for the related rows (row predicate, property read + * rule, organisation boundary). */ public function __construct( private readonly SchemaMapper $schemaMapper, private readonly MagicTableHandler $tableHandler, - private readonly MagicRbacHandler $rbacHandler, private readonly IDBConnection $db, private readonly LoggerInterface $logger, + private readonly RelatedRowAccess $access, ) { }//end __construct() + /** + * Whether the caller may read a property, for a filter that names it (nested-path filters on the outer table). + * + * @param Schema $schema The schema the property belongs to. + * @param string $property The property name. + * + * @return bool False when it is withheld. + * + * @spec openspec/specs/row-field-level-security/spec.md#requirement-a-property-read-rule-holds-on-every-route-that-returns-its-value + */ + public function mayReadProperty(Schema $schema, string $property): bool { + return $this->access->mayReadProperty(schema: $schema, property: $property); + }//end mayReadProperty() + /** * Narrow a query by every `_related` block it carries. * @@ -78,6 +91,8 @@ public function __construct( * @param IQueryBuilder $qb The query being built. * @param array $query The request query. * @param Register $register The register the related rows live in. + * @param callable $organisationScopeFor Given the related schema and the register id, the organisation scope + * the list path applies to that table: `array{mode: string, uuids: array}`. * @param string $outerAlias The alias of the outer object row. * * @return int How many clauses were applied. @@ -87,7 +102,7 @@ public function __construct( * @spec openspec/changes/query-related-schema-rows/specs/zoeken-filteren/spec.md * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md */ - public function apply(IQueryBuilder $qb, array $query, Register $register, string $outerAlias = 't'): int { + public function apply(IQueryBuilder $qb, array $query, Register $register, callable $organisationScopeFor, string $outerAlias = 't'): int { $filters = []; if (array_key_exists(RelatedRowFilterParser::KEY, $query) === true) { $filters = (new RelatedRowFilterParser())->parse($query); @@ -105,7 +120,7 @@ public function apply(IQueryBuilder $qb, array $query, Register $register, strin } $clause = new RelatedRowExistsClause(); - $engine = $this->engine(); + $engine = $clause->engineFor(db: $this->db, logger: $this->logger); $applied = 0; foreach (array_values($filters) as $position => $filter) { @@ -121,6 +136,8 @@ public function apply(IQueryBuilder $qb, array $query, Register $register, strin continue; } + $this->access->assertReadable(filter: $filter, schema: $schema); + $rendered = $clause->render( filter: $filter, engine: $engine, @@ -133,11 +150,12 @@ public function apply(IQueryBuilder $qb, array $query, Register $register, strin // ONE. The two schemas have different authorization blocks, and // reusing the outer query's predicate would decide who may read // case properties by asking who may read cases. - accessPredicate: $this->rbacHandler->buildRbacPredicateForAlias( + accessPredicate: $this->access->predicate( + qb: $qb, schema: $schema, alias: $alias, - action: 'read', - tableName: $relatedTable + tableName: $relatedTable, + scope: $organisationScopeFor($schema, $register->getId()) ), parameterPrefix: $alias, storage: RelatedRowExistsClause::STORAGE_COLUMNS @@ -198,35 +216,4 @@ private function resolveSchema(string $name): Schema { sprintf('There is no schema called \'%s\' to filter related rows on.', $name) ); }//end resolveSchema() - - /** - * Which engine the SQL must be written for. - * - * Read off the LIVE CONNECTION rather than configured separately, because a - * second source of truth for the engine is a second thing that can be wrong - * about it, and being wrong would surface as a syntax error in production - * and nowhere else. - * - * The detection matches `MagicRbacHandler::isPostgres()` deliberately, - * including its fallback: when the platform cannot be read, both default to - * MariaDB syntax. Two different guesses would put MariaDB JSON functions - * and Postgres operators in the same statement. - * - * @return string The engine. - */ - private function engine(): string { - try { - $platform = $this->db->getDatabasePlatform(); - if (stripos(get_debug_type($platform), 'PostgreSQL') !== false) { - return RelatedRowExistsClause::ENGINE_POSTGRES; - } - } catch (Throwable $e) { - $this->logger->warning( - message: '[RelatedRowQueryApplier] Could not read the database platform; defaulting to MariaDB syntax', - context: ['file' => __FILE__, 'line' => __LINE__, 'exception' => $e->getMessage()] - ); - } - - return RelatedRowExistsClause::ENGINE_MARIADB; - }//end engine() }//end class diff --git a/lib/Service/Rbac/ViewerReachResolver.php b/lib/Service/Rbac/ViewerReachResolver.php index 8c3de279f9..a8d565cbdd 100644 --- a/lib/Service/Rbac/ViewerReachResolver.php +++ b/lib/Service/Rbac/ViewerReachResolver.php @@ -209,8 +209,10 @@ public function refusedFields(array $view, ViewerReach $reach, array $update): a * Refuse a save whose alert names recipients this caller may not add. * * The view is judged as it will be saved: the stored view (or, on create, - * just its owner) with the body's `isPublic` and `sharedWith` laid over it. - * The stored alert's recipients are the ones not judged again. + * just its owner) with the body's `isPublic` and `sharedWith` laid over it; + * a caller replacing the whole view (PUT) lays over the `isPublic` the save + * will write. The stored alert's recipients are not judged as additions, but + * one the save itself stops reaching is refused. * * @param array $view The stored view, or `['owner' => ]` on create. * @param ViewerReach $reach The caller's reach. @@ -223,13 +225,31 @@ public function refusedFields(array $view, ViewerReach $reach, array $update): a * @spec openspec/changes/view-alert-recipients/specs/saved-search-views/spec.md */ public function assertMayNameRecipients(array $view, ViewerReach $reach, array $data): void { - $previous = ($view['alert']['recipients'] ?? []); + $previous = array_values(array_filter((array)($view['alert']['recipients'] ?? []), 'is_string')); + $saved = array_merge($view, array_intersect_key($data, ['isPublic' => true, 'sharedWith' => true])); $refused = $this->refusedRecipients( - view: array_merge($view, array_intersect_key($data, ['isPublic' => true, 'sharedWith' => true])), + view: $saved, reach: $reach, alert: ViewAlert::declaredIn(body: $data), - previous: array_values(array_filter((array)$previous, 'is_string')) + previous: $previous ); + + // 🔴 A SAVE THAT NARROWS REACH RE-JUDGES THE RECIPIENTS ALREADY STORED. + // Stored recipients are exempt from the "may this caller add them" test, + // and an update with no `alert` key declares none, so un-publishing a + // view or removing a share left a recipient who could no longer read it. + // A stored recipient who read the view as stored but would not read it as + // saved is refused here, so the owner removes them in the same request + // rather than leaving them on an alert about a view they cannot open. + foreach ($previous as $recipient) { + if ($this->recipientReads(view: $view, recipient: $recipient) === true + && $this->recipientReads(view: $saved, recipient: $recipient) === false + ) { + $refused[] = $recipient; + } + } + + $refused = array_values(array_unique($refused)); if ($refused !== []) { throw new ViewAlertRecipientException(recipients: $refused); } @@ -287,8 +307,10 @@ public function refusedRecipients(array $view, ViewerReach $reach, ?array $alert * @param string $recipient The recipient. * * @return bool True when the view reaches every person the recipient names. + * + * @spec openspec/changes/view-alert-recipients/specs/saved-search-views/spec.md */ - private function recipientReads(array $view, string $recipient): bool { + public function recipientReads(array $view, string $recipient): bool { if (str_starts_with($recipient, ViewAlert::USER_PREFIX) === false) { // A group reads the view when the view is shared with it or public. return $this->shares->accessFor(view: $view, userId: '', userGroups: [$recipient]) !== null; diff --git a/tests/Unit/Controller/FlowRunControllerTest.php b/tests/Unit/Controller/FlowRunControllerTest.php index 8cb9c35ad0..3cf6ae8be8 100644 --- a/tests/Unit/Controller/FlowRunControllerTest.php +++ b/tests/Unit/Controller/FlowRunControllerTest.php @@ -565,6 +565,45 @@ public function testTheHistoryNarrowsToSeveralFlowsAndASinceMoment(): void { $this->controller->index(); } + /** + * A `since` with a UTC offset is bound as the UTC instant it names. + * + * `12:00:00+02:00` is 10:00:00Z; binding the object's own wall clock compared + * it as 12:00:00, two hours off, on both the history and the summary. + * + * @return void + */ + public function testASinceWithAnOffsetIsBoundAsUtc(): void { + $this->params(['since' => '2026-10-19T12:00:00+02:00', 'flowIds' => ['flow-a']]); + + $this->mapper->expects($this->once()) + ->method('findAllRuns') + ->willReturnCallback(function (...$args): array { + $this->assertSame('2026-10-19 10:00:00', $args[6]->format('Y-m-d H:i:s')); + $this->assertSame('UTC', $args[6]->getTimezone()->getName()); + return []; + }); + $this->controller->index(); + }//end testASinceWithAnOffsetIsBoundAsUtc() + + /** + * The same normalisation on the summary's `runsSince`. + * + * @return void + */ + public function testTheSummaryBindsAnOffsetSinceAsUtc(): void { + $this->params(['since' => '2026-10-19T12:00:00+02:00', 'flowIds' => 'flow-a']); + $this->flowOwnership->method('idsOwnedByCaller')->willReturn([]); + + $this->mapper->expects($this->once()) + ->method('summarizeByFlow') + ->willReturnCallback(function (array $flowIds, \DateTime $since): array { + $this->assertSame('2026-10-19 10:00:00', $since->format('Y-m-d H:i:s')); + return []; + }); + $this->controller->summary(); + }//end testTheSummaryBindsAnOffsetSinceAsUtc() + public function testResumeSignalsTheSuspendedRunWithTheRequestBody(): void { $run = new FlowRun(); $run->setUuid('run-1'); diff --git a/tests/Unit/Controller/ViewAlertRecipientsTest.php b/tests/Unit/Controller/ViewAlertRecipientsTest.php index 7ce583b1d7..a0c7e78483 100644 --- a/tests/Unit/Controller/ViewAlertRecipientsTest.php +++ b/tests/Unit/Controller/ViewAlertRecipientsTest.php @@ -259,4 +259,54 @@ public function testAStrangerMayNotSetAnAlertAtAll(): void { $this->assertSame(['alert'], $response->getData()['fields']); $this->assertNull($this->written); } + + /** + * 🔴 A PUT IS JUDGED AGAINST THE VIEW IT WILL SAVE, NOT THE ONE STORED. + * + * The body omits `isPublic`, which a PUT writes as false. Judged against the + * stored public view, a stranger was admitted as a recipient of a view that + * is private once saved. + * + * @return void + */ + public function testAPutThatOmitsIsPublicIsJudgedAsPrivate(): void { + $this->stored(isPublic: true); + $this->request->method('getParams')->willReturn( + ['name' => 'Queue', 'query' => ['@self' => ['register' => 1]]] + $this->alertBody(['user:stranger']) + ); + + $response = $this->controllerAs('owner')->update('7'); + + $this->assertSame(403, $response->getStatus()); + $this->assertSame(['user:stranger'], $response->getData()['recipients']); + $this->assertNull($this->written); + } + + /** + * The control: a PATCH that omits `isPublic` keeps the stored public view, so the same recipient is allowed. + * + * @return void + */ + public function testAPatchThatOmitsIsPublicKeepsTheStoredReach(): void { + $this->stored(isPublic: true); + $this->request->method('getParams')->willReturn($this->alertBody(['user:stranger'])); + + $this->assertSame(200, $this->controllerAs('owner')->patch('7')->getStatus()); + } + + /** + * Un-publishing a view whose stored alert names a recipient reached only through it is refused. + * + * @return void + */ + public function testUnpublishingARecipientsOnlyWayInIsRefused(): void { + $this->stored(recipients: ['user:stranger'], isPublic: true); + $this->request->method('getParams')->willReturn(['isPublic' => false]); + + $response = $this->controllerAs('owner')->patch('7'); + + $this->assertSame(403, $response->getStatus()); + $this->assertSame(['user:stranger'], $response->getData()['recipients']); + $this->assertNull($this->written); + } }//end class diff --git a/tests/Unit/Db/MagicMapper/MagicMapperFolderIdsTest.php b/tests/Unit/Db/MagicMapper/MagicMapperFolderIdsTest.php new file mode 100644 index 0000000000..094f71bca7 --- /dev/null +++ b/tests/Unit/Db/MagicMapper/MagicMapperFolderIdsTest.php @@ -0,0 +1,220 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @spec openspec/changes/organisation-usage-measured/specs/organisation-usage-measured/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Db\MagicMapper; + +use Exception; +use OCA\OpenRegister\Db\MagicMapper; +use OCA\OpenRegister\Db\RegisterMapper; +use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Service\DateTimeNormalizer; +use OCA\OpenRegister\Service\SettingsService; +use OCP\DB\IPreparedStatement; +use OCP\DB\IResult; +use OCP\DB\QueryBuilder\IExpressionBuilder; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IAppConfig; +use OCP\IConfig; +use OCP\IDBConnection; +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * `MagicMapper::findFolderIdsByOrganisation()`. + * + * @covers \OCA\OpenRegister\Db\MagicMapper + * @uses \OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler + */ +class MagicMapperFolderIdsTest extends TestCase { + + private IDBConnection&MockObject $db; + + private LoggerInterface&MockObject $logger; + + private SchemaMapper&MockObject $schemaMapper; + + private RegisterMapper&MockObject $registerMapper; + + private IUserSession&MockObject $userSession; + + private IGroupManager&MockObject $groupManager; + + protected function setUp(): void { + parent::setUp(); + $this->db = $this->createMock(IDBConnection::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->schemaMapper = $this->createMock(SchemaMapper::class); + $this->registerMapper = $this->createMock(RegisterMapper::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->groupManager = $this->createMock(IGroupManager::class); + }//end setUp() + + /** + * Build a MagicMapper with the shared mock dependencies. + * + * @return MagicMapper + */ + private function makeMapper(): MagicMapper { + $container = $this->makeContainer(); + + return new MagicMapper( + $this->db, + $this->schemaMapper, + $this->registerMapper, + $this->createMock(IConfig::class), + $this->createMock(IEventDispatcher::class), + $this->userSession, + $this->groupManager, + $this->createMock(IUserManager::class), + $this->createMock(IAppConfig::class), + $this->logger, + $this->createMock(SettingsService::class), + $container + ); + }//end makeMapper() + + /** + * Build a DI container that resolves the lazily-fetched collaborators + * MagicMapper pulls during construction (DateTimeNormalizer, ConditionMatcher, + * SchemaTypeConverter), matching the production container contract. + * + * @return ContainerInterface&MockObject + */ + private function makeContainer(): ContainerInterface { + $dateTimeNormalizer = $this->createMock(DateTimeNormalizer::class); + $conditionMatcher = $this->createMock(\OCA\OpenRegister\Service\ConditionMatcher::class); + $schemaTypeConverter = $this->createMock(\OCA\OpenRegister\Service\Object\SchemaTypeConverter::class); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willReturnCallback( + function (string $id) use ($dateTimeNormalizer, $conditionMatcher, $schemaTypeConverter) { + if ($id === DateTimeNormalizer::class) { + return $dateTimeNormalizer; + } + if ($id === \OCA\OpenRegister\Service\ConditionMatcher::class) { + return $conditionMatcher; + } + if ($id === \OCA\OpenRegister\Service\Object\SchemaTypeConverter::class) { + return $schemaTypeConverter; + } + return null; + } + ); + return $container; + }//end makeContainer() + + /** + * A query builder whose query yields the given folder ids, or throws. + * + * @param array|null $folders The `_folder` values, or null to throw on execute. + * + * @return IQueryBuilder&MockObject + */ + private function folderQuery(?array $folders): IQueryBuilder { + $qb = $this->createMock(IQueryBuilder::class); + foreach (['selectDistinct', 'from', 'where', 'andWhere'] as $method) { + $qb->method($method)->willReturnSelf(); + } + + $qb->method('expr')->willReturn($this->createMock(IExpressionBuilder::class)); + $qb->method('createNamedParameter')->willReturn(':p'); + + if ($folders === null) { + $qb->method('executeQuery')->willThrowException(new Exception('Unknown column _folder')); + return $qb; + } + + $result = $this->createMock(IResult::class); + $result->method('fetchOne')->willReturnOnConsecutiveCalls(...array_merge($folders, [false])); + $qb->method('executeQuery')->willReturn($result); + + return $qb; + }//end folderQuery() + + /** + * Two magic tables are discovered, with the given queries behind them. + * + * @param IQueryBuilder $first The first table's query. + * @param IQueryBuilder $second The second table's query. + * + * @return void + */ + private function discoverTwoTables(IQueryBuilder $first, IQueryBuilder $second): void { + $stmt = $this->createMock(IPreparedStatement::class); + $result = $this->createMock(IResult::class); + $result->method('fetchAll')->willReturn( + [['table_name' => 'oc_openregister_table_1_1'], ['table_name' => 'oc_openregister_table_1_2']] + ); + $stmt->method('execute')->willReturn($result); + $this->db->method('prepare')->willReturn($stmt); + $this->db->method('getQueryBuilder')->willReturnOnConsecutiveCalls($first, $second); + }//end discoverTwoTables() + + /** + * The control: every table readable, the distinct numeric folders come back. + * + * @return void + */ + public function testEveryReadableTableContributesItsFolders(): void { + $this->discoverTwoTables($this->folderQuery([11, '12']), $this->folderQuery([12, 13])); + $this->logger->expects($this->never())->method('warning'); + + $this->assertSame([11, 12, 13], $this->makeMapper()->findFolderIdsByOrganisation(organisationUuid: 'org-1')); + }//end testEveryReadableTableContributesItsFolders() + + /** + * 🔴 A TABLE THAT FAILS MAKES THE MEASUREMENT FAIL, AND IS LOGGED BY NAME. + * + * Skipping it returned a plain int that was too low, which the service then + * presented as a measured storage figure. + * + * @return void + */ + public function testAFailingTableFailsTheMeasurementAndIsLogged(): void { + $this->discoverTwoTables($this->folderQuery([11]), $this->folderQuery(null)); + + $logged = []; + $this->logger->method('warning')->willReturnCallback( + function (string $message, array $context = []) use (&$logged): void { + $logged[] = $context; + } + ); + + try { + $this->makeMapper()->findFolderIdsByOrganisation(organisationUuid: 'org-1'); + $this->fail('Expected the scan to fail'); + } catch (RuntimeException $e) { + $this->assertStringContainsString('oc_openregister_table_1_2', $e->getMessage()); + $this->assertStringNotContainsString('oc_openregister_table_1_1', $e->getMessage()); + } + + $this->assertCount(1, $logged); + $this->assertSame('oc_openregister_table_1_2', $logged[0]['table']); + }//end testAFailingTableFailsTheMeasurementAndIsLogged() +}//end class diff --git a/tests/Unit/Db/MagicMapper/MagicMapperUnionRoutingTest.php b/tests/Unit/Db/MagicMapper/MagicMapperUnionRoutingTest.php new file mode 100644 index 0000000000..b945f23e6f --- /dev/null +++ b/tests/Unit/Db/MagicMapper/MagicMapperUnionRoutingTest.php @@ -0,0 +1,151 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://OpenRegister.app + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + * + * @spec openspec/changes/query-referenced-schema-rows/specs/zoeken-filteren/spec.md + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Db\MagicMapper; + +use Exception; +use OCA\OpenRegister\Db\MagicMapper; +use OCA\OpenRegister\Db\RegisterMapper; +use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Service\DateTimeNormalizer; +use OCA\OpenRegister\Service\SettingsService; +use OCP\DB\IPreparedStatement; +use OCP\DB\IResult; +use OCP\DB\QueryBuilder\IExpressionBuilder; +use OCP\DB\QueryBuilder\IQueryBuilder; +use OCP\EventDispatcher\IEventDispatcher; +use OCP\IAppConfig; +use OCP\IConfig; +use OCP\IDBConnection; +use OCP\IGroupManager; +use OCP\IUserManager; +use OCP\IUserSession; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\LoggerInterface; +use RuntimeException; + +/** + * `MagicMapper::shouldUseUnionQuery()`: related-row filters need the sequential route. + * + * @covers \OCA\OpenRegister\Db\MagicMapper + * @uses \OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler + */ +class MagicMapperUnionRoutingTest extends TestCase { + + private IDBConnection&MockObject $db; + + private LoggerInterface&MockObject $logger; + + private SchemaMapper&MockObject $schemaMapper; + + private RegisterMapper&MockObject $registerMapper; + + private IUserSession&MockObject $userSession; + + private IGroupManager&MockObject $groupManager; + + protected function setUp(): void { + parent::setUp(); + $this->db = $this->createMock(IDBConnection::class); + $this->logger = $this->createMock(LoggerInterface::class); + $this->schemaMapper = $this->createMock(SchemaMapper::class); + $this->registerMapper = $this->createMock(RegisterMapper::class); + $this->userSession = $this->createMock(IUserSession::class); + $this->groupManager = $this->createMock(IGroupManager::class); + }//end setUp() + + /** + * Build a MagicMapper with the shared mock dependencies. + * + * @return MagicMapper + */ + private function makeMapper(): MagicMapper { + $container = $this->makeContainer(); + + return new MagicMapper( + $this->db, + $this->schemaMapper, + $this->registerMapper, + $this->createMock(IConfig::class), + $this->createMock(IEventDispatcher::class), + $this->userSession, + $this->groupManager, + $this->createMock(IUserManager::class), + $this->createMock(IAppConfig::class), + $this->logger, + $this->createMock(SettingsService::class), + $container + ); + }//end makeMapper() + + /** + * Build a DI container that resolves the lazily-fetched collaborators + * MagicMapper pulls during construction (DateTimeNormalizer, ConditionMatcher, + * SchemaTypeConverter), matching the production container contract. + * + * @return ContainerInterface&MockObject + */ + private function makeContainer(): ContainerInterface { + $dateTimeNormalizer = $this->createMock(DateTimeNormalizer::class); + $conditionMatcher = $this->createMock(\OCA\OpenRegister\Service\ConditionMatcher::class); + $schemaTypeConverter = $this->createMock(\OCA\OpenRegister\Service\Object\SchemaTypeConverter::class); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willReturnCallback( + function (string $id) use ($dateTimeNormalizer, $conditionMatcher, $schemaTypeConverter) { + if ($id === DateTimeNormalizer::class) { + return $dateTimeNormalizer; + } + if ($id === \OCA\OpenRegister\Service\ConditionMatcher::class) { + return $conditionMatcher; + } + if ($id === \OCA\OpenRegister\Service\Object\SchemaTypeConverter::class) { + return $schemaTypeConverter; + } + return null; + } + ); + return $container; + }//end makeContainer() + + /** + * A plain cross-table search keeps the UNION route; `_related`, `_referenced` and aggregations leave it. + * + * The raw-SQL UNION builder refuses the related-row keys, which would be an + * uncaught exception on the controller routes; the sequential route builds + * them as EXISTS subqueries instead. + * + * @return void + */ + public function testARelatedRowFilterTakesTheSequentialRoute(): void { + $route = new \ReflectionMethod(MagicMapper::class, 'shouldUseUnionQuery'); + $mapper = $this->makeMapper(); + $related = ['caseProperty' => ['case' => ['value' => 'x']]]; + + $this->assertTrue($route->invoke($mapper, ['_limit' => 10])); + $this->assertFalse($route->invoke($mapper, ['_related' => $related])); + $this->assertFalse($route->invoke($mapper, ['_referenced' => $related])); + $this->assertFalse($route->invoke($mapper, ['_aggregations' => ['x']])); + }//end testARelatedRowFilterTakesTheSequentialRoute() +}//end class diff --git a/tests/Unit/Db/MagicMapper/MagicSearchHandlerUnionTenancyTest.php b/tests/Unit/Db/MagicMapper/MagicSearchHandlerUnionTenancyTest.php index d940db6f2f..bf03c28447 100644 --- a/tests/Unit/Db/MagicMapper/MagicSearchHandlerUnionTenancyTest.php +++ b/tests/Unit/Db/MagicMapper/MagicSearchHandlerUnionTenancyTest.php @@ -344,4 +344,124 @@ public function testAnUnreadableFlagKeepsTheBoundary(): void { $this->assertStringContainsString("_organisation IN ('org-a')", $sql); }//end testAnUnreadableFlagKeepsTheBoundary() + + /** + * A related-row subquery gets the SAME organisation scope the list applies to that table. + * + * @return void + */ + public function testARelatedRowSubqueryIsConfinedToTheCallersOrganisation(): void { + $scope = ['mode' => MagicOrganizationHandler::SCOPE_IN, 'uuids' => ['org-a']]; + $method = new \ReflectionMethod(MagicSearchHandler::class, 'organisationScopeForRelated'); + + $decided = $method->invoke($this->handler($scope), [], new Schema(), 1); + + $this->assertSame($scope, $decided); + }//end testARelatedRowSubqueryIsConfinedToTheCallersOrganisation() + + /** + * Where the list waives the boundary (an RBAC rule governs), the subquery waives it too, and a grant holder keeps it. + * + * @return void + */ + public function testARelatedRowSubqueryWaivesTheBoundaryExactlyWhereTheListDoes(): void { + $scope = ['mode' => MagicOrganizationHandler::SCOPE_IN, 'uuids' => ['org-a']]; + $method = new \ReflectionMethod(MagicSearchHandler::class, 'organisationScopeForRelated'); + + $waived = $method->invoke($this->handler($scope, conditionalBypass: true), [], new Schema(), 1); + $this->assertSame(MagicOrganizationHandler::SCOPE_ALL, $waived['mode']); + + $held = $method->invoke($this->handler($scope, conditionalBypass: true, holdsGrants: true), [], new Schema(), 1); + $this->assertSame($scope, $held); + }//end testARelatedRowSubqueryWaivesTheBoundaryExactlyWhereTheListDoes() + /** + * 🔴 THE UNION BUILDER REFUSES `_related` AND `_referenced` INSTEAD OF DROPPING THEM. + * + * Underscore keys are skipped there, so a cross-table search carrying one + * answered the unfiltered set. The single-table path refuses with the same + * exception type. + * + * @return void + */ + public function testTheUnionBuilderRefusesRelatedRowFilters(): void { + $handler = $this->handler(['mode' => MagicOrganizationHandler::SCOPE_ALL, 'uuids' => []]); + + foreach (['_related', '_referenced'] as $key) { + try { + $handler->buildWhereConditionsSql(query: [$key => ['caseProperty' => ['case' => ['value' => 'x']]]], schema: new Schema()); + $this->fail('Expected a refusal for ' . $key); + } catch (\OCA\OpenRegister\Exception\RelatedRowFilterNotSupportedException $e) { + $this->assertStringContainsString("'" . $key . "'", $e->getMessage()); + } + } + + // The control: without the keys the same builder answers. + $this->assertIsArray($handler->buildWhereConditionsSql(query: [], schema: new Schema())); + }//end testTheUnionBuilderRefusesRelatedRowFilters() + + /** + * `_multitenancy=false` waives the boundary for the related subquery as it does for the list. + * + * @return void + */ + public function testARelatedRowSubqueryHonoursTheMultitenancyOptOut(): void { + $scope = ['mode' => MagicOrganizationHandler::SCOPE_IN, 'uuids' => ['org-a']]; + $method = new \ReflectionMethod(MagicSearchHandler::class, 'organisationScopeForRelated'); + + $off = $method->invoke($this->handler($scope), ['_multitenancy' => false], new Schema(), 1); + $this->assertSame(MagicOrganizationHandler::SCOPE_ALL, $off['mode']); + + // A string 'false' from the query string is read as false too; an unreadable value keeps the boundary ON. + $string = $method->invoke($this->handler($scope), ['_multitenancy' => 'false'], new Schema(), 1); + $this->assertSame(MagicOrganizationHandler::SCOPE_ALL, $string['mode']); + $garbage = $method->invoke($this->handler($scope), ['_multitenancy' => ['x']], new Schema(), 1); + $this->assertSame($scope, $garbage); + }//end testARelatedRowSubqueryHonoursTheMultitenancyOptOut() + + /** + * `_rbac=false` does not widen the tenant edge: no RBAC rule can waive the boundary, so it stands. + * + * @return void + */ + public function testARelatedRowSubqueryKeepsTheBoundaryWhenRbacIsOff(): void { + $scope = ['mode' => MagicOrganizationHandler::SCOPE_IN, 'uuids' => ['org-a']]; + $method = new \ReflectionMethod(MagicSearchHandler::class, 'organisationScopeForRelated'); + + // Even a handler that WOULD waive the boundary for an RBAC caller must not do so with RBAC off. + $decided = $method->invoke($this->handler($scope, conditionalBypass: true), ['_rbac' => false], new Schema(), 1); + + $this->assertSame($scope, $decided); + }//end testARelatedRowSubqueryKeepsTheBoundaryWhenRbacIsOff() + + /** + * An anonymous caller (no active organisation) gets the empty scope, which the subquery renders as no rows. + * + * @return void + */ + public function testARelatedRowSubqueryGivesAnAnonymousCallerNoRows(): void { + $none = ['mode' => MagicOrganizationHandler::SCOPE_NONE, 'uuids' => []]; + $method = new \ReflectionMethod(MagicSearchHandler::class, 'organisationScopeForRelated'); + + $decided = $method->invoke($this->handler($none), [], new Schema(), 1); + + $this->assertSame($none, $decided); + }//end testARelatedRowSubqueryGivesAnAnonymousCallerNoRows() + + /** + * A public schema drops the boundary unless the caller explicitly asked for multitenancy. + * + * @return void + */ + public function testARelatedRowSubqueryOfAPublicSchemaDropsTheBoundaryUnlessExplicit(): void { + $scope = ['mode' => MagicOrganizationHandler::SCOPE_IN, 'uuids' => ['org-a']]; + $method = new \ReflectionMethod(MagicSearchHandler::class, 'organisationScopeForRelated'); + $public = new Schema(); + $public->setAuthorization(['read' => ['public']]); + + $dropped = $method->invoke($this->handler($scope), [], $public, 1); + $this->assertSame(MagicOrganizationHandler::SCOPE_ALL, $dropped['mode']); + + $explicit = $method->invoke($this->handler($scope), ['_multitenancy_explicit' => true], $public, 1); + $this->assertSame($scope, $explicit); + }//end testARelatedRowSubqueryOfAPublicSchemaDropsTheBoundaryUnlessExplicit() }//end class diff --git a/tests/Unit/Db/MagicSearchHandlerNestedPathFilterTest.php b/tests/Unit/Db/MagicSearchHandlerNestedPathFilterTest.php index 1d9a24c27a..c014b990a2 100644 --- a/tests/Unit/Db/MagicSearchHandlerNestedPathFilterTest.php +++ b/tests/Unit/Db/MagicSearchHandlerNestedPathFilterTest.php @@ -76,12 +76,16 @@ class MagicSearchHandlerNestedPathFilterTest extends TestCase { /** * The handler on a PostgreSQL connection double. * + * @param bool $mayReadProperties Whether the caller may read the root property of a nested path. + * * @return MagicSearchHandler */ - private function handler(): MagicSearchHandler { + private function handler(bool $mayReadProperties = true): MagicSearchHandler { $db = $this->createMock(IDBConnection::class); $db->method('getDatabasePlatform')->willReturn($this->createMock(PostgreSQL120Platform::class)); $logger = $this->createMock(LoggerInterface::class); + $related = $this->createMock(RelatedRowQueryApplier::class); + $related->method('mayReadProperty')->willReturn($mayReadProperties); return new MagicSearchHandler( db: $db, @@ -90,7 +94,7 @@ private function handler(): MagicSearchHandler { organizationHandler: $this->createMock(MagicOrganizationHandler::class), schemaTypeConverter: new SchemaTypeConverter(), dateTimeNormalizer: new DateTimeNormalizer($logger), - relatedRows: $this->createMock(RelatedRowQueryApplier::class) + relatedRows: $related ); }//end handler() @@ -168,6 +172,32 @@ private function rawObject(array $query): array { return $method->invoke($this->handler(), $query, $schema, $connection, true); }//end rawObject() + /** + * A path into an object property the caller may not read matches nothing, on both paths. + * + * A filter on `dunning.currentStage` is an oracle on `dunning`; the rule that + * withholds the property from a response has to withhold it from a filter. + * + * @return void + */ + public function testAPathIntoAnUnreadablePropertyMatchesNothing(): void { + $schema = $this->createMock(Schema::class); + $schema->method('getProperties')->willReturn(self::PROPERTIES); + $this->captured = []; + + $method = new ReflectionMethod(MagicSearchHandler::class, 'applyObjectFilters'); + $method->invoke($this->handler(mayReadProperties: false), $this->queryBuilder(), ['dunning.currentStage' => 2], $schema); + $this->assertSame(['1 = 0'], $this->captured); + + $connection = $this->createMock(IDBConnection::class); + $connection->method('quote')->willReturnCallback(static fn ($v) => "'{$v}'"); + $raw = new ReflectionMethod(MagicSearchHandler::class, 'buildObjectFilterConditionsSql'); + $this->assertSame( + ['1=0'], + $raw->invoke($this->handler(mayReadProperties: false), ['dunning' => ['currentStage' => 2]], $schema, $connection, true) + ); + }//end testAPathIntoAnUnreadablePropertyMatchesNothing() + /** * The dotted key and the nested map the request layer rebuilds read the same path. * @@ -303,4 +333,24 @@ public function testSplitCollectsEveryLeaf(): void { $nested ); }//end testSplitCollectsEveryLeaf() + /** + * A segment with a trailing newline is not a plain name and is refused. + * + * `$` matches before a final newline, so `a\n` passed the segment check and + * reached the JSON path literal, where MariaDB rejects it with a 500. + * + * @return void + */ + public function testASegmentWithATrailingNewlineIsRefused(): void { + $filter = new NestedPathFilter(); + + foreach (["a\n", "currentStage\n", "\n", "a\r\n"] as $segment) { + $this->assertNull($filter->expression(column: 't.dunning', path: [$segment], isPostgres: true), json_encode($segment)); + $this->assertNull($filter->expression(column: 't.dunning', path: ['ok', $segment], isPostgres: false), json_encode($segment)); + } + + $this->assertNotNull($filter->expression(column: 't.dunning', path: ['currentStage'], isPostgres: true)); + $this->assertSame(['1 = 0'], $this->applyObject(["dunning.current\nStage" => 'a'])); + $this->assertSame(['1 = 0'], $this->applyObject(['dunning' => ["currentStage\n" => 'a']])); + }//end testASegmentWithATrailingNewlineIsRefused() }//end class diff --git a/tests/Unit/Db/MagicSearchHandlerOrNullModifierTest.php b/tests/Unit/Db/MagicSearchHandlerOrNullModifierTest.php index e8c164ea4c..a29bdd4b95 100644 --- a/tests/Unit/Db/MagicSearchHandlerOrNullModifierTest.php +++ b/tests/Unit/Db/MagicSearchHandlerOrNullModifierTest.php @@ -80,6 +80,8 @@ protected function setUp(): void { $logger = $this->createMock(LoggerInterface::class); $db = $this->createMock(IDBConnection::class); $db->method('getDatabasePlatform')->willReturn($this->createMock(PostgreSQL120Platform::class)); + $related = $this->createMock(RelatedRowQueryApplier::class); + $related->method('mayReadProperty')->willReturn(true); $this->handler = new MagicSearchHandler( db: $db, @@ -88,7 +90,7 @@ protected function setUp(): void { organizationHandler: $this->createMock(MagicOrganizationHandler::class), schemaTypeConverter: new SchemaTypeConverter(), dateTimeNormalizer: new DateTimeNormalizer($logger), - relatedRows: $this->createMock(RelatedRowQueryApplier::class) + relatedRows: $related ); $this->captured = []; }//end setUp() diff --git a/tests/Unit/Listener/ViewAlertCrossedListenerTest.php b/tests/Unit/Listener/ViewAlertCrossedListenerTest.php index 236cf7c68e..4a11832c7b 100644 --- a/tests/Unit/Listener/ViewAlertCrossedListenerTest.php +++ b/tests/Unit/Listener/ViewAlertCrossedListenerTest.php @@ -33,12 +33,14 @@ use OCA\OpenRegister\Listener\ViewAlertCrossedListener; use OCA\OpenRegister\Service\Notification\EmailSender; use OCA\OpenRegister\Service\Notification\NotificationRecipientResolver; +use OCA\OpenRegister\Service\Rbac\ViewerReachResolver; use OCA\OpenRegister\Service\View\ViewAlert; use OCP\IGroup; use OCP\IGroupManager; use OCP\IL10N; use OCP\IUser; use OCP\IUserManager; +use OCP\IUserSession; use OCP\L10N\IFactory; use OCP\Mail\IMailer; use OCP\Mail\IMessage; @@ -53,6 +55,8 @@ * @uses \OCA\OpenRegister\Event\ViewAlertCrossedEvent * @uses \OCA\OpenRegister\Service\Notification\EmailSender * @uses \OCA\OpenRegister\Service\Notification\NotificationRecipientResolver + * @uses \OCA\OpenRegister\Service\Rbac\ViewerReachResolver + * @uses \OCA\OpenRegister\Service\Rbac\ViewShareResolver * @uses \OCA\OpenRegister\Service\View\ViewAlert */ class ViewAlertCrossedListenerTest extends TestCase { @@ -123,6 +127,10 @@ private function listener(): ViewAlertCrossedListener { $groupManager = $this->createMock(IGroupManager::class); $groupManager->method('groupExists')->willReturnCallback(static fn (string $gid): bool => $gid === 'teamleads'); $groupManager->method('get')->willReturnCallback(static fn (string $gid): ?IGroup => ($gid === 'teamleads' ? $group : null)); + $groupManager->method('isAdmin')->willReturnCallback(static fn (string $uid): bool => $uid === 'admin'); + $groupManager->method('isInGroup')->willReturnCallback( + static fn (string $uid, string $gid): bool => ($gid === 'teamleads' && in_array($uid, ['alice', 'bob'], true) === true) + ); $this->warnings = new \ArrayObject(); $logger = new class($this->warnings) extends AbstractLogger { @@ -186,7 +194,8 @@ public function log($level, $message, array $context = []): void { email: new EmailSender(userManager: $userManager, mailer: $mailer, logger: $logger), l10nFactory: $factory, userManager: $userManager, - logger: $logger + logger: $logger, + viewers: new ViewerReachResolver(userSession: $this->createMock(IUserSession::class), groupManager: $groupManager, logger: $logger) ); }//end listener() @@ -245,15 +254,20 @@ private function recordingMessage(): IMessage { }//end recordingMessage() /** - * A view owned by `owner`, named "Overdue cases". + * A view owned by `owner`, named "Overdue cases", public unless told otherwise. + * + * @param bool $isPublic Whether everyone reads it. + * @param array> $sharedWith Its shares. * * @return View */ - private function view(): View { + private function view(bool $isPublic=true, array $sharedWith=[]): View { $view = new View(); $view->setUuid('view-uuid-1'); $view->setName('Overdue cases'); $view->setOwner('owner'); + $view->setIsPublic($isPublic); + $view->setSharedWith($sharedWith); return $view; }//end view() @@ -263,14 +277,15 @@ private function view(): View { * * @param array $declared The alert block as a view stores it. * @param int $count The count that crossed. + * @param View|null $view The view that crossed, a public one by default. * * @return ViewAlertCrossedEvent */ - private function crossing(array $declared, int $count = 23): ViewAlertCrossedEvent { + private function crossing(array $declared, int $count = 23, ?View $view = null): ViewAlertCrossedEvent { $alert = ViewAlert::parse($declared); $this->assertNotNull($alert); - return new ViewAlertCrossedEvent(view: $this->view(), alert: $alert, count: $count); + return new ViewAlertCrossedEvent(view: ($view ?? $this->view()), alert: $alert, count: $count); }//end crossing() /** @@ -378,4 +393,42 @@ public function testTheListenerIsSubscribedToTheCrossingEvent(): void { $application ); }//end testTheListenerIsSubscribedToTheCrossingEvent() + + /** + * 🔴 A RECIPIENT WHO NO LONGER READS THE VIEW IS NOT TOLD ABOUT IT. + * + * The save judged the recipients at one instant. After the owner un-publishes + * the view, bob (who reached it only because it was public) must stop getting + * its name and count; carol (shared by name) and the owner still do, and an + * administrator reads everything. The drop is logged. + */ + public function testARecipientWhoNoLongerReadsTheViewIsDroppedAndLogged(): void { + $private = $this->view(isPublic: false, sharedWith: [['user' => 'carol', 'mode' => 'read']]); + $this->listener()->handle( + $this->crossing( + ['operator' => 'gte', 'threshold' => 20, 'recipients' => ['user:bob', 'user:carol', 'user:owner']], + view: $private + ) + ); + + $users = array_column($this->notified, 'user'); + sort($users); + $this->assertSame(['carol'], $users, 'bob lost the view, owner is not a user on this server'); + $this->assertStringContainsString('bob', implode("\n", $this->warnings->getArrayCopy())); + }//end testARecipientWhoNoLongerReadsTheViewIsDroppedAndLogged() + + /** + * A group the view is still shared with keeps its members; a group it was removed from loses them. + */ + public function testAGroupRecipientFollowsTheViewsShares(): void { + $sharedWithTeam = $this->view(isPublic: false, sharedWith: [['group' => 'teamleads', 'mode' => 'read']]); + $this->listener()->handle($this->crossing(['operator' => 'gte', 'threshold' => 20, 'recipients' => ['teamleads']], view: $sharedWithTeam)); + $users = array_column($this->notified, 'user'); + sort($users); + $this->assertSame(['alice', 'bob'], $users); + + $this->notified = []; + $this->listener()->handle($this->crossing(['operator' => 'gte', 'threshold' => 20, 'recipients' => ['teamleads']], view: $this->view(isPublic: false))); + $this->assertSame([], $this->notified, 'the share was removed, so the group no longer reads the view'); + }//end testAGroupRecipientFollowsTheViewsShares() }//end class diff --git a/tests/Unit/Middleware/RelatedRowFilterMiddlewareTest.php b/tests/Unit/Middleware/RelatedRowFilterMiddlewareTest.php new file mode 100644 index 0000000000..667e35b732 --- /dev/null +++ b/tests/Unit/Middleware/RelatedRowFilterMiddlewareTest.php @@ -0,0 +1,113 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://www.OpenRegister.app + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Middleware; + +use OCA\OpenRegister\Db\MagicMapper\MagicFacetHandler; +use OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler; +use OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler; +use OCA\OpenRegister\Db\MagicMapper\MagicSearchHandler; +use OCA\OpenRegister\Db\Schema; +use OCA\OpenRegister\Exception\RelatedRowFilterNotSupportedException; +use OCA\OpenRegister\Middleware\RelatedRowFilterMiddleware; +use OCA\OpenRegister\Service\DateTimeNormalizer; +use OCA\OpenRegister\Service\Object\SchemaTypeConverter; +use OCA\OpenRegister\Service\Query\RelatedRowQueryApplier; +use OCP\AppFramework\Controller; +use OCP\AppFramework\Http\JSONResponse; +use OCP\IDBConnection; +use PHPUnit\Framework\TestCase; +use Psr\Log\LoggerInterface; + +/** + * `RelatedRowFilterMiddleware` and the multi-schema facet refusal it answers. + * + * @covers \OCA\OpenRegister\Middleware\RelatedRowFilterMiddleware + * @covers \OCA\OpenRegister\Db\MagicMapper\MagicFacetHandler + * @uses \OCA\OpenRegister\Db\MagicMapper\MagicSearchHandler + * @uses \OCA\OpenRegister\Exception\RelatedRowFilterNotSupportedException + */ +class RelatedRowFilterMiddlewareTest extends TestCase { + + /** + * The refusal becomes a 400 carrying the exception's message in the route's error shape. + * + * @return void + */ + public function testTheRefusalAnswers400WithItsMessage(): void { + $response = (new RelatedRowFilterMiddleware())->afterException( + controller: $this->createMock(Controller::class), + methodName: 'index', + exception: new RelatedRowFilterNotSupportedException('\'_related\' filters are not supported') + ); + + $this->assertInstanceOf(JSONResponse::class, $response); + $this->assertSame(400, $response->getStatus()); + $this->assertSame(['error' => '\'_related\' filters are not supported'], $response->getData()); + }//end testTheRefusalAnswers400WithItsMessage() + + /** + * Every other exception, including a plain InvalidArgumentException, is rethrown. + * + * @return void + */ + public function testOtherExceptionsAreRethrown(): void { + $this->expectException(\InvalidArgumentException::class); + + (new RelatedRowFilterMiddleware())->afterException( + controller: $this->createMock(Controller::class), + methodName: 'index', + exception: new \InvalidArgumentException('something else') + ); + }//end testOtherExceptionsAreRethrown() + + /** + * 🔴 THE MULTI-SCHEMA FACET ROUTE REFUSES, BEFORE ITS OWN CATCH BLOCKS CAN SWALLOW IT. + * + * The per-facet queries catch every exception and return empty buckets, so + * the refusal has to come first; without it the filter was dropped and the + * counts were wrong with only a log line to show for it. + * + * @return void + */ + public function testTheUnionFacetsRefuseRelatedRowFilters(): void { + $logger = $this->createMock(LoggerInterface::class); + $search = new MagicSearchHandler( + db: $this->createMock(IDBConnection::class), + logger: $logger, + rbacHandler: $this->createMock(MagicRbacHandler::class), + organizationHandler: $this->createMock(MagicOrganizationHandler::class), + schemaTypeConverter: new SchemaTypeConverter(), + dateTimeNormalizer: new DateTimeNormalizer($logger), + relatedRows: $this->createMock(RelatedRowQueryApplier::class) + ); + $facets = new MagicFacetHandler(db: $this->createMock(IDBConnection::class), logger: $logger, searchHandler: $search); + $tables = [['tableName' => 'openregister_table_1_2', 'register' => null, 'schema' => new Schema()]]; + + foreach (['_related', '_referenced'] as $key) { + try { + $facets->getSimpleFacetsUnion(tableConfigs: $tables, query: ['_facets' => ['status'], $key => ['s' => ['case' => ['value' => 'x']]]]); + $this->fail('Expected a refusal for ' . $key); + } catch (RelatedRowFilterNotSupportedException $e) { + $this->assertStringContainsString("'" . $key . "'", $e->getMessage()); + } + } + }//end testTheUnionFacetsRefuseRelatedRowFilters() +}//end class diff --git a/tests/Unit/Service/File/TaggingHandlerInternalTagTest.php b/tests/Unit/Service/File/TaggingHandlerInternalTagTest.php index ab81cb83be..787114a6f3 100644 --- a/tests/Unit/Service/File/TaggingHandlerInternalTagTest.php +++ b/tests/Unit/Service/File/TaggingHandlerInternalTagTest.php @@ -91,4 +91,76 @@ public function testAUserTagStaysVisibleAndAssignable(): void { $this->assertSame(['visible' => true, 'assignable' => true], $created['invoice'] ?? null); }//end testAUserTagStaysVisibleAndAssignable() + /** + * Attach one existing tag to a file and record how the manager was asked to update it. + * + * @param string $name The existing tag's name. + * @param bool $visible Whether it is visible now. + * @param bool $assignable Whether it is assignable now. + * @param bool $updateFails Whether the manager refuses the update. + * + * @return array> The updateTag() calls. + */ + private function updatesForExisting(string $name, bool $visible, bool $assignable, bool $updateFails=false): array { + $updates = []; + $tag = $this->createMock(ISystemTag::class); + $tag->method('getId')->willReturn('7'); + $tag->method('getName')->willReturn($name); + $tag->method('isUserVisible')->willReturn($visible); + $tag->method('isUserAssignable')->willReturn($assignable); + $tag->method('getColor')->willReturn('00ff00'); + + $manager = $this->createMock(ISystemTagManager::class); + $manager->method('getAllTags')->willReturn([$tag]); + $manager->expects($this->never())->method('createTag'); + $manager->method('updateTag')->willReturnCallback( + function (string $tagId, string $newName, bool $userVisible, bool $userAssignable, ?string $color) use (&$updates, $updateFails): void { + $updates[] = compact('tagId', 'newName', 'userVisible', 'userAssignable', 'color'); + if ($updateFails === true) { + throw new \Exception('refused'); + } + } + ); + + $mapper = $this->createMock(ISystemTagObjectMapper::class); + $mapper->method('getTagIdsForObjects')->willReturn([]); + $mapper->expects($this->once())->method('assignTags'); + + $handler = new TaggingHandler($manager, $mapper, $this->createMock(LoggerInterface::class), $this->createMock(IUserSession::class)); + $handler->attachTagsToFile(fileId: '12', tags: [$name]); + + return $updates; + }//end updatesForExisting() + + /** + * 🔴 AN EXISTING OBJECT TAG THAT IS STILL VISIBLE OR ASSIGNABLE IS HIDDEN WHEN IT IS NEXT USED. + * + * A tag created before the hidden default, or by a user in the Files tag UI, + * kept both flags true, so the outcome held for new installs only. + * + * @return void + */ + public function testAnExistingVisibleObjectTagIsHiddenWhenItIsNextUsed(): void { + $updates = $this->updatesForExisting(name: 'object:3f2c-uuid', visible: true, assignable: true); + + $this->assertSame( + [['tagId' => '7', 'newName' => 'object:3f2c-uuid', 'userVisible' => false, 'userAssignable' => false, 'color' => '00ff00']], + $updates + ); + + $this->assertCount(1, $this->updatesForExisting(name: 'object:3f2c-uuid', visible: false, assignable: true)); + }//end testAnExistingVisibleObjectTagIsHiddenWhenItIsNextUsed() + + /** + * Controls: an already hidden object tag and a user's own tag are not updated, and a refused update does not lose the link. + * + * @return void + */ + public function testOtherExistingTagsAreLeftAlone(): void { + $this->assertSame([], $this->updatesForExisting(name: 'object:3f2c-uuid', visible: false, assignable: false)); + $this->assertSame([], $this->updatesForExisting(name: 'invoice', visible: true, assignable: true)); + + // The assignTags() expectation inside the helper proves the file is still linked. + $this->assertCount(1, $this->updatesForExisting(name: 'object:3f2c-uuid', visible: true, assignable: true, updateFails: true)); + }//end testOtherExistingTagsAreLeftAlone() }//end class diff --git a/tests/Unit/Service/Flow/FlowStepReportTest.php b/tests/Unit/Service/Flow/FlowStepReportTest.php index b22d5ffa38..57493741a9 100644 --- a/tests/Unit/Service/Flow/FlowStepReportTest.php +++ b/tests/Unit/Service/Flow/FlowStepReportTest.php @@ -68,4 +68,20 @@ public function testAStepKeepsAtMostTheCapAndSaysHowManyWereLeftOut(): void { $report->line('info', 'Next hop.'); $this->assertSame('Next hop.', $report->take()['lines'][0]['message']); } + + public function testALongMessageIsCutToTheCapAndEndsInAMarker(): void { + $report = new FlowStepReport(); + $report->line('info', str_repeat('x', 100000)); + $report->line('info', str_repeat('é', FlowStepReport::MAX_MESSAGE_LENGTH + 1)); + $report->line('info', str_repeat('y', FlowStepReport::MAX_MESSAGE_LENGTH)); + + $lines = $report->take()['lines']; + + $this->assertSame(FlowStepReport::MAX_MESSAGE_LENGTH, mb_strlen($lines[0]['message'])); + $this->assertStringEndsWith(FlowStepReport::TRUNCATION_MARKER, $lines[0]['message']); + $this->assertStringStartsWith('xxx', $lines[0]['message']); + $this->assertSame(FlowStepReport::MAX_MESSAGE_LENGTH, mb_strlen($lines[1]['message']), 'multibyte text is cut by characters'); + $this->assertStringEndsWith(FlowStepReport::TRUNCATION_MARKER, $lines[1]['message']); + $this->assertSame(str_repeat('y', FlowStepReport::MAX_MESSAGE_LENGTH), $lines[2]['message'], 'a message at the cap is untouched'); + } } diff --git a/tests/Unit/Service/Query/PropertyReadGuardTest.php b/tests/Unit/Service/Query/PropertyReadGuardTest.php new file mode 100644 index 0000000000..427f7dff2d --- /dev/null +++ b/tests/Unit/Service/Query/PropertyReadGuardTest.php @@ -0,0 +1,111 @@ + + * @copyright 2026 Conduction B.V. + * @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12 + * + * @link https://www.OpenRegister.app + * + * SPDX-License-Identifier: EUPL-1.2 + * SPDX-FileCopyrightText: 2026 Conduction B.V. + */ + +declare(strict_types=1); + +namespace OCA\OpenRegister\Tests\Unit\Service\Query; + +use OCA\OpenRegister\Db\Schema; +use OCA\OpenRegister\Service\PropertyRbacHandler; +use OCA\OpenRegister\Service\Query\PropertyReadGuard; +use PHPUnit\Framework\TestCase; +use Psr\Container\ContainerInterface; +use Psr\Log\NullLogger; + +/** + * `PropertyReadGuard`. + * + * @covers \OCA\OpenRegister\Service\Query\PropertyReadGuard + * @uses \OCA\OpenRegister\Db\Schema + */ +class PropertyReadGuardTest extends TestCase { + + /** + * A schema, with or without a property that carries an authorization block. + * + * @param bool $governed Whether a property carries authorization. + * + * @return Schema The schema. + */ + private function schema(bool $governed): Schema { + $schema = new Schema(); + $schema->setId(3); + $properties = ['name' => ['type' => 'string']]; + if ($governed === true) { + $properties['salary'] = ['type' => 'number', 'authorization' => ['read' => ['hr']]]; + } + + $schema->setProperties($properties); + + return $schema; + }//end schema() + + /** + * A schema without property authorization asks nobody and admits. + * + * @return void + */ + public function testASchemaWithoutPropertyAuthorizationAdmitsEveryProperty(): void { + $container = $this->createMock(ContainerInterface::class); + $container->expects($this->never())->method('get'); + + $guard = new PropertyReadGuard(container: $container, logger: new NullLogger()); + + $this->assertTrue($guard->mayRead(schema: $this->schema(governed: false), property: 'name')); + }//end testASchemaWithoutPropertyAuthorizationAdmitsEveryProperty() + + /** + * The property read rule decides, asked with an empty object. + * + * @return void + */ + public function testTheReadRuleDecidesWithAnEmptyObject(): void { + $schema = $this->schema(governed: true); + $rbac = $this->createMock(PropertyRbacHandler::class); + $rbac->expects($this->exactly(2))->method('canReadProperty') + ->with($schema, $this->anything(), []) + ->willReturnCallback(static fn (Schema $s, string $property): bool => $property !== 'salary'); + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->with(PropertyRbacHandler::class)->willReturn($rbac); + + $guard = new PropertyReadGuard(container: $container, logger: new NullLogger()); + + $this->assertTrue($guard->mayRead(schema: $schema, property: 'name')); + $this->assertFalse($guard->mayRead(schema: $schema, property: 'salary')); + }//end testTheReadRuleDecidesWithAnEmptyObject() + + /** + * Fails closed: no container, or a handler that throws, withholds the property. + * + * @return void + */ + public function testItFailsClosed(): void { + $schema = $this->schema(governed: true); + + $this->assertFalse( + (new PropertyReadGuard(container: null, logger: new NullLogger()))->mayRead(schema: $schema, property: 'salary') + ); + + $container = $this->createMock(ContainerInterface::class); + $container->method('get')->willThrowException(new \RuntimeException('no handler')); + + $this->assertFalse( + (new PropertyReadGuard(container: $container, logger: new NullLogger()))->mayRead(schema: $schema, property: 'salary') + ); + }//end testItFailsClosed() +}//end class diff --git a/tests/Unit/Service/Query/ReferencedRowFilterTest.php b/tests/Unit/Service/Query/ReferencedRowFilterTest.php index 360722bcab..453e80881c 100644 --- a/tests/Unit/Service/Query/ReferencedRowFilterTest.php +++ b/tests/Unit/Service/Query/ReferencedRowFilterTest.php @@ -29,6 +29,7 @@ namespace OCA\OpenRegister\Tests\Unit\Service\Query; use InvalidArgumentException; +use OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler; use OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler; use OCA\OpenRegister\Db\MagicMapper\MagicTableHandler; use OCA\OpenRegister\Db\Register; @@ -37,6 +38,7 @@ use OCA\OpenRegister\Service\Query\RelatedRowExistsClause; use OCA\OpenRegister\Service\Query\RelatedRowFilter; use OCA\OpenRegister\Service\Query\RelatedRowFilterParser; +use OCA\OpenRegister\Service\Query\RelatedRowAccess; use OCA\OpenRegister\Service\Query\RelatedRowQueryApplier; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -126,6 +128,7 @@ public function testACamelCasePropertyReadsItsSnakeCaseColumn(): void { public function testTheApplierAddsForwardBlocksWithTheReferencedSchemasOwnAccessPredicate(): void { $schema = new Schema(); $schema->setId(7); + $schema->setProperties(['case' => ['type' => 'string']]); $schemaMapper = $this->createMock(SchemaMapper::class); $schemaMapper->expects($this->once())->method('findBySlug')->with('subsidieAanvraag', 2)->willReturn([$schema]); @@ -152,13 +155,33 @@ public function testTheApplierAddsForwardBlocksWithTheReferencedSchemasOwnAccess $applied = (new RelatedRowQueryApplier( schemaMapper: $schemaMapper, tableHandler: $tableHandler, - rbacHandler: $rbac, db: $this->createMock(IDBConnection::class), - logger: new NullLogger() - ))->apply(qb: $qb, query: $this->forwardQuery(), register: $register); + logger: new NullLogger(), + access: new RelatedRowAccess(rbac: $rbac, logger: new NullLogger()) + ))->apply(qb: $qb, query: $this->forwardQuery(), register: $register, organisationScopeFor: static fn (): array => ['mode' => MagicOrganizationHandler::SCOPE_ALL, 'uuids' => []]); $this->assertSame(1, $applied); $this->assertStringContainsString('rel0._uuid = t."subsidieaanvraag"', $sql[0]); $this->assertStringContainsString("rel0._owner = 'alice'", $sql[0]); } + + public function testAColumnNameFollowsTheRuleThatNamesTheColumns(): void { + $sql = fn (string $property): string => (new RelatedRowExistsClause())->render( + filter: new RelatedRowFilter('caseProperty', 'case', [['field' => $property, 'operator' => 'eq', 'value' => 'x']]), + engine: RelatedRowExistsClause::ENGINE_POSTGRES, + table: 'oc_openregister_table_1_2', + outerAlias: 't', + innerAlias: 'rel0', + accessPredicate: 'TRUE', + parameterPrefix: 'rel0', + storage: RelatedRowExistsClause::STORAGE_COLUMNS + )['sql']; + + $this->assertStringContainsString('rel0."due_date" = :rel0_c0', $sql('due-date')); + $this->assertStringContainsString('rel0."first_name" = :rel0_c0', $sql('first__name')); + $this->assertStringContainsString('rel0."is_active" = :rel0_c0', $sql('isActive_')); + + // Capped at 64 characters, as MagicSearchHandler::sanitizeColumnName caps it. + $this->assertStringContainsString('rel0."' . str_repeat('a', 64) . '" = :rel0_c0', $sql(str_repeat('a', 70))); + } } diff --git a/tests/Unit/Service/Query/RelatedRowExistsClauseTest.php b/tests/Unit/Service/Query/RelatedRowExistsClauseTest.php index 22c0a4c26b..bc1c73d52e 100644 --- a/tests/Unit/Service/Query/RelatedRowExistsClauseTest.php +++ b/tests/Unit/Service/Query/RelatedRowExistsClauseTest.php @@ -421,9 +421,36 @@ public function testAMagicTableUsesTheUnderscoredMetadataColumns(): void { public function testANonIdentifierColumnNameIsRefused(): void { $this->expectException(InvalidArgumentException::class); - $this->renderColumns([['field' => 'found"; DROP TABLE x --', 'operator' => 'eq', 'value' => '1']]); + // Leading digit: the column-naming rule keeps it, and it is not an identifier. + $this->renderColumns([['field' => '9lives', 'operator' => 'eq', 'value' => '1']]); }//end testANonIdentifierColumnNameIsRefused() + /** + * The injection payload is neutralised by the column-naming rule, so no quote or statement break reaches the SQL. + * + * @return void + */ + public function testAnInjectionPayloadNeverReachesTheSql(): void { + $sql = $this->renderColumns([['field' => 'found"; DROP TABLE x --', 'operator' => 'eq', 'value' => '1']])['sql']; + + $this->assertStringContainsString('"found_drop_table_x"', $sql); + $this->assertStringNotContainsString('DROP', $sql); + $this->assertStringNotContainsString(';', $sql); + }//end testAnInjectionPayloadNeverReachesTheSql() + + /** + * The identifier check ends at the end of the string, not before a trailing newline. + * + * @return void + */ + public function testAnIdentifierWithATrailingNewlineIsRefused(): void { + $method = new \ReflectionMethod(RelatedRowExistsClause::class, 'quoteIdentifier'); + + $this->assertSame('"case_id"', $method->invoke($this->clause, 'case_id')); + $this->expectException(InvalidArgumentException::class); + $method->invoke($this->clause, "case_id\n"); + }//end testAnIdentifierWithATrailingNewlineIsRefused() + /** * An unknown storage is refused rather than guessed. * diff --git a/tests/Unit/Service/Query/RelatedRowPrefixedDatabaseTest.php b/tests/Unit/Service/Query/RelatedRowPrefixedDatabaseTest.php index eea768d766..91d217b7e1 100644 --- a/tests/Unit/Service/Query/RelatedRowPrefixedDatabaseTest.php +++ b/tests/Unit/Service/Query/RelatedRowPrefixedDatabaseTest.php @@ -32,11 +32,13 @@ use Doctrine\DBAL\Connection; use Doctrine\DBAL\DriverManager; +use OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler; use OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler; use OCA\OpenRegister\Db\MagicMapper\MagicTableHandler; use OCA\OpenRegister\Db\Register; use OCA\OpenRegister\Db\Schema; use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Service\Query\RelatedRowAccess; use OCA\OpenRegister\Service\Query\RelatedRowQueryApplier; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -47,6 +49,9 @@ * @covers \OCA\OpenRegister\Service\Query\RelatedRowQueryApplier * @covers \OCA\OpenRegister\Service\Query\RelatedRowExistsClause * @uses \OCA\OpenRegister\Db\Register + * @uses \OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler + * @uses \OCA\OpenRegister\Service\Query\PropertyReadGuard + * @uses \OCA\OpenRegister\Service\Query\RelatedRowAccess * @uses \OCA\OpenRegister\Db\Schema * @uses \OCA\OpenRegister\Service\Query\RelatedRowFilter * @uses \OCA\OpenRegister\Service\Query\RelatedRowFilterParser @@ -88,6 +93,7 @@ protected function setUp(): void { private function clausesFor(array $query): array { $schema = new Schema(); $schema->setId(2); + $schema->setProperties(['case' => ['type' => 'string']]); $schemaMapper = $this->createMock(SchemaMapper::class); $schemaMapper->method('findBySlug')->willReturn([$schema]); @@ -118,10 +124,10 @@ private function clausesFor(array $query): array { (new RelatedRowQueryApplier( schemaMapper: $schemaMapper, tableHandler: $tableHandler, - rbacHandler: $rbac, db: $this->createMock(IDBConnection::class), - logger: new NullLogger() - ))->apply(qb: $qb, query: $query, register: $register); + logger: new NullLogger(), + access: new RelatedRowAccess(rbac: $rbac, logger: new NullLogger()) + ))->apply(qb: $qb, query: $query, register: $register, organisationScopeFor: static fn (): array => ['mode' => MagicOrganizationHandler::SCOPE_ALL, 'uuids' => []]); return [$where, $parameters]; } diff --git a/tests/Unit/Service/Query/RelatedRowQueryApplierTest.php b/tests/Unit/Service/Query/RelatedRowQueryApplierTest.php index caeb5d3be6..3a36207b3f 100644 --- a/tests/Unit/Service/Query/RelatedRowQueryApplierTest.php +++ b/tests/Unit/Service/Query/RelatedRowQueryApplierTest.php @@ -27,11 +27,14 @@ namespace OCA\OpenRegister\Tests\Unit\Service\Query; use InvalidArgumentException; +use OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler; use OCA\OpenRegister\Db\MagicMapper\MagicRbacHandler; use OCA\OpenRegister\Db\MagicMapper\MagicTableHandler; use OCA\OpenRegister\Db\Register; use OCA\OpenRegister\Db\Schema; use OCA\OpenRegister\Db\SchemaMapper; +use OCA\OpenRegister\Service\Query\PropertyReadGuard; +use OCA\OpenRegister\Service\Query\RelatedRowAccess; use OCA\OpenRegister\Service\Query\RelatedRowQueryApplier; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -42,8 +45,12 @@ * `RelatedRowQueryApplier`. * * @covers \OCA\OpenRegister\Service\Query\RelatedRowQueryApplier + * @covers \OCA\OpenRegister\Service\Query\RelatedRowAccess * @uses \OCA\OpenRegister\Db\Register + * @uses \OCA\OpenRegister\Db\MagicMapper\MagicOrganizationHandler * @uses \OCA\OpenRegister\Db\Schema + * @uses \OCA\OpenRegister\Service\Query\PropertyReadGuard + * @uses \OCA\OpenRegister\Service\Query\RelatedRowAccess * @uses \OCA\OpenRegister\Service\Query\RelatedRowExistsClause * @uses \OCA\OpenRegister\Service\Query\RelatedRowFilter * @uses \OCA\OpenRegister\Service\Query\RelatedRowFilterParser @@ -55,10 +62,11 @@ class RelatedRowQueryApplierTest extends TestCase { * * @param array $found What findBySlug returns. * @param bool $tableExists Whether the related schema's table exists. + * @param PropertyReadGuard|null $guard The property read guard, when the test needs to decide it. * * @return RelatedRowQueryApplier The applier. */ - private function applierFinding(array $found, bool $tableExists=true): RelatedRowQueryApplier { + private function applierFinding(array $found, bool $tableExists=true, ?PropertyReadGuard $guard=null): RelatedRowQueryApplier { $schemaMapper = $this->createMock(SchemaMapper::class); $schemaMapper->method('findBySlug')->willReturn($found); @@ -72,12 +80,21 @@ private function applierFinding(array $found, bool $tableExists=true): RelatedRo return new RelatedRowQueryApplier( schemaMapper: $schemaMapper, tableHandler: $tableHandler, - rbacHandler: $rbac, db: $this->createMock(IDBConnection::class), - logger: new NullLogger() + logger: new NullLogger(), + access: new RelatedRowAccess(rbac: $rbac, logger: new NullLogger(), guard: $guard) ); }//end applierFinding() + /** + * An organisation scope that admits every organisation (admin bypass, system context). + * + * @return callable The scope resolver. + */ + private function allOrganisations(): callable { + return static fn (): array => ['mode' => MagicOrganizationHandler::SCOPE_ALL, 'uuids' => []]; + }//end allOrganisations() + /** * A schema. * @@ -86,6 +103,7 @@ private function applierFinding(array $found, bool $tableExists=true): RelatedRo private function schema(): Schema { $schema = new Schema(); $schema->setId(2); + $schema->setProperties(['case' => ['type' => 'string'], 'value' => ['type' => 'number']]); return $schema; }//end schema() @@ -132,7 +150,8 @@ public function testAQueryWithoutRelatedBlocksIsUntouched(): void { $applied = $this->applierFinding([])->apply( qb: $qb, query: ['_limit' => 10], - register: $this->register() + register: $this->register(), + organisationScopeFor: $this->allOrganisations() ); $this->assertSame(0, $applied); @@ -156,7 +175,8 @@ public function testAnUnresolvableSchemaIsRefusedNotSkipped(): void { $this->applierFinding([])->apply( qb: $qb, query: $this->relatedQuery(), - register: $this->register() + register: $this->register(), + organisationScopeFor: $this->allOrganisations() ); }//end testAnUnresolvableSchemaIsRefusedNotSkipped() @@ -174,7 +194,8 @@ public function testAnAmbiguousSchemaNameIsRefused(): void { $this->applierFinding([$this->schema(), $this->schema()])->apply( qb: $this->createMock(IQueryBuilder::class), query: $this->relatedQuery(), - register: $this->register() + register: $this->register(), + organisationScopeFor: $this->allOrganisations() ); }//end testAnAmbiguousSchemaNameIsRefused() @@ -195,7 +216,8 @@ public function testAResolvableBlockNarrowsTheQuery(): void { $applied = $this->applierFinding([$this->schema()])->apply( qb: $qb, query: $this->relatedQuery(), - register: $this->register() + register: $this->register(), + organisationScopeFor: $this->allOrganisations() ); $this->assertSame(1, $applied); @@ -226,7 +248,8 @@ public function testTheRegisterMayComeFromTheQueryItself(): void { $applied = $this->applierFinding([$this->schema()])->apply( qb: $qb, query: $query, - register: $this->register() + register: $this->register(), + organisationScopeFor: $this->allOrganisations() ); $this->assertSame(1, $applied); @@ -262,10 +285,169 @@ function ($condition) use (&$conditions, $qb) { ], ], ], - register: $this->register() + register: $this->register(), + organisationScopeFor: $this->allOrganisations() ); $this->assertSame(expected: 1, actual: $applied); $this->assertSame(expected: ['1 = 0'], actual: $conditions); }//end testAReferencedSchemaWithoutATableMatchesNothing() + + /** + * 🔴 A CONDITION ON A PROPERTY THE CALLER MAY NOT READ IS REFUSED. + * + * The related schema's row predicate says which rows may be consulted, not + * which of their fields: `gt`/`lt` on a withheld `salary` is a binary search + * for its value. The query ends, and nothing is added to it. + * + * @return void + */ + public function testAConditionOnAnUnreadablePropertyIsRefused(): void { + $guard = $this->createMock(PropertyReadGuard::class); + $guard->expects($this->atLeastOnce())->method('mayRead')->willReturn(false); + + $qb = $this->createMock(IQueryBuilder::class); + $qb->expects($this->never())->method('andWhere'); + + $this->expectException(InvalidArgumentException::class); + + $this->applierFinding(found: [$this->schema()], guard: $guard)->apply( + qb: $qb, + query: $this->relatedQuery(), + register: $this->register(), + organisationScopeFor: $this->allOrganisations() + ); + }//end testAConditionOnAnUnreadablePropertyIsRefused() + + /** + * The control: the same block with a readable property is applied. + * + * @return void + */ + public function testAConditionOnAReadablePropertyIsApplied(): void { + $guard = $this->createMock(PropertyReadGuard::class); + $guard->method('mayRead')->willReturn(true); + + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('createFunction')->willReturnArgument(0); + $qb->expects($this->once())->method('andWhere'); + + $applied = $this->applierFinding(found: [$this->schema()], guard: $guard)->apply( + qb: $qb, + query: $this->relatedQuery(), + register: $this->register(), + organisationScopeFor: $this->allOrganisations() + ); + + $this->assertSame(1, $applied); + }//end testAConditionOnAReadablePropertyIsApplied() + /** + * 🔴 THE RELATED ROWS ARE CONFINED TO THE CALLER'S ORGANISATION. + * + * The row predicate carries no `_organisation` term, so on a schema with no + * authorization block a foreign organisation's row satisfied a related-row + * condition. The subquery now carries the boundary the list applies. + * + * @return void + */ + public function testTheRelatedRowsAreConfinedToTheCallersOrganisation(): void { + $sql = []; + $parameters = []; + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('createFunction')->willReturnCallback(static function (string $text) use (&$sql): string { + $sql[] = $text; + return $text; + }); + $qb->method('setParameter')->willReturnCallback(function (string $name, mixed $value) use (&$parameters, $qb) { + $parameters[$name] = $value; + return $qb; + }); + + $this->applierFinding([$this->schema()])->apply( + qb: $qb, + query: $this->relatedQuery(), + register: $this->register(), + organisationScopeFor: static fn (): array => ['mode' => MagicOrganizationHandler::SCOPE_IN_OR_NULL, 'uuids' => ['org-a', 'org-b']] + ); + + $this->assertStringContainsString("(rel0._owner = 'alice') AND (rel0._organisation IN (:rel0_org0, :rel0_org1) OR rel0._organisation IS NULL)", $sql[0]); + $this->assertSame('org-a', $parameters['rel0_org0']); + $this->assertSame('org-b', $parameters['rel0_org1']); + }//end testTheRelatedRowsAreConfinedToTheCallersOrganisation() + + /** + * No organisation, or a scope the renderer does not know, matches nothing; null-only matches the org-less rows. + * + * @return void + */ + public function testAScopeThatAdmitsNothingMatchesNothing(): void { + $cases = [ + 'none' => [['mode' => MagicOrganizationHandler::SCOPE_NONE, 'uuids' => []], 'AND 1 = 0'], + 'unknown' => [['mode' => 'surprise', 'uuids' => ['org-a']], 'AND 1 = 0'], + 'empty' => [['mode' => MagicOrganizationHandler::SCOPE_IN, 'uuids' => []], 'AND 1 = 0'], + 'null' => [['mode' => MagicOrganizationHandler::SCOPE_NULL_ONLY, 'uuids' => []], 'AND rel0._organisation IS NULL'], + ]; + + foreach ($cases as $label => [$scope, $expected]) { + $sql = []; + $qb = $this->createMock(IQueryBuilder::class); + $qb->method('createFunction')->willReturnCallback(static function (string $text) use (&$sql): string { + $sql[] = $text; + return $text; + }); + + $this->applierFinding([$this->schema()])->apply( + qb: $qb, + query: $this->relatedQuery(), + register: $this->register(), + organisationScopeFor: static fn (): array => $scope + ); + + $this->assertStringContainsString($expected, $sql[0], $label); + } + }//end testAScopeThatAdmitsNothingMatchesNothing() + /** + * 🔴 A METADATA COLUMN OR AN UNKNOWN NAME IS NOT A PROPERTY OF THE RELATED SCHEMA. + * + * Without the lookup, `_owner`, `_authorization` and `_status` of readable + * rows could be compared with `gt`/`lt` (sharing metadata by binary search), + * and an unknown name reached the database as an unknown column. + * + * @return void + */ + public function testAFieldThatIsNotAPropertyOfTheRelatedSchemaIsRefused(): void { + foreach (['_owner', '_authorization', '_status', 'noSuchProperty'] as $field) { + $qb = $this->createMock(IQueryBuilder::class); + $qb->expects($this->never())->method('andWhere'); + + try { + $this->applierFinding([$this->schema()])->apply( + qb: $qb, + query: ['_related' => ['caseProperty' => ['case' => [$field => ['gt' => 'a']]]]], + register: $this->register(), + organisationScopeFor: $this->allOrganisations() + ); + $this->fail('Expected a refusal for ' . $field); + } catch (InvalidArgumentException $e) { + $this->assertStringContainsString("'" . $field . "'", $e->getMessage()); + } + } + }//end testAFieldThatIsNotAPropertyOfTheRelatedSchemaIsRefused() + + /** + * The foreign key the join compares is a property of the related schema too, for `_related`. + * + * @return void + */ + public function testAForeignKeyThatIsNotAPropertyIsRefused(): void { + $this->expectException(InvalidArgumentException::class); + $this->expectExceptionMessage("'_organisation'"); + + $this->applierFinding([$this->schema()])->apply( + qb: $this->createMock(IQueryBuilder::class), + query: ['_related' => ['caseProperty' => ['_organisation' => ['value' => 'x']]]], + register: $this->register(), + organisationScopeFor: $this->allOrganisations() + ); + }//end testAForeignKeyThatIsNotAPropertyIsRefused() }//end class diff --git a/tests/Unit/Service/Rbac/ViewerReachResolverTest.php b/tests/Unit/Service/Rbac/ViewerReachResolverTest.php index f0893de558..30cc169b88 100644 --- a/tests/Unit/Service/Rbac/ViewerReachResolverTest.php +++ b/tests/Unit/Service/Rbac/ViewerReachResolverTest.php @@ -29,6 +29,7 @@ namespace Unit\Service\Rbac; +use OCA\OpenRegister\Exception\ViewAlertRecipientException; use OCA\OpenRegister\Service\Rbac\ViewerReach; use OCA\OpenRegister\Service\Rbac\ViewerReachResolver; use OCP\IGroupManager; @@ -241,4 +242,58 @@ public function testUserSharesAreCheckedAgainstTheUserManager(): void { $this->assertSame('share.unknown-user', $withUsers->shareFindings(sharedWith: [['user' => 'nobody', 'mode' => 'read']])[0]['code']); $this->assertSame('share.unknown-user', $this->resolver->shareFindings(sharedWith: [['user' => 'fatma', 'mode' => 'read']])[0]['code']); }//end testUserSharesAreCheckedAgainstTheUserManager() + /** + * 🔴 A SAVE THAT NARROWS REACH REFUSES THE STORED RECIPIENTS IT STOPS REACHING. + * + * Stored recipients are not judged as additions and an update with no + * `alert` key declares none, so un-publishing the view left pieter on an + * alert about a view he can no longer open. + * + * @return void + */ + public function testASaveThatUnpublishesTheViewRefusesTheStoredRecipientsItStopsReaching(): void { + $this->groupManager->method('isAdmin')->willReturn(false); + $this->groupManager->method('isInGroup')->willReturn(false); + + $stored = ['owner' => 'annemarie', 'isPublic' => true, 'sharedWith' => [], 'alert' => ['recipients' => ['user:pieter']]]; + + try { + $this->resolver->assertMayNameRecipients( + view: $stored, + reach: new ViewerReach(userId: 'annemarie', groups: [], isAdmin: false), + data: ['isPublic' => false] + ); + $this->fail('Expected the narrowing save to be refused'); + } catch (ViewAlertRecipientException $e) { + $this->assertSame(['user:pieter'], $e->getRecipients()); + } + + // The control: a save that does not narrow reach is untouched. + $this->resolver->assertMayNameRecipients( + view: $stored, + reach: new ViewerReach(userId: 'annemarie', groups: [], isAdmin: false), + data: ['name' => 'Renamed'] + ); + $this->addToAssertionCount(1); + }//end testASaveThatUnpublishesTheViewRefusesTheStoredRecipientsItStopsReaching() + + /** + * Removing the share a group recipient reached the view through is refused too. + * + * @return void + */ + public function testRemovingTheShareAStoredGroupRecipientReachesTheViewThroughIsRefused(): void { + $this->expectException(ViewAlertRecipientException::class); + + $this->resolver->assertMayNameRecipients( + view: [ + 'owner' => 'annemarie', + 'isPublic' => false, + 'sharedWith' => [['group' => 'sales', 'mode' => 'read']], + 'alert' => ['recipients' => ['sales']], + ], + reach: new ViewerReach(userId: 'annemarie', groups: [], isAdmin: false), + data: ['sharedWith' => []] + ); + }//end testRemovingTheShareAStoredGroupRecipientReachesTheViewThroughIsRefused() }//end class