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

Allow GOV.UK Admins to manage their permissions in their account page #2355

Merged
merged 5 commits into from
Sep 26, 2023
Merged

Allow GOV.UK Admins to manage their permissions in their account page #2355

merged 5 commits into from
Sep 26, 2023

Conversation

chrisroos
Copy link
Contributor

@chrisroos chrisroos commented Sep 11, 2023 •
edited
Loading

https://trello.com/c/7Hfe7eu0

This PR adds functionality that allows GOV.UK Admins (users with role of "superadmin" or "admin") to manage their access to applications and to view their permissions for those applications that they have access to.

Routes added

  • /account/applications/<id>
    • redirects to /account/applications as we don't have anything to show on this page at present
  • /account/applications/<id>/permissions
    • lists the permissions the user has for the application
    • only accessible to GOV.UK Admins that have the signin permission for the application
  • /account/applications/<id>/signin_permission
    • used to create/destroy the current user's signin permission for the application
    • only accessible to GOV.UK Admins
  • /account/applications/<id>/signin_permission/delete
    • used to confirm that the current user intends to remove their signin permission for the application
    • only access to GOV.UK Admins with the signin permission for the application

Screenshots

/account/applications

Signon - My Account - Applications

/account/applications/<id>/signin_permission/delete

Signon - My Account - Remove access to application

/account/applications/<id>/permissions

Signon - My Account - Application permissions

@chrisroos chrisroos force-pushed the allow-govuk-admins-to-manage-their-access-to-apps branch 13 times, most recently from 8a2897d to f210537 Compare September 20, 2023 16:17
@chrisroos chrisroos changed the title WIP: Add ability for (super)admins to give themselves access to apps WIP: Allow GOV.UK Admins to manage their permissions in their account page Sep 20, 2023
@chrisroos chrisroos force-pushed the allow-govuk-admins-to-manage-their-access-to-apps branch 5 times, most recently from de12cf6 to 267beca Compare September 21, 2023 10:40
@chrisroos chrisroos changed the title WIP: Allow GOV.UK Admins to manage their permissions in their account page Allow GOV.UK Admins to manage their permissions in their account page Sep 21, 2023
@chrisroos chrisroos marked this pull request as ready for review September 21, 2023 10:49
@floehopper floehopper self-requested a review September 21, 2023 11:29
floehopper
floehopper approved these changes Sep 21, 2023
View reviewed changes
Copy link
Contributor

@floehopper floehopper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks great to me - good work! 🎉

I've added quite a few minor/subjective comments which I'm happy for you to address or not as you see fit.

I think the only one that should block merging is what I think are a couple of copy/paste errors in test/policies/account_applications_policy_test.rb for the show? method.

app/views/account/applications/index.html.erb Show resolved Hide resolved
app/views/account/applications/index.html.erb Outdated Show resolved Hide resolved
app/views/account/applications/index.html.erb Show resolved Hide resolved
test/policies/account_applications_policy_test.rb Show resolved Hide resolved
test/policies/account_applications_policy_test.rb Outdated Show resolved Hide resolved
app/helpers/batch_invitation_permissions_helper.rb Show resolved Hide resolved
app/controllers/account/permissions_controller.rb Outdated Show resolved Hide resolved
app/models/user.rb Outdated Show resolved Hide resolved
app/views/account/applications/index.html.erb Outdated Show resolved Hide resolved
app/policies/account_applications_policy.rb Outdated Show resolved Hide resolved
@chrisroos
Copy link
Contributor Author

Thanks for reviewing, @floehopper 👍

I'm working through your comments now.

@chrisroos chrisroos force-pushed the allow-govuk-admins-to-manage-their-access-to-apps branch 6 times, most recently from c1d2887 to a370877 Compare September 25, 2023 14:18
@chrisroos chrisroos force-pushed the allow-govuk-admins-to-manage-their-access-to-apps branch 2 times, most recently from f44b8d3 to 8cec3ff Compare September 26, 2023 09:51
@chrisroos
Allow GOV.UK Admins to give themselves access to apps
cd35197 
This adds a "Grant access" button next to each app in the list of "Apps
you don't have access to" on /account/applications. This page is
currently only available to GOV.UK Admins and this functionality
replicates what they're already able to do on their /users/<id>/edit page.

I experimented with using the button component for the "Grant access"
button but couldn't work out how to pass the visually hidden span into
it.

I considered using a more generic `PermissionsController` (instead of
`SigninPermissionsController`) given that the signin permission is
modelled the same as all other permissions, and that we're going to
allow users to manage other permissions in future. I decided against it
for now because the signin permission feels conceptually different to
the other permissions. We can revisit this decision in future if it
turns out there's a better approach.
@chrisroos
Add route for /account/applications/<id>
dcf2745 
Although not strictly necessary I've chosen to add a route for
`/account/applications/<id>` so that we have hackable URLs. This route
currently redirects to `/account/applications` but we might want to
present something at this location in future.
@chrisroos chrisroos force-pushed the allow-govuk-admins-to-manage-their-access-to-apps branch from 8990e8c to 99a3927 Compare September 26, 2023 11:43
@chrisroos
Allow GOV.UK Admins to remove their access to app
9444243 
Users are required to confirm the removal of access to avoid the problem
that exists in the current `/users/<id>/edit` page where Publishing
Managers can accidentally remove their access to an application but then
have to open a support request to get that access back again. NOTE.  The
functionality in this PR is currently restricted to GOV.UK Admins but
will be made available to Publishing Managers soon.

I experimented with using the button component for the "Remove access"
button but couldn't work out how to pass the visually hidden span into
it.
@chrisroos
Move permissions_for to PermissionsHelper
a138f22 
In preparation for using this helper in the /account pages.

There's nothing specific to batch invitations in this method and moving
it won't break any existing functionality because we currently include
all helpers into every view.
@chrisroos
Allow GOV.UK Admins to view their app permissions
32826fe 
This adds the `/account/applications/<id>/permissions` page which lists
all app permissions and whether the current user has that permission or
not.

This new page is currently only accessible to GOV.UK Admins that have
the signin permission to the app.

The list of permissions has "signin" first, followed by:

- other permissions the user has been granted; ordered alphabetically
- other permissions the user has not been granted; ordered alphabetically
@chrisroos chrisroos force-pushed the allow-govuk-admins-to-manage-their-access-to-apps branch from 99a3927 to 32826fe Compare September 26, 2023 12:41
@floehopper floehopper self-requested a review September 26, 2023 13:05
@floehopper floehopper self-assigned this Sep 26, 2023
floehopper
floehopper approved these changes Sep 26, 2023
View reviewed changes
Copy link
Contributor

@floehopper floehopper 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 - well done for powering through! 😄

@chrisroos
Copy link
Contributor Author

Thanks, @floehopper!

@chrisroos chrisroos merged commit 8239500 into main Sep 26, 2023
6 checks passed
@chrisroos chrisroos deleted the allow-govuk-admins-to-manage-their-access-to-apps branch September 26, 2023 13:53
@chrisroos chrisroos mentioned this pull request Sep 26, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@floehopper floehopper floehopper approved these changes

Assignees

@floehopper floehopper

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chrisroos @floehopper