[ARVADOS] created: d901415a244f16562fdcb87b2f2ded40cc369c45

Git user git at public.curoverse.com
Thu Aug 31 10:42:38 EDT 2017


        at  d901415a244f16562fdcb87b2f2ded40cc369c45 (commit)


commit d901415a244f16562fdcb87b2f2ded40cc369c45
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Aug 31 10:37:13 2017 -0400

    12032: Refactor readable_by to minimize subqueries.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index 46b96a9..d09283d 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -180,7 +180,7 @@ class ApplicationController < ActionController::Base
   end
 
   def find_objects_for_index
-    @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name))})
+    @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || 'untrash' == action_name)})
     apply_where_limit_order_params
   end
 
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 9e0cdf8..8e195ff 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -163,13 +163,14 @@ class Arvados::V1::GroupsController < ApplicationController
         end
       end.compact
 
-      if klass == Collection and params[:include_trash]
-        @objects = klass.unscoped.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
-          order(request_order).where(where_conds)
-      else
-        @objects = klass.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
-          order(request_order).where(where_conds)
-      end
+      query_on = if klass == Collection and params[:include_trash]
+                   klass.unscoped
+                 else
+                   klass
+                 end
+      @objects = query_on.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
+                 order(request_order).where(where_conds)
+
       klass_limit = limit_all - all_objects.count
       @limit = klass_limit
       apply_where_limit_order_params klass
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 518d4c8..4a4f7ec 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -265,52 +265,46 @@ class ArvadosModel < ActiveRecord::Base
     # Check if any of the users are admin.
     if users_list.select { |u| u.is_admin }.any?
       if !include_trash
-        # exclude rows that are trashed.
+        # exclude rows that are explicitly trashed.
         if self.column_names.include? "owner_uuid"
