Make editing the permissions of other users the same as editing your own permissions #2558

chrisroos · 2023-11-28T14:59:30Z

This PR brings the permission editing functionality for other users (under /users/<id>/) into line with the functionality we have for editing your own permissions (under /account/).

We've copied the relevant controllers and templates from their /account/ counterparts and modified them to work for editing another user. This has introduced some duplication and also highlighted some minor problems with the original implementation. We'll address those things separately.

Commits overview

Show the list of applications that a user does/doesn't have access to
Allow a user to give another user access to an application
Allow a user to remove another user's access to an application
Allow a user to view the permissions a user has for an application
Allow a user to update the permissions a user has for an application
Update the "Manage permissions" link on the edit user page so that it points to the new functionality
Remove the now-redundant functionality for managing user permissions using the table user interface

Differences from the existing behaviour

The current functionality allows you to grant another user non-signin permissions to an app without also having to grant them the signin permission. The new functionality requires the target user to have the signin permission before any other permissions can be granted. This constraint is currently implemented in the controllers but it would probably be better to move it the models.

Differences from functionality in the `/account/` namespace

The routes nested under a specific application (i.e. /users/<id>/applications/<id>/<path>) require the user to have the signin permission. This means, for example, that a user's application permissions can only be edited if that user already has access to the application. We'll backport this to the /account/ namespace separately.
The signin permission can only be granted for a non-API application. Although not possible through the UI it's technically possible to grant yourself the signin permission to an API application through the equivalent /account/ functionality.
The new Pundit policies we've introduced vary from their original counterparts in that they have to take the target user into account. We should be able to replace the account-specific policies with these more generic implementations.

Routes added

GET /users/<id>/applications - to display two lists of applications: those the user has access to and those the user doesn't have access to.
GET /users/<id>/applications/<id> - we don't have any information to show at this path so we redirect to /applications in order to have hackable URLs.
POST /users/<id>/applications/<id>/signin_permission - to add the application signin permission to the user.
DELETE /users/<id>/applications/<id>/signin_permission - to remove the application signin permission from the user.
GET /users/<id>/applications/<id>/signin_permission/delete - to request confirmation that the application signin permission should be removed from the user.
GET /users/<id>/applications/<id>/permissions - to display the list of supported permissions for the application, as well as whether the user has each of those supported permissions.
PUT /users/<id>/applications/<id>/permissions - to modify the users permissions for the application.
GET /users/<id>/applications/<id>/permissions/edit - to display a form allowing the user to choose which of the application's supported permissions the user should have.

These all mirror their /account/ counterparts.

Screenshots

Admin managing Org Admin - Before and After

Super Org Admin managing Org Admin - Before and After

Copied from the equivalent functionality in the account namespace. Co-authored-by: Chris Lowis <[email protected]>

This adds the `DELETE /users/<id>/appplications/<id>/signin_permission` route. Copied Account::SigninPermissionsController#destroy and updated it to allow users to remove the signin permission from another user. Co-authored-by: Chris Lowis <[email protected]>

This adds the `GET /users/<id>/applications/<id>/delete` route which displays a page asking the user to confirm the deletion of the permission. Copied Account::SigninPermissionsController#delete and updated it to allow users to remove the signin permission from another user. Co-authored-by: Chris Lowis <[email protected]>

Copied from the equivalent functionality in the account namespace. Co-authored-by: Chris Lowis <[email protected]>

This adds the `GET /users/<id>/applications/<id>/permissions` route which lists each application permission along with whether the user has been granted that permission. Copied from `Account::PermissionsController#show` and updated to allow the signed in user to view another user's permissions. These permissions are currently visible on the user edit page (/users/<id>/edit) which is only accessible to signed in users that can edit the target user based on the logic in `UserPolicy#edit?`. We're doing the same thing in this new action. Note the additional `stub_policy_for_navigation_links` method in the tests. This is to satisfy the call to `policy(User).index?` in `NavigationItemsHelper#navigation_items`[1]. [1]: https://github.com/alphagov/signon/blob/2bd159ff90fcf293bf494425fc7653c263c2420a/app/helpers/navigation_items_helper.rb#L10 Co-authored-by: Chris Lowis <[email protected]>

Co-authored-by: Chris Lowis <[email protected]>

We want to use this `message_for_success` helper method in our new `Users::PermissionsController` so we've renamed the module to make it clear that it's not intended to be restricted to the account namespace. Co-authored-by: Chris Lowis <[email protected]>

This adds the `PUT /users/<id>/applications/<id>/permissions` route. Copied from `Account::PermissionsController#update` and updated to allow the signed in user to view another user's permissions. Co-authored-by: Chris Roos <[email protected]>

This adds the `GET /users/<id>/applications/<id>/permissions/edit` route to allow the signed in user to select which permissions the target user should have for the selected application. Copied from `Account::PermissionsController` and updated to allow the signed in user to edit another user's permissions. Co-authored-by: Chris Roos <[email protected]>

