From 698b72f9c5b678ff3b34530f4bd99b21691207cc Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Wed, 26 Aug 2026 13:56:37 +0700 Subject: [PATCH 1/3] fix(api): report an authorization failure as 403, not 409 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- spp_api_v2_change_request/README.rst | 15 +++++ spp_api_v2_change_request/__manifest__.py | 2 +- spp_api_v2_change_request/readme/HISTORY.md | 4 ++ .../routers/change_request.py | 25 ++++++-- .../static/description/index.html | 18 +++++- spp_api_v2_change_request/tests/__init__.py | 1 + .../tests/test_error_status_mapping.py | 59 +++++++++++++++++++ 7 files changed, 117 insertions(+), 7 deletions(-) create mode 100644 spp_api_v2_change_request/tests/test_error_status_mapping.py diff --git a/spp_api_v2_change_request/README.rst b/spp_api_v2_change_request/README.rst index 3e61291f1..62e742ffa 100644 --- a/spp_api_v2_change_request/README.rst +++ b/spp_api_v2_change_request/README.rst @@ -628,6 +628,21 @@ A complete workflow from creation to application: Changelog ========= +19.0.2.0.2 +~~~~~~~~~~ + +- fix(api): an authorization failure on a change-request state + transition now returns ``403 Forbidden`` instead of ``409 Conflict``. + ``AccessError`` subclasses ``UserError`` in Odoo, so the ``$submit`` / + ``$approve`` / ``$apply`` / ``$reset`` endpoints — which caught + ``UserError`` and returned a conflict — reported permission failures + as conflicts. A client is then told to resolve a conflict it cannot + see, and one that retries on 409 loops on a permission error that will + never clear. Reachable on ``$apply`` in particular now that applying + requires the change-request manager role, where the endpoint's own + scope check already returned 403, so the same endpoint reported two + authorization failures with different statuses. + 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_api_v2_change_request/__manifest__.py b/spp_api_v2_change_request/__manifest__.py index 56a1e1cd3..e1d9bea72 100644 --- a/spp_api_v2_change_request/__manifest__.py +++ b/spp_api_v2_change_request/__manifest__.py @@ -1,7 +1,7 @@ { # pylint: disable=pointless-statement "name": "OpenSPP API V2 - Change Request", "category": "OpenSPP/Integration", - "version": "19.0.2.0.1", + "version": "19.0.2.0.2", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_api_v2_change_request/readme/HISTORY.md b/spp_api_v2_change_request/readme/HISTORY.md index 1db815178..ca3d844e0 100644 --- a/spp_api_v2_change_request/readme/HISTORY.md +++ b/spp_api_v2_change_request/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.0.2 + +- fix(api): an authorization failure on a change-request state transition now returns `403 Forbidden` instead of `409 Conflict`. `AccessError` subclasses `UserError` in Odoo, so the `$submit` / `$approve` / `$apply` / `$reset` endpoints — which caught `UserError` and returned a conflict — reported permission failures as conflicts. A client is then told to resolve a conflict it cannot see, and one that retries on 409 loops on a permission error that will never clear. Reachable on `$apply` in particular now that applying requires the change-request manager role, where the endpoint's own scope check already returned 403, so the same endpoint reported two authorization failures with different statuses. + ### 19.0.2.0.1 - fix: skip field types before getattr and isolate detail prefetch (#129) diff --git a/spp_api_v2_change_request/routers/change_request.py b/spp_api_v2_change_request/routers/change_request.py index 85a9945b3..84c6a39b2 100644 --- a/spp_api_v2_change_request/routers/change_request.py +++ b/spp_api_v2_change_request/routers/change_request.py @@ -6,7 +6,7 @@ from urllib.parse import urlencode from odoo.api import Environment -from odoo.exceptions import UserError, ValidationError +from odoo.exceptions import AccessError, UserError, ValidationError from odoo.addons.fastapi.dependencies import odoo_env from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client @@ -43,6 +43,21 @@ change_request_router = APIRouter(tags=["ChangeRequest"], prefix="/ChangeRequest") +def _status_for_odoo_error(exc: Exception) -> int: + """Map an Odoo exception raised by a state transition to an HTTP status. + + ``AccessError`` must be distinguished before ``UserError``: it subclasses + ``UserError`` in Odoo, so a bare ``except UserError`` reports an + authorization failure as ``409 Conflict``. That is wrong twice over -- a + 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. + """ + if isinstance(exc, AccessError): + return status.HTTP_403_FORBIDDEN + return status.HTTP_409_CONFLICT + + def _build_reference(p1: str, p2: str, p3: str) -> str: """Reconstruct CR reference from path segments (e.g., CR/2026/00001).""" return f"{p1}/{p2}/{p3}" @@ -372,7 +387,7 @@ async def submit_change_request( service.submit(cr) except UserError as e: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=_status_for_odoo_error(e), detail=str(e), ) from e @@ -414,7 +429,7 @@ async def approve_change_request( service.approve(cr, comment=comment) except UserError as e: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=_status_for_odoo_error(e), detail=str(e), ) from e @@ -536,7 +551,7 @@ async def apply_change_request( service.apply(cr) except UserError as e: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=_status_for_odoo_error(e), detail=str(e), ) from e @@ -576,7 +591,7 @@ async def reset_change_request( service.reset_to_draft(cr) except UserError as e: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=_status_for_odoo_error(e), detail=str(e), ) from e diff --git a/spp_api_v2_change_request/static/description/index.html b/spp_api_v2_change_request/static/description/index.html index 3f8fbb3fd..de81842da 100644 --- a/spp_api_v2_change_request/static/description/index.html +++ b/spp_api_v2_change_request/static/description/index.html @@ -947,13 +947,29 @@

