Skip to content

fix(jobs): the owner lookup must bypass RBAC, or it can never find the owner - #222

Open
rubenvdlinde wants to merge 1 commit into
developmentfrom
fix/job-owner-impersonation-rbac
Open

fix(jobs): the owner lookup must bypass RBAC, or it can never find the owner#222
rubenvdlinde wants to merge 1 commit into
developmentfrom
fix/job-owner-impersonation-rbac

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

The defect

Every export on the dev instance sat at status: queued. The reason is in the log, once you catch a run that actually executes the job:

OpenBuild: owner impersonation lookup failed for object <uuid>:
  User 'Anonymous' does not have permission to 'read' objects in schema 'Export Job'
OpenBuild export: lifecycle transition "start" failed on job <uuid>: <same>
OpenBuild export failed
OpenBuild export: lifecycle transition "fail" failed on job <uuid>: <same>

JobOwnerImpersonator::impersonate() reads the object to discover who to impersonate. That read necessarily runs before the impersonation, so the caller is still the background job's session — nobody. An RBAC-checked read is evaluated as Anonymous and refused by any schema that does not grant anonymous read.

It is a chicken-and-egg, not a permission decision: you cannot read the object to learn its owner without already being someone.

The fail transition that should have recorded why is refused for the same reason — which is why the object never even reaches failed. It just sits at queued, looking exactly like a job nobody picked up. That ambiguity is what made this expensive to find.

Why the opt-out is safe

This adds _rbac: false, _multitenancy: false to one read, and the reasoning matters because opt-outs like this are exactly what the fleet has been bitten by before:

  • The id is not user input — it is the argument the pipeline enqueued for a job it created.
  • Exactly one field is consumed from the result: getOwner().
  • The outcome is strictly more restrictive, not less. The work then runs as that owner, and every write inside $work is RBAC-checked against them. Failing this lookup does not deny the write — it runs the job as Anonymous, which is the weaker identity.
  • _multitenancy is off for the same structural reason: a session-less job has no organisation context to scope by.

Fixes the same defect for all three call sites: ExportJobService, RuleActionDispatcher, DocumentGenerationService.

About the test

It uses a hand-written fake rather than a PHPUnit mock, deliberately. The production call passes named arguments, and a mock cannot observe those: when named arguments skip intermediate positions, PHP hands the generated mock only the positional ones, so a willReturnCallback sees its own defaults (_rbac => true) whether or not the fix is present. That test would pass on broken code and fail on fixed code — worse than no test.

Verification status, stated precisely: the test's failing arm is proven — run against the unfixed deployed class it reports _rbac => true, _multitenancy => true. Its passing arm is left to CI, because an in-container run resolves OCA\OpenBuild\ to the installed app no matter where the test lives (confirmed with ReflectionClass::getFileName()), and the installed copy is a shared checkout I must not edit. CI checks the repo out as the installed app, so it exercises this code directly.

Related

These are three independent reasons an export could not complete. This one bites every instance, including a fresh CI one; the other two bite instances whose schema version has drifted.

🤖 Generated with Claude Code

…e owner

Every export on the dev instance sat at `status: queued`. The reason is in the
log, once you catch a run that actually executes the job:

  OpenBuild: owner impersonation lookup failed for object <uuid>:
  User 'Anonymous' does not have permission to 'read' objects in schema
  'Export Job'
  OpenBuild export: lifecycle transition "start" failed on job <uuid>: <same>
  OpenBuild export failed
  OpenBuild export: lifecycle transition "fail" failed on job <uuid>: <same>

`JobOwnerImpersonator::impersonate()` reads the object to discover WHO to
impersonate. That read necessarily runs BEFORE the impersonation, so the
caller is still the background job's session — nobody. An RBAC-checked read is
therefore evaluated as `Anonymous` and refused by any schema that does not
grant anonymous `read`. It is a chicken-and-egg, not a permission decision:
you cannot read the object to learn its owner without already being someone.

The `fail` transition that should have recorded why is refused for the same
reason, which is why the object never even reaches `failed` — it just sits at
`queued` looking like a job nobody picked up.

Why the opt-out is safe rather than a hole: the id is not user input (it is
the argument the pipeline enqueued for a job it created), exactly one field is
consumed from the result (`getOwner()`), and the outcome is strictly MORE
restrictive — the work then runs AS that owner and every write inside is
RBAC-checked against them. Failing this lookup does not deny the write; it
runs the job as Anonymous, the weaker identity.

Fixes the same defect for all three call sites: ExportJobService,
RuleActionDispatcher and DocumentGenerationService.

⚠️ The test uses a hand-written fake rather than a PHPUnit mock, deliberately.
The production call passes NAMED arguments, and a mock cannot observe those —
when named arguments skip intermediate positions PHP hands the generated mock
only the positional ones, so a willReturnCallback sees its own defaults
(`_rbac => true`) whether or not the fix is present. That test would pass on
broken code and fail on fixed code.

Verified: the test's FAILING arm is real — run against the unfixed deployed
class it reports `_rbac => true, _multitenancy => true`. Its passing arm is
left to CI, because an in-container run resolves OCA\OpenBuild\ to the
INSTALLED app no matter where the test lives (confirmed with
ReflectionClass::getFileName()), and the installed copy is a shared checkout I
must not edit.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openbuild @ 61a3faa

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-manifest
test-l10n
check-gitignore
check-nc-floor
format
composer ✅ 106/106
npm ✅ 626/626
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright
Hydra gates

Quality workflow — 2026-08-16 11:41 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