fix(jobs): the owner lookup must bypass RBAC, or it can never find the owner - #222
fix(jobs): the owner lookup must bypass RBAC, or it can never find the owner#222rubenvdlinde wants to merge 1 commit into
Conversation
…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.
Quality Report — ConductionNL/openbuild @
|
| 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.
|
Superseded by #229 — closing. Verified, not assumed: the one-line fix in this PR is present byte-for-byte on #229's head. $object = $service->find($objectId, _rbac: false, _multitenancy: false);together with the same rationale comment, and a leaner unit test covering the same behaviour. Why this PR could not go green on its own: its eight failing jobs are its base, not its diff. It was cut from an older Nothing is lost by closing — the fix and its diagnosis both ship in #229. |
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: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 asAnonymousand refused by any schema that does not grant anonymousread.It is a chicken-and-egg, not a permission decision: you cannot read the object to learn its owner without already being someone.
The
failtransition that should have recorded why is refused for the same reason — which is why the object never even reachesfailed. It just sits atqueued, 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: falseto one read, and the reasoning matters because opt-outs like this are exactly what the fleet has been bitten by before:getOwner().$workis RBAC-checked against them. Failing this lookup does not deny the write — it runs the job asAnonymous, which is the weaker identity._multitenancyis 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
willReturnCallbacksees 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 resolvesOCA\OpenBuild\to the installed app no matter where the test lives (confirmed withReflectionClass::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
exportJob's declared schema version so instances converge.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