3171: Do not follow permission graph through a User, unless permission on the User...

diff --git a/services/api/app/controllers/arvados/v1/users_controller.rb b/services/api/app/controllers/arvados/v1/users_controller.rb
index 271299b6c97608c68b14d0500e86d779ba610b3d..0afb4e506c765a884c086cd41bb9450452fcb2ff 100644 (file)
--- a/services/api/app/controllers/arvados/v1/users_controller.rb
+++ b/services/api/app/controllers/arvados/v1/users_controller.rb
@@ -136,14 +136,16 @@ class Arvados::V1::UsersController < ApplicationController
     }
   end
 
     }
   end
 

-  def find_objects_for_index
+  def apply_filters

     if (action_name == "index") and (not @read_users.any? { |u| u.is_admin })
     if (action_name == "index") and (not @read_users.any? { |u| u.is_admin })

-      # Non-admin index returns very basic information about all active users.
-      # We ignore where and filters params to avoid leaking information.
-      @where = {}
-      @filters = []
-      @select = ["uuid", "is_active", "email", "first_name", "last_name"]
-      @objects = model_class.where(is_active: true)
+      # Non-admin index returns very basic information about readable users.
+      safe_attrs = ["uuid", "is_active", "email", "first_name", "last_name"]
+      if @select
+        @select = @select & safe_attrs
+      else
+        @select = safe_attrs
+      end
+      @filters += [['is_active', '=', true]]

     end
     super
   end
     end
     super
   end

diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index 3058081c1e85b46f29060e35b138c205255d8545..f4a7de29e276d065849911ef04c1c78770cc97f6 100644 (file)
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -89,8 +89,9 @@ class Link < ArvadosModel
   end
 
   # A user is permitted to create, update or modify a permission link
   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.
+  # if and only if they have "manage" permission on the object
+  # indicated by the permission link's head_uuid.
+  #

   # All other links are treated as regular ArvadosModel objects.
   #
   def ensure_owner_uuid_is_permitted
   # All other links are treated as regular ArvadosModel objects.
   #
   def ensure_owner_uuid_is_permitted


@@ -105,14 +106,4 @@ class Link < ArvadosModel
     end
   end
 
     end
   end
 

-  # A user can give all other users permissions on projects.
-  def skip_uuid_read_permission_check
-    skipped_attrs = super
-    if link_class == "permission" and
-        (ArvadosModel.resource_class_for_uuid(head_uuid) == Group) and
-        (ArvadosModel.resource_class_for_uuid(tail_uuid) == User)
-      skipped_attrs << "tail_uuid"
-    end
-    skipped_attrs
-  end

 end
 end

diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 19e84dca7cf4d37288882cc46037c3494a6e8c34..4df38ced67e1c0d0bce3b5e3c7743ac2b73e522f 100644 (file)
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -103,12 +103,13 @@ class User < ArvadosModel
         Group.where('owner_uuid in (?)', lookup_uuids).each do |group|
           newgroups << [group.owner_uuid, group.uuid, 'can_manage']
         end
         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,
+        # add any permission links from the current lookup_uuids to a Group.
+        Link.where('link_class = ? and tail_uuid in (?) and ' \
+                   '(head_uuid like ? or (name = ? and head_uuid like ?))',

                    'permission',
                    'permission',

+                   lookup_uuids,

                    Group.uuid_like_pattern,
                    Group.uuid_like_pattern,

+                   'can_manage',

                    User.uuid_like_pattern).each do |link|
           newgroups << [link.tail_uuid, link.head_uuid, link.name]
         end
                    User.uuid_like_pattern).each do |link|
           newgroups << [link.tail_uuid, link.head_uuid, link.name]
         end

diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 24399f500e224151d175f94049cb0eb4181cf129..315f2bd472ddc05799ff2811d7d99d30a15d83a7 100644 (file)
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -132,10 +132,105 @@ class PermissionTest < ActiveSupport::TestCase
     end
   end
 
     end
   end
 

