12032: Bug fix so include_trash still respects permissions.

Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz@veritasgenetics.com>

diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 0312d956a6168d957f50136d4c03df691f88279c..3803d37691132782e927e6e32cbb5fbee80657c7 100644 (file)
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -28,7 +28,7 @@ class Arvados::V1::CollectionsController < ApplicationController
 
   def find_objects_for_index
     if params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name)
-      @objects = Collection.unscoped.readable_by(*@read_users)
+      @objects = Collection.readable_by(*@read_users, {include_trash: true, query_on: Collection.unscoped})
     end
     super
   end

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 4a4f7ec9819b4e7ba662788d27c56bf0801b7438..d65697057f6a85fc1393b593e993225a024669fb 100644 (file)
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -282,29 +282,37 @@ class ArvadosModel < ActiveRecord::Base
       # Can read object (evidently a group or user) whose UUID is listed
       # explicitly in user_uuids.
       sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
-      trashed_check = if include_trash then "EXISTS" else "0 IN " end
+      if include_trash
+        not_trashed = ""
+        not_null = "IS NOT NULL"
+      else
+        not_trashed = "0 IN"
+        not_null = ""
+      end
+
       perm_check = "perm_level >= 1"
 
       if self.column_names.include? "owner_uuid"
         # to be readable:
         #   row(s) exists granting readable permission to uuid or owner, and none are trashed
-        sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+        sql_conds += ["#{not_trashed} (SELECT MAX(trashed) FROM permission_view "+
                       "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
-                      "(#{sql_table}.uuid = target_uuid OR (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL)))"]
+                      "(#{sql_table}.uuid = target_uuid OR (#{sql_table}.owner_uuid = target_uuid "+
+                      "AND target_owner_uuid IS NOT NULL))) #{not_null}"]
         #   reader is owner, and item is not trashed
-        not_trashed_check = if include_trash then
+        not_trashed_clause = if include_trash then
                               ""
                             else
                               "AND 1 NOT IN (SELECT MAX(trashed) FROM permission_view "+
                                 "WHERE #{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid)"
                             end
-        sql_conds += ["(#{sql_table}.owner_uuid IN (:user_uuids) #{not_trashed_check})"]
+        sql_conds += ["(#{sql_table}.owner_uuid IN (:user_uuids) #{not_trashed_clause})"]
       else
         # to be readable:
         #  * a non-trash row exists with readable permission uuid
-        sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+        sql_conds += ["#{not_trashed} (SELECT MAX(trashed) FROM permission_view "+
                                 "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
-                                "#{sql_table}.uuid = target_uuid) "]
+                                "#{sql_table}.uuid = target_uuid) #{not_null}"]
       end
 
       if sql_table == "links"

diff --git a/services/api/test/functional/arvados/v1/collections_controller_test.rb b/services/api/test/functional/arvados/v1/collections_controller_test.rb
index 4cf8778e6aede93043d574634f1ead00df952c03..47f0887bd19ce5f4446088fa2290afda7fca8e40 100644 (file)
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -1112,7 +1112,7 @@ EOS
 
   test 'cannot index collection in trashed subproject' do
     authorize_with :active
-    get :index
+    get :index, { limit: 1000 }
     assert_response :success
     item_uuids = json_response['items'].map do |item|
       item['uuid']
@@ -1123,7 +1123,20 @@ EOS
   test 'can index collection in untrashed subproject' do
     authorize_with :active
     Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
-    get :index
+    get :index, { limit: 1000 }
+    assert_response :success
+    item_uuids = json_response['items'].map do |item|
+      item['uuid']
+    end
+    assert_includes(item_uuids, collections(:collection_in_trashed_subproject).uuid)
+  end
+
+  test 'can index trashed subproject collection with include_trash' do
+    authorize_with :active
+    get :index, {
+          include_trash: true,
+          limit: 1000
+        }
     assert_response :success
     item_uuids = json_response['items'].map do |item|
       item['uuid']

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 5db8475541da8874377959e608fb6a6585b08336..368781997e49babaabe85b3f2296d77b6a2a7f48 100644 (file)
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -689,5 +689,21 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
       assert_response :success
       assert_match /^trashed subproject 3 \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
     end
+
+    test "move trashed subproject to new owner #{auth}" do
+      authorize_with auth
+      put :update, {
+            id: groups(:trashed_subproject).uuid,
+            group: {
+              owner_uuid: users(:active).uuid
+            },
+            include_trashed: true,
+            format: :json,
+          }
+      # Currently fails but might want to change it in the future
+      # so leave the test here.
+      assert_response 404
+    end
+
   end
 end