ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

allow imported sis csvs to be downloaded 

after filtering out passwords of course

test plan:
* with the sis imports refactor feature flag on,
 upload some zips and csvs to sis imports,
 including some users.csv files with passwords
* view the sis imports API index and show
 endpoints
* should be able to use the urls in the new
 "csv_attachments" attribute to download versions
 of the imported files without passwords

closes #CORE-1655

Change-Id: I31e34d42f4abf2597efd6066dabea6f230632855
Reviewed-on: https://gerrit.instructure.com/158899
Tested-by: Jenkins
QA-Review: Jeremy Putnam <jeremyp@instructure.com>
Reviewed-by: Rob Orton <rob@instructure.com>
Product-Review: Rob Orton <rob@instructure.com>
This commit is contained in:
James Williams 2018-07-27 07:18:24 -06:00
parent 617bdeb12e
commit 41e901b7a1
9 changed files with 120 additions and 17 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
app/controllers/sis_imports_api_controller.rb
View File

@ -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

5
app/models/attachment.rb
View File

@ -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

22
app/models/sis_batch.rb
View File

@ -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

14
lib/api/v1/attachment.rb
View File

@ -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
)
if skip_permission_checks
hash['locked_for_user'] = false
else
locked_json(hash, attachment, user, 'file')
end
if includes.include? 'user'
context = attachment.context

6
lib/api/v1/sis_import.rb
View File

@ -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

4
lib/api/v1/submission.rb
View File

@ -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

24
lib/sis/csv/import_refactored.rb
View File

@ -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

23
spec/apis/v1/sis_imports_api_spec.rb
View File

@ -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",

27
spec/models/sis_batch_spec.rb
View File

@ -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