fix(spp_programs): close membership ACL bypass and approval/payment bugs

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),
         )

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"]
 

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

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

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:

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"]
 

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

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",
+        )

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)