[ARVADOS] updated: 1.3.0-2628-g6fff1104a

Git user git at public.arvados.org
Thu Jun 11 17:15:56 UTC 2020


Summary of changes:

  discards  53f015422ac101cb4613b416569c3a59e3c977d7 (commit)
  discards  1f1cec38f109a93513fab7f2a2d0c774290ac8fa (commit)
  discards  4fc51865972d6da87e452e67e94e0658ad9d447e (commit)
  discards  b321c55278ef9026c073999b2c0686113b0662f0 (commit)
  discards  2f60417332da76d28532b206740c9a5bb70b78d3 (commit)
  discards  45a8702148d57c838233ec9496ab23dadd1dd108 (commit)
  discards  ca5707d5d3d72faef87b41391c28ad01e063e1d3 (commit)
  discards  95ebc73d3b172e818c05c0d36b024aec875bddad (commit)
  discards  893ed442bdbb7f3458a4273b274fd13b2a2d9b4b (commit)
  discards  be10b35d06e901ff69e238c3e003080b3e82f190 (commit)
       via  6fff1104ab84f8ed8683cd3a2d5b5d28832ca35e (commit)
       via  940e92544b0b52993315f3f7a92379941d826381 (commit)
       via  283027c14437ad5aa94a489a5a4ef29fdc666755 (commit)
       via  a8683a6beb8c63d5db5442d5a3cc8c3c68a849e6 (commit)
       via  6508dc52a192117b9d2d4b20f8cd328844e85fa6 (commit)
       via  fe054ed4c9019df6efcb93fdd0f55235048f6631 (commit)
       via  62cbe8a4373e5405ceb6fa4b3ef7acde2d85f9ea (commit)
       via  77dcd1d6de3782d2faa596dedac834045b925487 (commit)
       via  58201706fdf0cd305200d4bd5ff25922e281cdc2 (commit)
       via  6f514b3e6aa21afddaa527bf852cff3a5801aa19 (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 (53f015422ac101cb4613b416569c3a59e3c977d7)
            \
             N -- N -- N (6fff1104ab84f8ed8683cd3a2d5b5d28832ca35e)

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 6fff1104ab84f8ed8683cd3a2d5b5d28832ca35e
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Mon Jun 8 10:43:47 2020 -0400

    16007: Migration works with large database
    
    Changing individual permissions on groups that affect a lot of objects
    is expensive.  The migration now suppresses permission updates and
    does a batch update at the end.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/db/migrate/20200602141328_fix_roles_projects.rb b/services/api/db/migrate/20200602141328_fix_roles_projects.rb
index f9b2bd6fa..e17ef6de5 100644
--- a/services/api/db/migrate/20200602141328_fix_roles_projects.rb
+++ b/services/api/db/migrate/20200602141328_fix_roles_projects.rb
@@ -5,9 +5,13 @@
 require 'fix_roles_projects'
 
 class FixRolesProjects < ActiveRecord::Migration[5.0]
-  def change
+  def up
+    # defined in a function for easy testing.
+    fix_roles_projects
+  end
+
+  def down
     # This migration is not reversible.  However, the results are
     # backwards compatible.
-    fix_roles_projects
   end
 end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index a8885f584..1b9a104a6 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -3189,6 +3189,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('20190808145904'),
 ('20190809135453'),
 ('20190905151603'),
-('20200501150153');
+('20200501150153'),
+('20200602141328');
 
 
diff --git a/services/api/lib/fix_roles_projects.rb b/services/api/lib/fix_roles_projects.rb
index dafef61aa..448c50cee 100644
--- a/services/api/lib/fix_roles_projects.rb
+++ b/services/api/lib/fix_roles_projects.rb
@@ -2,23 +2,25 @@
 #
 # SPDX-License-Identifier: AGPL-3.0
 
+include CurrentApiClient
+
 def fix_roles_projects
-  # This migration is not reversible.  However, the behavior it
-  # enforces is backwards-compatible, and most of the time there
-  # shouldn't be anything to do at all.
-  act_as_system_user do
-    ActiveRecord::Base.transaction do
-      q = ActiveRecord::Base.connection.exec_query %{
+  batch_update_permissions do
+    # This migration is not reversible.  However, the behavior it
+    # enforces is backwards-compatible, and most of the time there
+    # shouldn't be anything to do at all.
+    act_as_system_user do
+      ActiveRecord::Base.transaction do
+        q = ActiveRecord::Base.connection.exec_query %{
 select uuid from groups limit 1
 }
 
-      # 1) any group not group_class != project becomes a 'role' (both empty and invalid groups)
-      ActiveRecord::Base.connection.exec_query %{
+        # 1) any group not group_class != project becomes a 'role' (both empty and invalid groups)
+        ActiveRecord::Base.connection.exec_query %{
 UPDATE groups set group_class='role' where group_class != 'project' or group_class is null
     }
 
-      Group.where(group_class: "role").each do |g|
-        if g.owner_uuid != system_user_uuid
+        Group.where("group_class='role' and owner_uuid != '#{system_user_uuid}'").each do |g|
           # 2) Ownership of a role becomes a can_manage link
           Link.create!(link_class: 'permission',
                        name: 'can_manage',
@@ -28,35 +30,46 @@ UPDATE groups set group_class='role' where group_class != 'project' or group_cla
           g.save_with_unique_name!
         end
 
-        # 3) If a role owns anything, give it to system user and it
-        # becomes a can_manage link
         ActiveRecord::Base.descendants.reject(&:abstract_class?).each do |klass|
           next if [ApiClientAuthorization,
                    AuthorizedKey,
-                   Log].include?(klass)
+                   Log,
+                   Group].include?(klass)
           next if !klass.columns.collect(&:name).include?('owner_uuid')
 
-          klass.where(owner_uuid: g.uuid).each do |owned|
+          # 3) If a role owns anything, give it to system user and it
+          # becomes a can_manage link
+          klass.joins("join groups on groups.uuid=#{klass.table_name}.owner_uuid and groups.group_class='role'").each do |owned|
             Link.create!(link_class: 'permission',
                          name: 'can_manage',
-                         tail_uuid: g.uuid,
+                         tail_uuid: owned.owner_uuid,
                          head_uuid: owned.uuid)
             owned.owner_uuid = system_user_uuid
             owned.save_with_unique_name!
           end
         end
-      end
 
-      # 4) Projects can't have outgoing permission links.  Just delete them.
-      q = ActiveRecord::Base.connection.exec_query %{
+        Group.joins("join groups as g2 on g2.uuid=groups.owner_uuid and g2.group_class='role'").each do |owned|
+          Link.create!(link_class: 'permission',
+                       name: 'can_manage',
+                       tail_uuid: owned.owner_uuid,
+                       head_uuid: owned.uuid)
+          owned.owner_uuid = system_user_uuid
+          owned.save_with_unique_name!
+        end
+
+        # 4) Projects can't have outgoing permission links.  Just
+        # print a warning and delete them.
+        q = ActiveRecord::Base.connection.exec_query %{
 select links.uuid from links, groups where groups.uuid = links.tail_uuid and
        links.link_class = 'permission' and groups.group_class = 'project'
 }
-      q.each do |lu|
-        ln = Link.find_by_uuid(lu['uuid'])
-        puts "Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed"
-        Rails.logger.warn "Destroying invalid permission link from project #{ln.tail_uuid} to #{ln.head_uuid}"
-        ln.destroy!
+        q.each do |lu|
+          ln = Link.find_by_uuid(lu['uuid'])
+          puts "WARNING: Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed"
+          Rails.logger.warn "Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed"
+          ln.destroy!
+        end
       end
     end
   end
diff --git a/services/api/lib/update_permissions.rb b/services/api/lib/update_permissions.rb
index 1d9c1006b..ca7e55839 100644
--- a/services/api/lib/update_permissions.rb
+++ b/services/api/lib/update_permissions.rb
@@ -8,6 +8,8 @@ REVOKE_PERM = 0
 CAN_MANAGE_PERM = 3
 
 def update_permissions perm_origin_uuid, starting_uuid, perm_level
+  return if Thread.current[:suppress_update_permissions]
+
   #
   # Update a subset of the permission table affected by adding or
   # removing a particular permission relationship (ownership or a
@@ -130,7 +132,7 @@ end
 
 def check_permissions_against_full_refresh
   # No-op except when running tests
-  return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh]
+  return unless Rails.env == 'test' and !Thread.current[:no_check_permissions_against_full_refresh] and !Thread.current[:suppress_update_permissions]
 
   # For checking correctness of the incremental permission updates.
   # Check contents of the current 'materialized_permission' table
@@ -181,6 +183,17 @@ def skip_check_permissions_against_full_refresh
   end
 end
 
+def batch_update_permissions
+  check_perm_was = Thread.current[:suppress_update_permissions]
+  Thread.current[:suppress_update_permissions] = true
+  begin
+    yield
+  ensure
+    Thread.current[:suppress_update_permissions] = check_perm_was
+    refresh_permissions
+  end
+end
+
 # Used to account for permissions that a user gains by having
 # can_manage on another user.
 #

commit 940e92544b0b52993315f3f7a92379941d826381
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Tue Jun 2 16:49:38 2020 -0400

    16007: Fix FUSE test
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index e328e9a1b..ee0d786bb 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -9,6 +9,13 @@ public:
   description: Public Project
   group_class: project
 
+public_role:
+  uuid: zzzzz-j7d0g-jt30l961gq3t0oi
+  owner_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  name: Public Role
+  description: Public Role
+  group_class: role
+
 private:
   uuid: zzzzz-j7d0g-rew6elm53kancon
   owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
diff --git a/services/fuse/tests/test_mount.py b/services/fuse/tests/test_mount.py
index 593d945cf..b2816ac16 100644
--- a/services/fuse/tests/test_mount.py
+++ b/services/fuse/tests/test_mount.py
@@ -128,7 +128,7 @@ class FuseMagicTest(MountTestBase):
         super(FuseMagicTest, self).setUp(api=api)
 
         self.test_project = run_test_server.fixture('groups')['aproject']['uuid']
-        self.non_project_group = run_test_server.fixture('groups')['public']['uuid']
+        self.non_project_group = run_test_server.fixture('groups')['public_role']['uuid']
         self.collection_in_test_project = run_test_server.fixture('collections')['foo_collection_in_aproject']['name']
 
         cw = arvados.CollectionWriter()

commit 283027c14437ad5aa94a489a5a4ef29fdc666755
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Tue Jun 2 16:45:21 2020 -0400

    16007: Add migration to fix invalid groups & permission links
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/db/migrate/20200602141328_fix_roles_projects.rb b/services/api/db/migrate/20200602141328_fix_roles_projects.rb
new file mode 100644
index 000000000..f9b2bd6fa
--- /dev/null
+++ b/services/api/db/migrate/20200602141328_fix_roles_projects.rb
@@ -0,0 +1,13 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+require 'fix_roles_projects'
+
+class FixRolesProjects < ActiveRecord::Migration[5.0]
+  def change
+    # This migration is not reversible.  However, the results are
+    # backwards compatible.
+    fix_roles_projects
+  end
+end
diff --git a/services/api/lib/fix_roles_projects.rb b/services/api/lib/fix_roles_projects.rb
new file mode 100644
index 000000000..dafef61aa
--- /dev/null
+++ b/services/api/lib/fix_roles_projects.rb
@@ -0,0 +1,63 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+def fix_roles_projects
+  # This migration is not reversible.  However, the behavior it
+  # enforces is backwards-compatible, and most of the time there
+  # shouldn't be anything to do at all.
+  act_as_system_user do
+    ActiveRecord::Base.transaction do
+      q = ActiveRecord::Base.connection.exec_query %{
+select uuid from groups limit 1
+}
+
+      # 1) any group not group_class != project becomes a 'role' (both empty and invalid groups)
+      ActiveRecord::Base.connection.exec_query %{
+UPDATE groups set group_class='role' where group_class != 'project' or group_class is null
+    }
+
+      Group.where(group_class: "role").each do |g|
+        if g.owner_uuid != system_user_uuid
+          # 2) Ownership of a role becomes a can_manage link
+          Link.create!(link_class: 'permission',
+                       name: 'can_manage',
+                       tail_uuid: g.owner_uuid,
+                       head_uuid: g.uuid)
+          g.owner_uuid = system_user_uuid
+          g.save_with_unique_name!
+        end
+
+        # 3) If a role owns anything, give it to system user and it
+        # becomes a can_manage link
+        ActiveRecord::Base.descendants.reject(&:abstract_class?).each do |klass|
+          next if [ApiClientAuthorization,
+                   AuthorizedKey,
+                   Log].include?(klass)
+          next if !klass.columns.collect(&:name).include?('owner_uuid')
+
+          klass.where(owner_uuid: g.uuid).each do |owned|
+            Link.create!(link_class: 'permission',
+                         name: 'can_manage',
+                         tail_uuid: g.uuid,
+                         head_uuid: owned.uuid)
+            owned.owner_uuid = system_user_uuid
+            owned.save_with_unique_name!
+          end
+        end
+      end
+
+      # 4) Projects can't have outgoing permission links.  Just delete them.
+      q = ActiveRecord::Base.connection.exec_query %{
+select links.uuid from links, groups where groups.uuid = links.tail_uuid and
+       links.link_class = 'permission' and groups.group_class = 'project'
+}
+      q.each do |lu|
+        ln = Link.find_by_uuid(lu['uuid'])
+        puts "Projects cannot have outgoing permission links, '#{ln.name}' link from #{ln.tail_uuid} to #{ln.head_uuid} will be removed"
+        Rails.logger.warn "Destroying invalid permission link from project #{ln.tail_uuid} to #{ln.head_uuid}"
+        ln.destroy!
+      end
+    end
+  end
+end
diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index 4293b0466..b30d1edc2 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -198,20 +198,6 @@ foo_file_readable_by_active_redundant_permission_via_private_group:
   head_uuid: zzzzz-4zz18-znfnqtbbv4spc3w
   properties: {}
 
-foo_file_readable_by_aproject:
-  uuid: zzzzz-o0j2j-fp1d8395ldqw22p
-  owner_uuid: zzzzz-tpzed-000000000000000
-  created_at: 2014-01-24 20:42:26 -0800
-  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
-  modified_by_user_uuid: zzzzz-tpzed-000000000000000
-  modified_at: 2014-01-24 20:42:26 -0800
-  updated_at: 2014-01-24 20:42:26 -0800
-  tail_uuid: zzzzz-j7d0g-v955i6s2oi1cbso
-  link_class: permission
-  name: can_read
-  head_uuid: zzzzz-4zz18-znfnqtbbv4spc3w
-  properties: {}
-
 bar_file_readable_by_active:
   uuid: zzzzz-o0j2j-8hppiuduf8eqdng
   owner_uuid: zzzzz-tpzed-000000000000000
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 631e0137c..3d1fda927 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -3,6 +3,7 @@
 # SPDX-License-Identifier: AGPL-3.0
 
 require 'test_helper'
+require 'fix_roles_projects'
 
 class GroupTest < ActiveSupport::TestCase
 
@@ -293,4 +294,53 @@ class GroupTest < ActiveSupport::TestCase
       end
     end
   end
+
+  def insert_group uuid, owner_uuid, name, group_class
+    q = ActiveRecord::Base.connection.exec_query %{
+insert into groups (uuid, owner_uuid, name, group_class, created_at, updated_at)
+       values ('#{uuid}', '#{owner_uuid}',
+               '#{name}', #{if group_class then "'"+group_class+"'" else 'NULL' end},
+               statement_timestamp(), statement_timestamp())
+}
+    uuid
+  end
+
+  test "migration to fix roles and projects" do
+    g1 = insert_group Group.generate_uuid, system_user_uuid, 'group with no class', nil
+    g2 = insert_group Group.generate_uuid, users(:active).uuid, 'role owned by a user', 'role'
+
+    g3 = insert_group Group.generate_uuid, system_user_uuid, 'role that owns a project', 'role'
+    g4 = insert_group Group.generate_uuid, g3, 'the project', 'project'
+
+    g5 = insert_group Group.generate_uuid, users(:active).uuid, 'a project with an outgoing permission link', 'project'
+
+    g6 = insert_group Group.generate_uuid, system_user_uuid, 'name collision', 'role'
+    g7 = insert_group Group.generate_uuid, users(:active).uuid, 'name collision', 'role'
+
+    refresh_permissions
+
+    act_as_system_user do
+      l1 = Link.create!(link_class: 'permission', name: 'can_manage', tail_uuid: g3, head_uuid: g4)
+      q = ActiveRecord::Base.connection.exec_query %{
+update links set tail_uuid='#{g5}' where uuid='#{l1.uuid}'
+}
+    refresh_permissions
+    end
+
+    assert_equal nil, Group.find_by_uuid(g1).group_class
+    assert_equal users(:active).uuid, Group.find_by_uuid(g2).owner_uuid
+    assert_equal g3, Group.find_by_uuid(g4).owner_uuid
+    assert !Link.where(tail_uuid: users(:active).uuid, head_uuid: g2, link_class: "permission", name: "can_manage").any?
+    assert !Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any?
+    assert Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any?
+
+    fix_roles_projects
+
+    assert_equal 'role', Group.find_by_uuid(g1).group_class
+    assert_equal system_user_uuid, Group.find_by_uuid(g2).owner_uuid
+    assert_equal system_user_uuid, Group.find_by_uuid(g4).owner_uuid
+    assert Link.where(tail_uuid: users(:active).uuid, head_uuid: g2, link_class: "permission", name: "can_manage").any?
+    assert Link.where(tail_uuid: g3, head_uuid: g4, link_class: "permission", name: "can_manage").any?
+    assert !Link.where(link_class: 'permission', name: 'can_manage', tail_uuid: g5, head_uuid: g4).any?
+  end
 end

commit a8683a6beb8c63d5db5442d5a3cc8c3c68a849e6
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Tue Jun 2 13:26:48 2020 -0400

    16007: Reenable updating the permission level of a link
    
    Add test to ensure permission table is synchronized when updating the
    level of an existing permission link.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/apps/workbench/test/integration/projects_test.rb b/apps/workbench/test/integration/projects_test.rb
index 17ab5e466..7a5103007 100644
--- a/apps/workbench/test/integration/projects_test.rb
+++ b/apps/workbench/test/integration/projects_test.rb
@@ -132,7 +132,7 @@ class ProjectsTest < ActionDispatch::IntegrationTest
     show_object_using('active', 'groups', 'aproject', 'A Project')
     click_on "Sharing"
     click_on "Share with groups"
-    good_uuid = api_fixture("groups")["private"]["uuid"]
+    good_uuid = api_fixture("groups")["future_project_viewing_group"]["uuid"]
     assert(page.has_selector?(".selectable[data-object-uuid=\"#{good_uuid}\"]"),
            "'share with groups' listing missing owned user group")
     bad_uuid = api_fixture("groups")["asubproject"]["uuid"]
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index e5aa3ff49..fc1cd07f0 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -13,7 +13,7 @@ class Link < ArvadosModel
 
   validate :name_links_are_obsolete
   validate :permission_to_attach_to_objects
-  before_update :cannot_alter_permissions
+  before_update :restrict_alter_permissions
   after_update :call_update_permissions
   after_create :call_update_permissions
   before_destroy :clear_permissions
@@ -50,6 +50,11 @@ class Link < ArvadosModel
     # All users can write links that don't affect permissions
     return true if self.link_class != 'permission'
 
+    if PERM_LEVEL[self.name].nil?
+      errors.add(:name, "is invalid permission, must be one of 'can_read', 'can_write', 'can_manage', 'can_login'")
+      return false
+    end
+
     rsc_class = ArvadosModel::resource_class_for_uuid tail_uuid
     if rsc_class == Group
       tail_obj = Group.find_by_uuid(tail_uuid)
@@ -84,13 +89,13 @@ class Link < ArvadosModel
     false
   end
 
-  def cannot_alter_permissions
+  def restrict_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"
+    if link_class_changed? || tail_uuid_changed? || head_uuid_changed?
+      raise "Can only alter permission link level"
     end
   end
 
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index 51414b65d..ab098007b 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -46,7 +46,7 @@ class PermissionTest < ActiveSupport::TestCase
   end
 
   test "readable_by" do
-    set_user_from_auth :active_trustedclient
+    set_user_from_auth :admin
 
     ob = Collection.create!
     Link.create!(tail_uuid: users(:active).uuid,
@@ -57,7 +57,7 @@ class PermissionTest < ActiveSupport::TestCase
   end
 
   test "writable_by" do
-    set_user_from_auth :active_trustedclient
+    set_user_from_auth :admin
 
     ob = Collection.create!
     Link.create!(tail_uuid: users(:active).uuid,

commit 6508dc52a192117b9d2d4b20f8cd328844e85fa6
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Tue Jun 2 10:09:29 2020 -0400

    16007: Update group-sync tool for new restrictions on roles
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz at curii.com>

diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index 2f6643337..4293b0466 100644
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -1111,3 +1111,17 @@ tagged_collection_readable_by_spectator:
   name: can_read
   head_uuid: zzzzz-4zz18-taggedcolletion
   properties: {}
+
+active_manages_viewing_group:
+  uuid: zzzzz-o0j2j-activemanagesvi
+  owner_uuid: zzzzz-tpzed-000000000000000
+  created_at: 2014-01-24 20:42:26 -0800
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-000000000000000
+  modified_at: 2014-01-24 20:42:26 -0800
+  updated_at: 2014-01-24 20:42:26 -0800
+  tail_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  link_class: permission
+  name: can_manage
+  head_uuid: zzzzz-j7d0g-futrprojviewgrp
+  properties: {}
diff --git a/tools/sync-groups/sync-groups.go b/tools/sync-groups/sync-groups.go
index 7c5cd0558..1d8266661 100644
--- a/tools/sync-groups/sync-groups.go
+++ b/tools/sync-groups/sync-groups.go
@@ -222,8 +222,9 @@ func SetParentGroup(cfg *ConfigParams) error {
 				log.Println("Default parent group not found, creating...")
 			}
 			groupData := map[string]string{
-				"name":       cfg.ParentGroupName,
-				"owner_uuid": cfg.SysUserUUID,
+				"name":        cfg.ParentGroupName,
+				"owner_uuid":  cfg.SysUserUUID,
+				"group_class": "role",
 			}
 			if err := CreateGroup(cfg, &parentGroup, groupData); err != nil {
 				return fmt.Errorf("error creating system user owned group named %q: %s", groupData["name"], err)
@@ -479,17 +480,21 @@ func GetRemoteGroups(cfg *ConfigParams, allUsers map[string]arvados.User) (remot
 
 	params := arvados.ResourceListParams{
 		Filters: []arvados.Filter{{
-			Attr:     "owner_uuid",
+			Attr:     "tail_uuid",
 			Operator: "=",
 			Operand:  cfg.ParentGroupUUID,
 		}},
 	}
-	results, err := GetAll(cfg.Client, "groups", params, &GroupList{})
+	results, err := GetAll(cfg.Client, "links", params, &LinkList{})
 	if err != nil {
 		return remoteGroups, groupNameToUUID, fmt.Errorf("error getting remote groups: %s", err)
 	}
 	for _, item := range results {
-		group := item.(arvados.Group)
+		var group arvados.Group
+		err = GetGroup(cfg, &group, item.(arvados.Link).HeadUUID)
+		if err != nil {
+			return remoteGroups, groupNameToUUID, fmt.Errorf("error getting remote group: %s", err)
+		}
 		// Group -> User filter
 		g2uFilter := arvados.ResourceListParams{
 			Filters: []arvados.Filter{{
diff --git a/tools/sync-groups/sync-groups_test.go b/tools/sync-groups/sync-groups_test.go
index 3ef360079..640e645d0 100644
--- a/tools/sync-groups/sync-groups_test.go
+++ b/tools/sync-groups/sync-groups_test.go
@@ -170,7 +170,7 @@ func RemoteGroupExists(cfg *ConfigParams, groupName string) (uuid string, err er
 		}, {
 			Attr:     "owner_uuid",
 			Operator: "=",
-			Operand:  cfg.ParentGroupUUID,
+			Operand:  cfg.SysUserUUID,
 		}, {
 			Attr:     "group_class",
 			Operator: "=",

commit fe054ed4c9019df6efcb93fdd0f55235048f6631
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 e16433c81..21e57e143 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 62cbe8a4373e5405ceb6fa4b3ef7acde2d85f9ea
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 7270f7cdf..01a31adb9 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -585,7 +585,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 cd1ff40c2..e5aa3ff49 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 b0f49dd22..51414b65d 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -423,8 +423,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 77dcd1d6de3782d2faa596dedac834045b925487
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 8afebfb79..7270f7cdf 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -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
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 d6fc818cd..e16433c81 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 b0cef9735..b0f49dd22 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -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)

commit 58201706fdf0cd305200d4bd5ff25922e281cdc2
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Fri May 29 00:04:12 2020 -0400

    16007: Validate group_class is set to 'project' or 'role'
    
    Fix tests.
    
    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 36814a316..d6fc818cd 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -17,6 +17,7 @@ class Group < ArvadosModel
   attribute :properties, :jsonbHash, default: {}
 
   validate :ensure_filesystem_compatible_name
+  validate :check_group_class
   before_create :assign_name
   after_create :after_ownership_change
   after_create :update_trash
@@ -44,6 +45,12 @@ class Group < ArvadosModel
     super if group_class == 'project'
   end
 
+  def check_group_class
+    if group_class != 'project' && group_class != 'role'
+      errors.add :group_class, "value must be one of 'project' or 'role', was '#{group_class}'"
+    end
+  end
+
   def update_trash
     if trash_at_changed? or owner_uuid_changed?
       # The group was added or removed from the trash.
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 22c999ecd..68377eb5f 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -107,6 +107,7 @@ bad_group_has_ownership_cycle_a:
   modified_at: 2014-05-03 18:50:08 -0400
   updated_at: 2014-05-03 18:50:08 -0400
   name: Owned by bad group b
+  group_class: project
 
 bad_group_has_ownership_cycle_b:
   uuid: zzzzz-j7d0g-0077nzts8c178lw
@@ -117,6 +118,7 @@ bad_group_has_ownership_cycle_b:
   modified_at: 2014-05-03 18:50:08 -0400
   updated_at: 2014-05-03 18:50:08 -0400
   name: Owned by bad group a
+  group_class: project
 
 anonymous_group:
   uuid: zzzzz-j7d0g-anonymouspublic
diff --git a/services/api/test/functional/application_controller_test.rb b/services/api/test/functional/application_controller_test.rb
index 175a8f71e..2cfa05444 100644
--- a/services/api/test/functional/application_controller_test.rb
+++ b/services/api/test/functional/application_controller_test.rb
@@ -100,7 +100,7 @@ class ApplicationControllerTest < ActionController::TestCase
         @controller = Arvados::V1::GroupsController.new
         authorize_with :active
         post :create, params: {
-          group: {},
+          group: {group_class: "project"},
           ensure_unique_name: boolparam
         }
         assert_response :success
@@ -113,7 +113,8 @@ class ApplicationControllerTest < ActionController::TestCase
         post :create, params: {
           group: {
             name: groups(:aproject).name,
-            owner_uuid: groups(:aproject).owner_uuid
+            owner_uuid: groups(:aproject).owner_uuid,
+            group_class: "project"
           },
           ensure_unique_name: boolparam
         }
diff --git a/services/api/test/functional/arvados/v1/repositories_controller_test.rb b/services/api/test/functional/arvados/v1/repositories_controller_test.rb
index cfcd917d6..84bd846c9 100644
--- a/services/api/test/functional/arvados/v1/repositories_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/repositories_controller_test.rb
@@ -150,7 +150,7 @@ class Arvados::V1::RepositoriesControllerTest < ActionController::TestCase
   test "get_all_permissions obeys group permissions" do
     act_as_user system_user do
       r = Repository.create!(name: 'admin/groupcanwrite', owner_uuid: users(:admin).uuid)
-      g = Group.create!(group_class: 'group', name: 'repo-writers')
+      g = Group.create!(group_class: 'role', name: 'repo-writers')
       u1 = users(:active)
       u2 = users(:spectator)
       Link.create!(tail_uuid: g.uuid, head_uuid: r.uuid, link_class: 'permission', name: 'can_manage')
@@ -158,7 +158,7 @@ class Arvados::V1::RepositoriesControllerTest < ActionController::TestCase
       Link.create!(tail_uuid: u2.uuid, head_uuid: g.uuid, link_class: 'permission', name: 'can_read')
 
       r = Repository.create!(name: 'admin/groupreadonly', owner_uuid: users(:admin).uuid)
-      g = Group.create!(group_class: 'group', name: 'repo-readers')
+      g = Group.create!(group_class: 'role', name: 'repo-readers')
       u1 = users(:active)
       u2 = users(:spectator)
       Link.create!(tail_uuid: g.uuid, head_uuid: r.uuid, link_class: 'permission', name: 'can_read')
diff --git a/services/api/test/functional/arvados/v1/users_controller_test.rb b/services/api/test/functional/arvados/v1/users_controller_test.rb
index 817a1c9ef..0ce9f1137 100644
--- a/services/api/test/functional/arvados/v1/users_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/users_controller_test.rb
@@ -660,7 +660,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
 
   test "non-admin user gets only safe attributes from users#show" do
     g = act_as_system_user do
-      create :group
+      create :group, group_class: "role"
     end
     users = create_list :active_user, 2, join_groups: [g]
     token = create :token, user: users[0]
@@ -672,7 +672,7 @@ class Arvados::V1::UsersControllerTest < ActionController::TestCase
   [2, 4].each do |limit|
     test "non-admin user can limit index to #{limit}" do
       g = act_as_system_user do
-        create :group
+        create :group, group_class: "role"
       end
       users = create_list :active_user, 4, join_groups: [g]
       token = create :token, user: users[0]
diff --git a/services/api/test/integration/groups_test.rb b/services/api/test/integration/groups_test.rb
index 445670a3d..702176127 100644
--- a/services/api/test/integration/groups_test.rb
+++ b/services/api/test/integration/groups_test.rb
@@ -206,7 +206,8 @@ class NonTransactionalGroupsTest < ActionDispatch::IntegrationTest
     post "/arvados/v1/groups",
       params: {
         group: {
-          name: name
+          name: name,
+          group_class: "project"
         },
         async: true
       },
diff --git a/services/api/test/performance/permission_test.rb b/services/api/test/performance/permission_test.rb
index d0e6413b1..e5d62cf4c 100644
--- a/services/api/test/performance/permission_test.rb
+++ b/services/api/test/performance/permission_test.rb
@@ -24,7 +24,7 @@ class PermissionPerfTest < ActionDispatch::IntegrationTest
     act_as_system_user do
       puts("Time spent creating records:", Benchmark.measure do
              ActiveRecord::Base.transaction do
-               root = Group.create!(owner_uuid: users(:permission_perftest).uuid)
+               root = Group.create!(owner_uuid: users(:permission_perftest).uuid, group_class: "project")
                n += 1
                a = create_eight root.uuid
                n += 8
diff --git a/services/api/test/unit/arvados_model_test.rb b/services/api/test/unit/arvados_model_test.rb
index d447c76c6..c1db8c8b5 100644
--- a/services/api/test/unit/arvados_model_test.rb
+++ b/services/api/test/unit/arvados_model_test.rb
@@ -97,7 +97,7 @@ class ArvadosModelTest < ActiveSupport::TestCase
     while longstring.length < 2**16
       longstring = longstring + longstring
     end
-    g = Group.create! name: 'Has a long description', description: longstring
+    g = Group.create! name: 'Has a long description', description: longstring, group_class: "project"
     g = Group.find_by_uuid g.uuid
     assert_equal g.description, longstring
   end
@@ -248,7 +248,7 @@ class ArvadosModelTest < ActiveSupport::TestCase
 
   test 'create and retrieve using created_at time' do
     set_user_from_auth :active
-    group = Group.create! name: 'test create and retrieve group'
+    group = Group.create! name: 'test create and retrieve group', group_class: "project"
     assert group.valid?, "group is not valid"
 
     results = Group.where(created_at: group.created_at)
@@ -258,7 +258,7 @@ class ArvadosModelTest < ActiveSupport::TestCase
 
   test 'create and update twice and expect different update times' do
     set_user_from_auth :active
-    group = Group.create! name: 'test create and retrieve group'
+    group = Group.create! name: 'test create and retrieve group', group_class: "project"
     assert group.valid?, "group is not valid"
 
     # update 1
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 24d7333ab..f7a531fc0 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -31,8 +31,8 @@ class GroupTest < ActiveSupport::TestCase
   test "cannot create a new ownership cycle" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar")
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", group_class: "project")
 
     g_foo.owner_uuid = g_bar.uuid
     assert g_foo.save, lambda { g_foo.errors.messages }
@@ -45,7 +45,7 @@ class GroupTest < ActiveSupport::TestCase
   test "cannot create a single-object ownership cycle" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
+    g_foo = Group.create!(name: "foo", group_class: "project")
     assert g_foo.save
 
     # Ensure I have permission to manage this group even when its owner changes
@@ -63,7 +63,7 @@ class GroupTest < ActiveSupport::TestCase
   test "trash group hides contents" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
+    g_foo = Group.create!(name: "foo", group_class: "project")
     col = Collection.create!(owner_uuid: g_foo.uuid)
 
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
@@ -77,9 +77,9 @@ class GroupTest < ActiveSupport::TestCase
   test "trash group" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
-    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid, group_class: "project")
+    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid, group_class: "project")
 
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
@@ -98,9 +98,9 @@ class GroupTest < ActiveSupport::TestCase
   test "trash subgroup" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
-    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid, group_class: "project")
+    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid, group_class: "project")
 
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
@@ -118,9 +118,9 @@ class GroupTest < ActiveSupport::TestCase
   test "trash subsubgroup" do
     set_user_from_auth :active_trustedclient
 
-    g_foo = Group.create!(name: "foo")
-    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
-    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
+    g_foo = Group.create!(name: "foo", group_class: "project")
+    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid, group_class: "project")
+    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid, group_class: "project")
 
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
@@ -168,7 +168,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")
+    g_foo = Group.create!(name: "foo", group_class: "project")
     u_bar = User.create!(first_name: "bar")
 
     assert Group.readable_by(users(:admin)).where(uuid: g_foo.uuid).any?
@@ -237,7 +237,7 @@ class GroupTest < ActiveSupport::TestCase
     set_user_from_auth :active
     ["", "{SOLIDUS}"].each do |subst|
       Rails.configuration.Collections.ForwardSlashNameSubstitution = subst
-      g = Group.create
+      g = Group.create group_class: "project"
       [[nil, true],
        ["", true],
        [".", false],
diff --git a/services/api/test/unit/owner_test.rb b/services/api/test/unit/owner_test.rb
index ca02e2db5..e356f4d9f 100644
--- a/services/api/test/unit/owner_test.rb
+++ b/services/api/test/unit/owner_test.rb
@@ -21,7 +21,11 @@ class OwnerTest < ActiveSupport::TestCase
   Group.all
   [User, Group].each do |o_class|
     test "create object with legit #{o_class} owner" do
-      o = o_class.create!
+      if o_class == Group
+        o = o_class.create! group_class: "project"
+      else
+        o = o_class.create!
+      end
       i = Specimen.create(owner_uuid: o.uuid)
       assert i.valid?, "new item should pass validation"
       assert i.uuid, "new item should have an ID"
@@ -44,9 +48,19 @@ class OwnerTest < ActiveSupport::TestCase
 
     [User, Group].each do |new_o_class|
       test "change owner from legit #{o_class} to legit #{new_o_class} owner" do
-        o = o_class.create!
+        o = if o_class == Group
+              o_class.create! group_class: "project"
+            else
+              o_class.create!
+            end
         i = Specimen.create!(owner_uuid: o.uuid)
-        new_o = new_o_class.create!
+
+        new_o = if new_o_class == Group
+              new_o_class.create! group_class: "project"
+            else
+              new_o_class.create!
+            end
+
         assert(Specimen.where(uuid: i.uuid).any?,
                "new item should really be in DB")
         assert(i.update_attributes(owner_uuid: new_o.uuid),
@@ -55,7 +69,11 @@ class OwnerTest < ActiveSupport::TestCase
     end
 
     test "delete #{o_class} that owns nothing" do
-      o = o_class.create!
+      if o_class == Group
+        o = o_class.create! group_class: "project"
+      else
+        o = o_class.create!
+      end
       assert(o_class.where(uuid: o.uuid).any?,
              "new #{o_class} should really be in DB")
       assert(o.destroy, "should delete #{o_class} that owns nothing")
@@ -65,7 +83,11 @@ class OwnerTest < ActiveSupport::TestCase
 
     test "change uuid of #{o_class} that owns nothing" do
       # (we're relying on our admin credentials here)
-      o = o_class.create!
+      if o_class == Group
+        o = o_class.create! group_class: "project"
+      else
+        o = o_class.create!
+      end
       assert(o_class.where(uuid: o.uuid).any?,
              "new #{o_class} should really be in DB")
       old_uuid = o.uuid
diff --git a/services/api/test/unit/permission_test.rb b/services/api/test/unit/permission_test.rb
index d9383cf8e..b0cef9735 100644
--- a/services/api/test/unit/permission_test.rb
+++ b/services/api/test/unit/permission_test.rb
@@ -153,6 +153,7 @@ class PermissionTest < ActiveSupport::TestCase
     set_user_from_auth :admin
 
     owner_grp = Group.create!(owner_uuid: users(:active).uuid, group_class: "role")
+
     sp_grp = Group.create!(group_class: "project")
 
     Link.create!(link_class: 'permission',
@@ -227,7 +228,7 @@ class PermissionTest < ActiveSupport::TestCase
     # anyone any additional permissions.)
     g = nil
     act_as_user manager do
-      g = create :group, name: "NoBigSecret Lab"
+      g = create :group, name: "NoBigSecret Lab", group_class: "role"
       assert_empty(User.readable_by(manager).where(uuid: minion.uuid),
                    "saw a user I shouldn't see")
       assert_raises(ArvadosModel::PermissionDeniedError,
@@ -323,7 +324,7 @@ class PermissionTest < ActiveSupport::TestCase
                      "#{a.first_name} should not be able to see 'b' in the user list")
 
     act_as_system_user do
-      g = create :group
+      g = create :group, group_class: "role"
       [a,b].each do |u|
         create(:permission_link,
                name: 'can_read', tail_uuid: u.uuid, head_uuid: g.uuid)

commit 6f514b3e6aa21afddaa527bf852cff3a5801aa19
Author: Peter Amstutz <peter.amstutz at curii.com>
Date:   Thu Jun 11 13:12:04 2020 -0400

    16007: Add REVOKE_PERM and CAN_MANAGE_PERM constants
    
    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 485205f1e..36814a316 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -80,13 +80,13 @@ on conflict (group_uuid) do update set trash_at=EXCLUDED.trash_at;
   def before_ownership_change
     if owner_uuid_changed? and !self.owner_uuid_was.nil?
       MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
-      update_permissions self.owner_uuid_was, self.uuid, 0
+      update_permissions self.owner_uuid_was, self.uuid, REVOKE_PERM
     end
   end
 
   def after_ownership_change
     if owner_uuid_changed?
-      update_permissions self.owner_uuid, self.uuid, 3
+      update_permissions self.owner_uuid, self.uuid, CAN_MANAGE_PERM
     end
   end
 
diff --git a/services/api/app/models/link.rb b/services/api/app/models/link.rb
index da4ca6c87..cd1ff40c2 100644
--- a/services/api/app/models/link.rb
+++ b/services/api/app/models/link.rb
@@ -80,7 +80,7 @@ class Link < ArvadosModel
 
   def clear_permissions
     if self.link_class == 'permission'
-      update_permissions tail_uuid, head_uuid, 0
+      update_permissions tail_uuid, head_uuid, REVOKE_PERM
     end
   end
 
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index a2922cb7b..d65cfb9c4 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -146,18 +146,18 @@ SELECT 1 FROM #{PERMISSION_VIEW}
   def before_ownership_change
     if owner_uuid_changed? and !self.owner_uuid_was.nil?
       MaterializedPermission.where(user_uuid: owner_uuid_was, target_uuid: uuid).delete_all
-      update_permissions self.owner_uuid_was, self.uuid, 0
+      update_permissions self.owner_uuid_was, self.uuid, REVOKE_PERM
     end
   end
 
   def after_ownership_change
     if owner_uuid_changed?
-      update_permissions self.owner_uuid, self.uuid, 3
+      update_permissions self.owner_uuid, self.uuid, CAN_MANAGE_PERM
     end
   end
 
   def clear_permissions
-    update_permissions self.owner_uuid, self.uuid, 0
+    update_permissions self.owner_uuid, self.uuid, REVOKE_PERM
     MaterializedPermission.where("user_uuid = ? or target_uuid = ?", uuid, uuid).delete_all
   end
 
@@ -447,11 +447,11 @@ update #{PERMISSION_VIEW} set target_uuid=$1 where target_uuid = $2
         update_attributes!(redirect_to_user_uuid: new_user.uuid, username: nil)
       end
       skip_check_permissions_against_full_refresh do
-        update_permissions self.owner_uuid, self.uuid, 3
-        update_permissions self.uuid, self.uuid, 3
-        update_permissions new_user.owner_uuid, new_user.uuid, 3
+        update_permissions self.owner_uuid, self.uuid, CAN_MANAGE_PERM
+        update_permissions self.uuid, self.uuid, CAN_MANAGE_PERM
+        update_permissions new_user.owner_uuid, new_user.uuid, CAN_MANAGE_PERM
       end
-      update_permissions new_user.uuid, new_user.uuid, 3
+      update_permissions new_user.uuid, new_user.uuid, CAN_MANAGE_PERM
     end
   end
 
diff --git a/services/api/lib/update_permissions.rb b/services/api/lib/update_permissions.rb
index 4d3986a4b..1d9c1006b 100644
--- a/services/api/lib/update_permissions.rb
+++ b/services/api/lib/update_permissions.rb
@@ -4,6 +4,9 @@
 
 require '20200501150153_permission_table_constants'
 
+REVOKE_PERM = 0
+CAN_MANAGE_PERM = 3
+
 def update_permissions perm_origin_uuid, starting_uuid, perm_level
   #
   # Update a subset of the permission table affected by adding or

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list