fix(jobs): the owner lookup must bypass RBAC, or it can never find the owner - #222
Open
rubenvdlinde wants to merge 1 commit into
Open
fix(jobs): the owner lookup must bypass RBAC, or it can never find the owner#222rubenvdlinde wants to merge 1 commit into
rubenvdlinde 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.
Contributor
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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