From 95f477a1cdcfb47fd14008b91275b8dda2647621 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 10:28:44 +0100 Subject: [PATCH 01/32] Change oauth_applications.retired column to NOT NULL 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. --- ..._add_not_null_constraint_to_oauth_applications_retired.rb | 5 +++++ db/schema.rb | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20231018141956_add_not_null_constraint_to_oauth_applications_retired.rb diff --git a/db/migrate/20231018141956_add_not_null_constraint_to_oauth_applications_retired.rb b/db/migrate/20231018141956_add_not_null_constraint_to_oauth_applications_retired.rb new file mode 100644 index 000000000..625474cab --- /dev/null +++ b/db/migrate/20231018141956_add_not_null_constraint_to_oauth_applications_retired.rb @@ -0,0 +1,5 @@ +class AddNotNullConstraintToOauthApplicationsRetired < ActiveRecord::Migration[7.0] + def change + change_column_null :oauth_applications, :retired, false, false + end +end diff --git a/db/schema.rb b/db/schema.rb index b127d2538..ddef01575 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_10_17_153357) do +ActiveRecord::Schema[7.0].define(version: 2023_10_18_141956) do create_table "batch_invitation_application_permissions", id: :integer, charset: "utf8mb3", force: :cascade do |t| t.integer "batch_invitation_id", null: false t.integer "supported_permission_id", null: false @@ -94,7 +94,7 @@ t.string "home_uri" t.string "description" t.boolean "supports_push_updates", default: true - t.boolean "retired", default: false + t.boolean "retired", default: false, null: false t.boolean "show_on_dashboard", default: true, null: false t.boolean "confidential", default: true, null: false t.index ["name"], name: "unique_application_name", unique: true From 908d0a9da4cb6c67f200bdcd262cd386901967f5 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 10:50:22 +0100 Subject: [PATCH 02/32] Add Application.retired scope 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. --- app/models/doorkeeper/application.rb | 1 + test/models/doorkeeper/application_test.rb | 16 ++++++++++++++++ 2 files changed, 17 insertions(+) diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index d377b8b7b..e299161e5 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -8,6 +8,7 @@ class Doorkeeper::Application < ActiveRecord::Base default_scope { ordered_by_name } scope :ordered_by_name, -> { order("oauth_applications.name") } scope :support_push_updates, -> { where(supports_push_updates: true) } + scope :retired, -> { where(retired: true) } scope :not_retired, -> { where(retired: false) } scope :can_signin, lambda { |user| diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index a2417f8c0..277632407 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -180,6 +180,22 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase assert_empty Doorkeeper::Application.with_signin_delegatable end + context ".retired" do + setup do + @app = create(:application) + end + + should "include apps that have been retired" do + @app.update!(retired: true) + assert_equal [@app], Doorkeeper::Application.retired + end + + should "exclude apps that have not been retired" do + @app.update!(retired: false) + assert_equal [], Doorkeeper::Application.retired + end + end + context ".not_retired" do setup do @app = create(:application) From 5ba4f9f722f789af6457f45599f739076eb37052 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 10:52:04 +0100 Subject: [PATCH 03/32] Assert retired/non_retired apps are in relevant tab 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. --- test/controllers/doorkeeper_applications_controller_test.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/controllers/doorkeeper_applications_controller_test.rb b/test/controllers/doorkeeper_applications_controller_test.rb index 8b4f51ddc..b70e2d5b1 100644 --- a/test/controllers/doorkeeper_applications_controller_test.rb +++ b/test/controllers/doorkeeper_applications_controller_test.rb @@ -9,8 +9,12 @@ class DoorkeeperApplicationsControllerTest < ActionController::TestCase context "GET index" do should "list applications" do create(:application, name: "My first app") + create(:application, name: "My retired app", retired: true) + get :index - assert_select "td", /My first app/ + + assert_select "#active td", /My first app/ + assert_select "#retired td", /My retired app/ end end From 4adf7cb3d060b4a0f8838608dfc586b821780566 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 11:25:58 +0100 Subject: [PATCH 04/32] Use scopes vs predicate methods on apps index page --- app/views/doorkeeper_applications/index.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/doorkeeper_applications/index.html.erb b/app/views/doorkeeper_applications/index.html.erb index ad3d907c9..e346665ee 100644 --- a/app/views/doorkeeper_applications/index.html.erb +++ b/app/views/doorkeeper_applications/index.html.erb @@ -5,12 +5,12 @@ { id: "active", label: "Active applications", - content: render("application_list", applications: @applications.reject(&:retired?)) + content: render("application_list", applications: @applications.not_retired) }, { id: "retired", label: "Retired applications", - content: render("application_list", applications: @applications.select(&:retired?)) + content: render("application_list", applications: @applications.retired) } ] } %> From 4212490924b4bb09c36c63fab295a5de5e71f2af Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 10:57:45 +0100 Subject: [PATCH 05/32] Add test for retired apps for new invitation I'm planning to make a change in this area and I wanted to ensure there was some test coverage in place. --- test/controllers/invitations_controller_test.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/controllers/invitations_controller_test.rb b/test/controllers/invitations_controller_test.rb index b0397e240..eea335010 100644 --- a/test/controllers/invitations_controller_test.rb +++ b/test/controllers/invitations_controller_test.rb @@ -83,6 +83,17 @@ class InvitationsControllerTest < ActionController::TestCase assert_select ".gem-c-option-select[data-filter-element]" end end + + should "not render form checkbox inputs for permissions for retired applications" do + application = create(:application, retired: true) + signin_permission = application.signin_permission + + get :new + + assert_select "form" do + assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{signin_permission.to_param}']", count: 0 + end + end end context "when inviter is signed in as a superadmin" do From 77b382a38c91e6a572f62efd12910c3e861bfe05 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 11:26:53 +0100 Subject: [PATCH 06/32] Use scope vs predicate method on new invitation page --- app/views/devise/invitations/new.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index 503cb935f..e887c86bf 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -86,7 +86,7 @@ heading_size: "l", } do %> - <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> + <% policy_scope(:user_permission_manageable_application).not_retired.map do |application| %> <% options = options_for_permission_option_select(application:, user: f.object) %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, From 819400f51ef4a68e279519a8bec3d0670c3544fe Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 10:57:45 +0100 Subject: [PATCH 07/32] Add test for retired apps for new batch invitation I'm planning to make a change in this area and I wanted to ensure there was some test coverage in place. --- .../batch_invitation_permissions_controller_test.rb | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/test/controllers/batch_invitation_permissions_controller_test.rb b/test/controllers/batch_invitation_permissions_controller_test.rb index d0a6c7fe1..b9bd3c01d 100644 --- a/test/controllers/batch_invitation_permissions_controller_test.rb +++ b/test/controllers/batch_invitation_permissions_controller_test.rb @@ -77,6 +77,17 @@ class BatchInvitationPermissionsControllerTest < ActionController::TestCase assert_select "input[type='checkbox'][checked='checked'][name='user[supported_permission_ids][]'][value='#{permission.to_param}']" end end + + should "not include permissions for retired apps" do + application = create(:application, retired: true) + signin_permission = application.signin_permission + + get :new, params: { batch_invitation_id: @batch_invitation.id } + + assert_select "form" do + assert_select "input[type='checkbox'][name='user[supported_permission_ids][]'][value='#{signin_permission.to_param}']", count: 0 + end + end end context "POST create" do From caecb3f0a407762aa8c274ac0ece1113f4ebcdaa Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 11:26:53 +0100 Subject: [PATCH 08/32] Use scope vs predicate method on new batch invitation permissions page --- app/views/batch_invitation_permissions/new.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index e7c7e0fef..2e9e90183 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -19,7 +19,7 @@ %> <%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> - <% policy_scope(:user_permission_manageable_application).reject(&:retired?).map do |application| %> + <% policy_scope(:user_permission_manageable_application).not_retired.map do |application| %> <% options = options_for_permission_option_select(application:, user: User.with_default_permissions) %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, From 4010730067cb5eb1a8dd42e17407984754b52f54 Mon Sep 17 00:00:00 2001 From: James Mead Date: Mon, 16 Oct 2023 11:58:30 +0100 Subject: [PATCH 09/32] Flatten contexts in Doorkeeper::ApplicationTest 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. --- test/models/doorkeeper/application_test.rb | 114 +++++++++++---------- 1 file changed, 58 insertions(+), 56 deletions(-) diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index 277632407..9a1476718 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -144,7 +144,7 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase end end - context "scopes" do + context ".can_signin" do should "return applications that the user can signin into" do user = create(:user) application = create(:application) @@ -167,7 +167,9 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase assert_empty Doorkeeper::Application.can_signin(user) end + end + context ".with_signin_delegatable" do should "return applications that support delegation of signin permission" do application = create(:application, with_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME]) @@ -179,83 +181,83 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase assert_empty Doorkeeper::Application.with_signin_delegatable end + end - context ".retired" do - setup do - @app = create(:application) - end + context ".retired" do + setup do + @app = create(:application) + end - should "include apps that have been retired" do - @app.update!(retired: true) - assert_equal [@app], Doorkeeper::Application.retired - end + should "include apps that have been retired" do + @app.update!(retired: true) + assert_equal [@app], Doorkeeper::Application.retired + end - should "exclude apps that have not been retired" do - @app.update!(retired: false) - assert_equal [], Doorkeeper::Application.retired - end + should "exclude apps that have not been retired" do + @app.update!(retired: false) + assert_equal [], Doorkeeper::Application.retired end + end - context ".not_retired" do - setup do - @app = create(:application) - end + context ".not_retired" do + setup do + @app = create(:application) + end - should "include apps that have not been retired" do - @app.update!(retired: false) - assert_equal [@app], Doorkeeper::Application.not_retired - end + should "include apps that have not been retired" do + @app.update!(retired: false) + assert_equal [@app], Doorkeeper::Application.not_retired + end - should "exclude apps that have been retired" do - @app.update!(retired: true) - assert_equal [], Doorkeeper::Application.not_retired - end + should "exclude apps that have been retired" do + @app.update!(retired: true) + assert_equal [], Doorkeeper::Application.not_retired end + end - context ".with_signin_permission_for" do - setup do - @user = create(:user) - @app = create(:application) - end + context ".with_signin_permission_for" do + setup do + @user = create(:user) + @app = create(:application) + end - should "include applications the user has the signin permission for" do - @user.grant_application_signin_permission(@app) + should "include applications the user has the signin permission for" do + @user.grant_application_signin_permission(@app) - assert_equal [@app], Doorkeeper::Application.with_signin_permission_for(@user) - end + assert_equal [@app], Doorkeeper::Application.with_signin_permission_for(@user) + end - should "exclude applications the user does not have the signin permission for" do - create(:supported_permission, application: @app, name: "not-signin") + should "exclude applications the user does not have the signin permission for" do + create(:supported_permission, application: @app, name: "not-signin") - @user.grant_application_permission(@app, %w[not-signin]) + @user.grant_application_permission(@app, %w[not-signin]) - assert_equal [], Doorkeeper::Application.with_signin_permission_for(@user) - end + assert_equal [], Doorkeeper::Application.with_signin_permission_for(@user) end + end - context ".without_signin_permission_for" do - setup do - @user = create(:user) - @app = create(:application) - end + context ".without_signin_permission_for" do + setup do + @user = create(:user) + @app = create(:application) + end - should "exclude applications the user has the signin permission for" do - @user.grant_application_signin_permission(@app) + should "exclude applications the user has the signin permission for" do + @user.grant_application_signin_permission(@app) - assert_equal [], Doorkeeper::Application.without_signin_permission_for(@user) - end + assert_equal [], Doorkeeper::Application.without_signin_permission_for(@user) + end - should "include applications the user does not have the signin permission for" do - create(:supported_permission, application: @app, name: "not-signin") + should "include applications the user does not have the signin permission for" do + create(:supported_permission, application: @app, name: "not-signin") - @user.grant_application_permission(@app, %w[not-signin]) + @user.grant_application_permission(@app, %w[not-signin]) - assert_equal [@app], Doorkeeper::Application.without_signin_permission_for(@user) - end + assert_equal [@app], Doorkeeper::Application.without_signin_permission_for(@user) + end - should "include applications the user doesn't have any permissions for" do - assert_equal [@app], Doorkeeper::Application.without_signin_permission_for(@user) - end + should "include applications the user doesn't have any permissions for" do + assert_equal [@app], Doorkeeper::Application.without_signin_permission_for(@user) end end From 8486632de409f28d0d3eac0a0960d40c460f7a68 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 09:51:50 +0100 Subject: [PATCH 10/32] Move tests for retired/not_retired application scopes 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. --- test/models/doorkeeper/application_test.rb | 64 +++++++++++----------- 1 file changed, 32 insertions(+), 32 deletions(-) diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index 9a1476718..ed7a461ee 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -144,6 +144,38 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase end end + context ".retired" do + setup do + @app = create(:application) + end + + should "include apps that have been retired" do + @app.update!(retired: true) + assert_equal [@app], Doorkeeper::Application.retired + end + + should "exclude apps that have not been retired" do + @app.update!(retired: false) + assert_equal [], Doorkeeper::Application.retired + end + end + + context ".not_retired" do + setup do + @app = create(:application) + end + + should "include apps that have not been retired" do + @app.update!(retired: false) + assert_equal [@app], Doorkeeper::Application.not_retired + end + + should "exclude apps that have been retired" do + @app.update!(retired: true) + assert_equal [], Doorkeeper::Application.not_retired + end + end + context ".can_signin" do should "return applications that the user can signin into" do user = create(:user) @@ -183,38 +215,6 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase end end - context ".retired" do - setup do - @app = create(:application) - end - - should "include apps that have been retired" do - @app.update!(retired: true) - assert_equal [@app], Doorkeeper::Application.retired - end - - should "exclude apps that have not been retired" do - @app.update!(retired: false) - assert_equal [], Doorkeeper::Application.retired - end - end - - context ".not_retired" do - setup do - @app = create(:application) - end - - should "include apps that have not been retired" do - @app.update!(retired: false) - assert_equal [@app], Doorkeeper::Application.not_retired - end - - should "exclude apps that have been retired" do - @app.update!(retired: true) - assert_equal [], Doorkeeper::Application.not_retired - end - end - context ".with_signin_permission_for" do setup do @user = create(:user) From 1cc448e27b4239e57b48bf863552636a330ea742 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 10:44:04 +0100 Subject: [PATCH 11/32] Simplify disabling of Rails/ApplicationRecord cop 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! --- app/models/doorkeeper/application.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index e299161e5..988e88aae 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -1,8 +1,6 @@ require "doorkeeper/orm/active_record/application" -# rubocop:disable Rails/ApplicationRecord -class Doorkeeper::Application < ActiveRecord::Base - # rubocop:enable Rails/ApplicationRecord +class Doorkeeper::Application < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord has_many :supported_permissions, dependent: :destroy default_scope { ordered_by_name } From 66925c339186c306bb46451c65c8f8db18df798c Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 10:45:30 +0100 Subject: [PATCH 12/32] Blank lines to distinguish default scopes from other scopes --- app/models/api_user.rb | 1 + app/models/doorkeeper/application.rb | 1 + app/models/supported_permission.rb | 1 + 3 files changed, 3 insertions(+) diff --git a/app/models/api_user.rb b/app/models/api_user.rb index 00c1cf651..e9c6f3a01 100644 --- a/app/models/api_user.rb +++ b/app/models/api_user.rb @@ -1,5 +1,6 @@ class ApiUser < User default_scope { where(api_user: true).order(:name) } + validate :reason_for_2sv_exemption_blank validate :require_2sv_is_false diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index 988e88aae..f5d38d1b6 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -4,6 +4,7 @@ class Doorkeeper::Application < ActiveRecord::Base # rubocop:disable Rails/Appli has_many :supported_permissions, dependent: :destroy default_scope { ordered_by_name } + scope :ordered_by_name, -> { order("oauth_applications.name") } scope :support_push_updates, -> { where(supports_push_updates: true) } scope :retired, -> { where(retired: true) } diff --git a/app/models/supported_permission.rb b/app/models/supported_permission.rb index e0d3959de..8e652ce7b 100644 --- a/app/models/supported_permission.rb +++ b/app/models/supported_permission.rb @@ -9,6 +9,7 @@ class SupportedPermission < ApplicationRecord validate :signin_permission_name_not_changed default_scope { order(:name) } + scope :delegatable, -> { where(delegatable: true) } scope :grantable_from_ui, -> { where(grantable_from_ui: true) } scope :default, -> { where(default: true) } From b4c099971b91c240d72a2967b8d373fa89a40677 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 11:11:34 +0100 Subject: [PATCH 13/32] Add empty DoorkeeperApplicationsController#edit method To make it clearer that this controller has an `edit` action which renders the view containing the form for updating an application. --- app/controllers/doorkeeper_applications_controller.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb index 11ec5a92c..1c2e9f03d 100644 --- a/app/controllers/doorkeeper_applications_controller.rb +++ b/app/controllers/doorkeeper_applications_controller.rb @@ -11,6 +11,8 @@ def index @applications = Doorkeeper::Application.all end + def edit; end + def update if @application.update(doorkeeper_application_params) redirect_to doorkeeper_applications_path, notice: "Successfully updated #{@application.name}" From dbc1829cfc80519d4a0ca4505c776417249cdcf6 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 11:13:21 +0100 Subject: [PATCH 14/32] Add tests for editing a retired 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. --- .../doorkeeper_applications_controller_test.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/controllers/doorkeeper_applications_controller_test.rb b/test/controllers/doorkeeper_applications_controller_test.rb index b70e2d5b1..0d5d39620 100644 --- a/test/controllers/doorkeeper_applications_controller_test.rb +++ b/test/controllers/doorkeeper_applications_controller_test.rb @@ -24,6 +24,12 @@ class DoorkeeperApplicationsControllerTest < ActionController::TestCase get :edit, params: { id: app.id } assert_select "input[name='doorkeeper_application[name]'][value='My first app']" end + + should "render the form for retired app" do + app = create(:application, name: "My first app", retired: true) + get :edit, params: { id: app.id } + assert_select "input[name='doorkeeper_application[name]'][value='My first app']" + end end context "PUT update" do @@ -36,6 +42,13 @@ class DoorkeeperApplicationsControllerTest < ActionController::TestCase assert_match(/updated/, flash[:notice]) end + should "update a retired application" do + app = create(:application, name: "My first app", retired: true) + put :update, params: { id: app.id, doorkeeper_application: { name: "A better name" } } + + assert_equal "A better name", app.reload.name + end + should "redisplay the form if save fails" do app = create(:application, name: "My first app") put :update, params: { id: app.id, doorkeeper_application: { name: "" } } From 545be3451fd5fe0f9e60ff029b840e69fdd7ca95 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 11:29:31 +0100 Subject: [PATCH 15/32] Group tests for granting permissions to a user 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. --- test/models/user_test.rb | 74 +++++++++++++++++++++------------------- 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 616fb403b..a4d8f799f 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -564,15 +564,47 @@ def setup end end - test "can grant signin permission to allow user to access the app" do - app = create(:application) - user = create(:user) + context "granting permissions" do + should "grant signin permission to allow user to access the app" do + app = create(:application) + user = create(:user) + + assert_not user.has_access_to?(app) + + user.grant_application_signin_permission(app) + + assert user.has_access_to?(app) + end + + should "not create duplication permission when granting an already granted permission" do + app = create(:application, name: "my_app") + user = create(:user) + + user.grant_application_signin_permission(app) + user.grant_application_signin_permission(app) + + assert_user_has_permissions [SupportedPermission::SIGNIN_NAME], app, user + end + + should "grant permissions to user and return the created permission" do + app = create(:application, name: "my_app", with_supported_permissions: ["Create publications", "Delete publications"]) + user = create(:user) + + permission = user.grant_application_permission(app, "Create publications") + + assert_equal permission, user.application_permissions.first + assert_user_has_permissions ["Create publications"], app, user + end - assert_not user.has_access_to?(app) + should "return multiple permissions in name order" do + app = create(:application, name: "my_app", with_supported_permissions: %w[edit]) + user = create(:user) - user.grant_application_signin_permission(app) + user.grant_application_signin_permission(app) + user.grant_application_permission(app, "edit") - assert user.has_access_to?(app) + assert_user_has_permissions ["edit", SupportedPermission::SIGNIN_NAME], app, user + end end context "#has_permission?" do @@ -626,36 +658,6 @@ def setup end end - test "can grant permissions to users and return the created permission" do - app = create(:application, name: "my_app", with_supported_permissions: ["Create publications", "Delete publications"]) - user = create(:user) - - permission = user.grant_application_permission(app, "Create publications") - - assert_equal permission, user.application_permissions.first - assert_user_has_permissions ["Create publications"], app, user - end - - test "granting an already granted permission doesn't cause duplicates" do - app = create(:application, name: "my_app") - user = create(:user) - - user.grant_application_signin_permission(app) - user.grant_application_signin_permission(app) - - assert_user_has_permissions [SupportedPermission::SIGNIN_NAME], app, user - end - - test "returns multiple permissions in name order" do - app = create(:application, name: "my_app", with_supported_permissions: %w[edit]) - user = create(:user) - - user.grant_application_signin_permission(app) - user.grant_application_permission(app, "edit") - - assert_user_has_permissions ["edit", SupportedPermission::SIGNIN_NAME], app, user - end - test "inviting a user sets confirmed_at" do if (user = User.find_by(email: "j@1.com")) user.delete From c2fad7c8cd5f2a25b1dd097d0f2c9f7e606206a7 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 11:38:43 +0100 Subject: [PATCH 16/32] Remove redundant app name from some user tests 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. --- test/models/user_test.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a4d8f799f..7a4099735 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -577,7 +577,7 @@ def setup end should "not create duplication permission when granting an already granted permission" do - app = create(:application, name: "my_app") + app = create(:application) user = create(:user) user.grant_application_signin_permission(app) @@ -587,7 +587,7 @@ def setup end should "grant permissions to user and return the created permission" do - app = create(:application, name: "my_app", with_supported_permissions: ["Create publications", "Delete publications"]) + app = create(:application, with_supported_permissions: ["Create publications", "Delete publications"]) user = create(:user) permission = user.grant_application_permission(app, "Create publications") @@ -597,7 +597,7 @@ def setup end should "return multiple permissions in name order" do - app = create(:application, name: "my_app", with_supported_permissions: %w[edit]) + app = create(:application, with_supported_permissions: %w[edit]) user = create(:user) user.grant_application_signin_permission(app) From 3ed9d73a26c177f7e06015e50933fe563a4b63ed Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 11:44:47 +0100 Subject: [PATCH 17/32] Add test for granting permissions to access retired app 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). --- test/models/user_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 7a4099735..9664e9b50 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -596,6 +596,18 @@ def setup assert_user_has_permissions ["Create publications"], app, user end + should "grant permission to user for a retired application" do + app = create(:application, retired: true, with_supported_permissions: %w[edit]) + user = create(:user) + + signin_permission = user.grant_application_signin_permission(app) + edit_permission = user.grant_application_permission(app, "edit") + + assert_includes user.application_permissions, signin_permission + assert_includes user.application_permissions, edit_permission + assert_user_has_permissions ["edit", SupportedPermission::SIGNIN_NAME], app, user + end + should "return multiple permissions in name order" do app = create(:application, with_supported_permissions: %w[edit]) user = create(:user) From 9a9cd02d8574529b19f572a8352f031c3f07455e Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:01:11 +0100 Subject: [PATCH 18/32] Add not_retired to Doorkeeper::Application default scope 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 --- .../doorkeeper_applications_controller.rb | 2 +- app/models/doorkeeper/application.rb | 2 +- app/models/user.rb | 2 ++ .../doorkeeper_applications/index.html.erb | 2 +- test/factories/oauth_applications.rb | 6 +++--- test/models/doorkeeper/application_test.rb | 20 +++++++++++++++++-- test/models/user_test.rb | 8 ++++---- 7 files changed, 30 insertions(+), 12 deletions(-) diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb index 1c2e9f03d..4b126161a 100644 --- a/app/controllers/doorkeeper_applications_controller.rb +++ b/app/controllers/doorkeeper_applications_controller.rb @@ -29,7 +29,7 @@ def users_with_access private def load_and_authorize_application - @application = Doorkeeper::Application.find(params[:id]) + @application = Doorkeeper::Application.unscoped.find(params[:id]) authorize @application end diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index f5d38d1b6..ac285333b 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -3,7 +3,7 @@ class Doorkeeper::Application < ActiveRecord::Base # rubocop:disable Rails/ApplicationRecord has_many :supported_permissions, dependent: :destroy - default_scope { ordered_by_name } + default_scope { not_retired.ordered_by_name } scope :ordered_by_name, -> { order("oauth_applications.name") } scope :support_push_updates, -> { where(supports_push_updates: true) } diff --git a/app/models/user.rb b/app/models/user.rb index 0cb71541e..80298a5cf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -208,6 +208,8 @@ def grant_application_permission(application, supported_permission_name) end def grant_application_permissions(application, supported_permission_names) + return [] if application.retired? + supported_permission_names.map do |supported_permission_name| supported_permission = SupportedPermission.find_by(application_id: application.id, name: supported_permission_name) grant_permission(supported_permission) diff --git a/app/views/doorkeeper_applications/index.html.erb b/app/views/doorkeeper_applications/index.html.erb index e346665ee..85f1b8c3a 100644 --- a/app/views/doorkeeper_applications/index.html.erb +++ b/app/views/doorkeeper_applications/index.html.erb @@ -10,7 +10,7 @@ { id: "retired", label: "Retired applications", - content: render("application_list", applications: @applications.retired) + content: render("application_list", applications: @applications.unscoped.retired) } ] } %> diff --git a/test/factories/oauth_applications.rb b/test/factories/oauth_applications.rb index 2e1bbb89b..6e88dd4a8 100644 --- a/test/factories/oauth_applications.rb +++ b/test/factories/oauth_applications.rb @@ -18,19 +18,19 @@ # this line takes care of tests creating signin in order to look complete or modify delegatable on it. app.signin_permission.update(delegatable: false) && next if permission_name == SupportedPermission::SIGNIN_NAME - create(:supported_permission, application_id: app.id, name: permission_name) + create(:supported_permission, application: app, name: permission_name) end evaluator.with_supported_permissions_not_grantable_from_ui.each do |permission_name| next if permission_name == SupportedPermission::SIGNIN_NAME - create(:supported_permission, application_id: app.id, name: permission_name, grantable_from_ui: false) + create(:supported_permission, application: app, name: permission_name, grantable_from_ui: false) end evaluator.with_delegatable_supported_permissions.each do |permission_name| next if permission_name == SupportedPermission::SIGNIN_NAME - create(:delegatable_supported_permission, application_id: app.id, name: permission_name) + create(:delegatable_supported_permission, application: app, name: permission_name) end end end diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index ed7a461ee..84d4d6a59 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -144,6 +144,22 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase end end + context ".all (default scope)" do + setup do + @app = create(:application) + end + + should "include apps that have not been retired" do + @app.update!(retired: false) + assert_equal [@app], Doorkeeper::Application.all + end + + should "exclude apps that have been retired" do + @app.update!(retired: true) + assert_equal [], Doorkeeper::Application.all + end + end + context ".retired" do setup do @app = create(:application) @@ -151,12 +167,12 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase should "include apps that have been retired" do @app.update!(retired: true) - assert_equal [@app], Doorkeeper::Application.retired + assert_equal [@app], Doorkeeper::Application.unscoped.retired end should "exclude apps that have not been retired" do @app.update!(retired: false) - assert_equal [], Doorkeeper::Application.retired + assert_equal [], Doorkeeper::Application.unscoped.retired end end diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 9664e9b50..e935dea67 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -596,16 +596,16 @@ def setup assert_user_has_permissions ["Create publications"], app, user end - should "grant permission to user for a retired application" do + should "not grant permission to user for a retired application" do app = create(:application, retired: true, with_supported_permissions: %w[edit]) user = create(:user) signin_permission = user.grant_application_signin_permission(app) edit_permission = user.grant_application_permission(app, "edit") - assert_includes user.application_permissions, signin_permission - assert_includes user.application_permissions, edit_permission - assert_user_has_permissions ["edit", SupportedPermission::SIGNIN_NAME], app, user + assert_nil signin_permission + assert_nil edit_permission + assert_user_has_permissions [], app, user end should "return multiple permissions in name order" do From 50487f995094bf902cb6b9e888865d4b71d60f96 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:27:13 +0100 Subject: [PATCH 19/32] Remove redundant not_retired from can_signin scope 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. --- app/models/doorkeeper/application.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index ac285333b..65a17f5f0 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -9,11 +9,7 @@ class Doorkeeper::Application < ActiveRecord::Base # rubocop:disable Rails/Appli scope :support_push_updates, -> { where(supports_push_updates: true) } scope :retired, -> { where(retired: true) } scope :not_retired, -> { where(retired: false) } - scope :can_signin, - lambda { |user| - with_signin_permission_for(user) - .not_retired - } + scope :can_signin, ->(user) { with_signin_permission_for(user) } scope :with_signin_delegatable, lambda { joins(:supported_permissions) From dd1b8c4ccac10bda10f30cbd7249a8aed333df79 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:27:13 +0100 Subject: [PATCH 20/32] Remove redundant not_retired scope from applications page 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. --- app/views/doorkeeper_applications/index.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/doorkeeper_applications/index.html.erb b/app/views/doorkeeper_applications/index.html.erb index 85f1b8c3a..f12573a76 100644 --- a/app/views/doorkeeper_applications/index.html.erb +++ b/app/views/doorkeeper_applications/index.html.erb @@ -5,7 +5,7 @@ { id: "active", label: "Active applications", - content: render("application_list", applications: @applications.not_retired) + content: render("application_list", applications: @applications) }, { id: "retired", From a992e97f7647188d2524031f7dd72ede4a443e86 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:27:13 +0100 Subject: [PATCH 21/32] Remove redundant not_retired scope from batch invitation page 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. --- app/views/batch_invitation_permissions/new.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index 2e9e90183..d557a7350 100644 --- a/app/views/batch_invitation_permissions/new.html.erb +++ b/app/views/batch_invitation_permissions/new.html.erb @@ -19,7 +19,7 @@ %> <%= form_for @batch_invitation, url: :batch_invitation_permissions, method: :post do |f| %> - <% policy_scope(:user_permission_manageable_application).not_retired.map do |application| %> + <% policy_scope(:user_permission_manageable_application).map do |application| %> <% options = options_for_permission_option_select(application:, user: User.with_default_permissions) %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, From f38da591319a81151472ecc6e30e690e5e39200a Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:27:13 +0100 Subject: [PATCH 22/32] Remove redundant not_retired scope from account apps page 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. --- app/controllers/account/applications_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/account/applications_controller.rb b/app/controllers/account/applications_controller.rb index c5684eb91..59a8002cd 100644 --- a/app/controllers/account/applications_controller.rb +++ b/app/controllers/account/applications_controller.rb @@ -13,6 +13,6 @@ def index authorize [:account, Doorkeeper::Application] @applications_with_signin = Doorkeeper::Application.can_signin(current_user) - @applications_without_signin = Doorkeeper::Application.not_retired.without_signin_permission_for(current_user) + @applications_without_signin = Doorkeeper::Application.without_signin_permission_for(current_user) end end From 02434180f47851ec31e7f73dc21fed721f5505b3 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:27:13 +0100 Subject: [PATCH 23/32] Remove redundant not_retired scope from account permissions page 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. --- app/controllers/account/permissions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/account/permissions_controller.rb b/app/controllers/account/permissions_controller.rb index eeb623fd7..f8eced59b 100644 --- a/app/controllers/account/permissions_controller.rb +++ b/app/controllers/account/permissions_controller.rb @@ -4,7 +4,7 @@ class Account::PermissionsController < ApplicationController before_action :authenticate_user! def index - @application = Doorkeeper::Application.not_retired.find(params[:application_id]) + @application = Doorkeeper::Application.find(params[:application_id]) authorize [:account, @application], :view_permissions? From 5fa7286d9d6ebe69724079ce89b5d935df0c034f Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 14:27:10 +0100 Subject: [PATCH 24/32] Extract Account::SigninPermissionsController#application To reduce duplication. --- .../account/signin_permissions_controller.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/app/controllers/account/signin_permissions_controller.rb b/app/controllers/account/signin_permissions_controller.rb index 00652d65e..718054d71 100644 --- a/app/controllers/account/signin_permissions_controller.rb +++ b/app/controllers/account/signin_permissions_controller.rb @@ -6,8 +6,6 @@ class Account::SigninPermissionsController < ApplicationController def create authorize [:account, Doorkeeper::Application], :grant_signin_permission? - application = Doorkeeper::Application.not_retired.find(params[:application_id]) - params = { supported_permission_ids: current_user.supported_permissions.map(&:id) + [application.signin_permission.id] } UserUpdate.new(current_user, params, current_user, user_ip_address).call @@ -15,14 +13,10 @@ def create end def delete - @application = Doorkeeper::Application.not_retired.find(params[:application_id]) - - authorize [:account, @application], :remove_signin_permission? + authorize [:account, application], :remove_signin_permission? end def destroy - application = Doorkeeper::Application.not_retired.find(params[:application_id]) - authorize [:account, application], :remove_signin_permission? params = { supported_permission_ids: current_user.supported_permissions.map(&:id) - [application.signin_permission.id] } @@ -30,4 +24,10 @@ def destroy redirect_to account_applications_path end + +private + + def application + @application ||= Doorkeeper::Application.not_retired.find(params[:application_id]) + end end From 194188d286a7e639f7295877e691549b9b4be35e Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:27:13 +0100 Subject: [PATCH 25/32] Remove redundant not_retired scope from account signin permissions page 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. --- app/controllers/account/signin_permissions_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/account/signin_permissions_controller.rb b/app/controllers/account/signin_permissions_controller.rb index 718054d71..e964b9307 100644 --- a/app/controllers/account/signin_permissions_controller.rb +++ b/app/controllers/account/signin_permissions_controller.rb @@ -28,6 +28,6 @@ def destroy private def application - @application ||= Doorkeeper::Application.not_retired.find(params[:application_id]) + @application ||= Doorkeeper::Application.find(params[:application_id]) end end From 54aaaa7d8d633cd70d210b74f6e722e0f762d563 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 12:27:13 +0100 Subject: [PATCH 26/32] Remove redundant not_retired scope from new invitation page 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. --- app/views/devise/invitations/new.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index e887c86bf..bef70f9af 100644 --- a/app/views/devise/invitations/new.html.erb +++ b/app/views/devise/invitations/new.html.erb @@ -86,7 +86,7 @@ heading_size: "l", } do %> - <% policy_scope(:user_permission_manageable_application).not_retired.map do |application| %> + <% policy_scope(:user_permission_manageable_application).map do |application| %> <% options = options_for_permission_option_select(application:, user: f.object) %> <%= render("govuk_publishing_components/components/option_select", { title: application.name, From 7f23aeb6e60da8fea76ecef31789af6356c24dac Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 16:48:18 +0100 Subject: [PATCH 27/32] Add test for User#application_permissions association I'm about to make a change to this, so I wanted to add some test coverage first. --- test/models/user_test.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index e935dea67..c91c22d0a 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -10,6 +10,15 @@ def setup @user = create(:user) end + context "#application_permissions" do + should "return user application permissions for user" do + application = create(:application) + user_application_permission = create(:user_application_permission, user: @user, application:) + + assert_includes @user.application_permissions, user_application_permission + end + end + context "#require_2sv" do should "default to false for normal users" do assert_not create(:user).require_2sv? From ab6d5af12a1eb96d38273b4659a41f04f6d14d33 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 17:00:01 +0100 Subject: [PATCH 28/32] Disambiguate name column in User#permissions_for 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. --- app/models/user.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 80298a5cf..223ff9857 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -161,8 +161,8 @@ def permissions_for(application) application_permissions .joins(:supported_permission) .where(application_id: application.id) - .order("supported_permissions.name") - .pluck(:name) + .order(SupportedPermission.arel_table[:name]) + .pluck(SupportedPermission.arel_table[:name]) end # Avoid N+1 queries by using the relations eager loaded with `includes()`. From 99d487136087e4523b0a2edcaf708774fad9cb9e Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 16:50:09 +0100 Subject: [PATCH 29/32] Exclude retired apps from User#application_permissions 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. --- app/models/user.rb | 2 +- test/models/user_test.rb | 7 +++++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 223ff9857..514c025ed 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -57,7 +57,7 @@ class User < ApplicationRecord validate :organisation_has_mandatory_2sv, on: :create has_many :authorisations, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id - has_many :application_permissions, class_name: "UserApplicationPermission", inverse_of: :user, dependent: :destroy + has_many :application_permissions, -> { joins(:application) }, class_name: "UserApplicationPermission", inverse_of: :user, dependent: :destroy has_many :supported_permissions, through: :application_permissions has_many :batch_invitations belongs_to :organisation diff --git a/test/models/user_test.rb b/test/models/user_test.rb index c91c22d0a..0ffbac983 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -17,6 +17,13 @@ def setup assert_includes @user.application_permissions, user_application_permission end + + should "not include user application permissions for retired applications" do + application = create(:application, retired: true) + user_application_permission = create(:user_application_permission, user: @user, application:) + + assert_not_includes @user.application_permissions, user_application_permission + end end context "#require_2sv" do From 42eca69ee4dd2f8076fdd903782bd9cdab6ba496 Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 16:48:18 +0100 Subject: [PATCH 30/32] Add test for User#supported_permissions association I'm about to make a change to this, so I wanted to add some test coverage first. --- test/models/user_test.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 0ffbac983..631b0ae5a 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -26,6 +26,16 @@ def setup end end + context "#supported_permissions" do + should "return supported permissions for user" do + application = create(:application) + supported_permission = create(:supported_permission, application:) + create(:user_application_permission, user: @user, application:, supported_permission:) + + assert_includes @user.supported_permissions, supported_permission + end + end + context "#require_2sv" do should "default to false for normal users" do assert_not create(:user).require_2sv? From ab0c18fbe39533739f88d4b225a184ae19027a9a Mon Sep 17 00:00:00 2001 From: James Mead Date: Tue, 17 Oct 2023 16:50:09 +0100 Subject: [PATCH 31/32] Exclude retired apps from User#supported_permissions 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. --- app/models/user.rb | 2 +- test/models/user_test.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 514c025ed..2ba801863 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -58,7 +58,7 @@ class User < ApplicationRecord has_many :authorisations, class_name: "Doorkeeper::AccessToken", foreign_key: :resource_owner_id has_many :application_permissions, -> { joins(:application) }, class_name: "UserApplicationPermission", inverse_of: :user, dependent: :destroy - has_many :supported_permissions, through: :application_permissions + has_many :supported_permissions, -> { joins(:application) }, through: :application_permissions has_many :batch_invitations belongs_to :organisation diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 631b0ae5a..3c6b6f00b 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -34,6 +34,14 @@ def setup assert_includes @user.supported_permissions, supported_permission end + + should "not include supported permissions for retired applications" do + application = create(:application, retired: true) + supported_permission = create(:supported_permission, application:) + create(:user_application_permission, user: @user, application:, supported_permission:) + + assert_not_includes @user.supported_permissions, supported_permission + end end context "#require_2sv" do From 2bb738ccc4d049ad090e23ecb0352e9293f4890c Mon Sep 17 00:00:00 2001 From: James Mead Date: Thu, 19 Oct 2023 09:13:24 +0100 Subject: [PATCH 32/32] Tweak hint text for retiring an application Make it clear that retiring an application will hide it from most pages; not just the dashboard. --- app/views/doorkeeper_applications/edit.html.erb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/app/views/doorkeeper_applications/edit.html.erb b/app/views/doorkeeper_applications/edit.html.erb index 46bd889d9..dcec6151e 100644 --- a/app/views/doorkeeper_applications/edit.html.erb +++ b/app/views/doorkeeper_applications/edit.html.erb @@ -107,8 +107,10 @@

- Retiring an application will hide the application from the dashboard. It will - not delete data. If you're retiring an app you should also disable push updates. + Retiring an application indicates that it is no longer in use. + Retired applications will not appear on most pages including the dashboard. + Data associated with retired applications is not deleted. + If you're retiring an application you should also disable push updates.