From 47b7df9667141ab682455500e47a01d77c28333b Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Wed, 1 Jul 2026 11:00:04 +0800 Subject: [PATCH 1/4] security(cr): add record rules to detail models to enforce CR ownership Detail models (spp.cr.detail.*) are separate tables that do not inherit the parent spp.change.request record rules, so a low-privileged group_cr_user could read/write detail rows of change requests they do not own via RPC, tampering with pending/approved requests (e.g. re-pointing assign_program's program_id). Add per-model ir.rules mirroring the parent CR scope (users limited to own/related CRs; validators and managers unrestricted) for all 11 core detail models and the assign_program detail. Includes a completeness test asserting every concrete detail model carries a group_cr_user rule. --- spp_change_request_v2/security/rules.xml | 538 ++++++++++++++++++ spp_change_request_v2/tests/__init__.py | 2 + .../tests/test_detail_record_rules.py | 112 ++++ spp_cr_type_assign_program/__manifest__.py | 1 + spp_cr_type_assign_program/security/rules.xml | 77 +++ spp_cr_type_assign_program/tests/__init__.py | 2 + .../tests/test_detail_security.py | 99 ++++ 7 files changed, 831 insertions(+) create mode 100644 spp_change_request_v2/tests/test_detail_record_rules.py create mode 100644 spp_cr_type_assign_program/security/rules.xml create mode 100644 spp_cr_type_assign_program/tests/test_detail_security.py diff --git a/spp_change_request_v2/security/rules.xml b/spp_change_request_v2/security/rules.xml index 8d1675802..dbde95914 100644 --- a/spp_change_request_v2/security/rules.xml +++ b/spp_change_request_v2/security/rules.xml @@ -53,4 +53,542 @@ + + + + + CR Detail (add_member): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (add_member): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (add_member): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (add_member): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (edit_individual): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (edit_individual): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (edit_individual): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (edit_individual): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (edit_group): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (edit_group): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (edit_group): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (edit_group): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (remove_member): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (remove_member): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (remove_member): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (remove_member): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (change_hoh): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (change_hoh): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (change_hoh): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (change_hoh): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (exit_registrant): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (exit_registrant): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (exit_registrant): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (exit_registrant): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (transfer_member): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (transfer_member): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (transfer_member): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (transfer_member): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (update_id): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (update_id): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (update_id): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (update_id): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (create_group): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (create_group): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (create_group): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (create_group): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (merge_registrants): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (merge_registrants): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (merge_registrants): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (merge_registrants): Manager Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (split_household): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + CR Detail (split_household): Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (split_household): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + CR Detail (split_household): Manager Access + + [(1, '=', 1)] + + + + + + diff --git a/spp_change_request_v2/tests/__init__.py b/spp_change_request_v2/tests/__init__.py index bc75e9cc0..215875233 100644 --- a/spp_change_request_v2/tests/__init__.py +++ b/spp_change_request_v2/tests/__init__.py @@ -25,3 +25,5 @@ from . import test_conflict_dynamic_approval from . import test_html_escaping from . import test_wizard_html_escaping + +from . import test_detail_record_rules diff --git a/spp_change_request_v2/tests/test_detail_record_rules.py b/spp_change_request_v2/tests/test_detail_record_rules.py new file mode 100644 index 000000000..6c179978b --- /dev/null +++ b/spp_change_request_v2/tests/test_detail_record_rules.py @@ -0,0 +1,112 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""Security: CR detail models must enforce parent-CR ownership via ir.rule. + +Regression tests for the missing-record-rule vulnerability: a separate detail +model does not inherit the parent ``spp.change.request`` record rules, so a +low-privileged ``group_cr_user`` could read/write detail rows of change +requests they do not own (directly via RPC, bypassing the UI). Each concrete +detail model must ship its own ir.rule mirroring the parent's ownership scope. +""" + +from odoo.exceptions import AccessError +from odoo.tests import tagged + +from .common import CRTestCase, get_or_create_cr_type + + +@tagged("post_install", "-at_install") +class TestDetailRecordRules(CRTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.internal_group = cls.env.ref("base.group_user") + cls.user_group = cls.env.ref("spp_change_request_v2.group_cr_user") + cls.validator_group = cls.env.ref("spp_change_request_v2.group_cr_validator") + Users = cls.env["res.users"].with_context(no_reset_password=True) + cls.user_a = Users.create( + { + "name": "CR User A", + "login": "cr_detail_user_a", + "email": "cr_detail_user_a@test.com", + "group_ids": [(4, cls.internal_group.id), (4, cls.user_group.id)], + } + ) + cls.user_b = Users.create( + { + "name": "CR User B", + "login": "cr_detail_user_b", + "email": "cr_detail_user_b@test.com", + "group_ids": [(4, cls.internal_group.id), (4, cls.user_group.id)], + } + ) + cls.validator = Users.create( + { + "name": "CR Validator", + "login": "cr_detail_validator", + "email": "cr_detail_validator@test.com", + "group_ids": [(4, cls.internal_group.id), (4, cls.validator_group.id)], + } + ) + cls.edit_type = get_or_create_cr_type(cls.env, "edit_individual") + + def _make_detail_owned_by(self, user): + """Create a CR (owned by ``user``) and return its detail record.""" + cr = self.CR.with_user(user).create( + { + "request_type_id": self.edit_type.id, + "registrant_id": self.test_individual.id, + } + ) + detail = cr.with_user(user).get_detail() + return cr, detail + + # ------------------------------------------------------------------ + # Completeness: every concrete detail model must be scoped + # ------------------------------------------------------------------ + + def test_every_concrete_detail_model_has_user_rule(self): + """Guard against a detail model shipping without an ownership rule.""" + models = self.env["ir.model"].search([("model", "=like", "spp.cr.detail.%")]) + self.assertTrue(models, "expected at least one spp.cr.detail.* model") + Rule = self.env["ir.rule"] + unscoped = [] + for model in models: + if self.env[model.model]._abstract: + continue + rules = Rule.search([("model_id", "=", model.id)]) + user_rules = rules.filtered(lambda r: self.user_group in r.groups and r.perm_read) + if not user_rules: + unscoped.append(model.model) + self.assertFalse( + unscoped, + "detail models missing a group_cr_user ir.rule (ownership bypass): %s" % ", ".join(unscoped), + ) + + # ------------------------------------------------------------------ + # Functional: cross-user isolation (edit_individual as a representative) + # ------------------------------------------------------------------ + + def test_cr_user_cannot_read_others_detail(self): + _cr, detail = self._make_detail_owned_by(self.user_a) + # A different cr_user cannot even see it via search. + found = self.env["spp.cr.detail.edit_individual"].with_user(self.user_b).search([("id", "=", detail.id)]) + self.assertFalse(found, "user B must not see user A's detail row") + # Direct read of the known id is denied. + with self.assertRaises(AccessError): + detail.with_user(self.user_b).read(["change_request_id"]) + + def test_cr_user_cannot_write_others_detail(self): + _cr, detail = self._make_detail_owned_by(self.user_a) + # Writing even a same-value field triggers the record-rule check. + with self.assertRaises(AccessError): + detail.with_user(self.user_b).write({"change_request_id": detail.change_request_id.id}) + + def test_cr_user_can_access_own_detail(self): + _cr, detail = self._make_detail_owned_by(self.user_a) + # The owner reads their own detail without error. + self.assertTrue(detail.with_user(self.user_a).read(["change_request_id"])) + + def test_validator_can_access_any_detail(self): + _cr, detail = self._make_detail_owned_by(self.user_a) + # Validators (implying cr_user) retain full visibility, matching the parent CR rule. + self.assertTrue(detail.with_user(self.validator).read(["change_request_id"])) diff --git a/spp_cr_type_assign_program/__manifest__.py b/spp_cr_type_assign_program/__manifest__.py index f81be529e..1e8b81ee6 100644 --- a/spp_cr_type_assign_program/__manifest__.py +++ b/spp_cr_type_assign_program/__manifest__.py @@ -14,6 +14,7 @@ ], "data": [ "security/ir.model.access.csv", + "security/rules.xml", "views/detail_assign_program_views.xml", "data/cr_types.xml", ], diff --git a/spp_cr_type_assign_program/security/rules.xml b/spp_cr_type_assign_program/security/rules.xml new file mode 100644 index 000000000..0b73d517d --- /dev/null +++ b/spp_cr_type_assign_program/security/rules.xml @@ -0,0 +1,77 @@ + + + + + + + CR Detail (assign_program): User Access + + [ + '|', + ('change_request_id.create_uid', '=', user.id), + ('change_request_id.registrant_id', 'in', user.partner_id.ids) + ] + + + + + + + + + + CR Detail (assign_program): Validator Access + + [(1, '=', 1)] + + + + + + + + + + CR Detail (assign_program): HQ Validator Access + + [(1, '=', 1)] + + + + + + + + + + CR Detail (assign_program): Manager Access + + [(1, '=', 1)] + + + + + + + diff --git a/spp_cr_type_assign_program/tests/__init__.py b/spp_cr_type_assign_program/tests/__init__.py index 270981145..32d199be1 100644 --- a/spp_cr_type_assign_program/tests/__init__.py +++ b/spp_cr_type_assign_program/tests/__init__.py @@ -1 +1,3 @@ from . import test_assign_program + +from . import test_detail_security diff --git a/spp_cr_type_assign_program/tests/test_detail_security.py b/spp_cr_type_assign_program/tests/test_detail_security.py new file mode 100644 index 000000000..6ccc69469 --- /dev/null +++ b/spp_cr_type_assign_program/tests/test_detail_security.py @@ -0,0 +1,99 @@ +"""Security: the assign_program detail must enforce parent-CR ownership. + +Regression test for the reported "Assign-program detail ACL bypasses CR +ownership" issue: without an ir.rule, a low-privileged ``group_cr_user`` could +re-point ``program_id`` on a change request they do not own, enrolling a +beneficiary into an unauthorized program. +""" + +from odoo.exceptions import AccessError +from odoo.tests import tagged + +from odoo.addons.spp_change_request_v2.tests.common import CRTestCase + +ASSIGN_PROGRAM_CR_TYPE_DEFS = { + "name": "Assign to Program", + "target_type": "both", + "detail_model": "spp.cr.detail.assign_program", + "apply_strategy": "custom", + "apply_model": "spp.cr.apply.assign_program", +} + + +@tagged("post_install", "-at_install") +class TestAssignProgramDetailSecurity(CRTestCase): + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.cr_type = cls.CRType.search([("code", "=", "assign_program")], limit=1) + if not cls.cr_type: + cls.cr_type = cls.CRType.create({"code": "assign_program", **ASSIGN_PROGRAM_CR_TYPE_DEFS}) + + Program = cls.env["spp.program"] + cls.program_a = Program.create({"name": "Program A", "target_type": "individual"}) + cls.program_b = Program.create({"name": "Program B", "target_type": "individual"}) + + cls.internal_group = cls.env.ref("base.group_user") + cls.user_group = cls.env.ref("spp_change_request_v2.group_cr_user") + cls.validator_group = cls.env.ref("spp_change_request_v2.group_cr_validator") + Users = cls.env["res.users"].with_context(no_reset_password=True) + cls.user_a = Users.create( + { + "name": "Assign User A", + "login": "assign_user_a", + "email": "assign_user_a@test.com", + "group_ids": [(4, cls.internal_group.id), (4, cls.user_group.id)], + } + ) + cls.user_b = Users.create( + { + "name": "Assign User B", + "login": "assign_user_b", + "email": "assign_user_b@test.com", + "group_ids": [(4, cls.internal_group.id), (4, cls.user_group.id)], + } + ) + cls.validator = Users.create( + { + "name": "Assign Validator", + "login": "assign_validator", + "email": "assign_validator@test.com", + "group_ids": [(4, cls.internal_group.id), (4, cls.validator_group.id)], + } + ) + + def _make_cr_owned_by(self, user, program=None): + cr = self.CR.with_user(user).create( + { + "request_type_id": self.cr_type.id, + "registrant_id": self.test_individual.id, + } + ) + detail = cr.with_user(user).get_detail() + if program is not None: + detail.with_user(user).program_id = program.id + return cr, detail + + def test_cr_user_cannot_read_others_detail(self): + _cr, detail = self._make_cr_owned_by(self.user_a, self.program_a) + found = self.env["spp.cr.detail.assign_program"].with_user(self.user_b).search([("id", "=", detail.id)]) + self.assertFalse(found, "user B must not see user A's assign-program detail") + with self.assertRaises(AccessError): + detail.with_user(self.user_b).read(["program_id"]) + + def test_cr_user_cannot_repoint_program_on_others_detail(self): + """The exact reported attack: tamper with another user's program assignment.""" + _cr, detail = self._make_cr_owned_by(self.user_a, self.program_a) + with self.assertRaises(AccessError): + detail.with_user(self.user_b).write({"program_id": self.program_b.id}) + # The value is unchanged. + self.assertEqual(detail.program_id, self.program_a) + + def test_owner_can_set_program(self): + _cr, detail = self._make_cr_owned_by(self.user_a) + detail.with_user(self.user_a).write({"program_id": self.program_a.id}) + self.assertEqual(detail.program_id, self.program_a) + + def test_validator_can_read_any_detail(self): + _cr, detail = self._make_cr_owned_by(self.user_a, self.program_a) + self.assertTrue(detail.with_user(self.validator).read(["program_id"])) From d9a894702ca1cb12931410b7c26ac4fe238b0430 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Wed, 1 Jul 2026 12:41:11 +0800 Subject: [PATCH 2/4] security(cr): mirror parent area filter to detail models + harden test Post-review follow-ups: - Add a global area-filter ir.rule to every CR detail model (11 core + assign_program), mirroring the parent CR's area rule. Detail models do not inherit the parent's global rule, so an area-scoped officer could otherwise reach (via RPC) detail rows of change requests whose registrant is outside their center area. Global rule ANDs with the ownership rules; users without center areas are unaffected. - Strengthen the completeness test to require, per concrete detail model, that group_cr_user is scoped on read/write/create (not just read), that each higher role retains a permissive rule, and that a global area rule exists. - Add a functional test proving the area filter scopes a detail by its registrant's area, isolating the area dimension from ownership. --- .../security/area_filter_rules.xml | 174 ++++++++++++++++++ .../tests/test_detail_record_rules.py | 83 ++++++++- spp_cr_type_assign_program/security/rules.xml | 21 +++ 3 files changed, 269 insertions(+), 9 deletions(-) diff --git a/spp_change_request_v2/security/area_filter_rules.xml b/spp_change_request_v2/security/area_filter_rules.xml index 1a352d5c6..51425052c 100644 --- a/spp_change_request_v2/security/area_filter_rules.xml +++ b/spp_change_request_v2/security/area_filter_rules.xml @@ -31,4 +31,178 @@ can be referenced without defensive guards. + + + + + CR Detail (add_member): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (edit_individual): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (edit_group): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (remove_member): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (change_hoh): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (exit_registrant): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (transfer_member): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (update_id): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (create_group): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (merge_registrants): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + + + + CR Detail (split_household): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + diff --git a/spp_change_request_v2/tests/test_detail_record_rules.py b/spp_change_request_v2/tests/test_detail_record_rules.py index 6c179978b..f742a0cf0 100644 --- a/spp_change_request_v2/tests/test_detail_record_rules.py +++ b/spp_change_request_v2/tests/test_detail_record_rules.py @@ -64,23 +64,88 @@ def _make_detail_owned_by(self, user): # Completeness: every concrete detail model must be scoped # ------------------------------------------------------------------ - def test_every_concrete_detail_model_has_user_rule(self): - """Guard against a detail model shipping without an ownership rule.""" + def test_every_concrete_detail_model_is_fully_scoped(self): + """Guard against a detail model shipping without complete ownership rules. + + Asserts, for every concrete ``spp.cr.detail.*`` model, that ``group_cr_user`` + is scoped on EVERY operation the ACL grants it (read/write/create) — a rule + missing only ``perm_write`` would still leave a tamper path — and that the + higher roles each retain a permissive rule (else the group hierarchy would + cage them behind the restrictive user rule). + """ models = self.env["ir.model"].search([("model", "=like", "spp.cr.detail.%")]) self.assertTrue(models, "expected at least one spp.cr.detail.* model") Rule = self.env["ir.rule"] - unscoped = [] + higher_roles = [ + ("validator", self.validator_group), + ("validator_hq", self.env.ref("spp_change_request_v2.group_cr_validator_hq")), + ("manager", self.env.ref("spp_change_request_v2.group_cr_manager")), + ] + problems = [] for model in models: if self.env[model.model]._abstract: continue rules = Rule.search([("model_id", "=", model.id)]) - user_rules = rules.filtered(lambda r: self.user_group in r.groups and r.perm_read) - if not user_rules: - unscoped.append(model.model) - self.assertFalse( - unscoped, - "detail models missing a group_cr_user ir.rule (ownership bypass): %s" % ", ".join(unscoped), + + def grants(group, perm, _rules=rules): + return any(group in r.groups and getattr(r, perm) for r in _rules) + + for perm in ("perm_read", "perm_write", "perm_create"): + if not grants(self.user_group, perm): + problems.append(f"{model.model}: group_cr_user missing {perm} rule (bypass)") + for label, group in higher_roles: + if not grants(group, "perm_read"): + problems.append(f"{model.model}: {label} missing read rule (would be caged)") + # A global (no-group) read rule mirrors the parent CR area filter. + if not any(not r.groups and r.perm_read for r in rules): + problems.append(f"{model.model}: missing global area-filter rule") + self.assertFalse(problems, "detail model rule gaps:\n " + "\n ".join(problems)) + + # ------------------------------------------------------------------ + # Functional: area scoping (mirrors the parent CR area filter) + # ------------------------------------------------------------------ + + def test_area_filter_scopes_detail_by_registrant_area(self): + """An area-scoped user cannot reach details of out-of-area CRs they own. + + Ownership is held constant (the area user creates both CRs while + unrestricted), so this isolates the area dimension: once the user is + restricted to area_1, only the in-area detail remains readable. + """ + Area = self.env["spp.area"] + area_1 = Area.create({"draft_name": "CR Detail Area 1"}) + area_2 = Area.create({"draft_name": "CR Detail Area 2"}) + reg_in = self.Partner.create( + {"name": "Reg In Area", "is_registrant": True, "is_group": False, "area_id": area_1.id} ) + reg_out = self.Partner.create( + {"name": "Reg Out Area", "is_registrant": True, "is_group": False, "area_id": area_2.id} + ) + # user_a has no center areas yet -> unrestricted create; owns both CRs. + cr_in = self.CR.with_user(self.user_a).create( + {"request_type_id": self.edit_type.id, "registrant_id": reg_in.id} + ) + cr_out = self.CR.with_user(self.user_a).create( + {"request_type_id": self.edit_type.id, "registrant_id": reg_out.id} + ) + detail_in = cr_in.with_user(self.user_a).get_detail() + detail_out = cr_out.with_user(self.user_a).get_detail() + + # Unrestricted (no center areas): both readable — global roles unaffected. + self.assertTrue(detail_out.with_user(self.user_a).read(["change_request_id"])) + + # Restrict user_a to area_1 (center_area_ids is a stored computed field; + # write it directly, after creation, to isolate the area dimension). + self.user_a.sudo().center_area_ids = [(6, 0, [area_1.id])] + self.assertEqual(self.user_a.center_area_ids, area_1) + # ir.rule evaluates and caches its domain per (model, mode); the earlier + # unrestricted read cached an empty domain, so drop the cache to pick up + # the new center-area scope (a real role change invalidates this too). + self.env.registry.clear_cache() + + self.assertTrue(detail_in.with_user(self.user_a).read(["change_request_id"])) + with self.assertRaises(AccessError): + detail_out.with_user(self.user_a).read(["change_request_id"]) # ------------------------------------------------------------------ # Functional: cross-user isolation (edit_individual as a representative) diff --git a/spp_cr_type_assign_program/security/rules.xml b/spp_cr_type_assign_program/security/rules.xml index 0b73d517d..d6bbe4a5f 100644 --- a/spp_cr_type_assign_program/security/rules.xml +++ b/spp_cr_type_assign_program/security/rules.xml @@ -74,4 +74,25 @@ + + + + CR Detail (assign_program): visible only within user's center areas + + [('change_request_id.registrant_id.area_id', 'child_of', user.center_area_ids.ids)] if user.center_area_ids else [] + + + + + + From 18b46b8d7cd6a40076676df27ad9b57b6dc616aa Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 22:26:14 +0800 Subject: [PATCH 3/4] chore(spp_change_request_v2,spp_cr_type_assign_program): bump versions + changelog Record the CR detail-model record-rule hardening (spp_change_request_v2 19.0.2.0.8, spp_cr_type_assign_program 19.0.1.0.2) in versions and HISTORY changelogs (regenerated READMEs). --- spp_change_request_v2/README.rst | 44 ++++++++++++------- spp_change_request_v2/__manifest__.py | 2 +- spp_change_request_v2/readme/HISTORY.md | 4 ++ .../static/description/index.html | 29 ++++++++---- spp_cr_type_assign_program/README.rst | 14 ++++-- spp_cr_type_assign_program/__manifest__.py | 2 +- spp_cr_type_assign_program/readme/HISTORY.md | 18 ++++---- .../static/description/index.html | 20 +++++---- 8 files changed, 85 insertions(+), 48 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 97adb511a..67d36a09c 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -752,22 +752,22 @@ Methods available for override on detail models (all inherited from Related fields available on all detail models (from ``spp.cr.detail.base``): -+--------------------------+-----------+------------------------------------------------------------+ -| Field | Type | Source | -+==========================+===========+============================================================+ -| ``change_request_id`` | Many2one | Direct link to parent CR | -+--------------------------+-----------+------------------------------------------------------------+ -| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``approval_state`` | Selection | ``change_request_id.approval_state`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | -+--------------------------+-----------+------------------------------------------------------------+ -| ``field_to_modify`` | Selection | Dynamic field selector (populated by | -| | | ``_get_field_to_modify_selection``) | -+--------------------------+-----------+------------------------------------------------------------+ ++----------------------------+-----------+------------------------------------------------------------+ +| Field | Type | Source | ++============================+===========+============================================================+ +| ``change_request_id`` | Many2one | Direct link to parent CR | ++----------------------------+-----------+------------------------------------------------------------+ +| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``approval_state`` | Selection | ``change_request_id.approval_state`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | ++----------------------------+-----------+------------------------------------------------------------+ +| ``field_to_modify`` | Selection | Dynamic field selector (populated by | +| | | ``_get_field_to_modify_selection``) | ++----------------------------+-----------+------------------------------------------------------------+ CR Type Fields Reference ~~~~~~~~~~~~~~~~~~~~~~~~ @@ -853,6 +853,18 @@ Before declaring a new CR type complete: Changelog ========= +19.0.2.0.8 +~~~~~~~~~~ + +- fix(security): add per-model record rules to the CR detail models + (``spp.cr.detail.*``) enforcing parent change-request ownership, plus + the parent's area filter. Detail models are separate tables that do + not inherit the ``spp.change.request`` record rules, so a + low-privilege CR user could previously read/write detail rows of + change requests they do not own (e.g. re-point ``assign_program``'s + ``program_id``) directly via RPC. A completeness test guards against a + future detail model shipping without a rule. + 19.0.2.0.7 ~~~~~~~~~~ diff --git a/spp_change_request_v2/__manifest__.py b/spp_change_request_v2/__manifest__.py index 3a26a2c99..07ffc94e2 100644 --- a/spp_change_request_v2/__manifest__.py +++ b/spp_change_request_v2/__manifest__.py @@ -1,6 +1,6 @@ { "name": "OpenSPP Change Request V2", - "version": "19.0.2.0.7", + "version": "19.0.2.0.8", "sequence": 50, "category": "OpenSPP", "summary": "Configuration-driven change request system with UX improvements, conflict detection and duplicate prevention", diff --git a/spp_change_request_v2/readme/HISTORY.md b/spp_change_request_v2/readme/HISTORY.md index 8644831eb..0d1d1de5a 100644 --- a/spp_change_request_v2/readme/HISTORY.md +++ b/spp_change_request_v2/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.0.8 + +- fix(security): add per-model record rules to the CR detail models (`spp.cr.detail.*`) enforcing parent change-request ownership, plus the parent's area filter. Detail models are separate tables that do not inherit the `spp.change.request` record rules, so a low-privilege CR user could previously read/write detail rows of change requests they do not own (e.g. re-point `assign_program`'s `program_id`) directly via RPC. A completeness test guards against a future detail model shipping without a rule. + ### 19.0.2.0.7 - fix(security): align CR Requestor / CR Local Validator / CR HQ Validator roles with the OP#951 menu audit — replace the `spp_registry.group_registry_read` (Tier-3, no menu) link with `spp_registry.group_registry_viewer` so these roles see the Registry menu; add `spp_hazard.group_hazard_viewer` so they retain Hazard visibility once the menu root is gated. Adds `spp_hazard` to module dependencies. diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index ef26ac45e..e6d92f00d 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1160,9 +1160,9 @@

Methods Reference

spp.cr.detail.base):

