Skip to content

Commit e69c2f3

Browse files
committed
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.
1 parent a1f888d commit e69c2f3

File tree

5 files changed

+29
-11
lines changed

5 files changed

+29
-11
lines changed

app/models/ability.rb

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -105,9 +105,7 @@ def define_school_student_abilities(user:, school:)
105105
def define_experience_cs_admin_abilities(user)
106106
return unless user&.experience_cs_admin?
107107

108-
can :create, Project
109-
can :update, Project
110-
can :destroy, Project
108+
can %i[read create update destroy], Project, user_id: nil
111109
end
112110

113111
def school_teacher_can_manage_lesson?(user:, school:, lesson:)

spec/features/project/creating_a_project_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,7 +209,7 @@
209209
end
210210
end
211211

212-
context 'when the user is an Experience CS admin' do
212+
context 'when an Experience CS admin creates a starter Scratch project' do
213213
let(:experience_cs_admin) { create(:experience_cs_admin_user) }
214214
let(:params) do
215215
{

spec/features/project/updating_a_project_spec.rb

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,8 @@
1111

1212
let(:headers) { { Authorization: UserProfileMock::TOKEN } }
1313
let(:project_type) { Project::Types::PYTHON }
14-
let!(:project) { create(:project, name: 'Test Project', user_id: owner.id, locale: 'en', project_type:) }
14+
let(:user_id) { owner.id }
15+
let!(:project) { create(:project, name: 'Test Project', user_id:, locale: 'en', project_type:) }
1516
let(:owner) { create(:owner, school:) }
1617
let(:school) { create(:school) }
1718

@@ -55,8 +56,9 @@
5556
expect(response).to have_http_status(:unauthorized)
5657
end
5758

58-
context 'when the user is an Experience CS admin and project type is scratch' do
59+
context 'when an Experience CS admin creates a starter Scratch project' do
5960
let(:experience_cs_admin) { create(:experience_cs_admin_user) }
61+
let(:user_id) { nil }
6062
let(:project_type) { Project::Types::SCRATCH }
6163
let(:params) { { project: { name: 'Test Project' } } }
6264

spec/models/ability_spec.rb

Lines changed: 22 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -140,11 +140,29 @@
140140
end
141141

142142
context 'with an experience-cs admin' do
143-
let(:user) { build(:experience_cs_admin_user) }
143+
let(:user) { build(:experience_cs_admin_user, id: user_id) }
144+
let(:another_project) { build(:project) }
144145

145-
it { is_expected.to be_able_to(:create, starter_project) }
146-
it { is_expected.to be_able_to(:update, starter_project) }
147-
it { is_expected.to be_able_to(:destroy, starter_project) }
146+
context 'with a starter project' do
147+
it { is_expected.to be_able_to(:read, starter_project) }
148+
it { is_expected.to be_able_to(:create, starter_project) }
149+
it { is_expected.to be_able_to(:update, starter_project) }
150+
it { is_expected.to be_able_to(:destroy, starter_project) }
151+
end
152+
153+
context 'with own project' do
154+
it { is_expected.to be_able_to(:read, project) }
155+
it { is_expected.to be_able_to(:create, project) }
156+
it { is_expected.to be_able_to(:update, project) }
157+
it { is_expected.to be_able_to(:destroy, project) }
158+
end
159+
160+
context 'with another user\'s project' do
161+
it { is_expected.not_to be_able_to(:read, another_project) }
162+
it { is_expected.not_to be_able_to(:create, another_project) }
163+
it { is_expected.not_to be_able_to(:update, another_project) }
164+
it { is_expected.not_to be_able_to(:destroy, another_project) }
165+
end
148166
end
149167

150168
# rubocop:disable RSpec/MultipleMemoizedHelpers

spec/requests/projects/destroy_spec.rb

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
end
3838
end
3939

40-
context 'when experience-cs admin deleting a Scratch starter project' do
40+
context 'when an Experience CS admin destroys a starter Scratch project' do
4141
let(:project) do
4242
create(
4343
:project, {

0 commit comments

Comments
 (0)