Fix authorization in Api::ProjectsController #553
Conversation
floehopper
force-pushed
the
fix-authorization-in-api-projects-controller
branch
from
e62097e to
ed1be1b
Compare
raspberrypiherokubot
temporarily deployed
to
editor-api-p-fix-author-tua1dw
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.
|
Having discussed this with @chrisroos, I've decided to park this until we have time to go through the various clients of editor-api and see whether any of them might be relying on being able to lookup projects by |
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.
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.
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.
I've just added the following paragraph to the PR description:
Is that enough to convince you that this is a safe change to make? Marking as ready for review. |
I think we'd have to inspect the logs to be really confident that IDs aren't being used in these requests. But I agree that it seems unlikely that IDs are being used. I've been looking through the history and think I have a better idea of what might've happened: Prior to ce88a04 ("Make component content nullable" on 7th July 2022) the Commit 4dc6d78 ("Project serve based on lang (#160)" on 13th March 2023) updated the Commit ebaa275 ("Add partial feature tests for updating a project" on 25th Feb 2024) added So, although it's been possible to find projects by their ID since 13th March 2023, I agree that it's unlikely to have been used given that the project IDs aren't being exposed as far as we can tell. |
floehopper
force-pushed
the
fix-authorization-in-api-projects-controller
branch
2 times, most recently
from
1ff08c9 to
b48f478
Compare
This is more idiomatic and thus easier to read.
This value doesn't change in any of the contexts, so using a `let` block is of limited value. Also it will avoid triggering a violation of `RSpec/MultipleMemoizedHelpers` in a later commit.
Previously this spec was not representative of how the API was being used in practice and it was only really working by accident. The PUT requests in the examples were using the `Project#id` rather than the `Project#identifier` as they should have been. The spec was working even though the `Api::ProjectsController#load_project` before action was *not* finding the project, because the `load_and_authorize_resource` before action was then finding the project using `Project.find(params[:id])` and setting the `@project` instance variable used by the rest of the logic in the controller. However, clients of editor-api like editor-standalone use the project identifier as the resource ID in the URL path [1], so this spec was completely unrepresentative of how the endpoint was really being used. In this commit I've changed the examples to use the `Project#identifier` as the resource ID in the PUT requests. This means that the `Api::ProjectsController#load_project` before action now finds the project by identifier and since it sets the `@project` instance variable the `load_and_authorize_resource` before action no longer attempts to load the resource, it just does the authorize step using the already loaded project. In order to make this work reliably, I had to explicitly set the `Project#locale` to one of the fallback locales in `ProjectLoader` [2], i.e. 'en' or `nil`. Otherwise, the random locale selected in the project factory [3] meant that sometimes the `load_project` before action (which uses the `ProjectLoader` did not find the project and the `load_and_authorize_resource` before action raised an `ActiveRecord::RecordNotFound` resulting in a 404 Not Found. Now the spec is no longer making use of the randome locale from the factory, I've taken the opportunity to add some new examples demonstrating the behaviour when the project has different locales. This effectively demonstrates that `load_project` is wired up to `ProjectLoader` correctly. As an aside, I'm not convinced that having the factory select a locale at random is a good idea. I found it very confusing when it led to undeterministic specs. However, I'll leave that for another time. [1]: https://github.com/RaspberryPiFoundation/editor-standalone/blob/1d4375635cb6890794732072d608dbd4b05b3bb0/src/utils/apiCallHandler/projects.js#L16 [2]: https://github.com/RaspberryPiFoundation/editor-api/blob/b4bd337d09a88b1f41ecdc13136f7d11da0dcf89/lib/project_loader.rb#L8 [3]: https://github.com/RaspberryPiFoundation/editor-api/blob/b4bd337d09a88b1f41ecdc13136f7d11da0dcf89/spec/factories/project.rb#L9
As I explained in the previous commit, the `load_project` before action
is where we want the project to be loaded, i.e. via `ProjectLoader` so
that it's found by a combination of `Project#identifier` and
`Project#locale`.
To make this clearer, I've changed the `load_and_authorize_resource`
before action to `authorize_resource` [1], so the CanCanCan
authorization uses the project found by the `load_project` before
action.
However, this meant that if the project was *not* found by the
`load_project` before action an exception was raised in
`Project::Update.call` resulting in a 422 Unprocessable Entity response
with the following error message:
Error persisting changes: undefined method `components' for nil:NilClass
To fix this I'm now raising an `ActiveRecord::RecordNotFound` exception
in the `load_project` before action if no project is found. This results
in the expected 404 Not Found response.
I think there's a strong case to be made the this exception raising
behaviour should be added to `ProjectLoader#load`. However, that's a
bigger change with a lot more risk, so I'm going to leave that for now.
Note that I've retained the load resource functionality for the `create`
action, because the `load_project` before action isn't triggered for
`create` and the authorize resource functionality seems to rely on the
project built by the load resource step and I want to keep changes to a
minimum.
[1]: https://github.com/CanCanCommunity/cancancan/blob/3.4.0/docs/controller_helpers.md#authorize_resource-load_resource-load_and_authorize_resource
floehopper
force-pushed
the
fix-authorization-in-api-projects-controller
branch
from
b48f478 to
485e5de
Compare
Amazing detective work! That does give me more confidence. I've now rebased this against |
raspberrypiherokubot
temporarily deployed
to
editor-api-p-fix-author-fls0u3
Previously this spec was not representative of how the API was being used in practice and it was only really working by accident. The
PUTrequests in the examples were using theProject#idrather than theProject#identifieras they should have been.The spec was working even though the
Api::ProjectsController#load_projectbefore action was not findingthe project, because the
load_and_authorize_resourcebefore action was then finding the project usingProject.find(params[:id])and setting the@projectinstance variable used by the rest of the logic in the controller.However,
editor-standaloneuses the project identifier as the resource ID in the URL path, so this spec was completely unrepresentative of how the endpoint was really being used.I've changed the examples to use the
Project#identifieras the resource ID in thePUTrequests. This means that theApi::ProjectsController#load_projectbefore action now finds the project by identifier and since it sets the@projectinstance variable theload_and_authorize_resourcebefore action no longer attempts to load the resource, it just does the authorize step using the already loaded project.In order to make this work reliably, I had to explicitly set the
Project#localeto one of the fallback locales inProjectLoader, i.e. 'en' ornil. Otherwise, the random locale selected in the project factory meant that sometimes theload_projectbefore action (which uses theProjectLoaderdid not find the project and theload_and_authorize_resourcebefore action raised anActiveRecord::RecordNotFoundresulting in a 404 Not Found.Now the spec is no longer making use of the random locale from the factory, I've taken the opportunity to add some new examples demonstrating the behaviour when the project has different locales. This effectively demonstrates that
load_projectis wired up toProjectLoadercorrectly.To make it clearer the
load_projectbefore action is where we want the project to be loaded, I've changed theload_and_authorize_resourcebefore action toauthorize_resource, so the CanCanCan authorization uses the project found by theload_projectbefore action.However, this meant that if the project was not found by the
load_projectbefore action an exception was raised inProject::Update.callresulting in a422 Unprocessable Entityresponse with the following error message:To fix this I'm now raising an
ActiveRecord::RecordNotFoundexception in theload_projectbefore action if no project is found. This results in the expected404 Not Foundresponse.I think there's a strong case to be made the this exception raising behaviour should be added to
ProjectLoader#load. However, that's a bigger change with more risk, so I'm going to leave that for now.Note that I've retained the load resource functionality for the
createaction, because theload_projectbefore action isn't triggered forcreateand the authorize resource functionality seems to rely on the project built by the load resource step and I want to keep changes to a minimum.I'm confident that no clients of
editor-apiare trying to find projects byProject#idas opposed toProject#identifier, becauseProject#idis never exposed inProject-related JSON responses, e.g. inapp/views/api/projects/show.json.jbuildernor in any of the JSON views inapp/views/api/projects/.