security(graduation): enforce manager-only assessment approval server-side#267
Open
gonzalesedwin1123 wants to merge 3 commits into
Open
security(graduation): enforce manager-only assessment approval server-side#267gonzalesedwin1123 wants to merge 3 commits into
gonzalesedwin1123 wants to merge 3 commits into
Conversation
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.
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.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 19.0 #267 +/- ##
==========================================
+ Coverage 75.31% 75.34% +0.03%
==========================================
Files 1098 1098
Lines 65317 65391 +74
==========================================
+ Hits 49191 49269 +78
+ Misses 16126 16122 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
spp.graduation.assessmentgated its approve/reject/reset buttons togroup_spp_graduation_managerin the view only. The Python workflow methods checked only the state transition, andgroup_spp_graduation_userhas write/create on their own assessments (record ruleassessor_id == user.id). So a normal graduation user could, via RPC, self-approve their own assessment (action_submitthenaction_approve, which sets agraduation_date) — or bypass the methods entirely with a rawwrite({'state':'approved', ...})orcreate({'state':'approved', ...})— crossing the intended manager approval boundary and corrupting beneficiary graduation status.Fix (server-side, two layers)
action_approve/action_reject/action_reset_draftraiseAccessErrorunless the caller is a graduation manager (_is_graduation_manager()= manager group, with superuser/sudo exempt so system/programmatic flows aren't blocked).action_submitstays 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 movestatedraft → submitted(create must bedraft; no-op same-state writes allowed). This closes the raw-RPC bypass that the action guards alone would miss.Tests
action_approve/reject/reset_drafton own assessment; rawwriteofstate='approved'/graduation_date/approved_by_id;createof a non-draft assessment; un-submit (submitted→draft); create for a different assessor.state='approved',approved_by_id,graduation_date); user submit; user editingrecommendation/notes on their own draft.spp_graduation57/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_graduationonly treats a beneficiary as graduated whenstate=='approved'andgraduation_dateis set (both manager-only) — sorecommendation/scores are not levers. The three review test-gap suggestions were added.