diff --git a/spp_api_v2_change_request/README.rst b/spp_api_v2_change_request/README.rst
index 3e61291f1..1522874fc 100644
--- a/spp_api_v2_change_request/README.rst
+++ b/spp_api_v2_change_request/README.rst
@@ -628,6 +628,24 @@ 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 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/__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..ffadca302 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 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
- 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..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 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
@@ -43,6 +43,25 @@
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.
+
+ ``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, AccessDenied)):
+ 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 +391,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 +433,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
@@ -453,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
@@ -494,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
@@ -536,7 +555,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 +595,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..0c677065b 100644
--- a/spp_api_v2_change_request/static/description/index.html
+++ b/spp_api_v2_change_request/static/description/index.html
@@ -947,13 +947,32 @@ 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 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
- 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/__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..a6fa3af2f
--- /dev/null
+++ b/spp_api_v2_change_request/tests/test_error_status_mapping.py
@@ -0,0 +1,183 @@
+# 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.
+"""
+
+import ast
+import inspect
+from unittest.mock import patch
+
+from odoo.exceptions import AccessDenied, AccessError, 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 ..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")
+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)
+
+ 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 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
+
+ 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",
+ 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)