Skip to content

fix(multitenancy): guard empty orX() in applyActiveOrgFilter — silently-empty object-source reads (fixes #2084) - #2088

Merged
rubenvdlinde merged 1 commit into
developmentfrom
fix/multitenancy-empty-orx
Jul 24, 2026
Merged

fix(multitenancy): guard empty orX() in applyActiveOrgFilter — silently-empty object-source reads (fixes #2084)#2088
rubenvdlinde merged 1 commit into
developmentfrom
fix/multitenancy-empty-orx

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Summary

Fixes #2084.

MultiTenancyTrait::applyActiveOrgFilter() always constructed
$qb->expr()->orX() with zero initial arguments and relied on every
downstream code path to ->add() at least one predicate afterwards.
OCP's IExpressionBuilder::orX() docblock documents that a zero-argument
call "requires at least one defined when converting to string", and this
NC34 instance already logs a deprecation Exception on every such call,
warning that it "will throw soon" in a future release.

  • buildOrganisationConditions() (replacing the former
    addOrganisationConditions()) now returns a plain array of predicates
    instead of mutating a composite expression in place.
  • applyActiveOrgFilter() only calls $qb->expr()->orX(...$predicates)
    once at least one real predicate exists.
  • If the predicate list is ever empty (an invariant break — today's only
    caller, applyOrganisationFilter(), already guards against an empty
    $activeOrgUuids before reaching this method), the guard fails
    CLOSED with an unconditional 1 = 0 rather than calling orX()
    with no arguments, or skipping the WHERE clause and failing open.
    This matches the tenant-isolation guarantee in
    openspec/specs/tenant-isolation-audit/spec.md ("the system MUST
    verify that no Organisation's query filter returns objects belonging to
    another Organisation") and mirrors the existing no-active-org behaviour
    in applyNoActiveOrgFilter().

Root-cause note (see issue for full investigation)

Live-reproduced on the shared dev instance: flipping
multitenancy to {"saasMode":true,"adminOverride":true} and querying
the v-app-market dbal-source (x-openregister-object-source) schema
under register 2479 returned total: 0 instead of real rows. The
nextcloud.log trace showed the orX() zero-arg deprecation warning
firing repeatedly (confirming this defect is real and present), but the
actual emptiness came from a second, related defect one layer up:
DbalObjectSourceProvider::resolveSource() calls
SourceMapper::findAll(filters: ['uuid' => $sourceId]), which applies
the same organisation-scoped filter (with allowNullOrg: false) to the
shared Source entity. Because the spectr intelligence-db Source
row's own organisation differs from the querying admin's active
organisation, SaaS mode (which disables admin override) excludes the
Source row entirely, resolveSource() returns null, and every
downstream find/findAll/count/aggregate on that schema silently
returns empty with only a warning-level log line
([ObjectSource:dbal-source] no source resolved for schema ...) — no
exception ever surfaces.

This PR fixes the orX() defect exactly as reported and adds a
regression test for it
, but does not by itself restore dbal-source
results while saasMode is true, because that second defect (shared
system Source rows being subject to per-tenant org filtering at all) is
a separate design question. Filing a follow-up issue for that; the
{"saasMode":false,"adminOverride":true} workaround should stay in place
on the shared instance until it's resolved.

Test plan

  • php -l lib/Db/MultiTenancyTrait.php
  • composer phpcs (project-wide, lib/ scope) — clean, 0 errors
  • New regression test tests/Unit/Db/MultiTenancyTraitApplyActiveOrgFilterTest.php:
    drives applyActiveOrgFilter() directly via reflection with an
    empty active-org list, asserts orX() is never invoked (let alone
    with zero args) and the query fails closed (andWhere('1 = 0'));
    a second test confirms the happy path still builds a real orX()
    filter with predicates.
  • Full unit suite (composer test:unit, 15149 tests) run for
    regressions — the 111 errors / 18 failures present are pre-existing
    and unrelated to this change (Maps/Photos/Polls deep-link URL
    format, SqlTypeMapper, SchemaLinkedTypes InvalidArgumentException
    assertions); none reference MultiTenancyTrait,
    applyActiveOrgFilter, or WebhookMapper.

…ly-empty object-source reads (fixes #2084)

MultiTenancyTrait::applyActiveOrgFilter() always called $qb->expr()->orX()
with zero initial arguments and relied on every caller adding at least one
predicate afterwards. OCP's IExpressionBuilder::orX() docblock documents
that a zero-argument call "requires at least one defined when converting
to string", and NC34 already logs a deprecation exception on every such
call, flagging that it will throw in a future release.

Build the organisation predicates into a plain array first
(buildOrganisationConditions(), replacing addOrganisationConditions()) and
only construct orX(...$predicates) once real predicates exist. If the
predicate list is ever empty — an invariant break, since the only caller
already guards against an empty $activeOrgUuids — fail CLOSED with an
unconditional `1 = 0` instead of calling orX() with no arguments or
skipping the WHERE clause, per the tenant-isolation guarantee in
openspec/specs/tenant-isolation-audit/spec.md ("the system MUST verify
that no Organisation's query filter returns objects belonging to another
Organisation").

Adds a regression test driving applyActiveOrgFilter() directly (via
reflection) with an empty active-org list, asserting orX() is never
called with zero arguments and the query fails closed.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ f2eb9b7

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

Quality workflow — 2026-07-24 20:05 UTC

Download the full PDF report from the workflow artifacts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant