Allow Per-department permissions #1362
Conversation
KludgeKML
changed the title
KludgeKML
force-pushed
the
per-department-permissions
branch
4 times, most recently
from
d641dab
to
e07357d
Compare
KludgeKML
force-pushed
the
per-department-permissions
branch
2 times, most recently
from
6bee9d0
to
2534848
Compare
There was a problem hiding this comment.
This is looking really good.
I especially like the use of system and request tests.
I've left a few inline comments, but there isn't anything major.
One thing I have noticed though, that Local links manager doesn't have a way to get users back to Sign-on to choose another app, e.g. a departmental publisher wanting to go into Whitehall.
See the "switch app" menu option in Whitehall:
Compared to Local links manager:
If we are opening this up to non-GDS people, we probably need to add something so that they don't get trapped.
KludgeKML
force-pushed
the
per-department-permissions
branch
2 times, most recently
from
c84644d
to
b218f4b
Compare
- Add Switch app option - Add check so that without the GDS Editor permission, a user won't see the Broken Links or Councils links - Add new System test to check this - Update authentication support file to simplify it - Include authentication by default in all test types - Add infer test type to RSpec Co-authored-by: Ramya Vidapanakal <[email protected]>
KludgeKML
force-pushed
the
per-department-permissions
branch
5 times, most recently
from
dac392d
to
c3d417c
Compare
There was a problem hiding this comment.
Thanks for updating how permissions are checked so that they follow a consistent pattern 🎉
I've resolved all the comments from the previous review. Thanks for fixing.🥇
The only thing that's still a bit of a brain twister is the logic for permissions in the controllers. I've tried to think of ways to simplify the logic in inline comments, but it's tricky. I think it might be easier to follow if permissions are specifically applied to controller actions. i.e. actions a, b, and c require "gds_editor", d, e and f require "gds_editor" or "organisation", and if the action doesn't require any then, don't mention it. It might make the code a bit more verbose, but there wouldn't be any need to figure out what needs what.
I've also added a few other minor comments inline.
| before_action :redirect_unless_gds_editor, except: %i[update download_links_csv upload_links_csv] | ||
| before_action :forbid_unless_gds_editor, only: %i[update download_links_csv upload_links_csv] |
There was a problem hiding this comment.
I find this a real brain teaser, probably because the list of links is the same in both actions. I get there, but it takes a while.
I think that as bad_homepage_url_and_status_csv is a file download, it should probably be forbidden if the user is not a gds_editor rather than a redirect.
There was a problem hiding this comment.
What I've done is just been explicit about everything. There's an argument that we should do:
before_action blah_permission, only: [ etc]
before_action blah_permission 2, only: [ etc_a etc_b]
before_action blah_permission 3, except [listed above]
...because it means that if someone adds an action it always gets the final permission. But we don't add actions that frequently, so it might be better to only use only and always just list out all actions they apply to.
(I've also fixed the download to make it forbidden)
There was a problem hiding this comment.
I think perhaps this was made more confusing by having "unless" in the action name. Things being truthy can make the logic easier to follow.
i.e. rather than forbid_unless_gds_editor perhaps it could be named allow_if_gds_editor. Then even if an except was used there'd only be one negative in the logic. I think it was the double negatives that were causing the brain twisting.
| @@ -0,0 +1,16 @@ | |||
| RSpec.describe "Services page" do | |||
There was a problem hiding this comment.
Should the other routes have tests too? i.e. download_links_csv and upload_links_csv.
| before_action :forbid_unless_permission, except: %i[index update_owner_form update_owner] | ||
| before_action :forbid_unless_gds_editor, only: %i[update_owner_form update_owner] |
There was a problem hiding this comment.
I'm still finding these permissions confusing. Using "except" and "only" is as confusing as reading "unless, else" for me, especially as the routes in both actions are almost the same.
So if I'm reading it correctly, everyone can see index, update_owner_form and update_owner?
But the exception for update_owner_form and update_owner are overwritten on the next line and restricted to gds_editor only?
Does update_owner_form and update_owner need to be covered by forbid_unless_permission if they're already covered by forbid_unless_gds_editor?
I for the controllers that the departments can access, I think it's only the create, update and destroy routes that need explicit permissions? The "GET" routes don't need to be restricted as everything else is restricted to only showing the data for the users organisation.
So perhaps all that's needed is the forbid_unless_gds_editor action?
before_action :forbid_unless_gds_editor, only: %i[update_owner_form update_owner]There was a problem hiding this comment.
As above, I've made this so it just uses only:
KludgeKML
force-pushed
the
per-department-permissions
branch
from
c3d417c
to
1681ed2
Compare
- Auto-redirect to services page if not a GDS Editor, or forbid if it's not a navigable page. - Edit link can be accessed by an owner. - Add request tests to enforce these checks. - Add shared examples to make tests simpler. Co-authored-by: Ramya Vidapanakal <[email protected]>
- Add system test
KludgeKML
force-pushed
the
per-department-permissions
branch
from
1681ed2
to
ddf1f59
Compare
- Only GDS Editors can see local authority list, or use local authority endpoints. - Add request tests Co-authored-by: Ramya Vidapanakal <[email protected]>
- GDS Editors can see list of all services, department users can only see their own services. - All actions on services can only be performed by GDS Editors or users from owning departments. - It's now legit for the services page not to have any services appearing because the current user might not belong to a department that owns any yet, and for GDS Editors a catastrophic loss of data shouldn't be a thing that a user has to detect. - Make the upload services CSV work like the same one in the services controller (redirect to the service on a good import, redirect to the form on a bad one) Co-authored-by: Ramya Vidapanakal <[email protected]>2
Co-authored-by: Ramya Vidapanakal <[email protected]>
- Only GDS Editors can see this for the moment Co-authored-by: Ramya Vidapanakal <[email protected]>
- Add owners to info block - Turn council link into plain text, since it is confusing having two action links in the table row anyway, and department users shouldn't be able to see them Co-authored-by: Ramya Vidapanakal <[email protected]>
- Use login_as_gds_editor because all old tests just assume you have all powers. - Add default organisation for factory user Co-authored-by: Ramya Vidapanakal <[email protected]>
KludgeKML
force-pushed
the
per-department-permissions
branch
from
ddf1f59
to
55f32b0
Compare
Adds the ability to mark a service as owned by one or more organisations, allowing users from those organisations that have been granted access to Local Links Manager in signon to view and edit links for those services. Other existing actions, and the ability to set owners, now only available with the new
GDS Editorpermission.A menu option to Switch app has been added, consistent with Whitehall.
New tests are Request/System specs - we have not updated old Controller/Feature tests in this app, that has been captured as technical debt for updating later.
https://trello.com/c/1eUXHHBz/22-restrict-access-to-a-specific-service
Screenshots
View of the services page when logged in as a user (note main menu does not offer the broken links or council options)
View of an owned service page when logged in as a user (note main menu does not offer the broken links or council options, update owners link is not available in sidebar)
View of the edit link page for an owned service page when logged in as a user (note main menu does not offer the broken links or council options, delete option is not available)
View of an owned service page when logged in with GDS Editor permissions (update owners link is available in sidebar)
New Update Owners form, available on when logged in with GDS Editor permissions