X-Git-Url: https://git.arvados.org/arvados.git/blobdiff_plain/a3aee2781cfcd006fa1b7ce3cfeeb1dd2d53c270..7f88afd565b76903ad4b27fb896ff0cd844dfb7f:/services/api/app/models/arvados_model.rb diff --git a/services/api/app/models/arvados_model.rb b/services/api/app/models/arvados_model.rb index a27d6aba46..00934322d2 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -16,6 +16,7 @@ class ArvadosModel < ApplicationRecord include DbCurrentTime extend RecordFilters + after_find :schedule_restoring_changes after_initialize :log_start_state before_save :ensure_permission_to_save before_save :ensure_owner_uuid_is_permitted @@ -137,6 +138,7 @@ class ArvadosModel < ApplicationRecord def reload(*args) super log_start_state + self end def self.create raw_params={}, *args @@ -284,6 +286,7 @@ class ArvadosModel < ApplicationRecord sql_conds = nil user_uuids = users_list.map { |u| u.uuid } + all_user_uuids = [] # For details on how the trashed_groups table is constructed, see # see db/migrate/20200501150153_permission_table.rb @@ -294,44 +297,61 @@ class ArvadosModel < ApplicationRecord exclude_trashed_records = "AND (#{sql_table}.trash_at is NULL or #{sql_table}.trash_at > statement_timestamp())" end + trashed_check = "" + if !include_trash && sql_table != "api_client_authorizations" + trashed_check = "#{sql_table}.owner_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " + + "where trash_at <= statement_timestamp()) #{exclude_trashed_records}" + end + if users_list.select { |u| u.is_admin }.any? # Admin skips most permission checks, but still want to filter on trashed items. - if !include_trash - if sql_table != "api_client_authorizations" - # Only include records where the owner is not trashed - sql_conds = "#{sql_table}.owner_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} "+ - "where trash_at <= statement_timestamp()) #{exclude_trashed_records}" - end + if !include_trash && sql_table != "api_client_authorizations" + # Only include records where the owner is not trashed + sql_conds = trashed_check end else - trashed_check = "" - if !include_trash then - trashed_check = "AND target_uuid NOT IN (SELECT group_uuid FROM #{TRASHED_GROUPS} where trash_at <= statement_timestamp())" - end - # The core of the permission check is a join against the # materialized_permissions table to determine if the user has at # least read permission to either the object itself or its - # direct owner. See + # direct owner (if traverse_owned is true). See # db/migrate/20200501150153_permission_table.rb for details on # how the permissions are computed. + # A user can have can_manage access to another user, this grants + # full access to all that user's stuff. To implement that we + # need to include those other users in the permission query. + + # This was previously implemented by embedding the subquery + # directly into the query, but it was discovered later that this + # causes the Postgres query planner to do silly things because + # the query heuristics assumed the subquery would have a lot + # more rows that it does, and choose a bad merge strategy. By + # doing the query here and embedding the result as a constant, + # Postgres also knows exactly how many items there are and can + # choose the right query strategy. + # + # (note: you could also do this with a temporary table, but that + # would require all every request be wrapped in a transaction, + # which is not currently the case). + + all_user_uuids = ActiveRecord::Base.connection.exec_query %{ +#{USER_UUIDS_SUBQUERY_TEMPLATE % {user: "'#{user_uuids.join "', '"}'", perm_level: 1}} +}, + 'readable_by.user_uuids' + + user_uuids_subquery = ":user_uuids" + # Note: it is possible to combine the direct_check and - # owner_check into a single EXISTS() clause, however it turns + # owner_check into a single IN (SELECT) clause, however it turns # out query optimizer doesn't like it and forces a sequential - # table scan. Constructing the query with separate EXISTS() + # table scan. Constructing the query with separate IN (SELECT) # clauses enables it to use the index. # # see issue 13208 for details. - user_uuids_subquery = %{ -select target_uuid from materialized_permissions where user_uuid in (:user_uuids) -and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and perm_level >= 1 -} - # Match a direct read permission link from the user to the record uuid direct_check = "#{sql_table}.uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+ - "WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 1 #{trashed_check})" + "WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 1)" # Match a read permission for the user to the record's # owner_uuid. This is so we can have a permissions table that @@ -351,20 +371,35 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p # other user owns. owner_check = "" if sql_table != "api_client_authorizations" and sql_table != "groups" then - owner_check = "OR #{sql_table}.owner_uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+ - "WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 1 #{trashed_check} AND traverse_owned) " + owner_check = "#{sql_table}.owner_uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+ + "WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 1 AND traverse_owned) " + + # We want to do owner_check before direct_check in the OR + # clause. The order of the OR clause isn't supposed to + # matter, but in practice, it does -- apparently in the + # absence of other hints, it uses the ordering from the query. + # For certain types of queries (like filtering on owner_uuid), + # every item will match the owner_check clause, so then + # Postgres will optimize out the direct_check entirely. + direct_check = " OR " + direct_check end links_cond = "" 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. + # 1) Match permission links incoming or outgoing on the + # user, i.e. granting permission on the user, or granting + # permission to the user. + # + # 2) Match permission links which grant permission on an + # object that this user can_manage. + # links_cond = "OR (#{sql_table}.link_class IN (:permission_link_classes) AND "+ - "(#{sql_table}.head_uuid IN (#{user_uuids_subquery}) OR #{sql_table}.tail_uuid IN (#{user_uuids_subquery})))" + " ((#{sql_table}.head_uuid IN (#{user_uuids_subquery}) OR #{sql_table}.tail_uuid IN (#{user_uuids_subquery})) OR " + + " #{sql_table}.head_uuid IN (SELECT target_uuid FROM #{PERMISSION_VIEW} "+ + " WHERE user_uuid IN (#{user_uuids_subquery}) AND perm_level >= 3))) " end - sql_conds = "(#{direct_check} #{owner_check} #{links_cond}) #{exclude_trashed_records}" + sql_conds = "(#{owner_check} #{direct_check} #{links_cond}) #{trashed_check.empty? ? "" : "AND"} #{trashed_check}" end @@ -378,8 +413,8 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p end self.where(sql_conds, - user_uuids: user_uuids, - permission_link_classes: ['permission', 'resources']) + user_uuids: all_user_uuids.collect{|c| c["target_uuid"]}, + permission_link_classes: ['permission']) end def save_with_unique_name! @@ -452,7 +487,7 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p end def logged_attributes - attributes.except(*Rails.configuration.AuditLogs.UnloggedAttributes.keys) + attributes.except(*Rails.configuration.AuditLogs.UnloggedAttributes.stringify_keys.keys) end def self.full_text_searchable_columns @@ -574,6 +609,9 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p logger.warn "User #{current_user.uuid} tried to set ownership of #{self.class.to_s} #{self.uuid} but does not have permission to write #{which} owner_uuid #{check_uuid}" errors.add :owner_uuid, "cannot be set or changed without write permission on #{which} owner" raise PermissionDeniedError + elsif rsc_class == Group && Group.find_by_uuid(owner_uuid).group_class != "project" + errors.add :owner_uuid, "must be a project" + raise PermissionDeniedError end end else @@ -582,7 +620,7 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p # itself. if !current_user.can?(write: self.uuid) logger.warn "User #{current_user.uuid} tried to modify #{self.class.to_s} #{self.uuid} without write permission" - errors.add :uuid, "is not writable" + errors.add :uuid, " #{uuid} is not writable by #{current_user.uuid}" raise PermissionDeniedError end end @@ -622,7 +660,12 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p end def permission_to_destroy - permission_to_update + if [system_user_uuid, system_group_uuid, anonymous_group_uuid, + anonymous_user_uuid, public_project_uuid].include? uuid + false + else + permission_to_update + end end def maybe_update_modified_by_fields @@ -746,6 +789,20 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p %r/[a-z0-9]{5}-#{uuid_prefix}-[a-z0-9]{15}/ end + def check_readable_uuid attr, attr_value + return if attr_value.nil? + if (r = ArvadosModel::resource_class_for_uuid attr_value) + unless skip_uuid_read_permission_check.include? attr + r = r.readable_by(current_user) + end + if r.where(uuid: attr_value).count == 0 + errors.add(attr, "'#{attr_value}' not found") + end + else + # Not a valid uuid or PDH, but that (currently) is not an error. + end + end + def ensure_valid_uuids specials = [system_user_uuid] @@ -754,16 +811,7 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p next if skip_uuid_existence_check.include? attr attr_value = send attr next if specials.include? attr_value - if attr_value - if (r = ArvadosModel::resource_class_for_uuid attr_value) - unless skip_uuid_read_permission_check.include? attr - r = r.readable_by(current_user) - end - if r.where(uuid: attr_value).count == 0 - errors.add(attr, "'#{attr_value}' not found") - end - end - end + check_readable_uuid attr, attr_value end end end @@ -813,6 +861,45 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p nil end + # Fill in implied zero/false values in database records that were + # created before #17014 made them explicit, and reset the Rails + # "changed" state so the record doesn't appear to have been modified + # after loading. + # + # Invoked by Container and ContainerRequest models as an after_find + # hook. + def fill_container_defaults_after_find + fill_container_defaults + set_attribute_was('runtime_constraints', runtime_constraints) + set_attribute_was('scheduling_parameters', scheduling_parameters) + clear_changes_information + end + + # Fill in implied zero/false values. Invoked by ContainerRequest as + # a before_validation hook in order to (a) ensure every key has a + # value in the updated database record and (b) ensure the attribute + # whitelist doesn't reject a change from an explicit zero/false + # value in the database to an implicit zero/false value in an update + # request. + def fill_container_defaults + self.runtime_constraints = { + 'API' => false, + 'cuda' => { + 'device_count' => 0, + 'driver_version' => '', + 'hardware_capability' => '', + }, + 'keep_cache_ram' => 0, + 'ram' => 0, + 'vcpus' => 0, + }.merge(attributes['runtime_constraints'] || {}) + self.scheduling_parameters = { + 'max_run_time' => 0, + 'partitions' => [], + 'preemptible' => false, + }.merge(attributes['scheduling_parameters'] || {}) + end + # ArvadosModel.find_by_uuid needs extra magic to allow it to return # an object in any class. def self.find_by_uuid uuid @@ -830,10 +917,24 @@ and target_uuid like '_____-tpzed-_______________' and traverse_owned=true and p Rails.configuration.AuditLogs.MaxDeleteBatch.to_i > 0) end + def schedule_restoring_changes + # This will be checked at log_start_state, to reset any (virtual) changes + # produced by the act of reading a serialized attribute. + @fresh_from_database = true + end + def log_start_state if is_audit_logging_enabled? @old_attributes = Marshal.load(Marshal.dump(attributes)) @old_logged_attributes = Marshal.load(Marshal.dump(logged_attributes)) + if @fresh_from_database + # This instance was created from reading a database record. Attributes + # haven't been changed, but those serialized attributes will be reported + # as unpersisted, so we restore them to avoid issues with lock!() and + # with_lock(). + restore_attributes + @fresh_from_database = nil + end end end