Skip to content

Commit 669ba4f

Browse files
committed
ensure id is valid in grade summary page
fixes CNVS-1869 test plan: - as a teacher, go to a student's grade summary page - in the url, replace the student's id with 'banana' or 'lqw' (e.g. /courses/1/grades/banana) - you should get a Page Not Found error page - navigate to /courses/:id/grades and the page should redirect you to gradebook Change-Id: Ic7d2b3de463615957a8d4178544ab19b7e651f60 Reviewed-on: https://gerrit.instructure.com/29215 Tested-by: Jenkins <jenkins@instructure.com> QA-Review: Caleb Guanzon <cguanzon@instructure.com> Reviewed-by: Simon Williams <simon@instructure.com> Product-Review: Simon Williams <simon@instructure.com>
1 parent 3714020 commit 669ba4f

4 files changed

Lines changed: 16 additions & 3 deletions

File tree

app/controllers/rubric_assessments_controller.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -79,7 +79,7 @@ def update
7979
# only check if there's no @assessment object, since that's the only time
8080
# this param matters (assessing_user_id and arg find_asset_for_assessment)
8181
user_id = params[:rubric_assessment][:user_id]
82-
if !@assessment && user_id !~ /\A\d+\Z/
82+
if !@assessment && user_id !~ Api::ID_REGEX
8383
raise ActiveRecord::RecordNotFound
8484
end
8585

app/controllers/rubrics_controller.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ def index
3333

3434
def show
3535
return unless authorized_action(@context, @current_user, :manage)
36-
if (id = params[:id]) =~ /\A\d+\Z/
36+
if (id = params[:id]) =~ Api::ID_REGEX
3737
js_env :ROOT_OUTCOME_GROUP => get_root_outcome
3838
@rubric_association = @context.rubric_associations.bookmarked.find_by_rubric_id(params[:id])
3939
raise ActiveRecord::RecordNotFound unless @rubric_association

app/presenters/grade_summary_presenter.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,7 @@ def selectable_courses
6060
def student_enrollment
6161
@student_enrollment ||= begin
6262
if @id_param # always use id if given
63+
validate_id
6364
user_id = Shard.relative_id_for(@id_param, @context.shard)
6465
@context.all_student_enrollments.find_by_user_id(user_id)
6566
elsif observed_students.present? # otherwise try to find an observed student
@@ -70,6 +71,11 @@ def student_enrollment
7071
end
7172
end
7273

74+
def validate_id
75+
raise ActiveRecord::RecordNotFound if ( !@id_param.is_a?(User) && (@id_param.to_s =~ Api::ID_REGEX).nil? )
76+
true
77+
end
78+
7379
def student
7480
@student ||= (student_enrollment && student_enrollment.user)
7581
end

spec/controllers/gradebooks_controller_spec.rb

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -62,7 +62,7 @@
6262
assert_unauthorized
6363
end
6464

65-
it" should allow access for a linked observer" do
65+
it "should allow access for a linked observer" do
6666
course_with_student(:active_all => true)
6767
@student = @user
6868
user(:active_all => true)
@@ -331,6 +331,13 @@ def check_grades_page(due_at)
331331
check_grades_page(@due_at + 1.day)
332332
end
333333
end
334+
335+
it "should raise an exception on a non-integer :id" do
336+
course_with_teacher_logged_in(:active_all => true)
337+
expect {
338+
get 'grade_summary', :course_id => @course.id, :id => "lqw"
339+
}.to raise_error(ActiveRecord::RecordNotFound)
340+
end
334341
end
335342

336343
describe "GET 'show'" do

0 commit comments

Comments
 (0)