[ARVADOS] updated: 6f70a514652050bde05301a4715be8769f213ac6
git at public.curoverse.com
git at public.curoverse.com
Fri Jun 27 16:12:16 EDT 2014
Summary of changes:
.../app/controllers/arvados/v1/links_controller.rb | 28 +++++++++
services/api/app/models/arvados_model.rb | 12 ++--
services/api/app/models/link.rb | 31 +++++-----
services/api/app/models/user.rb | 58 ++++---------------
services/api/config/routes.rb | 1 +
services/api/test/integration/permissions_test.rb | 66 ++++++++++++++++++++++
services/api/test/unit/link_test.rb | 4 ++
services/api/test/unit/permission_test.rb | 29 +++++++++-
8 files changed, 159 insertions(+), 70 deletions(-)
via 6f70a514652050bde05301a4715be8769f213ac6 (commit)
from 975b1912434cdd158abcf2d8b882d90c57c4299e (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 6f70a514652050bde05301a4715be8769f213ac6
Author: Tim Pierce <twp at curoverse.com>
Date: Thu Jun 26 14:17:48 2014 -0400
2873: add /permissions API method
The /permissions/:uuid method will return a list of all permissions that
the current user is allowed to see on the given uuid.
* New method LinksController::get_permissions, with a route from
/arvados/v1/permissions.
* LinksController overrides find_object_by_uuid to permit looking up a
uuid in any class, when called by get_permissions.
* Moved link permission checking to Link.ensure_owner_uuid_is_permitted.
* Use current_user.can? to check the user's permission on head_uuid.
Removed unnecessary owns? and can_manage? code.
* Unit tests:
* test/integration/permissions_test.rb: added tests:
* "get_permissions returns list"
* "get_permissions returns 404 for nonexistent uuid"
* "get_permissions returns 403 if user lacks manage permission"
* test/unit/link.rb: test that only permission and name links have
their ownership changed upon save.
* test/unit/permission_test.rb: test the following scenario: when user
"active" owns a group G which can_manage another group H, then
active user is permitted to create permission links directly on
objects in group H.
Refs #2873.
2873: perms
diff --git a/services/api/app/controllers/arvados/v1/links_controller.rb b/services/api/app/controllers/arvados/v1/links_controller.rb
index 188ecfc..c09ebb4 100644
--- a/services/api/app/controllers/arvados/v1/links_controller.rb
+++ b/services/api/app/controllers/arvados/v1/links_controller.rb
@@ -18,8 +18,36 @@ class Arvados::V1::LinksController < ApplicationController
super
end
+ 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
+ render_list
+ else
+ render :json => { errors: ['Forbidden'] }.to_json, status: 403
+ end
+ end
+
protected
+ # Override find_object_by_uuid: the get_permissions method may be
+ # 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])
+ .readable_by(*@read_users)
+ .where(uuid: params[:uuid])
+ @object = @objects.first
+ else
+ super
+ end
+ end
+
# Overrides ApplicationController load_where_param
def load_where_param
super
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index a6f2e41..ce5f161 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -219,14 +219,6 @@ class ArvadosModel < ActiveRecord::Base
end
if new_record?
return true
- elsif respond_to? :link_class and link_class == 'permission'
- # Users are permitted to modify permission links themselves
- # if they have "manage" permission on the destination object.
- if current_user.can_manage? head_uuid
- return true
- else
- raise PermissionDeniedError
- end
elsif current_user.uuid == self.owner_uuid_was or
current_user.uuid == self.uuid or
current_user.can? write: self.owner_uuid_was
@@ -462,6 +454,10 @@ class ArvadosModel < ActiveRecord::Base
nil
end
+ def self.lookup_by_uuid(uuid)
+ ArvadosModel.resource_class_for_uuid(uuid).find_by_uuid(uuid)
+ end
+
def log_start_state
@old_etag = etag
@old_attributes = logged_attributes
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 575f32c..3de536e 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -5,7 +5,6 @@ class Link < ArvadosModel
serialize :properties, Hash
before_create :permission_to_attach_to_objects
before_update :permission_to_attach_to_objects
- before_save :permission_link_ownership, if: "link_class == 'permission'"
after_update :maybe_invalidate_permissions_cache
after_create :maybe_invalidate_permissions_cache
after_destroy :maybe_invalidate_permissions_cache
@@ -53,15 +52,8 @@ class Link < ArvadosModel
return true if current_user.is_admin
# All users can grant permissions on objects they own or can manage
- return true if current_user.can_manage? head_uuid
-
- # Users with "can_grant" permission on an object can grant
- # permissions on that object
- has_grant_permission = self.class.
- where('link_class=? AND name=? AND tail_uuid=? AND head_uuid=?',
- 'permission', 'can_grant', current_user.uuid, self.head_uuid).
- count > 0
- return true if has_grant_permission
+ head_obj = ArvadosModel.lookup_by_uuid(head_uuid)
+ return true if current_user.can?(manage: head_obj)
# Default = deny.
false
@@ -96,9 +88,20 @@ class Link < ArvadosModel
end
end
- # permission_link_ownership: ensure that permission links are
- # owned by root.
- def permission_link_ownership
- self.owner_uuid = system_user_uuid
+ # A user is permitted to create, update or modify a permission link
+ # if and only if they have "manage" permission on the destination
+ # object.
+ # All other links are treated as regular ArvadosModel objects.
+ #
+ def ensure_owner_uuid_is_permitted
+ if link_class == 'permission'
+ ob = ArvadosModel.lookup_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
+ return true
+ else
+ super
+ end
end
end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index f1b5c44..2ef56bf 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -71,19 +71,30 @@ class User < ArvadosModel
# Return a hash of {group_uuid: perm_hash} where perm_hash[:read]
# and perm_hash[:write] are true if this user can read and write
# objects owned by group_uuid.
+ #
+ # The permission graph is built by repeatedly enumerating all
+ # permission links reachable from self.uuid, and then calling
+ # search_permissions
def group_permissions
Rails.cache.fetch "groups_for_user_#{self.uuid}" do
permissions_from = {}
todo = {self.uuid => true}
done = {}
+ # Build the equivalence class of permissions starting with
+ # self.uuid. On each iteration of this loop, todo contains
+ # the next set of uuids in the permission equivalence class
+ # to evaluate.
while !todo.empty?
lookup_uuids = todo.keys
lookup_uuids.each do |uuid| done[uuid] = true end
todo = {}
newgroups = []
+ # include all groups owned by the current set of uuids.
Group.where('owner_uuid in (?)', lookup_uuids).each do |group|
newgroups << [group.owner_uuid, group.uuid, 'can_manage']
end
+ # add any permission links from the current lookup_uuids to a
+ # User or Group.
Link.where('tail_uuid in (?) and link_class = ? and (head_uuid like ? or head_uuid like ?)',
lookup_uuids,
'permission',
@@ -179,55 +190,8 @@ class User < ArvadosModel
self.save!
end
- def owns? object_uuid
- return User.find_user_owning(object_uuid).andand.uuid == uuid
- end
-
- def can_manage? object_uuid
- is_admin or
- owns?(object_uuid) or
- has_permission?(:can_manage, object_uuid)
- end
-
protected
- # Returns the first User found in the ownership path for obj_uuid.
- # If obj_uuid is not owned by any user, returns nil.
- #
- # TODO(twp): this code largely stolen from
- # ArvadosModel::ensure_ownership_path_leads_to_user. See if we can
- # refactor these methods to share more code.
- #
- def self.find_user_owning obj_uuid
- uuid_in_path = {obj_uuid => true}
- # Walk up the owner_uuid chain for obj_uuid until one of these
- # conditions is met:
- # - the owner_uuid belongs to the User class
- # - no owner_uuid is found (no User owns this object)
- # - we discover an ownership cycle (a fatal consistency error)
- #
- x = obj_uuid
- while (owner_class = ArvadosModel.resource_class_for_uuid(x)) != User
- begin
- if !owner_class.respond_to? :find_by_uuid
- raise ActiveRecord::RecordNotFound.new
- else
- x = owner_class.find_by_uuid(x).owner_uuid
- end
- rescue ActiveRecord::RecordNotFound => e
- # errors.add :owner_uuid, "is not owned by any user: #{e}"
- return nil
- end
- # If there is an ownership cycle, we can conclude that
- # no User owns this object.
- if uuid_in_path[x]
- return nil
- end
- uuid_in_path[x] = true
- end
- return owner_class.find_by_uuid(x)
- end
-
def ensure_ownership_path_leads_to_user
true
end
diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb
index e4d2975..7093455 100644
--- a/services/api/config/routes.rb
+++ b/services/api/config/routes.rb
@@ -56,6 +56,7 @@ Server::Application.routes.draw do
get 'logins', on: :member
get 'get_all_logins', on: :collection
end
+ get '/permissions/:uuid', :to => 'links#get_permissions'
end
end
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index 2ebd62b..f83e8c3 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -283,4 +283,70 @@ class PermissionsTest < ActionDispatch::IntegrationTest
end
end
+ test "get_permissions returns list" do
+ # add some permissions
+ post "/arvados/v1/links", {
+ :format => :json,
+ :link => {
+ tail_uuid: users(:spectator).uuid,
+ link_class: 'permission',
+ name: 'can_read',
+ head_uuid: collections(:foo_file).uuid,
+ properties: {}
+ }
+ }, auth(:admin)
+ assert_response :success
+ can_read_uuid = json_response['uuid']
+
+ post "/arvados/v1/links", {
+ :format => :json,
+ :link => {
+ tail_uuid: users(:active).uuid,
+ link_class: 'permission',
+ name: 'can_write',
+ head_uuid: collections(:foo_file).uuid,
+ properties: {}
+ }
+ }, auth(:admin)
+ assert_response :success
+ can_write_uuid = json_response['uuid']
+
+ post "/arvados/v1/links", {
+ :format => :json,
+ :link => {
+ tail_uuid: users(:inactive).uuid,
+ link_class: 'permission',
+ name: 'can_manage',
+ head_uuid: collections(:foo_file).uuid,
+ properties: {}
+ }
+ }, auth(:admin)
+ assert_response :success
+ can_manage_uuid = json_response['uuid']
+
+ get "/arvados/v1/permissions/#{collections(:foo_file).uuid}", {
+ :format => :json,
+ }, auth(:admin)
+ 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"
+ end
+
+ test "get_permissions returns 404 for nonexistent uuid" do
+ nonexistent = Collection.generate_uuid
+ # make sure it really doesn't exist
+ get "/arvados/v1/collections/#{nonexistent}", { :format => :json }, auth(:admin)
+ assert_response 404
+
+ get "/arvados/v1/permissions/#{nonexistent}", { :format => :json }, auth(:active)
+ assert_response 404
+ end
+
+ test "get_permissions returns 403 if user lacks manage permission" do
+ get "/arvados/v1/permissions/#{collections(:foo_file).uuid}", { :format => :json }, auth(:active)
+ assert_response 403
+ end
end
diff --git a/services/api/test/unit/link_test.rb b/services/api/test/unit/link_test.rb
index 56a3804..e403265 100644
--- a/services/api/test/unit/link_test.rb
+++ b/services/api/test/unit/link_test.rb
@@ -13,6 +13,7 @@ class LinkTest < ActiveSupport::TestCase
link_class: 'name',
name: 'foo')
assert a.valid?, a.errors.to_s
+ assert_equal groups(:aproject).uuid, a.owner_uuid
assert_raises ActiveRecord::RecordNotUnique do
b = Link.create!(tail_uuid: groups(:aproject).uuid,
head_uuid: specimens(:owned_by_active_user).uuid,
@@ -27,11 +28,13 @@ class LinkTest < ActiveSupport::TestCase
link_class: 'name',
name: 'foo')
assert a.valid?, a.errors.to_s
+ assert_equal groups(:aproject).uuid, a.owner_uuid
b = Link.create!(tail_uuid: groups(:asubproject).uuid,
head_uuid: specimens(:owned_by_active_user).uuid,
link_class: 'name',
name: 'foo')
assert b.valid?, b.errors.to_s
+ assert_equal groups(:asubproject).uuid, b.owner_uuid
assert_not_equal(a.uuid, b.uuid,
"created two links and got the same uuid back.")
end
@@ -52,6 +55,7 @@ class LinkTest < ActiveSupport::TestCase
head_uuid: ob.uuid,
link_class: 'test',
name: 'test')
+ assert_equal users(:admin).uuid, link.owner_uuid
assert_raises(ActiveRecord::DeleteRestrictionError,
"should not delete #{ob.uuid} with link #{link.uuid}") do
ob.destroy
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 23f5f75..748c790 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -63,7 +63,34 @@ class PermissionTest < ActiveSupport::TestCase
assert ob.writable_by.include?(users(:active).uuid), "user does not have write permission"
end
- test "user with can_manage permission allowed to modify permission link" do
+ test "user owns group, group can_manage object's group, user can add permissions" do
+ set_user_from_auth :admin
+
+ owner_grp = Group.create!(owner_uuid: users(:active).uuid)
+
+ sp_grp = Group.create!
+ sp = Specimen.create!(owner_uuid: sp_grp.uuid)
+
+ manage_perm = Link.create!(link_class: 'permission',
+ name: 'can_manage',
+ tail_uuid: owner_grp.uuid,
+ head_uuid: sp_grp.uuid)
+
+ # active user owns owner_grp, which has can_manage permission on sp_grp
+ # user should be able to add permissions on sp.
+ set_user_from_auth :active_trustedclient
+ test_perm = Link.create(tail_uuid: users(:active).uuid,
+ head_uuid: sp.uuid,
+ link_class: 'permission',
+ name: 'can_write')
+ test_uuid = test_perm.uuid
+ assert test_perm.save, "could not save new permission on target object"
+ assert test_perm.destroy, "could not delete new permission on target object"
+ end
+
+ # TODO(twp): fix bug #3091, which should fix this test.
+ test "can_manage permission on a non-group object" do
+ skip
set_user_from_auth :admin
ob = Specimen.create!
-----------------------------------------------------------------------
hooks/post-receive
--
More information about the arvados-commits
mailing list