Changelog

+

19.0.2.0.2

+ +
+

19.0.2.0.1

-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_api_v2_change_request/tests/__init__.py b/spp_api_v2_change_request/tests/__init__.py index 53612c7d6..695c1e24a 100644 --- a/spp_api_v2_change_request/tests/__init__.py +++ b/spp_api_v2_change_request/tests/__init__.py @@ -2,3 +2,4 @@ from . import test_change_request_api from . import test_change_request_service from . import test_change_request_type_schema +from . import test_error_status_mapping diff --git a/spp_api_v2_change_request/tests/test_error_status_mapping.py b/spp_api_v2_change_request/tests/test_error_status_mapping.py new file mode 100644 index 000000000..d560793d7 --- /dev/null +++ b/spp_api_v2_change_request/tests/test_error_status_mapping.py @@ -0,0 +1,59 @@ +# Part of OpenSPP. See LICENSE file for full copyright and licensing details. +"""An authorization failure must surface as 403, not 409. + +``AccessError`` subclasses ``UserError`` in Odoo, so the state-transition +endpoints -- which caught ``UserError`` and returned ``409 Conflict`` -- reported +permission failures as conflicts. 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. + +This became reachable on ``$apply`` once applying a change request began +requiring the change-request manager role: the endpoint's own scope check +already returns 403, so the two authorization failures on one endpoint reported +different statuses. +""" + +from odoo.exceptions import AccessError, UserError, ValidationError +from odoo.tests import TransactionCase, tagged + +from ..routers.change_request import _status_for_odoo_error + + +@tagged("post_install", "-at_install") +class TestErrorStatusMapping(TransactionCase): + def test_access_error_is_forbidden(self): + self.assertEqual(_status_for_odoo_error(AccessError("nope")), 403) + + def test_plain_user_error_is_conflict(self): + self.assertEqual(_status_for_odoo_error(UserError("wrong state")), 409) + + def test_validation_error_is_conflict(self): + """Documents current behaviour; arguably 422, but out of scope here.""" + self.assertEqual(_status_for_odoo_error(ValidationError("bad")), 409) + + def test_access_error_is_not_shadowed_by_its_base_class(self): + """The whole bug: AccessError *is* a UserError, so order matters.""" + self.assertIsInstance(AccessError("nope"), UserError) + self.assertNotEqual( + _status_for_odoo_error(AccessError("nope")), + _status_for_odoo_error(UserError("nope")), + "an authorization failure must not report the same status as a conflict", + ) + + def test_every_state_transition_handler_uses_the_mapping(self): + """Guard against a new endpoint reintroducing a bare 409 for UserError.""" + import inspect + + from ..routers import change_request as module + + source = inspect.getsource(module) + blocks = source.split("except UserError as e:")[1:] + self.assertTrue(blocks, "expected at least one UserError handler to exist") + for block in blocks: + head = block[:200] + self.assertIn( + "_status_for_odoo_error", + head, + "a UserError handler returns a hard-coded status; AccessError would be reported as a conflict again", + ) From cbed615142e3fe3a066a819eb1705803d9d55688 Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Thu, 27 Aug 2026 17:30:02 +0700 Subject: [PATCH 2/3] fix(api): $reject and $request-revision report authorization failures as 403 too The same AccessError-shadowed-by-UserError bug fixed on the other four state-transition endpoints: both handlers caught (UserError, ValidationError) and returned a hard-coded 409, so an authorization failure surfaced as a conflict. Both now use _status_for_odoo_error; ValidationError still falls through to 409 unchanged, as it subclasses UserError. AccessDenied now maps to 403 alongside AccessError, matching the platform's global FastAPI error handler. The source-scan guard was blind to the tuple form of the except clause, which is exactly how the two missed handlers spelled it; it now matches except handlers on the AST. New route-level tests exercise the mapping through the real FastAPI handlers over HTTP. --- spp_api_v2_change_request/README.rst | 15 +- spp_api_v2_change_request/readme/HISTORY.md | 2 +- .../routers/change_request.py | 16 +- .../static/description/index.html | 15 +- .../tests/test_error_status_mapping.py | 146 ++++++++++++++++-- 5 files changed, 164 insertions(+), 30 deletions(-) diff --git a/spp_api_v2_change_request/README.rst b/spp_api_v2_change_request/README.rst index 62e742ffa..1522874fc 100644 --- a/spp_api_v2_change_request/README.rst +++ b/spp_api_v2_change_request/README.rst @@ -633,15 +633,18 @@ Changelog - fix(api): an authorization failure on a change-request state transition now returns ``403 Forbidden`` instead of ``409 Conflict``. - ``AccessError`` subclasses ``UserError`` in Odoo, so the ``$submit`` / - ``$approve`` / ``$apply`` / ``$reset`` endpoints — which caught - ``UserError`` and returned a conflict — reported permission failures - as conflicts. A client is then told to resolve a conflict it cannot - see, and one that retries on 409 loops on a permission error that will + ``AccessError`` subclasses ``UserError`` in Odoo, so all six + state-transition endpoints — ``$submit`` / ``$approve`` / ``$reject`` + / ``$request-revision`` / ``$apply`` / ``$reset`` — which caught + ``UserError`` and returned a conflict, reported permission failures as + conflicts. A client is then told to resolve a conflict it cannot see, + and one that retries on 409 loops on a permission error that will never clear. Reachable on ``$apply`` in particular now that applying requires the change-request manager role, where the endpoint's own scope check already returned 403, so the same endpoint reported two - authorization failures with different statuses. + authorization failures with different statuses. ``AccessDenied`` maps + to 403 the same way, matching the platform's global FastAPI error + handler. 19.0.2.0.1 ~~~~~~~~~~ diff --git a/spp_api_v2_change_request/readme/HISTORY.md b/spp_api_v2_change_request/readme/HISTORY.md index ca3d844e0..ffadca302 100644 --- a/spp_api_v2_change_request/readme/HISTORY.md +++ b/spp_api_v2_change_request/readme/HISTORY.md @@ -1,6 +1,6 @@ ### 19.0.2.0.2 -- fix(api): an authorization failure on a change-request state transition now returns `403 Forbidden` instead of `409 Conflict`. `AccessError` subclasses `UserError` in Odoo, so the `$submit` / `$approve` / `$apply` / `$reset` endpoints — which caught `UserError` and returned a conflict — reported permission failures as conflicts. A client is then told to resolve a conflict it cannot see, and one that retries on 409 loops on a permission error that will never clear. Reachable on `$apply` in particular now that applying requires the change-request manager role, where the endpoint's own scope check already returned 403, so the same endpoint reported two authorization failures with different statuses. +- fix(api): an authorization failure on a change-request state transition now returns `403 Forbidden` instead of `409 Conflict`. `AccessError` subclasses `UserError` in Odoo, so all six state-transition endpoints — `$submit` / `$approve` / `$reject` / `$request-revision` / `$apply` / `$reset` — which caught `UserError` and returned a conflict, reported permission failures as conflicts. A client is then told to resolve a conflict it cannot see, and one that retries on 409 loops on a permission error that will never clear. Reachable on `$apply` in particular now that applying requires the change-request manager role, where the endpoint's own scope check already returned 403, so the same endpoint reported two authorization failures with different statuses. `AccessDenied` maps to 403 the same way, matching the platform's global FastAPI error handler. ### 19.0.2.0.1 diff --git a/spp_api_v2_change_request/routers/change_request.py b/spp_api_v2_change_request/routers/change_request.py index 84c6a39b2..64f02a78a 100644 --- a/spp_api_v2_change_request/routers/change_request.py +++ b/spp_api_v2_change_request/routers/change_request.py @@ -6,7 +6,7 @@ from urllib.parse import urlencode from odoo.api import Environment -from odoo.exceptions import AccessError, UserError, ValidationError +from odoo.exceptions import AccessDenied, AccessError, UserError, ValidationError from odoo.addons.fastapi.dependencies import odoo_env from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client @@ -52,8 +52,12 @@ def _status_for_odoo_error(exc: Exception) -> int: 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. + + ``AccessDenied`` is grouped with ``AccessError``: both report an + authorization failure, and the platform's global handler + (``fastapi.error_handlers``) maps the pair to 403 the same way. """ - if isinstance(exc, AccessError): + if isinstance(exc, (AccessError, AccessDenied)): return status.HTTP_403_FORBIDDEN return status.HTTP_409_CONFLICT @@ -468,9 +472,9 @@ async def reject_change_request( try: service.reject(cr, reason=action_data.reason) - except (UserError, ValidationError) as e: + except UserError as e: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=_status_for_odoo_error(e), detail=str(e), ) from e @@ -509,9 +513,9 @@ async def request_revision_change_request( try: service.request_revision(cr, notes=action_data.notes) - except (UserError, ValidationError) as e: + except UserError as e: raise HTTPException( - status_code=status.HTTP_409_CONFLICT, + status_code=_status_for_odoo_error(e), detail=str(e), ) from e diff --git a/spp_api_v2_change_request/static/description/index.html b/spp_api_v2_change_request/static/description/index.html index de81842da..0c677065b 100644 --- a/spp_api_v2_change_request/static/description/index.html +++ b/spp_api_v2_change_request/static/description/index.html @@ -951,15 +951,18 @@

    19.0.2.0.2

    • fix(api): an authorization failure on a change-request state transition now returns 403 Forbidden instead of 409 Conflict. -AccessError subclasses UserError in Odoo, so the $submit / -$approve / $apply / $reset endpoints — which caught -UserError and returned a conflict — reported permission failures -as conflicts. A client is then told to resolve a conflict it cannot -see, and one that retries on 409 loops on a permission error that will +AccessError subclasses UserError in Odoo, so all six +state-transition endpoints — $submit / $approve / $reject +/ $request-revision / $apply / $reset — which caught +UserError and returned a conflict, reported permission failures as +conflicts. A client is then told to resolve a conflict it cannot see, +and one that retries on 409 loops on a permission error that will never clear. Reachable on $apply in particular now that applying requires the change-request manager role, where the endpoint’s own scope check already returned 403, so the same endpoint reported two -authorization failures with different statuses.
      • +authorization failures with different statuses. AccessDenied maps +to 403 the same way, matching the platform’s global FastAPI error +handler.
