fix(api): report an authorization failure as 403, not 409

[kneckinator]

An authorization failure on a change-request state transition is reported as 409 Conflict instead of 403 Forbidden.

The bug

AccessError subclasses UserError in Odoo (odoo/exceptions.py:77). Four endpoints in spp_api_v2_change_request/routers/change_request.py — $submit, $approve, $apply and $reset — did this:

except UserError as e:
    raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) from e

so any AccessError — from a record rule, or from an authorization guard — surfaced as a conflict. That is wrong twice over:

• the client is told to resolve a conflict it cannot see; and
• a client that retries on 409 (reasonable for a genuine conflict, which may clear) loops on a permission error that never will.

Why it matters now

$apply requires the change-request manager role as of the batch-2 work (#422, #365). That guard raises AccessError, so the documented operator instruction — "Deployments applying CRs via API must grant that role" — currently manifests as a 409, which reads as retryable rather than as "you lack permission".

The endpoint's own client-scope check already returns 403, so today the same endpoint reports two authorization failures with two different statuses.

The fix

The mapping moves into _status_for_odoo_error(), and each handler calls it — one line per site, rather than a fifth copy of the same block. All four endpoints are corrected, not just $apply: any of them can raise AccessError from a record rule, and the change-request detail models gained record rules in #261.

Tests

Five, including two beyond the obvious:

• test_access_error_is_not_shadowed_by_its_base_class asserts AccessError is a UserError and that the two still map differently, encoding the root cause rather than the symptom.
• test_every_state_transition_handler_uses_the_mapping inspects the router source and fails if any handler reverts to a hard-coded status — this is precisely the bug that comes back when the next endpoint is copy-pasted.

Full spp_api_v2_change_request suite: 85 tests, 0 failures.

Noted, not changed

ValidationError also subclasses UserError, so a validation failure on these endpoints reports 409 where create() uses 422 for the same condition. Current behaviour is pinned by a test with a note; changing it is a separate API-contract decision.

Merge order

Independent — no other open PR touches spp_api_v2_change_request. 19.0.2.0.1 → 19.0.2.0.2. Worth landing alongside #422, since that PR's release notes describe the manager requirement whose error this corrects.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.37%. Comparing base (82ac4c1) to head (cbed615).
⚠️ Report is 26 commits behind head on 19.0.

Additional details and impacted files

@@            Coverage Diff             @@
##             19.0     #460      +/-   ##
==========================================
+ Coverage   76.28%   76.37%   +0.09%     
==========================================
  Files         654      654              
  Lines       44035    44077      +42     
==========================================
+ Hits        33592    33666      +74     
+ Misses      10443    10411      -32     

Flag	Coverage Δ
spp_api_v2_change_request	71.91% <100.00%> (+5.37%)	⬆️
spp_base_common	91.07% <ø> (ø)
spp_programs	66.97% <ø> (+0.23%)	⬆️
spp_registry	87.79% <ø> (ø)
spp_security	69.56% <ø> (ø)

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

Files with missing lines	Coverage Δ
...pp_api_v2_change_request/routers/change_request.py	37.63% <100.00%> (+16.20%)	⬆️

... and 8 files with indirect coverage changes

🚀 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]

The core fix is right and I verified it: AccessError subclasses UserError at odoo/exceptions.py:77, _status_for_odoo_error checks the subclass first (the only ordering that works), the suite is 85/85, and the bite check is precise — restoring the bug fails exactly test_access_error_is_forbidden and test_access_error_is_not_shadowed_by_its_base_class and nothing else. The extracted helper and the intent behind the meta-test are both good.

Requesting changes because the fix is incomplete, and — the more concerning part — the meta-test that exists to prevent that incompleteness is blind to the two handlers it missed. The two are coupled.

Must fix 1 — $reject and $request-revision have the identical AccessError→409 bug, unfixed

change_request.py:471 (reject_change_request) and :512 (request_revision_change_request) are also state transitions — _do_reject() / _do_request_revision() (services/change_request_service.py:510-529) write records through the ORM and can raise AccessError from the detail-model record rules added in #261 — but both still catch:

except (UserError, ValidationError) as e:
    raise HTTPException(status_code=status.HTTP_409_CONFLICT, detail=str(e)) from e

so an authorization failure here returns 409, the exact bug this PR removes from the other four. Both require the change_request:approve scope (the endpoint-level check already returns 403 at :454/:496), so the same endpoint reports two authz failures under two statuses — the same inconsistency the PR body calls out for $apply. (Inline anchoring isn't possible here since these lines aren't in the diff, hence the body.)

Fix — collapse each to the uniform form (both AccessError and ValidationError are UserError subclasses, so except UserError still catches both, and _status_for_odoo_error keeps ValidationError at 409 via fall-through):

except UserError as e:
    raise HTTPException(status_code=_status_for_odoo_error(e), detail=str(e)) from e

This also brings both handlers back into the meta-test's view (Must fix 2, inline on the test).

Also noted, not changed (candidates for the same follow-up)

• ValidationError status is inconsistent. create() (:95) and update() (:345) map ValidationError → 422, while the transition handlers map it → 409 (explicitly in reject/request-revision, by fall-through in the four fixed ones). reject/request-revision raise ValidationError for missing reason/notes — "missing required input", which is 422 elsewhere. Defensible to leave for a scoped fix, but if you're already editing those two handlers for finding 1, consider aligning to 422 there.
• Other UserError subclasses fall through to 409. AccessDenied (arguably 403) and MissingError (arguably 404) both subclass UserError; LockError→409 is correct. isinstance(exc, (AccessError, AccessDenied)) would make the helper principled rather than AccessError-special-cased. Low priority.
• detail=str(e) returns raw Odoo messages to the client. For a typical API service user an AccessError body is just friendly model text plus the service account's name/id (no registrant PII), and this PR doesn't worsen it — every sibling handler already returns str(e). But this is now the path you're deliberately routing to clients, and docs/principles/api-error-responses.md calls for RFC 9457 Problem Details / anti-enumeration. Worth a follow-up to return a generic 403 detail on the AccessError branch. create() also maps AccessError→500 via a bare except Exception (:100) — same class, different endpoint, same follow-up.

