Move API user permission management to design system #2599

chrislo · 2023-12-14T14:13:47Z

This brings API User permission management into line with account permission management and user permission management.

Before

After

The comment in the `assert_has_signin_permission_for` method was incorrect. The presence of an application in the "Permissions" list is determined by `UserPermissionsControllerMethods#visible_applications`, which uses `User#authorisations` and not `User#application_permissions`. I've renamed the original method to better describe its behaviour, and I've added a new method to test that the user has the signin permission for the app. Unfortunately there's no way in the user interface to know whether an API User has the signin permission for an app so we have to rely on `User#has_access_to?`.

In preparation for refactoring this method. I realised that was no coverage around revoked access tokens.

Previously this method was returning an array of unique applications using Ruby's `group_by`. It's more idiomatic, and will allow us to chain on additional ActiveRecord query methods if we convert it to a has_many instead. This method was only being used via its alias `:applications_used` so we've changed the call site to use `:authorised_applications` instead (which we think is clearer). Co-authored-by: Chris Roos <chris.roos@gofreerange.com>

This both simplifies, and reduces the number of queries in (from 2 to 1), the `#visible_applications` method. It was made possible by our change to `User#authorised_applications` which converted it from a method to a `has_many`. Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>

This page will allow a superadmin to see which applications an API user has access to and eventually edit their permissions. We've based the view on `/users/applications/index.html.erb` but as managing access to applications is handled on the "manage tokens" page we do not need to show applications the user does not have access to here. Note it is possible for api users to have access tokens for applications that are not "api only" (e.g in production the "Content Publisher" api user has a token for "Whitehall") so we do not filter out applications based on their `api_only` field. Co-authored-by: Chris Roos <chris.roos@gofreerange.com>

floehopper · 2024-01-09T11:17:25Z

I think the breadcrumb link text could be terser, because later links are in the context of the previous links, so something like this:

Dashboard > API users > Test API User > Test API User's applications > Update Test API User's permissions for Test Application 1

Could become:

Dashboard > API users > Test API User > Applications > Update permissions for Test Application 1

I think this is more consistent with what we've done elsewhere.

test/integration/manage_api_users_test.rb

test/controllers/api_users/permissions_controller_test.rb

app/views/api_users/applications/index.html.erb

app/controllers/api_users_controller.rb

floehopper

This looks great - good work! I particularly like the change to User#authorised_applications and it feels really good to get rid of a bunch of code in ApiUsersContoller. However, while I understand why you've done it, I do have significant concerns about the amount of duplication that's been introduced. I worry that unless we do something about it now (when we're arguably in the best position to understand it all), it'll never get done.

Otherwise I've added a bunch of mostly minor comments. I think the ones I care most about are:

Only testing editing permissions for API-only apps - this might just be a misunderstanding on my part
Shortening the breadcrumb text
Moving the flash alert

I think I might've asked you about this before but would it possible to use the table component instead of using the lower-level CSS classes...?

Anyway, overall I think it looks good and I'm happy to approve it! ✅

chrisroos · 2024-01-09T14:03:24Z

I think the breadcrumb link text could be terser, because later links are in the context of the previous links, so something like this:

Dashboard > API users > Test API User > Test API User's applications > Update Test API User's permissions for Test Application 1

Could become:

Dashboard > API users > Test API User > Applications > Update permissions for Test Application 1

I think this is more consistent with what we've done elsewhere.

I agree that your suggestion is an improvement but these breadcrumbs for /api_users are consistent with /users so I think we'll leave this for now and tackle it when we address breadcrumbs site-wide in https://trello.com/c/dB0VDo9a.

We've copied the controller, template and test files from their user counterparts and modified as necessary. We've decided to add this duplication for now but will see whether we can reduce/remove it later. Note that we're using `UserUpdate` in this new controller, where we were using `User#update` in `ApiUsersController#update`[1]. Using `UserUpdate` has the following advantages: - consistency with the other controllers that modify a user's permissions (account/permissions_controller and users/permissions_controller). - record an `EventLog` about the update - record an `EventLog` about the permissions that have been added/removed [1]: https://github.com/alphagov/signon/blob/089516afdc0311525769d89c81bc837db4b16a2d/app/controllers/api_users_controller.rb#L42C25-L42C51 Co-authored-by: Chris Roos <chris.roos@gofreerange.com>

To mirror the behaviour in `ApiUsersController#manage_permissions`, which uses `ApiUsersController#visible_applications` to obtain the applications that can be managed.

We've copied the template change from views/users/applications/index.html.erb We've added a test to assert that a flash message is displayed but haven't tested the content of the message because that's being unit-tested in application_permissions_helper_test.rb. Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>

Copied the template changes and tests from users/applications.

I will remove the manage_permissions route in a later commit.

Permissions are now managed using the /api_users/<id>/applications page.

So that these flash messages appear above the page title instead of between the page title and page body. This is consistent with the manage_tokens.html.erb template. Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>

chrisroos · 2024-01-09T14:56:26Z

However, while I understand why you've done it, I do have significant concerns about the amount of duplication that's been introduced. I worry that unless we do something about it now (when we're arguably in the best position to understand it all), it'll never get done.

Agreed! I think we should address this asap now that we've got 3 places (/account, /users and /api_users) all doing very similar things.

I think I might've asked you about this before but would it possible to use the table component instead of using the lower-level CSS classes...?

Good question! In this PR we've copied existing code which isn't using the table component but I don't know whether there are any reasons that we can't use it. We'll investigate using the table component outside of this PR.

chrisroos · 2024-01-09T14:56:37Z

Thanks, @floehopper!

chrisroos · 2024-01-09T15:06:35Z

I've added the following as checklist items on this Trello card:

Improve breadcrumbs as per James's comment
Remove redundant calls to Mocha::ObjectMethods#once as per James's comment
Use Time.current instead of Time.zone.now as per James's comment
Consistently render flash messages in content_for(:custom_alerts) as per James's comment
Use the table component as per James's comment
Remove the duplication we've introduced as per James's comment

floehopper · 2024-01-09T16:06:52Z

I've added the following as checklist items on this Trello card:

Fab! 😍

chrislo force-pushed the manage-permissions-for-api-users branch 4 times, most recently from 87553f7 to 89a2841 Compare December 18, 2023 14:53

chrisroos force-pushed the manage-permissions-for-api-users branch 18 times, most recently from 9e3b5c6 to 20fdd9d Compare January 9, 2024 10:54

chrisroos changed the title ~~Manage permissions for api users~~ Improve permission management for API users Jan 9, 2024

chrisroos changed the title ~~Improve permission management for API users~~ Move API user permission management to design system Jan 9, 2024

chrisroos marked this pull request as ready for review January 9, 2024 11:08

chrisroos and others added 5 commits January 9, 2024 11:08

Improve test coverage of ApiUsersController#visible_applications

e3e3b20

In preparation for refactoring this method. I realised that was no coverage around revoked access tokens.

chrisroos requested a review from floehopper January 9, 2024 11:09

floehopper self-assigned this Jan 9, 2024