diff --git a/spp_api_v2_change_request/tests/test_error_status_mapping.py b/spp_api_v2_change_request/tests/test_error_status_mapping.py index d560793d7..a6fa3af2f 100644 --- a/spp_api_v2_change_request/tests/test_error_status_mapping.py +++ b/spp_api_v2_change_request/tests/test_error_status_mapping.py @@ -14,10 +14,34 @@ different statuses. """ -from odoo.exceptions import AccessError, UserError, ValidationError +import ast +import inspect +from unittest.mock import patch + +from odoo.exceptions import AccessDenied, AccessError, UserError, ValidationError from odoo.tests import TransactionCase, tagged -from ..routers.change_request import _status_for_odoo_error +from odoo.addons.fastapi.tests.common import FastAPITransactionCase +from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client + +from ..routers.change_request import _status_for_odoo_error, change_request_router +from ..services.change_request_service import ChangeRequestService +from .common import ChangeRequestTestCase + + +def _caught_exception_names(handler): + """Names of the exception classes an ``except`` clause catches.""" + node = handler.type + if node is None: + return set() + elts = node.elts if isinstance(node, ast.Tuple) else [node] + names = set() + for elt in elts: + if isinstance(elt, ast.Name): + names.add(elt.id) + elif isinstance(elt, ast.Attribute): + names.add(elt.attr) + return names @tagged("post_install", "-at_install") @@ -25,6 +49,11 @@ class TestErrorStatusMapping(TransactionCase): def test_access_error_is_forbidden(self): self.assertEqual(_status_for_odoo_error(AccessError("nope")), 403) + def test_access_denied_is_forbidden(self): + """Both authorization exceptions map to 403, mirroring the platform's + global handler (``fastapi.error_handlers`` groups them the same way).""" + self.assertEqual(_status_for_odoo_error(AccessDenied()), 403) + def test_plain_user_error_is_conflict(self): self.assertEqual(_status_for_odoo_error(UserError("wrong state")), 409) @@ -42,18 +71,113 @@ def test_access_error_is_not_shadowed_by_its_base_class(self): ) def test_every_state_transition_handler_uses_the_mapping(self): - """Guard against a new endpoint reintroducing a bare 409 for UserError.""" - import inspect + """Guard against a handler reintroducing a bare 409 for UserError. + Matched on the AST, not on a source substring: a string match on + ``except UserError as e:`` is blind to the tuple form + ``except (UserError, ValidationError) as e:`` and to renamed bindings, + which is exactly how the handlers this guard once missed spelled it. + Handlers that catch only ``ValidationError`` (create/update map it to + 422) are intentionally out of scope: ``ValidationError`` never carries + an authorization failure. + """ from ..routers import change_request as module - source = inspect.getsource(module) - blocks = source.split("except UserError as e:")[1:] - self.assertTrue(blocks, "expected at least one UserError handler to exist") - for block in blocks: - head = block[:200] + tree = ast.parse(inspect.getsource(module)) + handlers = [ + node + for node in ast.walk(tree) + if isinstance(node, ast.ExceptHandler) and "UserError" in _caught_exception_names(node) + ] + self.assertTrue(handlers, "expected at least one UserError handler to exist") + for handler in handlers: + names_used = {node.id for stmt in handler.body for node in ast.walk(stmt) if isinstance(node, ast.Name)} self.assertIn( "_status_for_odoo_error", - head, - "a UserError handler returns a hard-coded status; AccessError would be reported as a conflict again", + names_used, + f"the UserError handler at line {handler.lineno} returns a hard-coded status; " + "AccessError would be reported as a conflict again", + ) + + +@tagged("post_install", "-at_install") +class TestTransitionRoutesStatusMapping(FastAPITransactionCase, ChangeRequestTestCase): + """The AccessError -> 403 mapping, exercised through the real routes. + + The unit tests above call ``_status_for_odoo_error`` in isolation; these + call the actual FastAPI handlers over HTTP. The change-request record rules + apply one domain to read and write alike, so a CR that is readable but not + writable cannot be constructed from data alone; the ``AccessError`` is + therefore injected at the service boundary. The route, the handler and its + ``except`` clause are the real ones. + """ + + @classmethod + def setUpClass(cls): + super().setUpClass() + cls.default_fastapi_router = change_request_router + + org_type = cls.env["spp.consent.org.type"].search([("code", "=", "government")], limit=1) + if not org_type: + org_type = cls.env["spp.consent.org.type"].create( + { + "name": "Government", + "code": "government", + } ) + partner = cls.env["res.partner"].create({"name": "Route Test Org"}) + cls.api_client = cls.env["spp.api.client"].create( + { + "name": "Route Test Client", + "partner_id": partner.id, + "organization_type_id": org_type.id, + } + ) + # The action selection has no per-verb "approve"/"apply" values, so + # "all" is the only value that satisfies those scope checks. + cls.env["spp.api.client.scope"].create( + { + "client_id": cls.api_client.id, + "resource": "change_request", + "action": "all", + } + ) + api_client = cls.api_client + cls.default_fastapi_dependency_overrides = {get_authenticated_client: lambda: api_client} + + cls.change_request = cls.cr_model.create( + { + "request_type_id": cls.cr_type_edit.id, + "registrant_id": cls.registrant.id, + } + ) + + def _post(self, action, json=None): + with self._create_test_client() as client: + return client.post(f"/ChangeRequest/{self.change_request.name}/{action}", json=json) + + def test_reject_reports_access_error_as_forbidden(self): + with patch.object(ChangeRequestService, "reject", side_effect=AccessError("denied")): + response = self._post("$reject", json={"reason": "duplicate request"}) + self.assertEqual(response.status_code, 403) + + def test_request_revision_reports_access_error_as_forbidden(self): + with patch.object(ChangeRequestService, "request_revision", side_effect=AccessError("denied")): + response = self._post("$request-revision", json={"notes": "please clarify"}) + self.assertEqual(response.status_code, 403) + + def test_apply_reports_access_error_as_forbidden(self): + with patch.object(ChangeRequestService, "apply", side_effect=AccessError("denied")): + response = self._post("$apply") + self.assertEqual(response.status_code, 403) + + def test_reject_reports_plain_user_error_as_conflict(self): + with patch.object(ChangeRequestService, "reject", side_effect=UserError("wrong state")): + response = self._post("$reject", json={"reason": "duplicate request"}) + self.assertEqual(response.status_code, 409) + + def test_reject_reports_validation_error_as_conflict(self): + """Documents current behaviour; arguably 422, but out of scope here.""" + with patch.object(ChangeRequestService, "reject", side_effect=ValidationError("bad")): + response = self._post("$reject", json={"reason": "duplicate request"}) + self.assertEqual(response.status_code, 409) From 4014496d282aaadd3301b315134a713166a72e7c Mon Sep 17 00:00:00 2001 From: Ken Lewerentz Date: Thu, 27 Aug 2026 17:35:33 +0700 Subject: [PATCH 3/3] fix(api): consistent error statuses on change-request endpoints ValidationError on a state transition now returns 422, matching what create and update already return for the same condition; a missing rejection reason or revision notes is invalid input, not a conflict. MissingError returns 404 and an authorization failure on create returns 403 instead of being swallowed into a 500 by the bare except -- both aligned with the platform's global FastAPI error handler. The 403 body is now a fixed generic detail rather than the raw Odoo message, which named models and record rules. --- spp_api_v2_change_request/README.rst | 15 +++++ spp_api_v2_change_request/__manifest__.py | 2 +- spp_api_v2_change_request/readme/HISTORY.md | 4 ++ .../routers/change_request.py | 53 ++++++++++++--- .../static/description/index.html | 20 +++++- .../tests/test_error_status_mapping.py | 66 ++++++++++++++++--- 6 files changed, 141 insertions(+), 19 deletions(-) diff --git a/spp_api_v2_change_request/README.rst b/spp_api_v2_change_request/README.rst index 1522874fc..1d25b4570 100644 --- a/spp_api_v2_change_request/README.rst +++ b/spp_api_v2_change_request/README.rst @@ -628,6 +628,21 @@ A complete workflow from creation to application: Changelog ========= +19.0.2.0.3 +~~~~~~~~~~ + +- fix(api): error statuses on change-request endpoints are now + consistent across the module and aligned with the platform's global + FastAPI error handler. ``ValidationError`` on a state transition + returns ``422 Unprocessable Entity`` (previously ``409 Conflict``), + matching what create and update already returned for the same + condition — a missing rejection reason or revision notes is invalid + input, not a conflict. ``MissingError`` returns ``404 Not Found``. An + authorization failure on create returns ``403 Forbidden`` (previously + swallowed by the generic handler as a ``500``). The ``403`` response + body is now a fixed generic detail instead of the raw Odoo message, + which named models and record rules (anti-enumeration). + 19.0.2.0.2 ~~~~~~~~~~ diff --git a/spp_api_v2_change_request/__manifest__.py b/spp_api_v2_change_request/__manifest__.py index e1d9bea72..2d0be4d33 100644 --- a/spp_api_v2_change_request/__manifest__.py +++ b/spp_api_v2_change_request/__manifest__.py @@ -1,7 +1,7 @@ { # pylint: disable=pointless-statement "name": "OpenSPP API V2 - Change Request", "category": "OpenSPP/Integration", - "version": "19.0.2.0.2", + "version": "19.0.2.0.3", "sequence": 1, "author": "OpenSPP.org", "website": "https://github.com/OpenSPP/OpenSPP2", diff --git a/spp_api_v2_change_request/readme/HISTORY.md b/spp_api_v2_change_request/readme/HISTORY.md index ffadca302..47d4d1bf8 100644 --- a/spp_api_v2_change_request/readme/HISTORY.md +++ b/spp_api_v2_change_request/readme/HISTORY.md @@ -1,3 +1,7 @@ +### 19.0.2.0.3 + +- fix(api): error statuses on change-request endpoints are now consistent across the module and aligned with the platform's global FastAPI error handler. `ValidationError` on a state transition returns `422 Unprocessable Entity` (previously `409 Conflict`), matching what create and update already returned for the same condition — a missing rejection reason or revision notes is invalid input, not a conflict. `MissingError` returns `404 Not Found`. An authorization failure on create returns `403 Forbidden` (previously swallowed by the generic handler as a `500`). The `403` response body is now a fixed generic detail instead of the raw Odoo message, which named models and record rules (anti-enumeration). + ### 19.0.2.0.2 - fix(api): an authorization failure on a change-request state transition now returns `403 Forbidden` instead of `409 Conflict`. `AccessError` subclasses `UserError` in Odoo, so all six state-transition endpoints — `$submit` / `$approve` / `$reject` / `$request-revision` / `$apply` / `$reset` — which caught `UserError` and returned a conflict, reported permission failures as conflicts. A client is then told to resolve a conflict it cannot see, and one that retries on 409 loops on a permission error that will never clear. Reachable on `$apply` in particular now that applying requires the change-request manager role, where the endpoint's own scope check already returned 403, so the same endpoint reported two authorization failures with different statuses. `AccessDenied` maps to 403 the same way, matching the platform's global FastAPI error handler. diff --git a/spp_api_v2_change_request/routers/change_request.py b/spp_api_v2_change_request/routers/change_request.py index 64f02a78a..28ee835da 100644 --- a/spp_api_v2_change_request/routers/change_request.py +++ b/spp_api_v2_change_request/routers/change_request.py @@ -6,7 +6,13 @@ from urllib.parse import urlencode from odoo.api import Environment -from odoo.exceptions import AccessDenied, AccessError, UserError, ValidationError +from odoo.exceptions import ( + AccessDenied, + AccessError, + MissingError, + UserError, + ValidationError, +) from odoo.addons.fastapi.dependencies import odoo_env from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client @@ -55,13 +61,36 @@ def _status_for_odoo_error(exc: Exception) -> int: ``AccessDenied`` is grouped with ``AccessError``: both report an authorization failure, and the platform's global handler - (``fastapi.error_handlers``) maps the pair to 403 the same way. + (``fastapi.error_handlers``) maps the pair to 403 the same way. That + handler is also the model for ``MissingError`` -> 404. ``ValidationError`` + (also a ``UserError`` subclass) is invalid input and maps to 422, the + status the create and update endpoints use for the same condition. Only a + plain ``UserError`` -- a state-machine refusal such as "only pending + change requests can be rejected" -- is a genuine conflict. """ if isinstance(exc, (AccessError, AccessDenied)): return status.HTTP_403_FORBIDDEN + if isinstance(exc, MissingError): + return status.HTTP_404_NOT_FOUND + if isinstance(exc, ValidationError): + return status.HTTP_422_UNPROCESSABLE_ENTITY return status.HTTP_409_CONFLICT +def _detail_for_odoo_error(exc: Exception) -> str: + """Client-facing detail for an Odoo exception. + + An ``AccessError`` message names models, records and rules -- internals a + client must not be able to enumerate -- so the authorization branch + returns a fixed generic detail. Other user-facing errors pass through: + their text is written for the actor ("Only pending change requests can be + rejected", "Rejection reason is required"). + """ + if isinstance(exc, (AccessError, AccessDenied)): + return "Not authorized to perform this action" + return str(exc) + + def _build_reference(p1: str, p2: str, p3: str) -> str: """Reconstruct CR reference from path segments (e.g., CR/2026/00001).""" return f"{p1}/{p2}/{p3}" @@ -96,6 +125,14 @@ async def create_change_request( try: cr = service.create(cr_data, source=source) + except (AccessError, AccessDenied) as e: + # Must precede the bare except below, which reports 500 and would + # shadow an authorization failure the way UserError once shadowed + # AccessError on the transition endpoints. + raise HTTPException( + status_code=_status_for_odoo_error(e), + detail=_detail_for_odoo_error(e), + ) from e except ValidationError as e: raise HTTPException( status_code=status.HTTP_422_UNPROCESSABLE_ENTITY, @@ -392,7 +429,7 @@ async def submit_change_request( except UserError as e: raise HTTPException( status_code=_status_for_odoo_error(e), - detail=str(e), + detail=_detail_for_odoo_error(e), ) from e return service.to_api_schema(cr) @@ -434,7 +471,7 @@ async def approve_change_request( except UserError as e: raise HTTPException( status_code=_status_for_odoo_error(e), - detail=str(e), + detail=_detail_for_odoo_error(e), ) from e return service.to_api_schema(cr) @@ -475,7 +512,7 @@ async def reject_change_request( except UserError as e: raise HTTPException( status_code=_status_for_odoo_error(e), - detail=str(e), + detail=_detail_for_odoo_error(e), ) from e return service.to_api_schema(cr) @@ -516,7 +553,7 @@ async def request_revision_change_request( except UserError as e: raise HTTPException( status_code=_status_for_odoo_error(e), - detail=str(e), + detail=_detail_for_odoo_error(e), ) from e return service.to_api_schema(cr) @@ -556,7 +593,7 @@ async def apply_change_request( except UserError as e: raise HTTPException( status_code=_status_for_odoo_error(e), - detail=str(e), + detail=_detail_for_odoo_error(e), ) from e return service.to_api_schema(cr) @@ -596,7 +633,7 @@ async def reset_change_request( except UserError as e: raise HTTPException( status_code=_status_for_odoo_error(e), - detail=str(e), + detail=_detail_for_odoo_error(e), ) from e return service.to_api_schema(cr) diff --git a/spp_api_v2_change_request/static/description/index.html b/spp_api_v2_change_request/static/description/index.html index 0c677065b..84a17c05a 100644 --- a/spp_api_v2_change_request/static/description/index.html +++ b/spp_api_v2_change_request/static/description/index.html @@ -947,6 +947,22 @@

