From 982dd1af459b1feb358a30eb1eca10f07d1136ca Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 21 May 2025 17:22:04 +0100 Subject: [PATCH 01/10] Extract User#parsed_roles method I'm planning to introduce another role-related predicate method in a subsequent commit. Extracting this method first will make that change easier. I'm pretty convinced the call to `#to_s` was made redundant when the safe navigation operator was introduced in this commit [1]. However, I suppose it's theoretically possible for the parsed JSON returned from `HydraPublicApiClient.fetch_oauth_user` via `User.from_token` to contain non-String values, so I'm going to leave it in place for now. [1]: https://github.com/RaspberryPiFoundation/editor-api/commit/9a1fdb1b725b9379b940c9cc2f8628ea8fb6e0b4 --- app/models/user.rb | 6 +++++- spec/models/user_spec.rb | 32 ++++++++++++++++++++++---------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index a7d90a6be..3b101845d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -51,7 +51,11 @@ def student? end def admin? - (roles&.to_s&.split(',')&.map(&:strip) || []).include?('editor-admin') + parsed_roles.include?('editor-admin') + end + + def parsed_roles + roles&.to_s&.split(',')&.map(&:strip) || [] end def ==(other) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 103e90ada..c17c0e4b5 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -277,24 +277,36 @@ end end - describe '#admin?' do - it 'returns true if the user has the editor-admin role in Hydra' do - user = build(:user, roles: 'editor-admin') - expect(user).to be_admin + describe '#parsed_roles' do + it 'returns array of role names when roles is set to comma-separated string' do + user = build(:user, roles: 'role-1,role-2') + expect(user.parsed_roles).to eq(%w[role-1 role-2]) end - it 'returns false if the user does not have the editor-admin role in Hydra' do - user = build(:user, roles: 'another-editor-admin') - expect(user).not_to be_admin + it 'strips leading & trailing spaces from role names' do + user = build(:user, roles: ' role-1 , role-2 ') + expect(user.parsed_roles).to eq(%w[role-1 role-2]) end - it 'returns false if roles are empty in Hydra' do + it 'returns empty array when roles is set to empty string' do user = build(:user, roles: '') - expect(user).not_to be_admin + expect(user.parsed_roles).to eq([]) end - it 'returns false if roles are nil in Hydra' do + it 'returns empty array when roles is set to nil' do user = build(:user, roles: nil) + expect(user.parsed_roles).to eq([]) + end + end + + describe '#admin?' do + it 'returns true if the user has the editor-admin role in Hydra' do + user = build(:user, roles: 'editor-admin') + expect(user).to be_admin + end + + it 'returns false if the user does not have the editor-admin role in Hydra' do + user = build(:user, roles: 'another-editor-admin') expect(user).not_to be_admin end end From 13741b2d74bbc7b9e52754428247af8bf25524ec Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 21 May 2025 17:44:38 +0100 Subject: [PATCH 02/10] Introduce User#experience_cs_admin? method I'm planning to make use of this to grant access to `Api::PublicProjectsController#create` so that admin users on the `experience-cs` website can create public Scratch projects via the `editor-api` API. --- app/models/user.rb | 4 ++++ spec/factories/user.rb | 4 ++++ spec/models/user_spec.rb | 12 ++++++++++++ 3 files changed, 20 insertions(+) diff --git a/app/models/user.rb b/app/models/user.rb index 3b101845d..386ab2615 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -54,6 +54,10 @@ def admin? parsed_roles.include?('editor-admin') end + def experience_cs_admin? + parsed_roles.include?('experience-cs-admin') + end + def parsed_roles roles&.to_s&.split(',')&.map(&:strip) || [] end diff --git a/spec/factories/user.rb b/spec/factories/user.rb index 473db5ec7..d7470f672 100644 --- a/spec/factories/user.rb +++ b/spec/factories/user.rb @@ -11,6 +11,10 @@ roles { 'editor-admin' } end + factory :experience_cs_admin_user do + roles { 'experience-cs-admin' } + end + factory :student do email { nil } username { Faker::Internet.username } diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index c17c0e4b5..ce42c44b3 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -311,6 +311,18 @@ end end + describe '#experience_cs_admin?' do + it 'returns true if the user has the experience-cs-admin role in Hydra' do + user = build(:experience_cs_admin_user) + expect(user).to be_experience_cs_admin + end + + it 'returns false if the user does not have the experience-cs-admin role in Hydra' do + user = build(:user, roles: 'another-admin') + expect(user).not_to be_experience_cs_admin + end + end + describe '#school_roles' do subject(:user) { build(:user) } From f2da740c6a26e721dfb163a999c86f36f05b1a7e Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:46:18 +0100 Subject: [PATCH 03/10] Move before below lets in create project feature spec This is more idiomatic and easier to follow. --- spec/features/project/creating_a_project_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index bbcd47836..0e622c33f 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -3,11 +3,6 @@ require 'rails_helper' RSpec.describe 'Creating a project', type: :request do - before do - authenticated_in_hydra_as(teacher) - mock_phrase_generation - end - let(:headers) { { Authorization: UserProfileMock::TOKEN } } let(:teacher) { create(:teacher, school:) } let(:school) { create(:school) } @@ -24,6 +19,11 @@ } end + before do + authenticated_in_hydra_as(teacher) + mock_phrase_generation + end + it 'responds 201 Created' do post('/api/projects', headers:, params:) expect(response).to have_http_status(:created) From facfdb8736c28df50ca7a521753601dd8e7f174e Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:49:30 +0100 Subject: [PATCH 04/10] Ensure identifier generated when project created I'm about to make a change to how this works and I want to be sure the change won't break anything. --- spec/features/project/creating_a_project_spec.rb | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index 0e622c33f..dbd876540 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -3,11 +3,11 @@ require 'rails_helper' RSpec.describe 'Creating a project', type: :request do + let(:generated_identifier) { 'word1-word2-word3' } let(:headers) { { Authorization: UserProfileMock::TOKEN } } let(:teacher) { create(:teacher, school:) } let(:school) { create(:school) } let(:owner) { create(:owner, school:) } - let(:params) do { project: { @@ -21,7 +21,7 @@ before do authenticated_in_hydra_as(teacher) - mock_phrase_generation + mock_phrase_generation(generated_identifier) end it 'responds 201 Created' do @@ -29,6 +29,13 @@ expect(response).to have_http_status(:created) end + it 'generates an identifier for the project' do + post('/api/projects', headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:identifier]).to eq(generated_identifier) + end + it 'responds with the project JSON' do post('/api/projects', headers:, params:) data = JSON.parse(response.body, symbolize_names: true) From 6319dd3da080fe05dbc6505f1f974987b0daa33e Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 12:00:01 +0100 Subject: [PATCH 05/10] Allow ExCS admin to create starter projects This gives users with the "experience-cs-admin" role permission to create starter (or "public") projects (i.e. projects with no `user_id` set). I've added examples to the "Creating a project" feature spec to check this works as intended. Note that I've had to add support for `User#roles` to `UserProfileMock#user_to_hash` for the new examples that I've added to the feature spec. --- app/models/ability.rb | 8 ++++ .../project/creating_a_project_spec.rb | 44 +++++++++++++++++++ spec/models/ability_spec.rb | 6 +++ spec/support/user_profile_mock.rb | 3 +- 4 files changed, 60 insertions(+), 1 deletion(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index bca1f2dfa..b2ff42eb9 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -14,6 +14,8 @@ def initialize(user) define_school_teacher_abilities(user:, school:) if user.school_teacher?(school) define_school_owner_abilities(school:) if user.school_owner?(school) end + + define_experience_cs_admin_abilities(user) end private @@ -100,6 +102,12 @@ def define_school_student_abilities(user:, school:) can(%i[show_finished set_finished], SchoolProject, project: { user_id: user.id, lesson_id: nil }, school_id: school.id) end + def define_experience_cs_admin_abilities(user) + return unless user&.experience_cs_admin? + + can :create, Project + end + def school_teacher_can_manage_lesson?(user:, school:, lesson:) is_my_lesson = lesson.school_id == school.id && lesson.user_id == user.id is_my_class = lesson.school_class&.teacher_ids&.include?(user.id) diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index dbd876540..87a021154 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -207,4 +207,48 @@ expect(response).to have_http_status(:forbidden) end end + + context 'when the user is an Experience CS admin' do + let(:experience_cs_admin) { create(:experience_cs_admin_user) } + let(:params) do + { + project: { + name: 'Test Project', + locale: 'fr', + project_type: Project::Types::SCRATCH, + components: [] + } + } + end + + before do + authenticated_in_hydra_as(experience_cs_admin) + end + + it 'responds 201 Created' do + post('/api/projects', headers:, params:) + expect(response).to have_http_status(:created) + end + + it 'sets the project name to the specified value' do + post('/api/projects', headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:name]).to eq('Test Project') + end + + it 'sets the project locale to the specified value' do + post('/api/projects', headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:locale]).to eq('fr') + end + + it 'sets the project type to the specified value' do + post('/api/projects', headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:project_type]).to eq(Project::Types::SCRATCH) + end + end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 39624d64e..ee54e1570 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -139,6 +139,12 @@ end end + context 'with an experience-cs admin' do + let(:user) { build(:experience_cs_admin_user) } + + it { is_expected.to be_able_to(:create, starter_project) } + end + # rubocop:disable RSpec/MultipleMemoizedHelpers context "with a teacher's project where the lesson is visible to students" do let(:user) { create(:user) } diff --git a/spec/support/user_profile_mock.rb b/spec/support/user_profile_mock.rb index 4f56e353a..c694d158c 100644 --- a/spec/support/user_profile_mock.rb +++ b/spec/support/user_profile_mock.rb @@ -31,7 +31,8 @@ def user_to_hash(user, user_type, id_field = :id) id_field => user_type ? "#{user_type}:#{user.id}" : user.id, name: user.name, email: user.email, - username: user.username + username: user.username, + roles: user.roles } end From 3b55e10c5f06ef994a94cddabafc2178fbf8af16 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 12:02:09 +0100 Subject: [PATCH 06/10] Allow ExCS admin to set starter project identifier To achieve this, I've had to remove the explicit *overriding* of `Project#identifier` in `Project::Create.build_project` when the user is an Experience CS admin. Note that the `Project#check_unique_not_null` check which is triggered on a `before_validation` callback will generate an identifier if none is set in `Project::Create.build_project` --- app/controllers/api/projects_controller.rb | 2 +- app/graphql/mutations/create_project.rb | 2 +- lib/concepts/project/operations/create.rb | 10 +++++----- spec/concepts/project/create_spec.rb | 7 ++++--- spec/features/project/creating_a_project_spec.rb | 13 +++++++++++-- 5 files changed, 22 insertions(+), 12 deletions(-) diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index f44710faf..b7112e4d7 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -27,7 +27,7 @@ def show end def create - result = Project::Create.call(project_hash: project_params) + result = Project::Create.call(project_hash: project_params, current_user:) if result.success? @project = result[:project] diff --git a/app/graphql/mutations/create_project.rb b/app/graphql/mutations/create_project.rb index c7135168a..07c98c3f7 100644 --- a/app/graphql/mutations/create_project.rb +++ b/app/graphql/mutations/create_project.rb @@ -13,7 +13,7 @@ def resolve(**input) components: input[:components]&.map(&:to_h) ) - response = Project::Create.call(project_hash:) + response = Project::Create.call(project_hash:, current_user: context[:current_user]) raise GraphQL::ExecutionError, response[:error] unless response.success? { project: response[:project] } diff --git a/lib/concepts/project/operations/create.rb b/lib/concepts/project/operations/create.rb index 625cdba85..a2e942889 100644 --- a/lib/concepts/project/operations/create.rb +++ b/lib/concepts/project/operations/create.rb @@ -3,9 +3,9 @@ class Project class Create class << self - def call(project_hash:) + def call(project_hash:, current_user:) response = OperationResponse.new - response[:project] = build_project(project_hash) + response[:project] = build_project(project_hash, current_user) response[:project].save! response rescue StandardError => e @@ -16,9 +16,9 @@ def call(project_hash:) private - def build_project(project_hash) - identifier = PhraseIdentifier.generate - new_project = Project.new(project_hash.except(:components).merge(identifier:)) + def build_project(project_hash, current_user) + project_hash[:identifier] = PhraseIdentifier.generate unless current_user&.experience_cs_admin? + new_project = Project.new(project_hash.except(:components)) new_project.components.build(project_hash[:components]) new_project end diff --git a/spec/concepts/project/create_spec.rb b/spec/concepts/project/create_spec.rb index 2af9426f1..519b7d788 100644 --- a/spec/concepts/project/create_spec.rb +++ b/spec/concepts/project/create_spec.rb @@ -3,9 +3,10 @@ require 'rails_helper' RSpec.describe Project::Create, type: :unit do - subject(:create_project) { described_class.call(project_hash:) } + subject(:create_project) { described_class.call(project_hash:, current_user:) } - let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' } + let(:current_user) { create(:user) } + let(:user_id) { current_user.id } before do mock_phrase_generation @@ -16,7 +17,7 @@ let(:project_hash) { ActionController::Parameters.new({}).merge(user_id:) } context 'with valid content' do - subject(:create_project_with_content) { described_class.call(project_hash:) } + subject(:create_project_with_content) { described_class.call(project_hash:, current_user:) } let(:project_hash) do { diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index 87a021154..09f07f704 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -29,8 +29,9 @@ expect(response).to have_http_status(:created) end - it 'generates an identifier for the project' do - post('/api/projects', headers:, params:) + it 'generates an identifier for the project even if another identifier is specified' do + params_with_identifier = { project: { identifier: 'test-identifier', components: [] } } + post('/api/projects', headers:, params: params_with_identifier) data = JSON.parse(response.body, symbolize_names: true) expect(data[:identifier]).to eq(generated_identifier) @@ -213,6 +214,7 @@ let(:params) do { project: { + identifier: 'test-project', name: 'Test Project', locale: 'fr', project_type: Project::Types::SCRATCH, @@ -230,6 +232,13 @@ expect(response).to have_http_status(:created) end + it 'sets the project identifier to the specified (not the generated) value' do + post('/api/projects', headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:identifier]).to eq('test-project') + end + it 'sets the project name to the specified value' do post('/api/projects', headers:, params:) data = JSON.parse(response.body, symbolize_names: true) From efcb054f3bd76a2bd42a308ce21c7da3aac62c52 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 12:14:27 +0100 Subject: [PATCH 07/10] Allow ExCS admin to set starter project user_id Specifically to set `Project#user_id` to `nil` to signify a starter (or "public") project which can be viewed by anonymous users. The logic in `Api::ProjectsController#project_params` is a bit hard to follow. However, essentially we need to avoid the `base_params.merge(user_id: current_user&.id)` line in the `else` branch to avoid `Project#user_id` being set to the current user's ID. --- app/controllers/api/projects_controller.rb | 4 ++-- spec/features/project/creating_a_project_spec.rb | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/app/controllers/api/projects_controller.rb b/app/controllers/api/projects_controller.rb index b7112e4d7..2ac99ca99 100644 --- a/app/controllers/api/projects_controller.rb +++ b/app/controllers/api/projects_controller.rb @@ -80,8 +80,8 @@ def load_projects end def project_params - if school_owner? - # A school owner must specify who the project user is. + if school_owner? || current_user&.experience_cs_admin? + # A school owner or an Experience CS admin must specify who the project user is. base_params else # A school teacher may only create projects they own. diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index 09f07f704..cf2683e7c 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -218,6 +218,7 @@ name: 'Test Project', locale: 'fr', project_type: Project::Types::SCRATCH, + user_id: nil, components: [] } } @@ -259,5 +260,12 @@ expect(data[:project_type]).to eq(Project::Types::SCRATCH) end + + it 'sets the project user_id to the specified value (i.e. nil to represent a public project)' do + post('/api/projects', headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:user_id]).to be_nil + end end end From a193fa7c3b65e0b3133e83670a1a88ad94cab81a Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 19:20:26 +0100 Subject: [PATCH 08/10] Allow ExCS admin to update starter projects This gives users with the "experience-cs-admin" role permission to update starter (or "public") projects (i.e. projects with no `user_id` set). I've added an example to the "Updating a project" feature spec to check this works as intended. Note that in the feature spec, unlike in the other examples, I'm relying on the lookup via `ProjectLoader#load` which I think is the intended use (at least from Experience CS) and this is why I've had to set the project locale to one of the default fallback locales, i.e. 'en', in the spec setup. I haven't attempted to fix the other examples, but I've started looking at that in #553 and plan to address the problem separately. --- app/models/ability.rb | 1 + .../project/updating_a_project_spec.rb | 25 ++++++++++++++++++- spec/models/ability_spec.rb | 1 + 3 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index b2ff42eb9..8dea00a11 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -106,6 +106,7 @@ def define_experience_cs_admin_abilities(user) return unless user&.experience_cs_admin? can :create, Project + can :update, Project end def school_teacher_can_manage_lesson?(user:, school:, lesson:) diff --git a/spec/features/project/updating_a_project_spec.rb b/spec/features/project/updating_a_project_spec.rb index 41d325ce5..a9b78b6e0 100644 --- a/spec/features/project/updating_a_project_spec.rb +++ b/spec/features/project/updating_a_project_spec.rb @@ -10,7 +10,8 @@ end let(:headers) { { Authorization: UserProfileMock::TOKEN } } - let!(:project) { create(:project, name: 'Test Project', user_id: owner.id) } + let(:project_type) { Project::Types::PYTHON } + let!(:project) { create(:project, name: 'Test Project', user_id: owner.id, locale: 'en', project_type:) } let(:owner) { create(:owner, school:) } let(:school) { create(:school) } @@ -53,4 +54,26 @@ put("/api/projects/#{project.id}", params:) expect(response).to have_http_status(:unauthorized) end + + context 'when the user is an Experience CS admin and project type is scratch' do + let(:experience_cs_admin) { create(:experience_cs_admin_user) } + let(:project_type) { Project::Types::SCRATCH } + let(:params) { { project: { name: 'Test Project' } } } + + before do + authenticated_in_hydra_as(experience_cs_admin) + end + + it 'responds 200 OK' do + put("/api/projects/#{project.identifier}?project_type=scratch", headers:, params:) + expect(response).to have_http_status(:success) + end + + it 'sets the project name to the specified value' do + put("/api/projects/#{project.identifier}?project_type=scratch", headers:, params:) + data = JSON.parse(response.body, symbolize_names: true) + + expect(data[:name]).to eq('Test Project') + end + end end diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index ee54e1570..f723520b7 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -143,6 +143,7 @@ let(:user) { build(:experience_cs_admin_user) } it { is_expected.to be_able_to(:create, starter_project) } + it { is_expected.to be_able_to(:update, starter_project) } end # rubocop:disable RSpec/MultipleMemoizedHelpers From a1f888d54d40883b92b801ae2ae8861f5f1d15e7 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 19:37:50 +0100 Subject: [PATCH 09/10] Allow ExCS admin to destroy starter projects This gives users with the "experience-cs-admin" role permission to update starter (or "public") projects (i.e. projects with no `user_id` set). I've added an example to the destroy project request spec to check this works as intended. There are obviously some dangers associated with destroying a starter project, but I think it's still useful for an ExCS admin to be able to do it. --- app/models/ability.rb | 1 + spec/models/ability_spec.rb | 1 + spec/requests/projects/destroy_spec.rb | 23 +++++++++++++++++++++++ 3 files changed, 25 insertions(+) diff --git a/app/models/ability.rb b/app/models/ability.rb index 8dea00a11..87598810b 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -107,6 +107,7 @@ def define_experience_cs_admin_abilities(user) can :create, Project can :update, Project + can :destroy, Project end def school_teacher_can_manage_lesson?(user:, school:, lesson:) diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index f723520b7..930f39b94 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -144,6 +144,7 @@ it { is_expected.to be_able_to(:create, starter_project) } it { is_expected.to be_able_to(:update, starter_project) } + it { is_expected.to be_able_to(:destroy, starter_project) } end # rubocop:disable RSpec/MultipleMemoizedHelpers diff --git a/spec/requests/projects/destroy_spec.rb b/spec/requests/projects/destroy_spec.rb index f1d54066d..15ebc782b 100644 --- a/spec/requests/projects/destroy_spec.rb +++ b/spec/requests/projects/destroy_spec.rb @@ -36,6 +36,29 @@ expect(response).to have_http_status(:forbidden) end end + + context 'when experience-cs admin deleting a Scratch starter project' do + let(:project) do + create( + :project, { + project_type: Project::Types::SCRATCH, + user_id: nil, + locale: 'en' + } + ) + end + let(:experience_cs_admin) { create(:experience_cs_admin_user) } + + before do + authenticated_in_hydra_as(experience_cs_admin) + end + + it 'deletes the project' do + expect do + delete("/api/projects/#{project.identifier}?project_type=scratch", headers:) + end.to change(Project.unscoped, :count).by(-1) + end + end end context 'when no token is given' do From e69c2f3564ed855d43a47f4c0771c5ff82f26b4f Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 27 May 2025 17:27:22 +0100 Subject: [PATCH 10/10] Limit ExCS admin to managing only starter projects After a review by @chrisroos, we agreed a user with the "experience-cs-admin" role should: 1. Be able to manage starter (or public) projects, i.e. those that have `user_id` set to `nil`. 2. Be able to manage their own projects, i.e. those that have a `user_id` matching `User#id`. 3. Not be able to manage another user's projects, i.e. those that have a `user_id` that does not match `User#id`. I've expanded the examples in the `Ability` spec to cover these scenarios and amended the rules to conform with the spec. I'm taking "manage" permission as equivalent to the combination of read, create, update & destroy which covers all the standard RESTful controller actions. Point 1 was mostly already covered, except for read permission which allows access to show & index actions, so I've added that. Point 2 was already covered by permissions defined in `Ability#define_authenticated_non_student_abilities`. I've addressed point 3 by adding the `user_id: nil` constraint to the rules defined in `Ability#define_experience_cs_admin_abilities`. I've fixed the relevant examples in `spec/features/project/updating_a_project_spec.rb` by changing the project to be a starter project. I've tweaked the wording of the contexts in the three specs to clarify that they're about an Experience CS admin creating, updating & destroying a starter Scratch project which is our use case. Despite the confusion around `load_and_authorize_resource` discussed in #553, we're pretty confident that these CanCanCan rules are working as intended in `Api::ProjectsController`. And the specs seem to back that up. --- app/models/ability.rb | 4 +-- .../project/creating_a_project_spec.rb | 2 +- .../project/updating_a_project_spec.rb | 6 +++-- spec/models/ability_spec.rb | 26 ++++++++++++++++--- spec/requests/projects/destroy_spec.rb | 2 +- 5 files changed, 29 insertions(+), 11 deletions(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 87598810b..a7b65202e 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -105,9 +105,7 @@ def define_school_student_abilities(user:, school:) def define_experience_cs_admin_abilities(user) return unless user&.experience_cs_admin? - can :create, Project - can :update, Project - can :destroy, Project + can %i[read create update destroy], Project, user_id: nil end def school_teacher_can_manage_lesson?(user:, school:, lesson:) diff --git a/spec/features/project/creating_a_project_spec.rb b/spec/features/project/creating_a_project_spec.rb index cf2683e7c..c8b671deb 100644 --- a/spec/features/project/creating_a_project_spec.rb +++ b/spec/features/project/creating_a_project_spec.rb @@ -209,7 +209,7 @@ end end - context 'when the user is an Experience CS admin' do + context 'when an Experience CS admin creates a starter Scratch project' do let(:experience_cs_admin) { create(:experience_cs_admin_user) } let(:params) do { diff --git a/spec/features/project/updating_a_project_spec.rb b/spec/features/project/updating_a_project_spec.rb index a9b78b6e0..b0f31c34a 100644 --- a/spec/features/project/updating_a_project_spec.rb +++ b/spec/features/project/updating_a_project_spec.rb @@ -11,7 +11,8 @@ let(:headers) { { Authorization: UserProfileMock::TOKEN } } let(:project_type) { Project::Types::PYTHON } - let!(:project) { create(:project, name: 'Test Project', user_id: owner.id, locale: 'en', project_type:) } + let(:user_id) { owner.id } + let!(:project) { create(:project, name: 'Test Project', user_id:, locale: 'en', project_type:) } let(:owner) { create(:owner, school:) } let(:school) { create(:school) } @@ -55,8 +56,9 @@ expect(response).to have_http_status(:unauthorized) end - context 'when the user is an Experience CS admin and project type is scratch' do + context 'when an Experience CS admin creates a starter Scratch project' do let(:experience_cs_admin) { create(:experience_cs_admin_user) } + let(:user_id) { nil } let(:project_type) { Project::Types::SCRATCH } let(:params) { { project: { name: 'Test Project' } } } diff --git a/spec/models/ability_spec.rb b/spec/models/ability_spec.rb index 930f39b94..e3d959581 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -140,11 +140,29 @@ end context 'with an experience-cs admin' do - let(:user) { build(:experience_cs_admin_user) } + let(:user) { build(:experience_cs_admin_user, id: user_id) } + let(:another_project) { build(:project) } - it { is_expected.to be_able_to(:create, starter_project) } - it { is_expected.to be_able_to(:update, starter_project) } - it { is_expected.to be_able_to(:destroy, starter_project) } + context 'with a starter project' do + it { is_expected.to be_able_to(:read, starter_project) } + it { is_expected.to be_able_to(:create, starter_project) } + it { is_expected.to be_able_to(:update, starter_project) } + it { is_expected.to be_able_to(:destroy, starter_project) } + end + + context 'with own project' do + it { is_expected.to be_able_to(:read, project) } + it { is_expected.to be_able_to(:create, project) } + it { is_expected.to be_able_to(:update, project) } + it { is_expected.to be_able_to(:destroy, project) } + end + + context 'with another user\'s project' do + it { is_expected.not_to be_able_to(:read, another_project) } + it { is_expected.not_to be_able_to(:create, another_project) } + it { is_expected.not_to be_able_to(:update, another_project) } + it { is_expected.not_to be_able_to(:destroy, another_project) } + end end # rubocop:disable RSpec/MultipleMemoizedHelpers diff --git a/spec/requests/projects/destroy_spec.rb b/spec/requests/projects/destroy_spec.rb index 15ebc782b..bf139f029 100644 --- a/spec/requests/projects/destroy_spec.rb +++ b/spec/requests/projects/destroy_spec.rb @@ -37,7 +37,7 @@ end end - context 'when experience-cs admin deleting a Scratch starter project' do + context 'when an Experience CS admin destroys a starter Scratch project' do let(:project) do create( :project, {