fix(change_request): run auto-apply through the public action_apply - #461
Conversation
Requiring change-request manager rights to apply meant auto-apply-on- approve was routed to the internal mechanism instead, so that the approver could be a validator rather than a manager. But action_apply is the extension point modules override to hang post-apply work off an apply, and bypassing it left those overrides silently not running on approval — no error, just missing side effects, which is the hardest kind of regression to notice. Auto-apply now calls action_apply under sudo(), which the manager gate already exempts (env.su), so the authorization boundary is unchanged for RPC callers. sudo() sets su without changing uid, so applied_by_id still records the real approver — asserted by a test. The two guarantees from the original change are kept and still covered: a validator cannot call action_apply directly, and approving still applies.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #461 +/- ##
==========================================
- Coverage 76.18% 76.02% -0.16%
==========================================
Files 661 627 -34
Lines 44109 43024 -1085
==========================================
- Hits 33603 32709 -894
+ Misses 10506 10315 -191
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
The rationale is already in the comment above it — the manager gate exempts env.su, and sudo() preserves uid so attribution is unaffected — but semgrep's odoo-sudo-without-context needs the marker to see it.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
The substance is right and I'd approve it as-is on behavior — requesting changes only for two docstrings this PR makes actively false, both trivial. Fix those and this is an approve.
Verified on the substance:
- The contract regression is real:
action_applyis the documented extension point, and routing auto-apply to_apply_change_requestsilently skipped every downstream override on approval. Going back through the public entry point undersudo()is the right shape, and the gate'senv.suexemption was designed for exactly this. - No authorization widening: the strategy already ran under
sudo()on the old path (_do_apply), and the workflow authorizes the apply either way. A validator still cannot callaction_applydirectly — the existing tests for both #365 guarantees still pass. - Attribution holds:
sudo()keepsuid, soapplied_by_idrecords the approver, andtest_auto_apply_records_the_real_approverpins it — good instinct, that was the easy thing to get wrong. - The spy test correctly patches at the class, so it sees the call through the sudo'd recordset.
- Version
19.0.3.1.12assumes #459's3.1.11lands first; HISTORY will conflict textually at rebase since both edit the same region — expected for this chain, just flagging the merge-order dependency stands.
One observation, no change requested: downstream action_apply overrides now run under su on the auto-apply path but as the manager on manual apply. That's inherent to the design and the right trade (post-apply work must not fail on the approval path for ACL reasons), but override authors should know both modes exist — the updated docstring could say so.
| # internal mechanism keeps ``action_apply`` the single extension | ||
| # point for apply -- downstream modules override it to hang | ||
| # post-apply work off the apply, and routing around it left those | ||
| # overrides silently not running on approval. ``sudo()`` sets |
There was a problem hiding this comment.
Must fix (docs): action_apply's own docstring (line 1458, unchanged by this diff so commenting here) is now false — it still says the auto-apply-on-approve path "invokes _apply_change_request directly". After this PR it arrives at action_apply as a superuser caller. Since that paragraph documents the authorization boundary, it should describe the new reality: superuser callers are exempt, and auto-apply-on-approve is one of them (via this sudo() call, authorized by the approval workflow). Worth adding a line that overrides of action_apply therefore run under su on the auto-apply path and as the clicking manager on the manual path.
There was a problem hiding this comment.
Fixed in 35d4103. The paragraph now says superuser (env.su) callers are exempt and that auto-apply-on-approve is one of them, reaching action_apply through sudo() from _on_approve, authorized by the approval workflow — so the approver may be a validator rather than a manager.
Added your suggested point too: action_apply is the extension point, so an override runs on both paths — under su when auto-applied on approval, and as the manager who clicked Apply on the manual path. Noted alongside it that sudo() sets su without changing uid, so applied_by_id records the approver rather than the superuser (there is a test pinning that).
|
|
||
| def test_auto_apply_on_approve_runs_for_non_manager_approver(self): | ||
| """Auto-apply-on-approve must still work when the approver is a | ||
| validator (not a manager): _on_approve routes through the internal |
There was a problem hiding this comment.
Must fix (docs): stale — _on_approve no longer routes through the ungated internal mechanism; it goes through the public action_apply under sudo(), which the manager gate exempts. The test's assertion is still exactly right (a validator approver must work); only the explanation of why has changed.
There was a problem hiding this comment.
Fixed in 35d4103. Rewritten to say _on_approve reaches the public action_apply through sudo(), and the manager gate exempts superuser callers.
Agreed the assertion itself was already correct — only the stated reason had gone stale, which is the kind of thing that survives indefinitely because nothing fails when it drifts.
Two explanations were left describing the behaviour this branch replaced, both flagged in review. action_apply's docstring still said auto-apply-on-approve "invokes _apply_change_request directly". It now arrives at action_apply as a superuser caller instead, and since that paragraph documents the authorization boundary it has to describe what actually happens. Also notes that action_apply is the extension point, so an override runs under su on the auto-apply path and as the clicking manager on the manual one, with applied_by_id recording the approver either way. The docstring on test_auto_apply_on_approve_runs_for_non_manager_approver said _on_approve routes through the ungated internal mechanism. The assertion it makes is unchanged and still right — a validator approver must work — but the reason is now that the manager gate exempts superuser callers. Documentation only; no behaviour change.
gonzalesedwin1123
left a comment
gonzalesedwin1123
left a comment
There was a problem hiding this comment.
Approving — both docstrings now describe the real mechanism, and the action_apply one additionally documents the two execution modes for override authors (su on auto-apply, the clicking manager on manual) with the uid-preservation guarantee spelled out. That's exactly the paragraph a downstream developer needs. CI fully green.
Merge order reminder: after #459 (3.1.11), this takes 3.1.12; expect the same squash-induced HISTORY/manifest conflict #471 just had — mechanical to resolve.
#459 was squash-merged as 481cc27, conflicting with this branch in the version line, HISTORY, and the generated README/index.html. Resolved to 19.0.3.1.12 with both HISTORY entries stacked (3.1.12 over 3.1.11), and the README/index.html regenerated from the resolved fragments with the repo's pinned oca-gen-addon-readme hook.
#459 and #461 were squash-merged (481cc27, ac0e38e), conflicting with this branch in the version line, HISTORY, tests/__init__.py, and the generated README/index.html. Resolved to 19.0.3.1.13 with the HISTORY entries stacked (3.1.13 over 3.1.12 over 3.1.11), both test imports kept, and README/index.html regenerated from the resolved fragments with CI's table-width rendering applied.
3 participants
action_applyis the extension point modules override to hang post-apply work off an apply. Auto-apply-on-approve stopped going through it, so those overrides silently stopped running.What happened
#365 gated
action_applyon the change-request manager role, because it runs the apply strategy undersudo()and is reachable over RPC. Auto-apply was then routed to the internal mechanism so the approver could be a validator rather than a manager:That solved the authorization problem but changed the contract. Any module overriding
action_apply— the natural place to react to an apply — keeps working when a manager clicks Apply, and quietly does nothing when the same request is auto-applied on approval. No error, no log; only the side effects go missing.The fix
The manager gate already exempts
env.su, and says so:So auto-apply can use the public entry point:
sudo()sets superuser mode without changing the user, soapplied_by_idstill records the real approver — there is a test for that, since it would be an easy thing to get wrong.What is unchanged
Both guarantees from #365 hold, and both already had tests that still pass:
action_applydirectly is still refused (test_validator_cannot_apply_directly);test_auto_apply_on_approve_runs_for_non_manager_approver).The authorization boundary for RPC callers is untouched:
env.suis false there.Trade-off
Downstream overrides now run with
su=True. The apply itself already runs undersudo()via_do_apply, so this widens privilege only for post-apply side effects. The alternative — a separate_after_apply()hook called by both paths — separates authorization from extension more cleanly, but requires every consumer to move their override, which is the cost this change exists to avoid.Tests
Two added: auto-apply goes through
action_apply(spied), and the applying user is recorded as the approver rather than the superuser. Fullspp_change_request_v2suite: 397 tests, 0 failures.Merge order
19.0.3.1.10 → 19.0.3.1.12, leaving3.1.11to #459. Independent of #459 and #454 in content; only the version and changelog would conflict, and only if merged out of order.