From 122e87425ce101705ba18e1af25e59b2622b281f Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Mon, 17 Aug 2026 16:12:43 +0200 Subject: [PATCH 1/6] Authenticate Experience CS project syncs --- .env.example | 5 +- README.md | 7 +- app/controllers/api/projects_controller.rb | 1 + .../api/scratch/assets_controller.rb | 34 ++++-- app/controllers/concerns/identifiable.rb | 6 ++ .../experience_cs_service_authenticator.rb | 14 +++ config/application.rb | 1 + config/routes.rb | 1 + .../project/creating_a_project_spec.rb | 22 ++++ ...eating_and_showing_a_scratch_asset_spec.rb | 102 ++++++++++++++++++ spec/requests/projects/update_spec.rb | 15 +++ ...xperience_cs_service_authenticator_spec.rb | 38 +++++++ 12 files changed, 233 insertions(+), 13 deletions(-) create mode 100644 app/services/experience_cs_service_authenticator.rb create mode 100644 spec/services/experience_cs_service_authenticator_spec.rb diff --git a/.env.example b/.env.example index f423df83a..2de05f94c 100644 --- a/.env.example +++ b/.env.example @@ -1,6 +1,9 @@ # localhost is set as an origin by default in development (config/initializers/cors.rb) # so you probably only need to set ALLOWED_ORIGINS for debugging purposes ALLOWED_ORIGINS="" +# Must match EDITOR_API_SYNC_API_KEY in Experience CS. Used only for +# asynchronous public curriculum project and global Scratch asset syncs. +EXPERIENCE_CS_API_KEY=changeme AWS_ACCESS_KEY_ID=changeme AWS_S3_ACTIVE_STORAGE_BUCKET=changeme @@ -74,4 +77,4 @@ PARDOT_SUBSCRIPTION_URL= CLOUDFLARE_TURNSTILE_SECRET_KEY=1x0000000000000000000000000000000AA HOSTNAME=localhost -LEARNER_EXPERIENCE_TESTS_DISPATCH_TOKEN=changeme \ No newline at end of file +LEARNER_EXPERIENCE_TESTS_DISPATCH_TOKEN=changeme diff --git a/README.md b/README.md index 926511633..cd36853c7 100644 --- a/README.md +++ b/README.md @@ -161,7 +161,12 @@ Project images are uploaded via `POST` requests to `/projects/{project_identfier A project remix is created via a `POST` request to `projects/{original_project_identifier}/remix`. +Experience CS synchronizes public curriculum projects and their global Scratch +assets asynchronously. Configure `EXPERIENCE_CS_API_KEY` to the same secret as +Experience CS's `EDITOR_API_SYNC_API_KEY`. The corresponding request header is +accepted only for project create/update and global Scratch asset upload; it does +not authorize user-project operations. + ### Code Editor for Education Editor API provides routes for managing resources such as schools, school classes and lessons, as well as for inviting teachers and managing student accounts via `profile` requests. - diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index d89cd70ed..91817b2f6 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -4,6 +4,7 @@ module Api class ProjectsController < ApiController + prepend_before_action :load_experience_cs_service_user, only: %i[create update] before_action :authorize_user, only: %i[create update index destroy] before_action :load_project, only: %i[show update destroy show_context] before_action :load_projects, only: %i[index] diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index e2b811715..0e9f6e85e 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -6,8 +6,8 @@ class AssetsController < ApiController 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 + prepend_before_action :load_project_from_header, only: %i[show create create_global] + authorize_resource :project_from_header, except: %i[create_global] def show filename_with_extension = "#{params[:id]}.#{params[:format]}" @@ -24,30 +24,42 @@ def show def create filename_with_extension = "#{params[:id]}.#{params[:format]}" - scratch_asset = ScratchAsset.find_or_initialize_by( + create_asset( project: @project_from_header, uploaded_user_id: current_user.id, filename: filename_with_extension ) + end + + def create_global + authorize! :create, @project_from_header + raise CanCan::AccessDenied unless experience_cs_public_project? + + create_asset(project: nil, uploaded_user_id: nil, filename: "#{params[:id]}.#{params[:format]}") + end + + private + + def create_asset(attributes) + scratch_asset = ScratchAsset.find_or_initialize_by(attributes) if scratch_asset.new_record? begin scratch_asset.save! - scratch_asset.file.attach(io: request.body, filename: filename_with_extension) rescue ActiveRecord::RecordNotUnique - logger.info("Scratch asset already created during concurrent upload: #{filename_with_extension}") - ScratchAsset.find_by!( - project: @project_from_header, - uploaded_user_id: current_user.id, - filename: filename_with_extension - ) + logger.info("Scratch asset already created during concurrent upload: #{attributes.fetch(:filename)}") + scratch_asset = ScratchAsset.find_by!(attributes) end end + scratch_asset.file.attach(io: request.body, filename: attributes.fetch(:filename)) unless scratch_asset.file.attached? + render json: { status: 'ok', 'content-name': params[:id] }, status: :created end - private + def experience_cs_public_project? + current_user.experience_cs_admin? && @project_from_header.user_id.nil? && @project_from_header.school_id.nil? + end def load_project_from_header identifier = request.headers['X-Project-ID'] diff --git a/app/controllers/concerns/identifiable.rb b/app/controllers/concerns/identifiable.rb index 6cfe879c0..af43b0b3e 100644 --- a/app/controllers/concerns/identifiable.rb +++ b/app/controllers/concerns/identifiable.rb @@ -22,6 +22,12 @@ def load_current_user RequestStore.store[:safeguarding_flag_users_by_token][token] = @current_user end + def load_experience_cs_service_user + @current_user = ExperienceCsServiceAuthenticator.authenticate( + request.headers[ExperienceCsServiceAuthenticator::HEADER] + ) + end + def extract_token(header) header.sub(/^Bearer\s+/i, '') end diff --git a/app/services/experience_cs_service_authenticator.rb b/app/services/experience_cs_service_authenticator.rb new file mode 100644 index 000000000..39ea3f5ab --- /dev/null +++ b/app/services/experience_cs_service_authenticator.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class ExperienceCsServiceAuthenticator + HEADER = 'X-Experience-CS-API-Key' + USER_ID = '00000000-0000-0000-0000-000000000000' + + def self.authenticate(candidate) + api_key = Rails.configuration.x.experience_cs.service_api_key + return if api_key.blank? || candidate.blank? + return unless ActiveSupport::SecurityUtils.secure_compare(candidate, api_key) + + User.new(id: USER_ID, roles: 'experience-cs-admin') + end +end diff --git a/config/application.rb b/config/application.rb index b4d3b7f3d..838921ca0 100644 --- a/config/application.rb +++ b/config/application.rb @@ -75,6 +75,7 @@ class Application < Rails::Application config.x.cloudflare_turnstile.secret_key = ENV.fetch('CLOUDFLARE_TURNSTILE_SECRET_KEY', nil) config.x.cloudflare_turnstile.enabled = ENV['CLOUDFLARE_TURNSTILE_SECRET_KEY'].present? + config.x.experience_cs.service_api_key = ENV.fetch('EXPERIENCE_CS_API_KEY', nil) if ENV['RAILS_LOG_TO_STDOUT'].present? config.rails_semantic_logger.appenders do |appenders| diff --git a/config/routes.rb b/config/routes.rb index 1ce967e76..f71d8c5d9 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -38,6 +38,7 @@ namespace :scratch do resources :projects, only: %i[show update create] get '/assets/internalapi/asset/:id.:format/get/' => 'assets#show' + post '/assets/global/:id.:format' => 'assets#create_global' post '/assets/:id.:format' => 'assets#create' end diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index 459272f65..49bc0dfb0 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -308,5 +308,27 @@ project = Project.find_by!(identifier: 'test-project', locale: 'fr') expect(project.scratch_component.content.to_h).to eq(scratch_data.deep_stringify_keys) end + + context 'when authenticated with the Experience CS service API key' do + let(:headers) { { ExperienceCsServiceAuthenticator::HEADER => 'service-api-key' } } + + before do + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') + end + + it 'creates the public project' do + post('/api/projects', headers:, params:, as: :json) + + expect(response).to have_http_status(:created) + expect(Project).to exist(identifier: 'test-project', locale: 'fr', user_id: nil) + end + + it 'does not authorize user-project creation' do + params[:project][:user_id] = SecureRandom.uuid + + expect { post('/api/projects', headers:, params:, as: :json) }.not_to change(Project, :count) + expect(response).to have_http_status(:forbidden) + end + end 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 1ab289110..6001dce22 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 @@ -402,6 +402,24 @@ end end + context 'when an Experience CS admin uses the regular upload endpoint' do + let(:experience_cs_admin) { create(:experience_cs_admin_user) } + let(:project) { create_scratch_project(locale: 'en', user_id: nil) } + + before do + authenticated_in_hydra_as(experience_cs_admin) + end + + it 'keeps the asset scoped to the project and uploading user' do + make_request + + asset = ScratchAsset.find_by!(filename:) + expect(asset.project).to eq(project) + expect(asset.uploaded_user_id).to eq(experience_cs_admin.id) + expect(asset.file.download).to eq(upload) + end + end + it 'responds 401 unauthorized when user is not signed in' do post '/api/scratch/assets/example.svg', headers: { 'X-Project-ID' => project.identifier } @@ -409,6 +427,90 @@ end end + describe 'POST #create_global' do + let(:upload) { File.binread(file_fixture(filename)) } + let(:project) { create_scratch_project(locale: 'en', user_id: nil) } + let(:request_headers) do + { + 'Authorization' => UserProfileMock::TOKEN, + 'Content-Type' => 'application/octet-stream', + 'X-Project-ID' => project.identifier + } + end + let(:make_request) do + post '/api/scratch/assets/global/test_image_1.png', headers: request_headers, params: upload + end + + context 'when an Experience CS admin syncs a public project' do + before do + authenticated_in_hydra_as(create(:experience_cs_admin_user)) + end + + it 'creates a global asset' do + make_request + + asset = ScratchAsset.find_by!(filename:) + expect(asset.project).to be_nil + expect(asset.uploaded_user_id).to be_nil + expect(asset.file.download).to eq(upload) + end + + it 'does not replace an existing global asset' do + existing_asset = create_uploaded_scratch_asset(filename:, project: nil, body: 'existing-body') + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:created) + expect(existing_asset.reload.file.download).to eq('existing-body') + end + + it 'repairs an existing global asset whose file was not attached' do + existing_asset = create(:scratch_asset, filename:, project: nil) + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(existing_asset.reload.file.download).to eq(upload) + end + + it 'makes the uploaded asset available without signing in' do + make_request + + get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', + headers: { 'X-Project-ID' => project.identifier } + follow_redirect! while response.redirect? + + expect(response.body).to eq(upload) + expect(response.media_type).to eq('image/png') + end + end + + context 'when an Experience CS admin syncs a school project' do + let(:project) { create_scratch_project(locale: 'en', user_id: nil, school:) } + + before do + authenticated_in_hydra_as(create(:experience_cs_admin_user)) + end + + it 'does not make its asset global' do + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:forbidden) + end + end + + context 'when a teacher is logged in' do + before do + authenticated_in_hydra_as(teacher) + end + + it 'does not allow a user-project asset to be made global' do + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:forbidden) + end + end + end + def create_scratch_project(**attributes) create(:project, { project_type: Project::Types::CODE_EDITOR_SCRATCH, locale: nil }.merge(attributes)).tap do |scratch_project| create(:scratch_component, project: scratch_project) diff --git a/spec/requests/projects/update_spec.rb b/spec/requests/projects/update_spec.rb index 89d006a94..4853a0672 100644 --- a/spec/requests/projects/update_spec.rb +++ b/spec/requests/projects/update_spec.rb @@ -180,6 +180,21 @@ .not_to change(ScratchComponent, :count) expect(project.scratch_component.reload.content.to_h).to eq(scratch_data.deep_stringify_keys) end + + context 'when authenticated with the Experience CS service API key' do + let(:headers) { { ExperienceCsServiceAuthenticator::HEADER => 'service-api-key' } } + + before do + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') + end + + it 'updates the public project' do + put('/api/projects/experience-cs-project?locale=fr', params:, headers:, as: :json) + + expect(response).to have_http_status(:ok) + expect(project.reload.project_type).to eq(Project::Types::CODE_EDITOR_SCRATCH) + end + end end context 'when authed user is a teacher' do diff --git a/spec/services/experience_cs_service_authenticator_spec.rb b/spec/services/experience_cs_service_authenticator_spec.rb new file mode 100644 index 000000000..9887fa857 --- /dev/null +++ b/spec/services/experience_cs_service_authenticator_spec.rb @@ -0,0 +1,38 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ExperienceCsServiceAuthenticator do + subject(:authenticate) { described_class.authenticate(candidate) } + + before do + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') + end + + context 'with the configured API key' do + let(:candidate) { 'service-api-key' } + + it 'returns an Experience CS service user', :aggregate_failures do + expect(authenticate.id).to eq(described_class::USER_ID) + expect(authenticate).to be_experience_cs_admin + end + end + + context 'with a different API key' do + let(:candidate) { 'wrong-api-key' } + + it 'does not authenticate' do + expect(authenticate).to be_nil + end + end + + context 'without a configured API key' do + let(:candidate) { 'service-api-key' } + + it 'does not authenticate' do + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return(nil) + + expect(authenticate).to be_nil + end + end +end From 399b26b71f5e955c6fc28c64d363d0071863df5e Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:30:33 +0200 Subject: [PATCH 2/6] Make global Scratch asset uploads idempotent --- .../api/scratch/assets_controller.rb | 50 +++++++++++++++- ...eating_and_showing_a_scratch_asset_spec.rb | 59 ++++++++++++++++++- 2 files changed, 104 insertions(+), 5 deletions(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index 0e9f6e85e..a31756ed1 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -1,10 +1,13 @@ # frozen_string_literal: true +require 'digest/md5' + module Api module Scratch class AssetsController < ApiController include ActiveStorage::SetCurrent + prepend_before_action :load_experience_cs_service_user, only: %i[create_global] before_action :authorize_user, except: %i[show] prepend_before_action :load_project_from_header, only: %i[show create create_global] authorize_resource :project_from_header, except: %i[create_global] @@ -35,12 +38,17 @@ def create_global authorize! :create, @project_from_header raise CanCan::AccessDenied unless experience_cs_public_project? - create_asset(project: nil, uploaded_user_id: nil, filename: "#{params[:id]}.#{params[:format]}") + create_asset( + project: nil, + uploaded_user_id: nil, + filename: "#{params[:id]}.#{params[:format]}", + reject_conflicting_content: true + ) end private - def create_asset(attributes) + def create_asset(reject_conflicting_content: false, **attributes) scratch_asset = ScratchAsset.find_or_initialize_by(attributes) if scratch_asset.new_record? @@ -52,11 +60,47 @@ def create_asset(attributes) end end - scratch_asset.file.attach(io: request.body, filename: attributes.fetch(:filename)) unless scratch_asset.file.attached? + if reject_conflicting_content + return if attach_global_file(scratch_asset, attributes.fetch(:filename)) == :conflict + else + attach_file_unless_present(scratch_asset, attributes.fetch(:filename)) + end render json: { status: 'ok', 'content-name': params[:id] }, status: :created end + def attach_global_file(scratch_asset, filename) + scratch_asset.with_lock do + if scratch_asset.file.attached? + next :unchanged if global_file_matches?(scratch_asset) + + next reject_conflicting_global_file + end + + scratch_asset.file.attach(io: request.body, filename:) + :attached + end + end + + def attach_file_unless_present(scratch_asset, filename) + scratch_asset.file.attach(io: request.body, filename:) unless scratch_asset.file.attached? + end + + def global_file_matches?(scratch_asset) + scratch_asset.file.blob.checksum == request_body_checksum + end + + def reject_conflicting_global_file + render json: { error: 'Asset content conflicts with the existing global asset' }, status: :conflict + :conflict + end + + def request_body_checksum + @request_body_checksum ||= Digest::MD5.base64digest(request.body.read) + ensure + request.body.rewind + end + def experience_cs_public_project? current_user.experience_cs_admin? && @project_from_header.user_id.nil? && @project_from_header.school_id.nil? 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 6001dce22..249f0d08f 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 @@ -455,12 +455,24 @@ expect(asset.file.download).to eq(upload) end - it 'does not replace an existing global asset' do - existing_asset = create_uploaded_scratch_asset(filename:, project: nil, body: 'existing-body') + it 'accepts an existing global asset with the same content' do + existing_asset = create_uploaded_scratch_asset(filename:, project: nil, body: upload) expect { make_request }.not_to change(ScratchAsset, :count) expect(response).to have_http_status(:created) + expect(existing_asset.reload.file.download).to eq(upload) + end + + it 'rejects an existing global asset with conflicting content' do + existing_asset = create_uploaded_scratch_asset(filename:, project: nil, body: 'existing-body') + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:conflict) + expect(response.parsed_body).to eq( + 'error' => 'Asset content conflicts with the existing global asset' + ) expect(existing_asset.reload.file.download).to eq('existing-body') end @@ -484,6 +496,49 @@ end end + context 'when the Experience CS service syncs a public project' do + let(:request_headers) do + super().except('Authorization').merge(ExperienceCsServiceAuthenticator::HEADER => 'service-api-key') + end + + before do + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') + end + + it 'creates the global asset' do + expect { make_request }.to change(ScratchAsset, :count).by(1) + + expect(response).to have_http_status(:created) + expect(ScratchAsset.find_by!(filename:).file.download).to eq(upload) + end + + context 'when the project belongs to a user' do + let(:project) { create_scratch_project(locale: 'en', user_id: SecureRandom.uuid) } + + it 'does not create a global asset' do + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:forbidden) + end + end + end + + context 'when the Experience CS service key is invalid' do + let(:request_headers) do + super().except('Authorization').merge(ExperienceCsServiceAuthenticator::HEADER => 'wrong-api-key') + end + + before do + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') + end + + it 'rejects the upload' do + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:unauthorized) + end + end + context 'when an Experience CS admin syncs a school project' do let(:project) { create_scratch_project(locale: 'en', user_id: nil, school:) } From c43fcb4d387385033bb41e7e8428e44e5c6d5e33 Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Wed, 19 Aug 2026 13:18:08 +0200 Subject: [PATCH 3/6] Restrict Experience CS service project access --- app/controllers/api/projects_controller.rb | 34 +++++++++++++++++++ .../project/creating_a_project_spec.rb | 24 +++++++++++++ spec/requests/projects/update_spec.rb | 24 +++++++++++++ 3 files changed, 82 insertions(+) diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index 91817b2f6..2d1c49e2f 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -4,11 +4,14 @@ module Api class ProjectsController < ApiController + EXPERIENCE_CS_SERVICE_PROJECT_TYPES = [Project::Types::SCRATCH, Project::Types::CODE_EDITOR_SCRATCH].freeze + prepend_before_action :load_experience_cs_service_user, only: %i[create update] before_action :authorize_user, only: %i[create update index destroy] before_action :load_project, only: %i[show update destroy show_context] before_action :load_projects, only: %i[index] load_and_authorize_resource + before_action :authorize_experience_cs_service_project, only: %i[create update] before_action :verify_lesson_belongs_to_school, only: :create after_action :pagination_link_header, only: %i[index] @@ -64,6 +67,37 @@ def show_context private + def authorize_experience_cs_service_project + return unless current_user&.id == ExperienceCsServiceAuthenticator::USER_ID + return if experience_cs_service_project? + + raise CanCan::AccessDenied + end + + def experience_cs_service_project? + return public_scratch_project_attributes?(experience_cs_service_project_attributes) if action_name == 'create' + + public_scratch_project_attributes?(@project.attributes.symbolize_keys) && + public_scratch_project_attributes?(experience_cs_service_project_attributes) + end + + def experience_cs_service_project_attributes + existing_attributes = if action_name == 'create' + { user_id: nil, school_id: nil, project_type: Project.column_defaults['project_type'] } + else + @project.attributes.symbolize_keys + end + requested_attributes = base_params.slice(:user_id, :school_id, :project_type).to_h.symbolize_keys + + existing_attributes.merge(requested_attributes) + end + + def public_scratch_project_attributes?(attributes) + attributes[:user_id].nil? && + attributes[:school_id].nil? && + EXPERIENCE_CS_SERVICE_PROJECT_TYPES.include?(attributes[:project_type]) + end + def verify_lesson_belongs_to_school return if base_params[:lesson_id].blank? return if school&.lessons&.pluck(:id)&.include?(base_params[:lesson_id]) diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index 49bc0dfb0..cb2a2c85b 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -323,12 +323,36 @@ expect(Project).to exist(identifier: 'test-project', locale: 'fr', user_id: nil) end + it 'creates a public legacy Scratch project' do + params[:project].except!(:instructions, :scratch_component) + params[:project][:project_type] = Project::Types::SCRATCH + + post('/api/projects', headers:, params:, as: :json) + + expect(response).to have_http_status(:created) + expect(Project).to exist(identifier: 'test-project', locale: 'fr', project_type: Project::Types::SCRATCH) + end + it 'does not authorize user-project creation' do params[:project][:user_id] = SecureRandom.uuid expect { post('/api/projects', headers:, params:, as: :json) }.not_to change(Project, :count) expect(response).to have_http_status(:forbidden) end + + it 'does not authorize school-project creation' do + params[:project][:school_id] = create(:school).id + + expect { post('/api/projects', headers:, params:, as: :json) }.not_to change(Project, :count) + expect(response).to have_http_status(:forbidden) + end + + it 'does not authorize non-Scratch project creation' do + params[:project][:project_type] = Project::Types::PYTHON + + expect { post('/api/projects', headers:, params:, as: :json) }.not_to change(Project, :count) + expect(response).to have_http_status(:forbidden) + end end end end diff --git a/spec/requests/projects/update_spec.rb b/spec/requests/projects/update_spec.rb index 4853a0672..a55246ad3 100644 --- a/spec/requests/projects/update_spec.rb +++ b/spec/requests/projects/update_spec.rb @@ -194,6 +194,30 @@ expect(response).to have_http_status(:ok) expect(project.reload.project_type).to eq(Project::Types::CODE_EDITOR_SCRATCH) end + + it 'does not authorize updates to a school project' do + project.update!(school: create(:school)) + + put('/api/projects/experience-cs-project?locale=fr', params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + end + + it 'does not authorize updates to a non-Scratch project' do + project.update!(project_type: Project::Types::PYTHON) + + put('/api/projects/experience-cs-project?locale=fr', params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + end + + it 'does not authorize changing the project to a non-Scratch type' do + params[:project][:project_type] = Project::Types::PYTHON + + put('/api/projects/experience-cs-project?locale=fr', params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + end end end From b8ed3d2d6aef20da205135c055d82512c97dfe47 Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Thu, 20 Aug 2026 12:56:23 +0200 Subject: [PATCH 4/6] address PR comments --- app/controllers/api/projects_controller.rb | 32 ++++++----------- app/models/project.rb | 6 ++++ app/models/user.rb | 6 ++++ .../experience_cs_service_authenticator.rb | 3 +- spec/models/project_spec.rb | 35 +++++++++++++++++++ spec/models/user_spec.rb | 12 +++++++ spec/requests/projects/update_spec.rb | 24 +++++++++++++ ...xperience_cs_service_authenticator_spec.rb | 2 +- 8 files changed, 95 insertions(+), 25 deletions(-) diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index 2d1c49e2f..9da91fdd9 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -4,8 +4,6 @@ module Api class ProjectsController < ApiController - EXPERIENCE_CS_SERVICE_PROJECT_TYPES = [Project::Types::SCRATCH, Project::Types::CODE_EDITOR_SCRATCH].freeze - prepend_before_action :load_experience_cs_service_user, only: %i[create update] before_action :authorize_user, only: %i[create update index destroy] before_action :load_project, only: %i[show update destroy show_context] @@ -68,34 +66,24 @@ def show_context private def authorize_experience_cs_service_project - return unless current_user&.id == ExperienceCsServiceAuthenticator::USER_ID - return if experience_cs_service_project? + return unless current_user&.experience_cs_service_account? + return if experience_cs_service_project_change_permitted? raise CanCan::AccessDenied end - def experience_cs_service_project? - return public_scratch_project_attributes?(experience_cs_service_project_attributes) if action_name == 'create' + def experience_cs_service_project_change_permitted? + return false if action_name == 'update' && !@project.public_experience_cs_project? - public_scratch_project_attributes?(@project.attributes.symbolize_keys) && - public_scratch_project_attributes?(experience_cs_service_project_attributes) + requested_experience_cs_project.public_experience_cs_project? end - def experience_cs_service_project_attributes - existing_attributes = if action_name == 'create' - { user_id: nil, school_id: nil, project_type: Project.column_defaults['project_type'] } - else - @project.attributes.symbolize_keys - end - requested_attributes = base_params.slice(:user_id, :school_id, :project_type).to_h.symbolize_keys - - existing_attributes.merge(requested_attributes) - end + def requested_experience_cs_project + project = action_name == 'create' ? Project.new : @project.dup + attributes = base_params.slice(:user_id, :school_id, :project_type).to_h + project.assign_attributes(attributes) - def public_scratch_project_attributes?(attributes) - attributes[:user_id].nil? && - attributes[:school_id].nil? && - EXPERIENCE_CS_SERVICE_PROJECT_TYPES.include?(attributes[:project_type]) + project end def verify_lesson_belongs_to_school diff --git a/app/models/project.rb b/app/models/project.rb index 1da4542bb..6fcaf5835 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -8,6 +8,8 @@ module Types CODE_EDITOR_SCRATCH = 'code_editor_scratch' end + EXPERIENCE_CS_PROJECT_TYPES = [Types::SCRATCH, Types::CODE_EDITOR_SCRATCH].freeze + belongs_to :school, optional: true belongs_to :lesson, optional: true belongs_to :parent, optional: true, class_name: :Project, foreign_key: :remixed_from_id, inverse_of: :remixes @@ -96,6 +98,10 @@ def scratch_project? project_type == Types::CODE_EDITOR_SCRATCH end + def public_experience_cs_project? + user_id.nil? && school_id.nil? && EXPERIENCE_CS_PROJECT_TYPES.include?(project_type) + end + def self_and_ancestors projects = [] current_project = self diff --git a/app/models/user.rb b/app/models/user.rb index a564d572c..5bc5d8fff 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,6 +4,8 @@ class User include ActiveModel::Serialization include ActiveModel::Model + EXPERIENCE_CS_SERVICE_ACCOUNT_ID = '00000000-0000-0000-0000-000000000000' + ATTRIBUTES = %w[ country country_code @@ -59,6 +61,10 @@ def experience_cs_admin? parsed_roles.include?('experience-cs-admin') end + def experience_cs_service_account? + id == EXPERIENCE_CS_SERVICE_ACCOUNT_ID + end + def parsed_roles roles&.to_s&.split(',')&.map(&:strip) || [] end diff --git a/app/services/experience_cs_service_authenticator.rb b/app/services/experience_cs_service_authenticator.rb index 39ea3f5ab..cf182b46d 100644 --- a/app/services/experience_cs_service_authenticator.rb +++ b/app/services/experience_cs_service_authenticator.rb @@ -2,13 +2,12 @@ class ExperienceCsServiceAuthenticator HEADER = 'X-Experience-CS-API-Key' - USER_ID = '00000000-0000-0000-0000-000000000000' def self.authenticate(candidate) api_key = Rails.configuration.x.experience_cs.service_api_key return if api_key.blank? || candidate.blank? return unless ActiveSupport::SecurityUtils.secure_compare(candidate, api_key) - User.new(id: USER_ID, roles: 'experience-cs-admin') + User.new(id: User::EXPERIENCE_CS_SERVICE_ACCOUNT_ID, roles: 'experience-cs-admin') end end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 14ca4c4ad..e2f044a20 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -197,6 +197,41 @@ end end + describe '#public_experience_cs_project?' do + it 'returns true for public Experience CS project types', :aggregate_failures do + project_types = [described_class::Types::SCRATCH, described_class::Types::CODE_EDITOR_SCRATCH] + + project_types.each do |project_type| + project = build(:project, project_type:, user_id: nil, school_id: nil) + + expect(project).to be_public_experience_cs_project + end + end + + it 'returns false for a user-owned project' do + project = build(:project, project_type: described_class::Types::SCRATCH) + + expect(project).not_to be_public_experience_cs_project + end + + it 'returns false for a school-owned project' do + project = build( + :project, + project_type: described_class::Types::SCRATCH, + user_id: nil, + school_id: SecureRandom.uuid + ) + + expect(project).not_to be_public_experience_cs_project + end + + it 'returns false for a non-Scratch project' do + project = build(:project, project_type: described_class::Types::PYTHON, user_id: nil) + + expect(project).not_to be_public_experience_cs_project + end + end + describe 'create_school_project_if_needed' do let(:teacher) { create(:teacher, school:) } let(:teacher_project) { create(:project, school_id: school.id, user_id: teacher.id) } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 5e2529055..7b8a76568 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -312,6 +312,18 @@ end end + describe '#experience_cs_service_account?' do + it 'returns true for the Experience CS service account' do + user = build(:user, id: described_class::EXPERIENCE_CS_SERVICE_ACCOUNT_ID) + + expect(user).to be_experience_cs_service_account + end + + it 'returns false for another user' do + expect(user).not_to be_experience_cs_service_account + end + end + describe '#school_roles' do subject(:user) { build(:user) } diff --git a/spec/requests/projects/update_spec.rb b/spec/requests/projects/update_spec.rb index a55246ad3..876983cee 100644 --- a/spec/requests/projects/update_spec.rb +++ b/spec/requests/projects/update_spec.rb @@ -203,6 +203,14 @@ expect(response).to have_http_status(:forbidden) end + it 'does not authorize updates to a user-owned project' do + project.update!(user_id: SecureRandom.uuid) + + put('/api/projects/experience-cs-project?locale=fr', params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + end + it 'does not authorize updates to a non-Scratch project' do project.update!(project_type: Project::Types::PYTHON) @@ -218,6 +226,22 @@ expect(response).to have_http_status(:forbidden) end + + it 'does not authorize assigning the project to a user' do + params[:project][:user_id] = SecureRandom.uuid + + put('/api/projects/experience-cs-project?locale=fr', params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + end + + it 'does not authorize assigning the project to a school' do + params[:project][:school_id] = create(:school).id + + put('/api/projects/experience-cs-project?locale=fr', params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + end end end diff --git a/spec/services/experience_cs_service_authenticator_spec.rb b/spec/services/experience_cs_service_authenticator_spec.rb index 9887fa857..0d68e86f6 100644 --- a/spec/services/experience_cs_service_authenticator_spec.rb +++ b/spec/services/experience_cs_service_authenticator_spec.rb @@ -13,7 +13,7 @@ let(:candidate) { 'service-api-key' } it 'returns an Experience CS service user', :aggregate_failures do - expect(authenticate.id).to eq(described_class::USER_ID) + expect(authenticate).to be_experience_cs_service_account expect(authenticate).to be_experience_cs_admin end end From e124b1e28242d1f86984e7ae7288f7156dcc0627 Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Thu, 20 Aug 2026 15:40:15 +0200 Subject: [PATCH 5/6] add correct cancan verification --- app/controllers/api/scratch/assets_controller.rb | 8 ++------ app/models/ability.rb | 1 + spec/models/ability_spec.rb | 14 ++++++++++++++ 3 files changed, 17 insertions(+), 6 deletions(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index a31756ed1..2ea985f01 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -35,8 +35,8 @@ def create end def create_global - authorize! :create, @project_from_header - raise CanCan::AccessDenied unless experience_cs_public_project? + authorize! :create_global, ScratchAsset + raise CanCan::AccessDenied unless @project_from_header.public_experience_cs_project? create_asset( project: nil, @@ -101,10 +101,6 @@ def request_body_checksum request.body.rewind end - def experience_cs_public_project? - current_user.experience_cs_admin? && @project_from_header.user_id.nil? && @project_from_header.school_id.nil? - end - def load_project_from_header identifier = request.headers['X-Project-ID'] return render json: { error: 'X-Project-ID header is required' }, status: :bad_request if identifier.blank? diff --git a/app/models/ability.rb b/app/models/ability.rb index c8463d1fa..27cd52f59 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -150,6 +150,7 @@ def define_experience_cs_admin_abilities(user) return unless user&.experience_cs_admin? can %i[read create update destroy], Project, user_id: nil + can :create_global, ScratchAsset define_school_import_abilities(user) end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index fb9965129..c648e19aa 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -18,6 +18,20 @@ it { is_expected.not_to be_able_to(:read, build(:teacher_invitation, email_address: nil)) } end + describe 'ScratchAsset' do + context 'with an Experience CS admin' do + let(:user) { build(:experience_cs_admin_user) } + + it { is_expected.to be_able_to(:create_global, ScratchAsset) } + end + + context 'with a standard user' do + let(:user) { build(:user) } + + it { is_expected.not_to be_able_to(:create_global, ScratchAsset) } + end + end + describe 'Project' do context 'with no user' do let(:user) { nil } From 757150d4abd3c8e5db495623d41b8fe9427a08d8 Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Thu, 20 Aug 2026 17:19:03 +0200 Subject: [PATCH 6/6] update controller to avoid the header --- .../api/scratch/assets_controller.rb | 3 +- ...eating_and_showing_a_scratch_asset_spec.rb | 33 +++---------------- 2 files changed, 5 insertions(+), 31 deletions(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index 2ea985f01..ac8a7fc65 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -9,7 +9,7 @@ class AssetsController < ApiController prepend_before_action :load_experience_cs_service_user, only: %i[create_global] before_action :authorize_user, except: %i[show] - prepend_before_action :load_project_from_header, only: %i[show create create_global] + prepend_before_action :load_project_from_header, only: %i[show create] authorize_resource :project_from_header, except: %i[create_global] def show @@ -36,7 +36,6 @@ def create def create_global authorize! :create_global, ScratchAsset - raise CanCan::AccessDenied unless @project_from_header.public_experience_cs_project? create_asset( project: nil, 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 249f0d08f..c5bc96735 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 @@ -433,15 +433,14 @@ let(:request_headers) do { 'Authorization' => UserProfileMock::TOKEN, - 'Content-Type' => 'application/octet-stream', - 'X-Project-ID' => project.identifier + 'Content-Type' => 'application/octet-stream' } end let(:make_request) do post '/api/scratch/assets/global/test_image_1.png', headers: request_headers, params: upload end - context 'when an Experience CS admin syncs a public project' do + context 'when an Experience CS admin uploads a global asset' do before do authenticated_in_hydra_as(create(:experience_cs_admin_user)) end @@ -496,7 +495,7 @@ end end - context 'when the Experience CS service syncs a public project' do + context 'when the Experience CS service uploads a global asset' do let(:request_headers) do super().except('Authorization').merge(ExperienceCsServiceAuthenticator::HEADER => 'service-api-key') end @@ -511,16 +510,6 @@ expect(response).to have_http_status(:created) expect(ScratchAsset.find_by!(filename:).file.download).to eq(upload) end - - context 'when the project belongs to a user' do - let(:project) { create_scratch_project(locale: 'en', user_id: SecureRandom.uuid) } - - it 'does not create a global asset' do - expect { make_request }.not_to change(ScratchAsset, :count) - - expect(response).to have_http_status(:forbidden) - end - end end context 'when the Experience CS service key is invalid' do @@ -539,26 +528,12 @@ end end - context 'when an Experience CS admin syncs a school project' do - let(:project) { create_scratch_project(locale: 'en', user_id: nil, school:) } - - before do - authenticated_in_hydra_as(create(:experience_cs_admin_user)) - end - - it 'does not make its asset global' do - expect { make_request }.not_to change(ScratchAsset, :count) - - expect(response).to have_http_status(:forbidden) - end - end - context 'when a teacher is logged in' do before do authenticated_in_hydra_as(teacher) end - it 'does not allow a user-project asset to be made global' do + it 'does not allow a global asset to be created' do expect { make_request }.not_to change(ScratchAsset, :count) expect(response).to have_http_status(:forbidden)