Skip to content

Commit c83d367

Browse files
jmirchh75jmirchh75
committed
fix(tasks): return 403 instead of 500 for unauthorized submission access
1 parent 04b2c8f commit c83d367

2 files changed

Lines changed: 186 additions & 2 deletions

File tree

‎app/api/tasks_api.rb‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -282,7 +282,7 @@ class TasksApi < Grape::API
282282
task_definition = project.unit.task_definitions.find(params[:task_definition_id])
283283

284284
# check the user can put this task
285-
error!(error: 'You do not have permission to read submissions for this project.') unless authorise? current_user, project, :get_submission
285+
error!({ error: 'You do not have permission to read submissions for this project.' }, 403) unless authorise? current_user, project, :get_submission
286286

287287
# ensure there can be a pdf...
288288
needs_upload_docs = !task_definition.upload_requirements.empty?
@@ -337,7 +337,7 @@ class TasksApi < Grape::API
337337
task_definition = project.unit.task_definitions.find(params[:task_definition_id])
338338

339339
# check the user can put this task
340-
error!(error: 'You do not have permission to read submissions for this project.') unless authorise? current_user, project, :get_submission
340+
error!({ error: 'You do not have permission to read submissions for this project.' }, 403) unless authorise? current_user, project, :get_submission
341341

342342
# Get the actual task...
343343
task = project.task_for_task_definition(task_definition)