We've amended `stub_policy` to return a `stub_everything` now that rendering the users/applications.index.html.erb template results in multiple calls to `UserApplicationPermissionPolicy` (to both `#edit?` and `#delete?`). Co-authored-by: Chris Lowis <[email protected]>

…ller We shouldn't be able to view or modify a user's permissions for an application unless that user has access to that application. Co-authored-by: Chris Lowis <[email protected]>

To DRY up the controller. Co-authored-by: Chris Lowis <[email protected]>

And invoke it in a before filter so that it's consistent with how we're loading the user. Co-authored-by: Chris Lowis <[email protected]>

We shouldn't be able to remove a user's access from an application unless that user has access to that application. Co-authored-by: Chris Lowis <[email protected]>

Permissions for API only applications should only be managed through the ApiUsersController. Co-authored-by: Chris Lowis <[email protected]>

To DRY up controllers and templates. We were previously relying on the call to `find_by!` in the controllers to raise an exception if the user didn't have the signin permission for the application. This is now handled by `set_application` in both controllers so it's OK that `User#signin_permission` returns nil if it doesn't exist. Co-authored-by: Chris Lowis <[email protected]>

We've updated the various granting_permissions_tests to reflect the UI change. We've had to update the setup of the following tests so that they continue to pass. should "support granting app-specific permissions" We've had to grant the user access to the app as it's no longer possible to give a user any permissions to an app unless they first have the signin permission for that app. should "not be able to grant permissions that are not grantable_from_ui" We have to add signin permission, and another permission that is grantable from the UI, so that we can get to the permissions/edit page: we don't show the "Update permissions for <app>" link if the signin permission is the only grantable permission from the UI. We've removed the following tests that no longer make sense given the current implementation. should "not remove permissions the user has that the super organisation admin does not have" should "not remove permissions the user has that the super organisation admin cannot delegate" should "not remove permissions the user has that the organisation admin does not have" should "not remove permissions the user has that the organisation admin cannot delegate" These tests were necessary because in the previous implementation another user's applications permissions were updated in bulk through the edit user page. User's permissions are now updated for a single application at a time. In order for a Publishing Manager to be able to edit another user's permissions for an application they need to have the signin permission for that application, and the signin permission needs to be delegatable. This logic is defined in the `UserApplicationPermissionPolicy`. Co-authored-by: Chris Lowis <[email protected]>

Users' permissions for applications are now managed through the `Users::SigninPermissionsController` and `Users::PermissionsController`. Co-authored-by: Chris Lowis <[email protected]>

The `UsersController#update` action is now only used to update `User#require_2sv` from the `/users/<id>/require_2sv` page so this is no longer required. Users' permissions for applications are now managed through the `Users::SigninPermissionsController` and `Users::PermissionsController`.

This was here to handle the use of `UsersController#update` when it was invoked from the `/users/<id>/require_2sv` page. The `require_2sv` page contains a form with a single checkbox that allows the `User#require_2sv` boolean to be toggled. If the checkbox is left unchecked then the controller doesn't receive a `user` param and the call to `params.require(:user)` fails. We're avoiding this problem by adding a hidden field to the form that defaults `require_2sv` to `0` which is the same way that Rails' `check_box` helper handles it[1]. [1]: https://api.rubyonrails.org/classes/ActionView/Helpers/FormBuilder.html#method-i-check_box

This action is now only used as the target of the form on the `/users/<id>/require_2sv` page and that can't (currently at least) fail validation. I considered displaying errors on the require_2sv page and rendering that if `UserUpdate#call` fails but it doesn't seem worth the effort.

We found the use of `User#signin_permission_for` confusing because it seems to imply that there's something special about this instance of `UserApplicationPermission`. In fact, any instance of `UserApplicationPermission` that links to both a `User` and `Application` contains enough information for us to determine whether the current user is authorised to edit a user's permissions for an application. The use of `UserApplicationPermission.for` in this commit attempts to make that clearer. Ideally we'd retrofit this change into the earlier commits that use `User#signin_permission_for` but we've decided that it's not worth the effort. Co-authored-by: Chris Lowis <[email protected]>

chrisroos · 2023-12-12T10:52:24Z

Thanks @floehopper! I've rebased and merged and will address your comments separately.

chrisroos · 2023-12-12T10:58:16Z

Lastly I wanted to mention that I agree with approach you've taken in relation to UsersController#require_2sv. It would be nice to add a comment on this Trello card to explain that you've made some changes in this area.

I've added a comment to the Trello card.

chrisroos · 2023-12-12T10:59:13Z

As I discussed on a call with @chrislo, I think it would be worth having a new section or a sentence or two in the PR description explaining the difference between the old behaviour for managing another user's permissions and the new behaviour for managing another user's permissions.

I've updated the PR description to explain that users must now have the signin permission before they can be given other non-signin permissions.

