security(spp_approval): approval_state writable directly, bypassing the approval workflow (shared mixin) [Severity: Medium]

[gonzalesedwin1123]

Follow-up to PR #365 (security(spp_change_request_v2): enforce manager authorization on CR apply). Called out in that report but scoped out because it is cross-cutting on a shared mixin.

Severity: Medium (workflow-integrity bypass; enables self-approval)

Problem

approval_state is defined on the shared spp.approval.mixin (spp_approval/models/approval_mixin.py) with no field-level groups= and no write() guard. The concrete models grant broad write to non-approver roles — e.g. spp.change.request gives group_cr_user write (ir.model.access.csv: ...group_cr_user,1,1,1,0). So a group_cr_user (or any writer of a mixin-consumer model) can write({'approval_state': 'approved'}) directly over RPC, bypassing the approval workflow (action_approve / _do_approve, which set the state via controlled, group-checked logic).

Combined with the CR apply path this was the amplifier noted in #365 (a cr_user self-marks approved). #365 closes the dangerous sudo-apply sink (apply is now manager-gated), but the underlying ability to forge approval_state remains and affects every mixin consumer:

• spp_change_request_v2 (change requests)
• spp_programs (cycle, entitlement)
• spp_studio (logic, logic_approval)
• spp_drims (request)
• spp_disability_registry (assessment)
• spp_event_data (event_data)

An actor who can write the record but is not an authorized approver could mark it approved/rejected directly, defeating the approval gate for whatever that state governs in each domain.

Suggested fix (needs design)

Options, to be decided per the mixin's intended contract:

1. Protect the field on the mixin — make approval_state computed/readonly at the ORM level and only settable through the approval methods (e.g. set via a guarded internal path / SQL as _do_approve already does), or add field-level groups — but groups must be a superset across all consumers' approver groups, which may not be uniform.
2. write() guard on the mixin — reject direct writes to approval_state unless self.env.su or the write comes from the approval workflow (a context flag set by _do_approve/_do_reject), forcing all transitions through the workflow.
3. Per-model field groups where a single approver group exists.

Because the mixin is shared and each consumer has different approver groups, this needs its own analysis + full-stack testing across all six consumers.

Origin

• PR security(spp_change_request_v2): enforce manager authorization on CR apply (server-side) #365; plan internal/plans/sec-cr-apply-manager-authz.md (open question 2).