‎test/api/submission_access_test.rb‎

Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
# frozen_string_literal: true
2+
3+
require 'test_helper'
4+
5+
# Access-control regression coverage for the submission_details and
6+
# submission_files endpoints in tasks_api.rb. Neither endpoint currently
7+
# has any dedicated test coverage on main, despite both serving another
8+
# student's submission data/files behind a single `authorise?` check.
9+
#
10+
# These tests do not change any application behaviour - they only assert
11+
# that the existing `authorise? current_user, project, :get_submission`
12+
# guard actually blocks the object-reference paths it's meant to.
13+
class SubmissionAccessTest < ActiveSupport::TestCase
14+
include Rack::Test::Methods
15+
include TestHelpers::AuthHelper
16+
include TestHelpers::JsonHelper
17+
18+
def app
19+
Rails.application
20+
end
21+
22+
def setup
23+
@unit = FactoryBot.create(:unit, perform_submissions: true, student_count: 3, staff_count: 1)
24+
@task_definition = @unit.task_definitions.first
25+
@owning_project = @unit.projects.first
26+
@other_project = @unit.projects.second
27+
28+
@convenor = @unit.main_convenor_user
29+
@tutor = FactoryBot.create(:user, :tutor)
30+
@unit.employ_staff(@tutor, Role.tutor)
31+
32+
@other_unit = FactoryBot.create(:unit, student_count: 1, staff_count: 1)
33+
@other_unit_task_definition = @other_unit.task_definitions.first
34+
end
35+
36+
def details_endpoint(project: @owning_project, task_definition: @task_definition)
37+
"/api/projects/#{project.id}/task_def_id/#{task_definition.id}/submission_details"
38+
end
39+
40+
def files_endpoint(project: @owning_project, task_definition: @task_definition)
41+
"/api/projects/#{project.id}/task_def_id/#{task_definition.id}/submission_files"
42+
end
43+
44+
# ---------------------------------------------------------------------
45+
# submission_details
46+
# ---------------------------------------------------------------------
47+
48+
def test_submission_details_allows_owning_student
49+
add_auth_header_for(user: @owning_project.student)
50+
51+
get details_endpoint
52+
53+
assert_equal 200, last_response.status
54+
assert last_response_body.key?('has_pdf')
55+
assert last_response_body.key?('processing_pdf')
56+
end
57+
58+
# A student is not a unit_role, so the claimed_by_unit_role_id key
59+
# (staff-only data about who has claimed the overflow task) should not
60+
# appear in their response at all.
61+
def test_submission_details_does_not_expose_claim_info_to_student
62+
add_auth_header_for(user: @owning_project.student)
63+
64+
get details_endpoint
65+
66+
assert_equal 200, last_response.status
67+
refute last_response_body.key?('claimed_by_unit_role_id')
68+
end
69+
70+
def test_submission_details_allows_unit_convenor
71+
add_auth_header_for(user: @convenor)
72+
73+
get details_endpoint
74+
75+
assert_equal 200, last_response.status
76+
end
77+
78+
# Staff (anyone with a unit_role) should see the claim-tracking field,
79+
# even if it is null (no overflow claim exists yet).
80+
def test_submission_details_exposes_claim_info_to_convenor
81+
add_auth_header_for(user: @convenor)
82+
83+
get details_endpoint
84+
85+
assert_equal 200, last_response.status
86+
assert last_response_body.key?('claimed_by_unit_role_id')
87+
end
88+
89+
def test_submission_details_allows_unit_tutor
90+
add_auth_header_for(user: @tutor)
91+
92+
get details_endpoint
93+
94+
assert_equal 200, last_response.status
95+
end
96+
97+
def test_submission_details_blocks_other_student_in_same_unit
98+
add_auth_header_for(user: @other_project.student)
99+
100+
get details_endpoint(project: @owning_project)
101+
102+
assert_equal 403, last_response.status
103+
assert_equal 'You do not have permission to read submissions for this project.',
104+
last_response_body['error']
105+
end
106+
107+
def test_submission_details_blocks_staff_from_a_different_unit
108+
other_unit_staff = @other_unit.main_convenor_user
109+
add_auth_header_for(user: other_unit_staff)
110+
111+
get details_endpoint(project: @owning_project)
112+
113+
assert_equal 403, last_response.status
114+
end
115+
116+
def test_submission_details_blocks_unauthenticated_request
117+
header 'auth_token', nil
118+
header 'username', nil
119+
120+
get details_endpoint
121+
122+
assert_equal 419, last_response.status
123+
end
124+
125+
def test_submission_details_rejects_task_definition_from_another_unit
126+
add_auth_header_for(user: @owning_project.student)
127+
128+
get details_endpoint(task_definition: @other_unit_task_definition)
129+
130+
assert_equal 404, last_response.status
131+
end
132+
133+
# ---------------------------------------------------------------------
134+
# submission_files
135+
# ---------------------------------------------------------------------
136+
137+
def test_submission_files_allows_owning_student
138+
add_auth_header_for(user: @owning_project.student)
139+
140+
get files_endpoint
141+
142+
assert_equal 200, last_response.status
143+
end
144+
145+
def test_submission_files_blocks_other_student_in_same_unit
146+
add_auth_header_for(user: @other_project.student)
147+
148+
get files_endpoint(project: @owning_project)
149+
150+
assert_equal 403, last_response.status
151+
end
152+
153+
def test_submission_files_blocks_staff_from_a_different_unit
154+
other_unit_staff = @other_unit.main_convenor_user
155+
add_auth_header_for(user: other_unit_staff)
156+
157+
get files_endpoint(project: @owning_project)
158+
159+
assert_equal 403, last_response.status
160+
end
161+
162+
# Regression guard: the Content-Disposition filename is built from
163+
# project.student.username. Confirm that only ever happens for a caller
164+
# who has already passed the authorise? check - i.e. a cross-student
165+
# request never reaches the point where the filename (and therefore the
166+
# other student's username) is constructed or exposed in the response.
167+
def test_submission_files_does_not_leak_owning_students_username_to_blocked_caller
168+
add_auth_header_for(user: @other_project.student)
169+
170+
get files_endpoint(project: @owning_project)
171+
172+
assert_equal 403, last_response.status
173+
refute_match(/#{@owning_project.student.username}/, last_response.headers['Content-Disposition'].to_s)
174+
end
175+
176+
def test_submission_files_blocks_unauthenticated_request
177+
header 'auth_token', nil
178+
header 'username', nil
179+
180+
get files_endpoint
181+
182+
assert_equal 419, last_response.status
183+
end
184+
end

0 commit comments

Comments
 (0)