Hide retired apps on most pages #2446

floehopper · 2023-10-18T15:25:16Z

While the Trello card is about hiding API-only apps, the overall motivation is to reduce the number of apps on various permissions pages and so I thought a good first step would be to exclude permissions relating to retired apps.

This adds a condition to the default scope on the Doorkeeper::Application model to only include not_retired applications in any query by default. It also incorporates this default scope into the has_many :application_permissions & has_many :supported_permissions associations on the User model. While using a default scope is perhaps a bit controversial, I think the notion of retiring an application is effectively a "soft delete" and this seems like a good use case for a default scope, not least because it should make it less likely a retired app or its permissions show up on any pages added in the future.

All this should mean that retired apps will only be listed in the following places:

On the "Retired" tab on the apps index page. It should still be possible to view all the permissions supported by a retired application via this page.
On the API user edit page in the "Tokens" section. This is because I haven't (yet) incorporated the default scope on the Doorkeeper::Application model to the has_many :authorisations association on the User model. I've decided to hold off on that for now (a) to reduce the scope of this PR; and (b) because 2nd line might still get alerts for expired tokens associated with retired apps and I want them to be able to still be able to access the UI to revoke/regenerate the token.

Retired apps should no longer appear on any pages where a user's permissions are being added/removed/changed.

The most significant commits are:

The other commits are mostly adding test coverage, removing redundant code, or just generally tidying up.

I haven't yet removed the logic to wrap application names in <del> tags to strike-through the name, because I want to double-check no retired apps end up appearing anywhere in the integration or production environments before I make that change.

Joseph has recommended that we publicise this change in the #govuk-publishing Slack channel before we deploy it.

chrislo

This all looks good to me @floehopper - thanks for the detailed commit messages and easy-to-follow history. I've made one small suggestion that shouldn't block merging this.

test/factories/oauth_applications.rb

This column was already defaulting to FALSE and I can't see any reason why we'd want to allow NULL values; they'd just confuse matters. This migration replaces any NULL values with FALSE and prevents the column from ever being set to NULL in the future.

We already have a `not_retired` scope and there's at least a couple of places where using a `retired` scope would be an improvement.

Applications are listed in two tabs on the apps index page. This checks that non-retired apps are displayed in the "active" tab and retired apps are displayed in the "retired" tab. I'm planning to make a change in this area and I wanted to ensure there was some test coverage in place.

I'm planning to make a change in this area and I wanted to ensure there was some test coverage in place.

The "scopes" context wasn't adding much and not all tests for scopes were inside it anyway! This diff is best viewed with the --ignore-all-space option, because of the indentation changes.

I'm about to add a default scope which will make use of the not_retired scope. I think re-ordering the test contexts like this will mean the Doorkeeper::ApplicationTest makes more sense.

In `Doorkeeper::Application` class, c.f. `Doorkeeper::AccessToken` & `Doorkeeper::AccessGrant`. This is a bit orthogonal to the other changes in this PR, but I'm making changes in this area of the code and I was finding the verbose comments distracting!

To make it clearer that this controller has an `edit` action which renders the view containing the form for updating an application.

I'm about to add a default scope for `Doorkeeper::Application` which will mean that retired applications are not accessible from the UI by default. I think this is one of the places in the app where we *do* still want to have access to retired apps, so I'm adding this test coverage in advance of adding the default scope to make sure I don't break anything.

I'm about to make some changes in this area and I think doing this first will make it easier to see what's going on.

Since the application factory sets a name by default and the name isn't referenced in the rest of the test, there's no need to set it here and I was finding it distracting when reading the tests.

I'm planning to add a default scope to `Doorkeeper::Application` which will make retired apps unavailable by default. I suspect that will change the behaviour captured in this new test and so having the test in place first will make it clearer how the behaviour has changed in the subsequent commit(s).

