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.

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
merged 8 commits into from
Sep 28, 2023

Conversation

chrisroos
Copy link
Contributor

@chrisroos chrisroos commented Sep 21, 2023

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:

  • 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

Screenshot

image

@chrisroos chrisroos force-pushed the allow-publishing-managers-to-manage-their-apps branch from 8acf20b to 65fd50a Compare September 21, 2023 13:34
@chrisroos chrisroos force-pushed the allow-govuk-admins-to-manage-their-access-to-apps branch 9 times, most recently from 99a3927 to 32826fe Compare September 26, 2023 12:41
Base automatically changed from allow-govuk-admins-to-manage-their-access-to-apps to main September 26, 2023 13:53
@chrisroos chrisroos force-pushed the allow-publishing-managers-to-manage-their-apps branch 5 times, most recently from ecb77f7 to f1c4cfd Compare September 26, 2023 16:18
@chrisroos chrisroos marked this pull request as ready for review September 26, 2023 16:18
@chrislo chrislo self-assigned this Sep 28, 2023
Copy link
Contributor

@chrislo chrislo left a 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 chrisroos force-pushed the allow-publishing-managers-to-manage-their-apps branch from f1c4cfd to 036a2ee Compare September 28, 2023 11:10
@chrisroos
Copy link
Contributor Author

Thanks, @chrislo 👍

@chrisroos chrisroos merged commit 243f9eb into main Sep 28, 2023
7 checks passed
@chrisroos 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants