Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix Users and Account::PermissionsController#update #2598

Merged
merged 6 commits into from
Dec 14, 2023
Merged

Fix Users and Account::PermissionsController#update #2598

merged 6 commits into from
Dec 14, 2023

Commits on Dec 14, 2023

  1. Add test coverage for updating a user's permissions 

    This test is satisfied by the behaviour in the `UserUpdate` service.
We're adding it to give us confidence to make changes in this area in
future.

Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
    @chrisroos @chrislo
    chrisroos and chrislo committed Dec 14, 2023
    Configuration menu
    Copy the full SHA
    0384efc View commit details
    Browse the repository at this point in the history

  2. Retain signin permission for app when updating user permissions 

    We noticed that it was possible to construct a request to the
`Users::PermissionsController#update` action that resulted in the user's
signin permission being removed from the application. This won't happen
through normal use of the user interface but we should guard against it
nonetheless.

The same problem exists in `Account::PermissionsController#update` and
we intend to fix that in a separate commit.

Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
    @chrisroos @chrislo
    chrisroos and chrislo committed Dec 14, 2023
    Configuration menu
    Copy the full SHA
    f88e930 View commit details
    Browse the repository at this point in the history

  3. Fix issues in Users::PermissionsController#update 

    In addition to losing the signin permission (which we fixed in the
previous commit) we also noticed 3 other issues with the current
implementation. We attempted to add each of these tests in their own
commit but the fix we've come up with resolves all 3 so we've decided to
combine them.

1. should "not remove permissions the user already has that are not
   grantable from ui"

If a user had been granted a permission that isn't grantable from the UI
then submitting the form to update their permissions would result in
that permission being lost. In production the only permission not
grantable from the UI is the `user_update_permission` and this is only
given to the "Signon API Client" (in `SSOPushCredential.credentials`[1])
so we don't think this has been causing any real problems but we should
fix it nonetheless.

Note that we're sorting the `SupportedPermission#id`s that end up in the
arguments to `UserUpdate.new` to make it easier to test.

2. should "prevent permissions being added for other apps"

Although not possible through the user interface, it was possible to
craft a request to give permissions to applications other than the
application identified by `params[:application_id]` in the URL. There
was no real danger here because the `UserUpdate` service ensures that
the current user can only modify permissions for users and applications
they're allowed to manage.

3. should "prevent permissions being added that are not grantable from
   the ui"

Although not possible through the user interface, it was possible to
craft a request to add a permission that's not grantable through the UI.
As explained in (1) above, the only permission not grantable from the UI
is the `user_update_permission` and this doesn't have any effect for
users other than the "Signon API Client".

These same problems exists in `Account::PermissionsController#update` and
we intend to fix that in a separate commit.

[1]: https://github.com/alphagov/signon/blob/f6690d8d13890da3279313e47cedfcefaeeeb7e3/lib/sso_push_credential.rb#L11

Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
    @chrisroos @chrislo
    chrisroos and chrislo committed Dec 14, 2023
    Configuration menu
    Copy the full SHA
    6adf1cb View commit details
    Browse the repository at this point in the history

  4. Extract Users::PermissionsController#set_permissions 

    Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
    @chrisroos @chrislo
    chrisroos and chrislo committed Dec 14, 2023
    Configuration menu
    Copy the full SHA
    4ecb63b View commit details
    Browse the repository at this point in the history

  5. Extract Users::PermissionsController#build_user_update_params 

    Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
    @chrisroos @chrislo
    chrisroos and chrislo committed Dec 14, 2023
    Configuration menu
    Copy the full SHA
    92c5f5c View commit details
    Browse the repository at this point in the history

  6. Fix Account::PermissionsController#update 

    By duplicating the tests and implementation we've recently added for
`Users::PermissionsController#update`.

We discussed extracting a method to remove the duplication between the
two `build_user_update_params` methods but couldn't agree on where the
responsibility should live, so we've decided to leave it for now.

Note the change to `PunditHelpers#stub_policy` where we've updated it to
handle namespaced policies.

Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
    @chrisroos @chrislo
    chrisroos and chrislo committed Dec 14, 2023
    Configuration menu
    Copy the full SHA
    edf2e83 View commit details
    Browse the repository at this point in the history