[ARVADOS] updated: 1.3.0-2619-gb5db5ba31

Git user git at public.arvados.org
Mon Jun 1 22:04:51 UTC 2020


Summary of changes:
 services/api/app/models/group.rb      | 31 +++++++++++++++++++++++++++++++
 services/api/test/fixtures/groups.yml |  6 +++---
 2 files changed, 34 insertions(+), 3 deletions(-)

  discards  edbb66ec872bfc285f53c5a9e85773139f80f91e (commit)
  discards  a6f7c59c2dd70975d1c22ccbf317d4914198b191 (commit)
  discards  618ce6d7a5bd435fc9a885a3e555ccffd0dca785 (commit)
       via  b5db5ba3120c69ecd2018c48693f589c5cda53e8 (commit)
       via  d1497e36749831f60d7203210f4f82ac0589a1a0 (commit)
       via  3ecdb23b937d71121945c08ec4f88007647f341c (commit)

This update added new revisions after undoing existing revisions.  That is
to say, the old revision is not a strict subset of the new revision.  This
situation occurs when you --force push a change and generate a repository
containing something like this:

 * -- * -- B -- O -- O -- O (edbb66ec872bfc285f53c5a9e85773139f80f91e)
            \
             N -- N -- N (b5db5ba3120c69ecd2018c48693f589c5cda53e8)

When this happens we assume that you've already had alert emails for all
of the O revisions, and so we here report only the revisions in the N
branch from the common base, B.

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 b5db5ba3120c69ecd2018c48693f589c5cda53e8
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon Jun 1 18:03:56 2020 -0400

    16007: Roles are owned by system user
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 769b0ab63..05daec1d2 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -25,6 +25,8 @@ class Group < ArvadosModel
   before_update :before_ownership_change
   after_update :after_ownership_change
 
+  after_create :add_role_manage_link
+
   after_update :update_trash
   before_destroy :clear_permissions_and_trash
 
@@ -114,4 +116,33 @@ delete from trashed_groups where group_uuid=$1
     end
     true
   end
+
+  def ensure_owner_uuid_is_permitted
+    if group_class == "role"
+      @role_creator = nil
+      if new_record?
+        @role_creator = owner_uuid
+        self.owner_uuid = system_user_uuid
+        return true
+      end
+      if self.owner_uuid != system_user_uuid
+        raise "Owner uuid for role must be system user"
+      end
+      raise PermissionDeniedError unless current_user.can?(manage: uuid)
+      true
+    else
+      super
+    end
+  end
+
+  def add_role_manage_link
+    if group_class == "role" && @role_creator
+      act_as_system_user do
+       Link.create!(tail_uuid: @role_creator,
+                    head_uuid: self.uuid,
+                    link_class: "permission",
+                    name: "can_manage")
+      end
+    end
+  end
 end
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index a64970e60..e328e9a1b 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -18,14 +18,14 @@ private:
 
 private_role:
   uuid: zzzzz-j7d0g-pew6elm53kancon
-  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  owner_uuid: zzzzz-tpzed-000000000000000
   name: Private Role
   description: Private Role
   group_class: role
 
 private_and_can_read_foofile:
   uuid: zzzzz-j7d0g-22xp1wpjul508rk
-  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  owner_uuid: zzzzz-tpzed-000000000000000
   name: Private and Can Read Foofile
   description: Another Private Group
   group_class: role
@@ -95,7 +95,7 @@ asubproject:
 
 future_project_viewing_group:
   uuid: zzzzz-j7d0g-futrprojviewgrp
-  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  owner_uuid: zzzzz-tpzed-000000000000000
   created_at: 2014-04-21 15:37:48 -0400
   modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
   modified_by_user_uuid: zzzzz-tpzed-xurymjxw79nv3jz

