From 55a86cd179d88bc933bb3950b3acad2ec2e180f3 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Thu, 28 May 2026 16:29:26 +0100 Subject: [PATCH 1/9] feat: add toggle for school settings for scratch enabled This update schools endpoint isn't used, and there were no requests to it the last 30 days. We've chosen to limit the params so we don't get any unexpected behaviour, but can add old ones back in if we want to allow school owners to update them in the future. We've also removed the 422 test as can't seem to make a similar failing test. We've also checked the abilities and only school owners can update the school, but owners, teachers and students can view it. --- app/controllers/api/schools_controller.rb | 12 +++++++++--- app/views/api/schools/_school.json.jbuilder | 3 ++- .../20260528141937_add_scratch_enabled_to_school.rb | 5 +++++ db/schema.rb | 3 ++- spec/features/my_school/showing_my_school_spec.rb | 2 +- spec/features/school/updating_a_school_spec.rb | 11 +++-------- 6 files changed, 22 insertions(+), 14 deletions(-) create mode 100644 db/migrate/20260528141937_add_scratch_enabled_to_school.rb diff --git a/app/controllers/api/schools_controller.rb b/app/controllers/api/schools_controller.rb index 52427efe9..5fd28e792 100644 --- a/app/controllers/api/schools_controller.rb +++ b/app/controllers/api/schools_controller.rb @@ -16,7 +16,7 @@ def show end def create - result = School::Create.call(school_params:, creator_id: current_user.id, token: current_user.token) + result = School::Create.call(school_params: create_params, creator_id: current_user.id, token: current_user.token) if result.success? @school = result[:school] @@ -31,7 +31,7 @@ def create def update school = School.find(params[:id]) - result = School::Update.call(school:, school_params:) + result = School::Update.call(school:, school_params: update_params) if result.success? @school = result[:school] @@ -76,7 +76,7 @@ def import private - def school_params + def create_params params.expect( school: %i[name website @@ -99,5 +99,11 @@ def school_params user_origin] ) end + + def update_params + params.expect( + school: %i[scratch_enabled] + ) + end end end diff --git a/app/views/api/schools/_school.json.jbuilder b/app/views/api/schools/_school.json.jbuilder index 5aee45f78..8139342d4 100644 --- a/app/views/api/schools/_school.json.jbuilder +++ b/app/views/api/schools/_school.json.jbuilder @@ -17,7 +17,8 @@ json.call( :country_code, :verified_at, :created_at, - :updated_at + :updated_at, + :scratch_enabled ) include_roles = local_assigns.fetch(:roles, false) diff --git a/db/migrate/20260528141937_add_scratch_enabled_to_school.rb b/db/migrate/20260528141937_add_scratch_enabled_to_school.rb new file mode 100644 index 000000000..1c3f07f7f --- /dev/null +++ b/db/migrate/20260528141937_add_scratch_enabled_to_school.rb @@ -0,0 +1,5 @@ +class AddScratchEnabledToSchool < ActiveRecord::Migration[8.1] + def change + add_column :schools, :scratch_enabled, :boolean, default: false, null: false + end +end diff --git a/db/schema.rb b/db/schema.rb index 36ac84ff4..497d2d7fe 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_04_29_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_05_28_141937) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -335,6 +335,7 @@ t.string "reference" t.datetime "rejected_at" t.string "school_roll_number" + t.boolean "scratch_enabled", default: false, null: false t.datetime "updated_at", null: false t.integer "user_origin", default: 0 t.datetime "verified_at" diff --git a/spec/features/my_school/showing_my_school_spec.rb b/spec/features/my_school/showing_my_school_spec.rb index 8191e4223..568396146 100644 --- a/spec/features/my_school/showing_my_school_spec.rb +++ b/spec/features/my_school/showing_my_school_spec.rb @@ -20,7 +20,7 @@ school_json = school.to_json(only: %i[ id name website reference address_line_1 address_line_2 municipality administrative_area postal_code country_code code verified_at created_at updated_at district_name district_nces_id school_roll_number ]) - expected_data = JSON.parse(school_json, symbolize_names: true).merge(roles: ['owner'], import_in_progress: school.import_in_progress?, auto_join_enabled: school.auto_join_enabled?) + expected_data = JSON.parse(school_json, symbolize_names: true).merge(roles: ['owner'], import_in_progress: school.import_in_progress?, auto_join_enabled: school.auto_join_enabled?, scratch_enabled: false) get('/api/school', headers:) data = JSON.parse(response.body, symbolize_names: true) diff --git a/spec/features/school/updating_a_school_spec.rb b/spec/features/school/updating_a_school_spec.rb index 92f18442f..ddedd7697 100644 --- a/spec/features/school/updating_a_school_spec.rb +++ b/spec/features/school/updating_a_school_spec.rb @@ -7,14 +7,14 @@ authenticated_in_hydra_as(owner) end - let!(:school) { create(:school) } + let!(:school) { create(:school, scratch_enabled: false) } let(:headers) { { Authorization: UserProfileMock::TOKEN } } let(:owner) { create(:owner, school:) } let(:params) do { school: { - name: 'New Name' + scratch_enabled: true } } end @@ -28,7 +28,7 @@ put("/api/schools/#{school.id}", headers:, params:) data = JSON.parse(response.body, symbolize_names: true) - expect(data[:name]).to eq('New Name') + expect(data[:scratch_enabled]).to be(true) end it 'responds 404 Not Found when no school exists' do @@ -41,11 +41,6 @@ expect(response).to have_http_status(:bad_request) end - it 'responds 422 Unprocessable Entity when params are invalid' do - put("/api/schools/#{school.id}", headers:, params: { school: { name: ' ' } }) - expect(response).to have_http_status(:unprocessable_content) - end - it 'responds 401 Unauthorized when no token is given' do put "/api/schools/#{school.id}" expect(response).to have_http_status(:unauthorized) From e47ccde741268571549654198fc17df397ad8b72 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Mon, 1 Jun 2026 14:51:04 +0100 Subject: [PATCH 2/9] Allow existing Scratch projects to always be accessible The behaviour we want is to allow any existing scratch projects to be accessed updated and so I've removed all the checks from the Scratch Controller, except that they are logged in and part of a school. The school check could be removed in the future if we want to open up access to Scratch. Note that some controllers were missing the logged in check so I've updated the code and tests --- .../api/scratch/assets_controller.rb | 1 - .../api/scratch/projects_controller.rb | 3 --- .../api/scratch/scratch_controller.rb | 9 +++----- .../scratch/creating_a_scratch_asset_spec.rb | 20 ++++++---------- .../creating_a_scratch_project_spec.rb | 11 ++++----- ...eating_and_showing_a_scratch_asset_spec.rb | 20 ++++------------ .../scratch/showing_a_scratch_project_spec.rb | 23 +++++++++++++++---- .../updating_a_scratch_project_spec.rb | 16 +------------ 8 files changed, 38 insertions(+), 65 deletions(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index 142845b6c..f41a0878a 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -5,7 +5,6 @@ module Scratch class AssetsController < ScratchController include ActiveStorage::SetCurrent - skip_before_action :authorize_user, only: [:show] prepend_before_action :load_project_from_header, only: %i[show create] def show diff --git a/app/controllers/api/scratch/projects_controller.rb b/app/controllers/api/scratch/projects_controller.rb index 14f73b638..1abedb13c 100644 --- a/app/controllers/api/scratch/projects_controller.rb +++ b/app/controllers/api/scratch/projects_controller.rb @@ -5,10 +5,7 @@ module Scratch class ProjectsController < ScratchController include RemixSelection - skip_before_action :authorize_user, only: [:show] - skip_before_action :check_scratch_feature, only: [:show] before_action :load_project, only: %i[show update] - before_action :ensure_create_is_a_remix, only: %i[create] def show diff --git a/app/controllers/api/scratch/scratch_controller.rb b/app/controllers/api/scratch/scratch_controller.rb index 863ab9928..7cf33ac03 100644 --- a/app/controllers/api/scratch/scratch_controller.rb +++ b/app/controllers/api/scratch/scratch_controller.rb @@ -4,13 +4,10 @@ module Api module Scratch class ScratchController < ApiController before_action :authorize_user - before_action :check_scratch_feature + before_action :only_allow_schools_to_use_scratch - def check_scratch_feature - return if current_user.nil? - - school = current_user&.schools&.first - return if Flipper.enabled?(:cat_mode, school) + def only_allow_schools_to_use_scratch + return true if current_user.schools.any? raise ActiveRecord::RecordNotFound, 'Not Found' end diff --git a/spec/features/scratch/creating_a_scratch_asset_spec.rb b/spec/features/scratch/creating_a_scratch_asset_spec.rb index dedc1d182..bd66d5fa3 100644 --- a/spec/features/scratch/creating_a_scratch_asset_spec.rb +++ b/spec/features/scratch/creating_a_scratch_asset_spec.rb @@ -10,12 +10,7 @@ create(:scratch_component, project: scratch_project) end end - let(:auth_headers) { { 'Authorization' => UserProfileMock::TOKEN } } - let(:project_headers) { auth_headers.merge('X-Project-ID' => project.identifier) } - - before do - Flipper.enable_actor :cat_mode, school - end + let(:headers) { { 'Authorization' => UserProfileMock::TOKEN, 'X-Project-ID' => project.identifier } } it 'responds 401 Unauthorized when no Authorization header is provided' do post '/api/scratch/assets/example.svg', headers: { 'X-Project-ID' => project.identifier } @@ -23,20 +18,19 @@ expect(response).to have_http_status(:unauthorized) end - it 'responds 404 Not Found when cat_mode is not enabled' do - authenticated_in_hydra_as(teacher) - Flipper.disable :cat_mode - Flipper.disable_actor :cat_mode, school + 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 + post '/api/scratch/assets/example.svg', headers: headers expect(response).to have_http_status(:not_found) end - it 'creates an asset when cat_mode is enabled and the required headers are provided' do + it 'creates an asset when user is part of a school and the required headers are provided' do authenticated_in_hydra_as(teacher) - post '/api/scratch/assets/example.svg', headers: project_headers + post '/api/scratch/assets/example.svg', headers: headers expect(response).to have_http_status(:created) diff --git a/spec/features/scratch/creating_a_scratch_project_spec.rb b/spec/features/scratch/creating_a_scratch_project_spec.rb index 7df984067..a9909a1fa 100644 --- a/spec/features/scratch/creating_a_scratch_project_spec.rb +++ b/spec/features/scratch/creating_a_scratch_project_spec.rb @@ -36,9 +36,6 @@ before do mock_phrase_generation('new-project-id') create(:scratch_component, project: original_project) - - Flipper.disable :cat_mode - Flipper.disable_actor :cat_mode, school end def make_request(query: request_query, request_headers: headers, request_params: scratch_project) @@ -56,18 +53,18 @@ 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 cat_mode is not enabled' do - authenticated_in_hydra_as(teacher) + it 'responds 404 Not Found when user is not part of a school' do + user = create(:user) + authenticated_in_hydra_as(user) make_request expect(response).to have_http_status(:not_found) end - context 'when authenticated and cat_mode is enabled' do + context 'when authenticated and part of a school' do before do authenticated_in_hydra_as(teacher) - Flipper.enable_actor :cat_mode, school end it 'responds 403 Forbidden when not remixing' 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 03f05ac7e..3f9228877 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 @@ -13,10 +13,6 @@ let(:auth_headers) { { 'Authorization' => UserProfileMock::TOKEN } } let(:project_headers) { auth_headers.merge('X-Project-ID' => project.identifier) } - before do - Flipper.enable_actor :cat_mode, school - end - describe 'GET #show' do it 'responds 400 Bad Request when X-Project-ID is not provided' do get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: auth_headers @@ -206,7 +202,7 @@ post '/api/scratch/assets/test_image_1.png', headers: request_headers, params: upload end - context 'when user is logged in and cat_mode is enabled' do + context 'when a teacher is logged in' do before do authenticated_in_hydra_as(teacher) end @@ -384,18 +380,10 @@ end end - context 'when user is logged in and cat_mode is disabled' do - before do - authenticated_in_hydra_as(teacher) - Flipper.disable :cat_mode - Flipper.disable_actor :cat_mode, school - end - - it 'responds 404 Not Found when cat_mode is not enabled' do - post '/api/scratch/assets/example.svg', headers: request_headers + it 'responds 401 unauthorized when user is not part of a school' do + post '/api/scratch/assets/example.svg', headers: { 'X-Project-ID' => project.identifier } - expect(response).to have_http_status(:not_found) - end + expect(response).to have_http_status(:unauthorized) end end diff --git a/spec/features/scratch/showing_a_scratch_project_spec.rb b/spec/features/scratch/showing_a_scratch_project_spec.rb index 2ab60579a..c3e99ef94 100644 --- a/spec/features/scratch/showing_a_scratch_project_spec.rb +++ b/spec/features/scratch/showing_a_scratch_project_spec.rb @@ -3,7 +3,12 @@ require 'rails_helper' RSpec.describe 'Showing a Scratch project', type: :request do + let(:school) { create(:school) } + let(:teacher) { create(:teacher, school:) } + let(:headers) { { 'Authorization' => UserProfileMock::TOKEN } } + it 'returns scratch project JSON' do + authenticated_in_hydra_as(teacher) project = create( :project, project_type: Project::Types::CODE_EDITOR_SCRATCH, @@ -11,7 +16,7 @@ ) create(:scratch_component, project: project) - get "/api/scratch/projects/#{project.identifier}" + get "/api/scratch/projects/#{project.identifier}", headers: headers expect(response).to have_http_status(:ok) @@ -20,6 +25,7 @@ end it 'returns the stage target first when stored targets are out of order' do + authenticated_in_hydra_as(teacher) project = create( :project, project_type: Project::Types::CODE_EDITOR_SCRATCH, @@ -37,23 +43,32 @@ } ) - get "/api/scratch/projects/#{project.identifier}" + get "/api/scratch/projects/#{project.identifier}", headers: headers expect(response).to have_http_status(:ok) expect(response.parsed_body.fetch('targets').pluck('name')).to eq(%w[Stage Sprite1 Sprite2]) end it 'returns a 404 if project does not exist' do - get '/api/scratch/projects/non_existent_project' + authenticated_in_hydra_as(teacher) + get '/api/scratch/projects/non_existent_project', headers: headers expect(response).to have_http_status(:not_found) end it 'returns a 404 if project is not a scratch project' do + authenticated_in_hydra_as(teacher) project = create(:project, project_type: Project::Types::PYTHON, locale: 'en') - get "/api/scratch/projects/#{project.identifier}" + get "/api/scratch/projects/#{project.identifier}", headers: headers 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') + get "/api/scratch/projects/#{project.identifier}" + + expect(response).to have_http_status(:unauthorized) + end end diff --git a/spec/features/scratch/updating_a_scratch_project_spec.rb b/spec/features/scratch/updating_a_scratch_project_spec.rb index 5c32a885e..eb56d9800 100644 --- a/spec/features/scratch/updating_a_scratch_project_spec.rb +++ b/spec/features/scratch/updating_a_scratch_project_spec.rb @@ -7,28 +7,14 @@ let(:teacher) { create(:teacher, school:) } let(:auth_headers) { { 'Authorization' => UserProfileMock::TOKEN } } - before do - Flipper.disable :cat_mode - Flipper.disable_actor :cat_mode, school - end - it 'responds 401 Unauthorized when no Authorization header is provided' do put '/api/scratch/projects/any-identifier', params: { project: { targets: [] } } expect(response).to have_http_status(:unauthorized) end - it 'responds 404 Not Found when cat_mode is not enabled' do - authenticated_in_hydra_as(teacher) - - put '/api/scratch/projects/any-identifier', params: { content: { targets: [] } }, headers: auth_headers - - expect(response).to have_http_status(:not_found) - end - - it 'updates a project when cat_mode is enabled and an Authorization header is provided' do + it 'updates a project when an Authorization header is provided' do authenticated_in_hydra_as(teacher) - Flipper.enable_actor :cat_mode, school project = create( :project, project_type: Project::Types::CODE_EDITOR_SCRATCH, From e19a3b72576529379dee3953fed4e4592813d2b2 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Mon, 1 Jun 2026 15:17:38 +0100 Subject: [PATCH 3/9] Don't allow scratch projects to be created unless the feature is on We're adding a new setting to allow Scratch to be toggled by school owners since Flipper is not designed for gating large amounts of individual actors. --- app/controllers/api/lessons_controller.rb | 11 +++++ .../features/lesson/creating_a_lesson_spec.rb | 40 +++++++++++++++++++ 2 files changed, 51 insertions(+) diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 8289b2caa..796ef4e18 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -6,6 +6,7 @@ class LessonsController < ApiController before_action :authorize_user, except: %i[index show] before_action :verify_school_class_belongs_to_school, only: :create + before_action :verify_can_create_scratch_projects, only: %i[create create_copy] load_and_authorize_resource :lesson def index @@ -83,6 +84,12 @@ def verify_school_class_belongs_to_school raise ParameterError, 'school_class_id does not correspond to school_id' end + def verify_can_create_scratch_projects + return unless scratch_project? && !school.scratch_enabled? + + render json: { error: 'Forbidden' }, status: :forbidden + end + def user_remixes(lessons) lessons.map { |lesson| user_remix(lesson) } end @@ -97,6 +104,10 @@ def user_remix(lesson) ) end + def scratch_project? + base_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH + end + def lesson_params base_params.merge(user_id: current_user.id) end diff --git a/spec/features/lesson/creating_a_lesson_spec.rb b/spec/features/lesson/creating_a_lesson_spec.rb index 33d80c73e..0acffea98 100644 --- a/spec/features/lesson/creating_a_lesson_spec.rb +++ b/spec/features/lesson/creating_a_lesson_spec.rb @@ -186,4 +186,44 @@ expect(response).to have_http_status(:unprocessable_content) end end + + describe 'working with Scratch projects' do + let(:params) do + { + lesson: { + name: 'Test Lesson', + school_id: school.id, + project_attributes: { + name: 'Hello Scratch project', + project_type: Project::Types::CODE_EDITOR_SCRATCH, + scratch_component: { + content: { + example_data: 'true' + } + } + } + } + } + end + + it 'creates a lesson with a scratch component when school has Scratch enabled' do + school.update!(scratch_enabled: true) + post('/api/lessons', headers:, params:) + expect(response).to have_http_status(:created) + + data = JSON.parse(response.body, symbolize_names: true) + + lesson_id = data[:id] + + project = Lesson.find(lesson_id).project + expect(project.project_type).to eq(Project::Types::CODE_EDITOR_SCRATCH) + expect(project.scratch_component.content).to eq({ 'example_data' => 'true' }) + end + + it 'returns forbidden when school does not have Scratch enabled' do + school.update!(scratch_enabled: false) + post('/api/lessons', headers:, params:) + expect(response).to have_http_status(:forbidden) + end + end end From aafc881723f19e456f52dfa1bba825066a68dc92 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Tue, 2 Jun 2026 10:59:11 +0100 Subject: [PATCH 4/9] Restrict what lesson params can be updated Previously any params could be updated in lessons which was dangerous - we don't expect many of these params to be updated to changing them could create odd behaviour or allow other data to be accessed. I've checked the frontend, and only the name and visibility can be updated. I've done this now so that users can't turn their projects into Scratch projects unexpectedly. We could add attributes back in in the future if we want to. --- app/controllers/api/lessons_controller.rb | 28 +++++++++++-------- .../features/lesson/creating_a_lesson_spec.rb | 1 + .../features/lesson/updating_a_lesson_spec.rb | 6 ++-- 3 files changed, 22 insertions(+), 13 deletions(-) diff --git a/app/controllers/api/lessons_controller.rb b/app/controllers/api/lessons_controller.rb index 796ef4e18..22cfad672 100644 --- a/app/controllers/api/lessons_controller.rb +++ b/app/controllers/api/lessons_controller.rb @@ -30,7 +30,7 @@ def show end def create - result = Lesson::Create.call(lesson_params:) + result = Lesson::Create.call(lesson_params: create_params) if result.success? @lesson_with_user = result[:lesson].with_user @@ -41,7 +41,7 @@ def create end def create_copy - result = Lesson::CreateCopy.call(lesson: @lesson, lesson_params:) + result = Lesson::CreateCopy.call(lesson: @lesson, lesson_params: create_params) if result.success? @lesson_with_user = result[:lesson].with_user @@ -54,7 +54,7 @@ def create_copy def update # TODO: Consider removing user_id from the lesson_params for update so users can update other users' lessons without changing ownership # OR consider dropping user_id on lessons and using teacher id/ids on the class instead - result = Lesson::Update.call(lesson: @lesson, lesson_params:) + result = Lesson::Update.call(lesson: @lesson, lesson_params: update_params) if result.success? @lesson_with_user = result[:lesson].with_user @@ -78,8 +78,8 @@ def filtered_lessons_scope end def verify_school_class_belongs_to_school - return if base_params[:school_class_id].blank? - return if school&.classes&.pluck(:id)&.include?(base_params[:school_class_id]) + return if create_params[:school_class_id].blank? + return if school&.classes&.pluck(:id)&.include?(create_params[:school_class_id]) raise ParameterError, 'school_class_id does not correspond to school_id' end @@ -105,14 +105,20 @@ def user_remix(lesson) end def scratch_project? - base_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH + create_params.dig(:project_attributes, :project_type) == Project::Types::CODE_EDITOR_SCRATCH end - def lesson_params - base_params.merge(user_id: current_user.id) + def update_params + params.fetch(:lesson, {}).permit( + :name, + :visibility, + { + project_attributes: [:name] + } + ) end - def base_params + def create_params params.fetch(:lesson, {}).permit( :school_id, :school_class_id, @@ -129,7 +135,7 @@ def base_params { scratch_component: {} } ] } - ) + ).merge(user_id: current_user.id) end def school_owner? @@ -137,7 +143,7 @@ def school_owner? end def school - @school ||= @lesson&.school || School.find_by(id: base_params[:school_id]) || SchoolClass.find_by(id: params[:school_class_id])&.school + @school ||= @lesson&.school || School.find_by(id: create_params[:school_id]) || SchoolClass.find_by(id: params[:school_class_id])&.school end end end diff --git a/spec/features/lesson/creating_a_lesson_spec.rb b/spec/features/lesson/creating_a_lesson_spec.rb index 0acffea98..3679c672c 100644 --- a/spec/features/lesson/creating_a_lesson_spec.rb +++ b/spec/features/lesson/creating_a_lesson_spec.rb @@ -94,6 +94,7 @@ new_params = { lesson: params[:lesson].merge(user_id: 'ignored') } post('/api/lessons', headers:, params: new_params) + expect(response).to have_http_status(:created) data = JSON.parse(response.body, symbolize_names: true) expect(data[:user_id]).to eq(teacher.id) diff --git a/spec/features/lesson/updating_a_lesson_spec.rb b/spec/features/lesson/updating_a_lesson_spec.rb index cb9eee059..37b4643c9 100644 --- a/spec/features/lesson/updating_a_lesson_spec.rb +++ b/spec/features/lesson/updating_a_lesson_spec.rb @@ -117,15 +117,17 @@ expect(response).to have_http_status(:ok) end - it 'responds 422 Unprocessable Entity when trying to re-assign the lesson to a different class' do + it 'does not allow re-assigning the lesson to a different class' do school = create(:school, id: SecureRandom.uuid) teacher = create(:teacher, school:) school_class = create(:school_class, school:, teacher_ids: [teacher.id]) new_params = { lesson: params[:lesson].merge(school_class_id: school_class.id) } put("/api/lessons/#{lesson.id}", headers:, params: new_params) + expect(response).to have_http_status(:ok) - expect(response).to have_http_status(:unprocessable_content) + lesson.reload + expect(lesson.school_class_id).not_to eq(school_class.id) end end end From ea18699cea308fdf6482ca9e5090ae2c5cf9f2a4 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Tue, 2 Jun 2026 11:41:25 +0100 Subject: [PATCH 5/9] Move method to where it's used This is only relevant to the projects controller, so there's no need to have it in the shared controller --- app/controllers/api/scratch/projects_controller.rb | 4 ++++ app/controllers/api/scratch/scratch_controller.rb | 4 ---- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/controllers/api/scratch/projects_controller.rb b/app/controllers/api/scratch/projects_controller.rb index 1abedb13c..c7a7ab1df 100644 --- a/app/controllers/api/scratch/projects_controller.rb +++ b/app/controllers/api/scratch/projects_controller.rb @@ -97,6 +97,10 @@ def move_pending_scratch_upload_to_remix(pending_upload, remix_project) rescue ActiveRecord::RecordNotUnique pending_upload.destroy! end + + def load_project + @project = Project.find_by!(identifier: params[:id], project_type: Project::Types::CODE_EDITOR_SCRATCH) + end end end end diff --git a/app/controllers/api/scratch/scratch_controller.rb b/app/controllers/api/scratch/scratch_controller.rb index 7cf33ac03..293d7dd5a 100644 --- a/app/controllers/api/scratch/scratch_controller.rb +++ b/app/controllers/api/scratch/scratch_controller.rb @@ -11,10 +11,6 @@ def only_allow_schools_to_use_scratch raise ActiveRecord::RecordNotFound, 'Not Found' end - - def load_project - @project = Project.find_by!(identifier: params[:id], project_type: Project::Types::CODE_EDITOR_SCRATCH) - end end end end From ea915dccc09a76289f91c7dbcc82112bf2198f04 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Tue, 2 Jun 2026 11:50:03 +0100 Subject: [PATCH 6/9] Remove duplicated test file While we have been working on this we have ended up with two test files that cover creating assets. Use the more comprehensive file for now and move in the only one that wasn't covered. --- .../scratch/creating_a_scratch_asset_spec.rb | 41 ------------------- ...eating_and_showing_a_scratch_asset_spec.rb | 11 ++++- 2 files changed, 10 insertions(+), 42 deletions(-) delete mode 100644 spec/features/scratch/creating_a_scratch_asset_spec.rb diff --git a/spec/features/scratch/creating_a_scratch_asset_spec.rb b/spec/features/scratch/creating_a_scratch_asset_spec.rb deleted file mode 100644 index bd66d5fa3..000000000 --- a/spec/features/scratch/creating_a_scratch_asset_spec.rb +++ /dev/null @@ -1,41 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -RSpec.describe 'Creating a Scratch asset', type: :request do - let(:school) { create(:school) } - let(:teacher) { create(:teacher, school:) } - let(:project) do - create(:project, project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil, user_id: teacher.id).tap do |scratch_project| - create(:scratch_component, project: scratch_project) - end - end - let(:headers) { { 'Authorization' => UserProfileMock::TOKEN, 'X-Project-ID' => project.identifier } } - - it 'responds 401 Unauthorized when no Authorization header is provided' do - post '/api/scratch/assets/example.svg', headers: { 'X-Project-ID' => project.identifier } - - 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: headers - - expect(response).to have_http_status(:not_found) - end - - it 'creates an asset when user is part of a school and the required headers are provided' do - authenticated_in_hydra_as(teacher) - - post '/api/scratch/assets/example.svg', headers: headers - - expect(response).to have_http_status(:created) - - data = JSON.parse(response.body, symbolize_names: true) - expect(data[:status]).to eq('ok') - expect(data[:'content-name']).to eq('example') - end -end 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 3f9228877..17171e8fb 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 @@ -380,11 +380,20 @@ end end - it 'responds 401 unauthorized when user is not part of a school' do + it 'responds 401 unauthorized when user is not signed in' do post '/api/scratch/assets/example.svg', headers: { 'X-Project-ID' => project.identifier } 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) From a88e14283875391f7f26deefaf561162c875b0af Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Tue, 2 Jun 2026 12:10:27 +0100 Subject: [PATCH 7/9] Authorize showing project --- .../api/scratch/projects_controller.rb | 1 + .../scratch/showing_a_scratch_project_spec.rb | 26 +++++++++++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/scratch/projects_controller.rb b/app/controllers/api/scratch/projects_controller.rb index c7a7ab1df..6e605104e 100644 --- a/app/controllers/api/scratch/projects_controller.rb +++ b/app/controllers/api/scratch/projects_controller.rb @@ -9,6 +9,7 @@ class ProjectsController < ScratchController before_action :ensure_create_is_a_remix, only: %i[create] def show + authorize! :show, @project render json: @project.scratch_component.content_with_stage_first end diff --git a/spec/features/scratch/showing_a_scratch_project_spec.rb b/spec/features/scratch/showing_a_scratch_project_spec.rb index c3e99ef94..cf6689b7c 100644 --- a/spec/features/scratch/showing_a_scratch_project_spec.rb +++ b/spec/features/scratch/showing_a_scratch_project_spec.rb @@ -6,13 +6,18 @@ let(:school) { create(:school) } let(:teacher) { create(:teacher, 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) } it 'returns scratch project JSON' do authenticated_in_hydra_as(teacher) project = create( :project, project_type: Project::Types::CODE_EDITOR_SCRATCH, - locale: 'en' + locale: 'en', + school: school, + user_id: teacher.id, + lesson: lesson ) create(:scratch_component, project: project) @@ -29,7 +34,10 @@ project = create( :project, project_type: Project::Types::CODE_EDITOR_SCRATCH, - locale: 'en' + locale: 'en', + school: school, + lesson: lesson, + user_id: teacher.id ) create( :scratch_component, @@ -71,4 +79,18 @@ expect(response).to have_http_status(:unauthorized) end + + it 'returns a 403 forbidden if user does not have access to the project' do + authenticated_in_hydra_as(teacher) + project = create( + :project, + project_type: Project::Types::CODE_EDITOR_SCRATCH, + locale: 'en' + ) + create(:scratch_component, project: project) + + get "/api/scratch/projects/#{project.identifier}", headers: headers + + expect(response).to have_http_status(:forbidden) + end end From cdf0eedae2bbfba7c6fb9cb6f7d502500372db53 Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Tue, 2 Jun 2026 12:12:43 +0100 Subject: [PATCH 8/9] Add authorization for updating scratch projects --- .../api/scratch/projects_controller.rb | 1 + .../updating_a_scratch_project_spec.rb | 21 ++++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/controllers/api/scratch/projects_controller.rb b/app/controllers/api/scratch/projects_controller.rb index 6e605104e..8989be86f 100644 --- a/app/controllers/api/scratch/projects_controller.rb +++ b/app/controllers/api/scratch/projects_controller.rb @@ -48,6 +48,7 @@ def create end def update + authorize! :update, @project @project.scratch_component&.content = scratch_content_params @project.save! render json: { status: 'ok' }, status: :ok diff --git a/spec/features/scratch/updating_a_scratch_project_spec.rb b/spec/features/scratch/updating_a_scratch_project_spec.rb index eb56d9800..3df8e2a0c 100644 --- a/spec/features/scratch/updating_a_scratch_project_spec.rb +++ b/spec/features/scratch/updating_a_scratch_project_spec.rb @@ -6,6 +6,8 @@ let(:school) { create(:school) } let(:teacher) { create(:teacher, school:) } let(:auth_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) } it 'responds 401 Unauthorized when no Authorization header is provided' do put '/api/scratch/projects/any-identifier', params: { project: { targets: [] } } @@ -18,7 +20,10 @@ project = create( :project, project_type: Project::Types::CODE_EDITOR_SCRATCH, - locale: 'en' + locale: 'en', + school: school, + lesson: lesson, + user_id: teacher.id ) create(:scratch_component, project: project) @@ -31,4 +36,18 @@ expect(project.reload.scratch_component.content.to_h['targets']).to eq(['some update']) end + + it 'returns 403 Forbidden when trying to update a project user does not have access to' do + authenticated_in_hydra_as(teacher) + project = create( + :project, + project_type: Project::Types::CODE_EDITOR_SCRATCH, + locale: 'en' + ) + create(:scratch_component, project: project) + + put "/api/scratch/projects/#{project.identifier}", params: { targets: ['some update'] }, headers: auth_headers + + expect(response).to have_http_status(:forbidden) + end end From 2b204ba9f6e4a4b31e2ad75ce477103ab9dbdaec Mon Sep 17 00:00:00 2001 From: Chris Zetter <253059100+zetter-rpf@users.noreply.github.com> Date: Tue, 2 Jun 2026 13:57:55 +0100 Subject: [PATCH 9/9] Refactor scratch controllers to use authorize resource Using authorize_resource directly makes it easier to remove authorization concerns from controller actions, provide better defaults for the controller, and give us consistent behaviour for unauthorized requests. --- app/controllers/api/scratch/assets_controller.rb | 4 +--- .../api/scratch/projects_controller.rb | 16 ++++++++-------- .../scratch/creating_a_scratch_project_spec.rb | 4 ++-- 3 files changed, 11 insertions(+), 13 deletions(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index f41a0878a..6b4cad178 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -6,10 +6,10 @@ class AssetsController < ScratchController include ActiveStorage::SetCurrent prepend_before_action :load_project_from_header, only: %i[show create] + authorize_resource :project_from_header def show filename_with_extension = "#{params[:id]}.#{params[:format]}" - authorize! :show, @project_from_header scratch_asset = ScratchAsset.find_visible_to_project( project: @project_from_header, @@ -22,8 +22,6 @@ def show end def create - authorize! :show, @project_from_header - filename_with_extension = "#{params[:id]}.#{params[:format]}" scratch_asset = ScratchAsset.find_or_initialize_by( project: @project_from_header, diff --git a/app/controllers/api/scratch/projects_controller.rb b/app/controllers/api/scratch/projects_controller.rb index 8989be86f..7e5ecb5a5 100644 --- a/app/controllers/api/scratch/projects_controller.rb +++ b/app/controllers/api/scratch/projects_controller.rb @@ -5,18 +5,19 @@ module Scratch class ProjectsController < ScratchController include RemixSelection - before_action :load_project, only: %i[show update] + before_action :load_project, except: %i[create] + authorize_resource :project, except: %i[create] + before_action :ensure_create_is_a_remix, only: %i[create] + before_action :load_original_project, only: %i[create] + authorize_resource :original_project, only: %i[create] def show - authorize! :show, @project render json: @project.scratch_component.content_with_stage_first end def create - original_project = load_original_project(source_project_identifier) - return render json: { error: I18n.t('errors.admin.unauthorized') }, status: :unauthorized unless current_ability.can?(:show, original_project) - + original_project = @original_project remix_params = create_params return render json: { error: I18n.t('errors.project.remixing.invalid_params') }, status: :bad_request if remix_params.dig(:scratch_component, :content).blank? @@ -48,7 +49,6 @@ def create end def update - authorize! :update, @project @project.scratch_component&.content = scratch_content_params @project.save! render json: { status: 'ok' }, status: :ok @@ -73,8 +73,8 @@ def create_params } end - def load_original_project(identifier) - Project.find_by!(identifier:, project_type: Project::Types::CODE_EDITOR_SCRATCH) + def load_original_project + @original_project = Project.find_by!(identifier: source_project_identifier, project_type: Project::Types::CODE_EDITOR_SCRATCH) end def scratch_content_params diff --git a/spec/features/scratch/creating_a_scratch_project_spec.rb b/spec/features/scratch/creating_a_scratch_project_spec.rb index a9909a1fa..2f01eb160 100644 --- a/spec/features/scratch/creating_a_scratch_project_spec.rb +++ b/spec/features/scratch/creating_a_scratch_project_spec.rb @@ -93,13 +93,13 @@ def make_request(query: request_query, request_headers: headers, request_params: expect(response).to have_http_status(:not_found) end - it 'responds 401 Unauthorized when the user cannot access the original project' do + it 'responds 403 Forbidden when the user cannot access the original project' do inaccessible_project = create(:project, project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil) create(:scratch_component, project: inaccessible_project) make_request(query: { original_id: inaccessible_project.identifier, is_remix: '1' }) - expect(response).to have_http_status(:unauthorized) + expect(response).to have_http_status(:forbidden) end it 'responds 400 Bad Request when no Scratch content is submitted' do