16007: Make it so that only projects can own things
authorPeter Amstutz <peter.amstutz@curii.com>
Fri, 29 May 2020 21:25:05 +0000 (17:25 -0400)
committerPeter Amstutz <peter.amstutz@curii.com>
Wed, 17 Jun 2020 17:57:25 +0000 (13:57 -0400)
Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

services/api/app/models/arvados_model.rb
services/api/app/models/database_seeds.rb
services/api/app/models/group.rb
services/api/lib/current_api_client.rb
services/api/test/fixtures/groups.yml
services/api/test/functional/arvados/v1/filters_test.rb
services/api/test/functional/arvados/v1/groups_controller_test.rb
services/api/test/integration/permissions_test.rb
services/api/test/unit/group_test.rb
services/api/test/unit/permission_test.rb

index 8afebfb79eab56e24e83394f3923469d28faba94..7270f7cdf2b060166cbd5229e416244f141442de 100644 (file)
@@ -574,6 +574,9 @@ class ArvadosModel < ApplicationRecord
           logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
           errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
           raise PermissionDeniedError
+        elsif rsc_class == Group && Group.find_by_uuid(owner_uuid).group_class != "project"
+          errors.add :owner_uuid, "must be a project"
+          raise PermissionDeniedError
         end
       end
     else
index 39f491503ee583033f80ae72ef982261c6ba0af9..a86a2c85440d9396396dd27348d0631f46958194 100644 (file)
@@ -13,7 +13,6 @@ class DatabaseSeeds
     anonymous_group
     anonymous_group_read_permission
     anonymous_user
-    empty_collection
     refresh_permissions
     refresh_trashed
   end
index d6fc818cd7d59dcb239e967a1bfdf63e2aed0121..e16433c8157660d7a486879555ef61406557d772 100644 (file)
@@ -49,6 +49,9 @@ class Group < ArvadosModel
     if group_class != 'project' && group_class != 'role'
       errors.add :group_class, "value must be one of 'project' or 'role', was '#{group_class}'"
     end
+    if group_class_changed? && !group_class_was.nil?
+      errors.add :group_class, "cannot be modified after record is created"
+    end
   end
 
   def update_trash
index c7b48c0cdd6ff1ed0056a32e49f65c106afc100c..f3ec0a03e2110f9beb4f8d042b33142c75e30278 100644 (file)
@@ -8,7 +8,6 @@ $all_users_group = nil
 $anonymous_user = nil
 $anonymous_group = nil
 $anonymous_group_read_permission = nil
-$empty_collection = nil
 
 module CurrentApiClient
   def current_user
