From 7247b5c534e400a04e874afd74ca5faa5ef55e91 Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:00:00 +0200 Subject: [PATCH 1/5] Define Experience CS migration permissions --- app/models/ability.rb | 13 +++++++ app/models/project.rb | 6 +++ ...d_experience_cs_migrated_at_to_projects.rb | 7 ++++ db/schema.rb | 3 +- spec/models/ability_spec.rb | 39 +++++++++++++++++++ spec/models/project_spec.rb | 38 ++++++++++++++++++ 6 files changed, 105 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb diff --git a/app/models/ability.rb b/app/models/ability.rb index cd10e3cac..cadd6aa44 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -18,6 +18,7 @@ def initialize(user) define_editor_admin_abilities(user) define_experience_cs_admin_abilities(user) + define_experience_cs_service_abilities(user) end private @@ -72,6 +73,12 @@ def define_school_owner_abilities(school:) can(%i[read], :school_member) can(%i[read create import update destroy regenerate_join_code], SchoolClass, school: { id: school.id }) can(%i[read update show_context], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) + lesson_project_ids = Project.where( + school_id: school.id, + remixed_from_id: nil, + lesson_id: Lesson.where(school_id: school.id, visibility: %w[teachers students]).select(:id) + ).pluck(:id) + can(%i[read show_context], Project, school_id: school.id, remixed_from_id: lesson_project_ids) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } }) can(%i[read create destroy], :school_owner) can(%i[read create destroy], :school_teacher) @@ -152,6 +159,12 @@ def define_experience_cs_admin_abilities(user) define_school_import_abilities(user) end + def define_experience_cs_service_abilities(user) + return unless user&.experience_cs_service_account? + + can %i[migrate_from_experience_cs upload_migration_asset], Project, &:experience_cs_migration_target? + end + def school_teacher_can_manage_lesson?(user:, school:, lesson:) is_my_lesson = lesson.school_id == school.id && lesson.user_id == user.id is_my_class = lesson.school_class&.teacher_ids&.include?(user.id) diff --git a/app/models/project.rb b/app/models/project.rb index 8315c3700..152b10209 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -108,6 +108,12 @@ def public_experience_cs_project? user_id.nil? && school_id.nil? && EXPERIENCE_CS_PROJECT_TYPES.include?(project_type) end + def experience_cs_migration_target? + return false unless user_id.present? && school_id.present? + + project_type == Types::SCRATCH || experience_cs_migrated_at.present? + end + def self_and_ancestors projects = [] current_project = self diff --git a/db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb b/db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb new file mode 100644 index 000000000..180368ed4 --- /dev/null +++ b/db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddExperienceCsMigratedAtToProjects < ActiveRecord::Migration[8.1] + def change + add_column :projects, :experience_cs_migrated_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index e21d13889..35e0bd2a9 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_08_20_122510) do +ActiveRecord::Schema[8.1].define(version: 2026_08_24_120000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -234,6 +234,7 @@ create_table "projects", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.datetime "created_at", null: false + t.datetime "experience_cs_migrated_at" t.string "identifier", null: false t.jsonb "instruction_steps" t.text "instructions" diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 5f34facff..dbcc8a2b3 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -32,6 +32,45 @@ end end + describe 'Experience CS service account' do + let(:user) do + build( + :user, + id: User::EXPERIENCE_CS_SERVICE_ACCOUNT_ID, + roles: 'experience-cs-admin' + ) + end + let(:migratable_project) do + build( + :project, + school_id: SecureRandom.uuid, + user_id: SecureRandom.uuid, + locale: nil, + project_type: Project::Types::SCRATCH + ) + end + + it { is_expected.to be_able_to(:migrate_from_experience_cs, migratable_project) } + it { is_expected.to be_able_to(:upload_migration_asset, migratable_project) } + it { is_expected.not_to be_able_to(:update, migratable_project) } + it { is_expected.not_to be_able_to(:create, migratable_project) } + it { is_expected.not_to be_able_to(:show, migratable_project) } + + it 'cannot migrate a native Code Classroom Scratch project' do + migratable_project.project_type = Project::Types::CODE_EDITOR_SCRATCH + + expect(ability).not_to be_able_to(:migrate_from_experience_cs, migratable_project) + expect(ability).not_to be_able_to(:upload_migration_asset, migratable_project) + end + + it 'can replay a previously migrated Experience CS project' do + migratable_project.project_type = Project::Types::CODE_EDITOR_SCRATCH + migratable_project.experience_cs_migrated_at = Time.current + + expect(ability).to be_able_to(:migrate_from_experience_cs, migratable_project) + 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 cbfe99353..a7bed37ab 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -265,6 +265,44 @@ end end + describe '#experience_cs_migration_target?' do + let(:project) do + build( + :project, + school_id: SecureRandom.uuid, + user_id: SecureRandom.uuid, + locale: nil, + project_type: described_class::Types::SCRATCH + ) + end + + it 'allows a user-owned school legacy Scratch stub' do + expect(project).to be_experience_cs_migration_target + end + + it 'allows a replay after a successful migration' do + project.project_type = described_class::Types::CODE_EDITOR_SCRATCH + project.experience_cs_migrated_at = Time.current + + expect(project).to be_experience_cs_migration_target + end + + it 'rejects an unmarked native Code Classroom Scratch project' do + project.project_type = described_class::Types::CODE_EDITOR_SCRATCH + + expect(project).not_to be_experience_cs_migration_target + end + + it 'rejects public and non-school projects', :aggregate_failures do + project.user_id = nil + expect(project).not_to be_experience_cs_migration_target + + project.user_id = SecureRandom.uuid + project.school_id = nil + expect(project).not_to be_experience_cs_migration_target + 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) } From d266b55a4cb0ab1082cb2e743ebcd784015c489a Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:00:11 +0200 Subject: [PATCH 2/5] Add Experience CS project migration endpoint --- ...rience_cs_project_migrations_controller.rb | 53 +++++++++++ config/routes.rb | 2 + .../update_spec.rb | 93 +++++++++++++++++++ 3 files changed, 148 insertions(+) create mode 100644 app/controllers/api/experience_cs_project_migrations_controller.rb create mode 100644 spec/requests/experience_cs_project_migrations/update_spec.rb diff --git a/app/controllers/api/experience_cs_project_migrations_controller.rb b/app/controllers/api/experience_cs_project_migrations_controller.rb new file mode 100644 index 000000000..f0a6164d4 --- /dev/null +++ b/app/controllers/api/experience_cs_project_migrations_controller.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +module Api + class ExperienceCsProjectMigrationsController < ApiController + prepend_before_action :load_experience_cs_service_user + before_action :authorize_user + before_action :load_project + before_action :authorize_migration + + def update + migrate_project! + render json: { + identifier: @project.identifier, + locale: @project.locale, + project_type: @project.project_type + } + rescue ActiveRecord::RecordInvalid, ActionController::ParameterMissing => e + render json: { error: e.message }, status: :unprocessable_content + end + + private + + def load_project + @project = Project.find_by!(identifier: params.expect(:id), locale: nil) + end + + def authorize_migration + authorize! :migrate_from_experience_cs, @project + end + + def migrate_project! + attributes = migration_params + @project.with_lock do + @project.update!( + attributes.slice(:name, :instructions).merge( + project_type: Project::Types::CODE_EDITOR_SCRATCH, + experience_cs_migrated_at: @project.experience_cs_migrated_at || Time.current + ) + ) + scratch_component = @project.scratch_component || @project.build_scratch_component + scratch_component.update!(attributes.require(:scratch_component).slice(:content)) + end + end + + def migration_params + params.fetch(:project, {}).permit( + :name, + { instructions: [:markdown_content] }, + { scratch_component: { content: {} } } + ) + end + end +end diff --git a/config/routes.rb b/config/routes.rb index f71d8c5d9..698b32c08 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -35,6 +35,8 @@ mount GraphiQL::Rails::Engine, at: '/graphql', graphql_path: '/graphql#execute' unless Rails.env.production? namespace :api do + put '/experience-cs/projects/:id/migrate', to: 'experience_cs_project_migrations#update' + namespace :scratch do resources :projects, only: %i[show update create] get '/assets/internalapi/asset/:id.:format/get/' => 'assets#show' diff --git a/spec/requests/experience_cs_project_migrations/update_spec.rb b/spec/requests/experience_cs_project_migrations/update_spec.rb new file mode 100644 index 000000000..027ec8e29 --- /dev/null +++ b/spec/requests/experience_cs_project_migrations/update_spec.rb @@ -0,0 +1,93 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe 'Experience CS project migration requests' do + let(:headers) { { ExperienceCsServiceAuthenticator::HEADER => 'service-api-key' } } + let(:school) { create(:school) } + let(:owner) { create(:teacher, school:) } + let(:project) do + create( + :project, + identifier: 'user-project-stub', + school:, + user_id: owner.id, + locale: nil, + project_type: Project::Types::SCRATCH + ) + end + let(:scratch_data) { { targets: [], monitors: [], extensions: [], meta: {} } } + let(:params) do + { + project: { + name: 'Migrated project', + instructions: [{ markdown_content: 'Make the sprite move.' }], + scratch_component: { content: scratch_data } + } + } + end + let(:path) { '/api/experience-cs/projects/user-project-stub/migrate' } + + before do + project + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') + end + + it 'replaces the exact stub in place' do + original_id = project.id + + put(path, params:, headers:, as: :json) + + expect(response).to have_http_status(:ok) + expect(project.reload).to have_attributes( + id: original_id, + identifier: 'user-project-stub', + locale: nil, + user_id: owner.id, + school_id: school.id, + name: 'Migrated project', + instructions: [{ 'markdown_content' => 'Make the sprite move.' }], + project_type: Project::Types::CODE_EDITOR_SCRATCH + ) + expect(project.scratch_component.content.to_h).to eq(scratch_data.deep_stringify_keys) + end + + it 'allows an idempotent replay' do + 2.times { put(path, params:, headers:, as: :json) } + + expect(response).to have_http_status(:ok) + expect(Project.where(id: project.id).count).to eq(1) + end + + it 'does not authorize a human Experience CS admin' do + authenticated_in_hydra_as(create(:experience_cs_admin_user)) + + put(path, params:, headers: { Authorization: UserProfileMock::TOKEN }, as: :json) + + expect(response).to have_http_status(:forbidden) + end + + it 'does not overwrite a native Code Classroom project' do + project.update!(project_type: Project::Types::CODE_EDITOR_SCRATCH) + + put(path, params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + end + + it 'does not fall back to a public locale' do + project.destroy! + public_project = create( + :project, + identifier: 'user-project-stub', + locale: 'en', + user_id: nil, + project_type: Project::Types::SCRATCH + ) + + put(path, params:, headers:, as: :json) + + expect(response).to have_http_status(:not_found) + expect(public_project.reload.project_type).to eq(Project::Types::SCRATCH) + end +end From f2e3f52ff80e0771ac66fc07a9c3085faee36746 Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Mon, 24 Aug 2026 15:00:20 +0200 Subject: [PATCH 3/5] Keep migrated project assets private --- README.md | 10 +- .../api/scratch/assets_controller.rb | 66 +++++++--- app/models/scratch_asset.rb | 2 +- ...eating_and_showing_a_scratch_asset_spec.rb | 117 ++++++++++++++++++ 4 files changed, 176 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index cd36853c7..376b96bc0 100644 --- a/README.md +++ b/README.md @@ -164,8 +164,14 @@ A project remix is created via a `POST` request to `projects/{original_project_i 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. +accepted for public project create/update and global Scratch asset upload. + +`PUT /api/experience-cs/projects/:identifier/migrate` lets the service replace +an exact locale-less legacy user-project stub with its Markdown instructions +and Scratch content. Successful migrations are marked so retries remain safe +without granting access to native Code Classroom projects. Project-scoped asset +uploads use `X-Project-ID` and remain subject to the project's normal viewing +permissions. ### Code Editor for Education diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index ac8a7fc65..e359cb1de 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -7,10 +7,10 @@ module Scratch class AssetsController < ApiController include ActiveStorage::SetCurrent - prepend_before_action :load_experience_cs_service_user, only: %i[create_global] + prepend_before_action :load_experience_cs_service_user, only: :create_global + prepend_before_action :load_project_asset_context, only: %i[show create] before_action :authorize_user, except: %i[show] - prepend_before_action :load_project_from_header, only: %i[show create] - authorize_resource :project_from_header, except: %i[create_global] + before_action :authorize_project_from_header, except: %i[create_global] def show filename_with_extension = "#{params[:id]}.#{params[:format]}" @@ -29,8 +29,9 @@ def create filename_with_extension = "#{params[:id]}.#{params[:format]}" create_asset( project: @project_from_header, - uploaded_user_id: current_user.id, - filename: filename_with_extension + uploaded_user_id: asset_uploader_id, + filename: filename_with_extension, + reject_conflicting_content: current_user.experience_cs_service_account? ) end @@ -60,7 +61,7 @@ def create_asset(reject_conflicting_content: false, **attributes) end if reject_conflicting_content - return if attach_global_file(scratch_asset, attributes.fetch(:filename)) == :conflict + return if attach_file_with_conflict_check(scratch_asset, attributes.fetch(:filename)) == :conflict else attach_file_unless_present(scratch_asset, attributes.fetch(:filename)) end @@ -68,12 +69,12 @@ def create_asset(reject_conflicting_content: false, **attributes) render json: { status: 'ok', 'content-name': params[:id] }, status: :created end - def attach_global_file(scratch_asset, filename) + def attach_file_with_conflict_check(scratch_asset, filename) scratch_asset.with_lock do if scratch_asset.file.attached? - next :unchanged if global_file_matches?(scratch_asset) + next :unchanged if file_matches?(scratch_asset) - next reject_conflicting_global_file + next reject_conflicting_file(scratch_asset) end scratch_asset.file.attach(io: request.body, filename:) @@ -85,12 +86,13 @@ 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) + def 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 + def reject_conflicting_file(scratch_asset) + scope = scratch_asset.global? ? 'global' : 'project' + render json: { error: "Asset content conflicts with the existing #{scope} asset" }, status: :conflict :conflict end @@ -100,14 +102,46 @@ def request_body_checksum request.body.rewind end + def load_project_asset_context + authenticate_experience_cs_service_asset_upload if action_name == 'create' + + load_project_from_header + end + + def authenticate_experience_cs_service_asset_upload + return if request.headers[ExperienceCsServiceAuthenticator::HEADER].blank? + + load_experience_cs_service_user + raise CanCan::AccessDenied unless current_user&.experience_cs_service_account? + 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? - @project_from_header = Project.find_by!( - identifier:, - project_type: Project::Types::CODE_EDITOR_SCRATCH - ) + project_scope = Project.where(identifier:) + project_scope = project_scope.where(locale: nil) if current_user&.experience_cs_service_account? + @project_from_header = project_scope.first! + return if project_accepts_asset_upload?(@project_from_header) + + raise ActiveRecord::RecordNotFound, 'Not Found' + end + + def authorize_project_from_header + action = current_user&.experience_cs_service_account? ? :upload_migration_asset : :show + authorize!(action, @project_from_header) + end + + def project_accepts_asset_upload?(project) + return project.experience_cs_migration_target? if current_user&.experience_cs_service_account? + + project.scratch_project? + end + + def asset_uploader_id + return @project_from_header.user_id if current_user.experience_cs_service_account? + + current_user.id end end end diff --git a/app/models/scratch_asset.rb b/app/models/scratch_asset.rb index e1347089a..6f48b6cfb 100644 --- a/app/models/scratch_asset.rb +++ b/app/models/scratch_asset.rb @@ -40,7 +40,7 @@ def response_content_type private def belongs_to_scratch_project - return if project.blank? || project.scratch_project? + return if project.blank? || Project::EXPERIENCE_CS_PROJECT_TYPES.include?(project.project_type) errors.add(:project, 'must be a Scratch project') 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 c5bc96735..1ffa9c815 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 @@ -420,6 +420,77 @@ end end + context 'when the Experience CS service uploads a migration asset' do + let(:request_headers) do + { + 'Content-Type' => 'application/octet-stream', + 'X-Project-ID' => project.identifier, + ExperienceCsServiceAuthenticator::HEADER => 'service-api-key' + } + end + let(:project) do + create( + :project, + school:, + user_id: teacher.id, + locale: nil, + project_type: + ) + end + let(:project_type) { Project::Types::SCRATCH } + + before do + allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') + end + + it 'stores the asset against the existing stub as an upload by its owner' do + expect { make_request }.to change(ScratchAsset, :count).by(1) + + asset = ScratchAsset.find_by!(filename:, project:) + expect(asset.uploaded_user_id).to eq(teacher.id) + expect(asset.file.download).to eq(upload) + expect(response).to have_http_status(:created) + end + + it 'accepts repeated uploads with identical bytes' do + make_request + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:created) + expect(ScratchAsset.find_by!(filename:, project:).file.download).to eq(upload) + end + + it 'rejects repeated uploads with conflicting bytes' do + existing_asset = create_uploaded_scratch_asset( + filename:, + project:, + uploaded_user_id: teacher.id, + body: 'existing bytes' + ) + + 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 project asset' + ) + expect(existing_asset.reload.file.download).to eq('existing bytes') + end + + it 'accepts the upload after the stub has already been converted' do + project.update!( + experience_cs_migrated_at: Time.current, + project_type: Project::Types::CODE_EDITOR_SCRATCH + ) + + make_request + + expect(response).to have_http_status(:created) + expect(ScratchAsset.find_by!(filename:, project:).uploaded_user_id).to eq(teacher.id) + 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 } @@ -427,6 +498,52 @@ end end + describe 'visibility of a migrated project asset' do + let(:student) { create(:student, school:) } + let(:class_teacher) { create(:teacher, school:) } + let(:school_owner) { create(:owner, school:) } + let(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id, class_teacher.id]) } + let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'students') } + let(:lesson_project) { create_scratch_project(school:, lesson:, user_id: teacher.id) } + let(:project) { create_scratch_project(school:, user_id: student.id, remixed_from_id: lesson_project.id) } + + before do + create(:class_student, school_class:, student_id: student.id) + create_uploaded_scratch_asset(filename:, project:, uploaded_user_id: student.id, body: 'private migration bytes') + end + + it 'is available to the student, class teachers, and school owners' do + [student, teacher, class_teacher, school_owner].each do |viewer| + authenticated_in_hydra_as(viewer) + + get( + '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', + headers: { Authorization: UserProfileMock::TOKEN, 'X-Project-ID' => project.identifier } + ) + follow_redirect! while response.redirect? + + expect(response.body).to eq('private migration bytes') + end + end + + it 'is unavailable to unrelated and unauthenticated callers' do + unrelated_teacher = create(:teacher, school: create(:school)) + authenticated_in_hydra_as(unrelated_teacher) + + get( + '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', + headers: { Authorization: UserProfileMock::TOKEN, 'X-Project-ID' => project.identifier } + ) + expect(response).to have_http_status(:forbidden) + + get( + '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', + headers: { 'X-Project-ID' => project.identifier } + ) + expect(response).to have_http_status(:unauthorized) + end + end + describe 'POST #create_global' do let(:upload) { File.binread(file_fixture(filename)) } let(:project) { create_scratch_project(locale: 'en', user_id: nil) } From af71d7e8d552c0006630e4fa96785b3f55a7791a Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Tue, 25 Aug 2026 15:56:42 +0200 Subject: [PATCH 4/5] address comments from copilot --- .../api/scratch/assets_controller.rb | 6 ++++- app/models/project.rb | 3 ++- ...eating_and_showing_a_scratch_asset_spec.rb | 22 +++++++++++++++++++ spec/models/project_spec.rb | 7 ++++++ 4 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index e359cb1de..739f9ec35 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -120,7 +120,11 @@ def load_project_from_header return render json: { error: 'X-Project-ID header is required' }, status: :bad_request if identifier.blank? project_scope = Project.where(identifier:) - project_scope = project_scope.where(locale: nil) if current_user&.experience_cs_service_account? + project_scope = if current_user&.experience_cs_service_account? + project_scope.where(locale: nil, project_type: Project::EXPERIENCE_CS_PROJECT_TYPES) + else + project_scope.where(project_type: Project::Types::CODE_EDITOR_SCRATCH) + end @project_from_header = project_scope.first! return if project_accepts_asset_upload?(@project_from_header) diff --git a/app/models/project.rb b/app/models/project.rb index 152b10209..a78e4b4ab 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -111,7 +111,8 @@ def public_experience_cs_project? def experience_cs_migration_target? return false unless user_id.present? && school_id.present? - project_type == Types::SCRATCH || experience_cs_migrated_at.present? + project_type == Types::SCRATCH || + (project_type == Types::CODE_EDITOR_SCRATCH && experience_cs_migrated_at.present?) end def self_and_ancestors 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 1ffa9c815..66dab0688 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 @@ -229,6 +229,28 @@ authenticated_in_hydra_as(teacher) end + context 'when another project type has the same identifier' do + let(:identifier) { 'shared-project-identifier' } + let(:project) { create_scratch_project(identifier:, user_id: teacher.id) } + + before do + create( + :project, + identifier:, + locale: 'en', + project_type: Project::Types::PYTHON, + user_id: nil + ) + end + + it 'uploads the asset to the Scratch project' do + make_request + + expect(ScratchAsset.find_by!(filename:).project).to eq(project) + expect(response).to have_http_status(:created) + end + end + it 'responds 400 Bad Request when X-Project-ID is not provided' do post '/api/scratch/assets/test_image_1.png', headers: { 'Content-Type' => 'application/octet-stream' }.merge(auth_headers), diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index a7bed37ab..74c6e7c77 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -293,6 +293,13 @@ expect(project).not_to be_experience_cs_migration_target end + it 'rejects a marked non-Scratch project' do + project.project_type = described_class::Types::PYTHON + project.experience_cs_migrated_at = Time.current + + expect(project).not_to be_experience_cs_migration_target + end + it 'rejects public and non-school projects', :aggregate_failures do project.user_id = nil expect(project).not_to be_experience_cs_migration_target From 207fd7c54a7b3583b4a986ae121073a2794adc15 Mon Sep 17 00:00:00 2001 From: abcampo-iry <261805581+abcampo-iry@users.noreply.github.com> Date: Wed, 26 Aug 2026 14:50:58 +0200 Subject: [PATCH 5/5] simplifying after chris comments --- README.md | 9 ++- ...rience_cs_project_migrations_controller.rb | 9 +-- .../api/scratch/assets_controller.rb | 69 ++++++++----------- app/models/ability.rb | 10 +-- app/models/project.rb | 5 +- config/routes.rb | 1 + ...d_experience_cs_migrated_at_to_projects.rb | 7 -- db/schema.rb | 3 +- ...eating_and_showing_a_scratch_asset_spec.rb | 23 +++---- spec/models/ability_spec.rb | 7 -- spec/models/project_spec.rb | 14 +--- .../update_spec.rb | 12 ++-- 12 files changed, 62 insertions(+), 107 deletions(-) delete mode 100644 db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb diff --git a/README.md b/README.md index 376b96bc0..dd3df052d 100644 --- a/README.md +++ b/README.md @@ -167,11 +167,10 @@ Experience CS's `EDITOR_API_SYNC_API_KEY`. The corresponding request header is accepted for public project create/update and global Scratch asset upload. `PUT /api/experience-cs/projects/:identifier/migrate` lets the service replace -an exact locale-less legacy user-project stub with its Markdown instructions -and Scratch content. Successful migrations are marked so retries remain safe -without granting access to native Code Classroom projects. Project-scoped asset -uploads use `X-Project-ID` and remain subject to the project's normal viewing -permissions. +an exact locale-less Experience CS user-project stub with its Markdown +instructions and Scratch content. Project-scoped migration assets use +`POST /api/experience-cs/projects/:identifier/assets/:filename` and remain +subject to the project's normal viewing permissions. ### Code Editor for Education diff --git a/app/controllers/api/experience_cs_project_migrations_controller.rb b/app/controllers/api/experience_cs_project_migrations_controller.rb index f0a6164d4..67f8aba92 100644 --- a/app/controllers/api/experience_cs_project_migrations_controller.rb +++ b/app/controllers/api/experience_cs_project_migrations_controller.rb @@ -5,7 +5,6 @@ class ExperienceCsProjectMigrationsController < ApiController prepend_before_action :load_experience_cs_service_user before_action :authorize_user before_action :load_project - before_action :authorize_migration def update migrate_project! @@ -24,17 +23,13 @@ def load_project @project = Project.find_by!(identifier: params.expect(:id), locale: nil) end - def authorize_migration - authorize! :migrate_from_experience_cs, @project - end - def migrate_project! attributes = migration_params @project.with_lock do + authorize! :migrate_from_experience_cs, @project @project.update!( attributes.slice(:name, :instructions).merge( - project_type: Project::Types::CODE_EDITOR_SCRATCH, - experience_cs_migrated_at: @project.experience_cs_migrated_at || Time.current + project_type: Project::Types::CODE_EDITOR_SCRATCH ) ) scratch_component = @project.scratch_component || @project.build_scratch_component diff --git a/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index 739f9ec35..79ec1589e 100644 --- a/app/controllers/api/scratch/assets_controller.rb +++ b/app/controllers/api/scratch/assets_controller.rb @@ -7,10 +7,12 @@ module Scratch class AssetsController < ApiController include ActiveStorage::SetCurrent - prepend_before_action :load_experience_cs_service_user, only: :create_global - prepend_before_action :load_project_asset_context, only: %i[show create] + prepend_before_action :load_experience_cs_service_user, only: %i[create_global create_migration] before_action :authorize_user, except: %i[show] - before_action :authorize_project_from_header, except: %i[create_global] + prepend_before_action :load_project_from_header, only: %i[show create] + authorize_resource :project_from_header, except: %i[create_global create_migration] + before_action :load_migration_project, only: :create_migration + before_action :authorize_migration_asset, only: :create_migration def show filename_with_extension = "#{params[:id]}.#{params[:format]}" @@ -29,9 +31,17 @@ def create filename_with_extension = "#{params[:id]}.#{params[:format]}" create_asset( project: @project_from_header, - uploaded_user_id: asset_uploader_id, - filename: filename_with_extension, - reject_conflicting_content: current_user.experience_cs_service_account? + uploaded_user_id: current_user.id, + filename: filename_with_extension + ) + end + + def create_migration + create_asset( + project: @migration_project, + uploaded_user_id: @migration_project.user_id, + filename: "#{params[:id]}.#{params[:format]}", + reject_conflicting_content: true ) end @@ -102,50 +112,25 @@ def request_body_checksum request.body.rewind end - def load_project_asset_context - authenticate_experience_cs_service_asset_upload if action_name == 'create' - - load_project_from_header - end - - def authenticate_experience_cs_service_asset_upload - return if request.headers[ExperienceCsServiceAuthenticator::HEADER].blank? - - load_experience_cs_service_user - raise CanCan::AccessDenied unless current_user&.experience_cs_service_account? - 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? - project_scope = Project.where(identifier:) - project_scope = if current_user&.experience_cs_service_account? - project_scope.where(locale: nil, project_type: Project::EXPERIENCE_CS_PROJECT_TYPES) - else - project_scope.where(project_type: Project::Types::CODE_EDITOR_SCRATCH) - end - @project_from_header = project_scope.first! - return if project_accepts_asset_upload?(@project_from_header) - - raise ActiveRecord::RecordNotFound, 'Not Found' - end - - def authorize_project_from_header - action = current_user&.experience_cs_service_account? ? :upload_migration_asset : :show - authorize!(action, @project_from_header) + @project_from_header = Project.find_by!( + identifier:, + project_type: Project::Types::CODE_EDITOR_SCRATCH + ) end - def project_accepts_asset_upload?(project) - return project.experience_cs_migration_target? if current_user&.experience_cs_service_account? - - project.scratch_project? + def load_migration_project + @migration_project = Project.find_by!( + identifier: params.expect(:project_id), + locale: nil + ) end - def asset_uploader_id - return @project_from_header.user_id if current_user.experience_cs_service_account? - - current_user.id + def authorize_migration_asset + authorize! :upload_migration_asset, @migration_project end end end diff --git a/app/models/ability.rb b/app/models/ability.rb index cadd6aa44..c5c096990 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -73,12 +73,12 @@ def define_school_owner_abilities(school:) can(%i[read], :school_member) can(%i[read create import update destroy regenerate_join_code], SchoolClass, school: { id: school.id }) can(%i[read update show_context], Project, school_id: school.id, lesson: { visibility: %w[teachers students] }) - lesson_project_ids = Project.where( + can( + %i[read show_context], + Project, school_id: school.id, - remixed_from_id: nil, - lesson_id: Lesson.where(school_id: school.id, visibility: %w[teachers students]).select(:id) - ).pluck(:id) - can(%i[read show_context], Project, school_id: school.id, remixed_from_id: lesson_project_ids) + parent: { school_id: school.id, lesson: { visibility: %w[teachers students] } } + ) can(%i[read create create_batch destroy], ClassStudent, school_class: { school: { id: school.id } }) can(%i[read create destroy], :school_owner) can(%i[read create destroy], :school_teacher) diff --git a/app/models/project.rb b/app/models/project.rb index a78e4b4ab..73a911856 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -109,10 +109,7 @@ def public_experience_cs_project? end def experience_cs_migration_target? - return false unless user_id.present? && school_id.present? - - project_type == Types::SCRATCH || - (project_type == Types::CODE_EDITOR_SCRATCH && experience_cs_migrated_at.present?) + user_id.present? && school_id.present? && project_type == Types::SCRATCH end def self_and_ancestors diff --git a/config/routes.rb b/config/routes.rb index 698b32c08..264f7765f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -36,6 +36,7 @@ namespace :api do put '/experience-cs/projects/:id/migrate', to: 'experience_cs_project_migrations#update' + post '/experience-cs/projects/:project_id/assets/:id.:format', to: 'scratch/assets#create_migration' namespace :scratch do resources :projects, only: %i[show update create] diff --git a/db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb b/db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb deleted file mode 100644 index 180368ed4..000000000 --- a/db/migrate/20260824120000_add_experience_cs_migrated_at_to_projects.rb +++ /dev/null @@ -1,7 +0,0 @@ -# frozen_string_literal: true - -class AddExperienceCsMigratedAtToProjects < ActiveRecord::Migration[8.1] - def change - add_column :projects, :experience_cs_migrated_at, :datetime - end -end diff --git a/db/schema.rb b/db/schema.rb index 35e0bd2a9..e21d13889 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_08_24_120000) do +ActiveRecord::Schema[8.1].define(version: 2026_08_20_122510) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pgcrypto" @@ -234,7 +234,6 @@ create_table "projects", id: :uuid, default: -> { "gen_random_uuid()" }, force: :cascade do |t| t.datetime "created_at", null: false - t.datetime "experience_cs_migrated_at" t.string "identifier", null: false t.jsonb "instruction_steps" t.text "instructions" 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 66dab0688..9ccb55f3a 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 @@ -217,11 +217,13 @@ describe 'POST #create' do let(:upload) { File.binread(file_fixture(filename)) } + let(:request_path) { '/api/scratch/assets/test_image_1.png' } let(:request_headers) do { 'Content-Type' => 'application/octet-stream', 'X-Project-ID' => project.identifier }.merge(auth_headers) end - let(:make_request) do - post '/api/scratch/assets/test_image_1.png', headers: request_headers, params: upload + + def make_request + post request_path, headers: request_headers, params: upload end context 'when a teacher is logged in' do @@ -446,20 +448,19 @@ let(:request_headers) do { 'Content-Type' => 'application/octet-stream', - 'X-Project-ID' => project.identifier, ExperienceCsServiceAuthenticator::HEADER => 'service-api-key' } end + let(:request_path) { "/api/experience-cs/projects/#{project.identifier}/assets/test_image_1.png" } let(:project) do create( :project, school:, user_id: teacher.id, locale: nil, - project_type: + project_type: Project::Types::SCRATCH ) end - let(:project_type) { Project::Types::SCRATCH } before do allow(Rails.configuration.x.experience_cs).to receive(:service_api_key).and_return('service-api-key') @@ -500,16 +501,12 @@ expect(existing_asset.reload.file.download).to eq('existing bytes') end - it 'accepts the upload after the stub has already been converted' do - project.update!( - experience_cs_migrated_at: Time.current, - project_type: Project::Types::CODE_EDITOR_SCRATCH - ) + it 'rejects uploads after the stub has been converted' do + project.update!(project_type: Project::Types::CODE_EDITOR_SCRATCH) - make_request + expect { make_request }.not_to change(ScratchAsset, :count) - expect(response).to have_http_status(:created) - expect(ScratchAsset.find_by!(filename:, project:).uploaded_user_id).to eq(teacher.id) + expect(response).to have_http_status(:forbidden) end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index dbcc8a2b3..319c7c848 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -62,13 +62,6 @@ expect(ability).not_to be_able_to(:migrate_from_experience_cs, migratable_project) expect(ability).not_to be_able_to(:upload_migration_asset, migratable_project) end - - it 'can replay a previously migrated Experience CS project' do - migratable_project.project_type = Project::Types::CODE_EDITOR_SCRATCH - migratable_project.experience_cs_migrated_at = Time.current - - expect(ability).to be_able_to(:migrate_from_experience_cs, migratable_project) - end end describe 'Project' do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 74c6e7c77..7cc8d3a3b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -276,26 +276,18 @@ ) end - it 'allows a user-owned school legacy Scratch stub' do + it 'allows a user-owned school Scratch stub' do expect(project).to be_experience_cs_migration_target end - it 'allows a replay after a successful migration' do - project.project_type = described_class::Types::CODE_EDITOR_SCRATCH - project.experience_cs_migrated_at = Time.current - - expect(project).to be_experience_cs_migration_target - end - - it 'rejects an unmarked native Code Classroom Scratch project' do + it 'rejects a Code Classroom Scratch project' do project.project_type = described_class::Types::CODE_EDITOR_SCRATCH expect(project).not_to be_experience_cs_migration_target end - it 'rejects a marked non-Scratch project' do + it 'rejects a non-Scratch project' do project.project_type = described_class::Types::PYTHON - project.experience_cs_migrated_at = Time.current expect(project).not_to be_experience_cs_migration_target end diff --git a/spec/requests/experience_cs_project_migrations/update_spec.rb b/spec/requests/experience_cs_project_migrations/update_spec.rb index 027ec8e29..d887c25ff 100644 --- a/spec/requests/experience_cs_project_migrations/update_spec.rb +++ b/spec/requests/experience_cs_project_migrations/update_spec.rb @@ -52,11 +52,15 @@ expect(project.scratch_component.content.to_h).to eq(scratch_data.deep_stringify_keys) end - it 'allows an idempotent replay' do - 2.times { put(path, params:, headers:, as: :json) } + it 'rejects a replay without overwriting Code Classroom changes' do + put(path, params:, headers:, as: :json) + code_classroom_data = scratch_data.merge(meta: { updated_in_code_classroom: true }) + project.reload.scratch_component.update!(content: code_classroom_data) - expect(response).to have_http_status(:ok) - expect(Project.where(id: project.id).count).to eq(1) + put(path, params:, headers:, as: :json) + + expect(response).to have_http_status(:forbidden) + expect(project.reload.scratch_component.content.to_h).to eq(code_classroom_data.deep_stringify_keys) end it 'does not authorize a human Experience CS admin' do