commit d1497e36749831f60d7203210f4f82ac0589a1a0
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon Jun 1 14:59:51 2020 -0400

    16007: Only users and roles can be granted permission
    
    Permission link tail_uuid must be a user or group_class role.
    
    Also disallow modifying permission links.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 23d30f24b..a5e86e345 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -580,7 +580,7 @@ class ArvadosModel < ApplicationRecord
       # itself.
       if !current_user.can?(write: self.uuid)
         logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} without write permission"
-        errors.add :uuid, "is not writable"
+        errors.add :uuid, " #{uuid} is not writable by #{current_user.uuid}"
         raise PermissionDeniedError
       end
     end
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index da4ca6c87..a7dfe8175 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -12,8 +12,8 @@ class Link < ArvadosModel
   attribute :properties, :jsonbHash, default: {}
 
   validate :name_links_are_obsolete
-  before_create :permission_to_attach_to_objects
-  before_update :permission_to_attach_to_objects
+  validate :permission_to_attach_to_objects
+  before_update :cannot_alter_permissions
   after_update :call_update_permissions
   after_create :call_update_permissions
   before_destroy :clear_permissions
@@ -50,13 +50,32 @@ class Link < ArvadosModel
     # All users can write links that don't affect permissions
     return true if self.link_class != 'permission'
 
+    rsc_class = ArvadosModel::resource_class_for_uuid tail_uuid
+    if rsc_class == Group
+      tail_obj = Group.find_by_uuid(tail_uuid)
+      if tail_obj.nil?
+        errors.add(:tail_uuid, "does not exist")
+        return false
+      end
+      if tail_obj.group_class != "role"
+        errors.add(:tail_uuid, "must be a role, was #{tail_obj.group_class}")
+        return false
+      end
+    elsif rsc_class != User
+      errors.add(:tail_uuid, "must be a user or role")
+      return false
+    end
+
     # Administrators can grant permissions
     return true if current_user.is_admin
 
     head_obj = ArvadosModel.find_by_uuid(head_uuid)
 
     # No permission links can be pointed to past collection versions
-    return false if head_obj.is_a?(Collection) && head_obj.current_version_uuid != head_uuid
+    if head_obj.is_a?(Collection) && head_obj.current_version_uuid != head_uuid
+      errors.add(:head_uuid, "cannot point to a past version of a collection")
+      return false
+    end
 
     # All users can grant permissions on objects they own or can manage
     return true if current_user.can?(manage: head_obj)
@@ -65,6 +84,16 @@ class Link < ArvadosModel
     false
   end
 
+  def cannot_alter_permissions
+    return true if self.link_class != 'permission' && self.link_class_was != 'permission'
+
+    return true if current_user.andand.uuid == system_user.uuid
+
+    if link_class_changed? || name_changed? || tail_uuid_changed? || head_uuid_changed?
+      raise "Cannot alter a permission link"
+    end
+  end
+
   PERM_LEVEL = {
     'can_read' => 1,
     'can_login' => 1,
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 89235d65b..a64970e60 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -16,6 +16,13 @@ private:
   description: Private Project
   group_class: project
 
+private_role:
+  uuid: zzzzz-j7d0g-pew6elm53kancon
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  name: Private Role
+  description: Private Role
+  group_class: role
+
 private_and_can_read_foofile:
   uuid: zzzzz-j7d0g-22xp1wpjul508rk
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index ff33fe65b..66a62543b 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -87,7 +87,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
           tail_uuid: users(:spectator).uuid,
           link_class: 'permission',
           name: 'can_read',
-          head_uuid: groups(:private).uuid,
+          head_uuid: groups(:private_role).uuid,
           properties: {}
         }
       },
