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
3 changes: 2 additions & 1 deletion app/controllers/api/scratch/assets_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

module Api
module Scratch
class AssetsController < ScratchController
class AssetsController < ApiController
include ActiveStorage::SetCurrent

before_action :authorize_user, except: %i[show]

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.

Could this go up to ScratchController since it's used in both controllers? Alternatively we should probably remove ScratchController as it's now empty.

prepend_before_action :load_project_from_header, only: %i[show create]
authorize_resource :project_from_header
Comment thread
rammodhvadia marked this conversation as resolved.

Expand Down
3 changes: 2 additions & 1 deletion app/controllers/api/scratch/projects_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@

module Api
module Scratch
class ProjectsController < ScratchController
class ProjectsController < ApiController
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.

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.

I don't think this is valid. The original_project allows a non school user the :show action on an anonymous project, which is all that's needed to remix

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.

It works in your test, which is good enough for me.

Comment thread
rammodhvadia marked this conversation as resolved.
before_action :load_project, except: %i[create]
authorize_resource :project, except: %i[create]
Comment thread
rammodhvadia marked this conversation as resolved.

Expand Down
16 changes: 0 additions & 16 deletions app/controllers/api/scratch/scratch_controller.rb

This file was deleted.

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
14 changes: 11 additions & 3 deletions spec/features/scratch/showing_a_scratch_project_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,11 +73,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(teacher)
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 thread
rammodhvadia marked this conversation as resolved.

it 'returns a 403 forbidden if user does not have access to the project' do
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