Skip to content
Open
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
1 change: 1 addition & 0 deletions app/controllers/api/scratch/assets_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module Scratch
class AssetsController < ScratchController
include ActiveStorage::SetCurrent

before_action :authorize_user, except: %i[show]
prepend_before_action :load_project_from_header, only: %i[show create]
authorize_resource :project_from_header
Comment on lines +8 to 10

Expand Down
1 change: 1 addition & 0 deletions app/controllers/api/scratch/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ module Scratch
class ProjectsController < ScratchController
include RemixSelection

before_action :authorize_user, except: %i[show]
Comment thread
cursor[bot] marked this conversation as resolved.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Remix checks create not show

Medium Severity

After dropping the school-only gate, non-school users can hit scratch remix create, but authorize_resource :original_project authorizes :create on the original. Anonymous starters allow :show but not :create, so remixing a public template can return forbidden despite the new spec expecting success.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 6843e7b. Configure here.

before_action :load_project, except: %i[create]
authorize_resource :project, except: %i[create]
Comment on lines +8 to 10

Expand Down
8 changes: 0 additions & 8 deletions app/controllers/api/scratch/scratch_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,6 @@
module Api
module Scratch
class ScratchController < ApiController
before_action :authorize_user
before_action :only_allow_schools_to_use_scratch

def only_allow_schools_to_use_scratch
return true if current_user.schools.any?

raise ActiveRecord::RecordNotFound, 'Not Found'
end
end
end
end
6 changes: 2 additions & 4 deletions app/models/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ class Ability
include CanCan::Ability

def initialize(user)
define_common_non_student_abilities(user)
define_common_abilities

return unless user

Expand All @@ -21,9 +21,7 @@ def initialize(user)

private

def define_common_non_student_abilities(user)
return if user&.student?

def define_common_abilities
# Anyone can view projects not owned by a user or a school.
can :show, Project, user_id: nil, school_id: nil
can :show, Component, project: { user_id: nil, school_id: nil }
Expand Down
5 changes: 5 additions & 0 deletions spec/factories/project.rb
Original file line number Diff line number Diff line change
Expand Up @@ -67,5 +67,10 @@
instructions { Faker::Lorem.paragraph }
school factory: %i[school]
end

factory :scratch_project do
project_type { Project::Types::CODE_EDITOR_SCRATCH }
scratch_component
end
end
end
32 changes: 27 additions & 5 deletions spec/features/scratch/creating_a_scratch_project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -54,13 +54,35 @@ def make_request(query: request_query, request_headers: headers, request_params:
expect(response).to have_http_status(:unauthorized)
end

it 'responds 404 Not Found when user is not part of a school' do
user = create(:user)
authenticated_in_hydra_as(user)
context 'when authenticated but not part of a school' do
let(:user) { create(:user) }

make_request
before do
authenticated_in_hydra_as(user)
end

it 'responds 403 forbidden when the user cannot access the original project' do
make_request

expect(response).to have_http_status(:not_found)
expect(response).to have_http_status(:forbidden)
end

context 'when the original project is anonymous scratch project' do
let(:original_project) do
create(
:project,
user_id: nil,
project_type: Project::Types::CODE_EDITOR_SCRATCH,
locale: 'en'
)
end

it 'responds 200 ok' do
make_request

expect(response).to have_http_status(:ok)
end
end
end

context 'when authenticated and part of a school' do
Expand Down
33 changes: 23 additions & 10 deletions spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,29 @@
expect(response).to have_http_status(:bad_request)
end

context 'when the user can view the project' do
context 'when the user is not logged in' do
let(:project_headers) { { 'X-Project-ID' => project.identifier } }

it 'responds with unauthorized' do
create_uploaded_scratch_asset(filename: 'test_image_1.png', project:, body: 'global-body')
get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: project_headers

expect(response).to have_http_status(:unauthorized)
end

context 'when the project is anonymous' do
let(:project) { create_scratch_project(locale: 'en', user_id: nil) }

it 'redirects (302 Found)' do
create_uploaded_scratch_asset(filename: 'test_image_1.png', project: nil, body: 'global-body')
get '/api/scratch/assets/internalapi/asset/test_image_1.png/get/', headers: project_headers

expect(response).to have_http_status(:found)
end
end
end

context 'when the project owner is logged in' do
before do
authenticated_in_hydra_as(teacher)
end
Expand Down Expand Up @@ -385,15 +407,6 @@

expect(response).to have_http_status(:unauthorized)
end

it 'responds 404 Not Found when user is not part of a school' do
user = create(:user)
authenticated_in_hydra_as(user)

post '/api/scratch/assets/example.svg', headers: project_headers

expect(response).to have_http_status(:not_found)
end
end

def create_scratch_project(**attributes)
Expand Down
15 changes: 12 additions & 3 deletions spec/features/scratch/showing_a_scratch_project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
RSpec.describe 'Showing a Scratch project', type: :request do
let(:school) { create(:school) }
let(:teacher) { create(:teacher, school:) }
let(:student) { create(:student, school:) }
let(:headers) { { 'Authorization' => UserProfileMock::TOKEN } }
let(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) }
let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id) }
Expand Down Expand Up @@ -73,11 +74,19 @@
expect(response).to have_http_status(:not_found)
end

it 'returns a 401 unauthorized if not logged in' do
project = create(:project, project_type: Project::Types::PYTHON, locale: 'en')
it 'returns a 200 ok if not logged in for an anonymous scratch project' do
project = create(:scratch_project, locale: 'en', user_id: nil)
get "/api/scratch/projects/#{project.identifier}"

expect(response).to have_http_status(:unauthorized)
expect(response).to have_http_status(:ok)
end

it 'returns a 200 ok if logged in for an anonymous scratch project' do
authenticated_in_hydra_as(student)
project = create(:scratch_project, locale: 'en', user_id: nil)
get "/api/scratch/projects/#{project.identifier}", headers: headers

expect(response).to have_http_status(:ok)
end
Comment on lines +84 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.

If we add the headers: in here, the test fails, i.e. students are blocked.


it 'returns a 403 forbidden if user does not have access to the project' do
Expand Down
2 changes: 1 addition & 1 deletion spec/models/ability_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -492,7 +492,7 @@

context 'with a starter project' do
it { is_expected.not_to be_able_to(:index, starter_project) }
it { is_expected.not_to be_able_to(:show, starter_project) }
it { is_expected.to be_able_to(:show, starter_project) }
it { is_expected.not_to be_able_to(:create, starter_project) }
it { is_expected.not_to be_able_to(:update, starter_project) }
it { is_expected.not_to be_able_to(:destroy, starter_project) }
Expand Down
4 changes: 3 additions & 1 deletion spec/requests/projects/index_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,9 @@
context 'when user is logged in' do
before do
# create non user projects
create_list(:project, 2)
create(:project)
create(:project, user_id: nil)

authenticated_in_hydra_as(owner)
end

Expand Down
Loading