X-Git-Url: https://git.arvados.org/arvados.git/blobdiff_plain/7499f61a2912cfdb1a316808fafa6e6ee77ee2e0..ba34a22d9918ae97306472c04701e69090821c82:/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 3966b7c393..1ff46c3616 100644 --- a/services/api/app/models/arvados_model.rb +++ b/services/api/app/models/arvados_model.rb @@ -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 @@ -246,6 +246,15 @@ class ArvadosModel < ApplicationRecord # If current user cannot write this object, just return # [self.owner_uuid]. def writable_by + # 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)) + # Return [] if nobody can write because this object is inside a + # frozen project + return [] if FrozenGroup.where(uuid: owner_uuid).any? return [owner_uuid] if not current_user unless (owner_uuid == current_user.uuid or current_user.is_admin or @@ -264,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 @@ -286,31 +315,45 @@ class ArvadosModel < ApplicationRecord sql_conds = nil user_uuids = users_list.map { |u| u.uuid } + all_user_uuids = [] + + admin = users_list.select { |u| u.is_admin }.any? # For details on how the trashed_groups table is constructed, see # see db/migrate/20200501150153_permission_table.rb - exclude_trashed_records = "" - if !include_trash and (sql_table == "groups" or sql_table == "collections") then - # Only include records that are not trashed - exclude_trashed_records = "AND (#{sql_table}.trash_at is NULL or #{sql_table}.trash_at > statement_timestamp())" + # 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 + excluded_trash = "(#{sql_table}.owner_uuid IN (SELECT group_uuid FROM #{TRASHED_GROUPS} " + + "WHERE trash_at <= statement_timestamp()))" + if sql_table == "groups" || sql_table == "collections" + excluded_trash = "(#{excluded_trash} OR #{sql_table}.trash_at <= statement_timestamp() IS TRUE)" + end + + if include_trash + # Exclude trash inside frozen projects + excluded_trash = "(#{excluded_trash} AND #{sql_table}.owner_uuid IN (SELECT uuid FROM #{FROZEN_GROUPS}))" + end 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 admin + # Admin skips most permission checks, but still want to filter + # 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})" 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 @@ -321,19 +364,38 @@ class ArvadosModel < ApplicationRecord # 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. - user_uuids_subquery = USER_UUIDS_SUBQUERY_TEMPLATE % {user: ":user_uuids", perm_level: 1} + + # 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. # 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 @@ -353,20 +415,43 @@ class ArvadosModel < ApplicationRecord # 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 + + if Rails.configuration.Users.RoleGroupsVisibleToAll && + sql_table == "groups" && + users_list.select { |u| u.is_active }.any? + # All role groups are readable (but we still need the other + # direct_check clauses to handle non-role groups). + direct_check += " OR #{sql_table}.group_class = 'role'" 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}) AND NOT (#{excluded_trash})" end @@ -380,8 +465,8 @@ class ArvadosModel < ApplicationRecord 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! @@ -393,12 +478,11 @@ class ArvadosModel < ApplicationRecord conn.exec_query 'SAVEPOINT save_with_unique_name' begin save! + conn.exec_query 'RELEASE SAVEPOINT save_with_unique_name' rescue ActiveRecord::RecordNotUnique => rn raise if max_retries == 0 max_retries -= 1 - conn.exec_query 'ROLLBACK TO SAVEPOINT save_with_unique_name' - # Dig into the error to determine if it is specifically calling out a # (owner_uuid, name) uniqueness violation. In this specific case, and # the client requested a unique name with ensure_unique_name==true, @@ -416,6 +500,8 @@ class ArvadosModel < ApplicationRecord detail = err.result.error_field(PG::Result::PG_DIAG_MESSAGE_DETAIL) raise unless /^Key \(owner_uuid, name\)=\([a-z0-9]{5}-[a-z0-9]{5}-[a-z0-9]{15}, .*?\) already exists\./.match detail + conn.exec_query 'ROLLBACK TO SAVEPOINT save_with_unique_name' + new_name = "#{name_was} (#{db_current_time.utc.iso8601(3)})" if new_name == name # If the database is fast enough to do two attempts in the @@ -433,10 +519,8 @@ class ArvadosModel < ApplicationRecord self[:current_version_uuid] = nil end end - conn.exec_query 'SAVEPOINT save_with_unique_name' + retry - ensure - conn.exec_query 'RELEASE SAVEPOINT save_with_unique_name' end end end @@ -573,8 +657,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" @@ -584,8 +672,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 @@ -602,7 +694,7 @@ class ArvadosModel < ApplicationRecord end def permission_to_create - current_user.andand.is_active + return current_user.andand.is_active end def permission_to_update @@ -668,7 +760,7 @@ class ArvadosModel < ApplicationRecord false end - def self.where_serialized(colname, value, md5: false) + def self.where_serialized(colname, value, md5: false, multivalue: false) colsql = colname.to_s if md5 colsql = "md5(#{colsql})" @@ -681,7 +773,16 @@ class ArvadosModel < ApplicationRecord sql = "#{colsql} IN (?)" sorted = deep_sort_hash(value) end - params = [sorted.to_yaml, SafeJSON.dump(sorted)] + params = [] + if multivalue + sorted.each do |v| + params << v.to_yaml + params << SafeJSON.dump(v) + end + else + params << sorted.to_yaml + params << SafeJSON.dump(sorted) + end if md5 params = params.map { |x| Digest::MD5.hexdigest(x) } end @@ -828,6 +929,50 @@ class ArvadosModel < ApplicationRecord 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 + # Make sure this is correctly sorted by key, because we merge in + # whatever is in the database on top of it, this will be the order + # that gets used downstream rather than the order the keys appear + # in the database. + self.runtime_constraints = { + 'API' => false, + 'cuda' => { + 'device_count' => 0, + 'driver_version' => '', + 'hardware_capability' => '', + }, + 'keep_cache_disk' => 0, + '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