chrisroos · 2023-12-12T11:52:56Z

Another thing I noticed is that I don't think the new implementation respects the .permitted_user_params methods on the sub-classes of Roles::Base. I think this used to be enforced in the UsersController via the UserParameterSanitiser. While it might be a bit academic, because all the sub-classes except Roles::Normal include supported_permission_ids, I did try to retain this functionality in other similar controllers via User#permitted_params e.g. in Users::NamesController#user_params. At the very least it would be worth mentioning the change in the PR description.

You're correct that we're not using Roles::<role>.permitted_user_params in Users::PermissionsController. This was previously necessary as permissions were updated, along with other user attributes, in the UsersController#update action. Now that permissions are only updated (for other users) in the Users::PermissionsController we can rely on the UserApplicationPermissionsPolicy to control who's authorised to perform that action.

chrislo force-pushed the update-permission-editing-for-other-users branch from ecded08 to 8c5a663 Compare November 28, 2023 16:33

This was referenced Nov 29, 2023

Update permission editing on users edit page #2530

Closed

Refactor account pundit policies #2554

Closed

chrisroos force-pushed the update-permission-editing-for-other-users branch 8 times, most recently from 121be98 to bd1326b Compare November 29, 2023 17:35

chrislo force-pushed the update-permission-editing-for-other-users branch 4 times, most recently from d20da1d to 55f91f0 Compare November 30, 2023 15:14

chrisroos force-pushed the update-permission-editing-for-other-users branch 15 times, most recently from 44b9f17 to 4589cb7 Compare December 5, 2023 12:50

chrisroos and others added 23 commits December 12, 2023 10:37

Update UI to allow users to grant app access to other users

1931938

Copied from the equivalent functionality in the account namespace. Co-authored-by: Chris Lowis <[email protected]>

Update UI to allow users to remove app access from other users

edbc3a2

Copied from the equivalent functionality in the account namespace. Co-authored-by: Chris Lowis <[email protected]>

Update UI to allow users to view user application permissions

ca40cda

Co-authored-by: Chris Lowis <[email protected]>

Add Users::PermissionsController#update

1d12ef4

This adds the `PUT /users/<id>/applications/<id>/permissions` route. Copied from `Account::PermissionsController#update` and updated to allow the signed in user to view another user's permissions. Co-authored-by: Chris Roos <[email protected]>

Only find applications user has access to in Users::PermissionsContro…

5f256de

…ller We shouldn't be able to view or modify a user's permissions for an application unless that user has access to that application. Co-authored-by: Chris Lowis <[email protected]>

Extract Users::SigninPermissionsController#set_user

0fb10d6

To DRY up the controller. Co-authored-by: Chris Lowis <[email protected]>

Rename Users::SigninPermissionsController#application

61c5b22

And invoke it in a before filter so that it's consistent with how we're loading the user. Co-authored-by: Chris Lowis <[email protected]>

Only find applications user has access to in SigninPermissionsController

68df6a9

We shouldn't be able to remove a user's access from an application unless that user has access to that application. Co-authored-by: Chris Lowis <[email protected]>

Exclude API only apps from Users::SigninPermissionsController

6e7c32b

Permissions for API only applications should only be managed through the ApiUsersController. Co-authored-by: Chris Lowis <[email protected]>

Remove UsersController#manage_permissions

3b8a318

Users' permissions for applications are now managed through the `Users::SigninPermissionsController` and `Users::PermissionsController`. Co-authored-by: Chris Lowis <[email protected]>

Inline UsersController#permitted_user_params

f87004c

chrisroos force-pushed the update-permission-editing-for-other-users branch from 8b753b8 to 8f8672f Compare December 12, 2023 10:40

chrisroos merged commit 2a8b4cf into main Dec 12, 2023
14 checks passed

chrisroos deleted the update-permission-editing-for-other-users branch December 12, 2023 10:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make editing the permissions of other users the same as editing your own permissions #2558

Make editing the permissions of other users the same as editing your own permissions #2558

chrisroos commented Nov 28, 2023 •

edited

Loading

chrisroos commented Dec 12, 2023

chrisroos commented Dec 12, 2023

chrisroos commented Dec 12, 2023

chrisroos commented Dec 12, 2023

Make editing the permissions of other users the same as editing your own permissions #2558

Make editing the permissions of other users the same as editing your own permissions #2558

Conversation

chrisroos commented Nov 28, 2023 • edited Loading

Commits overview

Differences from the existing behaviour

Differences from functionality in the /account/ namespace

Routes added

Screenshots

Admin managing Org Admin - Before and After

Super Org Admin managing Org Admin - Before and After

chrisroos commented Dec 12, 2023

chrisroos commented Dec 12, 2023

chrisroos commented Dec 12, 2023

chrisroos commented Dec 12, 2023

chrisroos commented Nov 28, 2023 •

edited

Loading

Differences from functionality in the `/account/` namespace