Fix delegatable permissions #3070
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
yndajas
changed the base branch from
main
to
remove-user-manageable-application-policy
Base automatically changed from
remove-user-manageable-application-policy
to
main
August 7, 2024 11:04
yndajas
force-pushed
the
1184-redux
branch
5 times, most recently
from
5909af4 to
58c27b0
Compare
yndajas
changed the base branch from
main
to
remove-user-application-permission-policy
yndajas
changed the base branch from
remove-user-application-permission-policy
to
use-application-policy-in-users-applications-controller
yndajas
force-pushed
the
use-application-policy-in-users-applications-controller
branch
from
6cdf5f8 to
3efa22d
Compare
Base automatically changed from
use-application-policy-in-users-applications-controller
to
main
August 8, 2024 09:00
yndajas
force-pushed
the
1184-redux
branch
6 times, most recently
from
e5015f1 to
eb7ddf9
Compare
yndajas
changed the base branch from
main
to
update-users-application-policy-tests
yndajas
force-pushed
the
update-users-application-policy-tests
branch
from
ebe3d50 to
25d5c76
Compare
yndajas
force-pushed
the
1184-redux
branch
3 times, most recently
from
979b4a1 to
916d3bf
Compare
yndajas
force-pushed
the
1184-redux
branch
2 times, most recently
from
97d5fe9 to
23cc099
Compare
yndajas
force-pushed
the
1184-redux
branch
3 times, most recently
from
7e65ed6 to
4122286
Compare
yndajas
force-pushed
the
1184-redux
branch
2 times, most recently
from
23986bc to
598e232
Compare
yndajas
force-pushed
the
refactor-permissions-controller-tests
branch
from
84d0a0a to
6eeb875
Compare
yndajas
force-pushed
the
1184-redux
branch
4 times, most recently
from
2f09002 to
3213155
Compare
yndajas
force-pushed
the
1184-redux
branch
2 times, most recently
from
37889b1 to
66a4dd1
Compare
Gweaton
approved these changes
Aug 13, 2024
We want publishing managers to be able to manage delegatable supported permissions, rather than all permissions for apps with a delegatable signin permission At this point, publishing managers will still see all permissions when on the edit permissions pages, but the app will either error or simply ignore any requested changes to non-delegatable permissions (still updating delegatable permissions in the same manner as before). A later change will update the UI/UX to reflect the manageable permissions
We don't care about the delegability of the signin permissions here. We've already updated the `SupportedPermissionPolicy` to filter the manageable permissions by delagability. This is simply about whether a user can edit their own permissions, regardless of whether there are in-scope permissions for them to manage
We don't care about the delegability of the signin permissions here. We've already updated the `SupportedPermissionPolicy` to filter the manageable permissions by delagability. This is simply about whether a user can edit another user's permissions, regardless of whether there are in-scope permissions for them to manage
These will be used in `ApplicationTableHelper#update_permissions_link` to determine whether a user should be shown a link to edit permissions for a given app
We've already updated the `Account::ApplicationPolicy` and `Users::ApplicationPolicy`, which are called in `account_applications_permissions_links` and `users_applications_permissions_links` respectively. These will determine whether `update_permissions_link` is called at all. As before, `users_applications_permissions_links` then has responsibility for determining whether there are any permissions that the user can manage. This is updated to use the latest logic The tests are a little abstracted here. The aim is to keep the three 'happy' path conditions DRY. The method is essentially two parts: a conditional gate that decides whether to exit early with an empty string, and a further conditional expression that determines that path to use. The latter is the same regardless of how you get there. Arguably this could be a separate method for simpler testing, but it should then probably be a private one
We've already updated the policies to prevent publishing managers from adding or removing non-delegatable permissions to/from users This will: - filter out non-delegatable permissions higher up in the dependency chain (in `UserUpdatePermissionBuilder`) - filter the permissions users see when accessing the edit permissions pages The added `update` tests only cover when the current user is a publishing manager with access to the app, since if they didn't have access to the app the policy (which we stub) would return false, a condition already covered by the unauthorised test In both tests, I wanted to stub the `publishing_manager?` method rather than using either a super organisation admin or an organisation admin. For the account permissions controller test, in order for the `user.update` call in `UserUpdate` to work, I also had to stub a validator method (`organisation_admin_belongs_to_organisation`) because of the grantee being a publishing manager
If there are no permissions that the current user can manage for the given grantee, we should redirect back to the applications page and flash a message to that effect On the applications page, if there are no manageable permissions for a given app, we don't include the "Update permissions" link. However, prior to this commit, a user could manually visit the edit permissions path using their browser's URL bar. On that page, they wouldn't have seen any permissions to edit, and if they already had the page loaded, the `update` action would reject any changes to unmanageable permission. However, this commit makes it so that users will be redirected, meaning that in this edge case they won't see a confusing mostly empty edit permissions page
These were missed in 360025c when earlier tests with the same description were updated
This updates the documentation to reflect how things work after having fixed delegatable permissions I wasn't sure whether to include some additional dependencies, like `has_non_signin_permissions_grantable_from_ui?` and `has_delegatable_non_signin_permissions_grantable_from_ui?` in the `ApplicationTableHelper`, but we didn't previously include `sorted_supported_permissions_grantable_from_ui`, which they replaced The documentation is not meant to be exhaustive, but rather a good starting point at a reasonable level of depth to get a good picture of how all the relevant files contribute to the overall access and permissions picture, so I think the current level is sufficient
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
This is the main PR for fixing delegatable permissions. This is the first that will actually change how things work: publishing managers (organisation and super organisation admins) will now only be able to manage permissions marked as delegatable (for users of equal or lower level in the same organisation, or a child organisation for super organisation admins)
This application is owned by the publishing platform team. Please let us know in #govuk-publishing-platform when you raise any PRs.
Follow these steps if you are doing a Rails upgrade.