diff --git a/app/controllers/sis_imports_api_controller.rb b/app/controllers/sis_imports_api_controller.rb index 5b72a5c0e23..4ab63135a23 100644 --- a/app/controllers/sis_imports_api_controller.rb +++ b/app/controllers/sis_imports_api_controller.rb @@ -239,6 +239,15 @@ # "description": "The ID of the SIS Import that this import was diffed against", # "example": 1, # "type": "integer" +# }, +# "csv_attachments": { +# "description": "An array of CSV files for processing", +# "example": [], +# "type": "array", +# "items": { +# "type": "array", +# "items": {"$ref": "File"} +# } # } # } # } @@ -593,5 +602,4 @@ class SisImportsApiController < ApplicationController render json: {aborted: true} end end - end diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 8a6a386d4cd..2c88a2eca9d 100644 --- a/app/models/attachment.rb +++ b/app/models/attachment.rb @@ -1197,10 +1197,6 @@ class Attachment < ActiveRecord::Base can :attach_to_submission_comment end - # checking if an attachment is locked is expensive and pointless for - # submission attachments - attr_writer :skip_submission_attachment_lock_checks - # prevent an access attempt shortly before unlock_at from caching permissions beyond that time def touch_on_unlock Shackles.activate(:master) do @@ -1210,7 +1206,6 @@ class Attachment < ActiveRecord::Base end def locked_for?(user, opts={}) - return false if @skip_submission_attachment_lock_checks return false if opts[:check_policies] && self.grants_right?(user, :read_as_admin) return {:asset_string => self.asset_string, :manually_locked => true} if self.locked || Folder.is_locked?(self.folder_id) Rails.cache.fetch(locked_cache_key(user), :expires_in => 1.minute) do diff --git a/app/models/sis_batch.rb b/app/models/sis_batch.rb index 5b5c0cef5c5..bee5116bea5 100644 --- a/app/models/sis_batch.rb +++ b/app/models/sis_batch.rb @@ -749,4 +749,26 @@ class SisBatch < ActiveRecord::Base RETURNING t.id SQL end + + attr_writer :downloadable_attachments + def self.load_downloadable_attachments(batches) + batches = Array(batches) + all_ids = batches.map{|sb| sb.data&.dig(:downloadable_attachment_ids) || []}.flatten + all_attachments = all_ids.any? ? Attachment.where(:context_type => self.name, :context_id => batches, :id => all_ids).to_a.group_by(&:context_id) : {} + batches.each do |b| + b.downloadable_attachments = all_attachments[b.id] || [] + end + end + + def downloadable_attachments + @downloadable_attachments ||= + begin + ids = data[:downloadable_attachment_ids] + if ids.present? + self.shard.activate { Attachment.where(:id => ids).polymorphic_where(:context => self).to_a } + else + [] + end + end + end end diff --git a/lib/api/v1/attachment.rb b/lib/api/v1/attachment.rb index 0908377cf5a..b67d37cd60d 100644 --- a/lib/api/v1/attachment.rb +++ b/lib/api/v1/attachment.rb @@ -46,18 +46,18 @@ module Api::V1::Attachment } return hash if options[:only] && options[:only].include?('names') - options.reverse_merge!(submission_attachment: false) + options.reverse_merge!(skip_permission_checks: false) includes = options[:include] || [] # it takes loads of queries to figure out that a teacher doesn't have # :update permission on submission attachments. we'll handle the # permissions ourselves instead of using the usual stuff to save thousands # of queries - submission_attachment = options[:submission_attachment] + skip_permission_checks = options[:skip_permission_checks] # this seems like a stupid amount of branching but it avoids expensive # permission checks - hidden_for_user = if submission_attachment + hidden_for_user = if skip_permission_checks false elsif !attachment.hidden? false @@ -67,7 +67,7 @@ module Api::V1::Attachment !can_view_hidden_files?(attachment.context, user) end - downloadable = !attachment.locked_for?(user, check_policies: true) + downloadable = skip_permission_checks || !attachment.locked_for?(user, check_policies: true) if downloadable # using the multi-parameter form because not every class that mixes in @@ -97,7 +97,7 @@ module Api::V1::Attachment 'updated_at' => attachment.updated_at, 'unlock_at' => attachment.unlock_at, 'locked' => !!attachment.locked, - 'hidden' => submission_attachment ? false : !!attachment.hidden?, + 'hidden' => skip_permission_checks ? false : !!attachment.hidden?, 'lock_at' => attachment.lock_at, 'hidden_for_user' => hidden_for_user, 'thumbnail_url' => thumbnail_url, @@ -105,7 +105,11 @@ module Api::V1::Attachment 'mime_class' => attachment.mime_class, 'media_entry_id' => attachment.media_entry_id ) - locked_json(hash, attachment, user, 'file') + if skip_permission_checks + hash['locked_for_user'] = false + else + locked_json(hash, attachment, user, 'file') + end if includes.include? 'user' context = attachment.context diff --git a/lib/api/v1/sis_import.rb b/lib/api/v1/sis_import.rb index 87b9f0b76c5..5c155a807fb 100644 --- a/lib/api/v1/sis_import.rb +++ b/lib/api/v1/sis_import.rb @@ -23,6 +23,7 @@ module Api::V1::SisImport include Api::V1::SisImportError def sis_imports_json(batches, user, session) + SisBatch.load_downloadable_attachments(batches) batches.map do |f| sis_import_json(f, user, session) end @@ -32,16 +33,17 @@ module Api::V1::SisImport json = api_json(batch, user, session) if batch.errors_attachment_id # skip permission checks since the context is a sis_import it will fail permission checks - batch.errors_attachment.skip_submission_attachment_lock_checks = true json[:errors_attachment] = attachment_json( batch.errors_attachment, user, {}, # skip permission checks since the context is a sis_import it will fail permission checks - {submission_attachment: true} + {skip_permission_checks: true} ) end json[:user] = user_json(batch.user, user, session) if batch.user + atts = batch.downloadable_attachments + json[:csv_attachments] = attachments_json(atts, user, {}, {skip_permission_checks: true}) if atts.any? json end end diff --git a/lib/api/v1/submission.rb b/lib/api/v1/submission.rb index 7fc9b60a172..7f668c97f81 100644 --- a/lib/api/v1/submission.rb +++ b/lib/api/v1/submission.rb @@ -171,17 +171,15 @@ module Api::V1::Submission attachments = attempt.versioned_attachments.dup attachments << attempt.attachment if attempt.attachment && attempt.attachment.context_type == 'Submission' && attempt.attachment.context_id == attempt.id hash['attachments'] = attachments.map do |attachment| - attachment.skip_submission_attachment_lock_checks = true includes = includes.include?('canvadoc_document_id') ? ['preview_url', 'canvadoc_document_id'] : ['preview_url'] atjson = attachment_json(attachment, user, {}, - submission_attachment: true, + skip_permission_checks: true, include: includes, enable_annotations: true, # we want annotations on submission's attachment preview_urls moderated_grading_whitelist: attempt.moderated_grading_whitelist, enrollment_type: user_type(context, user), anonymous_instructor_annotations: assignment.anonymous_instructor_annotations? ) - attachment.skip_submission_attachment_lock_checks = false atjson end.compact unless attachments.blank? end diff --git a/lib/sis/csv/import_refactored.rb b/lib/sis/csv/import_refactored.rb index 9d86fda29eb..5d8c9ae2c0f 100644 --- a/lib/sis/csv/import_refactored.rb +++ b/lib/sis/csv/import_refactored.rb @@ -38,6 +38,8 @@ module SIS xlist user enrollment admin group_category group group_membership grade_publishing_results user_observer}.freeze + HEADERS_TO_EXCLUDE_FOR_DOWNLOAD = %w{password ssha_password}.freeze + def initialize(root_account, opts = {}) opts = opts.with_indifferent_access @root_account = root_account @@ -84,6 +86,7 @@ module SIS def prepare @tmp_dirs = [] + @batch.data[:downloadable_attachment_ids] ||= [] @files.each do |file| if File.file?(file) if File.extname(file).downcase == '.zip' @@ -336,6 +339,12 @@ module SIS row.each(&:downcase!) importer = IMPORTERS.index do |type| if SIS::CSV.const_get(type.to_s.camelcase + 'Importer').send(type.to_s + '_csv?', row) + if type == :user && (row & HEADERS_TO_EXCLUDE_FOR_DOWNLOAD).any? + filtered_att = create_filtered_csv(csv, row) + @batch.data[:downloadable_attachment_ids] << filtered_att.id + else + @batch.data[:downloadable_attachment_ids] << att.id + end @csvs[type] << csv @headers[type].merge(row) true @@ -385,6 +394,21 @@ module SIS rescue Iconv::Failure false end + + def create_filtered_csv(csv, headers) + Dir.mktmpdir do |tmp_dir| + path = File.join(tmp_dir, File.basename(csv[:fullpath]).sub(/\.csv$/i, "_filtered.csv")) + new_csv = ::CSV.open(path, 'wb', headers: headers - HEADERS_TO_EXCLUDE_FOR_DOWNLOAD, write_headers: true) + ::CSV.foreach(csv[:fullpath], CSVBaseImporter::PARSE_ARGS) do |row| + HEADERS_TO_EXCLUDE_FOR_DOWNLOAD.each do |header| + row.delete(header) + end + new_csv << row + end + new_csv.close + create_batch_attachment(path) + end + end end end end diff --git a/spec/apis/v1/sis_imports_api_spec.rb b/spec/apis/v1/sis_imports_api_spec.rb index b3559f0aa53..84070eacf0e 100644 --- a/spec/apis/v1/sis_imports_api_spec.rb +++ b/spec/apis/v1/sis_imports_api_spec.rb @@ -101,6 +101,8 @@ describe SisImportsApiController, type: :request do expect(json.has_key?("started_at")).to eq true json.delete("started_at") json.delete("user") + json.delete("csv_attachments") + json['data'].delete("downloadable_attachment_ids") batch = SisBatch.last expect(json).to eq({ "data" => { "import_type"=>"instructure_csv"}, @@ -139,6 +141,8 @@ describe SisImportsApiController, type: :request do expect(json.has_key?("started_at")).to eq true json.delete("started_at") json.delete("user") + json.delete("csv_attachments") + json["data"].delete("downloadable_attachment_ids") expected_data = { "data" => { "import_type" => "instructure_csv", "completed_importers" => ["user"], @@ -669,6 +673,8 @@ describe SisImportsApiController, type: :request do json["sis_imports"].first.delete("ended_at") json["sis_imports"].first.delete("started_at") json["sis_imports"].first.delete("user") + json["sis_imports"].first.delete("csv_attachments") + json["sis_imports"].first['data'].delete("downloadable_attachment_ids") expected_data = {"sis_imports"=>[{ "data" => { "import_type" => "instructure_csv", @@ -714,6 +720,23 @@ describe SisImportsApiController, type: :request do expect(links.first[:uri].path).to eq api_v1_account_sis_imports_path end + it "should return downloadable attachments if available" do + batch = post_csv( + "user_id,password,login_id,status,ssha_password", + "user_1,supersecurepwdude,user_1,active,hunter2" + ) + + run_jobs + json = api_call(:get, "/api/v1/accounts/#{@account.id}/sis_imports.json", + { :controller => 'sis_imports_api', :action => 'index', + :format => 'json', :account_id => @account.id.to_s }) + + atts_json = json["sis_imports"].first["csv_attachments"] + expect(atts_json.count).to eq 1 + expect(atts_json.first["id"]).to eq batch.downloadable_attachments.last.id + expect(atts_json.first["url"]).to be_present + end + it "should filter sis imports by date if requested" do batch = @account.sis_batches.create json = api_call(:get, "/api/v1/accounts/#{@account.id}/sis_imports.json", diff --git a/spec/models/sis_batch_spec.rb b/spec/models/sis_batch_spec.rb index a28f70a9976..751047144f1 100644 --- a/spec/models/sis_batch_spec.rb +++ b/spec/models/sis_batch_spec.rb @@ -85,6 +85,33 @@ describe SisBatch do expect(batch.parallel_importers.pluck(:importer_type)).to match_array %w(course user) end + it 'should create filtered versions of csvs with passwords' do + batch = process_csv_data([%{user_id,password,login_id,status,ssha_password + user_1,supersecurepwdude,user_1,active,hunter2}]) + expect(batch).to be_imported + atts = batch.downloadable_attachments + expect(atts.count).to eq 1 + + atts.first.open do |file| + @row = ::CSV.new(file, :headers => true).first.to_h + end + expect(@row).to eq({"user_id" => "user_1", "login_id" => "user_1", "status" => "active"}) + end + + it 'should be able to preload downloadable attachments' do + batch1 = process_csv_data([%{user_id,password,login_id,status,ssha_password + user_1,supersecurepwdude,user_1,active,hunter2}, + %{course_id,short_name,long_name,term_id,status + course_1,course_1,course_1,term_1,active}]) + batch2 = @account.sis_batches.create! + SisBatch.load_downloadable_attachments([batch1, batch2]) + + expect(batch2.instance_variable_get(:@downloadable_attachments)).to eq [] + atts = batch1.instance_variable_get(:@downloadable_attachments) + expect(atts.count).to eq 2 + expect(atts.map(&:id)).to match_array(batch1.data[:downloadable_attachment_ids]) + end + it "should keep the batch in initializing state during create_with_attachment" do batch = SisBatch.create_with_attachment(@account, 'instructure_csv', stub_file_data('test.csv', 'abc', 'text'), user_factory) do |b| expect(b.attachment).not_to be_new_record