@@ -105,7 +105,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
       params: {
         :format => :json,
         :link => {
-          tail_uuid: groups(:private).uuid,
+          tail_uuid: groups(:private_role).uuid,
           link_class: 'permission',
           name: 'can_read',
           head_uuid: collections(:foo_file).uuid,
@@ -149,7 +149,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
       params: {
         :format => :json,
         :link => {
-          tail_uuid: groups(:private).uuid,
+          tail_uuid: groups(:private_role).uuid,
           link_class: 'permission',
           name: 'can_read',
           head_uuid: collections(:foo_file).uuid,
@@ -173,7 +173,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
           tail_uuid: users(:spectator).uuid,
           link_class: 'permission',
           name: 'can_read',
-          head_uuid: groups(:private).uuid,
+          head_uuid: groups(:private_role).uuid,
           properties: {}
         }
       },
@@ -216,7 +216,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
           tail_uuid: users(:spectator).uuid,
           link_class: 'permission',
           name: 'can_read',
-          head_uuid: groups(:private).uuid,
+          head_uuid: groups(:private_role).uuid,
           properties: {}
         }
       },
@@ -228,7 +228,7 @@ class PermissionsTest < ActionDispatch::IntegrationTest
       params: {
         :format => :json,
         :link => {
-          tail_uuid: groups(:private).uuid,
+          tail_uuid: groups(:private_role).uuid,
           link_class: 'permission',
           name: 'can_read',
           head_uuid: groups(:empty_lonely_group).uuid,
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index e34c1a44f..631e0137c 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -205,7 +205,7 @@ class GroupTest < ActiveSupport::TestCase
   test "trashed does not propagate across permission links" do
     set_user_from_auth :admin
 
-    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_foo = Group.create!(name: "foo", group_class: "role")
     u_bar = User.create!(first_name: "bar")
 
     assert Group.readable_by(users(:admin)).where(uuid: g_foo.uuid).any?
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index c085c8f83..27ae4c6a9 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -393,8 +393,14 @@ 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: "project")
-    col = Collection.create!(owner_uuid: grp.uuid)
+    grp = Group.create!(owner_uuid: system_user_uuid, group_class: "role")
+    col = Collection.create!(owner_uuid: system_user_uuid)
+
+    l0 = Link.create!(tail_uuid: grp.uuid,
+                 head_uuid: col.uuid,
+                 link_class: 'permission',
+                 name: 'can_read')
+
     assert_empty Collection.readable_by(users(:active)).where(uuid: col.uuid)
     assert_empty User.readable_by(users(:active)).where(uuid: users(:project_viewer).uuid)
 

commit 3ecdb23b937d71121945c08ec4f88007647f341c
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Fri May 29 17:25:05 2020 -0400

    16007: Make it so that only projects can own things
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index cc6f7816a..23d30f24b 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -569,6 +569,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
diff --git a/services/api/app/models/database_seeds.rb b/services/api/app/models/database_seeds.rb
index 39f491503..a86a2c854 100644
--- a/services/api/app/models/database_seeds.rb
+++ b/services/api/app/models/database_seeds.rb
@@ -13,7 +13,6 @@ class DatabaseSeeds
     anonymous_group
     anonymous_group_read_permission
     anonymous_user
-    empty_collection
     refresh_permissions
     refresh_trashed
   end
diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 4dc9a434d..769b0ab63 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -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
diff --git a/services/api/lib/current_api_client.rb b/services/api/lib/current_api_client.rb
index c7b48c0cd..f3ec0a03e 100644
--- a/services/api/lib/current_api_client.rb
+++ b/services/api/lib/current_api_client.rb
@@ -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
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 68377eb5f..89235d65b 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -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:
diff --git a/services/api/test/functional/arvados/v1/filters_test.rb b/services/api/test/functional/arvados/v1/filters_test.rb
index b30afd745..26270b1c3 100644
--- a/services/api/test/functional/arvados/v1/filters_test.rb
+++ b/services/api/test/functional/arvados/v1/filters_test.rb
@@ -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
diff --git a/services/api/test/functional/arvados/v1/groups_controller_test.rb b/services/api/test/functional/arvados/v1/groups_controller_test.rb
index 2b5e8d5a9..ff89cd212 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -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
diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index eec41aa08..ff33fe65b 100644
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -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
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index f7a531fc0..e34c1a44f 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -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
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 340a378ba..c085c8f83 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -393,7 +393,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)

-----------------------------------------------------------------------


hooks/post-receive
-- 




More information about the arvados-commits mailing list