diff --git a/spp_programs/models/cycle.py b/spp_programs/models/cycle.py index a7b0052c..480d725a 100644 --- a/spp_programs/models/cycle.py +++ b/spp_programs/models/cycle.py @@ -461,10 +461,10 @@ def _compute_all_entitlements_approved(self): self.env.cr.execute( """ SELECT DISTINCT cycle_id FROM spp_entitlement - WHERE cycle_id IN %s AND state != 'approved' + WHERE cycle_id IN %s AND state IS DISTINCT FROM 'approved' UNION SELECT DISTINCT cycle_id FROM spp_entitlement_inkind - WHERE cycle_id IN %s AND state != 'approved' + WHERE cycle_id IN %s AND state IS DISTINCT FROM 'approved' """, (cycle_ids, cycle_ids), ) diff --git a/spp_programs/models/cycle_membership.py b/spp_programs/models/cycle_membership.py index 374bdfa8..8d0cc883 100644 --- a/spp_programs/models/cycle_membership.py +++ b/spp_programs/models/cycle_membership.py @@ -130,6 +130,12 @@ def bulk_create_memberships(self, vals_list, chunk_size=1000, skip_duplicates=Fa Returns the count of inserted rows. :return: Recordset (skip_duplicates=False) or int count (skip_duplicates=True) """ + # This is a public (RPC-callable) method whose skip_duplicates path uses + # 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") + if not vals_list: return 0 if skip_duplicates else self.env["spp.cycle.membership"] diff --git a/spp_programs/models/managers/entitlement_manager_base.py b/spp_programs/models/managers/entitlement_manager_base.py index a2d1a4ed..16237445 100644 --- a/spp_programs/models/managers/entitlement_manager_base.py +++ b/spp_programs/models/managers/entitlement_manager_base.py @@ -589,7 +589,13 @@ def approve_entitlements(self, entitlements): :return state_err: Integer number of errors :return message: String description of the errors """ - amt = 0.0 + if not entitlements: + return (0, "") + + # Track approved amount and fund balance per program so a mixed-program + # recordset is evaluated against each entitlement's own program fund. + amt_by_program = {} + fund_balance_by_program = {} # Odoo 19's account.payment expects an "outstanding" account; ensure one exists for the company company = self.env.company if not company.transfer_account_id: @@ -615,14 +621,15 @@ def approve_entitlements(self, entitlements): entitlements.mapped("partner_id") entitlements.mapped("journal_id.currency_id") - # Fetch fund balance once for the whole batch instead of per entitlement - fund_balance = self.check_fund_balance(entitlements[0].cycle_id.program_id.id) - for rec in entitlements: if rec.state in ("draft", "pending_validation"): - remaining_balance = fund_balance - amt + prog_id = rec.cycle_id.program_id.id + # Fetch each program's balance once and reuse it across the batch. + if prog_id not in fund_balance_by_program: + fund_balance_by_program[prog_id] = self.check_fund_balance(prog_id) + remaining_balance = fund_balance_by_program[prog_id] - amt_by_program.get(prog_id, 0.0) if remaining_balance >= rec.initial_amount: - amt += rec.initial_amount + amt_by_program[prog_id] = amt_by_program.get(prog_id, 0.0) + rec.initial_amount # Prepare journal entry (account.move) via account.payment amount = rec.initial_amount new_service_fee = None diff --git a/spp_programs/models/managers/entitlement_manager_cash.py b/spp_programs/models/managers/entitlement_manager_cash.py index c0fb0f34..620fa572 100644 --- a/spp_programs/models/managers/entitlement_manager_cash.py +++ b/spp_programs/models/managers/entitlement_manager_cash.py @@ -394,7 +394,13 @@ def approve_entitlements(self, entitlements): :return state_err: Integer number of errors :return message: String description of the errors """ - amt = 0.0 + if not entitlements: + return (0, "") + + # Track approved amount and fund balance per program so a mixed-program + # recordset is evaluated against each entitlement's own program fund. + amt_by_program = {} + fund_balance_by_program = {} # Odoo 19's account.payment expects an "outstanding" account; ensure one exists for the company company = self.env.company if not company.transfer_account_id: @@ -416,17 +422,18 @@ def approve_entitlements(self, entitlements): entitlements.mapped("partner_id.property_account_payable_id") entitlements.mapped("journal_id.currency_id") - # Fetch fund balance once for the whole batch instead of per entitlement - fund_balance = self.check_fund_balance(entitlements[0].cycle_id.program_id.id) - state_err = 0 message = "" sw = 0 for rec in entitlements: if rec.state in ("draft", "pending_validation"): - remaining_balance = fund_balance - amt + prog_id = rec.cycle_id.program_id.id + # Fetch each program's balance once and reuse it across the batch. + if prog_id not in fund_balance_by_program: + fund_balance_by_program[prog_id] = self.check_fund_balance(prog_id) + remaining_balance = fund_balance_by_program[prog_id] - amt_by_program.get(prog_id, 0.0) if remaining_balance >= rec.initial_amount: - amt += rec.initial_amount + amt_by_program[prog_id] = amt_by_program.get(prog_id, 0.0) + rec.initial_amount # Prepare journal entry (account.move) via account.payment amount = rec.initial_amount new_service_fee = None diff --git a/spp_programs/models/managers/payment_manager.py b/spp_programs/models/managers/payment_manager.py index 12b133e3..50705612 100644 --- a/spp_programs/models/managers/payment_manager.py +++ b/spp_programs/models/managers/payment_manager.py @@ -5,7 +5,7 @@ import logging from uuid import uuid4 -from odoo import _, api, fields, models +from odoo import Command, _, api, fields, models from odoo.exceptions import ValidationError from odoo.addons.job_worker.delay import group @@ -267,6 +267,10 @@ def _prepare_payments(self, cycle, entitlements): } ) batch_payments.write({"batch_id": curr_batch.id}) + # payment_ids is an independent Many2many (not the inverse of + # batch_id), so it must be populated explicitly or the batch + # would display/iterate zero payments. + curr_batch.payment_ids = [Command.set(batch_payments.ids)] if not batches: batches = curr_batch else: diff --git a/spp_programs/models/program_membership.py b/spp_programs/models/program_membership.py index 7e93543f..98c934c8 100644 --- a/spp_programs/models/program_membership.py +++ b/spp_programs/models/program_membership.py @@ -413,6 +413,13 @@ def bulk_create_memberships(self, vals_list, chunk_size=1000, skip_duplicates=Fa raising IntegrityError. Returns the count of inserted rows. :return: Recordset (skip_duplicates=False) or int count (skip_duplicates=True) """ + # This is a public (RPC-callable) method. The skip_duplicates path uses + # raw SQL and the ORM path runs through sudo(), both of which bypass 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") + if not vals_list: return 0 if skip_duplicates else self.env["spp.program.membership"] diff --git a/spp_programs/tests/__init__.py b/spp_programs/tests/__init__.py index e5682fe4..0b7897e4 100644 --- a/spp_programs/tests/__init__.py +++ b/spp_programs/tests/__init__.py @@ -38,3 +38,7 @@ from . import test_canary_patterns from . import test_concurrency from . import test_async_lock_recovery +from . import test_membership_acl_bypass +from . import test_cycle_null_entitlement_approval +from . import test_approve_entitlements_program_isolation +from . import test_payment_batch_payment_ids diff --git a/spp_programs/tests/test_approve_entitlements_program_isolation.py b/spp_programs/tests/test_approve_entitlements_program_isolation.py new file mode 100644 index 00000000..fb0714d0 --- /dev/null +++ b/spp_programs/tests/test_approve_entitlements_program_isolation.py @@ -0,0 +1,94 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security scan finding: "Fund balance cached from first entitlement program". + +``approve_entitlements`` fetches the fund balance once with +``check_fund_balance(entitlements[0].cycle_id.program_id.id)`` and reuses it +for every record in the batch. Two consequences: + +1. If a mixed-program recordset is passed, entitlements belonging to a later + program are approved against the *first* program's balance instead of their + own — bypassing the later program's fund limit. +2. ``entitlements[0]`` raises ``IndexError`` on an empty recordset instead of + returning cleanly. + +The previous implementation called ``check_fund_balance(rec.cycle_id.program_id.id)`` +inside the loop, so each entitlement was evaluated against its own program. +""" + +import uuid +from unittest.mock import patch + +from odoo import fields +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestApproveEntitlementsProgramIsolation(TransactionCase): + def _make_program_with_cycle(self): + program = self.env["spp.program"].create({"name": f"Program {uuid.uuid4().hex[:8]}"}) + journal = self.env["account.journal"].create( + {"name": "J", "type": "bank", "code": f"J{uuid.uuid4().hex[:4].upper()}"} + ) + program.journal_id = journal.id + cycle = self.env["spp.cycle"].create( + { + "name": "Cycle", + "program_id": program.id, + "start_date": fields.Date.today(), + "end_date": fields.Date.today(), + } + ) + manager = self.env["spp.program.entitlement.manager.default"].create( + {"name": "Mgr", "program_id": program.id, "amount_per_cycle": 100.0} + ) + return program, cycle, manager + + def _make_entitlement(self, cycle, partner): + return self.env["spp.entitlement"].create( + { + "partner_id": partner.id, + "cycle_id": cycle.id, + "initial_amount": 100.0, + "state": "pending_validation", + "is_cash_entitlement": True, + } + ) + + def test_empty_recordset_does_not_crash(self): + """approve_entitlements on an empty recordset must not raise IndexError.""" + _program, _cycle, manager = self._make_program_with_cycle() + empty = self.env["spp.entitlement"] + # On the buggy code entitlements[0] raises IndexError before the loop. + manager.approve_entitlements(empty) + + def test_each_entitlement_checked_against_its_own_program(self): + """A mixed-program batch must evaluate each entitlement's own program fund. + + We record every program_id passed to check_fund_balance. With the + single-fetch bug only the first program is ever queried, so the second + program's id is missing from the recorded set. + """ + _p1, cycle1, manager = self._make_program_with_cycle() + p2, cycle2, _manager2 = self._make_program_with_cycle() + partner_a = self.env["res.partner"].create({"name": "A", "is_registrant": True}) + partner_b = self.env["res.partner"].create({"name": "B", "is_registrant": True}) + + ent1 = self._make_entitlement(cycle1, partner_a) + ent2 = self._make_entitlement(cycle2, partner_b) + mixed = ent1 | ent2 + + checked_program_ids = [] + + def _record(self_mgr, program_id): + checked_program_ids.append(program_id) + return 10000.0 # plenty of funds so the loop runs to completion + + with patch.object(type(manager), "check_fund_balance", _record): + manager.approve_entitlements(mixed) + + self.assertIn( + p2.id, + checked_program_ids, + "second program's fund balance was never checked — its entitlement was " + "approved against the first program's funds", + ) diff --git a/spp_programs/tests/test_cycle_null_entitlement_approval.py b/spp_programs/tests/test_cycle_null_entitlement_approval.py new file mode 100644 index 00000000..615d9dd2 --- /dev/null +++ b/spp_programs/tests/test_cycle_null_entitlement_approval.py @@ -0,0 +1,84 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security scan finding: "NULL entitlement states can pass approval check". + +``_compute_all_entitlements_approved`` finds unapproved cycles with the SQL +predicate ``state != 'approved'``. In SQL ``NULL != 'approved'`` evaluates to +UNKNOWN (not TRUE), so an entitlement whose ``state`` is NULL is excluded from +``cycles_with_unapproved``. A cycle containing only approved entitlements plus +one NULL-state entitlement therefore computes ``all_entitlements_approved = +True`` — re-introducing the unapproved entitlement into a "fully approved" +cycle. The previous ``all(ent.state == "approved" ...)`` logic treated +NULL/False as "not approved" and was safe. + +The entitlement ``state`` field has a default of "draft" but is not required, +so NULL rows are reachable via raw SQL / imports / RPC writes. +""" + +import uuid + +from odoo import fields +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestCycleNullEntitlementApproval(TransactionCase): + def setUp(self): + super().setUp() + self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) + self.journal = self.env["account.journal"].create( + {"name": "Test Journal", "type": "bank", "code": f"TJ{uuid.uuid4().hex[:4].upper()}"} + ) + self.program.journal_id = self.journal.id + self.cycle = self.env["spp.cycle"].create( + { + "name": "Test Cycle", + "program_id": self.program.id, + "start_date": fields.Date.today(), + "end_date": fields.Date.today(), + } + ) + self.partners = self.env["res.partner"].create( + [{"name": f"Registrant {i}", "is_registrant": True} for i in range(2)] + ) + + def _make_entitlement(self, partner, state): + return self.env["spp.entitlement"].create( + { + "partner_id": partner.id, + "cycle_id": self.cycle.id, + "initial_amount": 100.0, + "state": state, + "is_cash_entitlement": True, + } + ) + + def test_null_state_entitlement_is_not_treated_as_approved(self): + """A cycle with an approved + a NULL-state entitlement is NOT fully approved.""" + approved = self._make_entitlement(self.partners[0], "approved") + other = self._make_entitlement(self.partners[1], "draft") + + # Force the second entitlement's state to NULL, the way a raw import / + # RPC write that omits the default could. (ORM create would apply the + # 'draft' default, so we go through SQL to reproduce the NULL row.) + self.env.cr.execute("UPDATE spp_entitlement SET state = NULL WHERE id = %s", (other.id,)) + self.cycle.invalidate_recordset(["all_entitlements_approved"]) + + # Sanity: there really is a NULL-state entitlement in this cycle. + self.env.cr.execute( + "SELECT COUNT(*) FROM spp_entitlement WHERE cycle_id = %s AND state IS NULL", + (self.cycle.id,), + ) + self.assertEqual(self.env.cr.fetchone()[0], 1, "precondition: one NULL-state entitlement must exist") + self.assertEqual(approved.state, "approved") + + self.assertFalse( + self.cycle.all_entitlements_approved, + "Cycle has a NULL-state (unapproved) entitlement; all_entitlements_approved must be False", + ) + + def test_all_approved_is_true_when_truly_all_approved(self): + """Control: when every entitlement is approved, the flag is True.""" + self._make_entitlement(self.partners[0], "approved") + self._make_entitlement(self.partners[1], "approved") + self.cycle.invalidate_recordset(["all_entitlements_approved"]) + self.assertTrue(self.cycle.all_entitlements_approved) diff --git a/spp_programs/tests/test_membership_acl_bypass.py b/spp_programs/tests/test_membership_acl_bypass.py new file mode 100644 index 00000000..e2d1fb87 --- /dev/null +++ b/spp_programs/tests/test_membership_acl_bypass.py @@ -0,0 +1,116 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security scan finding: "Public raw membership bulk insert bypasses ACLs". + +``bulk_create_memberships`` is a public ``@api.model`` method (no leading +underscore), so it is reachable through the web/external RPC layer by any +authenticated user. Its ``skip_duplicates=True`` path runs a raw +``INSERT ... ON CONFLICT`` and its ORM path (program memberships) runs through +``sudo()`` — both bypassing the ORM's ACL checks. + +The fix gates ``bulk_create_memberships`` with an explicit +``self.check_access("create")``. These tests assert the secured behaviour: + +* a low-privileged user (no create rights) is denied — both via the ORM and + via the bulk helper; +* a user that *does* have create rights can still use the helper. +""" + +import uuid + +from odoo import fields +from odoo.exceptions import AccessError +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestMembershipAclBypass(TransactionCase): + def setUp(self): + super().setUp() + # Data created as the test admin. + self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) + self.cycle = self.env["spp.cycle"].create( + { + "name": "Test Cycle", + "program_id": self.program.id, + "start_date": fields.Date.today(), + "end_date": fields.Date.today(), + } + ) + self.partner = self.env["res.partner"].create({"name": "Beneficiary", "is_registrant": True}) + + # A plain internal user with NO spp_programs access groups: it holds + # none of the ir.model.access rights on spp.cycle.membership / + # spp.program.membership, so create() must be denied for it. + self.low_priv_user = self.env["res.users"].create( + { + "name": "Low Priv User", + "login": f"lowpriv_{uuid.uuid4().hex[:8]}", + "group_ids": [(6, 0, [self.env.ref("base.group_user").id])], + } + ) + # A user that legitimately may create memberships (officer group grants + # create on both membership models). + self.officer_user = self.env["res.users"].create( + { + "name": "Officer User", + "login": f"officer_{uuid.uuid4().hex[:8]}", + "group_ids": [ + ( + 6, + 0, + [ + self.env.ref("base.group_user").id, + self.env.ref("spp_programs.group_programs_officer").id, + ], + ) + ], + } + ) + + def test_orm_create_is_denied_for_low_priv_user(self): + """Control: the low-priv user cannot create a cycle membership via the ORM.""" + with self.assertRaises(AccessError): + self.env["spp.cycle.membership"].with_user(self.low_priv_user).create( + {"partner_id": self.partner.id, "cycle_id": self.cycle.id, "state": "enrolled"} + ) + + def test_raw_bulk_insert_enforces_acl(self): + """The skip_duplicates raw-SQL path must NOT let a low-priv user create rows. + + Before the fix the raw INSERT bypassed ACLs entirely; now + bulk_create_memberships gates on check_access("create"). + """ + before = self.env["spp.cycle.membership"].sudo().search_count([("cycle_id", "=", self.cycle.id)]) + + with self.assertRaises(AccessError): + self.env["spp.cycle.membership"].with_user(self.low_priv_user).bulk_create_memberships( + [{"partner_id": self.partner.id, "cycle_id": self.cycle.id, "state": "enrolled"}], + skip_duplicates=True, + ) + + after = self.env["spp.cycle.membership"].sudo().search_count([("cycle_id", "=", self.cycle.id)]) + self.assertEqual(after, before, "raw bulk insert created a row despite the user lacking create rights") + + def test_program_membership_bulk_enforces_acl(self): + """The program-membership ORM path (which uses sudo()) must also be gated.""" + before = self.env["spp.program.membership"].sudo().search_count([("program_id", "=", self.program.id)]) + + with self.assertRaises(AccessError): + self.env["spp.program.membership"].with_user(self.low_priv_user).bulk_create_memberships( + [{"partner_id": self.partner.id, "program_id": self.program.id, "state": "enrolled"}], + ) + + after = self.env["spp.program.membership"].sudo().search_count([("program_id", "=", self.program.id)]) + self.assertEqual(after, before, "bulk helper created a row despite the user lacking create rights") + + def test_privileged_user_can_still_bulk_create(self): + """A user with create rights can still use the bulk helper (no regression).""" + count = ( + self.env["spp.cycle.membership"] + .with_user(self.officer_user) + .bulk_create_memberships( + [{"partner_id": self.partner.id, "cycle_id": self.cycle.id, "state": "enrolled"}], + skip_duplicates=True, + ) + ) + self.assertEqual(count, 1, "officer with create rights should be able to bulk-create memberships") diff --git a/spp_programs/tests/test_payment_batch_payment_ids.py b/spp_programs/tests/test_payment_batch_payment_ids.py new file mode 100644 index 00000000..2d096817 --- /dev/null +++ b/spp_programs/tests/test_payment_batch_payment_ids.py @@ -0,0 +1,87 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security scan finding: "Batch payment creation leaves payment batches empty". + +The chunked batch-assignment path in ``payment_manager._prepare_payments`` +creates payments and assigns them with +``batch_payments.write({"batch_id": curr_batch.id})``. But +``spp.payment.batch.payment_ids`` is an *independent* Many2many field, not the +inverse of ``spp.payment.batch_id`` (a separate Many2one), so writing +``batch_id`` alone leaves the batch's ``payment_ids`` empty. Views +(``payment_batch_view.xml``) and the batch ``unlink`` cleanup +(``record.payment_ids.unlink()``) rely on ``payment_ids``. + +The fix populates ``curr_batch.payment_ids`` explicitly in the manager. This +test drives ``_prepare_payments`` and asserts every generated batch lists its +payments. +""" + +import uuid + +from odoo import fields +from odoo.tests import TransactionCase, tagged + + +@tagged("post_install", "-at_install") +class TestPaymentBatchPaymentIds(TransactionCase): + def setUp(self): + super().setUp() + self.program = self.env["spp.program"].create({"name": f"Test Program {uuid.uuid4().hex[:8]}"}) + self.journal = self.env["account.journal"].create( + {"name": "Test Journal", "type": "bank", "code": f"TJ{uuid.uuid4().hex[:4].upper()}"} + ) + self.program.journal_id = self.journal.id + self.cycle = self.env["spp.cycle"].create( + { + "name": "Test Cycle", + "program_id": self.program.id, + "start_date": fields.Date.today(), + "end_date": fields.Date.today(), + } + ) + self.payment_manager = self.env["spp.program.payment.manager.default"].create( + { + "name": "Test Payment Manager", + "program_id": self.program.id, + "create_batch": True, + } + ) + batch_tag = self.env["spp.payment.batch.tag"].create( + {"name": "Test Tag", "order": 1, "domain": "[]", "max_batch_size": 500} + ) + self.payment_manager.batch_tag_ids = [(4, batch_tag.id)] + + self.entitlements = self.env["spp.entitlement"] + for i in range(5): + reg = self.env["res.partner"].create({"name": f"Registrant {i}", "is_registrant": True}) + self.entitlements |= self.env["spp.entitlement"].create( + { + "partner_id": reg.id, + "cycle_id": self.cycle.id, + "initial_amount": 100.0, + "state": "approved", + "is_cash_entitlement": True, + } + ) + + def test_generated_batches_list_their_payments(self): + """Every batch created by _prepare_payments must populate payment_ids. + + Before the fix only batch_id (the Many2one) was set, leaving the batch's + payment_ids Many2many empty so the batch displayed/iterated zero payments. + """ + payments, batches = self.payment_manager._prepare_payments(self.cycle, self.entitlements) + + self.assertTrue(batches, "expected at least one payment batch to be created") + total_in_batches = self.env["spp.payment"] + for batch in batches: + self.assertTrue( + batch.payment_ids, + f"batch {batch.name} has an empty payment_ids — it will display zero payments", + ) + total_in_batches |= batch.payment_ids + # payment_ids and batch_id must agree. + for payment in batch.payment_ids: + self.assertEqual(payment.batch_id, batch) + + # Every created payment is reachable through some batch's payment_ids. + self.assertEqual(set(total_in_batches.ids), set(payments.ids))