diff --git a/spp_api_v2_change_request/README.rst b/spp_api_v2_change_request/README.rst index 1522874f..1d25b457 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 e1d9bea7..2d0be4d3 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 ffadca30..47d4d1bf 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 64f02a78..28ee835d 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 0c677065..84a17c05 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

-
+

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 a6fa3af2..61e1f377 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)