improve MagpieAdapter error report when missing OWS 'request' paramet…

…er + fix inconsistency in parsed/sent params when service caching is enabled

CHANGES.rst

@@ -7,7 +7,22 @@ Changes
 `Unreleased <https://github.com/Ouranosinc/Magpie/tree/master>`_ (latest)
 ------------------------------------------------------------------------------------
 
-* Nothing new for the moment.
+Features / Changes
+~~~~~~~~~~~~~~~~~~~~~
+* Improve error reporting of ``MagpieAdapter`` when validating the *requested* ``Permission``. If the `Service`
+  implementation raises an ``HTTP Bad Request [400]`` due to insufficient, invalid or missing parameters from
+  the request to properly resolve the corresponding `Magpie` ``Permission``, more details about the cause will
+  be reported in the `Twitcher` response body. Also, code ``400`` is returned instead of ``500``.
+
+Bug Fixes
+~~~~~~~~~~~~~~~~~~~~~
+* Fix an issue in ``MagpieAdapter`` when `Service` caching is enabled (in `Twitcher` INI configuration) that caused
+  implementations derived from ``ServiceOWS`` (WPS, WMS, WFS) to incorrectly retrieve and parse the cached request
+  parameters instead of the new ones from the incoming request.
+  Because ``ServiceOWS`` implementations employ request parameter ``request`` (in query or body based on HTTP method)
+  to infer their corresponding `Magpie` ``Permission`` (e.g.: ``GetCapabilities``, ``GetMap``, etc.), this produced
+  potential inconsistencies between the *requested* ``Permission`` that `Twitcher` was evaluating with `Magpie`, and
+  the *actual request* sent to the `Service` behind the proxy.
 
 `3.13.0 <https://github.com/Ouranosinc/Magpie/tree/3.13.0>`_ (2021-06-29)
 ------------------------------------------------------------------------------------

magpie/adapter/magpieowssecurity.py

@@ -5,9 +5,10 @@
 from beaker.cache import cache_region, cache_regions
 from pyramid.authentication import IAuthenticationPolicy
 from pyramid.authorization import IAuthorizationPolicy
-from pyramid.httpexceptions import HTTPForbidden, HTTPNotFound, HTTPOk, HTTPUnauthorized
+from pyramid.httpexceptions import HTTPBadRequest, HTTPForbidden, HTTPNotFound, HTTPOk, HTTPUnauthorized
 from pyramid.settings import asbool
 from requests.cookies import RequestsCookieJar
+from simplejson import JSONDecodeError
 from six.moves.urllib.parse import urlparse
 
 from magpie.api.exception import evaluate_call, verify_param
@@ -22,7 +23,7 @@
 #   Twitcher available only when this module is imported from it.
 #   It is installed during tests for evaluation.
 #   Module 'magpie.adapter' should not be imported from 'magpie' package.
-from twitcher.owsexceptions import OWSAccessForbidden  # noqa
+from twitcher.owsexceptions import OWSAccessForbidden, OWSInvalidParameterValue, OWSMissingParameterValue  # noqa
 from twitcher.owssecurity import OWSSecurityInterface  # noqa
 from twitcher.utils import parse_service_name  # noqa
 
@@ -115,18 +116,43 @@ def check_request(self, request):
 
         In the case `Twitcher` proxy path is matched, the :term:`Logged User` **MUST** be allowed access following
         :term:`Effective Permissions` resolution via :term:`ACL`. Otherwise, :exception:`OWSForbidden` is raised.
