Allow Experience CS admins to manage starter/public projects#552
Merged
floehopper merged 10 commits intomainfrom May 28, 2025
Merged
Allow Experience CS admins to manage starter/public projects#552floehopper merged 10 commits intomainfrom
floehopper merged 10 commits intomainfrom
Conversation
This was referenced May 25, 2025
raspberrypiherokubot
temporarily deployed
to
editor-api-p-allow-expe-knnaes
floehopper
force-pushed
the
allow-experience-cs-admins-to-manage-public-projects
branch
from
910105d to
7892057
Compare
floehopper
force-pushed
the
allow-experience-cs-admins-to-manage-public-projects
branch
from
7892057 to
0e0661c
Compare
floehopper
changed the title
floehopper
force-pushed
the
allow-experience-cs-admins-to-manage-public-projects
branch
from
0e0661c to
d93e972
Compare
floehopper
force-pushed
the
allow-experience-cs-admins-to-manage-public-projects
branch
from
d93e972 to
5f0e4ae
Compare
floehopper
force-pushed
the
allow-experience-cs-admins-to-manage-public-projects
branch
from
5f0e4ae to
28eec06
Compare
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]: 9a1fdb1
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.
This is more idiomatic and easier to follow.
I'm about to make a change to how this works and I want to be sure the change won't break anything.
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.
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`
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.
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.
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.
floehopper
force-pushed
the
allow-experience-cs-admins-to-manage-public-projects
branch
from
28eec06 to
a1f888d
Compare
chrisroos
previously approved these changes
May 27, 2025
Contributor
chrisroos
left a comment
chrisroos
left a comment
There was a problem hiding this comment.
I've been through this in detail and subsequently chatted to @floehopper about it. We agreed that it's probably better to restrict the experience-cs-admin permissions further and only allow them to CRUD Projects where the user_id is nil to hopefully avoid admins updating user-owned projects. James is going to work on that change but I'm going to approve this in the meantime so that it's ready to merge once that change has been made.
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.
sebjacobs
approved these changes
May 27, 2025
floehopper
deleted the
allow-experience-cs-admins-to-manage-public-projects
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See https://github.com/RaspberryPiFoundation/experience-cs/issues/405
This introduces a new "experience-cs-admin" role which allows the user to create, update & destroy starter/public projects, i.e. projects where the
user_idis set tonil. The intention is to use this from the Experience CS admin UI to allow the Learning Team to create and manage starter/public Scratch projects.I've tried very hard to keep the risk of breaking existing Code Editor functionality to a minimum without introducing too much extra complexity. Indeed, initially I considered a separate controller and set of endpoints (see my work in #549 & #551), however I felt as if this was introducing too much of a maintenance burden. As you'll see from those other pull requests, I did also consider adding some validation, e.g. for the format of
Project#identifierand for the presence ofProject#name. However, without access to the production environment, I couldn't ascertain whether this would make any existing projects invalid. And, in any case, with such a "low-level" change, it would've been difficult to be certain that it didn't break existing Code Editor functionality.Aside from the adding support for the new "experience-cs-admin" role, the two main changes in this pull request are in (a)
Api::ProjectsController#project_paramswhere we now allow an Experience CS admin to setProject#user_idtonilto signify a starter/public project'; and (b) inProject::Create#build_projectwhere we now allow a user to an Experience CS adminProject#identifier.While working on this I noticed some issues with the way project loading was working. Essentially I strongly suspect some of the specs are not representative of how the endpoints are actually being used and are only working by accident. The implementation itself is probably (mostly) working OK, but it's extremely confusing. I hope to address that in #553.
While working on this I also noticed some issues with the use of the
@current_userinstance variable inApi::ProjectsController, but I'll open a separate pull request for that.Supersedes #549 & #551