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

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
 ~~~~~~~~~~
 

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",

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)

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
 

spp_api_v2_change_request/static/description/index.html

@@ -947,13 +947,32 @@ <h2>Changelog</h2>
 </div>
 </div>
 <div class="section" id="section-1">
+<h1>19.0.2.0.2</h1>
+<ul class="simple">
+<li>fix(api): an authorization failure on a change-request state
+transition now returns <tt class="docutils literal">403 Forbidden</tt> instead of <tt class="docutils literal">409 Conflict</tt>.
+<tt class="docutils literal">AccessError</tt> subclasses <tt class="docutils literal">UserError</tt> in Odoo, so all six
+state-transition endpoints — <tt class="docutils literal">$submit</tt> / <tt class="docutils literal">$approve</tt> / <tt class="docutils literal">$reject</tt>
+/ <tt class="docutils literal"><span class="pre">$request-revision</span></tt> / <tt class="docutils literal">$apply</tt> / <tt class="docutils literal">$reset</tt> — which caught
+<tt class="docutils literal">UserError</tt> 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 <tt class="docutils literal">$apply</tt> 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. <tt class="docutils literal">AccessDenied</tt> maps
+to 403 the same way, matching the platform’s global FastAPI error
+handler.</li>
+</ul>
+</div>
+<div class="section" id="section-2">
 <h1>19.0.2.0.1</h1>
 <ul class="simple">
 <li>fix: skip field types before getattr and isolate detail prefetch
 (#129)</li>
 </ul>
 </div>
-<div class="section" id="section-2">
+<div class="section" id="section-3">
 <h1>19.0.2.0.0</h1>
 <ul class="simple">
 <li>Initial migration to OpenSPP2</li>

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

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)