Skip to content

Fix missing authorization check before REST project remix creation #864

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
abcampo-iry merged 1 commit into from
Jun 11, 2026
+45 −0
Merged

Fix missing authorization check before REST project remix creation #864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 2 additions & 0 deletions app/controllers/api/projects/remixes_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,8 @@ def show_identifier
end

def create
authorize! :show, project

# Ensure we have a fallback value to prevent bad requests
remix_origin = request.origin || request.referer
result = Project::CreateRemix.call(params: remix_params,
Expand Down
43 changes: 43 additions & 0 deletions spec/requests/projects/remix_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,49 @@
expect(response).to have_http_status(:not_found)
end

context 'when the original project belongs to another user' do
let!(:original_project) { create(:project, user_id: create(:user).id) }

it 'returns forbidden without creating a remix' do
allow(Project::CreateRemix).to receive(:call).and_call_original

expect do
post("/api/projects/#{original_project.identifier}/remix", params: { project: project_params }, headers:)
end.not_to change(Project, :count)

expect(response).to have_http_status(:forbidden)
expect(Project::CreateRemix).not_to have_received(:call)
end
end

context 'when a student cannot view the teacher-only original project' do
let(:student) { create(:student, school:) }
let(:teacher) { create(:teacher, school:) }
let(:school_class) { create(:school_class, school:, teacher_ids: [teacher.id]) }
let(:lesson) { create(:lesson, school:, school_class:, user_id: teacher.id, visibility: 'teachers') }
let!(:original_project) do
lesson.project.tap do |project|
project.update!(school:, user_id: teacher.id, instructions: 'Teacher-only instructions')
end
end

before do
create(:class_student, school_class:, student_id: student.id)
authenticated_in_hydra_as(student)
end

it 'returns forbidden without creating a remix' do
allow(Project::CreateRemix).to receive(:call).and_call_original

expect do
post("/api/projects/#{original_project.identifier}/remix", params: { project: project_params }, headers:)
end.not_to change(Project, :count)

expect(response).to have_http_status(:forbidden)
expect(Project::CreateRemix).not_to have_received(:call)
end
end
Comment on lines +171 to +212

@zetter-rpf zetter-rpf Jun 11, 2026

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've been thinking about the testing around this (not just for the PR but for request/feature specs).

A lot of the detail in this test is actually testing the behaviour of the ability rather than the controller and it requires a lot of setup to do that. I agree that there should be at least one test at this level that is checking a 'forbidden' state but maybe any more cases should be pushed down to the ability.

What I think would be more valuable, is configuring our controllers so we we can't forget to check an ability for an action.

@abcampo-iry abcampo-iry Jun 11, 2026 •
edited
Loading

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 saw in the ticket.
Agreed.
I can remove the test from there and then trying to go down the road that you described here:

As part of this we might want to consider adding check_authorization to our application controller. This is a cancancan feature that prevents us from 'forgetting' to add in authorization to a controller and would require us to explicitly decide if a controller shouldn't authorize resources (you can do a similar thing with authorize user, although that's not the problem here.)
https://github.com/CanCanCommunity/cancancan/blob/develop/docs/changing_defaults.md#check_authorization

I can create a ticket and add a different PR

@zetter-rpf zetter-rpf Jun 11, 2026

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 can create a ticket and add a different PR

Thanks. Might be one for next sprint.


context 'when project cannot be saved' do
before do
authenticated_in_hydra_as(owner)
Expand Down
Loading