diff --git a/CHANGES.rst b/CHANGES.rst index 2241c2b87..38e45498e 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -13,6 +13,10 @@ Features / Changes specified configuration of the specific service (resolves `#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 ~~~~~~~~~~~~~~~~~~~~~ diff --git a/docs/services.rst b/docs/services.rst index 072e300e3..c68f03a8a 100644 --- a/docs/services.rst +++ b/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 ```` 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 ```` 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 ````, 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 ```` 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 ````, 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 ````, 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 diff --git a/magpie/adapter/magpieowssecurity.py b/magpie/adapter/magpieowssecurity.py index 81d26622c..1eb8ce2f2 100644 --- a/magpie/adapter/magpieowssecurity.py +++ b/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): """ diff --git a/magpie/services.py b/magpie/services.py index ed8072464..d6c78f6a3 100644 --- a/magpie/services.py +++ b/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") @@ -687,7 +687,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"] @@ -717,7 +717,7 @@ 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) @@ -725,7 +725,7 @@ def resource_requested(self): 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): diff --git a/tests/test_services.py b/tests/test_services.py index b94ca9d69..2fe867f81 100644 --- a/tests/test_services.py +++ b/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,11 +414,10 @@ 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: @@ -419,6 +425,19 @@ def test_ServiceTHREDDS_effective_permissions(self): 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