security(programs): enforce system-admin authorization on Force Unlock#336
Draft
gonzalesedwin1123 wants to merge 9 commits into
Draft
security(programs): enforce system-admin authorization on Force Unlock#336gonzalesedwin1123 wants to merge 9 commits into
gonzalesedwin1123 wants to merge 9 commits into
Conversation
TDD red: action_force_unlock on spp.cycle/spp.program has no server-side authorization, so officers, managers and cycle approvers (who hold write ACL) can clear an active operation lock via RPC. New tests assert the call raises AccessError for those roles and leaves the lock set, while system admins and trusted sudo() flows still succeed.
action_force_unlock on spp.cycle and spp.program cleared an active operation lock (is_locked) with no server-side authorization. The view buttons are gated to base.group_system, but object methods are reachable via RPC/call_kw regardless of button visibility, and officers, managers and cycle approvers hold write ACL on these models - so any of them could force-clear a lock while async entitlement/payment/eligibility jobs were still running, enabling conflicting operations and integrity/availability issues. - guard both methods with base.group_system (fail fast, before the loop), mirroring the existing has_group/AccessError idiom in cycle.py; exempt self.env.su so trusted server-side sudo() flows are not blocked - correct the misleading 'manager-only' docstrings and test comment to system-administrator-only - no ACL change (write is legitimate for normal cycle/program work; the method is the exposure) and no migration (runtime check, no schema/data change) - bump to 19.0.2.2.1 with HISTORY fragment
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Member
Author
|
Scope follow-up filed: #337 — the async |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 19.0 #336 +/- ##
==========================================
- Coverage 74.28% 71.42% -2.86%
==========================================
Files 372 211 -161
Lines 25385 13509 -11876
==========================================
- Hits 18857 9649 -9208
+ Misses 6528 3860 -2668 Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
Follows the staff review of the initial method-only guard: is_locked /
locked_reason were plain writable fields, so officers/managers/approvers
(who hold write ACL) could bypass the action_force_unlock guard entirely
with a direct write({'is_locked': False}) over RPC.
- add a write() guard on spp.cycle and spp.program rejecting direct
changes to is_locked/locked_reason unless self.env.su or the caller is
base.group_system; this is the actual sink behind Force Unlock
- route the async pipeline's lock acquire/release through new
_acquire_operation_lock/_release_operation_lock helpers that sudo(), so
the ~25 manager call sites (entitlement/payment/eligibility/cycle) keep
working when the job runs as the initiating non-admin user
- action_force_unlock keeps its base.group_system method guard (its write
runs in admin context and passes the new write() guard)
- tests: direct write({'is_locked': ...}) by officer/manager/approver
raises AccessError and the lock stays; admin/sudo succeed; the sudo
helpers still work for a non-admin operating user; non-lock field edits
by a manager are unaffected
- HISTORY updated to describe the field-level boundary
The mark_*_as_done/_failed completion callbacks remain callable via RPC
(they must run as the operating user) and are tracked separately in #337.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
The _acquire/_release_operation_lock helpers sudo() the write of the two lock fields because the write() guard restricts them to system admins while the async pipeline runs as the initiating non-admin user. Suppress odoo-sudo-without-context with justification (sudo is scoped to the two lock fields only).
Staff review of Option A found the pipeline conversion was incomplete: I
had only converted dict-style .write({'is_locked':...}) sites in
spp_programs/models/managers, missing attribute-assignment writes and
dependent modules. The new spp.program/spp.cycle write() guard would
raise AccessError for the non-admin operating user at those sites,
leaving programs stuck-locked or breaking async imports.
- spp_programs eligibility_manager.mark_import_as_done: the on_done
callback cleared the lock via self.program_id.is_locked = False
(attribute assignment) — now uses _release_operation_lock()
- spp_program_geofence (released): its _import_registrants_async /
mark_import_as_done wrote the spp.program lock directly and broke for
non-admin imports under the guard; routed through the inherited
helpers; version 19.0.1.0.0 -> 19.0.1.0.1 + HISTORY
- spp_farmer_registry_demo (released): demo generator cleared the cycle
lock directly (works only because demo runs as admin) — converted to
the helper to remove the latent trap; version 19.0.2.1.1 -> 19.0.2.1.2
+ HISTORY
- add a regression test driving the real eligibility mark_import_as_done
as a non-admin officer (would have caught the miss)
Verified: exhaustive grep shows the only remaining direct lock writes on
spp.cycle/spp.program are the two admin-guarded action_force_unlock
methods. Suites: spp_programs 693, spp_program_geofence 41,
spp_farmer_registry_demo 132 — all green.
Staff-review nit: the callback-vector lock-release residual (public mark_* methods can clear the lock via RPC) was disclosed only in a commit message. Add a NOTE(#337) comment by the helpers so a maintainer reading the source sees the known, tracked gap.
This was referenced Jul 24, 2026
Draft
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.
Summary
The async operation lock (
is_locked/locked_reasononspp.cycleandspp.program) protects in-flight entitlement / payment / eligibility jobs, and the Force Unlock buttons are gated tobase.group_systemin the views. But there was no server-side enforcement:action_force_unlockdid no group check, and — more fundamentally —is_locked/locked_reasonwere plain writable fields. Becauseir.model.access.csvgrants write on both models togroup_programs_officer,group_programs_manager, andgroup_programs_cycle_approver, any of those roles could clear an active lock via RPC — either by callingaction_force_unlock, or simply:…enabling conflicting operations and the integrity/availability risks in benefit processing (double entitlements/payments, racing eligibility recompute). Shipped in released tag
2026.07.Fix — the lock is now a real server-side boundary
write()guard onspp.cycleandspp.program: a direct write tois_locked/locked_reasonraisesAccessErrorunlessself.env.suor the caller holdsbase.group_system. This closes the direct-write sink, not just the labeled button.action_force_unlockkeeps itsbase.group_systemmethod guard (its write runs in admin context and passes thewrite()guard)._acquire_operation_lock/_release_operation_lockhelpers thatsudo(), so the ~25 legitimate acquire/release sites keep working when a job runs as the initiating (non-admin) user. All lock writes were lock-only dicts, so thesudo()touches nothing else.base.group_system, matching the view gating and the report's intent (managers are explicitly excluded). Stale "manager-only" docstrings/comment corrected.19.0.2.2.0 → 19.0.2.2.1+HISTORY.mdfragment.Tests (TDD)
New
spp_programs/tests/test_force_unlock_authz.py:action_force_unlockand a directwrite({'is_locked': ...})raiseAccessErrorfor officer / manager / cycle approver — and the lock stays set; setting the lock out of band is blocked too.base.group_systemuser (andsudo()) succeed via both paths.sudo()helpers still release/acquire the lock for a non-admin operating user (pipeline regression).spp_programs692/692 (0 failed); ruff/ruff-format/pylint-odoo clean.History of this PR
The first revision guarded only
action_force_unlock. An independent staff review flagged that the plain-field direct write bypassed it entirely — our own "guard every sender-reachable path to the same sink" pitfall. This revision closes the field sink and makes the lock an enforced boundary.Residual, tracked separately
The async completion callbacks
mark_*_as_done/mark_*_as_failedare public and RPC-reachable and (now via thesudo()helper) clear the lock; they must run as the non-admin operating user, so they can't take thebase.group_systemgate and need a job-identity fix instead. Tracked in #337 — this PR does not close that path, and does not claim to.Footprint note (Option A ripple into dependent modules)
Because the
write()boundary lives onspp.program/spp.cycle, every writer of the lock fields repo-wide must go through the sudo helpers, or it hits the guard. A staff review caught that the first Option-A revision missed several — now fixed, which grows this PR to three modules:spp_programs— main fix; also convertedeligibility_manager.mark_import_as_done, which cleared the lock via attribute assignment (self.program_id.is_locked = False) in theon_donecallback that runs as the non-admin operating user.spp_program_geofence(released, 19.0.1.0.0 → 19.0.1.0.1) — its async import acquired/released thespp.programlock directly and would have raisedAccessErrorfor a non-admin import under the new guard. Routed through the inherited helpers.spp_farmer_registry_demo(released, 19.0.2.1.1 → 19.0.2.1.2) — the demo generator cleared the cycle lock directly; it only worked because demo generation runs as admin. Converted to the helper to remove the latent trap.Exhaustive grep confirms the only remaining direct writes to
is_locked/locked_reasononspp.cycle/spp.programare the two admin-guardedaction_force_unlockmethods. (spp_areaandspp_demohave their ownis_lockedfields on unrelated models — not affected.) Suites green:spp_programs693,spp_program_geofence41,spp_farmer_registry_demo132.Merge order
Per the cross-PR interaction analysis (2026-07-24,
internal/plans/security-prs-interaction-analysis.md): this PR merges before #341 onspp_farmer_registry_demo(this PR takes19.0.2.1.2; #341 already reserves.1.3). If the order is ever reversed, this PR must renumber to.1.4. After this PR lands, #341 flips to CONFLICTING (metadata-only: manifest/HISTORY/README) and silently stops getting CI — re-merge it immediately. Semantics are complementary: #341's non-admin demo users keep working under this PR's write() guard precisely because this PR converts the demo generator's lock writes to the sudo helper. Also note: soft preference to land #327/#329 (admin-group de-escalations) in the same release — until they land, dci/key admins still transitbase.group_systemand pass this PR's new force-unlock gate.