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 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? diff --git a/app/controllers/account/signin_permissions_controller.rb b/app/controllers/account/signin_permissions_controller.rb index 00652d65e..e964b9307 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.find(params[:application_id]) + end end diff --git a/app/controllers/doorkeeper_applications_controller.rb b/app/controllers/doorkeeper_applications_controller.rb index 11ec5a92c..4b126161a 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}" @@ -27,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/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 d377b8b7b..65a17f5f0 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -1,19 +1,15 @@ 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 } + default_scope { not_retired.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| - with_signin_permission_for(user) - .not_retired - } + scope :can_signin, ->(user) { with_signin_permission_for(user) } scope :with_signin_delegatable, lambda { joins(:supported_permissions) 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) } diff --git a/app/models/user.rb b/app/models/user.rb index 0cb71541e..2ba801863 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -57,8 +57,8 @@ 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 :supported_permissions, through: :application_permissions + has_many :application_permissions, -> { joins(:application) }, class_name: "UserApplicationPermission", inverse_of: :user, dependent: :destroy + has_many :supported_permissions, -> { joins(:application) }, through: :application_permissions has_many :batch_invitations belongs_to :organisation @@ -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()`. @@ -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/batch_invitation_permissions/new.html.erb b/app/views/batch_invitation_permissions/new.html.erb index e7c7e0fef..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).reject(&: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, diff --git a/app/views/devise/invitations/new.html.erb b/app/views/devise/invitations/new.html.erb index 503cb935f..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).reject(&: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, 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.

diff --git a/app/views/doorkeeper_applications/index.html.erb b/app/views/doorkeeper_applications/index.html.erb index ad3d907c9..f12573a76 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) }, { id: "retired", label: "Retired applications", - content: render("application_list", applications: @applications.select(&:retired?)) + content: render("application_list", applications: @applications.unscoped.retired) } ] } %> 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 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 diff --git a/test/controllers/doorkeeper_applications_controller_test.rb b/test/controllers/doorkeeper_applications_controller_test.rb index 8b4f51ddc..0d5d39620 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 @@ -20,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 @@ -32,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: "" } } 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 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 a2417f8c0..84d4d6a59 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -144,7 +144,55 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase end end - context "scopes" do + 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) + end + + should "include apps that have been retired" do + @app.update!(retired: true) + 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.unscoped.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) application = create(:application) @@ -167,7 +215,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,67 +229,51 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase assert_empty Doorkeeper::Application.with_signin_delegatable 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 + 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 diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 616fb403b..3c6b6f00b 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -10,6 +10,40 @@ 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 + + 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 "#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 + + 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 should "default to false for normal users" do assert_not create(:user).require_2sv? @@ -564,15 +598,59 @@ 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) + user = create(:user) + + user.grant_application_signin_permission(app) + user.grant_application_signin_permission(app) - assert_not user.has_access_to?(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, 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 + + 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_nil signin_permission + assert_nil edit_permission + assert_user_has_permissions [], app, user + end + + should "return multiple permissions in name order" do + app = create(:application, 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 +704,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