-
Notifications
You must be signed in to change notification settings - Fork 35
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.
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
Allow Publishing Managers to manage their apps #2370
Merged
chrisroos
merged 8 commits into
main
from
allow-publishing-managers-to-manage-their-apps
Sep 28, 2023
Merged
Allow Publishing Managers to manage their apps #2370
chrisroos
merged 8 commits into
main
from
allow-publishing-managers-to-manage-their-apps
Sep 28, 2023
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
chrisroos
force-pushed
the
allow-publishing-managers-to-manage-their-apps
branch
from
September 21, 2023 13:34
8acf20b
to
65fd50a
Compare
chrisroos
force-pushed
the
allow-govuk-admins-to-manage-their-access-to-apps
branch
9 times, most recently
from
September 26, 2023 12:41
99a3927
to
32826fe
Compare
Base automatically changed from
allow-govuk-admins-to-manage-their-access-to-apps
to
main
September 26, 2023 13:53
chrisroos
force-pushed
the
allow-publishing-managers-to-manage-their-apps
branch
5 times, most recently
from
September 26, 2023 16:18
ecb77f7
to
f1c4cfd
Compare
chrislo
approved these changes
Sep 28, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me @chrisroos!
If the user attempts to do something they're not authorized to do then we should first try to take them back to the page they were on, and only redirect to the root path as a fallback. Ideally users wouldn't be able to use the UI to navigate to actions they're not authorized to execute but this small change will make the experience slightly better if they are able to.
Calum spotted that I'd missed this.
To avoid the column widths varying based on the length of text in the (permission) Name column.
In order to allow Publishing Managers to remove their own signin permission from apps I'm going to need an instance of the Application so that I can check whether it has delegatable permissions. This preparatory change will allow me to pass an instance of Application to `authorize` in order to automagically find this `Account::ApplicationPolicy` class.
In preparation for allowing Publishing Managers to use the /account/applications page. Publishing Managers can only remove their signin permission from an application if the application has delegatable permissions, so we need an instance of Application to check whether the user is authorized to remove their access. I've chosen to move all permission related methods over to keep them together.
Publishing Managers can: - View permissions for all applications they have access to - Remove their access from applications with delegatable permissions Publishing Managers cannot: - Grant themselves access to applications - Remove their access from applications that don't have delegatable permissions
Publishing Managers aren't allowed to grant themselves access to applications so we shouldn't show them this button.
Publishing Managers can only remove their access from applications that have delegatable permissions. We should only display the button if they're allowed to remove their access.
chrisroos
force-pushed
the
allow-publishing-managers-to-manage-their-apps
branch
from
September 28, 2023 11:10
f1c4cfd
to
036a2ee
Compare
Thanks, @chrislo 👍 |
chrisroos
deleted the
allow-publishing-managers-to-manage-their-apps
branch
September 28, 2023 11:19
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.
https://trello.com/c/7Hfe7eu0
This builds on PR #2355 to enable Publishing Managers (users with a role of "super_organisation_admin" or "organisation_admin") to access /account/applications.
Publishing Managers can:
Publishing Managers cannot:
Screenshot