Retiring an app is effectively a "soft delete" and so retired apps should only appear in a very few pages in the UI, i.e. pages where the user explicitly wants to view retired apps and pages where the user want to edit a retired app, e.g. to un-retired it. My motivation for doing this is from this Trello card [1] where we want to reduce the number of apps displayed on various pages to only include the useful ones. That card is actually about hiding API-only apps, but it makes sense to me to hide retired apps from most pages as a first step. Changing the default scope like this broke the `User#grant_application_permissions` method, because `User#grant_permission` ended up trying to create a `UserApplicationPermission`, but failed because the application was `nil`. I've chosen to "fix" this method by changing its behaviour so it no longer grants permissions for a *retired* app. I did consider raising an exception if the app was retired, but in the end I decided that was a bit drastic. I've made use of `Doorkeeper::Application.unscoped` in the `DoorkeeperApplicationsController` and its view templates to ensure that the user can still view and edit retired apps. It's possible that this commit will break some untested behaviour. However, we should find out about that relatively quickly and I think the benefits outweigh the risks. Note that I've had to tweak the `after :create` hook in the `application` factory to avoid a validation error in a test. I _think_ this is because the application in the test is retired and when the hook is executed `app` doesn't have an `id` which somehow results in the relatively recently added presence validation for `Doorkeeper::Application#application` failing. I only actually needed to change the code in one of the loops within the hook to fix the test, but it makes sense to me that I should change all of them at the same time. I haven't investigated this very thoroughly, because (a) I think the new version of the code is more idiomatic Rails; and (b) I really dislike these FactoryBot hooks and I hope to get rid of them sometime soon! [1]: https://trello.com/c/kvmb5OHO

Now that `not_retired` is included in the default scope, there's no need to repeat it here. I'm confident that this scope has sufficient test coverage in `Doorkeeper::ApplicationTest` to give me confidence that this should not have broken anything.

Now that `not_retired` is included in the default scope, there's no need to repeat it here. I'm confident that this scope has sufficient test coverage in `DoorkeeperApplicationsControllerTest` to give me confidence that this should not have broken anything.

Now that `not_retired` is included in the default scope, there's no need to repeat it here. I'm confident that this scope has sufficient test coverage in `BatchInvitationPermissionsControllerTest` to give me confidence that this should not have broken anything.

Now that `not_retired` is included in the default scope, there's no need to repeat it here. I'm confident that this scope has sufficient test coverage in `AccountApplicationsTest` to give me confidence that this should not have broken anything.

Now that `not_retired` is included in the default scope, there's no need to repeat it here. I'm confident that this scope has sufficient test coverage in `Account::PermissionsControllerTest` to give me confidence that this should not have broken anything.

To reduce duplication.

Now that `not_retired` is included in the default scope, there's no need to repeat it here. I'm confident that this scope has sufficient test coverage in `Account::SigninPermissionsControllerTest` to give me confidence that this should not have broken anything.

Now that `not_retired` is included in the default scope, there's no need to repeat it here. I'm confident that this scope has sufficient test coverage in `InvitationsControllerTest` to give me confidence that this should not have broken anything.

I'm about to make a change to this, so I wanted to add some test coverage first.

I'm about to make some changes to `User#application_permissions` which will introduce a join onto the `oauth_applications` table which also has a `name` column thus making this change necessary.

We don't want to display a user's permissions for retired apps on any page and so to ensure that I've introduced this `joins(:application)` scope to the `User#application_permissions` `has_many` association so that it picks up the default scope on `Doorkeeper::Application`, i.e. `not_retired`. I don't think the `joins` scope itself will change which `UserApplicationPermission` records are returned, because `user_application_permissions.application_id` has a `NOT NULL` and a foreign key constraint and `UserApplicationPermission#application` has a `presence` validation on it. The `joins` scope just serves to include the default scope on `Doorkeeper::Application` in the query.

I'm about to make a change to this, so I wanted to add some test coverage first.

We don't want to display a user's permissions for retired apps on any page and so to ensure that I've introduced this `joins(:application)` scope to the `User#supported_permissions` `has_many` association so that it picks up the default scope on `Doorkeeper::Application`, i.e. `not_retired`. I don't think the `joins` scope itself will change which `SupportedPermission` records are returned, because `supported_permissions.application_id` has a `NOT NULL` and a foreign key constraint and `SupportedPermission#application` has a `presence` validation on it. The `joins` scope just serves to include the default scope on `Doorkeeper::Application` in the query.