-          sql_conds += ["NOT EXISTS(SELECT target_uuid
+          sql_conds += ["NOT EXISTS(SELECT 1
                   FROM permission_view
                   WHERE trashed = 1 AND
                   (#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"]
         else
-          sql_conds += ["NOT EXISTS(SELECT target_uuid
+          sql_conds += ["NOT EXISTS(SELECT 1
                   FROM permission_view
                   WHERE trashed = 1 AND
                   (#{sql_table}.uuid = target_uuid))"]
         end
       end
     else
-      trash_clause = if !include_trash then "trashed = 0 AND" else "" end
-
       # Can read object (evidently a group or user) whose UUID is listed
       # explicitly in user_uuids.
       sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
-
-      direct_permission_check = "EXISTS(SELECT 1 FROM permission_view
-                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
-                  (#{sql_table}.uuid = target_uuid))"
+      trashed_check = if include_trash then "EXISTS" else "0 IN " end
+      perm_check = "perm_level >= 1"
 
       if self.column_names.include? "owner_uuid"
-        # if an explicit permission row exists for the uuid in question, apply
-        # the "direct_permission_check"
-        # if not, check for permission to read the owner instead
-        sql_conds += ["CASE
-                  WHEN EXISTS(select 1 FROM permission_view where target_uuid = #{sql_table}.uuid)
-                  THEN #{direct_permission_check}
-                  ELSE EXISTS(SELECT 1 FROM permission_view
-                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
-                  (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL))
-                  END"]
-        # Can also read if one of the users is the owner of the object.
-        trash_clause = if !include_trash
-                         "1 NOT IN (SELECT trashed
-                             FROM permission_view
-                             WHERE #{sql_table}.uuid = target_uuid) AND"
-                       else
-                         ""
-                       end
-        sql_conds += ["(#{trash_clause} #{sql_table}.owner_uuid IN (:user_uuids))"]
+        # 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 "+
+                      "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)))"]
+        #   reader is owner, and item is not trashed
+        not_trashed_check = 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})"]
       else
-        sql_conds += [direct_permission_check]
+        # to be readable:
+        #  * a non-trash row exists with readable permission uuid
+        sql_conds += ["#{trashed_check}(SELECT MAX(trashed) FROM permission_view "+
+                                "WHERE user_uuid IN (:user_uuids) AND #{perm_check} AND "+
+                                "#{sql_table}.uuid = target_uuid) "]
       end
 
       if sql_table == "links"
diff --git a/services/api/test/integration/groups_test.rb b/services/api/test/integration/groups_test.rb
index 6226ffd..c4ab3cf 100644
--- a/services/api/test/integration/groups_test.rb
+++ b/services/api/test/integration/groups_test.rb
@@ -99,7 +99,8 @@ class GroupsTest < ActionDispatch::IntegrationTest
   test "group contents with include trash collections" do
     get "/arvados/v1/groups/contents", {
       include_trash: "true",
-      filters: [["uuid", "is_a", "arvados#collection"]].to_json
+      filters: [["uuid", "is_a", "arvados#collection"]].to_json,
+      limit: 1000
     }, auth(:active)
     assert_response 200
 
@@ -108,4 +109,17 @@ class GroupsTest < ActionDispatch::IntegrationTest
     assert_includes coll_uuids, collections(:foo_collection_in_aproject).uuid
     assert_includes coll_uuids, collections(:expired_collection).uuid
   end
+
+  test "group contents without trash collections" do
+    get "/arvados/v1/groups/contents", {
+      filters: [["uuid", "is_a", "arvados#collection"]].to_json,
+      limit: 1000
+    }, auth(:active)
+    assert_response 200
+
+    coll_uuids = []
+    json_response['items'].each { |c| coll_uuids << c['uuid'] }
+    assert_includes coll_uuids, collections(:foo_collection_in_aproject).uuid
+    assert_not_includes coll_uuids, collections(:expired_collection).uuid
+  end
 end

commit 5b0057c61154b1f46ed83d6f392eae21c2cfa8f7
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Wed Aug 30 13:13:29 2017 -0400

    12032: Log.readable_by uses permission_view
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index 7f2d3ef..99d0e28 100644
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -67,17 +67,14 @@ class Log < ArvadosModel
       return self
     end
     user_uuids = users_list.map { |u| u.uuid }
-    uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
-    uuid_list.uniq!
-    permitted = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (:uuids))"
+
+    User.install_view('permission')
+
     joins("LEFT JOIN container_requests ON container_requests.container_uuid=logs.object_uuid").
-      where("logs.object_uuid IN #{permitted} OR "+
-            "container_requests.uuid IN (:uuids) OR "+
-            "container_requests.owner_uuid IN (:uuids) OR "+
-            "logs.object_uuid IN (:uuids) OR "+
-            "logs.owner_uuid IN (:uuids) OR "+
-            "logs.object_owner_uuid IN (:uuids)",
-            uuids: uuid_list)
+      where("EXISTS(SELECT target_uuid FROM permission_view "+
+            "WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND "+
+            "target_uuid IN (container_requests.uuid, container_requests.owner_uuid, logs.object_uuid, logs.owner_uuid, logs.object_owner_uuid))",
+            user_uuids: user_uuids)
   end
 
   protected
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index 0e2db76..9f053c0 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -152,8 +152,8 @@ class User < ArvadosModel
 
   # Return a hash of {user_uuid: group_perms}
   def self.all_group_permissions
-    install_view('permission')
     all_perms = {}
+    User.install_view('permission')
     ActiveRecord::Base.connection.
       exec_query('SELECT user_uuid, target_owner_uuid, perm_level, trashed
                   FROM permission_view
@@ -171,9 +171,8 @@ class User < ArvadosModel
   # and perm_hash[:write] are true if this user can read and write
   # objects owned by group_uuid.
   def calculate_group_permissions
-    self.class.install_view('permission')
-
     group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
+    User.install_view('permission')
     ActiveRecord::Base.connection.
       exec_query('SELECT target_owner_uuid, perm_level, trashed
                   FROM permission_view

commit 5e161d86721d036761f77c45a25b9d25bacc7be3
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Tue Aug 29 09:08:19 2017 -0400

    12032: Controller support for group trash.
    
    Conttroller support and testing for contents, index, show, trash, untrash,
    containers.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/controllers/application_controller.rb b/services/api/app/controllers/application_controller.rb
index a3d8f08..46b96a9 100644
--- a/services/api/app/controllers/application_controller.rb
+++ b/services/api/app/controllers/application_controller.rb
@@ -180,7 +180,7 @@ class ApplicationController < ActionController::Base
   end
 
   def find_objects_for_index
-    @objects ||= model_class.readable_by(*@read_users)
+    @objects ||= model_class.readable_by(*@read_users, {:include_trash => (params[:include_trash] || ['destroy', 'trash', 'untrash'].include?(action_name))})
     apply_where_limit_order_params
   end
 
diff --git a/services/api/app/controllers/arvados/v1/collections_controller.rb b/services/api/app/controllers/arvados/v1/collections_controller.rb
index 87d88fe..0312d95 100644
--- a/services/api/app/controllers/arvados/v1/collections_controller.rb
+++ b/services/api/app/controllers/arvados/v1/collections_controller.rb
@@ -3,9 +3,11 @@
 # SPDX-License-Identifier: AGPL-3.0
 
 require "arvados/keep"
+require "trashable"
 
 class Arvados::V1::CollectionsController < ApplicationController
   include DbCurrentTime
+  include TrashableController
 
   def self._index_requires_parameters
     (super rescue {}).
@@ -16,7 +18,6 @@ class Arvados::V1::CollectionsController < ApplicationController
       })
   end
 
-
   def create
     if resource_attrs[:uuid] and (loc = Keep::Locator.parse(resource_attrs[:uuid]))
       resource_attrs[:portable_data_hash] = loc.to_s
@@ -58,39 +59,6 @@ class Arvados::V1::CollectionsController < ApplicationController
     end
   end
 
-  def destroy
-    if !@object.is_trashed
-      @object.update_attributes!(trash_at: db_current_time)
-    end
-    earliest_delete = (@object.trash_at +
-                       Rails.configuration.blob_signature_ttl.seconds)
-    if @object.delete_at > earliest_delete
-      @object.update_attributes!(delete_at: earliest_delete)
-    end
-    show
-  end
-
-  def trash
-    if !@object.is_trashed
-      @object.update_attributes!(trash_at: db_current_time)
-    end
-    show
-  end
-
-  def untrash
-    if @object.is_trashed
-      @object.trash_at = nil
-
-      if params[:ensure_unique_name]
-        @object.save_with_unique_name!
-      else
-        @object.save!
-      end
-    else
-      raise InvalidStateTransitionError
-    end
-    show
-  end
 
   def find_collections(visited, sp, &b)
     case sp
diff --git a/services/api/app/controllers/arvados/v1/groups_controller.rb b/services/api/app/controllers/arvados/v1/groups_controller.rb
index 75ef095..9e0cdf8 100644
--- a/services/api/app/controllers/arvados/v1/groups_controller.rb
+++ b/services/api/app/controllers/arvados/v1/groups_controller.rb
@@ -2,7 +2,19 @@
 #
 # SPDX-License-Identifier: AGPL-3.0
 
+require "trashable"
+
 class Arvados::V1::GroupsController < ApplicationController
+  include TrashableController
+
+  def self._index_requires_parameters
+    (super rescue {}).
+      merge({
+        include_trash: {
+          type: 'boolean', required: false, description: "Include items whose is_trashed attribute is true."
+        },
+      })
+  end
 
   def self._contents_requires_parameters
     params = _index_requires_parameters.
@@ -152,10 +164,10 @@ class Arvados::V1::GroupsController < ApplicationController
       end.compact
 
       if klass == Collection and params[:include_trash]
-        @objects = klass.unscoped.readable_by(*@read_users).
+        @objects = klass.unscoped.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
           order(request_order).where(where_conds)
       else
-        @objects = klass.readable_by(*@read_users).
+        @objects = klass.readable_by(*@read_users, {:include_trash => params[:include_trash]}).
           order(request_order).where(where_conds)
       end
       klass_limit = limit_all - all_objects.count
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index ccd8f82..518d4c8 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -254,7 +254,8 @@ class ArvadosModel < ActiveRecord::Base
 
     # Collect the UUIDs of the authorized users.
     sql_table = kwargs.fetch(:table_name, table_name)
-    include_trashed = kwargs.fetch(:include_trashed, false)
+    include_trash = kwargs.fetch(:include_trash, false)
+    query_on = kwargs.fetch(:query_on, self)
 
     sql_conds = []
     user_uuids = users_list.map { |u| u.uuid }
@@ -263,49 +264,53 @@ class ArvadosModel < ActiveRecord::Base
 
     # Check if any of the users are admin.
     if users_list.select { |u| u.is_admin }.any?
-      if !include_trashed
+      if !include_trash
         # exclude rows that are trashed.
         if self.column_names.include? "owner_uuid"
           sql_conds += ["NOT EXISTS(SELECT target_uuid
-                  FROM permission_view pv
+                  FROM permission_view
                   WHERE trashed = 1 AND
-                  (#{sql_table}.uuid = pv.target_uuid OR #{sql_table}.owner_uuid = pv.target_uuid))"]
+                  (#{sql_table}.uuid = target_uuid OR #{sql_table}.owner_uuid = target_uuid))"]
         else
           sql_conds += ["NOT EXISTS(SELECT target_uuid
-                  FROM permission_view pv
+                  FROM permission_view
                   WHERE trashed = 1 AND
-                  (#{sql_table}.uuid = pv.target_uuid))"]
+                  (#{sql_table}.uuid = target_uuid))"]
         end
       end
     else
-      # Match objects which appear in the permission view
-      trash_clause = if !include_trashed then "trashed = 0 AND" else "" end
+      trash_clause = if !include_trash then "trashed = 0 AND" else "" end
 
-      # Match any object (evidently a group or user) whose UUID is
-      # listed explicitly in user_uuids.
+      # Can read object (evidently a group or user) whose UUID is listed
+      # explicitly in user_uuids.
       sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
 
+      direct_permission_check = "EXISTS(SELECT 1 FROM permission_view
+                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
+                  (#{sql_table}.uuid = target_uuid))"
+
       if self.column_names.include? "owner_uuid"
-        sql_conds += ["EXISTS(SELECT target_uuid
-                  FROM permission_view pv
+        # if an explicit permission row exists for the uuid in question, apply
+        # the "direct_permission_check"
+        # if not, check for permission to read the owner instead
+        sql_conds += ["CASE
+                  WHEN EXISTS(select 1 FROM permission_view where target_uuid = #{sql_table}.uuid)
+                  THEN #{direct_permission_check}
+                  ELSE EXISTS(SELECT 1 FROM permission_view
                   WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
-                  (#{sql_table}.uuid = pv.target_uuid OR
-                  (#{sql_table}.owner_uuid = pv.target_uuid AND pv.target_owner_uuid is NOT NULL)))"]
-        # Match any object whose owner is listed explicitly in
-        # user_uuids.
-        trash_clause = if !include_trashed
+                  (#{sql_table}.owner_uuid = target_uuid AND target_owner_uuid is NOT NULL))
+                  END"]
+        # Can also read if one of the users is the owner of the object.
+        trash_clause = if !include_trash
                          "1 NOT IN (SELECT trashed
-                             FROM permission_view pv
-                             WHERE #{sql_table}.uuid = pv.target_uuid) AND"
+                             FROM permission_view
+                             WHERE #{sql_table}.uuid = target_uuid) AND"
                        else
                          ""
                        end
         sql_conds += ["(#{trash_clause} #{sql_table}.owner_uuid IN (:user_uuids))"]
       else
-        sql_conds += ["EXISTS(SELECT target_uuid
-                  FROM permission_view pv
-                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
-                  (#{sql_table}.uuid = pv.target_uuid))"]
+        sql_conds += [direct_permission_check]
       end
 
       if sql_table == "links"
@@ -317,9 +322,9 @@ class ArvadosModel < ActiveRecord::Base
       end
     end
 
-    where(sql_conds.join(' OR '),
-          user_uuids: user_uuids,
-          permission_link_classes: ['permission', 'resources'])
+    query_on.where(sql_conds.join(' OR '),
+                    user_uuids: user_uuids,
+                    permission_link_classes: ['permission', 'resources'])
   end
 
   def save_with_unique_name!
diff --git a/services/api/app/models/container.rb b/services/api/app/models/container.rb
index 36c87af..d61ba75 100644
--- a/services/api/app/models/container.rb
+++ b/services/api/app/models/container.rb
@@ -297,17 +297,15 @@ class Container < ArvadosModel
   end
 
   def self.readable_by(*users_list)
-    if users_list.select { |u| u.is_admin }.any?
-      return self
+    # Load optional keyword arguments, if they exist.
+    if users_list.last.is_a? Hash
+      kwargs = users_list.pop
+    else
+      kwargs = {}
     end
-    user_uuids = users_list.map { |u| u.uuid }
-    uuid_list = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
-    uuid_list.uniq!
-    permitted = "(SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (:uuids))"
-    joins(:container_requests).
-      where("container_requests.uuid IN #{permitted} OR "+
-            "container_requests.owner_uuid IN (:uuids)",
-            uuids: uuid_list)
+    kwargs[:query_on] = joins(:container_requests)
+    users_list.push kwargs
+    ContainerRequest.readable_by(*users_list)
   end
 
   def final?
diff --git a/services/api/app/models/log.rb b/services/api/app/models/log.rb
index 73f143e..7f2d3ef 100644
--- a/services/api/app/models/log.rb
+++ b/services/api/app/models/log.rb
@@ -60,6 +60,9 @@ class Log < ArvadosModel
   end
 
   def self.readable_by(*users_list)
+    if users_list.last.is_a? Hash
+      users_list.pop
+    end
     if users_list.select { |u| u.is_admin }.any?
       return self
     end
diff --git a/services/api/config/routes.rb b/services/api/config/routes.rb
index 2e9d618..5b6fe80 100644
--- a/services/api/config/routes.rb
+++ b/services/api/config/routes.rb
@@ -30,6 +30,8 @@ Server::Application.routes.draw do
       resources :groups do
         get 'contents', on: :collection
         get 'contents', on: :member
+        post 'trash', on: :member
+        post 'untrash', on: :member
       end
       resources :humans
       resources :job_tasks
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 639610a..71ac8c4 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -37,9 +37,8 @@ perm_edges (tail_uuid, head_uuid, val, follow, trashed) AS (
               LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
               WHERE links.link_class = 'permission'
        UNION ALL
-       SELECT owner_uuid, uuid, 3, true, CASE
-              WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1
-              ELSE 0 END
+       SELECT owner_uuid, uuid, 3, true,
+              CASE WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1 ELSE 0 END
               FROM groups
        ),
 perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
diff --git a/services/api/lib/trashable.rb b/services/api/lib/trashable.rb
index 1e2f466..38ebaf7 100644
--- a/services/api/lib/trashable.rb
+++ b/services/api/lib/trashable.rb
@@ -89,3 +89,40 @@ module Trashable
     true
   end
 end
+
+module TrashableController
+  def destroy
+    if !@object.is_trashed
+      @object.update_attributes!(trash_at: db_current_time)
+    end
+    earliest_delete = (@object.trash_at +
+                       Rails.configuration.blob_signature_ttl.seconds)
+    if @object.delete_at > earliest_delete
+      @object.update_attributes!(delete_at: earliest_delete)
+    end
+    show
+  end
+
+  def trash
+    if !@object.is_trashed
+      @object.update_attributes!(trash_at: db_current_time)
+    end
+    show
+  end
+
+  def untrash
+    if @object.is_trashed
+      @object.trash_at = nil
+
+      if params[:ensure_unique_name]
+        @object.save_with_unique_name!
+      else
+        @object.save!
+      end
+    else
+      raise InvalidStateTransitionError
+    end
+    show
+  end
+
+end
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 1cb5817..2411831 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -313,6 +313,13 @@ trashed_subproject:
   owner_uuid: zzzzz-j7d0g-trashedproject1
   name: trashed subproject
   group_class: project
+  is_trashed: false
+
+trashed_subproject3:
+  uuid: zzzzz-j7d0g-trashedproject3
+  owner_uuid: zzzzz-j7d0g-trashedproject1
+  name: trashed subproject 3
+  group_class: project
   trash_at: 2001-01-01T00:00:00Z
   delete_at: 2038-03-01T00:00:00Z
   is_trashed: true
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 043d665..b80fcb1 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -533,111 +533,145 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
   ### trashed project tests ###
 
   [:active, :admin].each do |auth|
-    test "trashed project is hidden in contents (#{auth})" do
-      authorize_with auth
-      get :contents, {
-            id: users(:active).uuid,
-            format: :json,
-          }
-      item_uuids = json_response['items'].map do |item|
-        item['uuid']
+    # project: to query,    to untrash,    is visible, parent contents listing success
+    [[:trashed_project,     [],                 false, true],
+     [:trashed_project,     [:trashed_project], true,  true],
+     [:trashed_subproject,  [],                 false, false],
+     [:trashed_subproject,  [:trashed_project], true,  true],
+     [:trashed_subproject3, [:trashed_project], false, true],
+     [:trashed_subproject3, [:trashed_subproject3], false, false],
+     [:trashed_subproject3, [:trashed_project, :trashed_subproject3], true, true],
+    ].each do |project, untrash, visible, success|
+
+      test "contents listing #{project} #{untrash} as #{auth}" do
+        authorize_with auth
+        untrash.each do |pr|
+          Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+        end
+        get :contents, {
+              id: groups(project).owner_uuid,
+              format: :json
+            }
+        if success
+          assert_response :success
+          item_uuids = json_response['items'].map do |item|
+            item['uuid']
+          end
+          if visible
+            assert_includes(item_uuids, groups(project).uuid)
+          else
+            assert_not_includes(item_uuids, groups(project).uuid)
+          end
+        else
+          assert_response 404
+        end
       end
-      assert_not_includes(item_uuids, groups(:trashed_project).uuid)
-    end
 
-    test "untrashed project is visible in contents (#{auth})" do
-      authorize_with auth
-      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
-      get :contents, {
-            id: users(:active).uuid,
-            format: :json,
-          }
-      item_uuids = json_response['items'].map do |item|
-        item['uuid']
+      test "contents of #{project} #{untrash} as #{auth}" do
+        authorize_with auth
+        untrash.each do |pr|
+          Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+        end
+        get :contents, {
+              id: groups(project).uuid,
+              format: :json
+            }
+        if visible
+          assert_response :success
+        else
+          assert_response 404
+        end
       end
-      assert_includes(item_uuids, groups(:trashed_project).uuid)
-    end
 
-    test "trashed project is hidden in index (#{auth})" do
-      authorize_with :active
-      get :index, {
-            format: :json,
-          }
-      item_uuids = json_response['items'].map do |item|
-        item['uuid']
+      test "index #{project} #{untrash} as #{auth}" do
+        authorize_with auth
+        untrash.each do |pr|
+          Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+        end
+        get :index, {
+              format: :json,
+            }
+        assert_response :success
+        item_uuids = json_response['items'].map do |item|
+          item['uuid']
+        end
+        if visible
+          assert_includes(item_uuids, groups(project).uuid)
+        else
+          assert_not_includes(item_uuids, groups(project).uuid)
+        end
       end
-      assert_not_includes(item_uuids, groups(:trashed_project).uuid)
-      assert_not_includes(item_uuids, groups(:trashed_subproject).uuid)
-    end
 
-    test "untrashed project is visible in index (#{auth})" do
-      authorize_with :active
-      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
-      get :index, {
-            format: :json,
-          }
-      item_uuids = json_response['items'].map do |item|
-        item['uuid']
+      test "show #{project} #{untrash} as #{auth}" do
+        authorize_with auth
+        untrash.each do |pr|
+          Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+        end
+        get :show, {
+              id: groups(project).uuid,
+              format: :json
+            }
+        if visible
+          assert_response :success
+        else
+          assert_response 404
+        end
       end
-      assert_includes(item_uuids, groups(:trashed_project).uuid)
-      assert_includes(item_uuids, groups(:trashed_subproject).uuid)
-    end
 
-    test "cannot get contents of trashed project (#{auth})" do
-      authorize_with :active
-      get :contents, {
-            id: groups(:trashed_project).uuid,
-            format: :json,
-          }
-      assert_response 404
-    end
+      test "show include_trash #{project} #{untrash} as #{auth}" do
+        authorize_with auth
+        untrash.each do |pr|
+          Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+        end
+        get :show, {
+              id: groups(project).uuid,
+              format: :json,
+              include_trash: true
+            }
+        assert_response :success
+      end
 
-    test "cannot get contents of trashed subproject (#{auth})" do
-      authorize_with :active
-      get :contents, {
-            id: groups(:trashed_subproject).uuid,
-            format: :json,
-          }
-      assert_response 404
+      test "index include_trash #{project} #{untrash} as #{auth}" do
+        authorize_with auth
+        untrash.each do |pr|
+          Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+        end
+        get :index, {
+              format: :json,
+              include_trash: true
+            }
+        assert_response :success
+        item_uuids = json_response['items'].map do |item|
+          item['uuid']
+        end
+        assert_includes(item_uuids, groups(project).uuid)
+      end
     end
 
-    test "can get contents of untrashed project (#{auth})" do
-      authorize_with :active
-      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
-      get :contents, {
+    test "delete project #{auth}" do
+      authorize_with auth
+      [:trashed_project].each do |pr|
+        Group.find_by_uuid(groups(pr).uuid).update! is_trashed: false
+      end
+      assert !Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
+      post :destroy, {
             id: groups(:trashed_project).uuid,
             format: :json,
           }
-      assert_response 200
-    end
-
-    test "can get contents of untrashed subproject (#{auth})" do
-      authorize_with :active
-      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
-      get :contents, {
-            id: groups(:trashed_subproject).uuid,
-            format: :json,
-          }
-      assert_response 200
+      assert_response :success
+      assert Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
     end
 
-    test "cannot show trashed project (#{auth})" do
-      authorize_with :active
-      get :show, {
+    test "untrash project #{auth}" do
+      authorize_with auth
+      assert Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
+      post :untrash, {
             id: groups(:trashed_project).uuid,
             format: :json,
           }
-      assert_response 404
+      assert_response :success
+      assert !Group.find_by_uuid(groups(:trashed_project).uuid).is_trashed
     end
 
-    test "cannot show trashed subproject (#{auth})" do
-      authorize_with :active
-      get :show, {
-            id: groups(:trashed_subproject).uuid,
-            format: :json,
-          }
-      assert_response 404
-    end
   end
-
 end
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index e2c8829..4672acd 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -69,30 +69,87 @@ class GroupTest < ActiveSupport::TestCase
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
     g_foo.update! is_trashed: true
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
+    assert Collection.readable_by(users(:active), {:include_trash => true}).where(uuid: col.uuid).any?
     g_foo.update! is_trashed: false
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
   end
 
+  test "delete group" do
+    set_user_from_auth :active_trustedclient
 
-  test "delete group propagates to subgroups" do
+    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)
+
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
+    g_foo.update! is_trashed: true
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+
+    assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_bar.uuid).any?
+    assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_baz.uuid).any?
+  end
+
+
+  test "delete 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)
-    col = Collection.create!(owner_uuid: g_bar.uuid)
+    g_baz = Group.create!(name: "baz", owner_uuid: g_bar.uuid)
 
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
-    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
+    g_bar.update! is_trashed: true
+
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+
+    assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_bar.uuid).any?
+    assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_baz.uuid).any?
+  end
+
+  test "delete 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)
+
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
+    g_baz.update! is_trashed: true
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+    assert Group.readable_by(users(:active), {:include_trash => true}).where(uuid: g_baz.uuid).any?
+  end
+
+
+  test "delete group propagates to subgroups" do
+    set_user_from_auth :active_trustedclient
+
+    g_foo = groups(:trashed_project)
+    g_bar = groups(:trashed_subproject)
+    g_baz = groups(:trashed_subproject3)
+    col = collections(:collection_in_trashed_subproject)
 
-    g_foo.update! is_trashed: true
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
 
     set_user_from_auth :admin
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
 
     set_user_from_auth :active_trustedclient
@@ -100,6 +157,12 @@ class GroupTest < ActiveSupport::TestCase
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+
+    # this one should still be deleted.
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).empty?
+
+    g_baz.update! is_trashed: false
+    assert Group.readable_by(users(:active)).where(uuid: g_baz.uuid).any?
   end
 
 end

commit 549a1cc8d7854d58b1182adbee64c771222023c7
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Mon Aug 28 09:57:05 2017 -0400

    12032: Adding controller tests
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index c9783d0..1cb5817 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -310,7 +310,7 @@ trashed_project:
 
 trashed_subproject:
   uuid: zzzzz-j7d0g-trashedproject2
-  owner_uuid:
+  owner_uuid: zzzzz-j7d0g-trashedproject1
   name: trashed subproject
   group_class: project
   trash_at: 2001-01-01T00:00:00Z
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 56b0790..4cf8778 100644
--- a/services/api/test/functional/arvados/v1/collections_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/collections_controller_test.rb
@@ -1090,4 +1090,44 @@ EOS
     assert_nil json_response['delete_at']
     assert_match /^same name for trashed and persisted collections \(\d{4}-\d\d-\d\d.*?Z\)$/, json_response['name']
   end
+
+  test 'cannot show collection in trashed subproject' do
+    authorize_with :active
+    get :show, {
+      id: collections(:collection_in_trashed_subproject).uuid,
+      format: :json
+    }
+    assert_response 404
+  end
+
+  test 'can show collection in untrashed subproject' do
+    authorize_with :active
+    Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+    get :show, {
+      id: collections(:collection_in_trashed_subproject).uuid,
+      format: :json,
+    }
+    assert_response :success
+  end
+
+  test 'cannot index collection in trashed subproject' do
+    authorize_with :active
+    get :index
+    assert_response :success
+    item_uuids = json_response['items'].map do |item|
+      item['uuid']
+    end
+    assert_not_includes(item_uuids, collections(:collection_in_trashed_subproject).uuid)
+  end
+
+  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
+    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
 end
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 5ae1e9a..043d665 100644
--- a/services/api/test/functional/arvados/v1/groups_controller_test.rb
+++ b/services/api/test/functional/arvados/v1/groups_controller_test.rb
@@ -529,4 +529,115 @@ class Arvados::V1::GroupsControllerTest < ActionController::TestCase
     assert_includes(owners, groups(:aproject).uuid)
     assert_includes(owners, groups(:asubproject).uuid)
   end
+
+  ### trashed project tests ###
+
+  [:active, :admin].each do |auth|
+    test "trashed project is hidden in contents (#{auth})" do
+      authorize_with auth
+      get :contents, {
+            id: users(:active).uuid,
+            format: :json,
+          }
+      item_uuids = json_response['items'].map do |item|
+        item['uuid']
+      end
+      assert_not_includes(item_uuids, groups(:trashed_project).uuid)
+    end
+
+    test "untrashed project is visible in contents (#{auth})" do
+      authorize_with auth
+      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+      get :contents, {
+            id: users(:active).uuid,
+            format: :json,
+          }
+      item_uuids = json_response['items'].map do |item|
+        item['uuid']
+      end
+      assert_includes(item_uuids, groups(:trashed_project).uuid)
+    end
+
+    test "trashed project is hidden in index (#{auth})" do
+      authorize_with :active
+      get :index, {
+            format: :json,
+          }
+      item_uuids = json_response['items'].map do |item|
+        item['uuid']
+      end
+      assert_not_includes(item_uuids, groups(:trashed_project).uuid)
+      assert_not_includes(item_uuids, groups(:trashed_subproject).uuid)
+    end
+
+    test "untrashed project is visible in index (#{auth})" do
+      authorize_with :active
+      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+      get :index, {
+            format: :json,
+          }
+      item_uuids = json_response['items'].map do |item|
+        item['uuid']
+      end
+      assert_includes(item_uuids, groups(:trashed_project).uuid)
+      assert_includes(item_uuids, groups(:trashed_subproject).uuid)
+    end
+
+    test "cannot get contents of trashed project (#{auth})" do
+      authorize_with :active
+      get :contents, {
+            id: groups(:trashed_project).uuid,
+            format: :json,
+          }
+      assert_response 404
+    end
+
+    test "cannot get contents of trashed subproject (#{auth})" do
+      authorize_with :active
+      get :contents, {
+            id: groups(:trashed_subproject).uuid,
+            format: :json,
+          }
+      assert_response 404
+    end
+
+    test "can get contents of untrashed project (#{auth})" do
+      authorize_with :active
+      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+      get :contents, {
+            id: groups(:trashed_project).uuid,
+            format: :json,
+          }
+      assert_response 200
+    end
+
+    test "can get contents of untrashed subproject (#{auth})" do
+      authorize_with :active
+      Group.find_by_uuid(groups(:trashed_project).uuid).update! is_trashed: false
+      get :contents, {
+            id: groups(:trashed_subproject).uuid,
+            format: :json,
+          }
+      assert_response 200
+    end
+
+    test "cannot show trashed project (#{auth})" do
+      authorize_with :active
+      get :show, {
+            id: groups(:trashed_project).uuid,
+            format: :json,
+          }
+      assert_response 404
+    end
+
+    test "cannot show trashed subproject (#{auth})" do
+      authorize_with :active
+      get :show, {
+            id: groups(:trashed_subproject).uuid,
+            format: :json,
+          }
+      assert_response 404
+    end
+  end
+
 end

commit 61ba951ab92487e8123d83b1d77a16e24575ddcd
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Fri Aug 25 14:23:30 2017 -0400

    12032: Add test fixtures
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index e6195d7..639610a 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -37,7 +37,10 @@ perm_edges (tail_uuid, head_uuid, val, follow, trashed) AS (
               LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
               WHERE links.link_class = 'permission'
        UNION ALL
-       SELECT owner_uuid, uuid, 3, true, CASE is_trashed WHEN true THEN 1 ELSE 0 END FROM groups
+       SELECT owner_uuid, uuid, 3, true, CASE
+              WHEN trash_at IS NOT NULL and trash_at < clock_timestamp() THEN 1
+              ELSE 0 END
+              FROM groups
        ),
 perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
      SELECT 3::smallint             AS val,
diff --git a/services/api/test/fixtures/collections.yml b/services/api/test/fixtures/collections.yml
index 7777f74..8023503 100644
--- a/services/api/test/fixtures/collections.yml
+++ b/services/api/test/fixtures/collections.yml
@@ -703,6 +703,17 @@ same_name_as_trashed_coll_to_test_name_conflict_on_untrash:
   manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:file1 0:0:file2\n"
   name: same name for trashed and persisted collections
 
+collection_in_trashed_subproject:
+  uuid: zzzzz-4zz18-trashedproj2col
+  portable_data_hash: 80cf6dd2cf079dd13f272ec4245cb4a8+48
+  owner_uuid: zzzzz-j7d0g-trashedproject2
+  created_at: 2014-02-03T17:22:54Z
+  modified_by_client_uuid: zzzzz-ozdt8-brczlopd8u8d0jr
+  modified_by_user_uuid: zzzzz-tpzed-d9tiejq69daie8f
+  modified_at: 2014-02-03T17:22:54Z
+  updated_at: 2014-02-03T17:22:54Z
+  manifest_text: ". d41d8cd98f00b204e9800998ecf8427e+0 0:0:file1 0:0:file2\n"
+  name: collection in trashed subproject
 
 # Test Helper trims the rest of the file
 
diff --git a/services/api/test/fixtures/groups.yml b/services/api/test/fixtures/groups.yml
index 90d087f..c9783d0 100644
--- a/services/api/test/fixtures/groups.yml
+++ b/services/api/test/fixtures/groups.yml
@@ -298,3 +298,21 @@ starred_and_shared_active_user_project:
   name: Starred and shared active user project
   description: Starred and shared active user project
   group_class: project
+
+trashed_project:
+  uuid: zzzzz-j7d0g-trashedproject1
+  owner_uuid: zzzzz-tpzed-xurymjxw79nv3jz
+  name: trashed project
+  group_class: project
+  trash_at: 2001-01-01T00:00:00Z
+  delete_at: 2038-03-01T00:00:00Z
+  is_trashed: true
+
+trashed_subproject:
+  uuid: zzzzz-j7d0g-trashedproject2
+  owner_uuid:
+  name: trashed subproject
+  group_class: project
+  trash_at: 2001-01-01T00:00:00Z
+  delete_at: 2038-03-01T00:00:00Z
+  is_trashed: true

commit 724da26ed62a59b4879a8a02eaaab99bf681ee10
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Aug 24 21:58:38 2017 -0400

    12032: Fix hiding trashed groups directly owned by user.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 5a9d43f..ccd8f82 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -293,7 +293,14 @@ class ArvadosModel < ActiveRecord::Base
                   (#{sql_table}.owner_uuid = pv.target_uuid AND pv.target_owner_uuid is NOT NULL)))"]
         # Match any object whose owner is listed explicitly in
         # user_uuids.
-        sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
+        trash_clause = if !include_trashed
+                         "1 NOT IN (SELECT trashed
+                             FROM permission_view pv
+                             WHERE #{sql_table}.uuid = pv.target_uuid) AND"
+                       else
+                         ""
+                       end
+        sql_conds += ["(#{trash_clause} #{sql_table}.owner_uuid IN (:user_uuids))"]
       else
         sql_conds += ["EXISTS(SELECT target_uuid
                   FROM permission_view pv
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index 11f546b..e2c8829 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -90,10 +90,12 @@ class GroupTest < ActiveSupport::TestCase
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
     assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
 
-    assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_foo.uuid).any?
-    assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_bar.uuid).any?
-    assert Collection.readable_by(users(:active), {:include_trashed => true}).where(uuid: col.uuid).any?
+    set_user_from_auth :admin
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
 
+    set_user_from_auth :active_trustedclient
     g_foo.update! is_trashed: false
     assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
     assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?

commit 9620f75a4d257bb5b6381f6b9671ed4179db1d12
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Aug 24 17:09:49 2017 -0400

    12032: Tests for group delete behavior.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/models/group.rb b/services/api/app/models/group.rb
index 0dd2a73..861800d 100644
--- a/services/api/app/models/group.rb
+++ b/services/api/app/models/group.rb
@@ -3,12 +3,15 @@
 # SPDX-License-Identifier: AGPL-3.0
 
 require 'can_be_an_owner'
+require 'trashable'
 
 class Group < ArvadosModel
   include HasUuid
   include KindAndEtag
   include CommonApiTemplate
   include CanBeAnOwner
+  include Trashable
+
   after_create :invalidate_permissions_cache
   after_update :maybe_invalidate_permissions_cache
   before_create :assign_name
@@ -18,6 +21,9 @@ class Group < ArvadosModel
     t.add :group_class
     t.add :description
     t.add :writable_by
+    t.add :delete_at
+    t.add :trash_at
+    t.add :is_trashed
   end
 
   def maybe_invalidate_permissions_cache
diff --git a/services/api/db/migrate/20170824202826_trashable_groups.rb b/services/api/db/migrate/20170824202826_trashable_groups.rb
new file mode 100644
index 0000000..b7e3373
--- /dev/null
+++ b/services/api/db/migrate/20170824202826_trashable_groups.rb
@@ -0,0 +1,7 @@
+class TrashableGroups < ActiveRecord::Migration
+  def change
+    add_column :groups, :trash_at, :datetime
+    add_column :groups, :delete_at, :datetime
+    add_column :groups, :is_trashed, :boolean, null: false, default: false
+  end
+end
diff --git a/services/api/db/structure.sql b/services/api/db/structure.sql
index 7e6bb1d..90aa0a3 100644
--- a/services/api/db/structure.sql
+++ b/services/api/db/structure.sql
@@ -391,7 +391,10 @@ CREATE TABLE groups (
     name character varying(255) NOT NULL,
     description character varying(524288),
     updated_at timestamp without time zone NOT NULL,
-    group_class character varying(255)
+    group_class character varying(255),
+    trash_at timestamp without time zone,
+    delete_at timestamp without time zone,
+    is_trashed boolean DEFAULT false NOT NULL
 );
 
 
@@ -2791,3 +2794,5 @@ INSERT INTO schema_migrations (version) VALUES ('20170419175801');
 
 INSERT INTO schema_migrations (version) VALUES ('20170628185847');
 
+INSERT INTO schema_migrations (version) VALUES ('20170824202826');
+
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 82839e8..e6195d7 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -37,7 +37,7 @@ perm_edges (tail_uuid, head_uuid, val, follow, trashed) AS (
               LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
               WHERE links.link_class = 'permission'
        UNION ALL
-       SELECT owner_uuid, uuid, 3, true, 0::smallint FROM groups
+       SELECT owner_uuid, uuid, 3, true, CASE is_trashed WHEN true THEN 1 ELSE 0 END FROM groups
        ),
 perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
      SELECT 3::smallint             AS val,
diff --git a/services/api/test/unit/group_test.rb b/services/api/test/unit/group_test.rb
index a1123b1..11f546b 100644
--- a/services/api/test/unit/group_test.rb
+++ b/services/api/test/unit/group_test.rb
@@ -60,4 +60,44 @@ class GroupTest < ActiveSupport::TestCase
     assert g_foo.errors.messages[:owner_uuid].join(" ").match(/ownership cycle/)
   end
 
+  test "delete group hides contents" do
+    set_user_from_auth :active_trustedclient
+
+    g_foo = Group.create!(name: "foo")
+    col = Collection.create!(owner_uuid: g_foo.uuid)
+
+    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+    g_foo.update! is_trashed: true
+    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
+    g_foo.update! is_trashed: false
+    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+  end
+
+
+  test "delete group propagates to subgroups" do
+    set_user_from_auth :active_trustedclient
+
+    g_foo = Group.create!(name: "foo")
+    g_bar = Group.create!(name: "bar", owner_uuid: g_foo.uuid)
+    col = Collection.create!(owner_uuid: g_bar.uuid)
+
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+
+    g_foo.update! is_trashed: true
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).empty?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).empty?
+    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).empty?
+
+    assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active), {:include_trashed => true}).where(uuid: g_bar.uuid).any?
+    assert Collection.readable_by(users(:active), {:include_trashed => true}).where(uuid: col.uuid).any?
+
+    g_foo.update! is_trashed: false
+    assert Group.readable_by(users(:active)).where(uuid: g_foo.uuid).any?
+    assert Group.readable_by(users(:active)).where(uuid: g_bar.uuid).any?
+    assert Collection.readable_by(users(:active)).where(uuid: col.uuid).any?
+  end
+
 end

commit 0365e9f395deab27e1d310b1bf06d5b4f1e76eac
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Aug 24 16:22:12 2017 -0400

    12032: Additional refactoring of readable_by.  Refactor "trashable" into module.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index 457b1bf..5a9d43f 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -254,7 +254,7 @@ class ArvadosModel < ActiveRecord::Base
 
     # Collect the UUIDs of the authorized users.
     sql_table = kwargs.fetch(:table_name, table_name)
-    include_trashed = kwargs.fetch(:include_trashed, 0)
+    include_trashed = kwargs.fetch(:include_trashed, false)
 
     sql_conds = []
     user_uuids = users_list.map { |u| u.uuid }
@@ -263,57 +263,56 @@ class ArvadosModel < ActiveRecord::Base
 
     # Check if any of the users are admin.
     if users_list.select { |u| u.is_admin }.any?
-      # For admins, only filter on "trashed"
-      # sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
-      #             FROM permission_view
-      #             WHERE trashed in (:include_trashed)
-      #             GROUP BY user_uuid, target_uuid)"]
-
-      # if self.column_names.include? 'owner_uuid'
-      #   sql_conds[0] += "AND #{sql_table}.owner_uuid in (SELECT target_uuid
-      #             FROM permission_view
-      #             WHERE trashed in (:include_trashed)
-      #             GROUP BY user_uuid, target_uuid)"
-      # end
-      return where({})
+      if !include_trashed
+        # exclude rows that are trashed.
+        if self.column_names.include? "owner_uuid"
+          sql_conds += ["NOT EXISTS(SELECT target_uuid
+                  FROM permission_view pv
+                  WHERE trashed = 1 AND
+                  (#{sql_table}.uuid = pv.target_uuid OR #{sql_table}.owner_uuid = pv.target_uuid))"]
+        else
+          sql_conds += ["NOT EXISTS(SELECT target_uuid
+                  FROM permission_view pv
+                  WHERE trashed = 1 AND
+                  (#{sql_table}.uuid = pv.target_uuid))"]
+        end
+      end
     else
+      # Match objects which appear in the permission view
+      trash_clause = if !include_trashed then "trashed = 0 AND" else "" end
+
       # Match any object (evidently a group or user) whose UUID is
       # listed explicitly in user_uuids.
-      sql_conds += ["#{sql_table}.uuid in (:user_uuids)"]
-
-      # Match any object whose owner is listed explicitly in
-      # user_uuids.
-      sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
-
-      # At least read permission from user_uuid to target_uuid of object
-      sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
-                  FROM permission_view
-                  WHERE user_uuid in (:user_uuids) and perm_level >= 1 and trashed = (:include_trashed)
-                  GROUP BY user_uuid, target_uuid)"]
-
-      if self.column_names.include? 'owner_uuid'
-        # At least read permission from user_uuid to target_uuid that owns object
-        sql_conds += ["#{sql_table}.owner_uuid in (SELECT target_uuid
-                  FROM permission_view
-                  WHERE user_uuid in (:user_uuids) and
-                    target_owner_uuid IS NOT NULL and
-                    perm_level >= 1 and trashed = (:include_trashed)
-                  GROUP BY user_uuid, target_uuid)"]
+      sql_conds += ["#{sql_table}.uuid IN (:user_uuids)"]
+
+      if self.column_names.include? "owner_uuid"
+        sql_conds += ["EXISTS(SELECT target_uuid
+                  FROM permission_view pv
+                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
+                  (#{sql_table}.uuid = pv.target_uuid OR
+                  (#{sql_table}.owner_uuid = pv.target_uuid AND pv.target_owner_uuid is NOT NULL)))"]
+        # Match any object whose owner is listed explicitly in
+        # user_uuids.
+        sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
+      else
+        sql_conds += ["EXISTS(SELECT target_uuid
+                  FROM permission_view pv
+                  WHERE user_uuid IN (:user_uuids) AND perm_level >= 1 AND #{trash_clause}
+                  (#{sql_table}.uuid = pv.target_uuid))"]
       end
 
       if sql_table == "links"
         # Match any permission link that gives one of the authorized
         # users some permission _or_ gives anyone else permission to
         # view one of the authorized users.
-        sql_conds += ["(#{sql_table}.link_class in (:permission_link_classes) AND "+
+        sql_conds += ["(#{sql_table}.link_class IN (:permission_link_classes) AND "+
                       "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"]
       end
     end
 
     where(sql_conds.join(' OR '),
           user_uuids: user_uuids,
-          permission_link_classes: ['permission', 'resources'],
-          include_trashed: include_trashed)
+          permission_link_classes: ['permission', 'resources'])
   end
 
   def save_with_unique_name!
diff --git a/services/api/app/models/collection.rb b/services/api/app/models/collection.rb
index 6f13a63..ce7e9fe 100644
--- a/services/api/app/models/collection.rb
+++ b/services/api/app/models/collection.rb
@@ -4,6 +4,7 @@
 
 require 'arvados/keep'
 require 'sweep_trashed_collections'
+require 'trashable'
 
 class Collection < ArvadosModel
   extend CurrentApiClient
@@ -11,20 +12,16 @@ class Collection < ArvadosModel
   include HasUuid
   include KindAndEtag
   include CommonApiTemplate
+  include Trashable
 
   serialize :properties, Hash
 
-  before_validation :set_validation_timestamp
   before_validation :default_empty_manifest
   before_validation :check_encoding
   before_validation :check_manifest_validity
   before_validation :check_signatures
   before_validation :strip_signatures_and_update_replication_confirmed
-  before_validation :ensure_trash_at_not_in_past
-  before_validation :sync_trash_state
-  before_validation :default_trash_interval
   validate :ensure_pdh_matches_manifest_text
-  validate :validate_trash_and_delete_timing
   before_save :set_file_names
 
   # Query only untrashed collections by default.
@@ -486,81 +483,4 @@ class Collection < ArvadosModel
     super
   end
 
-  # Use a single timestamp for all validations, even though each
-  # validation runs at a different time.
-  def set_validation_timestamp
-    @validation_timestamp = db_current_time
-  end
-
-  # If trash_at is being changed to a time in the past, change it to
-  # now. This allows clients to say "expires {client-current-time}"
-  # without failing due to clock skew, while avoiding odd log entries
-  # like "expiry date changed to {1 year ago}".
-  def ensure_trash_at_not_in_past
-    if trash_at_changed? && trash_at
-      self.trash_at = [@validation_timestamp, trash_at].max
-    end
-  end
-
-  # Caller can move into/out of trash by setting/clearing is_trashed
-  # -- however, if the caller also changes trash_at, then any changes
-  # to is_trashed are ignored.
-  def sync_trash_state
-    if is_trashed_changed? && !trash_at_changed?
-      if is_trashed
-        self.trash_at = @validation_timestamp
-      else
-        self.trash_at = nil
-        self.delete_at = nil
-      end
-    end
-    self.is_trashed = trash_at && trash_at <= @validation_timestamp || false
-    true
-  end
-
-  def default_trash_interval
-    if trash_at_changed? && !delete_at_changed?
-      # If trash_at is updated without touching delete_at,
-      # automatically update delete_at to a sensible value.
-      if trash_at.nil?
-        self.delete_at = nil
-      else
-        self.delete_at = trash_at + Rails.configuration.default_trash_lifetime.seconds
-      end
-    elsif !trash_at || !delete_at || trash_at > delete_at
-      # Not trash, or bogus arguments? Just validate in
-      # validate_trash_and_delete_timing.
-    elsif delete_at_changed? && delete_at >= trash_at
-      # Fix delete_at if needed, so it's not earlier than the expiry
-      # time on any permission tokens that might have been given out.
-
-      # In any case there are no signatures expiring after now+TTL.
-      # Also, if the existing trash_at time has already passed, we
-      # know we haven't given out any signatures since then.
-      earliest_delete = [
-        @validation_timestamp,
-        trash_at_was,
-      ].compact.min + Rails.configuration.blob_signature_ttl.seconds
-
-      # The previous value of delete_at is also an upper bound on the
-      # longest-lived permission token. For example, if TTL=14,
-      # trash_at_was=now-7, delete_at_was=now+7, then it is safe to
-      # set trash_at=now+6, delete_at=now+8.
-      earliest_delete = [earliest_delete, delete_at_was].compact.min
-
-      # If delete_at is too soon, use the earliest possible time.
-      if delete_at < earliest_delete
-        self.delete_at = earliest_delete
-      end
-    end
-  end
-
-  def validate_trash_and_delete_timing
-    if trash_at.nil? != delete_at.nil?
-      errors.add :delete_at, "must be set if trash_at is set, and must be nil otherwise"
-    elsif delete_at && delete_at < trash_at
-      errors.add :delete_at, "must not be earlier than trash_at"
-    end
-    true
-  end
 end
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index c8c9bfb..0e2db76 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -155,10 +155,9 @@ class User < ArvadosModel
     install_view('permission')
     all_perms = {}
     ActiveRecord::Base.connection.
-      exec_query('SELECT user_uuid, target_owner_uuid, max(perm_level), max(trashed)
+      exec_query('SELECT user_uuid, target_owner_uuid, perm_level, trashed
                   FROM permission_view
-                  WHERE target_owner_uuid IS NOT NULL
-                  GROUP BY user_uuid, target_owner_uuid',
+                  WHERE target_owner_uuid IS NOT NULL',
                   # "name" arg is a query label that appears in logs:
                   "all_group_permissions",
                   ).rows.each do |user_uuid, group_uuid, max_p_val, trashed|
@@ -176,11 +175,10 @@ class User < ArvadosModel
 
     group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
     ActiveRecord::Base.connection.
-      exec_query('SELECT target_owner_uuid, max(perm_level), max(trashed)
+      exec_query('SELECT target_owner_uuid, perm_level, trashed
                   FROM permission_view
                   WHERE user_uuid = $1
-                  AND target_owner_uuid IS NOT NULL
-                  GROUP BY target_owner_uuid',
+                  AND target_owner_uuid IS NOT NULL',
                   # "name" arg is a query label that appears in logs:
                   "group_permissions for #{uuid}",
                   # "binds" arg is an array of [col_id, value] for '$1' vars:
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 10c4b27..82839e8 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -60,7 +60,8 @@ perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
 )
 SELECT user_uuid,
        target_uuid,
-       val AS perm_level,
+       MAX(val) AS perm_level,
        CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid,
-       trashed
-       FROM perm;
+       MAX(trashed) AS trashed
+       FROM perm
+       GROUP BY user_uuid, target_uuid, target_owner_uuid;
diff --git a/services/api/lib/trashable.rb b/services/api/lib/trashable.rb
new file mode 100644
index 0000000..1e2f466
--- /dev/null
+++ b/services/api/lib/trashable.rb
@@ -0,0 +1,91 @@
+# Copyright (C) The Arvados Authors. All rights reserved.
+#
+# SPDX-License-Identifier: AGPL-3.0
+
+module Trashable
+  def self.included(base)
+    base.before_validation :set_validation_timestamp
+    base.before_validation :ensure_trash_at_not_in_past
+    base.before_validation :sync_trash_state
+    base.before_validation :default_trash_interval
+    base.validate :validate_trash_and_delete_timing
+  end
+
+  # Use a single timestamp for all validations, even though each
+  # validation runs at a different time.
+  def set_validation_timestamp
+    @validation_timestamp = db_current_time
+  end
+
+  # If trash_at is being changed to a time in the past, change it to
+  # now. This allows clients to say "expires {client-current-time}"
+  # without failing due to clock skew, while avoiding odd log entries
+  # like "expiry date changed to {1 year ago}".
+  def ensure_trash_at_not_in_past
+    if trash_at_changed? && trash_at
+      self.trash_at = [@validation_timestamp, trash_at].max
+    end
+  end
+
+  # Caller can move into/out of trash by setting/clearing is_trashed
+  # -- however, if the caller also changes trash_at, then any changes
+  # to is_trashed are ignored.
+  def sync_trash_state
+    if is_trashed_changed? && !trash_at_changed?
+      if is_trashed
+        self.trash_at = @validation_timestamp
+      else
+        self.trash_at = nil
+        self.delete_at = nil
+      end
+    end
+    self.is_trashed = trash_at && trash_at <= @validation_timestamp || false
+    true
+  end
+
+  def default_trash_interval
+    if trash_at_changed? && !delete_at_changed?
+      # If trash_at is updated without touching delete_at,
+      # automatically update delete_at to a sensible value.
+      if trash_at.nil?
+        self.delete_at = nil
+      else
+        self.delete_at = trash_at + Rails.configuration.default_trash_lifetime.seconds
+      end
+    elsif !trash_at || !delete_at || trash_at > delete_at
+      # Not trash, or bogus arguments? Just validate in
+      # validate_trash_and_delete_timing.
+    elsif delete_at_changed? && delete_at >= trash_at
+      # Fix delete_at if needed, so it's not earlier than the expiry
+      # time on any permission tokens that might have been given out.
+
+      # In any case there are no signatures expiring after now+TTL.
+      # Also, if the existing trash_at time has already passed, we
+      # know we haven't given out any signatures since then.
+      earliest_delete = [
+        @validation_timestamp,
+        trash_at_was,
+      ].compact.min + Rails.configuration.blob_signature_ttl.seconds
+
+      # The previous value of delete_at is also an upper bound on the
+      # longest-lived permission token. For example, if TTL=14,
+      # trash_at_was=now-7, delete_at_was=now+7, then it is safe to
+      # set trash_at=now+6, delete_at=now+8.
+      earliest_delete = [earliest_delete, delete_at_was].compact.min
+
+      # If delete_at is too soon, use the earliest possible time.
+      if delete_at < earliest_delete
+        self.delete_at = earliest_delete
+      end
+    end
+  end
+
+  def validate_trash_and_delete_timing
+    if trash_at.nil? != delete_at.nil?
+      errors.add :delete_at, "must be set if trash_at is set, and must be nil otherwise"
+    elsif delete_at && delete_at < trash_at
+      errors.add :delete_at, "must not be earlier than trash_at"
+    end
+    true
+  end
+end

commit 4e40ff2ac4198765de3865de375e3a3f93e9aa65
Author: Peter Amstutz <peter.amstutz at curoverse.com>
Date:   Thu Aug 24 12:03:09 2017 -0400

    12032: Use permission_view in subquery to filter objects readable by user.
    
    Arvados-DCO-1.1-Signed-off-by: Peter Amstutz <pamstutz at veritasgenetics.com>

diff --git a/services/api/app/controllers/arvados/v1/repositories_controller.rb b/services/api/app/controllers/arvados/v1/repositories_controller.rb
index a28cee2..b88e10c 100644
--- a/services/api/app/controllers/arvados/v1/repositories_controller.rb
+++ b/services/api/app/controllers/arvados/v1/repositories_controller.rb
@@ -46,7 +46,7 @@ class Arvados::V1::RepositoriesController < ApplicationController
           # group also has access to the repository. Access level is
           # min(group-to-repo permission, user-to-group permission).
           user_aks.each do |user_uuid, _|
-            perm_mask = all_group_permissions[user_uuid][perm.tail_uuid]
+            perm_mask = all_group_permissions[user_uuid].andand[perm.tail_uuid]
             if not perm_mask
               next
             elsif perm_mask[:manage] and perm.name == 'can_manage'
diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb
index ea69735..457b1bf 100644
--- a/services/api/app/models/arvados_model.rb
+++ b/services/api/app/models/arvados_model.rb
@@ -252,49 +252,68 @@ class ArvadosModel < ActiveRecord::Base
       kwargs = {}
     end
 
-    # Check if any of the users are admin.  If so, we're done.
-    if users_list.select { |u| u.is_admin }.any?
-      # Return existing relation with no new filters.
-      return where({})
-    end
-
     # Collect the UUIDs of the authorized users.
-    user_uuids = users_list.map { |u| u.uuid }
-
-    # Collect the UUIDs of all groups readable by any of the
-    # authorized users. If one of these (or the UUID of one of the
-    # authorized users themselves) is an object's owner_uuid, that
-    # object is readable.
-    owner_uuids = user_uuids + users_list.flat_map { |u| u.groups_i_can(:read) }
-    owner_uuids.uniq!
-
-    sql_conds = []
     sql_table = kwargs.fetch(:table_name, table_name)
+    include_trashed = kwargs.fetch(:include_trashed, 0)
 
-    # Match any object (evidently a group or user) whose UUID is
-    # listed explicitly in owner_uuids.
-    sql_conds += ["#{sql_table}.uuid in (:owner_uuids)"]
+    sql_conds = []
+    user_uuids = users_list.map { |u| u.uuid }
 
-    # Match any object whose owner is listed explicitly in
-    # owner_uuids.
-    sql_conds += ["#{sql_table}.owner_uuid IN (:owner_uuids)"]
+    User.install_view('permission')
 
-    # Match the head of any permission link whose tail is listed
-    # explicitly in owner_uuids.
-    sql_conds += ["#{sql_table}.uuid IN (SELECT head_uuid FROM links WHERE link_class='permission' AND tail_uuid IN (:owner_uuids))"]
+    # Check if any of the users are admin.
+    if users_list.select { |u| u.is_admin }.any?
+      # For admins, only filter on "trashed"
+      # sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
+      #             FROM permission_view
+      #             WHERE trashed in (:include_trashed)
+      #             GROUP BY user_uuid, target_uuid)"]
+
+      # if self.column_names.include? 'owner_uuid'
+      #   sql_conds[0] += "AND #{sql_table}.owner_uuid in (SELECT target_uuid
+      #             FROM permission_view
+      #             WHERE trashed in (:include_trashed)
+      #             GROUP BY user_uuid, target_uuid)"
+      # end
+      return where({})
+    else
+      # Match any object (evidently a group or user) whose UUID is
+      # listed explicitly in user_uuids.
+      sql_conds += ["#{sql_table}.uuid in (:user_uuids)"]
+
+      # Match any object whose owner is listed explicitly in
+      # user_uuids.
+      sql_conds += ["#{sql_table}.owner_uuid IN (:user_uuids)"]
+
+      # At least read permission from user_uuid to target_uuid of object
+      sql_conds += ["#{sql_table}.uuid in (SELECT target_uuid
+                  FROM permission_view
+                  WHERE user_uuid in (:user_uuids) and perm_level >= 1 and trashed = (:include_trashed)
+                  GROUP BY user_uuid, target_uuid)"]
+
+      if self.column_names.include? 'owner_uuid'
+        # At least read permission from user_uuid to target_uuid that owns object
+        sql_conds += ["#{sql_table}.owner_uuid in (SELECT target_uuid
+                  FROM permission_view
+                  WHERE user_uuid in (:user_uuids) and
+                    target_owner_uuid IS NOT NULL and
+                    perm_level >= 1 and trashed = (:include_trashed)
+                  GROUP BY user_uuid, target_uuid)"]
+      end
 
-    if sql_table == "links"
-      # Match any permission link that gives one of the authorized
-      # users some permission _or_ gives anyone else permission to
-      # view one of the authorized users.
-      sql_conds += ["(#{sql_table}.link_class in (:permission_link_classes) AND "+
-                    "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"]
+      if sql_table == "links"
+        # Match any permission link that gives one of the authorized
+        # users some permission _or_ gives anyone else permission to
+        # view one of the authorized users.
+        sql_conds += ["(#{sql_table}.link_class in (:permission_link_classes) AND "+
+                      "(#{sql_table}.head_uuid IN (:user_uuids) OR #{sql_table}.tail_uuid IN (:user_uuids)))"]
+      end
     end
 
     where(sql_conds.join(' OR '),
-          owner_uuids: owner_uuids,
           user_uuids: user_uuids,
-          permission_link_classes: ['permission', 'resources'])
+          permission_link_classes: ['permission', 'resources'],
+          include_trashed: include_trashed)
   end
 
   def save_with_unique_name!
diff --git a/services/api/app/models/user.rb b/services/api/app/models/user.rb
index f093410..c8c9bfb 100644
--- a/services/api/app/models/user.rb
+++ b/services/api/app/models/user.rb
@@ -155,14 +155,14 @@ class User < ArvadosModel
     install_view('permission')
     all_perms = {}
     ActiveRecord::Base.connection.
-      exec_query('SELECT user_uuid, target_owner_uuid, max(perm_level)
+      exec_query('SELECT user_uuid, target_owner_uuid, max(perm_level), max(trashed)
                   FROM permission_view
                   WHERE target_owner_uuid IS NOT NULL
                   GROUP BY user_uuid, target_owner_uuid',
                   # "name" arg is a query label that appears in logs:
                   "all_group_permissions",
-                  ).rows.each do |user_uuid, group_uuid, max_p_val|
-      all_perms[user_uuid] ||= {}
+                  ).rows.each do |user_uuid, group_uuid, max_p_val, trashed|
+      all_perms[user_uuid] ||= {user_uuid => {:read => true, :write => true, :manage => true}}
       all_perms[user_uuid][group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
     end
     all_perms
@@ -174,9 +174,9 @@ class User < ArvadosModel
   def calculate_group_permissions
     self.class.install_view('permission')
 
-    group_perms = {}
+    group_perms = {self.uuid => {:read => true, :write => true, :manage => true}}
     ActiveRecord::Base.connection.
-      exec_query('SELECT target_owner_uuid, max(perm_level)
+      exec_query('SELECT target_owner_uuid, max(perm_level), max(trashed)
                   FROM permission_view
                   WHERE user_uuid = $1
                   AND target_owner_uuid IS NOT NULL
@@ -185,7 +185,7 @@ class User < ArvadosModel
                   "group_permissions for #{uuid}",
                   # "binds" arg is an array of [col_id, value] for '$1' vars:
                   [[nil, uuid]],
-                  ).rows.each do |group_uuid, max_p_val|
+                ).rows.each do |group_uuid, max_p_val, trashed|
       group_perms[group_uuid] = PERMS_FOR_VAL[max_p_val.to_i]
     end
     Rails.cache.write "#{PERM_CACHE_PREFIX}#{self.uuid}", group_perms, expires_in: PERM_CACHE_TTL
diff --git a/services/api/lib/create_permission_view.sql b/services/api/lib/create_permission_view.sql
index 6e24d30..10c4b27 100644
--- a/services/api/lib/create_permission_view.sql
+++ b/services/api/lib/create_permission_view.sql
@@ -2,6 +2,21 @@
 --
 -- SPDX-License-Identifier: AGPL-3.0
 
+-- constructing perm_edges
+--   1. get the list of all permission links,
+--   2. any can_manage link or permission link to a group means permission should "follow through"
+--      (as a special case, can_manage links to a user grant access to everything owned by the user,
+--       unlike can_read or can_write which only grant access to the user record)
+--   3. add all owner->owned relationships between groups as can_manage edges
+--
+-- constructing permissions
+--   1. base case: start with set of all users as the working set
+--   2. recursive case:
+--      join with edges where the tail is in the working set and "follow" is true
+--      produce a new working set with the head (target) of each edge
+--      set permission to the least permission encountered on the path
+--      propagate trashed flag down
+
 CREATE TEMPORARY VIEW permission_view AS
 WITH RECURSIVE
 perm_value (name, val) AS (
@@ -11,35 +26,41 @@ perm_value (name, val) AS (
      ('can_write',  2),
      ('can_manage', 3)
      ),
-perm_edges (tail_uuid, head_uuid, val, follow) AS (
+perm_edges (tail_uuid, head_uuid, val, follow, trashed) AS (
        SELECT links.tail_uuid,
               links.head_uuid,
               pv.val,
-              (pv.val = 3 OR groups.uuid IS NOT NULL) AS follow
+              (pv.val = 3 OR groups.uuid IS NOT NULL) AS follow,
+              0::smallint AS trashed
               FROM links
               LEFT JOIN perm_value pv ON pv.name = links.name
               LEFT JOIN groups ON pv.val<3 AND groups.uuid = links.head_uuid
               WHERE links.link_class = 'permission'
        UNION ALL
-       SELECT owner_uuid, uuid, 3, true FROM groups
+       SELECT owner_uuid, uuid, 3, true, 0::smallint FROM groups
        ),
-perm (val, follow, user_uuid, target_uuid) AS (
+perm (val, follow, user_uuid, target_uuid, trashed, startnode) AS (
      SELECT 3::smallint             AS val,
-            true                    AS follow,
+            false                   AS follow,
             users.uuid::varchar(32) AS user_uuid,
-            users.uuid::varchar(32) AS target_uuid
+            users.uuid::varchar(32) AS target_uuid,
+            0::smallint             AS trashed,
+            true                    AS startnode
             FROM users
      UNION
-     SELECT LEAST(perm.val, edges.val)::smallint AS val,
-            edges.follow                         AS follow,
-            perm.user_uuid::varchar(32)          AS user_uuid,
-            edges.head_uuid::varchar(32)         AS target_uuid
+     SELECT LEAST(perm.val, edges.val)::smallint  AS val,
+            edges.follow                          AS follow,
+            perm.user_uuid::varchar(32)           AS user_uuid,
+            edges.head_uuid::varchar(32)          AS target_uuid,
+            GREATEST(perm.trashed, edges.trashed)::smallint AS trashed,
+            false                                 AS startnode
             FROM perm
             INNER JOIN perm_edges edges
-            ON perm.follow AND edges.tail_uuid = perm.target_uuid
+            ON (perm.startnode or perm.follow) AND edges.tail_uuid = perm.target_uuid
 )
 SELECT user_uuid,
        target_uuid,
        val AS perm_level,
-       CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid
+       CASE follow WHEN true THEN target_uuid ELSE NULL END AS target_owner_uuid,
+       trashed
        FROM perm;

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


hooks/post-receive
-- 




More information about the arvados-commits mailing list