Changelog

+

19.0.2.0.3

+ +
+

19.0.2.0.2

-
+

19.0.2.0.1

  • fix: skip field types before getattr and isolate detail prefetch (#129)
-
+

19.0.2.0.0

  • Initial migration to OpenSPP2
    • diff --git a/spp_api_v2_change_request/tests/test_error_status_mapping.py b/spp_api_v2_change_request/tests/test_error_status_mapping.py index a6fa3af2f..61e1f3771 100644 --- a/spp_api_v2_change_request/tests/test_error_status_mapping.py +++ b/spp_api_v2_change_request/tests/test_error_status_mapping.py @@ -18,13 +18,17 @@ import inspect from unittest.mock import patch -from odoo.exceptions import AccessDenied, AccessError, UserError, ValidationError +from odoo.exceptions import AccessDenied, AccessError, MissingError, UserError, ValidationError from odoo.tests import TransactionCase, tagged from odoo.addons.fastapi.tests.common import FastAPITransactionCase from odoo.addons.spp_api_v2.middleware.auth import get_authenticated_client -from ..routers.change_request import _status_for_odoo_error, change_request_router +from ..routers.change_request import ( + _detail_for_odoo_error, + _status_for_odoo_error, + change_request_router, +) from ..services.change_request_service import ChangeRequestService from .common import ChangeRequestTestCase @@ -57,9 +61,30 @@ def test_access_denied_is_forbidden(self): def test_plain_user_error_is_conflict(self): self.assertEqual(_status_for_odoo_error(UserError("wrong state")), 409) - def test_validation_error_is_conflict(self): - """Documents current behaviour; arguably 422, but out of scope here.""" - self.assertEqual(_status_for_odoo_error(ValidationError("bad")), 409) + def test_validation_error_is_unprocessable(self): + """A validation failure is invalid input, and reports 422 -- the same + status the create and update endpoints use for the same condition.""" + self.assertEqual(_status_for_odoo_error(ValidationError("bad")), 422) + + def test_missing_error_is_not_found(self): + """A record that vanished mid-transition is 404, mirroring the + platform's global handler (``fastapi.error_handlers``).""" + self.assertEqual(_status_for_odoo_error(MissingError("gone")), 404) + + def test_forbidden_detail_is_generic(self): + """An AccessError message carries model names and rule text; the + client gets a generic detail instead (anti-enumeration).""" + detail = _detail_for_odoo_error(AccessError("secret record rule on spp.change.request")) + self.assertNotIn("secret", detail) + self.assertNotIn("spp.change.request", detail) + self.assertEqual(detail, _detail_for_odoo_error(AccessDenied())) + + def test_non_forbidden_detail_passes_through(self): + self.assertEqual(_detail_for_odoo_error(UserError("wrong state")), "wrong state") + self.assertEqual( + _detail_for_odoo_error(ValidationError("Rejection reason is required")), + "Rejection reason is required", + ) def test_access_error_is_not_shadowed_by_its_base_class(self): """The whole bug: AccessError *is* a UserError, so order matters.""" @@ -176,8 +201,33 @@ def test_reject_reports_plain_user_error_as_conflict(self): response = self._post("$reject", json={"reason": "duplicate request"}) self.assertEqual(response.status_code, 409) - def test_reject_reports_validation_error_as_conflict(self): - """Documents current behaviour; arguably 422, but out of scope here.""" + def test_reject_reports_validation_error_as_unprocessable(self): with patch.object(ChangeRequestService, "reject", side_effect=ValidationError("bad")): response = self._post("$reject", json={"reason": "duplicate request"}) - self.assertEqual(response.status_code, 409) + self.assertEqual(response.status_code, 422) + + def test_reject_forbidden_detail_is_generic(self): + with patch.object( + ChangeRequestService, + "reject", + side_effect=AccessError("record rule on spp.change.request denied user 7"), + ): + response = self._post("$reject", json={"reason": "duplicate request"}) + self.assertEqual(response.status_code, 403) + self.assertNotIn("record rule", response.json()["detail"]) + + def test_create_reports_access_error_as_forbidden(self): + """create() used to swallow AccessError in its bare except and report + 500; an authorization failure there is 403 like everywhere else.""" + payload = { + "type": "ChangeRequest", + "requestType": {"code": "edit_individual"}, + "registrant": {"system": "urn:openspp:vocab:id-type", "value": "TEST-123"}, + "detail": {"given_name": "Blocked"}, + } + with ( + patch.object(ChangeRequestService, "create", side_effect=AccessError("denied")), + self._create_test_client() as client, + ): + response = client.post("/ChangeRequest", json=payload) + self.assertEqual(response.status_code, 403)