From 22930ed48c77eb72419fe89bf5d91363200cb293 Mon Sep 17 00:00:00 2001 From: Ram Modhvadia Date: Mon, 22 Jun 2026 16:43:56 +0100 Subject: [PATCH 1/7] update before actions in scratch controller --- app/controllers/api/scratch/assets_controller.rb | 1 + app/controllers/api/scratch/projects_controller.rb | 1 + app/controllers/api/scratch/scratch_controller.rb | 8 -------- 3 files changed, 2 insertions(+), 8 deletions(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index 6b4cad178..835185c07 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -5,6 +5,7 @@ module Scratch class AssetsController < ScratchController include ActiveStorage::SetCurrent + before_action :authorize_user, except: %i[show] prepend_before_action :load_project_from_header, only: %i[show create] authorize_resource :project_from_header diff --git a/app/controllers/api/scratch/projects_controller.rb b/app/controllers/api/scratch/projects_controller.rb index 3383d90d5..f6887969d 100644 --- a/app/controllers/api/scratch/projects_controller.rb +++ b/app/controllers/api/scratch/projects_controller.rb @@ -5,6 +5,7 @@ module Scratch class ProjectsController < ScratchController include RemixSelection + before_action :authorize_user, except: %i[show] before_action :load_project, except: %i[create] authorize_resource :project, except: %i[create] diff --git a/app/controllers/api/scratch/scratch_controller.rb b/app/controllers/api/scratch/scratch_controller.rb index 293d7dd5a..4c115fe8b 100644 --- a/app/controllers/api/scratch/scratch_controller.rb +++ b/app/controllers/api/scratch/scratch_controller.rb @@ -3,14 +3,6 @@ module Api module Scratch class ScratchController < ApiController - before_action :authorize_user - before_action :only_allow_schools_to_use_scratch - - def only_allow_schools_to_use_scratch - return true if current_user.schools.any? - - raise ActiveRecord::RecordNotFound, 'Not Found' - end end end end From 6843e7b042da137fcc74347ce7bbd8c47ab0cc83 Mon Sep 17 00:00:00 2001 From: Ram Modhvadia Date: Tue, 23 Jun 2026 11:50:21 +0100 Subject: [PATCH 2/7] update tests --- spec/factories/project.rb | 5 +++ .../creating_a_scratch_project_spec.rb | 32 +++++++++++++++--- ...eating_and_showing_a_scratch_asset_spec.rb | 33 +++++++++++++------ .../scratch/showing_a_scratch_project_spec.rb | 15 +++++++-- 4 files changed, 67 insertions(+), 18 deletions(-) diff --git a/spec/factories/project.rb b/spec/factories/project.rb index 7813a13c0..82d00982a 100644 --- a/spec/factories/project.rb +++ b/spec/factories/project.rb @@ -67,5 +67,10 @@ instructions { Faker::Lorem.paragraph } school factory: %i[school] end + + factory :scratch_project do + project_type { Project::Types::CODE_EDITOR_SCRATCH } + scratch_component + end end end diff --git a/spec/features/scratch/creating_a_scratch_project_spec.rb b/spec/features/scratch/creating_a_scratch_project_spec.rb index 3c5f77c87..ed1c9e4cb 100644 --- a/spec/features/scratch/creating_a_scratch_project_spec.rb +++ b/spec/features/scratch/creating_a_scratch_project_spec.rb @@ -54,13 +54,35 @@ def make_request(query: request_query, request_headers: headers, request_params: expect(response).to have_http_status(:unauthorized) end - it 'responds 404 Not Found when user is not part of a school' do - user = create(:user) - authenticated_in_hydra_as(user) + context 'when authenticated but not part of a school' do + let(:user) { create(:user) } - make_request + before do + authenticated_in_hydra_as(user) + end + + it 'responds 403 forbidden when the user cannot access the original project' do + make_request - expect(response).to have_http_status(:not_found) + expect(response).to have_http_status(:forbidden) + end + + context 'when the original project is anonymous scratch project' do + let(:original_project) do + create( + :project, + user_id: nil, + project_type: Project::Types::CODE_EDITOR_SCRATCH, + locale: 'en' + ) + end + + it 'responds 200 ok' do + make_request + + expect(response).to have_http_status(:ok) + end + end end context 'when authenticated and part of a school' do diff --git a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index 17171e8fb..397e3d54e 100644 --- a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -20,7 +20,29 @@ expect(response).to have_http_status(:bad_request) end - context 'when the user can view the project' do + context 'when the user is not logged in' do + let(:project_headers) { { 'X-Project-ID' => project.identifier } } + + it 'responds with unathorized' do + create_uploaded_scratch_asset(filename: 'test_image_1.png', project:, body: 'global-body') + get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: project_headers + + expect(response).to have_http_status(:unauthorized) + end + + context 'when the project is anonymous' do + let(:project) { create_scratch_project(locale: 'en', user_id: nil) } + + it 'responds with 200 ok' do + create_uploaded_scratch_asset(filename: 'test_image_1.png', project: nil, body: 'global-body') + get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: project_headers + + expect(response).to have_http_status(:found) + end + end + end + + context 'when the project owner is logged in' do before do authenticated_in_hydra_as(teacher) end @@ -385,15 +407,6 @@ expect(response).to have_http_status(:unauthorized) end - - it 'responds 404 Not Found when user is not part of a school' do - user = create(:user) - authenticated_in_hydra_as(user) - - post '/api/scratch/assets/example.svg', headers: project_headers - - expect(response).to have_http_status(:not_found) - end end def create_scratch_project(**attributes) diff --git a/spec/features/scratch/showing_a_scratch_project_spec.rb b/spec/features/scratch/showing_a_scratch_project_spec.rb index cf6689b7c..a0262f1d8 100644 --- a/spec/features/scratch/showing_a_scratch_project_spec.rb +++ b/spec/features/scratch/showing_a_scratch_project_spec.rb @@ -5,6 +5,7 @@ RSpec.describe 'Showing a Scratch project', type: :request do let(:school) { create(:school) } let(:teacher) { create(:teacher, school:) } + let(:student) { create(:student, school:) } let(:headers) { { 'Authorization' => UserProfileMock::TOKEN } } let(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) } let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id) } @@ -73,11 +74,19 @@ expect(response).to have_http_status(:not_found) end - it 'returns a 401 unauthorized if not logged in' do - project = create(:project, project_type: Project::Types::PYTHON, locale: 'en') + it 'returns a 200 ok if not logged in for an anonymous scratch project' do + project = create(:scratch_project, locale: 'en', user_id: nil) get "/api/scratch/projects/#{project.identifier}" - expect(response).to have_http_status(:unauthorized) + expect(response).to have_http_status(:ok) + end + + it 'returns a 200 ok if logged in for an anonymous scratch project' do + authenticated_in_hydra_as(student) + project = create(:scratch_project, locale: 'en', user_id: nil) + get "/api/scratch/projects/#{project.identifier}" + + expect(response).to have_http_status(:ok) end it 'returns a 403 forbidden if user does not have access to the project' do From 2a73b6bac9c420f637a93dcf115bc3886a68193c Mon Sep 17 00:00:00 2001 From: Ram Modhvadia Date: Tue, 23 Jun 2026 13:47:45 +0100 Subject: [PATCH 3/7] fix typo Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../scratch/creating_and_showing_a_scratch_asset_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index 397e3d54e..a7caa6370 100644 --- a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -23,7 +23,7 @@ context 'when the user is not logged in' do let(:project_headers) { { 'X-Project-ID' => project.identifier } } - it 'responds with unathorized' do + it 'responds with unauthorized' do create_uploaded_scratch_asset(filename: 'test_image_1.png', project:, body: 'global-body') get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: project_headers From 9a9591c4335a7bd84b3a35e9663e504fa1307d78 Mon Sep 17 00:00:00 2001 From: Ram Modhvadia Date: Tue, 23 Jun 2026 13:48:15 +0100 Subject: [PATCH 4/7] correct test description Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- .../scratch/creating_and_showing_a_scratch_asset_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index a7caa6370..1ab289110 100644 --- a/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb @@ -33,7 +33,7 @@ context 'when the project is anonymous' do let(:project) { create_scratch_project(locale: 'en', user_id: nil) } - it 'responds with 200 ok' do + it 'redirects (302 Found)' do create_uploaded_scratch_asset(filename: 'test_image_1.png', project: nil, body: 'global-body') get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: project_headers From 53a8a395930b66418d8b4e50535addf16f7c5ac1 Mon Sep 17 00:00:00 2001 From: Ram Modhvadia Date: Tue, 23 Jun 2026 15:37:56 +0100 Subject: [PATCH 5/7] fix for project show ability --- app/models/ability.rb | 1 + spec/features/scratch/showing_a_scratch_project_spec.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 8365cf7a3..c6ae94faa 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -22,6 +22,7 @@ def initialize(user) private def define_common_non_student_abilities(user) + can :show, Project, user_id: nil, school_id: nil, project_type: Project::Types::CODE_EDITOR_SCRATCH return if user&.student? # Anyone can view projects not owned by a user or a school. diff --git a/spec/features/scratch/showing_a_scratch_project_spec.rb b/spec/features/scratch/showing_a_scratch_project_spec.rb index a0262f1d8..3e0157173 100644 --- a/spec/features/scratch/showing_a_scratch_project_spec.rb +++ b/spec/features/scratch/showing_a_scratch_project_spec.rb @@ -84,7 +84,7 @@ it 'returns a 200 ok if logged in for an anonymous scratch project' do authenticated_in_hydra_as(student) project = create(:scratch_project, locale: 'en', user_id: nil) - get "/api/scratch/projects/#{project.identifier}" + get "/api/scratch/projects/#{project.identifier}", headers: headers expect(response).to have_http_status(:ok) end From 233be29f0572dd69fe5e07bf1fe0c85acf55443d Mon Sep 17 00:00:00 2001 From: Ram Modhvadia Date: Tue, 23 Jun 2026 16:41:48 +0100 Subject: [PATCH 6/7] small refactor for non student abilities --- app/models/ability.rb | 7 ++----- spec/models/ability_spec.rb | 2 +- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index c6ae94faa..a90cb2a27 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -4,7 +4,7 @@ class Ability include CanCan::Ability def initialize(user) - define_common_non_student_abilities(user) + define_common_abilities return unless user @@ -21,10 +21,7 @@ def initialize(user) private - def define_common_non_student_abilities(user) - can :show, Project, user_id: nil, school_id: nil, project_type: Project::Types::CODE_EDITOR_SCRATCH - return if user&.student? - + def define_common_abilities # Anyone can view projects not owned by a user or a school. can :show, Project, user_id: nil, school_id: nil can :show, Component, project: { user_id: nil, school_id: nil } diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 27219062d..a7f81c3ed 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -492,7 +492,7 @@ context 'with a starter project' do it { is_expected.not_to be_able_to(:index, starter_project) } - it { is_expected.not_to be_able_to(:show, starter_project) } + it { is_expected.to be_able_to(:show, starter_project) } it { is_expected.not_to be_able_to(:create, starter_project) } it { is_expected.not_to be_able_to(:update, starter_project) } it { is_expected.not_to be_able_to(:destroy, starter_project) } From 4dbee2d49b5e0efec47fd73e95aa8be30a8c2856 Mon Sep 17 00:00:00 2001 From: Ram Modhvadia Date: Tue, 23 Jun 2026 16:51:05 +0100 Subject: [PATCH 7/7] update test --- spec/requests/projects/index_spec.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/spec/requests/projects/index_spec.rb b/spec/requests/projects/index_spec.rb index bde800cee..da5cf0b92 100644 --- a/spec/requests/projects/index_spec.rb +++ b/spec/requests/projects/index_spec.rb @@ -17,7 +17,9 @@ context 'when user is logged in' do before do # create non user projects - create_list(:project, 2) + create(:project) + create(:project, user_id: nil) + authenticated_in_hydra_as(owner) end