diff --git a/README.md b/README.md index cd36853c7..dd3df052d 100644 --- a/README.md +++ b/README.md @@ -164,8 +164,13 @@ 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 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 new file mode 100644 index 000000000..67f8aba92 --- /dev/null +++ b/app/controllers/api/experience_cs_project_migrations_controller.rb @@ -0,0 +1,48 @@ +# 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 + + 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 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 + ) + ) + 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/app/controllers/api/scratch/assets_controller.rb b/app/controllers/api/scratch/assets_controller.rb index ac8a7fc65..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: %i[create_global] + prepend_before_action :load_experience_cs_service_user, only: %i[create_global create_migration] 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] + 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]}" @@ -34,6 +36,15 @@ def create ) 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 + def create_global authorize! :create_global, ScratchAsset @@ -60,7 +71,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 +79,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 +96,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 @@ -109,6 +121,17 @@ def load_project_from_header project_type: Project::Types::CODE_EDITOR_SCRATCH ) end + + def load_migration_project + @migration_project = Project.find_by!( + identifier: params.expect(:project_id), + locale: nil + ) + end + + def authorize_migration_asset + authorize! :upload_migration_asset, @migration_project + end end end end diff --git a/app/models/ability.rb b/app/models/ability.rb index cd10e3cac..c5c096990 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] }) + can( + %i[read show_context], + Project, + school_id: school.id, + 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) @@ -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..73a911856 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -108,6 +108,10 @@ def public_experience_cs_project? user_id.nil? && school_id.nil? && EXPERIENCE_CS_PROJECT_TYPES.include?(project_type) end + def experience_cs_migration_target? + user_id.present? && school_id.present? && project_type == Types::SCRATCH + end + def self_and_ancestors projects = [] current_project = self 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/config/routes.rb b/config/routes.rb index f71d8c5d9..264f7765f 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -35,6 +35,9 @@ 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' + post '/experience-cs/projects/:project_id/assets/:id.:format', to: 'scratch/assets#create_migration' + namespace :scratch do resources :projects, only: %i[show update create] get '/assets/internalapi/asset/:id.:format/get/' => 'assets#show' 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..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 @@ -229,6 +231,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), @@ -420,6 +444,72 @@ end end + context 'when the Experience CS service uploads a migration asset' do + let(:request_headers) do + { + 'Content-Type' => 'application/octet-stream', + 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::Types::SCRATCH + ) + end + + 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 'rejects uploads after the stub has been converted' do + project.update!(project_type: Project::Types::CODE_EDITOR_SCRATCH) + + expect { make_request }.not_to change(ScratchAsset, :count) + + expect(response).to have_http_status(:forbidden) + 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 +517,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) } diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 5f34facff..319c7c848 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -32,6 +32,38 @@ 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 + 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..7cc8d3a3b 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -265,6 +265,43 @@ 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 Scratch stub' do + expect(project).to be_experience_cs_migration_target + end + + 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 non-Scratch project' do + project.project_type = described_class::Types::PYTHON + + 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) } 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..d887c25ff --- /dev/null +++ b/spec/requests/experience_cs_project_migrations/update_spec.rb @@ -0,0 +1,97 @@ +# 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 '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) + + 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 + 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