Merge pull request #458 from CSCfi/CSCMETAX-599-allower-projects-fix

CSCMETAX-599: [FIX|REF] allowed_projects: Further improve handling. R…

src/metax_api/api/rest/base/views/file_view.py

@@ -97,14 +97,13 @@ def update_bulk(self, request, *args, **kwargs):
         Checks that all files belongs to project in allowed_projects query parameter
         if given.
         """
-        allowed_projects = request.query_params.get('allowed_projects', None)
+        allowed_projects = CommonService.get_list_query_param(request, 'allowed_projects')
 
         if allowed_projects is not None:
             if not isinstance(request.data, list):
                 return Response(data={ 'detail': 'request.data is not a list'}, status=status.HTTP_400_BAD_REQUEST)
 
             file_ids = [f['identifier'] for f in request.data]
-            allowed_projects = set( project.strip() for project in allowed_projects.split(',') )
 
             if not FileService.verify_allowed_projects(allowed_projects, file_identifiers=file_ids):
                 return Response(data={"detail": "You do not have permission to update these files"},
@@ -117,7 +116,7 @@ def partial_update_bulk(self, request, *args, **kwargs):
         Checks that all files belongs to project in allowed_projects query parameter
         if given.
         """
-        allowed_projects = request.query_params.get('allowed_projects', None)
+        allowed_projects = CommonService.get_list_query_param(request, 'allowed_projects')
 
         if allowed_projects is not None:
             if not isinstance(request.data, list):
@@ -128,7 +127,6 @@ def partial_update_bulk(self, request, *args, **kwargs):
                 return Response(data={"detail": "File identifier is missing"},
                                 status=status.HTTP_400_BAD_REQUEST)
 
-            allowed_projects = set( project.strip() for project in allowed_projects.split(',') )
             if not FileService.verify_allowed_projects(allowed_projects, file_identifiers=file_ids):
                 return Response(data={"detail": "You do not have permission to update these files"},
                                 status=status.HTTP_403_FORBIDDEN)

src/metax_api/models/catalog_record.py

@@ -1335,9 +1335,11 @@ def _check_changed_files_permissions(self, file_changes):
         projects_removed = file_changes['changed_projects'].get('files_removed', set())
         project_changes = projects_added.union(projects_removed)
 
-        allowed_projects = self.request.query_params.get('allowed_projects', None)
+        from metax_api.services import CommonService
+        allowed_projects = CommonService.get_list_query_param(self.request, 'allowed_projects')
 
         if allowed_projects is not None:
+
             if not all(p in allowed_projects for p in project_changes):
                 raise Http403({'detail': [
                     'Unable to update dataset %s. You do not have permissions to all of the files and directories.'

src/metax_api/services/common_service.py

@@ -70,6 +70,23 @@ def get_boolean_query_param(request, param_name):
                 'boolean value must be true or false. received value was %s' % value
             ]})
 
+    @staticmethod
+    def get_list_query_param(request, param_name):
+        """
+        Retrieve an optional comma-separated list of string values from a query param.
+
+        Note: Returns set, not a list!
+
+        Note: In case method is needed before dispatch() is called, see what is done in
+        method get_boolean_query_param().
+        """
+        value = request.query_params.get(param_name, None)
+        if value is not None:
+            values_list = set( v.strip() for v in value.split(',') )
+            if values_list:
+                return values_list
+        return None
+
     @classmethod
     def create_bulk(cls, request, serializer_class, **kwargs):
         """

src/metax_api/tests/api/rest/base/views/datasets/write.py

@@ -261,13 +261,23 @@ def test_create_catalog_record_allowed_projects_ok(self):
         self.assertEqual(response.status_code, status.HTTP_201_CREATED, response.data)
 
     def test_create_catalog_record_allowed_projects_fail(self):
+        # dataset file not in allowed projects
         response = self.client.post('/rest/datasets?allowed_projects=no,permission', self.cr_test_data, format="json")
         self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN, response.data)
 
-    def test_create_catalog_record_allowed_projects_empty_value(self):
+        # ensure list is properly handled (separated by comma, end result should be list)
+        response = self.client.post('/rest/datasets?allowed_projects=no_good_project_x,another',
+            self.cr_test_data, format="json")
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN, response.data)
+
+        # handle empty value
         response = self.client.post('/rest/datasets?allowed_projects=', self.cr_test_data, format="json")
         self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN, response.data)
 
+        # Other trickery
+        response = self.client.post('/rest/datasets?allowed_projects=,', self.cr_test_data, format="json")
+        self.assertEqual(response.status_code, status.HTTP_403_FORBIDDEN, response.data)
+
     #
     # create list operations
     #