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
 
-  def find_objects_for_index
+  def apply_filters
     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

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
-  # 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
@@ -105,14 +106,4 @@ class Link < ArvadosModel
     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

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
-        # 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',
+                   lookup_uuids,
                    Group.uuid_like_pattern,
+                   'can_manage',
                    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
 
+  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
-    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|
@@ -161,15 +256,16 @@ class PermissionTest < ActiveSupport::TestCase
         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
-        ([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
-        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