Skip to content

update before actions in scratch controller#887

Open
rammodhvadia wants to merge 9 commits into
mainfrom
update-scratch-controller-before-actions
Open

update before actions in scratch controller#887
rammodhvadia wants to merge 9 commits into
mainfrom
update-scratch-controller-before-actions

Conversation

@rammodhvadia

@rammodhvadia rammodhvadia commented Jun 23, 2026

Copy link
Copy Markdown
Contributor
  • removes 'only_allow_schools_to_use_scratch` before action check
  • Allows show routes in assets/projects scratch controllers to run without authorising user
  • Update ability model to allow all users to view non-school projects (previously would not allow a student account to view these)

@cla-bot cla-bot Bot added the cla-signed label Jun 23, 2026
@github-actions

github-actions Bot commented Jun 23, 2026

Copy link
Copy Markdown

Test coverage

91.85% line coverage reported by SimpleCov.
Run: https://github.com/RaspberryPiFoundation/editor-api/actions/runs/28179503499

@rammodhvadia rammodhvadia marked this pull request as ready for review June 23, 2026 12:39
Copilot AI review requested due to automatic review settings June 23, 2026 12:39
Comment thread app/controllers/api/scratch/projects_controller.rb
class ProjectsController < ScratchController
include RemixSelection

before_action :authorize_user, except: %i[show]

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.

Copilot AI left a comment

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.

Pull request overview

This PR relaxes authentication requirements for the Scratch API so that show endpoints for Scratch projects/assets can be accessed without requiring a logged-in user, while keeping other Scratch actions authenticated/authorized.

Changes:

  • Removes the “only schools can use Scratch” gate from Api::Scratch::ScratchController.
  • Makes ProjectsController#show and AssetsController#show skip authorize_user (authentication), while retaining CanCan authorization.
  • Updates request specs and adds a :scratch_project factory to support anonymous Scratch-project scenarios.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
spec/features/scratch/showing_a_scratch_project_spec.rb Adds coverage for anonymous Scratch project show, but currently has an “authenticated” test that doesn’t actually send auth headers.
spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb Adds coverage for asset show when unauthenticated/anonymous; includes minor wording typos.
spec/features/scratch/creating_a_scratch_project_spec.rb Adjusts expected status codes for non-school users and adds an anonymous-original remix case.
spec/factories/project.rb Adds :scratch_project factory for Scratch projects with a scratch component.
app/controllers/api/scratch/scratch_controller.rb Removes authorize_user and only_allow_schools_to_use_scratch from the base Scratch controller.
app/controllers/api/scratch/projects_controller.rb Skips authentication for show; all other actions still require authentication.
app/controllers/api/scratch/assets_controller.rb Skips authentication for show; all other actions still require authentication.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb Outdated
Comment thread spec/features/scratch/creating_and_showing_a_scratch_asset_spec.rb Outdated
Comment thread app/controllers/api/scratch/projects_controller.rb
Comment thread app/controllers/api/scratch/projects_controller.rb
Comment thread app/controllers/api/scratch/assets_controller.rb
Comment thread spec/features/scratch/showing_a_scratch_project_spec.rb
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@rammodhvadia rammodhvadia temporarily deployed to editor-api-p-update-scr-jghg9l June 23, 2026 12:48 Inactive
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
@rammodhvadia rammodhvadia temporarily deployed to editor-api-p-update-scr-jghg9l June 23, 2026 12:48 Inactive
@rammodhvadia rammodhvadia temporarily deployed to editor-api-p-update-scr-nzgydn June 23, 2026 14:38 Inactive
@rammodhvadia rammodhvadia temporarily deployed to editor-api-p-update-scr-gjps56 June 23, 2026 15:42 Inactive
@rammodhvadia rammodhvadia temporarily deployed to editor-api-p-update-scr-wb6j5a June 23, 2026 15:51 Inactive
@mwtrew mwtrew self-requested a review June 25, 2026 12:15

@mwtrew mwtrew left a comment

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.

Some minor changes suggested, but looks good overall!

Comment thread app/models/ability.rb
private

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

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.

I don't think this should need to be changed. Authenticated "student" users are not supposed to be able to use the Editor - the student account concept only applies to Classroom.

This check already permits anonymous users and users logged in to a regular RPF account.

class AssetsController < ScratchController
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.

class ProjectsController < ScratchController
include RemixSelection

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.

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

@rammodhvadia rammodhvadia temporarily deployed to editor-api-p-update-scr-a3uvrd June 25, 2026 15:00 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants