19146: Remove unneeded special case checks, explain the needed one.
[arvados.git] / services / api / app / models / arvados_model.rb
index ffae867c158a5772f231962d5b6887ca7262a60c..c2725506c02ef75a85dee2a7c3a11fbd8db7e119 100644 (file)
@@ -220,7 +220,7 @@ class ArvadosModel < ApplicationRecord
   end
 
   def self.default_orders
-    ["#{table_name}.modified_at desc", "#{table_name}.uuid"]
+    ["#{table_name}.modified_at desc", "#{table_name}.uuid desc"]
   end
 
   def self.unique_columns
@@ -249,9 +249,9 @@ class ArvadosModel < ApplicationRecord
     # Return [] if this is a frozen project and the current user can't
     # unfreeze
     return [] if respond_to?(:frozen_by_uuid) && frozen_by_uuid &&
-                 !(Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
-                     current_user.andand.is_admin :
-                     current_user.can?(manage: uuid))
+                 (Rails.configuration.API.UnfreezeProjectRequiresAdmin ?
+                    !current_user.andand.is_admin :
+                    !current_user.can?(manage: uuid))
     # Return [] if nobody can write because this object is inside a
     # frozen project
     return [] if FrozenGroup.where(uuid: owner_uuid).any?
@@ -273,6 +273,26 @@ class ArvadosModel < ApplicationRecord
     end.compact.uniq
   end
 
+  def can_write
+    if respond_to?(:frozen_by_uuid) && frozen_by_uuid
+      # This special case is needed to return the correct value from a
+      # "freeze project" API, during which writable status changes
+      # from true to false.
+      #
+      # current_user.can?(write: self) returns true (which is correct
+      # in the context of permission-checking hooks) but the can_write
+      # value we're returning to the caller here represents the state
+      # _after_ the update, i.e., false.
+      return false
+    else
+      return current_user.can?(write: self)
+    end
+  end
+
+  def can_manage
+    return current_user.can?(manage: self)
+  end
+
   # Return a query with read permissions restricted to the union of the
   # permissions of the members of users_list, i.e. if something is readable by
   # any user in users_list, it will be readable in the query returned by this
@@ -302,6 +322,15 @@ class ArvadosModel < ApplicationRecord
     # For details on how the trashed_groups table is constructed, see
     # see db/migrate/20200501150153_permission_table.rb
 
+    # excluded_trash is a SQL expression that determines whether a row
+    # should be excluded from the results due to being trashed.
+    # Trashed items inside frozen projects are invisible to regular
+    # (non-admin) users even when using include_trash, so we have:
+    #
+    # (item_trashed || item_inside_trashed_project)
+    # &&
+    # (!caller_requests_include_trash ||
+    #  (item_inside_frozen_project && caller_is_not_admin))
     if (admin && include_trash) || sql_table == "api_client_authorizations"
       excluded_trash = "false"
     else
@@ -322,7 +351,7 @@ class ArvadosModel < ApplicationRecord
       # on trashed items.
       if !include_trash && sql_table != "api_client_authorizations"
         # Only include records where the owner is not trashed
-        sql_conds = "NOT #{excluded_trash}"
+        sql_conds = "NOT (#{excluded_trash})"
       end
     else
       # The core of the permission check is a join against the
@@ -422,7 +451,7 @@ class ArvadosModel < ApplicationRecord
                      "    WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 3))) "
       end
 
-      sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT #{excluded_trash}"
+      sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) AND NOT (#{excluded_trash})"
 
     end
 
@@ -629,8 +658,12 @@ class ArvadosModel < ApplicationRecord
         if check_uuid.nil?
           # old_owner_uuid is nil? New record, no need to check.
         elsif !current_user.can?(write: check_uuid)
-          logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
-          errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
+          if FrozenGroup.where(uuid: check_uuid).any?
+            errors.add :owner_uuid, "cannot be set or changed because #{which} owner is frozen"
+          else
+            logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}"
+            errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner"
+          end
           raise PermissionDeniedError
         elsif rsc_class == Group && Group.find_by_uuid(owner_uuid).group_class != "project"
           errors.add :owner_uuid, "must be a project"
@@ -640,8 +673,12 @@ class ArvadosModel < ApplicationRecord
     else
       # If the object already existed and we're not changing
       # owner_uuid, we only need write permission on the object
-      # itself.
-      if !current_user.can?(write: self.uuid)
+      # itself. (If we're in the act of unfreezing, we only need
+      # :unfreeze permission, which means "what write permission would
+      # be if target weren't frozen")
+      unless ((respond_to?(:frozen_by_uuid) && frozen_by_uuid_was && !frozen_by_uuid) ?
+                current_user.can?(unfreeze: uuid) :
+                current_user.can?(write: uuid))
         logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} without write permission"
         errors.add :uuid, " #{uuid} is not writable by #{current_user.uuid}"
         raise PermissionDeniedError
@@ -658,14 +695,7 @@ class ArvadosModel < ApplicationRecord
   end
 
   def permission_to_create
-    if !current_user.andand.is_active
-      return false
-    end
-    if self.respond_to?(:owner_uuid) && FrozenGroup.where(uuid: owner_uuid).any?
-      errors.add :owner_uuid, "#{owner_uuid} is frozen"
-      return false
-    end
-    return true
+    return current_user.andand.is_active
   end
 
   def permission_to_update
@@ -682,13 +712,6 @@ class ArvadosModel < ApplicationRecord
       logger.warn "User #{current_user.uuid} tried to change uuid of #{self.class.to_s} #{self.uuid_was} to #{self.uuid}"
       return false
     end
-    if self.respond_to?(:owner_uuid)
-      frozen = FrozenGroup.where(uuid: [owner_uuid, owner_uuid_in_database]).to_a
-      if frozen.any?
-        errors.add :uuid, "#{uuid} cannot be modified in frozen project #{frozen[0]}"
-        return false
-      end
-    end
     return true
   end