fix(change_request): run auto-apply through the public action_apply - #461
Open
kneckinator wants to merge 2 commits into
Open
fix(change_request): run auto-apply through the public action_apply#461kneckinator wants to merge 2 commits into
kneckinator wants to merge 2 commits into
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 75.93% 75.93% -0.01%
==========================================
Files 627 627
Lines 43000 42993 -7
==========================================
- Hits 32654 32647 -7
Misses 10346 10346
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.
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
2 participants
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.
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.