Fix Users and Account::PermissionsController#update #2598
Merged
Conversation
This file contains 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
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>
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
force-pushed
the
fix-permissions-controllers
branch
2 times, most recently
from
08af8f3
to
b0d03d2
Compare
chrisroos
changed the title
|
@chrisroos @chrislo The 3rd commit note "Fix issues in Users::PermissionsController#update" looks like it's got the remnants of a squashed commit at the bottom - it would be nice to tidy that up |
floehopper
approved these changes
Dec 14, 2023
There was a problem hiding this comment.
The 3rd commit note "Fix issues in Users::PermissionsController#update" looks like it's got the remnants of a squashed commit at the bottom - it would be nice to tidy that up.
As I mentioned earlier today, I'm nervous about the complexity of PunditHelpers#stub_policy but I think that's something we can tackle separately.
Otherwise this all looks great to me - good work!
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>
Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>
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>
chrislo
force-pushed
the
fix-permissions-controllers
branch
from
b0d03d2
to
edf2e83
Compare
|
Thanks @floehopper - we've fixed that commit message. Rebasing on main before merging. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Trello: https://trello.com/c/09yMSdcU
We noticed some problems with the implementation of both
Users::PermissionsController#updateandAccount::PermissionsController#update:updateupdateupdateUserUpdatefor applications other than the application whose permissions were being viewedSee the individual commits for more detail.
We're fixing these problems before moving the API User edit page to use similar functionality.