@@ -90,7 +89,8 @@ module CurrentApiClient
         ActiveRecord::Base.transaction do
           Group.where(uuid: system_group_uuid).
             first_or_create!(name: "System group",
-                             description: "System group") do |g|
+                             description: "System group",
+                             group_class: "role") do |g|
             g.save!
             User.all.collect(&:uuid).each do |user_uuid|
               Link.create!(link_class: 'permission',
@@ -188,22 +188,10 @@ module CurrentApiClient
     end
   end
 
-  def empty_collection_uuid
+  def empty_collection_pdh
     'd41d8cd98f00b204e9800998ecf8427e+0'
   end
 
-  def empty_collection
-    $empty_collection = check_cache $empty_collection do
-      act_as_system_user do
-        ActiveRecord::Base.transaction do
-          Collection.
-            where(portable_data_hash: empty_collection_uuid).
-            first_or_create!(manifest_text: '', owner_uuid: anonymous_group.uuid)
-        end
-      end
-    end
-  end
-
   private
 
   # If the given value is nil, or the cache has been cleared since it
index 68377eb5f9e825ad041c0347a0778a470edab920..89235d65b067029be186027c7e36b8c1227a6904 100644 (file)
@@ -6,15 +6,15 @@ public:
   uuid: zzzzz-j7d0g-it30l961gq3t0oi
   owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
   name: Public
-  description: Public Group
-  group_class: role
+  description: Public Project
+  group_class: project
 
 private:
   uuid: zzzzz-j7d0g-rew6elm53kancon
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
   name: Private
-  description: Private Group
-  group_class: role
+  description: Private Project
+  group_class: project
 
 private_and_can_read_foofile:
   uuid: zzzzz-j7d0g-22xp1wpjul508rk
@@ -248,17 +248,6 @@ fuse_owned_project:
   description: Test project belonging to FUSE test user
   group_class: project
 
-group_with_no_class:
-  uuid: zzzzz-j7d0g-groupwithnoclas
-  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
-  created_at: 2014-04-21 15:37:48 -0400
-  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
-  modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz
-  modified_at: 2014-04-21 15:37:48 -0400
-  updated_at: 2014-04-21 15:37:48 -0400
-  name: group_with_no_class
-  description: This group has no class at all. So rude!
-
 # This wouldn't pass model validation, but it enables a workbench
 # infinite-loop test. See #4389
 project_owns_itself:
index b30afd745345df01fdb986d8ead8b8c7ad2ab0c4..26270b1c3c9c9b4da0ec4c03f6a8d6fd861fbe70 100644 (file)
@@ -6,16 +6,16 @@ require 'test_helper'
 
 class Arvados::V1::FiltersTest < ActionController::TestCase
   test '"not in" filter passes null values' do
-    @controller = Arvados::V1::GroupsController.new
+    @controller = Arvados::V1::ContainerRequestsController.new
     authorize_with :admin
     get :index, params: {
-      filters: [ ['group_class', 'not in', ['project']] ],
-      controller: 'groups',
+      filters: [ ['container_uuid', 'not in', ['zzzzz-dz642-queuedcontainer', 'zzzzz-dz642-runningcontainr']] ],
+      controller: 'container_requests',
     }
     assert_response :success
     found = assigns(:objects)
-    assert_includes(found.collect(&:group_class), nil,
-                    "'group_class not in ['project']' filter should pass null")
+    assert_includes(found.collect(&:container_uuid), nil,
+                    "'container_uuid not in [zzzzz-dz642-queuedcontainer, zzzzz-dz642-runningcontainr]' filter should pass null")
   end
 
   test 'error message for non-array element in filters array' do
index 2b5e8d5a9d099947adbb21bec74c8508fd456d16..ff89cd2129b31ad5357d54066262a80cd702e41c 100644 (file)
@@ -29,8 +29,9 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     end
     assert_includes group_uuids, groups(:aproject).uuid
     assert_includes group_uuids, groups(:asubproject).uuid
+    assert_includes group_uuids, groups(:private).uuid
     assert_not_includes group_uuids, groups(:system_group).uuid
-    assert_not_includes group_uuids, groups(:private).uuid
+    assert_not_includes group_uuids, groups(:private_and_can_read_foofile).uuid
   end
 
   test "get list of groups that are not projects" do
@@ -44,8 +45,6 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     end
     assert_not_includes group_uuids, groups(:aproject).uuid
     assert_not_includes group_uuids, groups(:asubproject).uuid
-    assert_includes group_uuids, groups(:private).uuid
-    assert_includes group_uuids, groups(:group_with_no_class).uuid
   end
 
   test "get list of groups with bogus group_class" do
@@ -746,20 +745,23 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     assert_equal 0, json_response['included'].length
   end
 
-  test 'get shared, owned by non-project' do
+  test 'get shared, add permission link' do
     authorize_with :user_bar_in_sharing_group
 
     act_as_system_user do
-      Group.find_by_uuid(groups(:project_owned_by_foo).uuid).update!(owner_uuid: groups(:group_for_sharing_tests).uuid)
+      Link.create!(tail_uuid: groups(:group_for_sharing_tests).uuid,
+                   head_uuid: groups(:project_owned_by_foo).uuid,
+                   link_class: 'permission',
+                   name: 'can_manage')
     end
 
     get :shared, params: {:filters => [["group_class", "=", "project"]], :include => "owner_uuid"}
 
     assert_equal 1, json_response['items'].length
-    assert_equal json_response['items'][0]["uuid"], groups(:project_owned_by_foo).uuid
+    assert_equal groups(:project_owned_by_foo).uuid, json_response['items'][0]["uuid"]
 
     assert_equal 1, json_response['included'].length
-    assert_equal json_response['included'][0]["uuid"], groups(:group_for_sharing_tests).uuid
+    assert_equal users(:user_foo_in_sharing_group).uuid, json_response['included'][0]["uuid"]
   end
 
   ### contents with exclude_home_project
@@ -810,20 +812,23 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     assert_equal 0, json_response['included'].length
   end
 
-  test 'contents, exclude home, owned by non-project' do
+  test 'contents, exclude home, add permission link' do
     authorize_with :user_bar_in_sharing_group
 
     act_as_system_user do
-      Group.find_by_uuid(groups(:project_owned_by_foo).uuid).update!(owner_uuid: groups(:group_for_sharing_tests).uuid)
+      Link.create!(tail_uuid: groups(:group_for_sharing_tests).uuid,
+                   head_uuid: groups(:project_owned_by_foo).uuid,
+                   link_class: 'permission',
+                   name: 'can_manage')
     end
 
     get :contents, params: {:include => "owner_uuid", :exclude_home_project => true}
 
     assert_equal 1, json_response['items'].length
-    assert_equal json_response['items'][0]["uuid"], groups(:project_owned_by_foo).uuid
+    assert_equal groups(:project_owned_by_foo).uuid, json_response['items'][0]["uuid"]
 
     assert_equal 1, json_response['included'].length
-    assert_equal json_response['included'][0]["uuid"], groups(:group_for_sharing_tests).uuid
+    assert_equal users(:user_foo_in_sharing_group).uuid, json_response['included'][0]["uuid"]
   end
 
   test 'contents, exclude home, with parent specified' do
index eec41aa0857fd481d600c69fa51b9514241057cf..ff33fe65b17432a8dcb9af555ad76020b6adeb8f 100644 (file)
@@ -6,7 +6,6 @@ require 'test_helper'
 
 class PermissionsTest < ActionDispatch::IntegrationTest
   include DbCurrentTime
-  include CurrentApiClient  # for empty_collection
   fixtures :users, :groups, :api_client_authorizations, :collections
 
   test "adding and removing direct can_read links" do
@@ -441,7 +440,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
   test "active user can read the empty collection" do
     # The active user should be able to read the empty collection.
 
-    get("/arvados/v1/collections/#{empty_collection_uuid}",
+    get("/arvados/v1/collections/#{empty_collection_pdh}",
       params: {:format => :json},
       headers: auth(:active))
     assert_response :success
index f7a531fc057ca57c47d9827358b10ef5065de141..e34c1a44f6fe14d8abbaf7c9e4af54ff6c8d04c0 100644 (file)
@@ -60,6 +60,43 @@ class GroupTest < ActiveSupport::TestCase
     assert g_foo.errors.messages[:owner_uuid].join(" ").match(/ownership cycle/)
   end
 
+  test "cannot create a group that is not a 'role' or 'project'" do
+    set_user_from_auth :active_trustedclient
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      Group.create!(name: "foo")
+    end
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      Group.create!(name: "foo", group_class: "")
+    end
+
+    assert_raises(ActiveRecord::RecordInvalid) do
+      Group.create!(name: "foo", group_class: "bogus")
+    end
+  end
+
+  test "cannot change group_class on an already created group" do
+    set_user_from_auth :active_trustedclient
+    g = Group.create!(name: "foo", group_class: "role")
+    assert_raises(ActiveRecord::RecordInvalid) do
+      g.update_attributes!(group_class: "project")
+    end
+  end
+
+  test "role cannot own things" do
+    set_user_from_auth :active_trustedclient
+    role = Group.create!(name: "foo", group_class: "role")
+    assert_raises(ArvadosModel::PermissionDeniedError) do
+      Collection.create!(name: "bzzz123", owner_uuid: role.uuid)
+    end
+
+    c = Collection.create!(name: "bzzz124")
+    assert_raises(ArvadosModel::PermissionDeniedError) do
+      c.update_attributes!(owner_uuid: role.uuid)
+    end
+  end
+
   test "trash group hides contents" do
     set_user_from_auth :active_trustedclient
 
@@ -237,7 +274,8 @@ class GroupTest < ActiveSupport::TestCase
     set_user_from_auth :active
     ["", "{SOLIDUS}"].each do |subst|
       Rails.configuration.Collections.ForwardSlashNameSubstitution = subst
-      g = Group.create group_class: "project"
+      proj = Group.create group_class: "project"
+      role = Group.create group_class: "role"
       [[nil, true],
        ["", true],
        [".", false],
@@ -248,11 +286,10 @@ class GroupTest < ActiveSupport::TestCase
        ["../..", subst != ""],
        ["/", subst != ""],
       ].each do |name, valid|
-        g.name = name
-        g.group_class = "role"
-        assert_equal true, g.valid?
-        g.group_class = "project"
-        assert_equal valid, g.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
+        role.name = name
+        assert_equal true, role.valid?
+        proj.name = name
+        assert_equal valid, proj.valid?, "#{name.inspect} should be #{valid ? "valid" : "invalid"}"
       end
     end
   end
index 8e2569ccc4bddc2fab9c7e0f9b970a472a527b14..4849b385ba15990a837055f4d9a38f103932a8f3 100644 (file)
@@ -423,7 +423,7 @@ class PermissionTest < ActiveSupport::TestCase
 
   test "add user to group, then remove them" do
     set_user_from_auth :admin
-    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
+    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "project")
     col = Collection.create!(owner_uuid: grp.uuid)
     assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
     assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)