Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .env.example
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -74,4 +77,4 @@ PARDOT_SUBSCRIPTION_URL=
CLOUDFLARE_TURNSTILE_SECRET_KEY=1x0000000000000000000000000000000AA

HOSTNAME=localhost
LEARNER_EXPERIENCE_TESTS_DISPATCH_TOKEN=changeme
LEARNER_EXPERIENCE_TESTS_DISPATCH_TOKEN=changeme
7 changes: 6 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.

23 changes: 23 additions & 0 deletions app/controllers/api/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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]

Expand Down Expand Up @@ -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])
Expand Down
71 changes: 61 additions & 10 deletions app/controllers/api/scratch/assets_controller.rb
Original file line number Diff line number Diff line change
@@ -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]}"
Expand All @@ -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
Comment on lines +88 to +90

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the reasoning behind needing to hash the assets?

The asset name should already be the hash of the asset so I'm wondering if it's simpler to use that and skip saving it if the asset already exists.

Since global assets can only be created by trusted systems I think the risk is low (but I might be missing something?)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me the key is to compare checksums to avoid silently using the wrong asset if different content is uploaded with the same filename. It's quite inexpensive that check but you are right risk is low

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's reliable that ok - a reason to remove it would be if we didn't think it could hash files consistently because some part of them changed even though Scratch think it's the same file.


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']
Expand Down
6 changes: 6 additions & 0 deletions app/controllers/concerns/identifiable.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
6 changes: 6 additions & 0 deletions app/models/project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
6 changes: 6 additions & 0 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
13 changes: 13 additions & 0 deletions app/services/experience_cs_service_authenticator.rb
Original file line number Diff line number Diff line change
@@ -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)
Comment thread
abcampo-iry marked this conversation as resolved.

User.new(id: User::EXPERIENCE_CS_SERVICE_ACCOUNT_ID, roles: 'experience-cs-admin')
end
end
1 change: 1 addition & 0 deletions config/application.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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|
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
46 changes: 46 additions & 0 deletions spec/features/project/creating_a_project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Loading
Loading