Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix checking WorkflowInvocation for published workflows #4465

Merged

Conversation

Projects
None yet
4 participants
@mvdbeek
Copy link
Member

commented Aug 18, 2017

First step to addressing #4401, since a similar problem occurs with published workflows. I haven't tested shared workflows yet (no API to share workflows yet ...), but this may just be enough.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2017

Arg, these test API failures are certainly related, but they don't make a lot of sense to me, and they pass if I run them through nosetest in the debugger, but not if I run them through nosetest outside of the debugger.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2017

Aha, so it seems that we're not always getting an invocation id for public or shared workflows.
(galaxy.workflow.run: DEBUG: Workflow step 135 of invocation None invoked (0.732 ms))
Weirdly enough we do get some ID later, but when getting the invocation from the database we just get None, hence the error AttributeError: 'NoneType' object has no attribute 'user'.

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2017

Alright, I mixed up history_id and invocation_id, which both happen to be 1 if you run just a single test ... d'oh.

@mvdbeek mvdbeek force-pushed the mvdbeek:workflow_invocation_security_check branch from 7cdd823 to 9d4b074 Aug 19, 2017

@mvdbeek mvdbeek force-pushed the mvdbeek:workflow_invocation_security_check branch 2 times, most recently from 3e26db5 to 3fd7e6c Aug 19, 2017

@mvdbeek

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2017

@galaxybot test this

@mvdbeek mvdbeek force-pushed the mvdbeek:workflow_invocation_security_check branch from 3fd7e6c to 80e264d Aug 19, 2017

@mvdbeek mvdbeek added this to the 17.09 milestone Aug 19, 2017

@mvdbeek mvdbeek changed the title [WIP] Fix checking WorkflowInvocation for published workflows Fix checking WorkflowInvocation for published workflows Aug 19, 2017

@@ -90,9 +90,14 @@ def check_security(self, trans, has_workflow, check_ownership=True, check_access
if not check_ownership or check_accessible:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 20, 2017

Member

Shouldn't the condition be not check_ownership and not check_accessible?

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 20, 2017

Author Member

Right, I think it should. Let's see if that works.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 20, 2017

Author Member

Hmm, this break the subworkflow tests. Subworkflows call get_owned_workflow which calls check_security like this: self.check_security(trans, workflow, check_ownership=True), which effectively used to skip all checks. This fails now when looking for has_workflow.top_level_stored_workflow

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 20, 2017

Author Member

Also get_owned_workflow seems pretty strict as a requirement. If we fix this it would mean a shared workflow that you don't own can't be part of a subworkflow.

This comment has been minimized.

Copy link
@mvdbeek

mvdbeek Aug 20, 2017

Author Member

Alright, I fixed the failing tests with 64f8e74.

But the question remains, should you be able to construct subworkflows with workflows that you don't own ? I think the intention of the code was no, but the implementation was yes (even if you had no right to access the workflow ...). So should we allow this at least for workflows that the user can access? ping @jmchilton

This comment has been minimized.

Copy link
@jmchilton

jmchilton Aug 21, 2017

Member

It seems like we should allow this I guess (alternative we could copy to the workflow as one you do own). Hopefully they at least don't run when you do create them 😓?

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:

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 20, 2017

Member

and not trans.user_is_admin()

@jmchilton jmchilton merged commit e3b3457 into galaxyproject:dev Aug 21, 2017

6 checks passed

api test Build finished. 284 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
framework test Build finished. 161 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 44 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details

@mvdbeek mvdbeek deleted the mvdbeek:workflow_invocation_security_check branch Jun 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.