fix(spp_programs): close membership ACL bypass and approval/payment bugs#241
fix(spp_programs): close membership ACL bypass and approval/payment bugs#241kneckinator wants to merge 1 commit into
Conversation
Address four issues surfaced by a security scan of recent performance
refactors in spp_programs:
- Gate bulk_create_memberships with check_access("create") so this public,
RPC-callable helper no longer lets a low-privileged user bypass ACLs via
its raw INSERT ... ON CONFLICT path or the sudo() ORM path
(cycle_membership, program_membership).
- Treat NULL-state entitlements as unapproved in
_compute_all_entitlements_approved (state IS DISTINCT FROM 'approved'), so
a cycle containing a NULL-state entitlement is no longer reported as fully
approved.
- Evaluate the fund balance per program in approve_entitlements and guard the
empty recordset, so a mixed-program batch no longer approves entitlements
against the wrong program's funds and no longer crashes on an empty set
(DefaultCashEntitlementManager and SppCashEntitlementManager).
- Populate the payment batch's payment_ids when assigning batch_id in
_prepare_payments, so generated batches list their payments.
Add regression tests covering each fix.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses several security and functional issues: it fixes an ACL bypass in bulk membership creation by adding explicit create access checks, ensures NULL entitlement states do not bypass approval checks by using IS DISTINCT FROM in SQL queries, isolates fund balance checks per program in mixed-program entitlement batches, and correctly populates the independent payment_ids Many2many field during batch payment creation. However, the reviewer identified a critical issue where self.check_access("create") is used instead of the standard Odoo method self.check_access_rights("create") in both cycle_membership.py and program_membership.py, which will cause runtime AttributeError exceptions.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| # raw SQL that bypasses the ORM's ACL checks. Enforce model-level create | ||
| # access explicitly so a low-privileged user cannot use it to create | ||
| # memberships they could not create through the ORM. | ||
| self.check_access("create") |
There was a problem hiding this comment.
In Odoo, there is no standard check_access method on models. The standard and correct method to check model-level access rights is check_access_rights. Using check_access will raise an AttributeError at runtime.
| self.check_access("create") | |
| self.check_access_rights("create") |
| # ORM's ACL checks. Enforce model-level create access explicitly so a | ||
| # low-privileged user cannot use it to create memberships they could not | ||
| # create through the ORM. | ||
| self.check_access("create") |
There was a problem hiding this comment.
In Odoo, there is no standard check_access method on models. The standard and correct method to check model-level access rights is check_access_rights. Using check_access will raise an AttributeError at runtime.
| self.check_access("create") | |
| self.check_access_rights("create") |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 19.0 #241 +/- ##
========================================
Coverage 73.05% 73.06%
========================================
Files 1069 1070 +1
Lines 62080 62368 +288
========================================
+ Hits 45351 45567 +216
- Misses 16729 16801 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
1 participant
Summary
Fixes four issues surfaced by a security scan of recent performance refactors in
spp_programs. Each fix has a regression test.bulk_create_membershipsis a public, RPC-callable@api.modelmethod whoseskip_duplicates=Truepath runs rawINSERT ... ON CONFLICTand whose program-membership ORM path runs throughsudo()— both bypassing ACL checks, letting a low-privileged user create memberships (incl.enrolled) they couldn't create via the ORM.self.check_access("create")at entry, so all paths enforce model-level create rights._compute_all_entitlements_approvedusedstate != 'approved'; in SQLNULL != 'approved'is UNKNOWN, so a NULL-state (unapproved) entitlement was excluded and the cycle was reported as fully approved.state IS DISTINCT FROM 'approved'.approve_entitlementsfetched the balance once viaentitlements[0]and reused it for the whole batch — a mixed-program batch was checked against the wrong program's funds, and an empty recordset crashed onentitlements[0].program_id(one check per program, optimization preserved) and return cleanly for an empty recordset. Applied to bothDefaultCashEntitlementManagerandSppCashEntitlementManager._prepare_paymentsonly wrotebatch_id; sincespp.payment.batch.payment_idsis an independent Many2many (not the inverse ofbatch_id), generated batches displayed/iterated zero payments.curr_batch.payment_idswhen assigning the batch.Tests
New regression tests (all passing locally, with the relevant existing suites green):
test_membership_acl_bypass.py— low-priv user denied on both paths; an officer with create rights still succeeds.test_cycle_null_entitlement_approval.py— a NULL-state entitlement is treated as unapproved.test_approve_entitlements_program_isolation.py— empty recordset no longer crashes; each entitlement is checked against its own program.test_payment_batch_payment_ids.py— generated batches list their payments.Verified via
./scripts/test_single_module.sh spp_programson the affected + neighbouring suites: 60 passed, 0 failed, 0 errors.Notes / out of scope
payment_idsremains an independent Many2many (not converted to a true inverse), so any other code that setsbatch_iddirectly must likewise updatepayment_ids.