-        Failing to parse the request or any underlying component also raises that exception.
+
+        Failing to parse the request or any underlying component that raises an exception will be left up to the
+        parent caller to handle the exception. In most typical use case, this means `Twitcher` will raise a
+        generic :exception:`OWSException` with ``NoApplicableCode``, unless the exception was .
 
         :raises OWSForbidden: if user does not have access to the targeted resource under the service.
         :returns: nothing if user has access.
         """
         if request.path.startswith(self.twitcher_protected_path):
+            # each service implementation defines their ACL and permission resolution using request definition
             service_impl = self.get_service(request)
-            # should contain all the acl, this the only thing important
-            # parse request (GET/POST) to get the permission requested for that service
-            permission_requested = service_impl.permission_requested()
-            # convert permission enum to str for comparison
-            permission_requested = Permission.get(permission_requested).value if permission_requested else None
+
+            perm_exc = None
+            try:
+                # parse request (GET/POST) to get the permission requested for that service
+                permission_requested = service_impl.permission_requested()
+                # convert permission enum to str for comparison
+                permission_requested = Permission.get(permission_requested).value if permission_requested else None
+            except HTTPBadRequest as exc:
+                perm_exc = exc
+                # if special case of HTTPBadRequest was raised, attempt providing a better description of the error
+                # otherwise, Twitcher will capture other exceptions and re-raise them as generic OWSException
+                try:
+                    data = getattr(exc, "json", {})
+                except JSONDecodeError:
+                    data = {}
+                detail = data.pop("detail", None) or str(exc)
+                ows_err = OWSMissingParameterValue if data.get("value", None) is None else OWSInvalidParameterValue
+                locator = data.get("param", {}).get("name", None)
+                raise ows_err(detail, value=locator)
+            except Exception as exc:
+                perm_exc = exc
+                raise  # re-raise and let Twitcher handle it, only do this to obtain 'exc' for logging
+            finally:
+                if perm_exc is not None:
+                    LOGGER.debug("Error during service [%s] permission requested resolution [%s](%s)",
+                                 service_impl.service.resource_name, type(perm_exc).__name__, perm_exc)
 
             if permission_requested:
                 LOGGER.info("'%s' request '%s' permission on '%s'", request.user, permission_requested, request.path)

magpie/services.py

@@ -89,6 +89,11 @@ def permission_requested(self):
         If ``None`` is returned, the :term:`ACL` will effectively be resolved to denied access.
         Otherwise, one or more returned :class:`Permission` will indicate which permissions should be looked for to
         resolve the :term:`ACL` of the authenticated user and its groups.
+
+        If the request cannot be parsed for any reason to retrieve needed parameters (e.g.: Bad Request),
+        the :exception:`HTTPBadRequest` can be raised to indicate specifically the cause, which will
+        help :class:`magpie.adapter.magpieowssecurity.MagpieOWSSecurity` create a better response with
+        the relevant error details.
         """
         raise NotImplementedError("missing implementation of request permission converter")
 
@@ -412,23 +417,38 @@ class ServiceOWS(ServiceInterface):
 
     def __init__(self, service, request):
         # type: (models.Service, Request) -> None
-        super(ServiceOWS, self).__init__(service, request)
+        super(ServiceOWS, self).__init__(service, request)  # sets request, which in turn parses it with below setter
+
+    def _get_request(self):
+        # type: () -> Request
+        return self._request
+
+    def _set_request(self, request):
+        # type: (Request) -> None
+        self._request = request
+        # must reset the parser from scratch if request changes to ensure everything is updated with new inputs
         self.parser = ows_parser_factory(request)
         self.parser.parse(self.params_expected)  # run parsing to obtain guaranteed lowered-name parameters
 
+    request = property(_get_request, _set_request)
+
     @abc.abstractmethod
     def resource_requested(self):
         raise NotImplementedError
 
     def permission_requested(self):
         # type: () -> Permission
         try:
-            req = str(self.parser.params["request"]).lower()
-            perm = Permission.get(req)
-            if perm is None:
-                raise NotImplementedError(
-                    "Missing or unknown 'Permission' from OWS 'request' parameter: {!s}".format(req)
+            req = self.parser.params["request"]
+            perm = Permission.get(str(req).lower())
+            ax.verify_param(
+                perm, not_none=True, param_name="request", http_error=HTTPBadRequest,
+                content={"service": self.service.resource_name, "type": self.service_type, "value": req},
+                msg_on_fail=(
+                    "Missing or unknown 'Permission' inferred from OWS 'request' parameter: [{!s}]. ".format(req) +
+                    "Unable to resolve the requested access for service: [{!s}].".format(self.service.resource_name)
                 )
+            )
             return perm
         except KeyError as exc:
             raise NotImplementedError("Exception: [{!r}] for class '{}'.".format(exc, type(self)))