[ARVADOS] updated: bbc3324f62acfda616c3ef867331bddcdc4f5114
git at public.curoverse.com
git at public.curoverse.com
Wed Jul 2 09:00:59 EDT 2014
Summary of changes:
.../app/controllers/arvados/v1/links_controller.rb | 12 ++++------
services/api/app/models/arvados_model.rb | 12 ++++++++--
services/api/app/models/link.rb | 4 ++--
services/api/test/integration/permissions_test.rb | 28 ++++++++++++++--------
4 files changed, 34 insertions(+), 22 deletions(-)
via bbc3324f62acfda616c3ef867331bddcdc4f5114 (commit)
from 6f70a514652050bde05301a4715be8769f213ac6 (commit)
Those revisions listed above that are new to this repository have
not appeared on any other notification email; so we list those
revisions in full, below.
commit bbc3324f62acfda616c3ef867331bddcdc4f5114
Author: Tim Pierce <twp at curoverse.com>
Date: Wed Jul 2 08:59:58 2014 -0400
2873: changes for code review
Incorporating code review comments:
* @Link#get_permissions@ uses @Link.where@ instead of @find_objects_for_index@; @find_object_for_uuid@ just populates @@object@ with the head_uuid object and leaves @@objects@ alone.
* @ArvadosModel.lookup_by_uuid@ renamed @ArvadosModel.find_by_uuid@ and punts to the superclass if called as a subclass method.
* Test @get_permissions_returns_list@ checks that the :active user can get permissions. Also uses the group fixtures for testing permissions instead of collections (because can_manage permissions only work so far on users and groups.
(refs #2873)
diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb
index c09ebb4..43bab06 100644
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -21,12 +21,8 @@ class Arvados::V1::LinksController < ApplicationController
def get_permissions
if current_user.can?(manage: @object)
# find all links and return them
- @where = { link_class: "permission", head_uuid: params[:uuid] }
- @offset = 0
- @orders = []
- @filters = []
- @objects = nil
- find_objects_for_index
+ @objects = Link.where(link_class: "permission",
+ head_uuid: params[:uuid])
render_list
else
render :json => { errors: ['Forbidden'] }.to_json, status: 403
@@ -39,10 +35,10 @@ class Arvados::V1::LinksController < ApplicationController
# called on a uuid belonging to any class.
def find_object_by_uuid
if action_name == 'get_permissions'
- @objects = ArvadosModel::resource_class_for_uuid(params[:uuid])
+ @object = ArvadosModel::resource_class_for_uuid(params[:uuid])
.readable_by(*@read_users)
.where(uuid: params[:uuid])
- @object = @objects.first
+ .first
else
super
end
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index ce5f161..41286fe 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -454,8 +454,16 @@ class ArvadosModel < ActiveRecord::Base
nil
end
- def self.lookup_by_uuid(uuid)
- ArvadosModel.resource_class_for_uuid(uuid).find_by_uuid(uuid)
+ # ArvadosModel.find_by_uuid needs extra magic to allow it to return
+ # an object in any class.
+ def self.find_by_uuid uuid
+ if self == ArvadosModel
+ # If called directly as ArvadosModel.find_by_uuid rather than via subclass,
+ # delegate to the appropriate subclass based on the given uuid.
+ self.resource_class_for_uuid(uuid).find_by_uuid(uuid)
+ else
+ super
+ end
end
def log_start_state
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 3de536e..bb069ee 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -52,7 +52,7 @@ class Link < ArvadosModel
return true if current_user.is_admin
# All users can grant permissions on objects they own or can manage
- head_obj = ArvadosModel.lookup_by_uuid(head_uuid)
+ head_obj = ArvadosModel.find_by_uuid(head_uuid)
return true if current_user.can?(manage: head_obj)
# Default = deny.
@@ -95,7 +95,7 @@ class Link < ArvadosModel
#
def ensure_owner_uuid_is_permitted
if link_class == 'permission'
- ob = ArvadosModel.lookup_by_uuid(head_uuid)
+ ob = ArvadosModel.find_by_uuid(head_uuid)
raise PermissionDeniedError unless current_user.can?(manage: ob)
# All permission links should be owned by the system user.
self.owner_uuid = system_user_uuid
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index f83e8c3..ae338b7 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -284,6 +284,12 @@ class PermissionsTest < ActionDispatch::IntegrationTest
end
test "get_permissions returns list" do
+ # First confirm that user :active cannot get permissions on group :public
+ get "/arvados/v1/permissions/#{groups(:public).uuid}", {
+ :format => :json,
+ }, auth(:active)
+ assert_response 404
+
# add some permissions
post "/arvados/v1/links", {
:format => :json,
@@ -291,7 +297,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
tail_uuid: users(:spectator).uuid,
link_class: 'permission',
name: 'can_read',
- head_uuid: collections(:foo_file).uuid,
+ head_uuid: groups(:public).uuid,
properties: {}
}
}, auth(:admin)
@@ -301,10 +307,10 @@ class PermissionsTest < ActionDispatch::IntegrationTest
post "/arvados/v1/links", {
:format => :json,
:link => {
- tail_uuid: users(:active).uuid,
+ tail_uuid: users(:inactive).uuid,
link_class: 'permission',
name: 'can_write',
- head_uuid: collections(:foo_file).uuid,
+ head_uuid: groups(:public).uuid,
properties: {}
}
}, auth(:admin)
@@ -314,25 +320,27 @@ class PermissionsTest < ActionDispatch::IntegrationTest
post "/arvados/v1/links", {
:format => :json,
:link => {
- tail_uuid: users(:inactive).uuid,
+ tail_uuid: users(:active).uuid,
link_class: 'permission',
name: 'can_manage',
- head_uuid: collections(:foo_file).uuid,
+ head_uuid: groups(:public).uuid,
properties: {}
}
}, auth(:admin)
assert_response :success
can_manage_uuid = json_response['uuid']
- get "/arvados/v1/permissions/#{collections(:foo_file).uuid}", {
+ # Now user :active should be able to retrieve permissions
+ # on group :public.
+ get "/arvados/v1/permissions/#{groups(:public).uuid}", {
:format => :json,
- }, auth(:admin)
+ }, auth(:active)
assert_response :success
perm_uuids = json_response['items'].map { |item| item['uuid'] }
- assert perm_uuids.include?(can_read_uuid), "can_read_uuid not found"
- assert perm_uuids.include?(can_write_uuid), "can_write_uuid not found"
- assert perm_uuids.include?(can_manage_uuid), "can_manage_uuid not found"
+ assert_includes perm_uuids, can_read_uuid, "can_read_uuid not found"
+ assert_includes perm_uuids, can_write_uuid, "can_write_uuid not found"
+ assert_includes perm_uuids, can_manage_uuid, "can_manage_uuid not found"
end
test "get_permissions returns 404 for nonexistent uuid" do
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list