Follow-up from the batch-2 review of PR #365 (security: enforce manager authorization on CR apply). The fix is correct and complete — the server-side guard (env.su or has_group("spp_change_request_v2.group_cr_manager")) covers RPC, UI, wizards, auto-apply-on-approve, and the API v2 $apply endpoint. Two small hardening items were deferred out of the security batch.
Severity: Low (test coverage + HTTP status hygiene; no exploitable gap)
1. Add an API-level authorization test
The model-level tests (spp_change_request_v2/tests/test_apply_authorization.py) prove a non-manager RPC apply is rejected and a manager apply works. But the existing API tests (spp_api_v2_change_request) run as the admin test env (which holds manager), so they don't exercise the new authorization at the $apply route. Add a test asserting a non-manager endpoint user is rejected at service.apply() / $apply, and a manager succeeds. The sink is the same action_apply, proven at the model layer — this is belt-and-suspenders for the documented API behavior change.
2. Map the gate's AccessError to HTTP 403
AccessError subclasses UserError, and spp_api_v2_change_request/routers/change_request.py (~L537) catches UserError → 409 Conflict. The rejection is correctly denied and the message preserved, but the status should be 403 Forbidden. Add an AccessError branch (or reorder handlers) in the $apply route to return 403.
References
Batch-2 review: internal/plans/batch2-fix-review.md (#365 section). Guard: spp_change_request_v2/models/change_request.py:1429.
Follow-up from the batch-2 review of PR #365 (
security: enforce manager authorization on CR apply). The fix is correct and complete — the server-side guard (env.su or has_group("spp_change_request_v2.group_cr_manager")) covers RPC, UI, wizards, auto-apply-on-approve, and the API v2$applyendpoint. Two small hardening items were deferred out of the security batch.Severity: Low (test coverage + HTTP status hygiene; no exploitable gap)
1. Add an API-level authorization test
The model-level tests (
spp_change_request_v2/tests/test_apply_authorization.py) prove a non-manager RPC apply is rejected and a manager apply works. But the existing API tests (spp_api_v2_change_request) run as the admin test env (which holds manager), so they don't exercise the new authorization at the$applyroute. Add a test asserting a non-manager endpoint user is rejected atservice.apply()/$apply, and a manager succeeds. The sink is the sameaction_apply, proven at the model layer — this is belt-and-suspenders for the documented API behavior change.2. Map the gate's
AccessErrorto HTTP 403AccessErrorsubclassesUserError, andspp_api_v2_change_request/routers/change_request.py(~L537) catchesUserError→ 409 Conflict. The rejection is correctly denied and the message preserved, but the status should be 403 Forbidden. Add anAccessErrorbranch (or reorder handlers) in the$applyroute to return 403.References
Batch-2 review:
internal/plans/batch2-fix-review.md(#365 section). Guard:spp_change_request_v2/models/change_request.py:1429.