Merge branch '2873-permission-links-ownership'
authorTim Pierce <twp@curoverse.com>
Thu, 3 Jul 2014 15:45:47 +0000 (11:45 -0400)
committerTim Pierce <twp@curoverse.com>
Thu, 3 Jul 2014 15:45:47 +0000 (11:45 -0400)
Closes #2873. Huzzah!

services/api/app/controllers/arvados/v1/links_controller.rb
services/api/app/models/arvados_model.rb
services/api/app/models/link.rb
services/api/app/models/user.rb
services/api/config/routes.rb
services/api/test/integration/permissions_test.rb
services/api/test/unit/link_test.rb
services/api/test/unit/permission_test.rb

index 0772227adca9c0ffa3ac6d541209be8bcf6cecad..f76af60bb93503a3908d48afae6609ff593e9414 100644 (file)
@@ -19,8 +19,34 @@ class Arvados::V1::LinksController < ApplicationController
     super
   end
 
+  def get_permissions
+    if current_user.can?(manage: @object)
+      # find all links and return them
+      @objects = Link.where(link_class: "permission",
+                            head_uuid: params[:uuid])
+      @offset = 0
+      @limit = @objects.count
+      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'
+      @object = ArvadosModel::resource_class_for_uuid(params[:uuid])
+        .readable_by(*@read_users)
+        .where(uuid: params[:uuid])
+        .first
+    else
+      super
+    end
+  end
+
   # Overrides ApplicationController load_where_param
   def load_where_param
     super
index 2df6686f2883edd81adde92e9748b3fa95010614..41286fe0244de58126f76c4f044984056fe5b675 100644 (file)
@@ -160,6 +160,13 @@ class ArvadosModel < ActiveRecord::Base
     attributes
   end
 
+  def has_permission? perm_type, target_uuid
+    Link.where(link_class: "permission",
+               name: perm_type,
+               tail_uuid: uuid,
+               head_uuid: target_uuid).any?
+  end
+
   protected
 
   def ensure_ownership_path_leads_to_user
@@ -447,6 +454,18 @@ class ArvadosModel < ActiveRecord::Base
     nil
   end
 
+  # 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
     @old_etag = etag
     @old_attributes = logged_attributes
index 1b3fc34eab926a4486f794a42e10f5721e7deb46..bb069ee97d3dc6399ea2e48371b1f30952416723 100644 (file)
@@ -51,22 +51,9 @@ class Link < ArvadosModel
     # Administrators can grant permissions
     return true if current_user.is_admin
 
-    # All users can grant permissions on objects they own
-    head_obj = self.class.
-      resource_class_for_uuid(self.head_uuid).
-      where('uuid=?',head_uuid).
-      first
-    if head_obj
-      return true if head_obj.owner_uuid == current_user.uuid
-    end
-
-    # 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
+    # All users can grant permissions on objects they own or can manage
+    head_obj = ArvadosModel.find_by_uuid(head_uuid)
+    return true if current_user.can?(manage: head_obj)
 
     # Default = deny.
     false
@@ -100,4 +87,21 @@ class Link < ArvadosModel
       ensure_owner_uuid_is_permitted
     end
   end
+
+  # 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.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
+      return true
+    else
+      super
+    end
+  end
 end
index 677685d67abdb60270b113ffeb46d6bb5edea81c..e79c485f17493cde51cb7bec59c212bb5dc7857e 100644 (file)
@@ -75,19 +75,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',
index e4d2975a571699d85d39887cd30bd24725639db2..70934553f24d2679b5d42393a14357a4daec85cd 100644 (file)
@@ -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
 
index 2ebd62bc8b9b3a743dba496abce3bf59830e5a53..095c2dcc2e6d420fbd73b2e640779d393ddba5d4 100644 (file)
@@ -283,4 +283,93 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     end
   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}", nil, auth(:active)
