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..9da91fdd9 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -4,10 +4,12 @@ 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] 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] @@ -63,6 +65,27 @@ def show_context private + def authorize_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_change_permitted? + return false if action_name == 'update' && !@project.public_experience_cs_project? + + requested_experience_cs_project.public_experience_cs_project? + 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) + + project + 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/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index e2b811715..ac8a7fc65 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -1,13 +1,16 @@ # 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] - authorize_resource :project_from_header + authorize_resource :project_from_header, except: %i[create_global] def show filename_with_extension = "#{params[:id]}.#{params[:format]}" @@ -24,30 +27,78 @@ 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_global, ScratchAsset + + create_asset( + project: nil, + uploaded_user_id: nil, + filename: "#{params[:id]}.#{params[:format]}", + reject_conflicting_content: true + ) + end + + private + + def create_asset(reject_conflicting_content: false, **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 + 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 - private + 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 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/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/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 new file mode 100644 index 000000000..cf182b46d --- /dev/null +++ b/app/services/experience_cs_service_authenticator.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class ExperienceCsServiceAuthenticator + HEADER = 'X-Experience-CS-API-Key' + + 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::EXPERIENCE_CS_SERVICE_ACCOUNT_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..cb2a2c85b 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -308,5 +308,51 @@ 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 '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/features/scratch/creating_and_showing_a_scratch_asset_spec.rb b/spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb index 1ab289110..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 @@ -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,120 @@ 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' + } + 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 uploads a global asset' 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 '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 + + 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 the Experience CS service uploads a global asset' 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 + 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 a teacher is logged in' do + before do + authenticated_in_hydra_as(teacher) + end + + 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) + 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/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 } 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 89d006a94..876983cee 100644 --- a/spec/requests/projects/update_spec.rb +++ b/spec/requests/projects/update_spec.rb @@ -180,6 +180,69 @@ .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 + + 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 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) + + 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 + + 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 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..0d68e86f6 --- /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).to be_experience_cs_service_account + 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