From 03389ed13e0d1fa71a431efda7051675f87220f6 Mon Sep 17 00:00:00 2001
From: jkuensem <jkuensem@physik.uni-bielefeld.de>
Date: Fri, 15 Jan 2021 13:06:05 +0100
Subject: [PATCH] TMSS-480: More comments and debugging. Add more tests to make
sure object permission are enforced on object creation and on extra actions.
---
.../src/tmss/tmssapp/models/specification.py | 2 +-
.../tmss/tmssapp/viewsets/specification.py | 31 +++++++++++++-
SAS/TMSS/test/t_permissions.py | 28 +++++++++++++
SAS/TMSS/test/tmss_test_data_rest.py | 6 ++-
.../tmss_test_environment_unittest_setup.py | 42 +++++++++----------
5 files changed, 83 insertions(+), 26 deletions(-)
diff --git a/SAS/TMSS/src/tmss/tmssapp/models/specification.py b/SAS/TMSS/src/tmss/tmssapp/models/specification.py
index 11f95ce9340..da99762a013 100644
--- a/SAS/TMSS/src/tmss/tmssapp/models/specification.py
+++ b/SAS/TMSS/src/tmss/tmssapp/models/specification.py
@@ -417,7 +417,7 @@ class CycleQuota(Model):
class Project(NamedCommonPK):
# todo: cycles should be protected since we have to manually decide to clean up projects with a cycle or keep them without cycle, however, ManyToManyField does not allow for that
- cycles = ManyToManyField('Cycle', related_name='projects', null=True, help_text='Cycles to which this project belongs (NULLable).')
+ cycles = ManyToManyField('Cycle', related_name='projects', blank=True, help_text='Cycles to which this project belongs (NULLable).')
priority_rank = FloatField(null=False, help_text='Priority of this project w.r.t. other projects. Projects can interrupt observations of lower-priority projects.') # todo: add if needed: validators=[MinValueValidator(0.0), MaxValueValidator(1.0)]
trigger_priority = IntegerField(default=1000, help_text='Priority of this project w.r.t. triggers.') # todo: verify meaning and add to help_text: "Triggers with higher priority than this threshold can interrupt observations of projects."
auto_ingest = BooleanField(default=False, help_text='True if The data is ingested when the other scheduling unit tasks are finished. False if The data is ingested after approval in the QA validation workflow. At the end of this a flag is set that the data can be ingested.')
diff --git a/SAS/TMSS/src/tmss/tmssapp/viewsets/specification.py b/SAS/TMSS/src/tmss/tmssapp/viewsets/specification.py
index cc3c72586cb..5b2e7d6baf7 100644
--- a/SAS/TMSS/src/tmss/tmssapp/viewsets/specification.py
+++ b/SAS/TMSS/src/tmss/tmssapp/viewsets/specification.py
@@ -685,7 +685,30 @@ class TaskDraftViewSet(LOFARViewSet):
serializer_class = serializers.TaskDraftSerializer
permission_classes = (IsProjectMember,)
filter_backends = (IsProjectMemberFilterBackend,)
- filter_project_roles = ['PI']
+ # todo: both IsProjectMember and IsProjectMemberFilterBackend currently rely on the filter_project_roles getting
+ # defined to fine-tune what roles are allowed to get access and which ones do not. Essentially, while roles are
+ # provided by the user administration, the permissions that these roles get granted need to be defined within TMSS
+ # and this is currently the place to do so.
+ # This needs to be expanded in two way:
+ # 1. While it makes sense to define this on the views, it is hard to get an overview of what permissions are
+ # currently configured in order to see what role a user is required to get to perform certain actions. Also,
+ # implementing functionality to reconfigure this gets quite difficult. Eventually, we want to have a place in
+ # the database that encodes this for all views in a central place, allowing to display an overview or dynamic
+ # config from frontend.
+ # 2. We need to differentiate between actions, since some objects need to only allow 'PATCH' but not 'POST' for
+ # certain roles.
+ # For instance, we could set up table ProjectPermissions with columns 'view' or 'viewset' and one for each project
+ # role, which then hold a list for all actions that are allowed with that role.
+ # In permissions, instead of checking if a user role is in filter_project_roles, we can then look up the view by
+ # name, and check if any of the roles of the user contain the requested action.
+ # We could also swap the roles and actions, i.e. have a column for each action and list permitted roles as values,
+ # which should work nicely with the usual REST actions 'GET', 'POST', 'DELETE' actions, and we would not have to
+ # alter the db model when we introduce further project roles.
+ # To deal with actions, e.g. 'create_task_blueprint', it probably makes most sense to denote them in
+ # dot-notation alongside the regular viewset (e.g. have separate permission entries for TaskDraftViewSet and
+ # TaskDraftViewSet.create_task_blueprint) rather than defining them alongside the REST actions.
+ # (Note: when handling, these are accessible as view.action and request.method)
+ filter_project_roles = ['shared_support_user', 'friend_of_project', 'friend_of_project_primary']
# prefetch all reverse related references from other models on their related_name to avoid a ton of duplicate queries
queryset = queryset.prefetch_related('first_scheduling_relation') \
@@ -719,9 +742,13 @@ class TaskDraftViewSet(LOFARViewSet):
@swagger_auto_schema(responses={201: 'The created task blueprint, see Location in Response header',
403: 'forbidden'},
operation_description="Carve this draft task specification in stone, and make an (uneditable) blueprint out of it.")
- @action(methods=['get'], detail=True, url_name="create_task_blueprint", name="Create TaskBlueprint")
+ @action(methods=['get'], detail=True, url_name="create_task_blueprint", name="Create TaskBlueprint") # todo: I think these actions should be 'post'-only, since they alter the DB ?!
def create_task_blueprint(self, request, pk=None):
task_draft = get_object_or_404(models.TaskDraft, pk=pk)
+ # todo: it seems there is no way to pass IsProjectMember as a permission class to actions without explicitly
+ # registering them in the router for some reason. but explicitly doing a check_object_permission works as well.
+ # We may want to extend get_object_or_404 to also perform a permission check before returning an object...?
+ self.check_object_permissions(self.request, task_draft)
task_blueprint = create_task_blueprint_from_task_draft(task_draft)
# url path magic to construct the new task_blueprint_path url
diff --git a/SAS/TMSS/test/t_permissions.py b/SAS/TMSS/test/t_permissions.py
index c9e07d2ae98..001db8bce57 100755
--- a/SAS/TMSS/test/t_permissions.py
+++ b/SAS/TMSS/test/t_permissions.py
@@ -131,6 +131,34 @@ class ProjectPermissionTestCase(TestCase):
# make sure the list contains only the one more item we have permission for
GET_and_assert_in_expected_response_result_list(self, BASE_URL + '/task_draft/', taskdraft_test_data_shared_support_user, nbr_results + 1, auth=self.auth)
+ def test_task_draft_POST_raises_error_if_user_has_no_permission_for_related_project(self):
+ # try to create a task draft connected to project where we have no role and make sure that fails
+ taskdraft_test_data = self.test_data_creator.TaskDraft(scheduling_unit_draft_url=self.scheduling_unit_draft_forbidden_url, template_url=self.task_template_url)
+ POST_and_assert_expected_response(self, BASE_URL + '/task_draft/', taskdraft_test_data, 403, taskdraft_test_data, auth=self.auth)
+
+ def test_task_draft_POST_works_if_user_has_permission_for_related_project(self):
+ # create task draft connected to project where we have 'shared_support_user' role
+ taskdraft_test_data = self.test_data_creator.TaskDraft(scheduling_unit_draft_url=self.scheduling_unit_draft_shared_support_user_url, template_url=self.task_template_url)
+ POST_and_assert_expected_response(self, BASE_URL + '/task_draft/', taskdraft_test_data, 201, taskdraft_test_data)
+
+ # TaskDraft.actions
+
+ def test_task_draft_create_task_blueprint_GET_raises_error_if_user_has_no_permission_for_related_project(self):
+ # create task draft connected to project where we have no role
+ taskdraft_test_data = self.test_data_creator.TaskDraft(scheduling_unit_draft_url=self.scheduling_unit_draft_forbidden_url, template_url=self.task_template_url)
+ taskdraft_url = POST_and_assert_expected_response(self, BASE_URL + '/task_draft/', taskdraft_test_data, 201, taskdraft_test_data)['url']
+
+ # make sure we cannot create a blueprint from it
+ GET_and_assert_equal_expected_code(self, taskdraft_url + '/create_task_blueprint/', 403, auth=self.auth)
+
+ def test_task_draft_create_task_blueprint_GET_works_if_user_has_permission_for_related_project(self):
+ # create task draft connected to project where we have 'shared_support_user' role
+ taskdraft_test_data = self.test_data_creator.TaskDraft(scheduling_unit_draft_url=self.scheduling_unit_draft_shared_support_user_url, template_url=self.task_template_url)
+ taskdraft_url = POST_and_assert_expected_response(self, BASE_URL + '/task_draft/', taskdraft_test_data, 201, taskdraft_test_data)['url']
+
+ # make sure we cannot create a blueprint from it
+ GET_and_assert_equal_expected_code(self, taskdraft_url + '/create_task_blueprint/', 201, auth=self.auth)
+
# todo: add tests for other models with project permissions
if __name__ == "__main__":
diff --git a/SAS/TMSS/test/tmss_test_data_rest.py b/SAS/TMSS/test/tmss_test_data_rest.py
index 76b237962e4..122bcbde0a0 100644
--- a/SAS/TMSS/test/tmss_test_data_rest.py
+++ b/SAS/TMSS/test/tmss_test_data_rest.py
@@ -200,9 +200,11 @@ class TMSSRESTTestDataCreator():
"projects": [],
"quota": []}
- def Project(self, description="my project description", name=None):
+ def Project(self, description="my project description", name=None, cycles=None):
if name is None:
name = 'my_project_' + str(uuid.uuid4())
+ if cycles is None:
+ cycles = [self.post_data_and_get_url(self.Cycle(), '/cycle')]
return {"name": name,
"description": description,
"tags": [],
@@ -211,7 +213,7 @@ class TMSSRESTTestDataCreator():
"trigger_priority": 1000,
"can_trigger": False,
"private_data": True,
- "cycles": [self.post_data_and_get_url(self.Cycle(), '/cycle')],
+ "cycles": cycles,
"archive_subdirectory": 'my_project/'}
def ResourceType(self, description="my resource_type description"):
diff --git a/SAS/TMSS/test/tmss_test_environment_unittest_setup.py b/SAS/TMSS/test/tmss_test_environment_unittest_setup.py
index 45d148eb375..8436a3450d8 100644
--- a/SAS/TMSS/test/tmss_test_environment_unittest_setup.py
+++ b/SAS/TMSS/test/tmss_test_environment_unittest_setup.py
@@ -55,21 +55,21 @@ from lofar.sas.tmss.test.test_utils import assertDataWithUrls
import lofar.sas.tmss.tmss.settings as TMSS_SETTINGS
-def _call_API_and_assert_expected_response(test_instance, url, call, data, expected_code, expected_content):
+def _call_API_and_assert_expected_response(test_instance, url, call, data, expected_code, expected_content, auth=AUTH):
"""
Call API method on the provided url and assert the expected code is returned and the expected content is in the response content
:return: response as dict. This either contains the data of an entry or error details. If JSON cannot be parsed, return string.
"""
if call == 'PUT':
- response = requests.put(url, json=data, auth=AUTH)
+ response = requests.put(url, json=data, auth=auth)
elif call == 'POST':
- response = requests.post(url, json=data, auth=AUTH)
+ response = requests.post(url, json=data, auth=auth)
elif call == 'GET':
- response = requests.get(url, auth=AUTH)
+ response = requests.get(url, auth=auth)
elif call == 'PATCH':
- response = requests.patch(url, json=data, auth=AUTH)
+ response = requests.patch(url, json=data, auth=auth)
elif call == 'DELETE':
- response = requests.delete(url, auth=AUTH)
+ response = requests.delete(url, auth=auth)
else:
raise ValueError("The provided call '%s' is not a valid API method choice" % call)
@@ -110,37 +110,37 @@ def _call_API_and_assert_expected_response(test_instance, url, call, data, expec
return content
-def PUT_and_assert_expected_response(test_instance, url, data, expected_code, expected_content):
+def PUT_and_assert_expected_response(test_instance, url, data, expected_code, expected_content, auth=AUTH):
"""
PUT data on url and assert the expected code is returned and the expected content is in the response content
"""
- r_dict = _call_API_and_assert_expected_response(test_instance, url, 'PUT', data, expected_code, expected_content)
+ r_dict = _call_API_and_assert_expected_response(test_instance, url, 'PUT', data, expected_code, expected_content, auth=auth)
return r_dict
-def POST_and_assert_expected_response(test_instance, url, data, expected_code, expected_content):
+def POST_and_assert_expected_response(test_instance, url, data, expected_code, expected_content, auth=AUTH):
"""
POST data on url and assert the expected code is returned and the expected content is in the response content
:return: response dict
"""
- r_dict = _call_API_and_assert_expected_response(test_instance, url, 'POST', data, expected_code, expected_content)
+ r_dict = _call_API_and_assert_expected_response(test_instance, url, 'POST', data, expected_code, expected_content, auth=auth)
return r_dict
-def GET_and_assert_equal_expected_code(test_instance, url, expected_code):
+def GET_and_assert_equal_expected_code(test_instance, url, expected_code, auth=AUTH):
"""
GET from url and assert the expected code is returned and the expected content is in the response content
"""
- r_dict = _call_API_and_assert_expected_response(test_instance, url, 'GET', {}, expected_code, None)
+ r_dict = _call_API_and_assert_expected_response(test_instance, url, 'GET', {}, expected_code, None, auth=auth)
return r_dict
-def GET_and_assert_in_expected_response_result_list(test_instance, url, expected_content, expected_nbr_results, expected_id=None):
+def GET_and_assert_in_expected_response_result_list(test_instance, url, expected_content, expected_nbr_results, expected_id=None, auth=AUTH):
"""
GET from url and assert the expected code is returned and the expected content is in the response content
Use this check when multiple results (list) are returned
"""
- r_dict = _call_API_and_assert_expected_response(test_instance, url, 'GET', {}, 200, None)
+ r_dict = _call_API_and_assert_expected_response(test_instance, url, 'GET', {}, 200, None, auth=auth)
page_size = TMSS_SETTINGS.REST_FRAMEWORK.get('PAGE_SIZE')
if page_size is not None and expected_nbr_results > page_size:
logger.warning("Limited result length due to pagination setting (%d)", page_size)
@@ -174,36 +174,36 @@ def GET_and_assert_in_expected_response_result_list(test_instance, url, expected
return r_dict
-def GET_OK_and_assert_equal_expected_response(test_instance, url, expected_content):
+def GET_OK_and_assert_equal_expected_response(test_instance, url, expected_content, auth=AUTH):
"""
GET from url and assert the expected code is returned and the expected content is equal the response content
assertDataWithUrls is already checked in _call_API_and_assert_expected_response
"""
- r_dict = _call_API_and_assert_expected_response(test_instance, url, 'GET', {}, 200, expected_content)
+ r_dict = _call_API_and_assert_expected_response(test_instance, url, 'GET', {}, 200, expected_content, auth=auth)
# assertDataWithUrls(test_instance, r_dict, expected_content)
return r_dict
-def PATCH_and_assert_expected_response(test_instance, url, data, expected_code, expected_content):
+def PATCH_and_assert_expected_response(test_instance, url, data, expected_code, expected_content, auth=AUTH):
"""
POST data on url and assert the provided values have changed based on the server response.
:return: url for new item
"""
- r_dict = _call_API_and_assert_expected_response(test_instance, url, 'PATCH', data, expected_code, expected_content)
+ r_dict = _call_API_and_assert_expected_response(test_instance, url, 'PATCH', data, expected_code, expected_content, auth=auth)
return r_dict
-def DELETE_and_assert_gone(test_instance, url):
+def DELETE_and_assert_gone(test_instance, url, auth=AUTH):
"""
DELETE item at provided url and assert that the request was accepted by the server
:return: url for new item
"""
- response = requests.delete(url, auth=AUTH)
+ response = requests.delete(url, auth=auth)
if response.status_code != 204:
logger.error("!!! Unexpected: [%s] - %s %s: %s", test_instance.id(), 'DELETE', url, response.content)
test_instance.assertEqual(response.status_code, 204)
- response = requests.get(url, auth=AUTH)
+ response = requests.get(url, auth=auth)
if response.status_code != 404:
logger.error("!!! Unexpected: [%s] - %s %s: %s", test_instance.id(), 'GET', url, response.content)
test_instance.assertEqual(response.status_code, 404)
--
GitLab