+    assert_response 404
+
+    # add some permissions, including can_manage
+    # permission for user :active
+    post "/arvados/v1/links", {
+      :format => :json,
+      :link => {
+        tail_uuid: users(:spectator).uuid,
+        link_class: 'permission',
+        name: 'can_read',
+        head_uuid: groups(:public).uuid,
+        properties: {}
+      }
+    }, auth(:admin)
+    assert_response :success
+    can_read_uuid = json_response['uuid']
+
+    post "/arvados/v1/links", {
+      :format => :json,
+      :link => {
+        tail_uuid: users(:inactive).uuid,
+        link_class: 'permission',
+        name: 'can_write',
+        head_uuid: groups(:public).uuid,
+        properties: {}
+      }
+    }, auth(:admin)
+    assert_response :success
+    can_write_uuid = json_response['uuid']
+
+    post "/arvados/v1/links", {
+      :format => :json,
+      :link => {
+        tail_uuid: users(:active).uuid,
+        link_class: 'permission',
+        name: 'can_manage',
+        head_uuid: groups(:public).uuid,
+        properties: {}
+      }
+    }, auth(:admin)
+    assert_response :success
+    can_manage_uuid = json_response['uuid']
+
+    # Now user :active should be able to retrieve permissions
+    # on group :public.
+    get("/arvados/v1/permissions/#{groups(:public).uuid}",
+        { :format => :json },
+        auth(:active))
+    assert_response :success
+
+    perm_uuids = json_response['items'].map { |item| item['uuid'] }
+    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
+    nonexistent = Group.generate_uuid
+    # make sure it really doesn't exist
+    get "/arvados/v1/groups/#{nonexistent}", nil, auth(:admin)
+    assert_response 404
+
+    get "/arvados/v1/permissions/#{nonexistent}", nil, auth(:active)
+    assert_response 404
+  end
+
+  test "get_permissions returns 404 for unreadable uuid" do
+    get "/arvados/v1/permissions/#{groups(:public).uuid}", nil, auth(:active)
+    assert_response 404
+  end
+
+  test "get_permissions returns 403 if user can read but not manage" do
+    post "/arvados/v1/links", {
+      :link => {
+        tail_uuid: users(:active).uuid,
+        link_class: 'permission',
+        name: 'can_read',
+        head_uuid: groups(:public).uuid,
+        properties: {}
+      }
+    }, auth(:admin)
+    assert_response :success
+
+    get "/arvados/v1/permissions/#{groups(:public).uuid}", nil, auth(:active)
+    assert_response 403
+  end
 end
index 56a38045e4cf75fe2b8289e9f119c2e668c9b97e..e40326504a1283bed77f6d571a14d18051bb2fd9 100644 (file)
@@ -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
index 6e96dccc8df736f303f6926c116363b2a7f89a14..748c7907a29ec4dcc2ca2775ae5dfeb2c0056a98 100644 (file)
@@ -1,6 +1,8 @@
 require 'test_helper'
 
 class PermissionTest < ActiveSupport::TestCase
+  include CurrentApiClient
+
   test "Grant permissions on an object I own" do
     set_user_from_auth :active_trustedclient
 
@@ -28,4 +30,105 @@ class PermissionTest < ActiveSupport::TestCase
     assert_empty(Link.where(head_uuid: ob_uuid),
                  "Permission link was not deleted when object was deleted")
   end
+
+  test "permission links owned by root" do
+    set_user_from_auth :active_trustedclient
+    ob = Specimen.create!
+    perm_link = Link.create!(tail_uuid: users(:active).uuid,
+                             head_uuid: ob.uuid,
+                             link_class: 'permission',
+                             name: 'can_read')
+    assert_equal system_user_uuid, perm_link.owner_uuid
+  end
+
+  test "readable_by" do
+    set_user_from_auth :active_trustedclient
+
+    ob = Specimen.create!
+    Link.create!(tail_uuid: users(:active).uuid,
+                 head_uuid: ob.uuid,
+                 link_class: 'permission',
+                 name: 'can_read')
+    assert Specimen.readable_by(users(:active)).where(uuid: ob.uuid).any?, "user does not have read permission"
+  end
+
+  test "writable_by" do
+    set_user_from_auth :active_trustedclient
+
+    ob = Specimen.create!
+    Link.create!(tail_uuid: users(:active).uuid,
+                 head_uuid: ob.uuid,
+                 link_class: 'permission',
+                 name: 'can_write')
+    assert ob.writable_by.include?(users(:active).uuid), "user does not have write permission"
+  end
+
+  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!
+    # grant can_manage permission to active
+    perm_link = Link.create!(tail_uuid: users(:active).uuid,
+                             head_uuid: ob.uuid,
+                             link_class: 'permission',
+                             name: 'can_manage')
+    # ob is owned by :admin, the link is owned by root
+    assert_equal users(:admin).uuid, ob.owner_uuid
+    assert_equal system_user_uuid, perm_link.owner_uuid
+
+    # user "active" can modify the permission link
+    set_user_from_auth :active_trustedclient
+    perm_link.properties["foo"] = 'bar'
+    assert perm_link.save, "could not save modified link"
+
+    assert_equal 'bar', perm_link.properties['foo'], "link properties do not include foo = bar"
+  end
+
+  test "user without can_manage permission may not modify permission link" do
+    set_user_from_auth :admin
+
+    ob = Specimen.create!
+    # grant can_manage permission to active
+    perm_link = Link.create!(tail_uuid: users(:active).uuid,
+                             head_uuid: ob.uuid,
+                             link_class: 'permission',
+                             name: 'can_read')
+    # ob is owned by :admin, the link is owned by root
+    assert_equal ob.owner_uuid, users(:admin).uuid
+    assert_equal perm_link.owner_uuid, system_user_uuid
+
+    # user "active" may not modify the permission link
+    set_user_from_auth :active_trustedclient
+    perm_link.name = 'can_manage'
+    assert_raises ArvadosModel::PermissionDeniedError do
+      perm_link.save
+    end
+  end
 end