From 912da2f3b93e7ae655725b6619febf0dbbcc4455 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Fri, 18 Aug 2017 19:48:29 +0200 Subject: [PATCH 1/7] Fix checking WorkflowInvocation for published workflows --- lib/galaxy/managers/workflows.py | 11 +++++-- test/api/test_workflows.py | 53 +++++++++++++++++++++++++++++--- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index ed0e64b53b7..2f3d72a64ae 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -66,7 +66,7 @@ class WorkflowsManager(object): stored_workflow = self.get_stored_workflow(trans, workflow_id) # check to see if user has permissions to selected workflow - if stored_workflow.user != trans.user and not trans.user_is_admin(): + if stored_workflow.user != trans.user and not trans.user_is_admin() and not stored_workflow.published: if trans.sa_session.query(trans.app.model.StoredWorkflowUserShareAssociation).filter_by(user=trans.user, stored_workflow=stored_workflow).count() == 0: message = "Workflow is not owned by or shared with current user" raise exceptions.ItemAccessibilityException(message) @@ -90,9 +90,14 @@ class WorkflowsManager(object): if not check_ownership or check_accessible: return True - # If given an invocation follow to workflow... + # If given an invocation verify ownership of invocation if isinstance(has_workflow, model.WorkflowInvocation): - has_workflow = has_workflow.workflow + # We use the the owner of the history that is associated to the invocation as a proxy + # for the owner of the invocation. + if trans.user != has_workflow.history.user: + raise exceptions.ItemOwnershipException() + else: + return True # stored workflow contains security stuff - follow that workflow to # that unless given a stored workflow. diff --git a/test/api/test_workflows.py b/test/api/test_workflows.py index 9eae2da2377..58f282b6e6f 100644 --- a/test/api/test_workflows.py +++ b/test/api/test_workflows.py @@ -92,22 +92,23 @@ class BaseWorkflowsApiTestCase(api.ApiTestCase): def _upload_yaml_workflow(self, has_yaml, **kwds): return self.workflow_populator.upload_yaml_workflow(has_yaml, **kwds) - def _setup_workflow_run(self, workflow, inputs_by='step_id', history_id=None): - uploaded_workflow_id = self.workflow_populator.create_workflow(workflow) + def _setup_workflow_run(self, workflow=None, inputs_by='step_id', history_id=None, workflow_id=None): + if not workflow_id: + workflow_id = self.workflow_populator.create_workflow( workflow ) if not history_id: history_id = self.dataset_populator.new_history() hda1 = self.dataset_populator.new_dataset(history_id, content="1 2 3") hda2 = self.dataset_populator.new_dataset(history_id, content="4 5 6") workflow_request = dict( history="hist_id=%s" % history_id, - workflow_id=uploaded_workflow_id, + workflow_id=workflow_id, ) label_map = { 'WorkflowInput1': self._ds_entry(hda1), 'WorkflowInput2': self._ds_entry(hda2) } if inputs_by == 'step_id': - ds_map = self._build_ds_map(uploaded_workflow_id, label_map) + ds_map = self._build_ds_map(workflow_id, label_map) workflow_request["ds_map"] = ds_map elif inputs_by == "step_index": index_map = { @@ -1816,6 +1817,50 @@ steps: self._assert_status_code_is(step_response, 200) self._assert_has_keys(step_response.json(), "id", "order_index") + @skip_without_tool("cat1") + def test_invocations_accessible_imported_workflow(self): + workflow_id = self.workflow_populator.simple_workflow("test_usage", publish=True) + with self._different_user(): + other_import_response = self.__import_workflow(workflow_id) + self._assert_status_code_is(other_import_response, 200) + other_id = other_import_response.json()["id"] + workflow_request, history_id = self._setup_workflow_run(workflow_id=other_id) + response = self._get("workflows/%s/usage" % other_id) + self._assert_status_code_is(response, 200) + assert len(response.json()) == 0 + run_workflow_response = self._post("workflows", data=workflow_request) + self._assert_status_code_is(run_workflow_response, 200) + usage_details_response = self._get("workflows/%s/usage/%s" % (other_id, history_id)) + self._assert_status_code_is(usage_details_response, 200) + + @skip_without_tool("cat1") + def test_invocations_accessible_published_workflow(self): + workflow_id = self.workflow_populator.simple_workflow("test_usage", publish=True) + with self._different_user(): + workflow_request, history_id = self._setup_workflow_run(workflow_id=workflow_id) + workflow_request['workflow_id'] = workflow_request.pop('workflow_id') + response = self._get("workflows/%s/usage" % workflow_id) + self._assert_status_code_is(response, 200) + assert len(response.json()) == 0 + run_workflow_response = self._post("workflows", data=workflow_request) + self._assert_status_code_is(run_workflow_response, 200) + usage_details_response = self._get("workflows/%s/usage/%s" % (workflow_id, history_id)) + self._assert_status_code_is(usage_details_response, 200) + + @skip_without_tool("cat1") + def test_invocations_not_accessible_by_different_user_for_published_workflow(self): + workflow_id = self.workflow_populator.simple_workflow("test_usage", publish=True) + workflow_request, history_id = self._setup_workflow_run(workflow_id=workflow_id) + workflow_request['workflow_id'] = workflow_request.pop('workflow_id') + response = self._get("workflows/%s/usage" % workflow_id) + self._assert_status_code_is(response, 200) + assert len(response.json()) == 0 + run_workflow_response = self._post("workflows", data=workflow_request) + self._assert_status_code_is(run_workflow_response, 200) + with self._different_user(): + usage_details_response = self._get("workflows/%s/usage/%s" % (workflow_id, history_id)) + self._assert_status_code_is(usage_details_response, 403) + def _update_workflow(self, workflow_id, workflow_object): data = dict( workflow=workflow_object From 413233eec3ce9b3f930db8e40413856831dfc0cb Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 19 Aug 2017 10:42:43 +0200 Subject: [PATCH 2/7] Correctly raise Exception if invocation not found --- lib/galaxy/managers/workflows.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index 2f3d72a64ae..4743d75996e 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -116,12 +116,13 @@ class WorkflowsManager(object): return True def get_invocation(self, trans, decoded_invocation_id): - try: - workflow_invocation = trans.sa_session.query( - self.app.model.WorkflowInvocation - ).get(decoded_invocation_id) - except Exception: - raise exceptions.ObjectNotFound() + workflow_invocation = trans.sa_session.query( + self.app.model.WorkflowInvocation + ).get(decoded_invocation_id) + if not workflow_invocation: + encoded_wfi_id = trans.security.encode_id(decoded_invocation_id) + message = "'%s' is not a valid workflow invocation id" % encoded_wfi_id + raise exceptions.ObjectNotFound(message) self.check_security(trans, workflow_invocation, check_ownership=True, check_accessible=False) return workflow_invocation From 0a4203ed92acb795a2f32c5ca095146b388a2ec2 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 19 Aug 2017 12:55:08 +0200 Subject: [PATCH 3/7] Fix test to use invocation_id instead of history_id --- test/api/test_workflows.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/test/api/test_workflows.py b/test/api/test_workflows.py index 58f282b6e6f..98d37bb4686 100644 --- a/test/api/test_workflows.py +++ b/test/api/test_workflows.py @@ -94,7 +94,7 @@ class BaseWorkflowsApiTestCase(api.ApiTestCase): def _setup_workflow_run(self, workflow=None, inputs_by='step_id', history_id=None, workflow_id=None): if not workflow_id: - workflow_id = self.workflow_populator.create_workflow( workflow ) + workflow_id = self.workflow_populator.create_workflow(workflow) if not history_id: history_id = self.dataset_populator.new_history() hda1 = self.dataset_populator.new_dataset(history_id, content="1 2 3") @@ -1830,7 +1830,9 @@ steps: assert len(response.json()) == 0 run_workflow_response = self._post("workflows", data=workflow_request) self._assert_status_code_is(run_workflow_response, 200) - usage_details_response = self._get("workflows/%s/usage/%s" % (other_id, history_id)) + run_workflow_response = run_workflow_response.json() + invocation_id = run_workflow_response['id'] + usage_details_response = self._get("workflows/%s/usage/%s" % (other_id, invocation_id)) self._assert_status_code_is(usage_details_response, 200) @skip_without_tool("cat1") @@ -1844,7 +1846,9 @@ steps: assert len(response.json()) == 0 run_workflow_response = self._post("workflows", data=workflow_request) self._assert_status_code_is(run_workflow_response, 200) - usage_details_response = self._get("workflows/%s/usage/%s" % (workflow_id, history_id)) + run_workflow_response = run_workflow_response.json() + invocation_id = run_workflow_response['id'] + usage_details_response = self._get("workflows/%s/usage/%s" % (workflow_id, invocation_id)) self._assert_status_code_is(usage_details_response, 200) @skip_without_tool("cat1") @@ -1857,8 +1861,10 @@ steps: assert len(response.json()) == 0 run_workflow_response = self._post("workflows", data=workflow_request) self._assert_status_code_is(run_workflow_response, 200) + run_workflow_response = run_workflow_response.json() + invocation_id = run_workflow_response['id'] with self._different_user(): - usage_details_response = self._get("workflows/%s/usage/%s" % (workflow_id, history_id)) + usage_details_response = self._get("workflows/%s/usage/%s" % (workflow_id, invocation_id)) self._assert_status_code_is(usage_details_response, 403) def _update_workflow(self, workflow_id, workflow_object): From 80e264d35ffbf9eecacf37918fab7f7dbfa27cc7 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sat, 19 Aug 2017 14:43:59 +0200 Subject: [PATCH 4/7] Check ownership of invocation, instead of stored_workflow --- lib/galaxy/managers/workflows.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index 4743d75996e..0f58e3f7b2e 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -167,18 +167,21 @@ class WorkflowsManager(object): return workflow_invocation_step def build_invocations_query(self, trans, decoded_stored_workflow_id): - try: - stored_workflow = trans.sa_session.query( - self.app.model.StoredWorkflow - ).get(decoded_stored_workflow_id) - except Exception: + """Get invocations owned by the current user.""" + stored_workflow = trans.sa_session.query( + self.app.model.StoredWorkflow + ).get(decoded_stored_workflow_id) + if not stored_workflow: raise exceptions.ObjectNotFound() - self.check_security(trans, stored_workflow, check_ownership=True, check_accessible=False) - return trans.sa_session.query( + invocations = trans.sa_session.query( model.WorkflowInvocation ).filter_by( workflow_id=stored_workflow.latest_workflow_id ) + return [inv for inv in invocations if self.check_security(trans, + inv, + check_ownership=True, + check_accessible=False)] CreatedWorkflow = namedtuple("CreatedWorkflow", ["stored_workflow", "workflow", "missing_tools"]) From 2d16470132b86fb04bd67665bbdaf20e55874170 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 20 Aug 2017 11:21:01 +0200 Subject: [PATCH 5/7] Allow admin user to view workflow invocation (thx @nsoranzo) --- lib/galaxy/managers/workflows.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index 0f58e3f7b2e..508a673ac8f 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -94,7 +94,7 @@ class WorkflowsManager(object): if isinstance(has_workflow, model.WorkflowInvocation): # We use the the owner of the history that is associated to the invocation as a proxy # for the owner of the invocation. - if trans.user != has_workflow.history.user: + if trans.user != has_workflow.history.user and not trans.user_is_admin(): raise exceptions.ItemOwnershipException() else: return True From cff306b86bc6368b4489805b96f92da04761c013 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 20 Aug 2017 11:24:09 +0200 Subject: [PATCH 6/7] Only skip check_security checks if check_ownership and check_accessible are false --- lib/galaxy/managers/workflows.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/galaxy/managers/workflows.py b/lib/galaxy/managers/workflows.py index 508a673ac8f..10349db63d0 100644 --- a/lib/galaxy/managers/workflows.py +++ b/lib/galaxy/managers/workflows.py @@ -87,7 +87,7 @@ class WorkflowsManager(object): workflowinvocations. Throw an exception or returns True if user has needed level of access. """ - if not check_ownership or check_accessible: + if not check_ownership and not check_accessible: return True # If given an invocation verify ownership of invocation From 64f8e74adeb5f46e781fb3cf54294097c6301880 Mon Sep 17 00:00:00 2001 From: mvdbeek Date: Sun, 20 Aug 2017 12:49:43 +0200 Subject: [PATCH 7/7] Fix subworkflow tests --- test/unit/workflows/workflow_support.py | 1 + 1 file changed, 1 insertion(+) diff --git a/test/unit/workflows/workflow_support.py b/test/unit/workflows/workflow_support.py index 889c7071be4..ee04ded4fc9 100644 --- a/test/unit/workflows/workflow_support.py +++ b/test/unit/workflows/workflow_support.py @@ -18,6 +18,7 @@ class MockTrans(object): def save_workflow(self, workflow): stored_workflow = model.StoredWorkflow() stored_workflow.latest_workflow = workflow + workflow.stored_workflow = stored_workflow stored_workflow.user = self.user self.sa_session.add(stored_workflow) self.sa_session.flush()