From 36840a4eaf4e8829ca88e6bcd5bbb11737ba2ca5 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 21 May 2025 17:22:04 +0100 Subject: [PATCH 01/12] 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 ba8728688e02d3b3a8b3ebead0983fbf64262dbd Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 21 May 2025 17:44:38 +0100 Subject: [PATCH 02/12] 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/models/user_spec.rb | 12 ++++++++++++ 2 files changed, 16 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/models/user_spec.rb b/spec/models/user_spec.rb index c17c0e4b5..7cc065aa6 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(:user, roles: 'experience-cs-admin') + 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 a331e00f144185309d5a718f32a032b689221345 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 21 May 2025 17:54:47 +0100 Subject: [PATCH 03/12] Introduce permission for creating public projects I plan to use this to restrict creating public projects to experience-cs admin users. --- app/models/ability.rb | 6 ++++++ spec/factories/user.rb | 4 ++++ spec/models/ability_spec.rb | 16 ++++++++++++++++ 3 files changed, 26 insertions(+) diff --git a/app/models/ability.rb b/app/models/ability.rb index bca1f2dfa..1660892d9 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,10 @@ 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) + can :create, :public_project if user.experience_cs_admin? + 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/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/ability_spec.rb b/spec/models/ability_spec.rb index 39624d64e..9d7a40aea 100644 --- a/spec/models/ability_spec.rb +++ b/spec/models/ability_spec.rb @@ -29,6 +29,8 @@ it { is_expected.not_to be_able_to(:update, project) } it { is_expected.not_to be_able_to(:destroy, project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with a standard user' do @@ -56,6 +58,8 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with a teacher' do @@ -83,6 +87,8 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with an owner' do @@ -110,6 +116,8 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } end context 'with a student' do @@ -137,6 +145,14 @@ it { is_expected.not_to be_able_to(:update, another_project) } it { is_expected.not_to be_able_to(:destroy, another_project) } end + + it { is_expected.not_to be_able_to(:create, :public_project) } + end + + context 'with an experience-cs admin' do + let(:user) { build(:experience_cs_admin_user) } + + it { is_expected.to be_able_to(:create, :public_project) } end # rubocop:disable RSpec/MultipleMemoizedHelpers From dca4c4b8ea844a3318c48de5e9ac030d63455864 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:02:51 +0100 Subject: [PATCH 04/12] Improve Project::Create request spec * Remove redundant `create_project_with_content`. I suspect this was introduced due to a misunderstanding of how `subject` & `let` blocks are evaluated. The relevant difference is achieved through a variation of the `project_hash` `let` block in the two `context` blocks. * Move `subject`, `let` & `before` blocks into the `describe` or `context` blocks where they make the most sense. * Consistently use `Hash` vs `ActionController::Parameters`. Even though the latter is more realistic, it seems like an unnecessary complication. * Remove unneccessary call to `ActionController::Parameters.permit_all_parameters = true`. This was introduced in this commit [1] without explanation. --- spec/concepts/project/create_spec.rb | 23 ++++++++++------------- 1 file changed, 10 insertions(+), 13 deletions(-) diff --git a/spec/concepts/project/create_spec.rb b/spec/concepts/project/create_spec.rb index 2af9426f1..38e104a81 100644 --- a/spec/concepts/project/create_spec.rb +++ b/spec/concepts/project/create_spec.rb @@ -3,21 +3,16 @@ require 'rails_helper' RSpec.describe Project::Create, type: :unit do - subject(:create_project) { described_class.call(project_hash:) } - - let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' } + describe '.call' do + subject(:create_project) { described_class.call(project_hash:) } - before do - mock_phrase_generation - ActionController::Parameters.permit_all_parameters = true - end + let(:user_id) { 'e0675b6c-dc48-4cd6-8c04-0f7ac05af51a' } - describe '.call' do - let(:project_hash) { ActionController::Parameters.new({}).merge(user_id:) } + before do + mock_phrase_generation + end context 'with valid content' do - subject(:create_project_with_content) { described_class.call(project_hash:) } - let(:project_hash) do { project_type: Project::Types::PYTHON, @@ -32,16 +27,18 @@ end it 'returns success' do - expect(create_project_with_content.success?).to be(true) + expect(create_project.success?).to be(true) end it 'returns project with correct component content' do - new_project = create_project_with_content[:project] + new_project = create_project[:project] expect(new_project.components.first.content).to eq('print("hello world")') end end context 'when creation fails' do + let(:project_hash) { { user_id: } } + before do mock_project = instance_double(Project) allow(mock_project).to receive(:components).and_raise('Some error') From 256a4fe4940aee0f60b190afd73ab837ffcd108f Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:12:35 +0100 Subject: [PATCH 05/12] Refactor Project::Create.call & .build_project I'm planning to make some changes to these methods and these changes will make that easier. --- lib/concepts/project/operations/create.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/lib/concepts/project/operations/create.rb b/lib/concepts/project/operations/create.rb index 625cdba85..50e5f7910 100644 --- a/lib/concepts/project/operations/create.rb +++ b/lib/concepts/project/operations/create.rb @@ -5,8 +5,11 @@ class Create class << self def call(project_hash:) response = OperationResponse.new - response[:project] = build_project(project_hash) - response[:project].save! + + project = build_project(project_hash) + project.save! + + response[:project] = project response rescue StandardError => e Sentry.capture_exception(e) @@ -17,8 +20,8 @@ def call(project_hash:) private def build_project(project_hash) - identifier = PhraseIdentifier.generate - new_project = Project.new(project_hash.except(:components).merge(identifier:)) + project_hash[:identifier] = PhraseIdentifier.generate + new_project = Project.new(project_hash.except(:components)) new_project.components.build(project_hash[:components]) new_project end From cf77f46886efd2d1f4d38e56a27743e3c709d1b5 Mon Sep 17 00:00:00 2001 From: James Mead Date: Wed, 21 May 2025 22:06:45 +0100 Subject: [PATCH 06/12] Rename Project#check_unique_not_null -> generate_identifier I found the previous name confusing. --- app/models/project.rb | 4 ++-- spec/models/project_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index ebe956559..c3d3a98f6 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -20,7 +20,7 @@ module Types accepts_nested_attributes_for :components - before_validation :check_unique_not_null, on: :create + before_validation :generate_identifier, on: :create before_validation :create_school_project_if_needed validates :identifier, presence: true, uniqueness: { scope: :locale } @@ -81,7 +81,7 @@ def media private - def check_unique_not_null + def generate_identifier self.identifier ||= PhraseIdentifier.generate end diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 6102ac091..37bcb4077 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -154,7 +154,7 @@ end end - describe 'check_unique_not_null' do + describe 'generate_identifier' do let(:saved_project) { create(:project) } it 'generates an identifier if nil' do From ebba24cb73a43d45a09be896393567b4f592f677 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 22 May 2025 09:06:17 +0100 Subject: [PATCH 07/12] Simplify example for Project#generate_identifier The `saved_project` wasn't really being used and it's sufficient to assert the final state of `Project#identifier` rather than aserting that it's changed. --- spec/models/project_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 37bcb4077..51f9b01dc 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -155,11 +155,10 @@ end describe 'generate_identifier' do - let(:saved_project) { create(:project) } - it 'generates an identifier if nil' do - unsaved_project = build(:project, identifier: nil) - expect { unsaved_project.valid? }.to change { unsaved_project.identifier.nil? }.from(true).to(false) + project = build(:project, identifier: nil) + project.valid? + expect(project.identifier).not_to be_nil end end From 11448bb5a91b35b414fa7978cd3e86fa66fe7c29 Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 22 May 2025 09:07:46 +0100 Subject: [PATCH 08/12] Allow skipping of identifier generation When creating a public project we want the admin user to be able to specify the identifier and apply validation rules against it. However, prior to this change, if the supplied identifier was `nil`, the `before_validation` callback, `Project#generate_identifier`, would kick in and generate an identifier. Thus the presence validation on `Project#identifier` never failed. The new `Project#skip_identifier_generation` attribute gives us the ability in specific contexts to prevent the identifier being generated and thus allow the presence validation to fail. --- app/models/project.rb | 4 +++- spec/models/project_spec.rb | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/app/models/project.rb b/app/models/project.rb index c3d3a98f6..3b7f589c5 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -7,6 +7,8 @@ module Types SCRATCH = 'scratch' end + attr_accessor :skip_identifier_generation + belongs_to :school, optional: true belongs_to :lesson, optional: true belongs_to :parent, optional: true, class_name: :Project, foreign_key: :remixed_from_id, inverse_of: :remixes @@ -20,7 +22,7 @@ module Types accepts_nested_attributes_for :components - before_validation :generate_identifier, on: :create + before_validation :generate_identifier, on: :create, unless: :skip_identifier_generation before_validation :create_school_project_if_needed validates :identifier, presence: true, uniqueness: { scope: :locale } diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index 51f9b01dc..ab56c8471 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -160,6 +160,14 @@ project.valid? expect(project.identifier).not_to be_nil end + + context 'when skip_identifier_generation is true' do + it 'does not generate an identifier if nil' do + project = build(:project, identifier: nil, skip_identifier_generation: true) + project.valid? + expect(project.identifier).to be_nil + end + end end describe 'create_school_project_if_needed' do From 640273d1fef2d29a5a01ec33fb08ed01de325c1c Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:46:18 +0100 Subject: [PATCH 09/12] 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 f9f04874666ab57b5e5d2e22681945c8f0223cef Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:49:30 +0100 Subject: [PATCH 10/12] 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 4bae3a8c74a82de941c975332a5ed07fb1b21be6 Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:50:43 +0100 Subject: [PATCH 11/12] Simplify Project::Create.build_project There's no need to explicitly set `Project#identifier` here as well as in the `generate_identifier` `before_validation` in `Project`. While I'm not very convinced about the use of the `before_validation` callback, removing that would be a bigger and riskier piece of work. So instead I'm removing it here safe and relying on the `before_validation` to do its thing. Note that I added feature spec coverage to check the project is assigned a generated identifier in a previous commit. --- lib/concepts/project/operations/create.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/lib/concepts/project/operations/create.rb b/lib/concepts/project/operations/create.rb index 50e5f7910..0c6458f5f 100644 --- a/lib/concepts/project/operations/create.rb +++ b/lib/concepts/project/operations/create.rb @@ -20,7 +20,6 @@ def call(project_hash:) private def build_project(project_hash) - project_hash[:identifier] = PhraseIdentifier.generate new_project = Project.new(project_hash.except(:components)) new_project.components.build(project_hash[:components]) new_project From 3e97dcc6eaba107bb38adb9e80b8ad38672dee0f Mon Sep 17 00:00:00 2001 From: James Mead Date: Sun, 25 May 2025 10:22:12 +0100 Subject: [PATCH 12/12] Pass current_user to Project::Create.call I want to make some new behaviour conditional on the type of user. This change will make that easier. Note that I've used the `Identifiable#current_user` method (included into `ApiController`) to access the user unlike _some_ places in the `Api::ProjectsController` which use the `@current_user` instance variable directly. The latter feels dangerous, because the instance variable is only defined after the first time in a request that `Identifiable#current_user` is called and the value is memoized. Thus the places that are using the instance variable directly are relying on an earlier call to `Identifiable#current_user` (probably in `load_and_authorize_resource`) which feels risky and confusing. This seems like a fairly widespread problem across the API controllers, so I'm not going to attempt to fix it here. --- app/controllers/api/projects_controller.rb | 2 +- lib/concepts/project/operations/create.rb | 6 +++--- spec/concepts/project/create_spec.rb | 5 +++-- 3 files changed, 7 insertions(+), 6 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/lib/concepts/project/operations/create.rb b/lib/concepts/project/operations/create.rb index 0c6458f5f..7e40da299 100644 --- a/lib/concepts/project/operations/create.rb +++ b/lib/concepts/project/operations/create.rb @@ -3,10 +3,10 @@ class Project class Create class << self - def call(project_hash:) + def call(project_hash:, current_user:) response = OperationResponse.new - project = build_project(project_hash) + project = build_project(project_hash, current_user) project.save! response[:project] = project @@ -19,7 +19,7 @@ def call(project_hash:) private - def build_project(project_hash) + def build_project(project_hash, _current_user) new_project = Project.new(project_hash.except(:components)) new_project.components.build(project_hash[:components]) new_project diff --git a/spec/concepts/project/create_spec.rb b/spec/concepts/project/create_spec.rb index 38e104a81..4619a4b79 100644 --- a/spec/concepts/project/create_spec.rb +++ b/spec/concepts/project/create_spec.rb @@ -4,9 +4,10 @@ RSpec.describe Project::Create, type: :unit do describe '.call' 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) { build(:user) } + let(:user_id) { current_user.id } before do mock_phrase_generation