Skip to content

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

Open
kneckinator wants to merge 1 commit into
19.0from
fix/api-v2-cr-access-error-403
Open

fix(api): report an authorization failure as 403, not 409#460
kneckinator wants to merge 1 commit into
19.0from
fix/api-v2-cr-access-error-403

Conversation

@kneckinator

Copy link
Copy Markdown
Contributor

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.

AccessError subclasses UserError in Odoo, so the change-request state
transitions — $submit, $approve, $apply and $reset — which caught
UserError and returned 409 Conflict reported permission failures as
conflicts. The client is told to resolve a conflict it cannot see, and
one that retries on 409 (reasonable for a genuine conflict, which may
clear) loops on a permission error that never will.

It is most visible on $apply now that applying requires the
change-request manager role: the endpoint's own scope check already
returns 403, so the same endpoint reported two authorization failures
with different statuses.

The mapping lives in one helper rather than a fifth copy of the same
except block, and a test fails if a handler goes back to a hard-coded
status — this is exactly the bug that returns when the next endpoint is
copy-pasted.

ValidationError has the same shape (it also subclasses UserError, so a
validation failure reports 409 where create() uses 422). Current
behaviour is pinned by a test with a note rather than changed, being a
separate API-contract decision.
@codecov

codecov Bot commented Aug 26, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.28%. Comparing base (82ac4c1) to head (698b72f).

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             19.0     #460   +/-   ##
=======================================
  Coverage   76.28%   76.28%           
=======================================
  Files         654      654           
  Lines       44035    44039    +4     
=======================================
+ Hits        33592    33596    +4     
  Misses      10443    10443           
Flag Coverage Δ
spp_api_v2_change_request 66.79% <100.00%> (+0.25%) ⬆️
spp_base_common 91.07% <ø> (ø)
spp_programs 66.73% <ø> (ø)
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 23.11% <100.00%> (+1.68%) ⬆️
🚀 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant