20943: role groups should now handle being trashed

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <peter.amstutz@curii.com>

diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 11212e1b6910f42916251b27f0fcfe4a549bfadd..36839a1da0f55109d9a4e653be797f9231f1742b 100644 (file)
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -11,8 +11,6 @@ class Arvados::V1::GroupsController < ApplicationController
   skip_before_action :find_object_by_uuid, only: :shared
   skip_before_action :render_404_if_no_object, only: :shared
 
-  TRASHABLE_CLASSES = ['project']
-
   def self._index_requires_parameters
     (super rescue {}).
       merge({
@@ -101,15 +99,6 @@ class Arvados::V1::GroupsController < ApplicationController
     end
   end
 
-  def destroy
-    if !TRASHABLE_CLASSES.include?(@object.group_class)
-      @object.destroy
-      show
-    else
-      super # Calls destroy from TrashableController module
-    end
-  end
-
   def render_404_if_no_object
     if params[:action] == 'contents'
       if !params[:uuid]

diff --git a/services/api/app/controllers/sys_controller.rb b/services/api/app/controllers/sys_controller.rb
index 7d20cf77fdcd555de70d54fa931767337b7a586b..ae6e8d23b47a9fdaccbed09df70c8be410d120e2 100644 (file)
--- a/services/api/app/controllers/sys_controller.rb
+++ b/services/api/app/controllers/sys_controller.rb
@@ -19,16 +19,25 @@ class SysController < ApplicationController
         in_batches(of: 15).
         update_all('is_trashed = true')
 
-      # Sweep trashed projects and their contents (as well as role
-      # groups that were trashed before #18340 when that was
-      # disallowed)
+      # Want to make sure the #update_trash hook on the Group class
+      # runs.  It does a couple of important things:
+      #
+      # - For projects, puts all the subprojects in the trashed_groups table.
+      #
+      # - For role groups, starting from #20943, when a role group
+      # enters the trash it keeps its members but loses its outbound
+      # permissions.
       Group.
-        where('delete_at is not null and delete_at < statement_timestamp()').each do |project|
-          delete_project_and_contents(project.uuid)
+        where("is_trashed = false and trash_at < statement_timestamp()").each do |grp|
+        grp.is_trashed = true
+        grp.save
       end
+
+      # Sweep groups and their contents that are ready to be deleted
       Group.
-        where('is_trashed = false and trash_at < statement_timestamp()').
-        update_all('is_trashed = true')
+        where('delete_at is not null and delete_at < statement_timestamp()').each do |group|
+          delete_project_and_contents(group.uuid)
+      end
 
       # Sweep expired tokens
       ActiveRecord::Base.connection.execute("DELETE from api_client_authorizations where expires_at <= statement_timestamp()")

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index d4c81fe9d1d9cf2c558d644bf45bf2f495dc9ef6..0a75de0261c9a08c14b7b3b4c96e6982bea449f5 100644 (file)
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -171,10 +171,17 @@ with temptable as (select * from project_subtree_with_trash_at($1, LEAST($2, $3)
       [self.uuid,
        TrashedGroup.find_by_group_uuid(self.owner_uuid).andand.trash_at,
        self.trash_at])
+
     if frozen_descendants.any?
       raise ArgumentError.new("cannot trash project containing frozen project #{frozen_descendants[0]["uuid"]}")
     end
 
+    if self.trash_at and self.group_class == 'role'
+      # if this is a role group that is now in the trash, it loses all
+      # of its outgoing permissions.
+      Link.where(link_class: 'permission', tail_uuid: self.uuid).destroy_all
+    end
+
     ActiveRecord::Base.connection.exec_query(%{
 with temptable as (select * from project_subtree_with_trash_at($1, LEAST($2, $3)::timestamp)),
 

diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 9034ac6ee7d2dd72928388b51b4461bff2814af8..8a5ef310ce8469c86495186d9333be77af305e8b 100644 (file)
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -434,3 +434,13 @@ trashed_on_next_sweep:
   delete_at: 2038-03-01T00:00:00Z
   is_trashed: false
   modified_at: 2001-01-01T00:00:00Z
+
+trashed_role_on_next_sweep:
+  uuid: zzzzz-j7d0g-soontobetrashd2
+  owner_uuid: zzzzz-tpzed-000000000000000
+  name: soon to be trashed role group
+  group_class: role
+  trash_at: 2001-01-01T00:00:00Z
+  delete_at: 2038-03-01T00:00:00Z
+  is_trashed: false
+  modified_at: 2001-01-01T00:00:00Z

diff --git a/services/api/test/fixtures/links.yml b/services/api/test/fixtures/links.yml
index 61ad60451d088f15a554cf8661b28ed0f977def8..57824863bdbb2e36ac5f4409578d47ea966b2e53 100644 (file)
--- a/services/api/test/fixtures/links.yml
+++ b/services/api/test/fixtures/links.yml
@@ -980,3 +980,17 @@ future_project_user_member_of_all_users_group:
   name: can_write
   head_uuid: zzzzz-j7d0g-fffffffffffffff
   properties: {}
+
+foo_file_readable_by_soon_to_be_trashed_role:
+  uuid: zzzzz-o0j2j-5s8ry7sn7bwxb7w
+  owner_uuid: zzzzz-tpzed-000000000000000
+  created_at: 2014-01-24 20:42:26 -0800
+  modified_by_client_uuid: zzzzz-ozdt8-000000000000000
+  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-soontobetrashd2
+  link_class: permission
+  name: can_read
+  head_uuid: zzzzz-4zz18-znfnqtbbv4spc3w
+  properties: {}

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 52ed140bae06f248eed340d5c4a430b743b04144..c01fa97836f74e13da36e50159c07056bfb5b33f 100644 (file)
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -575,8 +575,8 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
             format: :json,
           }
       assert_response :success
-      # Should not be trashed
-      assert_nil Group.find_by_uuid(groups(grp).uuid)
+      # Should be trashed
+      assert Group.find_by_uuid(groups(grp).uuid).is_trashed
     end
   end
 

diff --git a/services/api/test/functional/sys_controller_test.rb b/services/api/test/functional/sys_controller_test.rb
index ab304c1c7ce94dbd07b3e46981a2957e04093a95..f89d58b988afb7aeef8f431f7ee4b10da997bff7 100644 (file)
--- a/services/api/test/functional/sys_controller_test.rb
+++ b/services/api/test/functional/sys_controller_test.rb
@@ -91,6 +91,17 @@ class SysControllerTest < ActionController::TestCase
     assert_not_empty Group.where('uuid=? and is_trashed=true', p.uuid)
   end
 
+  test "trash_sweep - move roles groups to trash" do
+    p = groups(:trashed_role_on_next_sweep)
+    assert_empty Group.where('uuid=? and is_trashed=true', p.uuid)
+    assert_not_empty Link.where(uuid: links(:foo_file_readable_by_soon_to_be_trashed_role).uuid)
+    authorize_with :admin
+    post :trash_sweep
+    assert_response :success
+    assert_not_empty Group.where('uuid=? and is_trashed=true', p.uuid)
+    assert_empty Link.where(uuid: links(:foo_file_readable_by_soon_to_be_trashed_role).uuid)
+  end
+
   test "trash_sweep - delete projects and their contents" do
     g_foo = groups(:trashed_project)
     g_bar = groups(:trashed_subproject)

diff --git a/services/api/test/integration/permissions_test.rb b/services/api/test/integration/permissions_test.rb
index 9636a82011ffd1e7d0559fa47d2ec5fa41dbc518..2251bf40c8af23767bb499c5a66e9b9a189b0622 100644 (file)
--- a/services/api/test/integration/permissions_test.rb
+++ b/services/api/test/integration/permissions_test.rb
@@ -273,6 +273,120 @@ class PermissionsTest < ActionDispatch::IntegrationTest
     assert_response 404
   end
 
+  test "adding can_read links from group to collection, user to group, then trash group" do
+    # try to read collection as spectator
+    get "/arvados/v1/collections/#{collections(:foo_file).uuid}",
+      params: {:format => :json},
+      headers: auth(:spectator)
+    assert_response 404
+
+    # add permission for group to read collection
+    post "/arvados/v1/links",
+      params: {
+        :format => :json,
+        :link => {
+          tail_uuid: groups(:private_role).uuid,
+          link_class: 'permission',
+          name: 'can_read',
+          head_uuid: collections(:foo_file).uuid,
+          properties: {}
+        }
+      },
+      headers: auth(:admin)
+    assert_response :success
+
+    # try to read collection as spectator
+    get "/arvados/v1/collections/#{collections(:foo_file).uuid}",
+      params: {:format => :json},
+      headers: auth(:spectator)
+    assert_response 404
+
+    # add permission for spectator to read group
+    post "/arvados/v1/links",
+      params: {
+        :format => :json,
+        :link => {
+          tail_uuid: users(:spectator).uuid,
+          link_class: 'permission',
+          name: 'can_read',
+          head_uuid: groups(:private_role).uuid,
+          properties: {}
+        }
+      },
+      headers: auth(:admin)
+    u = json_response['uuid']
+    assert_response :success
+
+    # try to read collection as spectator
+    get "/arvados/v1/collections/#{collections(:foo_file).uuid}",
+      params: {:format => :json},
+      headers: auth(:spectator)
+    assert_response :success
+
+    # put the group in the trash, this should keep the group members
+    # but delete the permissions.
+    post "/arvados/v1/groups/#{groups(:private_role).uuid}/trash",
+      params: {:format => :json},
+      headers: auth(:admin)
+    assert_response :success
+
+    # try to read collection as spectator, should fail now
+    get "/arvados/v1/collections/#{collections(:foo_file).uuid}",
+      params: {:format => :json},
+      headers: auth(:spectator)
+    assert_response 404
+
+    # should not be able to grant permission to a trashed group
+    post "/arvados/v1/links",
+      params: {
+        :format => :json,
+        :link => {
+          tail_uuid: groups(:private_role).uuid,
+          link_class: 'permission',
+          name: 'can_read',
+          head_uuid: collections(:foo_file).uuid,
+          properties: {}
+        }
+      },
+      headers: auth(:admin)
+    assert_response 422
+
+    # take the group out of the trash
+    post "/arvados/v1/groups/#{groups(:private_role).uuid}/untrash",
+      params: {:format => :json},
+      headers: auth(:admin)
+    assert_response :success
+
+    # when a role group is untrashed the permissions don't
+    # automatically come back
+    get "/arvados/v1/collections/#{collections(:foo_file).uuid}",
+      params: {:format => :json},
+      headers: auth(:spectator)
+    assert_response 404
+
+    # re-add permission for group to read collection
+    post "/arvados/v1/links",
+      params: {
+        :format => :json,
+        :link => {
+          tail_uuid: groups(:private_role).uuid,
+          link_class: 'permission',
+          name: 'can_read',
+          head_uuid: collections(:foo_file).uuid,
+          properties: {}
+        }
+      },
+      headers: auth(:admin)
+    assert_response :success
+
+    # since spectator is still be a member of the group, it should be
+    # able to read foo file now.
+    get "/arvados/v1/collections/#{collections(:foo_file).uuid}",
+      params: {:format => :json},
+      headers: auth(:spectator)
+    assert_response :success
+  end
+
   test "read-only group-admin cannot modify administered user" do
     put "/arvados/v1/users/#{users(:active).uuid}",
       params: {