Make it clear that retiring an application will hide it from most pages; not just the dashboard.

floehopper · 2023-10-19T09:46:45Z

This all looks good to me @floehopper - thanks for the detailed commit messages and easy-to-follow history. I've made one small suggestion that shouldn't block merging this.

Thanks, @chrislo!

This was accidentally broken in this commit [1] in #2446, because the `ordered_by_name` scope is part of the default scope, but we are removing that with the call to `unscoped`. [1]: 9a9cd02

The behaviour being tested was actually introduced when we added the `not_retired` scope to the default scope for `Doorkeeper::Application` in this commit [1] in #2446. However, I'm about to make some changes in this area and I want to have some coverage in place beforehand. [1]: 9a9cd02

The behaviour of this rake task was changed when we added `not_retired` to the default scope on `Doorkeeper::Application` in this commit [1] in #2446. Since then it has no longer synced app secrets for retired apps; instead it will raise an exception. This behaviour makes sense to me from a security point-of-view and I have confirmed that (even after the recent spate of app retirements) the Signon app still includes all the apps listed in `charts/app-config/templates/signon-secrets-sync-configmap.yaml` [2] which are used in the `sync-app-secrets-to-k8s` cron task e.g. in integration [3]. This means that despite the change in behaviour we should *not* see any exceptions. And if we do start seeing exceptions that probably means the list of apps in `signon-secrets-sync-configmap.yaml` needs to be updated. [1]: 9a9cd02 [2]: https://github.com/alphagov/govuk-helm-charts/blob/4c540acfa83899f0d73f9b06d480c712045cfc5f/charts/app-config/templates/signon-secrets-sync-configmap.yaml#L6-L42 [3]: https://github.com/alphagov/govuk-helm-charts/blob/4c540acfa83899f0d73f9b06d480c712045cfc5f/charts/app-config/values-integration.yaml#L2384-L2387

This covers the scenario where an app is retired *after* the job is enqueued but *before* it is executed. The behaviour being tested was actually introduced when we added the `not_retired` scope to the default scope for `Doorkeeper::Application` in this commit [1] in #2446. This is because both jobs call `Doorkeeper::Application.find_by` in their `#perform` methods which will include the default scope and so if the app is retired will return `nil` and be caught by the guard condition. [1]: 9a9cd02

The behaviour of this rake task was changed when we added `not_retired` to the default scope on `Doorkeeper::Application` in this commit [1] in #2446. Since then it has no longer synced app secrets for retired apps; instead it will raise an exception. This behaviour makes sense to me from a security point-of-view and I have confirmed that (even after the recent spate of app retirements) the Signon app still includes all the apps listed in `charts/app-config/templates/signon-secrets-sync-configmap.yaml` [2] which are used in the `sync-app-secrets-to-k8s` cron task e.g. in integration [3]. This means that despite the change in behaviour we should *not* see any exceptions. And if we do start seeing exceptions that probably means the list of apps in `signon-secrets-sync-configmap.yaml` needs to be updated. [1]: 9a9cd02 [2]: https://github.com/alphagov/govuk-helm-charts/blob/4c540acfa83899f0d73f9b06d480c712045cfc5f/charts/app-config/templates/signon-secrets-sync-configmap.yaml#L6-L42 [3]: https://github.com/alphagov/govuk-helm-charts/blob/4c540acfa83899f0d73f9b06d480c712045cfc5f/charts/app-config/values-integration.yaml#L2384-L2387

This covers the scenario where an app is retired *after* the job is enqueued but *before* it is executed. The behaviour being tested was actually introduced when we added the `not_retired` scope to the default scope for `Doorkeeper::Application` in this commit [1] in #2446. This is because both jobs call `Doorkeeper::Application.find_by` in their `#perform` methods which will include the default scope and so if the app is retired will return `nil` and be caught by the guard condition. [1]: 9a9cd02

