diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 575502db41f..e38293e24ad 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -182,11 +182,13 @@ class AccountsController < ApplicationController end return redirect_to account_settings_url(@account) if @account.site_admin? || !@account.grants_right?(@current_user, :read_course_list) js_env(:ACCOUNT_COURSES_PATH => account_courses_path(@account, :format => :json)) - load_course_right_side + include_crosslisted_courses = value_to_boolean(params[:include_crosslisted_courses]) + load_course_right_side(:include_crosslisted_courses => include_crosslisted_courses) @courses = @account.fast_all_courses(:term => @term, :limit => @maximum_courses_im_gonna_show, :hide_enrollmentless_courses => @hide_enrollmentless_courses, :only_master_courses => @only_master_courses, - :order => sort_order) + :order => sort_order, + :include_crosslisted_courses => include_crosslisted_courses) ActiveRecord::Associations::Preloader.new.preload(@courses, :enrollment_term) build_course_stats end @@ -359,7 +361,8 @@ class AccountsController < ApplicationController order += (params[:order] == "desc" ? " DESC, id DESC" : ", id") end - @courses = @account.associated_courses.order(order).where(:workflow_state => params[:state]) + opts = { :include_crosslisted_courses => value_to_boolean(params[:include_crosslisted_courses]) } + @courses = @account.associated_courses(opts).order(order).where(:workflow_state => params[:state]) if params[:hide_enrollmentless_courses] || value_to_boolean(params[:with_enrollments]) @courses = @courses.with_enrollments @@ -870,7 +873,7 @@ class AccountsController < ApplicationController end end - def load_course_right_side + def load_course_right_side(opts = {}) @root_account = @account.root_account @maximum_courses_im_gonna_show = 50 @term = nil @@ -878,7 +881,7 @@ class AccountsController < ApplicationController @term = @root_account.enrollment_terms.active.find(params[:enrollment_term_id]) rescue nil @term ||= @root_account.enrollment_terms.active[-1] end - associated_courses = @account.associated_courses.active + associated_courses = @account.associated_courses(opts).active associated_courses = associated_courses.for_term(@term) if @term @associated_courses_count = associated_courses.count @hide_enrollmentless_courses = params[:hide_enrollmentless_courses] == "1" @@ -1192,7 +1195,7 @@ class AccountsController < ApplicationController :app_center_access_token].freeze def permitted_account_attributes - [:name, :turnitin_account_id, :turnitin_shared_secret, + [:name, :turnitin_account_id, :turnitin_shared_secret, :include_crosslisted_courses, :turnitin_host, :turnitin_comments, :turnitin_pledge, :turnitin_originality, :default_time_zone, :parent_account, :default_storage_quota, :default_storage_quota_mb, :storage_quota, :default_locale, diff --git a/app/models/account.rb b/app/models/account.rb index 17e6287d17a..57d5d6e8d2c 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -459,13 +459,18 @@ class Account < ActiveRecord::Base @cached_users_name_like[query] ||= self.fast_all_users.name_like(query) end - def associated_courses + def associated_courses(opts = {}) if root_account? all_courses else shard.activate do - Course.where("EXISTS (?)", CourseAccountAssociation.where(account_id: self, course_section_id: nil) - .where("course_id=courses.id")) + if opts[:include_crosslisted_courses] + Course.where("EXISTS (?)", CourseAccountAssociation.where(account_id: self) + .where("course_id=courses.id")) + else + Course.where("EXISTS (?)", CourseAccountAssociation.where(account_id: self, course_section_id: nil) + .where("course_id=courses.id")) + end end end end @@ -477,7 +482,10 @@ class Account < ActiveRecord::Base def fast_course_base(opts = {}) opts[:order] ||= "#{Course.best_unicode_collation_key("courses.name")} ASC" columns = "courses.id, courses.name, courses.workflow_state, courses.course_code, courses.sis_source_id, courses.enrollment_term_id" - associated_courses = self.associated_courses.active.order(opts[:order]) + associated_courses = self.associated_courses( + :include_crosslisted_courses => opts[:include_crosslisted_courses] + ) + associated_courses = associated_courses.active.order(opts[:order]) associated_courses = associated_courses.with_enrollments if opts[:hide_enrollmentless_courses] associated_courses = associated_courses.master_courses if opts[:only_master_courses] associated_courses = associated_courses.for_term(opts[:term]) if opts[:term].present? diff --git a/config/brakeman.ignore b/config/brakeman.ignore index 6730b93617b..8ec101937de 100644 --- a/config/brakeman.ignore +++ b/config/brakeman.ignore @@ -75,6 +75,28 @@ }, "user_input": "Enrollment.active_student_conditions", "confidence": "High" + }, + { + "warning_type": "SQL Injection", + "warning_code": 0, + "fingerprint": "d8e0d77e896b3da629e04233f458668fff1d130a6b969287822c292af1daeaad", + "check_name": "SQL", + "message": "Possible SQL injection", + "file": "app/models/account.rb", + "line": 486, + "link": "https://brakemanscanner.org/docs/warning_types/sql_injection/", + "code": "self.associated_courses(:include_crosslisted_courses => opts[:include_crosslisted_courses]).active.order(\"#{Course.best_unicode_collation_key(\"courses.name\")} ASC\")", + "render_path": null, + "location": { + "type": "method", + "class": "Account", + "method": "fast_course_base" + }, + "user_input": "Course.best_unicode_collation_key(\"courses.name\")", + "confidence": "High", + "note": "No user input is passed in to via opts['order'] in this function (see accounts_controller:sort_order)" } - ] + ], + "updated": "2017-12-19 10:36:25 -0700", + "brakeman_version": "4.1.0" } diff --git a/spec/apis/v1/accounts_api_spec.rb b/spec/apis/v1/accounts_api_spec.rb index 1a17541797a..57522190c3e 100644 --- a/spec/apis/v1/accounts_api_spec.rb +++ b/spec/apis/v1/accounts_api_spec.rb @@ -696,8 +696,28 @@ describe "Accounts API", type: :request do @account1.account_users.create!(user: @user) json = api_call(:get, "/api/v1/accounts/#{@account1.id}/courses", { :controller => 'accounts', :action => 'courses_api', - :account_id => @account1.to_param, :format => 'json', - }) + :account_id => @account1.to_param, :format => 'json' }) + expect(json.length).to eq 1 + expect(json.first["name"]).to eq "course1" + end + + it "include crosslisted course when querying account section was crosslisted from if requested" do + @account2.account_users.create!(user: @user) + json = api_call(:get, "/api/v1/accounts/#{@account2.id}/courses?include_crosslisted_courses=true", + { :controller => 'accounts', :action => 'courses_api', + :include_crosslisted_courses => true, + :account_id => @account2.to_param, :format => 'json' }) + expect(json.length).to eq 2 + names = json.pluck("name") + expect(names.include?("course1")).to be_truthy + expect(names.include?("course2")).to be_truthy + end + + it "don't include crosslisted course when querying account section was crosslisted to even if requested" do + @account1.account_users.create!(user: @user) + json = api_call(:get, "/api/v1/accounts/#{@account1.id}/courses", + { :controller => 'accounts', :action => 'courses_api', + :account_id => @account1.to_param, :format => 'json' }) expect(json.length).to eq 1 expect(json.first["name"]).to eq "course1" end diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index 1108a0087b9..8224dba3452 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -228,6 +228,18 @@ describe AccountsController do get 'show', params: {:id => @account1.id }, :format => 'html' expect(assigns[:associated_courses_count]).to eq 1 end + + it "if crosslisted a section to another account, do show other if that param is not set" do + account_with_admin_logged_in(account: @account2) + get 'show', params: {:id => @account2.id, :include_crosslisted_courses => true}, :format => 'html' + expect(assigns[:associated_courses_count]).to eq 2 + end + + it "if crosslisted a section to this account, do *not* show other account's course even if param is not set" do + account_with_admin_logged_in(account: @account1) + get 'show', params: {:id => @account1.id, :include_crosslisted_courses => true}, :format => 'html' + expect(assigns[:associated_courses_count]).to eq 1 + end end # Check that both published and un-published courses have the correct count diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index a1ff162c91a..80b8c15e06f 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -141,9 +141,9 @@ describe Account do expect(@account.fast_all_courses({:term => EnrollmentTerm.where(sis_source_id: "T003").first}).map(&:sis_source_id).sort).to eq ["C005", "C006", "C007", "C008", "C009", "C005S", "C006S", "C007S", "C008S", "C009S"].sort end - it "don't double-count cross-listed courses" do - def check_account(account, expected_length, expected_course_names) - actual_courses = account.fast_all_courses + it "counting cross-listed courses only if requested" do + def check_account(account, include_crosslisted_courses, expected_length, expected_course_names) + actual_courses = account.fast_all_courses({ :include_crosslisted_courses => include_crosslisted_courses }) expect(actual_courses.length).to eq expected_length actual_course_names = actual_courses.pluck("name").sort! expect(actual_course_names).to eq(expected_course_names.sort!) @@ -156,8 +156,10 @@ describe Account do course_b = course_factory({ :account => account_b, :course_name => "course_b" }) course_b.course_sections.create!({ :name => "section_b" }) course_b.course_sections.first.crosslist_to_course(course_a) - check_account(account_a, 1, ["course_a"]) - check_account(account_b, 1, ["course_b"]) + check_account(account_a, false, 1, ["course_a"]) + check_account(account_a, true, 1, ["course_a"]) + check_account(account_b, false, 1, ["course_b"]) + check_account(account_b, true, 2, ["course_a", "course_b"]) end it "should list associated nonenrollmentless courses" do