-
Notifications
You must be signed in to change notification settings - Fork 24
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
Scottx611x/persistent visualization urls #2638
Changes from 4 commits
75588a8
99dbe52
8e608fa
a86d7bf
2614bc9
6e00190
a3e8986
91b6581
ca1272c
a666097
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -72,7 +72,7 @@ | |
VisualizationToolError, WorkflowTool) | ||
from .utils import (FileTypeValidationError, create_tool, | ||
create_tool_definition, get_workflows, | ||
validate_tool_annotation, | ||
user_has_access_to_tool, validate_tool_annotation, | ||
validate_tool_launch_configuration, | ||
validate_workflow_step_annotation) | ||
from .views import ToolDefinitionsViewSet, ToolsViewSet | ||
|
@@ -2979,6 +2979,38 @@ def test_get_with_invalid_tool_type_request_param(self): | |
self._make_tools_get_request(tool_type="coffee") | ||
self.assertEqual(self.get_response.data, []) | ||
|
||
def _test_launch_vis_container(self, user_has_permission=True): | ||
self.create_tool(ToolDefinition.VISUALIZATION) | ||
self.assertFalse(self.tool.is_running()) | ||
|
||
if user_has_permission: | ||
assign_perm('core.read_dataset', self.user, self.tool.dataset) | ||
|
||
# Need to set_password() to be able to login. Otherwise | ||
# usr.password in the hash representation which is not what the | ||
# login() expects | ||
temp_password = "password" | ||
self.user.set_password(temp_password) | ||
self.user.save() | ||
self.client.login(username=self.user.username, | ||
password=temp_password) | ||
|
||
with mock.patch.object(VisualizationTool, "launch") as launch_mock: | ||
get_response = self.client.get( | ||
"{}/".format(self.tool.get_relative_container_url()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the trailing slash required? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is unfortunately, Normal django routing middleware tacks this on automatically. The |
||
) | ||
if user_has_permission: | ||
self.assertTrue(launch_mock.called) | ||
else: | ||
self.assertEqual(get_response.status_code, 403) | ||
self.assertFalse(launch_mock.called) | ||
|
||
def test_vis_tool_url_after_container_removed_relaunches(self): | ||
self._test_launch_vis_container() | ||
|
||
def test_vis_tool_url_user_without_permission(self): | ||
self._test_launch_vis_container(user_has_permission=False) | ||
|
||
|
||
class WorkflowToolLaunchTests(ToolManagerTestBase): | ||
tasks_mock = "analysis_manager.tasks" | ||
|
@@ -3839,6 +3871,12 @@ def test_get_workflows_with_connection_error(self): | |
context.exception.message | ||
) | ||
|
||
def test_user_has_access_to_tool(self): | ||
self.create_tool(ToolDefinition.VISUALIZATION) | ||
assign_perm('core.read_dataset', self.user, self.tool.dataset) | ||
self.assertTrue(user_has_access_to_tool(self.user, self.tool)) | ||
self.assertFalse(user_has_access_to_tool(self.user2, self.tool)) | ||
|
||
|
||
class ParameterTests(TestCase): | ||
def test_cast_param_value_to_proper_type_bool(self): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -565,3 +565,8 @@ def create_expanded_workflow_graph(galaxy_workflow_dict): | |
) | ||
graph[parent_node_id][current_node_id]['input_id'] = edge_input_id | ||
return graph | ||
|
||
|
||
def user_has_access_to_tool(user, tool): | ||
return \ | ||
False if not user.has_perm('core.read_dataset', tool.dataset) else True | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,18 +1,22 @@ | ||
import logging | ||
|
||
from django.conf import settings | ||
from django.db import transaction | ||
from django.http import HttpResponseBadRequest, HttpResponseForbidden | ||
|
||
from django_docker_engine.proxy import Proxy | ||
from rest_framework import status | ||
from rest_framework.decorators import detail_route | ||
from rest_framework.generics import get_object_or_404 | ||
from rest_framework.response import Response | ||
from rest_framework.viewsets import ModelViewSet | ||
|
||
from core.models import DataSet | ||
|
||
from .models import Tool, ToolDefinition, VisualizationTool, WorkflowTool | ||
from .serializers import ToolDefinitionSerializer, ToolSerializer | ||
from .utils import create_tool, validate_tool_launch_configuration | ||
from .utils import (create_tool, user_has_access_to_tool, | ||
validate_tool_launch_configuration) | ||
|
||
logger = logging.getLogger(__name__) | ||
|
||
|
@@ -150,7 +154,7 @@ def relaunch(self, request, *args, **kwargs): | |
.format(tool_uuid, e) | ||
) | ||
|
||
if not request.user.has_perm('core.read_dataset', tool.dataset): | ||
if not user_has_access_to_tool(request.user, tool): | ||
return HttpResponseForbidden( | ||
"Requesting User does not have sufficient permissions to " | ||
"relaunch Tool with uuid: {}".format(tool_uuid) | ||
|
@@ -164,3 +168,59 @@ def relaunch(self, request, *args, **kwargs): | |
except Exception as e: | ||
logger.error(e) | ||
return HttpResponseBadRequest(e) | ||
|
||
|
||
class VisualizationToolProxy(Proxy, object): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was already a visualization-tool-proxy: Perhaps rename to reflect the difference? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would this be better left in the file where it's used? Or would that be weird? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agree with the renaming, I'd push to keep it in |
||
""" | ||
Wrapper around Django-Docker-Engine Proxy to allow for VisualizationTools | ||
that had been launched previously to have persisting urls even after | ||
their containers hve been destroyed, rather than relying on users to | ||
manually relaunch (although that remains an option). | ||
""" | ||
def __init__(self): | ||
super(VisualizationToolProxy, self).__init__( | ||
settings.DJANGO_DOCKER_ENGINE_DATA_DIR, | ||
please_wait_title='Please wait...', | ||
please_wait_body_html=''' | ||
<style> | ||
body {{ | ||
font-family: "Source Sans Pro",Helvetica,Arial,sans-serif; | ||
font-size: 40pt; | ||
text-align: center; | ||
}} | ||
div {{ | ||
position: absolute; | ||
top: 50%; | ||
left: 50%; | ||
margin-right: -50%; | ||
transform: translate(-50%, -50%) | ||
}} | ||
</style> | ||
|
||
<div> | ||
<img src="{0}/logo.svg" width='150'> | ||
<p>Please wait...</p> | ||
<img src="{0}/spinner.gif"> | ||
</div> | ||
'''.format(settings.STATIC_URL + 'images') | ||
) | ||
|
||
def _proxy_view(self, request, container_name, url): | ||
visualization_tool = get_object_or_404( | ||
VisualizationTool, | ||
container_name=container_name | ||
) | ||
if not user_has_access_to_tool(request.user, visualization_tool): | ||
return HttpResponseForbidden( | ||
"Requesting User does not have sufficient permissions to " | ||
"view Tool with uuid: {}".format(visualization_tool.uuid) | ||
) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could be more concise? "User does not have permission to view Tool {}" |
||
|
||
if not visualization_tool.is_running(): | ||
visualization_tool.launch() | ||
|
||
return super(VisualizationToolProxy, self)._proxy_view( | ||
request, | ||
container_name, | ||
url | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"in the hash" -> "is the hash"?