From 311c2d58b1a8717b2344381be2a05a69cb768597 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 13:49:30 +0800 Subject: [PATCH 1/4] security(graduation): enforce manager-only approval server-side spp.graduation.assessment gated its approve/reject/reset buttons to the graduation manager group in the VIEW only. The Python workflow methods checked just the state transition, and group_spp_graduation_user has write/create on their own assessments (assessor_id == user). So a normal user could, via RPC, self-approve their own assessment (action_submit then action_approve, setting a graduation_date) or bypass the methods entirely with a raw write({'state':'approved', ...}) / create({'state':'approved', ...}). Enforce the boundary server-side: - action_approve/action_reject/action_reset_draft raise AccessError unless the caller is a graduation manager (superuser/sudo exempt for system flows). - create()/write() forbid non-managers from setting the manager-only outcome fields (approved_by_id, approved_date, graduation_date) and from moving state beyond the one user-allowed transition draft -> submitted (create must be draft). Closes the raw-RPC bypass. action_submit stays user-allowed. Tests: user cannot approve/reject/reset own assessment, cannot write approved state or approval fields, cannot create a non-draft assessment; manager can still approve (graduation_date set); user can still submit. spp_graduation 54/54. --- .../models/graduation_assessment.py | 59 +++++++++++++- .../tests/test_graduation_security.py | 77 ++++++++++++++++++- 2 files changed, 134 insertions(+), 2 deletions(-) diff --git a/spp_graduation/models/graduation_assessment.py b/spp_graduation/models/graduation_assessment.py index bdcce3e19..62633fa04 100644 --- a/spp_graduation/models/graduation_assessment.py +++ b/spp_graduation/models/graduation_assessment.py @@ -1,7 +1,7 @@ from dateutil.relativedelta import relativedelta from odoo import _, api, fields, models -from odoo.exceptions import UserError, ValidationError +from odoo.exceptions import AccessError, UserError, ValidationError class GraduationAssessment(models.Model): @@ -136,6 +136,23 @@ def _compute_monitoring_end(self): else: rec.monitoring_end_date = False + # Fields only a graduation manager may set — they record the approval + # outcome and must never be writable by the assessor via RPC. + _MANAGER_ONLY_FIELDS = ("approved_by_id", "approved_date", "graduation_date") + + def _is_graduation_manager(self): + """True for graduation managers and for superuser/sudo (system) contexts. + + Approve/reject/reset are manager-only. The superuser/sudo exemption keeps + programmatic and system flows working; real end users are always checked + against the manager group. + """ + return self.env.su or self.env.user.has_group("spp_graduation.group_spp_graduation_manager") + + def _ensure_graduation_manager(self): + if not self._is_graduation_manager(): + raise AccessError(_("Only graduation managers can approve, reject, or reset graduation assessments.")) + def action_submit(self): for rec in self: if rec.state != "draft": @@ -143,6 +160,7 @@ def action_submit(self): rec.state = "submitted" def action_approve(self): + self._ensure_graduation_manager() for rec in self: if rec.state != "submitted": raise UserError(_("Only submitted assessments can be approved.")) @@ -157,17 +175,56 @@ def action_approve(self): rec.graduation_date = fields.Date.today() def action_reject(self): + self._ensure_graduation_manager() for rec in self: if rec.state != "submitted": raise UserError(_("Only submitted assessments can be rejected.")) rec.state = "rejected" def action_reset_draft(self): + self._ensure_graduation_manager() for rec in self: if rec.state not in ("submitted", "rejected"): raise UserError(_("Only submitted or rejected assessments can be reset to draft.")) rec.state = "draft" + @api.model_create_multi + def create(self, vals_list): + # A non-manager may only create draft assessments and may not preset the + # approval outcome fields (blocks create({'state':'approved', ...})). + if not self._is_graduation_manager(): + for vals in vals_list: + forbidden = [f for f in self._MANAGER_ONLY_FIELDS if vals.get(f)] + if forbidden: + raise AccessError( + _("Only graduation managers can set assessment approval fields: %s.") % ", ".join(forbidden) + ) + if vals.get("state", "draft") != "draft": + raise AccessError(_("Only graduation managers can create an assessment in a non-draft state.")) + return super().create(vals_list) + + def write(self, vals): + # Enforce the approval boundary at the ORM so it cannot be bypassed via + # RPC (the view button groups are UI-only). Non-managers may not set the + # manager-only outcome fields, and may only move state draft -> submitted. + if not self._is_graduation_manager(): + forbidden = [f for f in self._MANAGER_ONLY_FIELDS if f in vals] + if forbidden: + raise AccessError( + _("Only graduation managers can set assessment approval fields: %s.") % ", ".join(forbidden) + ) + if "state" in vals: + new_state = vals["state"] + for rec in self: + if new_state != rec.state and (rec.state, new_state) != ("draft", "submitted"): + raise AccessError( + _( + "Only graduation managers can change an assessment's state; " + "you may only submit a draft for approval." + ) + ) + return super().write(vals) + class GraduationCriteriaResponse(models.Model): _name = "spp.graduation.criteria.response" diff --git a/spp_graduation/tests/test_graduation_security.py b/spp_graduation/tests/test_graduation_security.py index 97308b6d0..ec1d6b848 100644 --- a/spp_graduation/tests/test_graduation_security.py +++ b/spp_graduation/tests/test_graduation_security.py @@ -1,6 +1,6 @@ """Security tests for graduation module.""" -from odoo import Command +from odoo import Command, fields from odoo.exceptions import AccessError from odoo.tests import TransactionCase, tagged @@ -190,3 +190,78 @@ def test_manager_can_reject(self): assessment.action_submit() assessment.with_user(self.manager).action_reject() self.assertEqual(assessment.state, "rejected") + + # ────────────────────────────────────────────────────────────────────────── + # Approval boundary — a graduation USER must not approve (or otherwise drive + # the approval workflow of) their own assessment, via the action methods OR + # via direct RPC write/create of the workflow fields. + # ────────────────────────────────────────────────────────────────────────── + + def _submitted_assessment_owned_by_user(self): + pathway = self.env["spp.graduation.pathway"].create({"name": "Test Pathway"}) + assessment = ( + self.env["spp.graduation.assessment"] + .with_user(self.user) + .create( + { + "pathway_id": pathway.id, + "partner_id": self.beneficiary.id, + "recommendation": "graduate", + } + ) + ) + assessment.action_submit() + self.assertEqual(assessment.state, "submitted") + return assessment + + def test_user_cannot_approve_own_assessment(self): + """Reported self-approval: a user must not approve their own assessment.""" + assessment = self._submitted_assessment_owned_by_user() + with self.assertRaises(AccessError): + assessment.action_approve() + self.assertEqual(assessment.state, "submitted") + self.assertFalse(assessment.graduation_date) + + def test_user_cannot_reject_or_reset_own_assessment(self): + assessment = self._submitted_assessment_owned_by_user() + with self.assertRaises(AccessError): + assessment.action_reject() + with self.assertRaises(AccessError): + assessment.action_reset_draft() + self.assertEqual(assessment.state, "submitted") + + def test_user_cannot_write_approved_state_directly(self): + """Raw-RPC bypass of the action methods must also be blocked.""" + assessment = self._submitted_assessment_owned_by_user() + with self.assertRaises(AccessError): + assessment.write({"state": "approved"}) + self.assertEqual(assessment.state, "submitted") + + def test_user_cannot_write_approval_fields_directly(self): + assessment = self._submitted_assessment_owned_by_user() + with self.assertRaises(AccessError): + assessment.write({"graduation_date": fields.Date.today()}) + with self.assertRaises(AccessError): + assessment.write({"approved_by_id": self.user.id}) + + def test_user_cannot_create_non_draft_assessment(self): + """Creating an already-approved assessment via RPC must be blocked.""" + pathway = self.env["spp.graduation.pathway"].create({"name": "Test Pathway"}) + with self.assertRaises(AccessError): + self.env["spp.graduation.assessment"].with_user(self.user).create( + { + "pathway_id": pathway.id, + "partner_id": self.beneficiary.id, + "state": "approved", + "graduation_date": fields.Date.today(), + } + ) + + def test_manager_approve_sets_graduation_date(self): + """Regression: a manager can still approve, and graduation_date is set for + a 'graduate' recommendation.""" + assessment = self._submitted_assessment_owned_by_user() + assessment.with_user(self.manager).action_approve() + self.assertEqual(assessment.state, "approved") + self.assertEqual(assessment.approved_by_id, self.manager) + self.assertTrue(assessment.graduation_date) From 3eccf0f34fa02005fd769ff706c205c9bdad9677 Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 13:56:36 +0800 Subject: [PATCH 2/4] test(graduation): cover draft-edit, un-submit, and assessor-substitution Post-review defense-in-depth tests: a user can still edit recommendation/notes on their own draft (guard does not over-block); a user cannot un-submit (submitted -> draft is manager-only); a user cannot create an assessment for a different assessor (record-rule protection). spp_graduation 57/57. --- .../tests/test_graduation_security.py | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/spp_graduation/tests/test_graduation_security.py b/spp_graduation/tests/test_graduation_security.py index ec1d6b848..db2c19d9c 100644 --- a/spp_graduation/tests/test_graduation_security.py +++ b/spp_graduation/tests/test_graduation_security.py @@ -265,3 +265,35 @@ def test_manager_approve_sets_graduation_date(self): self.assertEqual(assessment.state, "approved") self.assertEqual(assessment.approved_by_id, self.manager) self.assertTrue(assessment.graduation_date) + + def test_user_can_edit_own_draft_content(self): + """The guard must not over-block: a user can still edit non-workflow + content on their own draft assessment.""" + pathway = self.env["spp.graduation.pathway"].create({"name": "Test Pathway"}) + assessment = ( + self.env["spp.graduation.assessment"] + .with_user(self.user) + .create({"pathway_id": pathway.id, "partner_id": self.beneficiary.id}) + ) + assessment.write({"recommendation": "graduate", "recommendation_notes": "ready"}) + self.assertEqual(assessment.recommendation, "graduate") + + def test_user_cannot_unsubmit_own_assessment(self): + """A user may only move draft -> submitted; un-submitting is manager-only.""" + assessment = self._submitted_assessment_owned_by_user() + with self.assertRaises(AccessError): + assessment.write({"state": "draft"}) + self.assertEqual(assessment.state, "submitted") + + def test_user_cannot_create_assessment_for_another_assessor(self): + """A user cannot create an assessment attributed to a different assessor + (record-rule protection; documents the boundary).""" + pathway = self.env["spp.graduation.pathway"].create({"name": "Test Pathway"}) + with self.assertRaises(AccessError): + self.env["spp.graduation.assessment"].with_user(self.user).create( + { + "pathway_id": pathway.id, + "partner_id": self.beneficiary.id, + "assessor_id": self.manager.id, + } + ) From 3404f7710f432f0614ec466920eb99f2ea50df5b Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 14:54:16 +0800 Subject: [PATCH 3/4] security(graduation): freeze assessment content once submitted MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address Gemini review (security-high) on PR #267: the ORM guards blocked state changes and the manager-only outcome fields, but a non-manager assessor could still edit content (recommendation, partner_id, responses, ...) on an already submitted/approved/rejected assessment — e.g. submit recommendation='extend', flip to 'graduate' while pending, and the manager's approval then sets a graduation_date for a recommendation they did not review. Freeze the assessor-editable content fields (partner_id, pathway_id, assessment_date, recommendation, recommendation_notes, response_ids) for non-managers once the assessment leaves draft; editing requires a manager reset to draft. Only these business fields are frozen, so chatter/activity/technical writes on submitted records are unaffected. Manager/superuser unrestricted. Test: a user cannot change recommendation/notes on a submitted assessment. spp_graduation 58/58. --- .../models/graduation_assessment.py | 25 +++++++++++++++++++ .../tests/test_graduation_security.py | 10 ++++++++ 2 files changed, 35 insertions(+) diff --git a/spp_graduation/models/graduation_assessment.py b/spp_graduation/models/graduation_assessment.py index 62633fa04..6e87f4242 100644 --- a/spp_graduation/models/graduation_assessment.py +++ b/spp_graduation/models/graduation_assessment.py @@ -140,6 +140,19 @@ def _compute_monitoring_end(self): # outcome and must never be writable by the assessor via RPC. _MANAGER_ONLY_FIELDS = ("approved_by_id", "approved_date", "graduation_date") + # Assessor-editable content. Once the assessment leaves draft it is awaiting + # the manager's decision, so a non-manager must not change what will be + # approved (e.g. flip recommendation to "graduate" after submitting). Only + # these business fields are frozen — technical/chatter writes are unaffected. + _LOCKED_CONTENT_FIELDS = ( + "partner_id", + "pathway_id", + "assessment_date", + "recommendation", + "recommendation_notes", + "response_ids", + ) + def _is_graduation_manager(self): """True for graduation managers and for superuser/sudo (system) contexts. @@ -213,6 +226,18 @@ def write(self, vals): raise AccessError( _("Only graduation managers can set assessment approval fields: %s.") % ", ".join(forbidden) ) + # Content is frozen once the assessment leaves draft: the manager must + # approve exactly what was submitted (e.g. no flipping recommendation + # to "graduate" after submission). A manager can reset it to draft. + if any(f in vals for f in self._LOCKED_CONTENT_FIELDS): + for rec in self: + if rec.state != "draft": + raise AccessError( + _( + "A graduation assessment can only be edited while it is in draft. " + "Ask a manager to reset it to draft to make changes." + ) + ) if "state" in vals: new_state = vals["state"] for rec in self: diff --git a/spp_graduation/tests/test_graduation_security.py b/spp_graduation/tests/test_graduation_security.py index db2c19d9c..5b9f20961 100644 --- a/spp_graduation/tests/test_graduation_security.py +++ b/spp_graduation/tests/test_graduation_security.py @@ -278,6 +278,16 @@ def test_user_can_edit_own_draft_content(self): assessment.write({"recommendation": "graduate", "recommendation_notes": "ready"}) self.assertEqual(assessment.recommendation, "graduate") + def test_user_cannot_modify_submitted_assessment_content(self): + """A user must not change assessment content once it is submitted — else + they could flip the recommendation the manager is about to approve.""" + assessment = self._submitted_assessment_owned_by_user() + with self.assertRaises(AccessError): + assessment.write({"recommendation": "extend"}) + with self.assertRaises(AccessError): + assessment.write({"recommendation_notes": "changed after submit"}) + self.assertEqual(assessment.recommendation, "graduate") + def test_user_cannot_unsubmit_own_assessment(self): """A user may only move draft -> submitted; un-submitting is manager-only.""" assessment = self._submitted_assessment_owned_by_user() From c2a6e695cafc5a5fee7de25e3b9c99f11c25002d Mon Sep 17 00:00:00 2001 From: Edwin Gonzales Date: Thu, 2 Jul 2026 22:22:05 +0800 Subject: [PATCH 4/4] chore(spp_graduation): bump version to 19.0.2.0.2 + changelog Record the manager-only assessment approval hardening in the version and HISTORY changelog (regenerated README). --- spp_graduation/README.rst | 12 ++++++++++++ spp_graduation/__manifest__.py | 2 +- spp_graduation/readme/HISTORY.md | 4 ++++ spp_graduation/static/description/index.html | 15 ++++++++++++++- 4 files changed, 31 insertions(+), 2 deletions(-) diff --git a/spp_graduation/README.rst b/spp_graduation/README.rst index 430b8e472..42b600376 100644 --- a/spp_graduation/README.rst +++ b/spp_graduation/README.rst @@ -632,6 +632,18 @@ Test 10: Edge Cases Changelog ========= +19.0.2.0.2 +~~~~~~~~~~ + +- fix(security): enforce manager-only approval of graduation assessments + server-side. ``action_approve`` / ``action_reject`` / + ``action_reset_draft`` now require the graduation manager group (the + view-level button gating was UI-only and bypassable via RPC), and + non-managers can no longer set the approval fields (``approved_by_id`` + / ``approved_date`` / ``graduation_date``), move ``state`` beyond + ``draft → submitted``, or edit a submitted assessment's content. + Prevents a user from self-approving their own assessment. + 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_graduation/__manifest__.py b/spp_graduation/__manifest__.py index c0b3fb07d..3d88a3005 100644 --- a/spp_graduation/__manifest__.py +++ b/spp_graduation/__manifest__.py @@ -2,7 +2,7 @@ { "name": "OpenSPP Graduation Management", "summary": "Manage graduation and exit from time-bound social protection programs", - "version": "19.0.2.0.1", + "version": "19.0.2.0.2", "category": "OpenSPP", "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_graduation/readme/HISTORY.md b/spp_graduation/readme/HISTORY.md index 79088cff3..00958f00c 100644 --- a/spp_graduation/readme/HISTORY.md +++ b/spp_graduation/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.0.2 + +- fix(security): enforce manager-only approval of graduation assessments server-side. `action_approve` / `action_reject` / `action_reset_draft` now require the graduation manager group (the view-level button gating was UI-only and bypassable via RPC), and non-managers can no longer set the approval fields (`approved_by_id` / `approved_date` / `graduation_date`), move `state` beyond `draft → submitted`, or edit a submitted assessment's content. Prevents a user from self-approving their own assessment. + ### 19.0.2.0.1 - fix(views): add a "Graduation Criteria" menu item directly under the Graduation root, plus a list/form/search view and action for `spp.graduation.criteria`. The model and ACL were already shipped, but no UI surface existed — criteria could only be edited indirectly through the pathway form. Visible to `group_spp_graduation_user` and above. diff --git a/spp_graduation/static/description/index.html b/spp_graduation/static/description/index.html index 097a6bad7..cdc9a077d 100644 --- a/spp_graduation/static/description/index.html +++ b/spp_graduation/static/description/index.html @@ -1005,6 +1005,19 @@

Changelog

+

19.0.2.0.2

+
    +
  • fix(security): enforce manager-only approval of graduation assessments +server-side. action_approve / action_reject / +action_reset_draft now require the graduation manager group (the +view-level button gating was UI-only and bypassable via RPC), and +non-managers can no longer set the approval fields (approved_by_id +/ approved_date / graduation_date), move state beyond +draft → submitted, or edit a submitted assessment’s content. +Prevents a user from self-approving their own assessment.
    • +
+
+

19.0.2.0.1

  • fix(views): add a “Graduation Criteria” menu item directly under the @@ -1019,7 +1032,7 @@

    19.0.2.0.1

    the Settings → Users access-rights UI.
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2