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

Scottx611x/tool manager views cleanup #2991

Merged
merged 13 commits into from
Sep 10, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 4 additions & 4 deletions refinery/tool_manager/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ def setUp(self):

# Make reusable requests & responses
self.get_request = self.factory.get(
"{}?dataSetUuid={}".format(
"{}?data_set_uuid={}".format(
self.tool_defs_url_root,
self.dataset.uuid
)
Expand Down Expand Up @@ -705,7 +705,7 @@ def test_request_from_owned_dataset_shows_all_tool_defs(self):

def test_request_from_public_dataset_shows_vis_tools_only(self):
get_request = self.factory.get(
"{}?dataSetUuid={}".format(
"{}?data_set_uuid={}".format(
self.tool_defs_url_root,
self.public_dataset.uuid
)
Expand Down Expand Up @@ -746,7 +746,7 @@ def test_missing_dataset_in_get_yields_bad_request(self):
dataset_uuid = str(uuid.uuid4())

get_request = self.factory.get(
"{}?dataSetUuid={}".format(
"{}?data_set_uuid={}".format(
self.tool_defs_url_root,
dataset_uuid
)
Expand Down Expand Up @@ -2710,7 +2710,7 @@ def test_get_request_tools_owned_by_another_user(self):
# Try to GET the aforementioned Tool, and assert that another user
# can't do so
self._make_tools_get_request(user=self.user2)
self.assertEqual(self.get_response.status_code, 401)
self.assertEqual(self.get_response.status_code, 403)

def test_unallowed_http_verbs(self):
self.create_tool(ToolDefinition.WORKFLOW)
Expand Down
4 changes: 2 additions & 2 deletions refinery/tool_manager/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@

# DRF url routing
tool_manager_router = DefaultRouter()
tool_manager_router.register(r'tools', ToolsViewSet)
tool_manager_router.register(r'tools', ToolsViewSet, base_name="tools")
tool_manager_router.register(
r'tool_definitions',
ToolDefinitionsViewSet,
base_name="tooldefinition"
base_name="tooldefinitions"
)

django_docker_engine_url = url(
Expand Down
110 changes: 28 additions & 82 deletions refinery/tool_manager/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,18 @@

from django.conf import settings
from django.db import transaction
from django.http import (HttpResponseBadRequest, JsonResponse)
from django.http import (HttpResponseBadRequest, HttpResponseForbidden,
JsonResponse)
from django.shortcuts import render

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 .models import ToolDefinition, VisualizationTool, WorkflowTool
from .serializers import ToolDefinitionSerializer, ToolSerializer
from .utils import (
create_tool, user_has_access_to_tool, validate_tool_launch_configuration
Expand All @@ -28,19 +27,13 @@ class ToolManagerViewSetBase(ModelViewSet):
def __init__(self, **kwargs):
super(ToolManagerViewSetBase, self).__init__(**kwargs)
self.data_set = None
self.user_tools = []
self.visualization_tools = None
self.workflow_tools = None

def list(self, request, *args, **kwargs):
try:
data_set_uuid = self.request.query_params[
kwargs["data_set_uuid_lookup_name"]
]
data_set_uuid = self.request.query_params["data_set_uuid"]
except (AttributeError, KeyError) as e:
return HttpResponseBadRequest("Must specify a DataSet "
"UUID: {}".format(e))

try:
self.data_set = DataSet.objects.get(uuid=data_set_uuid)
except (DataSet.DoesNotExist, DataSet.MultipleObjectsReturned) as e:
Expand All @@ -49,25 +42,15 @@ def list(self, request, *args, **kwargs):
.format(data_set_uuid, e)
)
if self.request.user.has_perm('core.read_meta_dataset', self.data_set):
self.visualization_tools = \
VisualizationTool.objects.filter(dataset=self.data_set)
self.user_tools.extend(self.visualization_tools)

self.workflow_tools = WorkflowTool.objects.filter(
dataset=self.data_set
)
self.user_tools.extend(self.workflow_tools)
else:
return Response(
"User is not authorized to access DataSet with UUID: {}"
.format(self.data_set.uuid),
status.HTTP_401_UNAUTHORIZED
)
return super(ToolManagerViewSetBase, self).list(request)
return super(ToolManagerViewSetBase, self).list(request)
return HttpResponseForbidden(
"User is not authorized to access DataSet with UUID: {}"
.format(self.data_set.uuid)
)


class ToolDefinitionsViewSet(ToolManagerViewSetBase):
"""API endpoint that allows for ToolDefinitions to be fetched"""
"""ViewSet that allows for ToolDefinitions to be fetched"""
serializer_class = ToolDefinitionSerializer
lookup_field = 'uuid'
http_method_names = ['get']
Expand All @@ -80,69 +63,32 @@ def get_queryset(self):
return ToolDefinition.objects.filter(
tool_type=ToolDefinition.VISUALIZATION
)

def list(self, request, *args, **kwargs):
return super(ToolDefinitionsViewSet, self).list(
request,
data_set_uuid_lookup_name="dataSetUuid"
)
else:
return [] # get_queryset should return an iterable


class ToolsViewSet(ToolManagerViewSetBase):
"""API endpoint that allows for Tools to be fetched and launched"""
queryset = Tool.objects.all()
"""ViewSet that allows for Tools to be fetched, launched and relaunched"""
serializer_class = ToolSerializer
lookup_field = 'uuid'
http_method_names = ['get', 'post']

def list(self, request, *args, **kwargs):
self.data_set_uuid = request.query_params.get('data_set_uuid')
if self.data_set_uuid:
return super(ToolsViewSet, self).list(
request,
data_set_uuid_lookup_name="data_set_uuid"
)
return super(ToolManagerViewSetBase, self).list(request)

def get_queryset(self):
"""
This view returns a list of all the Tools that the currently user has
at least read_meta permissions on.
"""
# returns user's owned tools
if not self.data_set_uuid:
vis_tools = [v for v in VisualizationTool.objects.all()
if v.get_owner() == self.request.user]

workflow_tools = [w for w in WorkflowTool.objects.all()
if w.get_owner() == self.request.user]

tool_type = self.request.query_params.get("tool_type")
if tool_type == ToolDefinition.VISUALIZATION.lower():
return vis_tools
elif tool_type == ToolDefinition.WORKFLOW.lower():
return workflow_tools
else:
user_tools = vis_tools
user_tools.extend(workflow_tools)
return user_tools

elif self.request.user.has_perm('core.read_meta_dataset',
self.data_set):
tool_type = self.request.query_params.get("tool_type")

if not tool_type:
return self.user_tools

tool_types_to_tools = {
ToolDefinition.VISUALIZATION.lower(): self.visualization_tools,
ToolDefinition.WORKFLOW.lower(): self.workflow_tools
}
# get_queryset should return an iterable
return tool_types_to_tools.get(tool_type.lower()) or []

return Response("User is not authorized to view visualizations.",
status=status.HTTP_401_UNAUTHORIZED)
visualization_tools = list(
VisualizationTool.objects.filter(dataset=self.data_set)
)
workflow_tools = list(
WorkflowTool.objects.filter(dataset=self.data_set)
)
tool_type = self.request.query_params.get("tool_type")
if tool_type is None:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@scottx611x Any user can hit this endpoint and grab analyses or visualizations with a dataSetUuid. Thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this is just the get_queryset method. The permission logic happens in the list()

Actually looking into if I can use DRF permission_classes here instead.

return workflow_tools + visualization_tools
elif tool_type.upper() == ToolDefinition.WORKFLOW:
return workflow_tools
elif tool_type.upper() == ToolDefinition.VISUALIZATION:
return visualization_tools
else:
return [] # get_queryset should return an iterable

def create(self, request, *args, **kwargs):
"""
Expand Down
2 changes: 1 addition & 1 deletion refinery/ui/source/js/commons/services/tool-definitions.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
query: {
method: 'GET',
isArray: true,
params: { dataSetUuid: '' }
params: { data_set_uuid: '' }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, since it's a url query. Worth noting for json it's camelCase styling. https://google.github.io/styleguide/jsoncstyleguide.xml?showone=Property_Name_Format#Property_Name_Format

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I wasn't sold on one way vs. the other, but I can change the backend code to work with dataSetUuid

}
}
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@
.expectGET(
settings.appRoot +
settings.refineryApiV2 +
'/tool_definitions/?dataSetUuid='
'/tool_definitions/?data_set_uuid='
).respond(200, fakeResponse);
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
.expectGET(
settings.appRoot +
settings.refineryApiV2 +
'/tool_definitions/?dataSetUuid=' + window.dataSetUuid
'/tool_definitions/?data_set_uuid=' + window.dataSetUuid
).respond(200, []);

var scope = $rootScope.$new();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@
**/
function getTools () {
var params = {
dataSetUuid: dataSetUuid
data_set_uuid: dataSetUuid
};

var toolDefs = toolDefinitionsService.query(params);
Expand Down