BROWSE permission working for ServiceTHREDDS (fix #361) [with tests] …

…+ adjust MagpieAdapter to DENY by default

CHANGES.rst

@@ -13,6 +13,10 @@ Features / Changes
   specified configuration of the specific service (resolves `#361 <https://github.com/Ouranosinc/Magpie/issues/361>`_).
 * Add documentation details about parsing methodologies, specific custom configurations and respective usage of the
   various ``Service`` types provided by `Magpie`.
+* Adjust ``MagpieAdapter`` such that ``OWSAccessForbidden`` is raised by default if the ``Service`` implementation fails
+  to provide a valid ``Permission`` enum from ``permission_requested`` method. Incorrectly defined ``Service`` will
+  therefore not unexpectedly grant access to protected resources. Behaviour also aligns with default ``DENY`` access
+  obtained when resolving effective permissions through `Magpie` API routes.
 
 Bug Fixes
 ~~~~~~~~~~~~~~~~~~~~~

docs/services.rst

@@ -124,6 +124,7 @@ by the below configuration.
           prefixes:
             - fileServer
             - dodsC
+            - dap4
             - wcs
             - wms
 
@@ -139,11 +140,11 @@ An incoming request will be parsed according to configured values against the fo
 
 The above template demonstrates that `Magpie` will attempt to match the ``<prefix>`` part with any of the listed
 ``prefixes`` in the configuration. If a match is found, the corresponding *metadata* or *data* content will be assumed,
-according to where the match entry was located, to determine whether :attr:`Permission.BROWSE` or
-:attr:`Permission.READ` should be respectively assumed for this request access. If no ``<prefix>`` can be resolved, the
-permission will be immediately assumed as :attr:`Access.DENY`. To allow top-level access directly on the
-:term:`Service`'s root without ``<prefix>``, it is important to provide ``null`` within the desired ``prefixes`` list.
-Duplicates between the two lists of ``prefixes`` will favor entries in ``metadata_type`` over ``data_type``.
+according to where the match entry was located, to determine whether the requested :term:`Resource` should be validated
+respectively for :attr:`Permission.BROWSE` or :attr:`Permission.READ` access. If no ``<prefix>`` can be resolved, the
+permission will be immediately assumed as :attr:`Access.DENY` regardless of type. To allow top-level access directly on
+the :term:`Service`'s root without ``<prefix>``, it is important to provide ``null`` within the desired ``prefixes``
+list. Duplicates between the two lists of ``prefixes`` will favor entries in ``metadata_type`` over ``data_type``.
 
 After resolution of the content type from ``<prefix>``, the resolution of any amount of :class:`magpie.models.Directory`
 :term:`Resource` will be attempted. Any missing children directory :term:`Resource` will terminate the lookup process

magpie/adapter/magpieowssecurity.py

@@ -64,9 +64,11 @@ def check_request(self, request):
                     for attr_name in base_attr:
                         LOGGER.debug("  %s: %s", attr_name, getattr(authn_policy.cookie, attr_name))
 
-                if not has_permission:
-                    raise OWSAccessForbidden("Not authorized to access this resource. "
-                                             "User does not meet required permissions.")
+                if has_permission:
+                    return  # allowed
+
+            raise OWSAccessForbidden("Not authorized to access this resource. "
+                                     "User does not meet required permissions.")
 
     def update_request_cookies(self, request):
         """

magpie/services.py

@@ -78,9 +78,9 @@ def permission_requested(self):
         The method must specifically define how to convert generic request path, query, etc. elements into permissions
         that match the service and its children resources.
 
-        If ``None`` is returned, the ACL will effectively be resolved to denied access.
+        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 ACL of the authenticated user and its groups.
+        resolve the :term:`ACL` of the authenticated user and its groups.
         """
         raise NotImplementedError("missing implementation of request permission converter")
 
@@ -689,7 +689,7 @@ def get_config(self):
         cfg.setdefault("file_patterns", [r".*.nc"])
         cfg.setdefault("data_type", {"prefixes": []})
         if not cfg["data_type"]["prefixes"]:
-            cfg["data_type"]["prefixes"] = ["fileServer", "dodsC", "wcs", "wms"]
+            cfg["data_type"]["prefixes"] = ["fileServer", "dodsC", "dap4", "wcs", "wms"]
         cfg.setdefault("metadata_type", {"prefixes": []})
         if not cfg["metadata_type"]["prefixes"]:
             cfg["metadata_type"]["prefixes"] = [None, "catalog", "ncml", "uddc", "iso"]
@@ -719,15 +719,15 @@ def resource_requested(self):
                     try:
                         part_name = re.match(pattern, part_name)[0]
                         break
-                    except (TypeError, KeyError):  # fail match or fail to extract
+                    except (TypeError, KeyError):  # fail match or fail to extract (depending on configured pattern)
                         pass
             child_res_id = child_resource.resource_id
             child_resource = models.find_children_by_name(part_name, parent_id=child_res_id, db_session=self.request.db)
             if child_resource:
                 found_resource = child_resource
 
         # target resource reached if no more parts to process, otherwise we have some parent (minimally the service)
-        target = not len(path_parts)
+        target = not len(path_parts) and child_resource is not None
         return found_resource, target
 
     def permission_requested(self):

tests/test_services.py

@@ -299,8 +299,8 @@ def test_ServiceTHREDDS_effective_permissions(self):
 
         Legend::
 
-            b: browse   (metadata access of resource)
-            r: read     (interpreted as literal filesystem READ of data)
+            b: browse   (access of resource *metadata*, both for File information and Directory listing)
+            r: read     (interpreted as literal file system READ of *data*, effective resolution valid only on File)
             w: write    (unused by ACL resolution)
             A: allow
             D: deny
@@ -360,6 +360,9 @@ def test_ServiceTHREDDS_effective_permissions(self):
             utils.check_response_basic_info(resp, 403, expected_method="POST")
 
         # assign permissions
+        bAR = PermissionSet(Permission.BROWSE, Access.ALLOW, Scope.RECURSIVE)   # noqa
+        bDR = PermissionSet(Permission.BROWSE, Access.DENY, Scope.RECURSIVE)    # noqa
+        bDM = PermissionSet(Permission.BROWSE, Access.DENY, Scope.MATCH)        # noqa
         rAR = PermissionSet(Permission.READ, Access.ALLOW, Scope.RECURSIVE)     # noqa
         rDR = PermissionSet(Permission.READ, Access.DENY, Scope.RECURSIVE)      # noqa
         rDM = PermissionSet(Permission.READ, Access.DENY, Scope.MATCH)          # noqa
@@ -368,11 +371,15 @@ def test_ServiceTHREDDS_effective_permissions(self):
         wDM = PermissionSet(Permission.WRITE, Access.DENY, Scope.MATCH)         # noqa
         utils.TestSetup.create_TestUserResourcePermission(self, override_resource_id=svc_id, override_permission=wDM)
         utils.TestSetup.create_TestGroupResourcePermission(self, override_resource_id=svc_id, override_permission=wAR)
+        utils.TestSetup.create_TestUserResourcePermission(self, override_resource_id=dir1_id, override_permission=bAR)
         utils.TestSetup.create_TestUserResourcePermission(self, override_resource_id=dir1_id, override_permission=rAR)
+        utils.TestSetup.create_TestGroupResourcePermission(self, override_resource_id=dir2_id, override_permission=bDR)
         utils.TestSetup.create_TestGroupResourcePermission(self, override_resource_id=dir2_id, override_permission=rDR)
         utils.TestSetup.create_TestUserResourcePermission(self, override_resource_id=dir3_id, override_permission=wAR)
         utils.TestSetup.create_TestGroupResourcePermission(self, override_resource_id=dir3_id, override_permission=wDM)
         utils.TestSetup.create_TestGroupResourcePermission(self, override_resource_id=file1_id, override_permission=wDM)
+        utils.TestSetup.create_TestGroupResourcePermission(self, override_resource_id=dir4_id, override_permission=bAR)
+        utils.TestSetup.create_TestUserResourcePermission(self, override_resource_id=file2_id, override_permission=bDM)
         utils.TestSetup.create_TestGroupResourcePermission(self, override_resource_id=dir4_id, override_permission=rAR)
         utils.TestSetup.create_TestUserResourcePermission(self, override_resource_id=file2_id, override_permission=rDM)
         utils.TestSetup.create_TestUserResourcePermission(self, override_resource_id=file2_id, override_permission=wDR)
@@ -382,7 +389,7 @@ def test_ServiceTHREDDS_effective_permissions(self):
 
         # directory path with various path formats, only actual directory listing should be allowed
         dir_prefixes = [svc_name + "/catalog", svc_name + "/fileServer", svc_name + "/dodsC"]
-        dir_suffixes = ["", "/catalog.html"]
+        dir_suffixes = ["", "/catalog.html"]  # with or without explicit catalog HTML, this points to directory listing
         test_sub_dir = [
             (False, ""),
             (True, "/" + dir1_name),
@@ -407,18 +414,30 @@ def test_ServiceTHREDDS_effective_permissions(self):
             (True, "{}/{}/{}".format(dir4_name, dir5_name, file3_name)),
         ]
         for expect_allowed, file_path in test_files:
-            file_prefixes = ["dap4", "dodsC", "fileServer"]  # format of files accessors (anything else than 'catalog')
-            file_suffixes = [file_path, "{}.dds".format(file_path), "{}.dmr.xml".format(file_path),
-                             "{}.html".format(file_path), "{}.ascii?".format(file_path)]  # different representations
+            file_prefixes = ["dap4", "dodsC", "fileServer"]  # format of files accessors (anything in *data_type*)
+            file_suffixes = ["", ".dds", ".dmr.xml", ".html", ".ascii?"]  # different representations
             for prefix, suffix in itertools.product(file_prefixes, file_suffixes):
-                path = "/ows/proxy/{}/{}/{}".format(svc_name, prefix, suffix)
+                path = "/ows/proxy/{}/{}/{}{}".format(svc_name, prefix, file_path, suffix)
                 req = self.mock_request(path, method="GET")
                 msg = "Using combination [GET, {}]".format(path)
                 if expect_allowed:
                     utils.check_no_raise(lambda: self.ows.check_request(req), msg=msg)
                 else:
                     utils.check_raises(lambda: self.ows.check_request(req), OWSAccessForbidden, msg=msg)
 
+        # validate that unknown prefix is always denied even if resource is otherwise allowed when prefix is known
+        # this is mostly to ensure that new prefix/formats added to THREDDS don't suddenly provide access unexpectedly
+        test_allowed_resources = [
+            "/" + dir1_name,
+            "{}/{}/{}".format(dir4_name, dir5_name, file3_name),
+        ]
+        unknown_prefix = "random"
+        for allowed_resource in test_allowed_resources:
+            path = "/ows/proxy/{}/{}/{}".format(svc_name, unknown_prefix, allowed_resource)
+            req = self.mock_request(path, method="GET")
+            msg = "Unknown prefix must be refused even when resource is normally allowed. Using [GET, {}]".format(path)
+            utils.check_raises(lambda: self.ows.check_request(req), OWSAccessForbidden, msg=msg)
+
     @unittest.skip("impl")
     @pytest.mark.skip
     @utils.mock_get_settings