Verified clean

Single router in the module (no other UserError/AccessError/hard-coded-status site beyond the transition set; the 409s at :333/:339 are update()'s genuine optimistic-lock / non-draft guards, correctly left alone). Version 2.0.1→2.0.2 correct; HISTORY OCA newest-first; README/index.html regenerated consistently with renumbered anchors; changed-file set is exactly the 7 expected (no cross-module README drift); AccessError import added alongside first use.

[gonzalesedwin1123]

Must fix (Important 2): this guard is blind to the tuple form and passed over two live instances of the very bug it guards. The split token is the literal "except UserError as e:", but reject/request-revision read except (UserError, ValidationError) as e:, which does not contain that substring ('except UserError as e:' in 'except (UserError, ValidationError) as e:' → False). So test_every_state_transition_handler_uses_the_mapping inspects only the four already-fixed handlers and reports green while the two buggy ones sail through. Had the token matched, this test would have failed on this PR and forced the complete fix.

It's also gameable by trivial rewrites (as exc:, bare except UserError:) and the block[:200] window is fragile. In order of preference:

1. Best — a real end-to-end test. Call one transition endpoint as an authenticated client whose Odoo user is denied by a record rule, and assert HTTP 403. That exercises the AccessError→403 path through an actual handler; the current unit tests only call _status_for_odoo_error in isolation and never through a route.
2. At minimum, broaden the source scan to match every except clause naming UserError (e.g. a regex over except .*UserError.*: then assert the following lines reference _status_for_odoo_error), so the tuple form — and any future one — is covered. Once finding 1 collapses both handlers to except UserError, today's split would catch them, but broaden the token anyway so it can't regress.

[kneckinator]

Both done in cbed615:

Broadened scan — the guard now walks the AST instead of splitting on a source substring: every except handler whose caught types name UserError (tuple form, renamed binding, attribute form included) must reference _status_for_odoo_error in its body. Re-introducing either missed spelling now fails the test with the offending line number.

Route-level tests — TestTransitionRoutesStatusMapping calls $reject, $request-revision and $apply through the real router with FastAPITransactionCase, asserting HTTP 403 for AccessError and 409 for plain UserError through the actual handlers.

One honest deviation from your option 1: the denial is injected at the ChangeRequestService boundary (patch.object(..., side_effect=AccessError(...))) rather than provoked by a record rule. The CR record rules apply a single domain to read and write alike (rule_cr_user), so a CR that is readable but not writable cannot be constructed from data — a rule-denied user 404s at find_by_reference before ever reaching the except clause — and _check_can_reject's own guard raises UserError, not AccessError. The route, handler, and except clause under test are the real ones; only the exception's origin is synthetic.

[kneckinator]

Review addressed in cbed615.

Must fix 1 — $reject and $request-revision now use the uniform form you proposed: except UserError + _status_for_odoo_error(e). ValidationError is still caught via the subclass relationship and still falls through to 409 on these endpoints in this PR (contract unchanged here; moved in the follow-up below).

Must fix 2 — the meta-test matches except-handlers on the AST and new route-level tests exercise the mapping through the real handlers over HTTP; details in the inline reply.

Also taken from the noted list: AccessDenied now maps to 403 alongside AccessError — one line, and it mirrors the platform's global handler (fastapi/error_handlers.py maps AccessDenied | AccessError → 403).

Follow-up PR raised: #471 (stacked on this branch) implements the remaining noted items — ValidationError → 422 on transitions, MissingError → 404, generic 403 detail (anti-enumeration), and create() reporting an authorization failure as 403 instead of a bare-except 500.

Full module suite: 91 tests, 0 failures, 0 errors (was 85).

One observation from writing the route tests, out of scope for both PRs: spp.api.client.scope's action selection has no approve/apply values, so the change_request:approve / change_request:apply scope checks can only ever be satisfied by action="all" — per-verb grants for these endpoints cannot currently be configured.

[gonzalesedwin1123]

Re-review of cbed615 — both must-fixes verified, approving.

• Must fix 1: $reject and $request-revision now use except UserError + _status_for_odoo_error(e), identical to the other four transition endpoints. I swept every handler in the router: the remaining hard-coded 409s are the update endpoint's optimistic-lock mismatch and non-draft guard — genuine conflicts, correct as they are — and the except ValidationError → 422 exemption is sound, since an AccessError is not a ValidationError and propagates to the platform's global handler (403).
• Must fix 2: the guard now matches except handlers on the AST (tuple forms, attribute references, renamed bindings all covered) and asserts _status_for_odoo_error is used in the handler body — it would have caught both handlers the old substring version missed. The new FastAPITransactionCase route tests exercise the real handlers over HTTP with the AccessError injected at the service boundary, which is the right seam given the symmetric read/write record rules.
• AccessDenied → 403: correct and properly grouped — it subclasses UserError (so the handlers catch it) but not AccessError (so the isinstance tuple is needed), and it mirrors fastapi/error_handlers.py.

HISTORY rewrite covers all six endpoints, version stays 19.0.2.0.2 (unreleased, same PR), CI fully green including codecov. The remaining noted items are #471's scope as agreed.

Merge authorization is Edwin's, as usual.