From 374d559b2dff72cc50928b1b0328c931a705e1a9 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 18:44:29 +0800 Subject: [PATCH 1/5] security(cr): validate program access on assign-program detail The spp.cr.detail.assign_program program_id domain only constrains the UI, so a CR user could RPC-write an arbitrary program id. The apply strategy runs on self.sudo(), so program record rules and multi-company scope were bypassed at validation, membership creation, and preview - allowing assignment to a hidden/cross-company program and leaking its name. Add @api.constrains('program_id') on the detail that, in the writing user's own context, rejects a program the user cannot see (record rules) or that is outside their company scope (explicit env.companies check, since the global company ir.rule is not reliably applied to searches in the write context). Bump 19.0.1.0.1 -> 19.0.1.0.3 (dodging #261's 19.0.1.0.2 on the same module) + HISTORY. No migration: module is not in any released tag. --- spp_cr_type_assign_program/__manifest__.py | 2 +- .../details/assign_program.py | 35 +++++- spp_cr_type_assign_program/readme/HISTORY.md | 13 ++ spp_cr_type_assign_program/tests/__init__.py | 1 + .../tests/test_program_access.py | 114 ++++++++++++++++++ 5 files changed, 163 insertions(+), 2 deletions(-) create mode 100644 spp_cr_type_assign_program/tests/test_program_access.py diff --git a/spp_cr_type_assign_program/__manifest__.py b/spp_cr_type_assign_program/__manifest__.py index f81be529e..5b66fe5f9 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.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..393dc3f01 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,38 @@ 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()` applies the user's record rules (area/registrant scope, + etc.), so a program hidden by those rules returns empty and is + rejected. + - an explicit `company_id in env.companies` guard enforces multi-company + scope directly, independent of whether the global company `ir.rule` is + evaluated in the current write context (it is not always), so a + cross-company program is rejected 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 74862b003..7d7b2e7bb 100644 --- a/spp_cr_type_assign_program/readme/HISTORY.md +++ b/spp_cr_type_assign_program/readme/HISTORY.md @@ -1,3 +1,16 @@ +## 19.0.1.0.3 (2026-07-23) + +### Fixed + +- 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 (2026-05-04) ### Added diff --git a/spp_cr_type_assign_program/tests/__init__.py b/spp_cr_type_assign_program/tests/__init__.py index 270981145..bf1874cd0 100644 --- a/spp_cr_type_assign_program/tests/__init__.py +++ b/spp_cr_type_assign_program/tests/__init__.py @@ -1 +1,2 @@ from . import test_assign_program +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..577cd3500 --- /dev/null +++ b/spp_cr_type_assign_program/tests/test_program_access.py @@ -0,0 +1,114 @@ +"""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 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, + ), + ], + } + ) + + def _as_cr_user(self, records): + """Return `records` 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 records.with_user(self.cr_user).with_context(allowed_company_ids=[self.company_a.id]) + + def _make_detail(self, registrant): + cr = self.CR.create({"request_type_id": self.cr_type.id, "registrant_id": registrant.id}) + return cr.get_detail() + + def test_reject_cross_company_program(self): + detail = self._make_detail(self.test_individual) + with self.assertRaises(ValidationError): + self._as_cr_user(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.CR.create({"request_type_id": self.cr_type.id, "registrant_id": self.test_individual.id}) + with self.assertRaises(ValidationError): + self._as_cr_user(self.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): + detail = self._make_detail(self.test_individual) + self._as_cr_user(detail).write({"program_id": self.program_visible.id}) + self.assertEqual(detail.program_id, self.program_visible) From 137e2d441bfb33d6692ef6acbf5fed99f3b80471 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 18:52:57 +0800 Subject: [PATCH 2/5] docs(spp_cr_type_assign_program): regenerate README for 19.0.1.0.3 changelog Applies CI's pinned oca-gen output verbatim for the new HISTORY fragment. --- spp_cr_type_assign_program/README.rst | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/spp_cr_type_assign_program/README.rst b/spp_cr_type_assign_program/README.rst index 6a49d539f..a9a894353 100644 --- a/spp_cr_type_assign_program/README.rst +++ b/spp_cr_type_assign_program/README.rst @@ -91,6 +91,22 @@ Dependencies Changelog ========= +19.0.1.0.3 (2026-07-23) +----------------------- + +Fixed +~~~~~ + +- 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 (2026-05-04) ----------------------- From e49797016d79510174cd7ab41750d51350cbaac5 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 19:01:35 +0800 Subject: [PATCH 3/5] docs(spp_cr_type_assign_program): use OCA-style HISTORY heading The keep-a-changelog '## (date)' / '### Fixed' fragment made the oca-gen README generator crash (docutils 'Inconsistent title style: skip from level 2 to 4'). Switch to OCA flat '### ' entries (matching the module's DESCRIPTION heading ladder). README.rst reverted to base so CI's pinned generator regenerates it cleanly. --- spp_cr_type_assign_program/README.rst | 16 -------------- spp_cr_type_assign_program/readme/HISTORY.md | 22 ++++++++------------ 2 files changed, 9 insertions(+), 29 deletions(-) diff --git a/spp_cr_type_assign_program/README.rst b/spp_cr_type_assign_program/README.rst index a9a894353..6a49d539f 100644 --- a/spp_cr_type_assign_program/README.rst +++ b/spp_cr_type_assign_program/README.rst @@ -91,22 +91,6 @@ Dependencies Changelog ========= -19.0.1.0.3 (2026-07-23) ------------------------ - -Fixed -~~~~~ - -- 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 (2026-05-04) ----------------------- diff --git a/spp_cr_type_assign_program/readme/HISTORY.md b/spp_cr_type_assign_program/readme/HISTORY.md index 7d7b2e7bb..7639bf892 100644 --- a/spp_cr_type_assign_program/readme/HISTORY.md +++ b/spp_cr_type_assign_program/readme/HISTORY.md @@ -1,19 +1,15 @@ -## 19.0.1.0.3 (2026-07-23) +### 19.0.1.0.3 -### Fixed - -- security: validate server-side that the user selecting a program on +- 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 (2026-05-04) + 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. -### Added +### 19.0.1.0.0 - New module `spp_cr_type_assign_program` with the `assign_program` change request type. From 6d6d3270b96908a7fecae96c38974329aa2101b7 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 19:33:09 +0800 Subject: [PATCH 4/5] docs(spp_cr_type_assign_program): regenerate README/index for OCA HISTORY Applies CI's pinned oca-gen output verbatim for the OCA-style changelog. --- spp_cr_type_assign_program/README.rst | 20 ++++++++++++---- .../static/description/index.html | 24 ++++++++++++------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/spp_cr_type_assign_program/README.rst b/spp_cr_type_assign_program/README.rst index 6a49d539f..4d7bd12e3 100644 --- a/spp_cr_type_assign_program/README.rst +++ b/spp_cr_type_assign_program/README.rst @@ -91,11 +91,21 @@ Dependencies Changelog ========= -19.0.1.0.0 (2026-05-04) ------------------------ - -Added -~~~~~ +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. + +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/static/description/index.html b/spp_cr_type_assign_program/static/description/index.html index aa6fd572c..8fa8f8ffe 100644 --- a/spp_cr_type_assign_program/static/description/index.html +++ b/spp_cr_type_assign_program/static/description/index.html @@ -446,21 +446,29 @@

Dependencies

Table of contents

+
+

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.
    • +
-
-

Added

+
+

19.0.1.0.0

  • New module spp_cr_type_assign_program with the assign_program change request type.
    • From fbf04f6a2f72c9bdd5b0cd8c5aeef670a069511c Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 23 Jul 2026 20:08:54 +0800 Subject: [PATCH 5/5] security(cr): address staff-review findings on assign-program access - Correct the constraint docstring: the explicit company check is load-bearing, not defense-in-depth. Verified by experiment that the global multi-company ir.rule on spp.program is NOT applied to the search in this write/constraint context, so search-only would let a cross-company program through; drop the inaccurate 'area/registrant record rules' claim (spp.program has only the company rule). - Document in the apply strategy that program access is enforced at detail write time, so any future sudo writer of program_id must re-check. - Make the tests robust to in-flight PR #261's detail-ownership rule by using the cr_user's own partner as the CR registrant (matches #261's registrant branch); add a shared-program (company_id=False) allow test. Tests: 23 pass (was 22), run as a non-admin group_cr_user. --- .../details/assign_program.py | 14 +++-- .../strategies/assign_program.py | 12 +++- .../tests/test_program_access.py | 58 ++++++++++++++----- 3 files changed, 61 insertions(+), 23 deletions(-) diff --git a/spp_cr_type_assign_program/details/assign_program.py b/spp_cr_type_assign_program/details/assign_program.py index 393dc3f01..c9af590ae 100644 --- a/spp_cr_type_assign_program/details/assign_program.py +++ b/spp_cr_type_assign_program/details/assign_program.py @@ -49,13 +49,15 @@ def _check_program_access(self): program that user may target. Two checks, because neither alone is sufficient: - - `search()` applies the user's record rules (area/registrant scope, - etc.), so a program hidden by those rules returns empty and is - rejected. + - `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, independent of whether the global company `ir.rule` is - evaluated in the current write context (it is not always), so a - cross-company program is rejected deterministically. + 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 diff --git a/spp_cr_type_assign_program/strategies/assign_program.py b/spp_cr_type_assign_program/strategies/assign_program.py index 4f2fb11c0..8edd75a38 100644 --- a/spp_cr_type_assign_program/strategies/assign_program.py +++ b/spp_cr_type_assign_program/strategies/assign_program.py @@ -22,7 +22,17 @@ class SPPCRApplyAssignProgram(models.AbstractModel): _description = "CR Apply: Assign to Program" 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. + + Note: this runs under ``sudo`` (see ``spp.change.request._do_apply``), + so it does NOT re-check the caller's access to ``program_id``. That + access is enforced when the program is selected, by + ``spp.cr.detail.assign_program._check_program_access`` (a write-time + constraint in the user's own context). If a new code path ever sets + ``detail.program_id`` under sudo (e.g. a prefill mapping), it must + perform its own access check - the sudo apply here trusts the stored + value. + """ detail = change_request.get_detail() if not detail: raise UserError(_("No detail record found for this change request.")) diff --git a/spp_cr_type_assign_program/tests/test_program_access.py b/spp_cr_type_assign_program/tests/test_program_access.py index 577cd3500..8f979b1f5 100644 --- a/spp_cr_type_assign_program/tests/test_program_access.py +++ b/spp_cr_type_assign_program/tests/test_program_access.py @@ -79,29 +79,45 @@ def setUpClass(cls): ], } ) - - def _as_cr_user(self, records): - """Return `records` 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 records.with_user(self.cr_user).with_context(allowed_company_ids=[self.company_a.id]) - - def _make_detail(self, registrant): - cr = self.CR.create({"request_type_id": self.cr_type.id, "registrant_id": registrant.id}) - return cr.get_detail() + # 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): - detail = self._make_detail(self.test_individual) + _cr, detail = self._make_cr_and_detail() with self.assertRaises(ValidationError): - self._as_cr_user(detail).write({"program_id": self.program_cross_company.id}) + 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.CR.create({"request_type_id": self.cr_type.id, "registrant_id": self.test_individual.id}) + cr = self._make_cr() with self.assertRaises(ValidationError): - self._as_cr_user(self.env["spp.cr.detail.assign_program"]).create( + self._cr_user_env("spp.cr.detail.assign_program").create( { "change_request_id": cr.id, "program_id": self.program_cross_company.id, @@ -109,6 +125,16 @@ def test_reject_cross_company_program_on_create(self): ) def test_allow_visible_program(self): - detail = self._make_detail(self.test_individual) - self._as_cr_user(detail).write({"program_id": self.program_visible.id}) + _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)