The behaviour being tested was actually introduced when we added the `not_retired` scope to the default scope for `Doorkeeper::Application` in this commit [1] in #2446. However, I'm about to make some changes in this area and I want to have some coverage in place beforehand. [1]: 9a9cd02

floehopper requested review from chrisroos and chrislo October 18, 2023 15:31

chrislo self-assigned this Oct 18, 2023

floehopper force-pushed the hide-retired-apps-on-most-pages branch from 41aab71 to 4946651 Compare October 19, 2023 08:21

chrislo approved these changes Oct 19, 2023

View reviewed changes

test/factories/oauth_applications.rb Show resolved Hide resolved

floehopper added 25 commits October 19, 2023 10:46

Add Application.retired scope

908d0a9

We already have a `not_retired` scope and there's at least a couple of places where using a `retired` scope would be an improvement.

Use scopes vs predicate methods on apps index page

4adf7cb

Add test for retired apps for new invitation

4212490

I'm planning to make a change in this area and I wanted to ensure there was some test coverage in place.

Use scope vs predicate method on new invitation page

77b382a

Add test for retired apps for new batch invitation

819400f

I'm planning to make a change in this area and I wanted to ensure there was some test coverage in place.

Use scope vs predicate method on new batch invitation permissions page

caecb3f

Flatten contexts in Doorkeeper::ApplicationTest

4010730

The "scopes" context wasn't adding much and not all tests for scopes were inside it anyway! This diff is best viewed with the --ignore-all-space option, because of the indentation changes.

Move tests for retired/not_retired application scopes

8486632

I'm about to add a default scope which will make use of the not_retired scope. I think re-ordering the test contexts like this will mean the Doorkeeper::ApplicationTest makes more sense.

Blank lines to distinguish default scopes from other scopes

66925c3

Add empty DoorkeeperApplicationsController#edit method

b4c0999

To make it clearer that this controller has an `edit` action which renders the view containing the form for updating an application.

Group tests for granting permissions to a user

545be34

I'm about to make some changes in this area and I think doing this first will make it easier to see what's going on.

Remove redundant app name from some user tests

c2fad7c

Since the application factory sets a name by default and the name isn't referenced in the rest of the test, there's no need to set it here and I was finding it distracting when reading the tests.

Extract Account::SigninPermissionsController#application

5fa7286

To reduce duplication.

floehopper added 7 commits October 19, 2023 10:46

Add test for User#application_permissions association

7f23aeb

I'm about to make a change to this, so I wanted to add some test coverage first.

Disambiguate name column in User#permissions_for

ab6d5af

I'm about to make some changes to `User#application_permissions` which will introduce a join onto the `oauth_applications` table which also has a `name` column thus making this change necessary.

Add test for User#supported_permissions association

42eca69

I'm about to make a change to this, so I wanted to add some test coverage first.

Tweak hint text for retiring an application

2bb738c

Make it clear that retiring an application will hide it from most pages; not just the dashboard.

floehopper force-pushed the hide-retired-apps-on-most-pages branch from 4946651 to 2bb738c Compare October 19, 2023 09:46

floehopper merged commit b8adffc into main Oct 19, 2023
6 checks passed

floehopper deleted the hide-retired-apps-on-most-pages branch October 19, 2023 10:04

floehopper mentioned this pull request Oct 19, 2023

Hide API-only apps on most pages #2452

Merged

floehopper mentioned this pull request Oct 24, 2023

Hide access tokens for retired apps #2458

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hide retired apps on most pages #2446

Hide retired apps on most pages #2446

floehopper commented Oct 18, 2023 •

edited

Loading

chrislo left a comment

floehopper commented Oct 19, 2023

Hide retired apps on most pages #2446

Hide retired apps on most pages #2446

Conversation

floehopper commented Oct 18, 2023 • edited Loading

chrislo left a comment

Choose a reason for hiding this comment

floehopper commented Oct 19, 2023

floehopper commented Oct 18, 2023 •

edited

Loading