From 1b6ff9b08a7d06a67cd4e0e66875cf596dd57082 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 14 Aug 2026 20:56:11 +0800 Subject: [PATCH 1/2] security(cr): validate program access on assign-program detail (#338) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Squashed net change of the #338 branch for the batch-2 staging rebase. - @api.constrains on spp.cr.detail.assign_program rejects, in the writing user's own context, a program they cannot access (record rules) or that is outside their company scope — the program_id domain only constrained the UI, and apply runs under sudo. - Re-assert program access at the apply sink, bound to the CR requester's company scope, so a program_id written before the constraint existed (the module ships in released tags), imported, or set under a sudo prefill still cannot be applied cross-company. - preview() redacts the program name for such a record rather than leaking it. - Tests: cross-company reject on write/create, in-scope/shared allow, apply rejects a pre-existing poisoned record, preview redacts it. --- spp_cr_type_assign_program/README.rst | 16 +- spp_cr_type_assign_program/__manifest__.py | 2 +- .../details/assign_program.py | 37 +++- spp_cr_type_assign_program/readme/HISTORY.md | 20 ++ .../static/description/index.html | 16 +- .../strategies/assign_program.py | 63 +++++- spp_cr_type_assign_program/tests/__init__.py | 2 +- .../tests/test_program_access.py | 201 ++++++++++++++++++ 8 files changed, 339 insertions(+), 18 deletions(-) create mode 100644 spp_cr_type_assign_program/tests/test_program_access.py diff --git a/spp_cr_type_assign_program/README.rst b/spp_cr_type_assign_program/README.rst index 023c341a7..4d7bd12e3 100644 --- a/spp_cr_type_assign_program/README.rst +++ b/spp_cr_type_assign_program/README.rst @@ -91,14 +91,18 @@ Dependencies Changelog ========= -19.0.1.0.2 +19.0.1.0.3 ~~~~~~~~~~ -- 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. +- fix(security): validate server-side that the user selecting a program + on ``spp.cr.detail.assign_program`` can actually access it. The + ``program_id`` domain only constrained the UI, so a raw RPC write + could target a hidden or cross-company program; on apply the strategy + runs under ``sudo``, which would assign the membership and leak the + program name via preview while bypassing program record rules and + multi-company scope. An ``@api.constrains`` now rejects a program the + writing user cannot see (record rules) or that is outside their + company scope. 19.0.1.0.0 ~~~~~~~~~~ diff --git a/spp_cr_type_assign_program/__manifest__.py b/spp_cr_type_assign_program/__manifest__.py index 01fff2da4..8aa59f908 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.2", + "version": "19.0.1.0.3", "sequence": 53, "category": "OpenSPP", "summary": "Change request type for assigning a registrant to a program", diff --git a/spp_cr_type_assign_program/details/assign_program.py b/spp_cr_type_assign_program/details/assign_program.py index da30dade5..c9af590ae 100644 --- a/spp_cr_type_assign_program/details/assign_program.py +++ b/spp_cr_type_assign_program/details/assign_program.py @@ -1,4 +1,5 @@ -from odoo import api, fields, models +from odoo import _, api, fields, models +from odoo.exceptions import ValidationError class SPPCRDetailAssignProgram(models.Model): @@ -35,6 +36,40 @@ class SPPCRDetailAssignProgram(models.Model): readonly=True, ) + @api.constrains("program_id") + def _check_program_access(self): + """Reject a program the selecting user cannot access. + + The `program_id` domain only constrains the UI; a raw ORM/RPC write can + point it at an arbitrary program. Since the apply strategy runs under + `sudo` (spp.change.request._do_apply), an inaccessible program would + otherwise be assigned - and its name leaked via preview - bypassing + program record rules and multi-company scope. Enforce access here, in + the writing user's own context, so the stored value can only ever be a + program that user may target. + + Two checks, because neither alone is sufficient: + - `search()` requires the program to be visible to the user, enforcing + any record rule on `spp.program` (and rejecting a stale/deleted id). + - an explicit `company_id in env.companies` guard enforces multi-company + scope directly. This is load-bearing, not mere defense in depth: the + global multi-company `ir.rule` on `spp.program` is NOT reliably + applied to the search in this write/constraint context (verified by + test - a company-A user's search still returns a company-B program), + so relying on `search()` alone would let a cross-company program + through. The explicit check rejects it deterministically. + """ + for rec in self: + program = rec.program_id + if not program: + continue + # `or` short-circuits: if the record is not visible, program.company_id + # is not read (avoids an AccessError on a rule-hidden record). + if not self.env["spp.program"].search([("id", "=", program.id)]) or ( + program.company_id and program.company_id not in self.env.companies + ): + raise ValidationError(_("You do not have access to the selected program.")) + @api.depends("registrant_id", "registrant_id.is_group") def _compute_registrant_target_type(self): for rec in self: diff --git a/spp_cr_type_assign_program/readme/HISTORY.md b/spp_cr_type_assign_program/readme/HISTORY.md index 0809aecb5..bf0b3ff0b 100644 --- a/spp_cr_type_assign_program/readme/HISTORY.md +++ b/spp_cr_type_assign_program/readme/HISTORY.md @@ -1,3 +1,23 @@ +### 19.0.1.0.3 + +- fix(security): validate server-side that the user selecting a program on + `spp.cr.detail.assign_program` can actually access it. The `program_id` + domain only constrained the UI, so a raw RPC write could target a hidden or + cross-company program; on apply the strategy runs under `sudo`, which would + assign the membership and leak the program name via preview while bypassing + program record rules and multi-company scope. An `@api.constrains` now rejects + a program the writing user cannot see (record rules) or that is outside their + company scope. +- fix(security): re-assert program access at the apply sink (defense in depth). + The write-time constraint cannot cover a value it never saw — a record + written before the constraint shipped (the module is in released tags), an + import, or a future sudo prefill. The apply strategy now re-checks the + program against the change-request requester's company scope before creating + the membership, so a pre-existing out-of-scope `program_id` cannot be applied + cross-company, and `preview()` (which runs under sudo) redacts the program + name for such a record rather than leaking it. No-op in single-company + deployments. + ### 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. diff --git a/spp_cr_type_assign_program/static/description/index.html b/spp_cr_type_assign_program/static/description/index.html index 97aa7d1af..8fa8f8ffe 100644 --- a/spp_cr_type_assign_program/static/description/index.html +++ b/spp_cr_type_assign_program/static/description/index.html @@ -454,13 +454,17 @@

Changelog

-

19.0.1.0.2

+

19.0.1.0.3

diff --git a/spp_cr_type_assign_program/strategies/assign_program.py b/spp_cr_type_assign_program/strategies/assign_program.py index 4f2fb11c0..b625be798 100644 --- a/spp_cr_type_assign_program/strategies/assign_program.py +++ b/spp_cr_type_assign_program/strategies/assign_program.py @@ -21,8 +21,46 @@ class SPPCRApplyAssignProgram(models.AbstractModel): _inherit = "spp.cr.strategy.base" _description = "CR Apply: Assign to Program" + def _program_accessible_to_requester(self, change_request, program): + """Whether the CR *requester* (``create_uid``) may target ``program``. + + Bound to the requester - the identity whose authority the assignment + rides on - not the apply-time actor, which is sudo (and, after the + apply-authorization guard, a manager who may span companies). Mirrors + the write-time ``_check_program_access`` company scope. Returns True for + a company-shared program (``company_id`` False). No-op in single-company + deployments (every program's company is in every user's ``company_ids``). + """ + requester = change_request.create_uid + return not program.company_id or program.company_id in requester.company_ids + + def _check_program_access_at_apply(self, change_request, program): + """Raise unless the requester may target ``program``. + + Re-asserts access at the sudo sink, so a program written before the + write-time constraint existed (the module shipped in released tags + without it), imported, or slipped in under a sudo prefill, still cannot + be applied cross-company. + """ + if not self._program_accessible_to_requester(change_request, program): + raise UserError( + _("The change request creator does not have access to program '%(program)s'.") + % {"program": program.display_name} + ) + def validate(self, change_request): - """Validate the CR can be applied. Raises UserError on any failure.""" + """Validate the CR can be applied. Raises UserError on any failure. + + This runs under ``sudo`` (see ``spp.change.request._do_apply``). Program + access is enforced primarily at selection time by + ``spp.cr.detail.assign_program._check_program_access`` (a write-time + constraint in the user's own context), but that constraint cannot cover + a stored value it never saw: records written before the constraint + existed (the module shipped in released tags without it), an import, or + a future sudo prefill that sets ``program_id`` without triggering + constraints. So re-assert program access here, at the sink, before the + privileged membership create - see ``_check_program_access_at_apply``. + """ detail = change_request.get_detail() if not detail: raise UserError(_("No detail record found for this change request.")) @@ -31,6 +69,8 @@ def validate(self, change_request): if not program: raise UserError(_("Program is required to assign a registrant.")) + self._check_program_access_at_apply(change_request, program) + registrant = change_request.registrant_id if not registrant: raise UserError(_("Registrant is required.")) @@ -109,14 +149,31 @@ def apply(self, change_request): return True def preview(self, change_request): - """Preview what will happen on apply.""" + """Preview what will happen on apply. + + Runs under sudo (via ``_capture_preview_snapshot`` / the preview HTML), + so redact the program name for a stored program the requester cannot + access - otherwise a pre-existing out-of-scope ``program_id`` (see + ``_check_program_access_at_apply``) would leak the cross-company + program's name here even though it can never be applied. Preview must + stay non-throwing, so redact rather than raise. + """ detail = change_request.get_detail() if not detail or not detail.program_id: return {} + program = detail.program_id + if not self._program_accessible_to_requester(change_request, program): + return { + "_action": "create_program_membership", + "registrant": change_request.registrant_id.display_name, + "program": _("(program not accessible to the requester)"), + "initial_state": "draft", + } + return { "_action": "create_program_membership", "registrant": change_request.registrant_id.display_name, - "program": detail.program_id.display_name, + "program": program.display_name, "initial_state": "draft", } diff --git a/spp_cr_type_assign_program/tests/__init__.py b/spp_cr_type_assign_program/tests/__init__.py index 32d199be1..187bed209 100644 --- a/spp_cr_type_assign_program/tests/__init__.py +++ b/spp_cr_type_assign_program/tests/__init__.py @@ -1,3 +1,3 @@ from . import test_assign_program - from . import test_detail_security +from . import test_program_access diff --git a/spp_cr_type_assign_program/tests/test_program_access.py b/spp_cr_type_assign_program/tests/test_program_access.py new file mode 100644 index 000000000..6292bcdaf --- /dev/null +++ b/spp_cr_type_assign_program/tests/test_program_access.py @@ -0,0 +1,201 @@ +"""Security regression: the assign-program detail must reject a program the +selecting user cannot access. + +The detail's ``program_id`` Many2one ``domain`` only constrains the UI; a raw +ORM/RPC write can point it at an arbitrary program. On apply the strategy runs +under ``sudo`` (`spp.change.request._do_apply`), so program record rules and the +global multi-company rule on ``spp.program`` would be bypassed — assigning a +registrant to a hidden/cross-company program and leaking its name via preview. +An ``@api.constrains`` on the detail enforces, in the writing user's own +context, that the selected program is actually visible to them. +""" + +from odoo.exceptions import UserError, ValidationError +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 TestAssignProgramAccess(CRTestCase): + """A CR user must not be able to target a program they cannot access.""" + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.Program = cls.env["spp.program"] + + 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}) + + cls.company_a = cls.env.company + cls.company_b = cls.env["res.company"].create({"name": "CR Access Test Company B"}) + + # A program the test user CAN see (their own company). + cls.program_visible = cls.Program.create( + { + "name": "Company A Individual Program", + "target_type": "individual", + "company_id": cls.company_a.id, + } + ) + # A program in another company — hidden by the global multi-company rule + # spp_programs.rule_spp_program_company for a company-A-only user. + cls.program_cross_company = cls.Program.create( + { + "name": "Company B Individual Program", + "target_type": "individual", + "company_id": cls.company_b.id, + } + ) + + # A CR user scoped to company A only: can write assign-program details + # (group_cr_user) and read programs (group_programs_viewer), but the + # multi-company rule keeps company-B programs out of their reach. + cls.cr_user = cls.env["res.users"].create( + { + "name": "CR User (Company A)", + "login": "cr_user_company_a", + "company_id": cls.company_a.id, + "company_ids": [(6, 0, [cls.company_a.id])], + "group_ids": [ + ( + 4, + cls.env.ref("spp_change_request_v2.group_cr_user").id, + ), + ( + 4, + cls.env.ref("spp_programs.group_programs_viewer").id, + ), + ], + } + ) + # Make the cr_user's own partner a registrant so change requests can be + # created with it. Using it as the registrant models "the CR user's own + # change request" and keeps these tests robust to detail-ownership record + # rules (PR #261) that scope detail write to CRs the user owns/created. + cls.cr_user_registrant = cls.cr_user.partner_id + cls.cr_user_registrant.write({"is_registrant": True, "is_group": False}) + + def _cr_user_env(self, model): + """`model` in the cr_user's env, scoped to company A only — mirroring a + real company-A session, whose allowed companies are limited to the ones + the user belongs to.""" + return self.env[model].with_user(self.cr_user).with_context(allowed_company_ids=[self.company_a.id]) + + def _make_cr(self): + """Create a CR as admin (CR-name sequence generation needs privileged + access) with the cr_user's own partner as registrant, so a detail write + as cr_user is allowed both today and once PR #261's ownership rule lands. + Returns the CR (admin env).""" + return self.CR.create({"request_type_id": self.cr_type.id, "registrant_id": self.cr_user_registrant.id}) + + def _make_cr_and_detail(self): + """Return (cr, detail) with the detail bound to the cr_user's context — + the program_id write (what the constraint guards) then runs as cr_user.""" + cr = self._make_cr() + detail = cr.get_detail() + return cr, detail.with_user(self.cr_user).with_context(allowed_company_ids=[self.company_a.id]) + + def test_reject_cross_company_program(self): + _cr, detail = self._make_cr_and_detail() + with self.assertRaises(ValidationError): + detail.write({"program_id": self.program_cross_company.id}) + + def test_reject_cross_company_program_on_create(self): + # The constraint must also fire when the program is set at create time. + # Details are created lazily, so create one directly (do not call + # get_detail first, which would auto-create the single detail). + cr = self._make_cr() + with self.assertRaises(ValidationError): + self._cr_user_env("spp.cr.detail.assign_program").create( + { + "change_request_id": cr.id, + "program_id": self.program_cross_company.id, + } + ) + + def test_allow_visible_program(self): + _cr, detail = self._make_cr_and_detail() + detail.write({"program_id": self.program_visible.id}) + self.assertEqual(detail.program_id, self.program_visible) + + def test_allow_shared_program(self): + # A company-shared program (company_id = False) is in no company's + # exclusive scope and must remain selectable. + shared = self.Program.create( + {"name": "Shared Individual Program", "target_type": "individual", "company_id": False} + ) + _cr, detail = self._make_cr_and_detail() + detail.write({"program_id": shared.id}) + self.assertEqual(detail.program_id, shared) + + # --- apply-time sink re-check (defense in depth) ------------------------- + # The write-time constraint above cannot cover a value it never saw: a + # record written before the constraint shipped (the module is in released + # tags 2026.07/2026.08 without it), an import, or a future sudo prefill. + # The strategy re-asserts program access at apply, bound to the CR + # requester's company scope. + + def _plant_poisoned_cr(self, program): + """Simulate a pre-constraint record: a CR whose requester is the + company-A cr_user, carrying `program` on its detail — both written via + direct SQL to bypass the ORM constraint that would reject them today.""" + cr = self._make_cr() + detail = cr.get_detail() + self.env.cr.execute( + "UPDATE spp_change_request SET create_uid = %s WHERE id = %s", + (self.cr_user.id, cr.id), + ) + self.env.cr.execute( + "UPDATE spp_cr_detail_assign_program SET program_id = %s WHERE id = %s", + (program.id, detail.id), + ) + cr.invalidate_recordset(["create_uid"]) + detail.invalidate_recordset(["program_id"]) + return cr + + def test_apply_rejects_preexisting_cross_company_program(self): + """A cross-company program stored before the constraint existed must be + rejected at apply — the sudo strategy no longer trusts the stored value. + Reverting the sink check makes validate() pass this poisoned record.""" + cr = self._plant_poisoned_cr(self.program_cross_company) + with self.assertRaises(UserError): + self.env["spp.cr.apply.assign_program"].validate(cr) + + def test_apply_allows_in_scope_program(self): + """A program within the requester's company passes the sink re-check.""" + cr = self._plant_poisoned_cr(self.program_visible) + # Should not raise on the access check (may still fail later validate() + # rules; assert only the access gate directly). + self.env["spp.cr.apply.assign_program"]._check_program_access_at_apply(cr, self.program_visible) + + def test_apply_allows_shared_program(self): + """A company-shared program (company_id=False) passes the sink re-check.""" + shared = self.Program.create( + {"name": "Shared Program (apply)", "target_type": "individual", "company_id": False} + ) + cr = self._plant_poisoned_cr(shared) + self.env["spp.cr.apply.assign_program"]._check_program_access_at_apply(cr, shared) + + def test_preview_redacts_inaccessible_program(self): + """preview() runs under sudo; for a pre-existing out-of-scope program it + must not leak the program name (which apply would reject anyway).""" + cr = self._plant_poisoned_cr(self.program_cross_company) + preview = self.env["spp.cr.apply.assign_program"].preview(cr) + self.assertNotIn(self.program_cross_company.name, preview.get("program", "")) + + def test_preview_shows_accessible_program(self): + """preview() still shows the program name when the requester can access it.""" + cr = self._plant_poisoned_cr(self.program_visible) + preview = self.env["spp.cr.apply.assign_program"].preview(cr) + self.assertEqual(preview.get("program"), self.program_visible.display_name) From e3069bc667c15eca60094ed23b58fe73d38fc983 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Fri, 14 Aug 2026 21:07:23 +0800 Subject: [PATCH 2/2] docs(spp_cr_type_assign_program): regenerate README from fragments (#338) Applied verbatim from CI's pinned oca-gen output (run 31802666482). --- spp_cr_type_assign_program/README.rst | 19 ++++++++++++++++++ .../static/description/index.html | 20 +++++++++++++++++++ 2 files changed, 39 insertions(+) diff --git a/spp_cr_type_assign_program/README.rst b/spp_cr_type_assign_program/README.rst index 4d7bd12e3..1b0e23438 100644 --- a/spp_cr_type_assign_program/README.rst +++ b/spp_cr_type_assign_program/README.rst @@ -103,6 +103,25 @@ Changelog multi-company scope. An ``@api.constrains`` now rejects a program the writing user cannot see (record rules) or that is outside their company scope. +- fix(security): re-assert program access at the apply sink (defense in + depth). The write-time constraint cannot cover a value it never saw — + a record written before the constraint shipped (the module is in + released tags), an import, or a future sudo prefill. The apply + strategy now re-checks the program against the change-request + requester's company scope before creating the membership, so a + pre-existing out-of-scope ``program_id`` cannot be applied + cross-company, and ``preview()`` (which runs under sudo) redacts the + program name for such a record rather than leaking it. No-op in + single-company deployments. + +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. 19.0.1.0.0 ~~~~~~~~~~ diff --git a/spp_cr_type_assign_program/static/description/index.html b/spp_cr_type_assign_program/static/description/index.html index 8fa8f8ffe..7c6734540 100644 --- a/spp_cr_type_assign_program/static/description/index.html +++ b/spp_cr_type_assign_program/static/description/index.html @@ -465,9 +465,29 @@

19.0.1.0.3

multi-company scope. An @api.constrains now rejects a program the writing user cannot see (record rules) or that is outside their company scope. +
  • fix(security): re-assert program access at the apply sink (defense in +depth). The write-time constraint cannot cover a value it never saw — +a record written before the constraint shipped (the module is in +released tags), an import, or a future sudo prefill. The apply +strategy now re-checks the program against the change-request +requester’s company scope before creating the membership, so a +pre-existing out-of-scope program_id cannot be applied +cross-company, and preview() (which runs under sudo) redacts the +program name for such a record rather than leaking it. No-op in +single-company deployments.
    • +

    19.0.1.0.2

    + +
    +

    19.0.1.0.0