security(graduation): enforce manager-only assessment approval server-side

[gonzalesedwin1123]

Do not merge yet — held for human review.

Summary

spp.graduation.assessment gated its approve/reject/reset buttons to group_spp_graduation_manager in the view only. The Python workflow methods checked only the state transition, and group_spp_graduation_user has write/create on their own assessments (record rule assessor_id == user.id). So a normal graduation user could, via RPC, self-approve their own assessment (action_submit then action_approve, which sets a graduation_date) — or bypass the methods entirely with a raw write({'state':'approved', ...}) or create({'state':'approved', ...}) — crossing the intended manager approval boundary and corrupting beneficiary graduation status.

Fix (server-side, two layers)

• Action guards: action_approve/action_reject/action_reset_draft raise AccessError unless the caller is a graduation manager (_is_graduation_manager() = manager group, with superuser/sudo exempt so system/programmatic flows aren't blocked). action_submit stays user-allowed.
• create() / write() guards: a non-manager may not set the manager-only outcome fields (approved_by_id, approved_date, graduation_date) and may only move state draft → submitted (create must be draft; no-op same-state writes allowed). This closes the raw-RPC bypass that the action guards alone would miss.

Tests

• Blocked (as user): action_approve/reject/reset_draft on own assessment; raw write of state='approved' / graduation_date / approved_by_id; create of a non-draft assessment; un-submit (submitted→draft); create for a different assessor.
• Still works: manager approve (sets state='approved', approved_by_id, graduation_date); user submit; user editing recommendation/notes on their own draft.
• Green: spp_graduation 57/57; lint/semgrep clean.

Reviewed

Adversarial staff review: APPROVE, no blockers. Confirmed no unguarded write path, no sudo/automation escape, and that the downstream spp_case_graduation only treats a beneficiary as graduated when state=='approved' and graduation_date is set (both manager-only) — so recommendation/scores are not levers. The three review test-gap suggestions were added.

[gemini-code-assist]

Code Review

This pull request introduces robust security restrictions to prevent non-manager users from approving, rejecting, resetting, or directly modifying workflow-related fields on graduation assessments at the ORM level. It also adds comprehensive unit tests to verify these boundaries. The review feedback suggests further strengthening security by preventing non-managers from modifying any fields on assessments that are not in the draft state, and adding a corresponding test case to verify this behavior.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[codecov]

Codecov Report

❌ Patch coverage is 97.22222% with 1 line in your changes missing coverage. Please review.
⚠️ Please upload report for BASE (19.0-staging-sec-batch1@cb7f4c6). Learn more about missing BASE report.

Files with missing lines	Patch %	Lines
spp_graduation/models/graduation_assessment.py	97.22%	1 Missing ⚠️

Additional details and impacted files

@@                    Coverage Diff                     @@
##             19.0-staging-sec-batch1     #267   +/-   ##
==========================================================
  Coverage                           ?   68.57%           
==========================================================
  Files                              ?      103           
  Lines                              ?     8523           
  Branches                           ?        0           
==========================================================
  Hits                               ?     5845           
  Misses                             ?     2678           
  Partials                           ?        0           

Flag	Coverage Δ
spp_base_common	91.07% <ø> (?)
spp_case_graduation	100.00% <ø> (?)
spp_graduation	99.40% <97.22%> (?)
spp_programs	65.27% <ø> (?)
spp_registry	86.94% <ø> (?)
spp_security	69.56% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
spp_graduation/models/graduation_assessment.py	99.17% <97.22%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[gonzalesedwin1123]

Pulled from security batch 1 (retargeted back to 19.0) after an adversarial staff review returned NEEDS-CHANGES. The other four batch-1 PRs (#327, #329, #265, #266) proceed without it; this becomes its own work track.

The core of the fix is sound and stays: the three action guards (action_approve/action_reject/action_reset_draft), the write() state machine, and the RPC-direct-write tests are correct and genuinely fail without the fix. Confirmed separately: spp_graduation does not inherit spp.approval.mixin, so #369 does not apply here; upstream drift is nil (git log f260ffd3..cb7f4c6b -- spp_graduation spp_approval spp_programs is empty); version/no-migration handling is correct; independent of #327/#329.

Must fix before this can merge

1. BLOCKER — create() guard bypassable via context defaults. The check reads vals before super().create(), but Odoo applies default_* context keys inside create (_prepare_create_values → _add_missing_default_values → default_get). So create({'partner_id': P, 'pathway_id': W}, context={'default_state': 'approved', 'default_graduation_date': ..., 'default_approved_by_id': ...}) passes the guard as a plain graduation user, and the post-create rule check passes because assessor_id defaults to self. Result: a self-issued approved assessment with a graduation date, flipping spp.case.graduation_status to graduated. Same class as fix(spp_alerts): evaluate alert rules as their owner, not the elevated cron #364's HIGH-1. Fix by verifying after super().create() (raise rolls back), which also covers ir.default records and future field defaults.
2. HIGH — the content freeze does not freeze the content. The parent locks response_ids, but the child model spp.graduation.criteria.response has no create/write/unlink override, full CRUD ACL (ir.model.access.csv:8), and a permissive record rule (graduation_rules.xml:49-58). An assessor can rewrite scores on a submitted assessment; readiness_score and is_required_criteria_met are stored computes that recompute through the field machinery, not the overridden write(), so nothing raises. The manager then approves numbers different from those submitted. The views make this worse — the responses tab is only readonly in approved/rejected, so the UI invites it.
3. HIGH — segregation of duties (decision made: implement it). action_approve checks group membership only; it never compares assessor_id to the current user. And every manager is an assessor by construction (group_spp_graduation_manager implies the user group; group_spp_admin implies manager; base.group_system implies spp_admin), with a [(1,'=',1)] manager rule — so a manager can create → submit → approve their own assessment unchallenged. Add an explicit self-approval block (exempting env.su), with an opt-out group for single-officer deployments, and pin it with a test. The changelog wording must be corrected either way.
4. MEDIUM — views not updated. The server-side freeze applies at state != 'draft' but the view readonly conditions still say state in ('approved','rejected'), so the normal assessor flow hits a raw AccessError dialog on a submitted record. Separately, a rejected assessment is unrecoverable by its author (cannot edit, cannot reset — the button is manager-gated) while the banner instructs them to reset it.

Strongly recommended in the same change

5. assessor_id is writable at any state by a non-manager — Odoo does not re-check record rules after write() (verified: BaseModel.write calls check_access once against the pre-write recordset), so an assessor can reassign authorship of a submitted or approved assessment. Audit-trail forgery. The existing "assessor-substitution" test only covers the create path, which the record rule already blocked pre-fix.
6. _LOCKED_CONTENT_FIELDS is a denylist and has already failed open — spp_case_graduation adds case_id to this model and it is in neither tuple, so a non-manager can re-point an approved assessment at an arbitrary case, and spp.case.graduation_status/graduation_readiness compute straight off the linked assessments. company_id is likewise unlocked. Inverting to an allowlist (deny anything outside an assessor-editable set, gated on draft) subsumes finding 5 and closes case_id in one change — and matches the repo's own documented preference for default-deny.
7. env.su exemption in _is_graduation_manager() is a future footgun (no sudo() caller exists today) — worth a comment naming the invariant.
8. state, approved_by_id, approved_date, graduation_date should be copy=False — currently duplicating a non-draft record raises AccessError for a non-manager instead of yielding a fresh draft.

Note for whoever picks this up: all pre-existing tests run as superuser (TransactionCase uses SUPERUSER_ID, which sets env.su), so the guard's entire coverage is the new security tests — a green suite is not evidence of breadth here. Per TDD, findings 1 and 2 need failing tests written first; both are cheap to write.