-+-+ @@ -1339,6 +1339,19 @@

Changelog

+

19.0.2.0.8

+
    +
  • fix(security): add per-model record rules to the CR detail models +(spp.cr.detail.*) enforcing parent change-request ownership, plus +the parent’s area filter. Detail models are separate tables that do +not inherit the spp.change.request record rules, so a +low-privilege CR user could previously read/write detail rows of +change requests they do not own (e.g. re-point assign_program’s +program_id) directly via RPC. A completeness test guards against a +future detail model shipping without a rule.
    • +
+
+

19.0.2.0.7

  • fix(security): align CR Requestor / CR Local Validator / CR HQ @@ -1350,7 +1363,7 @@

    19.0.2.0.7

    dependencies.
-
+

19.0.2.0.6

  • fix(views): route post-submit CRs (pending / approved / applied / @@ -1365,7 +1378,7 @@

    19.0.2.0.6

    list so row-click goes through the stage router.
-
+

19.0.2.0.5

  • fix(security): add a global ir.rule on spp.change.request that @@ -1378,27 +1391,27 @@

    19.0.2.0.5

    roles).
-
+

19.0.2.0.3

  • fix: add HTML escaping to all computed Html fields with sanitize=False to prevent stored XSS (#50)
-
+

19.0.2.0.2

  • fix: fix batch approval wizard line deletion (#130)
-
+

19.0.2.0.1

  • fix: skip field types before getattr and isolate detail prefetch (#129)
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_cr_type_assign_program/README.rst b/spp_cr_type_assign_program/README.rst index 6a49d539f..023c341a7 100644 --- a/spp_cr_type_assign_program/README.rst +++ b/spp_cr_type_assign_program/README.rst @@ -91,11 +91,17 @@ Dependencies Changelog ========= -19.0.1.0.0 (2026-05-04) ------------------------ +19.0.1.0.2 +~~~~~~~~~~ -Added -~~~~~ +- fix(security): add record rules to ``spp.cr.detail.assign_program`` + enforcing parent change-request ownership and area scope. The model + previously had an ACL granting ``group_cr_user`` write/create but no + record rule, so a CR user could re-point ``program_id`` on + assign-program details of change requests they do not own via RPC. + +19.0.1.0.0 +~~~~~~~~~~ - New module ``spp_cr_type_assign_program`` with the ``assign_program`` change request type. diff --git a/spp_cr_type_assign_program/__manifest__.py b/spp_cr_type_assign_program/__manifest__.py index 1e8b81ee6..01fff2da4 100644 --- a/spp_cr_type_assign_program/__manifest__.py +++ b/spp_cr_type_assign_program/__manifest__.py @@ -1,6 +1,6 @@ { "name": "OpenSPP CR Type - Assign to Program", - "version": "19.0.1.0.1", + "version": "19.0.1.0.2", "sequence": 53, "category": "OpenSPP", "summary": "Change request type for assigning a registrant to a program", diff --git a/spp_cr_type_assign_program/readme/HISTORY.md b/spp_cr_type_assign_program/readme/HISTORY.md index 74862b003..0809aecb5 100644 --- a/spp_cr_type_assign_program/readme/HISTORY.md +++ b/spp_cr_type_assign_program/readme/HISTORY.md @@ -1,12 +1,10 @@ -## 19.0.1.0.0 (2026-05-04) +### 19.0.1.0.2 -### Added +- fix(security): add record rules to `spp.cr.detail.assign_program` enforcing parent change-request ownership and area scope. The model previously had an ACL granting `group_cr_user` write/create but no record rule, so a CR user could re-point `program_id` on assign-program details of change requests they do not own via RPC. -- New module `spp_cr_type_assign_program` with the `assign_program` change - request type. -- Detail model `spp.cr.detail.assign_program` with live program-domain - filtering based on the registrant's target type. -- Apply strategy `spp.cr.apply.assign_program` that creates a draft - `spp.program.membership` record on apply. -- Conflict rule that blocks duplicate in-flight assignments to the same - `(registrant, program)` pair. +### 19.0.1.0.0 + +- New module `spp_cr_type_assign_program` with the `assign_program` change request type. +- Detail model `spp.cr.detail.assign_program` with live program-domain filtering based on the registrant's target type. +- Apply strategy `spp.cr.apply.assign_program` that creates a draft `spp.program.membership` record on apply. +- Conflict rule that blocks duplicate in-flight assignments to the same `(registrant, program)` pair. diff --git a/spp_cr_type_assign_program/static/description/index.html b/spp_cr_type_assign_program/static/description/index.html index aa6fd572c..97aa7d1af 100644 --- a/spp_cr_type_assign_program/static/description/index.html +++ b/spp_cr_type_assign_program/static/description/index.html @@ -446,21 +446,25 @@

    Dependencies

    Table of contents

    +
    +

    19.0.1.0.2

    +
      +
    • fix(security): add record rules to spp.cr.detail.assign_program +enforcing parent change-request ownership and area scope. The model +previously had an ACL granting group_cr_user write/create but no +record rule, so a CR user could re-point program_id on +assign-program details of change requests they do not own via RPC.
      • +
    -
    -

    Added

    +
    +

    19.0.1.0.0

    • New module spp_cr_type_assign_program with the assign_program change request type.
      • From d4deaad5ab6a7d727125e40ef1c1a349e272c3dd Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 3 Jul 2026 00:33:25 +0800 Subject: [PATCH 4/4] docs: normalize README table widths to match CI generator The earlier README regeneration was run in a local env whose RST renderer produced wider table columns than CI's pinned oca-gen environment, causing the 'Generate addons README files from fragments' pre-commit hook to fail on CI. Restore the column widths CI's generator produces. --- spp_change_request_v2/README.rst | 32 +++++++++---------- .../static/description/index.html | 4 +-- 2 files changed, 18 insertions(+), 18 deletions(-) diff --git a/spp_change_request_v2/README.rst b/spp_change_request_v2/README.rst index 67d36a09c..ac7d893a1 100644 --- a/spp_change_request_v2/README.rst +++ b/spp_change_request_v2/README.rst @@ -752,22 +752,22 @@ Methods available for override on detail models (all inherited from Related fields available on all detail models (from ``spp.cr.detail.base``): -+----------------------------+-----------+------------------------------------------------------------+ -| Field | Type | Source | -+============================+===========+============================================================+ -| ``change_request_id`` | Many2one | Direct link to parent CR | -+----------------------------+-----------+------------------------------------------------------------+ -| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``approval_state`` | Selection | ``change_request_id.approval_state`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | -+----------------------------+-----------+------------------------------------------------------------+ -| ``field_to_modify`` | Selection | Dynamic field selector (populated by | -| | | ``_get_field_to_modify_selection``) | -+----------------------------+-----------+------------------------------------------------------------+ ++--------------------------+-----------+------------------------------------------------------------+ +| Field | Type | Source | ++==========================+===========+============================================================+ +| ``change_request_id`` | Many2one | Direct link to parent CR | ++--------------------------+-----------+------------------------------------------------------------+ +| ``registrant_id`` | Many2one | ``change_request_id.registrant_id`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``approval_state`` | Selection | ``change_request_id.approval_state`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``is_applied`` | Boolean | ``change_request_id.is_applied`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``use_dynamic_approval`` | Boolean | ``change_request_id.request_type_id.use_dynamic_approval`` | ++--------------------------+-----------+------------------------------------------------------------+ +| ``field_to_modify`` | Selection | Dynamic field selector (populated by | +| | | ``_get_field_to_modify_selection``) | ++--------------------------+-----------+------------------------------------------------------------+ CR Type Fields Reference ~~~~~~~~~~~~~~~~~~~~~~~~ diff --git a/spp_change_request_v2/static/description/index.html b/spp_change_request_v2/static/description/index.html index e6d92f00d..d30653011 100644 --- a/spp_change_request_v2/static/description/index.html +++ b/spp_change_request_v2/static/description/index.html @@ -1160,9 +1160,9 @@

      Methods Reference

      spp.cr.detail.base):

Field
-+-+
Field