+  test "manager user gets permission to minions' articles via can_manage link" do
+    manager = create :active_user, first_name: "Manage", last_name: "Er"
+    minion = create :active_user, first_name: "Min", last_name: "Ion"
+    minions_specimen = act_as_user minion do
+      Specimen.create!
+    end
+    # Manager creates a group. (Make sure it doesn't magically give
+    # anyone any additional permissions.)
+    g = nil
+    act_as_user manager do
+      g = create :group, name: "NoBigSecret Lab"
+      assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
+                   "saw a user I shouldn't see")
+      assert_raises(ArvadosModel::PermissionDeniedError,
+                    ActiveRecord::RecordInvalid,
+                    "gave can_read permission to a user I shouldn't see") do
+        create(:permission_link,
+               name: 'can_read', tail_uuid: minion.uuid, head_uuid: g.uuid)
+      end
+      %w(can_manage can_write can_read).each do |perm_type|
+        assert_raises(ArvadosModel::PermissionDeniedError,
+                      ActiveRecord::RecordInvalid,
+                      "escalated privileges") do
+          create(:permission_link,
+                 name: perm_type, tail_uuid: g.uuid, head_uuid: minion.uuid)
+        end
+      end
+      assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
+                   "manager saw minion too soon")
+      assert_empty(User.readable_by(minion).where(uuid: manager.uuid),
+                   "minion saw manager too soon")
+      assert_empty(Group.readable_by(minion).where(uuid: g.uuid),
+                   "minion saw manager's new NoBigSecret Lab group too soon")
+
+      # Manager declares everybody on the system should be able to see
+      # the NoBigSecret Lab group.
+      create(:permission_link,
+             name: 'can_read',
+             tail_uuid: 'zzzzz-j7d0g-fffffffffffffff',
+             head_uuid: g.uuid)
+      # ...but nobody has joined the group yet. Manager still can't see
+      # minion.
+      assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
+                   "manager saw minion too soon")
+    end
+
+    act_as_user minion do
+      # Minion can see the group.
+      assert_not_empty(Group.readable_by(minion).where(uuid: g.uuid),
+                       "minion could not see the NoBigSecret Lab group")
+      # Minion joins the group.
+      create(:permission_link,
+             name: 'can_read',
+             tail_uuid: g.uuid,
+             head_uuid: minion.uuid)
+    end
+
+    act_as_user manager do
+      # Now, manager can see minion.
+      assert_not_empty(User.readable_by(manager).where(uuid: minion.uuid),
+                       "manager could not see minion")
+      # But cannot obtain further privileges this way.
+      assert_raises(ArvadosModel::PermissionDeniedError,
+                    "escalated privileges") do
+        create(:permission_link,
+               name: 'can_manage', tail_uuid: manager.uuid, head_uuid: minion.uuid)
+      end
+      assert_empty(Specimen
+                     .readable_by(manager)
+                     .where(uuid: minions_specimen.uuid),
+                   "manager saw the minion's private stuff")
+      assert_raises(ArvadosModel::PermissionDeniedError,
+                   "manager could update minion's private stuff") do
+        minions_specimen.update_attributes(properties: {'x' => 'y'})
+      end
+    end
+
+    act_as_system_user do
+      # Root can give Manager more privileges over Minion.
+      create(:permission_link,
+             name: 'can_manage', tail_uuid: g.uuid, head_uuid: minion.uuid)
+    end
+
+    act_as_user manager do
+      # Now, manager can read and write Minion's stuff.
+      assert_not_empty(Specimen
+                         .readable_by(manager)
+                         .where(uuid: minions_specimen.uuid),
+                       "manager could not find minion's specimen by uuid")
+      assert_equal(true,
+                   minions_specimen.update_attributes(properties: {'x' => 'y'}),
+                   "manager could not update minion's specimen object")
+    end
+  end
+

   test "users with bidirectional read permission in group can see each other, but cannot see each other's private articles" do
   test "users with bidirectional read permission in group can see each other, but cannot see each other's private articles" do

-    a = create :active_user first_name: "A"
-    b = create :active_user first_name: "B"
-    other = create :active_user first_name: "OTHER"
+    a = create :active_user, first_name: "A"
+    b = create :active_user, first_name: "B"
+    other = create :active_user, first_name: "OTHER"

     act_as_system_user do
       g = create :group
       [a,b].each do |u|
     act_as_system_user do
       g = create :group
       [a,b].each do |u|


@@ -161,15 +256,16 @@ class PermissionTest < ActiveSupport::TestCase
         assert_raises ArvadosModel::PermissionDeniedError, "wrote without perm" do
           other.update_attributes!(prefs: {'pwned' => true})
         end
         assert_raises ArvadosModel::PermissionDeniedError, "wrote without perm" do
           other.update_attributes!(prefs: {'pwned' => true})
         end

-        assert_equal true, u.update_attributes!(prefs: {'thisisme' => true})
+        assert_equal(true, u.update_attributes!(prefs: {'thisisme' => true}),
+                     "#{u.first_name} can't update its own prefs")

       end
       act_as_user other do
       end
       act_as_user other do

-        ([other, a, b] - [u]).each do |x|
-          assert_raises ArvadosModel::PermissionDeniedError, "wrote without perm" do
-            x.update_attributes!(prefs: {'pwned' => true})
-          end
+        assert_raises(ArvadosModel::PermissionDeniedError,
+                        "OTHER wrote #{u.first_name} without perm") do
+          u.update_attributes!(prefs: {'pwned' => true})

         end
         end

-        assert_equal true, other.update_attributes!(prefs: {'thisisme' => true})
+        assert_equal(true, other.update_attributes!(prefs: {'thisisme' => true}),
+                     "OTHER can't update its own prefs")

       end
     end
   end
       end
     end
   end