From db3607b246666b561a0d574375f5df82b4668dcc Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 21 Sep 2023 12:36:57 +0100 Subject: [PATCH 1/8] Use redirect_back_or_to in user_not_authorized If the user attempts to do something they're not authorized to do then we should first try to take them back to the page they were on, and only redirect to the root path as a fallback. Ideally users wouldn't be able to use the UI to navigate to actions they're not authorized to execute but this small change will make the experience slightly better if they are able to. --- app/controllers/application_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index ff9783719..8bc7560f0 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -59,7 +59,7 @@ def api_user_via_token_has_signin_permission_on_app? def user_not_authorized(_exception) flash[:alert] = "You do not have permission to perform this action." - redirect_to root_path + redirect_back_or_to root_path end def notify_bad_request(_exception) From abc69899cb93d4bc347436bb000e8b949068ea51 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 26 Sep 2023 16:26:45 +0100 Subject: [PATCH 2/8] Use govuk-link class in "View permissions" link Calum spotted that I'd missed this. --- app/views/account/applications/index.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb index ef0c3e03e..9036b1f5e 100644 --- a/app/views/account/applications/index.html.erb +++ b/app/views/account/applications/index.html.erb @@ -31,7 +31,7 @@ <%= application.name %> <%= application.description %> - <%= link_to account_application_permissions_path(application) do %> + <%= link_to account_application_permissions_path(application), class: "govuk-link" do %> View permissions for <%= application.name %> <% end %> From daa545c083c0befcb22665ad3811dab0ac34a982 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 26 Sep 2023 15:59:15 +0100 Subject: [PATCH 3/8] Improve display of permissions table To avoid the column widths varying based on the length of text in the (permission) Name column. --- app/views/account/permissions/index.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/account/permissions/index.html.erb b/app/views/account/permissions/index.html.erb index f3ab0904d..83bbd7591 100644 --- a/app/views/account/permissions/index.html.erb +++ b/app/views/account/permissions/index.html.erb @@ -22,7 +22,7 @@ - + From 02b87fe5d0aad0a9b69a53062cd44094cc11e623 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 26 Sep 2023 14:19:25 +0100 Subject: [PATCH 4/8] Namespace the ApplicationPolicy In order to allow Publishing Managers to remove their own signin permission from apps I'm going to need an instance of the Application so that I can check whether it has delegatable permissions. This preparatory change will allow me to pass an instance of Application to `authorize` in order to automagically find this `Account::ApplicationPolicy` class. --- app/controllers/account/applications_controller.rb | 4 ++-- .../application_policy.rb} | 2 +- .../application_policy_test.rb} | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) rename app/policies/{account_applications_policy.rb => account/application_policy.rb} (64%) rename test/policies/{account_applications_policy_test.rb => account/application_policy_test.rb} (95%) diff --git a/app/controllers/account/applications_controller.rb b/app/controllers/account/applications_controller.rb index 90786c9f9..c5684eb91 100644 --- a/app/controllers/account/applications_controller.rb +++ b/app/controllers/account/applications_controller.rb @@ -4,13 +4,13 @@ class Account::ApplicationsController < ApplicationController before_action :authenticate_user! def show - authorize :account_applications + authorize [:account, Doorkeeper::Application] redirect_to account_applications_path end def index - authorize :account_applications + 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) diff --git a/app/policies/account_applications_policy.rb b/app/policies/account/application_policy.rb similarity index 64% rename from app/policies/account_applications_policy.rb rename to app/policies/account/application_policy.rb index df10948e4..1d3b690c6 100644 --- a/app/policies/account_applications_policy.rb +++ b/app/policies/account/application_policy.rb @@ -1,4 +1,4 @@ -class AccountApplicationsPolicy < BasePolicy +class Account::ApplicationPolicy < BasePolicy def index? current_user.govuk_admin? end diff --git a/test/policies/account_applications_policy_test.rb b/test/policies/account/application_policy_test.rb similarity index 95% rename from test/policies/account_applications_policy_test.rb rename to test/policies/account/application_policy_test.rb index c6232566c..4aed6b9f6 100644 --- a/test/policies/account_applications_policy_test.rb +++ b/test/policies/account/application_policy_test.rb @@ -1,7 +1,7 @@ require "test_helper" require "support/policy_helpers" -class AccountApplicationsPolicyTest < ActiveSupport::TestCase +class Account::ApplicationPolicyTest < ActiveSupport::TestCase include PolicyHelpers context "accessing index?" do From b6c3b147ee1c9ec0387f8d6dc7076e4b3b73f21d Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Tue, 26 Sep 2023 14:34:11 +0100 Subject: [PATCH 5/8] Move permission from UserPolicy to ApplicationPolicy In preparation for allowing Publishing Managers to use the /account/applications page. Publishing Managers can only remove their signin permission from an application if the application has delegatable permissions, so we need an instance of Application to check whether the user is authorized to remove their access. I've chosen to move all permission related methods over to keep them together. --- .../account/permissions_controller.rb | 2 +- .../account/signin_permissions_controller.rb | 6 +- app/policies/account/application_policy.rb | 3 + app/policies/user_policy.rb | 6 -- .../account/application_policy_test.rb | 79 +++++++++++++++++++ test/policies/user_policy_test.rb | 78 ------------------ 6 files changed, 86 insertions(+), 88 deletions(-) diff --git a/app/controllers/account/permissions_controller.rb b/app/controllers/account/permissions_controller.rb index 56c5d4591..08155e20a 100644 --- a/app/controllers/account/permissions_controller.rb +++ b/app/controllers/account/permissions_controller.rb @@ -8,7 +8,7 @@ class Account::PermissionsController < ApplicationController def index @application = Doorkeeper::Application.not_retired.find(params[:application_id]) - authorize current_user, :view_permissions? + authorize [:account, @application], :view_permissions? @permissions = permissions_for(@application).sort_by { |permission| current_user.has_permission?(permission) ? 0 : 1 } end diff --git a/app/controllers/account/signin_permissions_controller.rb b/app/controllers/account/signin_permissions_controller.rb index a1d668291..00652d65e 100644 --- a/app/controllers/account/signin_permissions_controller.rb +++ b/app/controllers/account/signin_permissions_controller.rb @@ -4,7 +4,7 @@ class Account::SigninPermissionsController < ApplicationController before_action :authenticate_user! def create - authorize current_user, :grant_signin_permission? + authorize [:account, Doorkeeper::Application], :grant_signin_permission? application = Doorkeeper::Application.not_retired.find(params[:application_id]) @@ -17,13 +17,13 @@ def create def delete @application = Doorkeeper::Application.not_retired.find(params[:application_id]) - authorize current_user, :remove_signin_permission? + authorize [:account, @application], :remove_signin_permission? end def destroy application = Doorkeeper::Application.not_retired.find(params[:application_id]) - authorize current_user, :remove_signin_permission? + authorize [:account, application], :remove_signin_permission? 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 diff --git a/app/policies/account/application_policy.rb b/app/policies/account/application_policy.rb index 1d3b690c6..7699f5a81 100644 --- a/app/policies/account/application_policy.rb +++ b/app/policies/account/application_policy.rb @@ -4,4 +4,7 @@ def index? end alias_method :show?, :index? + alias_method :grant_signin_permission?, :index? + alias_method :remove_signin_permission?, :index? + alias_method :view_permissions?, :index? end diff --git a/app/policies/user_policy.rb b/app/policies/user_policy.rb index 5c61a12d7..6d26412b7 100644 --- a/app/policies/user_policy.rb +++ b/app/policies/user_policy.rb @@ -35,12 +35,6 @@ def edit? alias_method :resend?, :edit? alias_method :event_logs?, :edit? - def grant_signin_permission? - current_user.govuk_admin? - end - alias_method :remove_signin_permission?, :grant_signin_permission? - alias_method :view_permissions?, :grant_signin_permission? - def edit_email_or_password? allow_self_only end diff --git a/test/policies/account/application_policy_test.rb b/test/policies/account/application_policy_test.rb index 4aed6b9f6..eca13a6fb 100644 --- a/test/policies/account/application_policy_test.rb +++ b/test/policies/account/application_policy_test.rb @@ -55,4 +55,83 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase end end end + + context "#grant_signin_permission?" do + %i[superadmin admin].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = build(:"#{user_role}_user") + end + + should "be permitted" do + assert permit?(@current_user, nil, :grant_signin_permission) + end + end + end + + %i[super_organisation_admin organisation_admin normal].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = build(:"#{user_role}_user") + end + + should "be forbidden" do + assert forbid?(@current_user, nil, :grant_signin_permission) + end + end + end + end + + context "#remove_signin_permission?" do + %i[superadmin admin].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = build(:"#{user_role}_user") + @application = build(:application) + end + + should "be permitted" do + assert permit?(@current_user, nil, :remove_signin_permission) + end + end + end + + %i[super_organisation_admin organisation_admin normal].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = build(:"#{user_role}_user") + end + + should "be forbidden" do + assert forbid?(@current_user, nil, :remove_signin_permission) + end + end + end + end + + context "#view_permissions?" do + %i[superadmin admin].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = build(:"#{user_role}_user") + end + + should "be permitted" do + assert permit?(@current_user, nil, :view_permissions) + end + end + end + + %i[super_organisation_admin organisation_admin normal].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = build(:"#{user_role}_user") + end + + should "be forbidden" do + assert forbid?(@current_user, nil, :view_permissions) + end + end + end + end end diff --git a/test/policies/user_policy_test.rb b/test/policies/user_policy_test.rb index 05e7a6426..79fea5cc4 100644 --- a/test/policies/user_policy_test.rb +++ b/test/policies/user_policy_test.rb @@ -283,82 +283,4 @@ class UserPolicyTest < ActiveSupport::TestCase end end end - - context "#grant_signin_permission?" do - %i[superadmin admin].each do |user_role| - context "for #{user_role} users" do - setup do - @current_user = build(:"#{user_role}_user") - end - - should "be permitted" do - assert permit?(@current_user, @current_user, :grant_signin_permission) - end - end - end - - %i[super_organisation_admin organisation_admin normal].each do |user_role| - context "for #{user_role} users" do - setup do - @current_user = build(:"#{user_role}_user") - end - - should "be forbidden" do - assert forbid?(@current_user, @current_user, :grant_signin_permission) - end - end - end - end - - context "#remove_signin_permission?" do - %i[superadmin admin].each do |user_role| - context "for #{user_role} users" do - setup do - @current_user = build(:"#{user_role}_user") - end - - should "be permitted" do - assert permit?(@current_user, @current_user, :remove_signin_permission) - end - end - end - - %i[super_organisation_admin organisation_admin normal].each do |user_role| - context "for #{user_role} users" do - setup do - @current_user = build(:"#{user_role}_user") - end - - should "be forbidden" do - assert forbid?(@current_user, @current_user, :remove_signin_permission) - end - end - end - end - - context "#view_permissions?" do - %i[superadmin admin].each do |user_role| - context "for #{user_role} users" do - setup do - @current_user = build(:"#{user_role}_user") - end - - should "be permitted" do - assert permit?(@current_user, @current_user, :view_permissions) - end - end - end - - %i[super_organisation_admin organisation_admin normal].each do |user_role| - context "for #{user_role} users" do - setup do - @current_user = build(:"#{user_role}_user") - end - - should "be forbidden" do - assert forbid?(@current_user, @current_user, :view_permissions) - end - end - end - end end From a6d89ddf7e97eef207b9cd36a1db452ec4d406d6 Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 21 Sep 2023 12:51:53 +0100 Subject: [PATCH 6/8] Allow Publishing Managers to use /account/applications Publishing Managers can: - View permissions for all applications they have access to - Remove their access from applications with delegatable permissions Publishing Managers cannot: - Grant themselves access to applications - Remove their access from applications that don't have delegatable permissions --- app/policies/account/application_policy.rb | 16 +++- .../account/application_policy_test.rb | 75 ++++++++++++++++--- 2 files changed, 77 insertions(+), 14 deletions(-) diff --git a/app/policies/account/application_policy.rb b/app/policies/account/application_policy.rb index 7699f5a81..c1e9a4f9e 100644 --- a/app/policies/account/application_policy.rb +++ b/app/policies/account/application_policy.rb @@ -1,10 +1,20 @@ class Account::ApplicationPolicy < BasePolicy def index? - current_user.govuk_admin? + current_user.govuk_admin? || current_user.publishing_manager? end alias_method :show?, :index? - alias_method :grant_signin_permission?, :index? - alias_method :remove_signin_permission?, :index? alias_method :view_permissions?, :index? + + def grant_signin_permission? + current_user.govuk_admin? + end + + def remove_signin_permission? + current_user.has_access_to?(record) && + ( + current_user.govuk_admin? || + current_user.publishing_manager? && record.signin_permission.delegatable? + ) + end end diff --git a/test/policies/account/application_policy_test.rb b/test/policies/account/application_policy_test.rb index eca13a6fb..944c1cf29 100644 --- a/test/policies/account/application_policy_test.rb +++ b/test/policies/account/application_policy_test.rb @@ -5,7 +5,7 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase include PolicyHelpers context "accessing index?" do - %i[superadmin admin].each do |user_role| + %i[superadmin admin super_organisation_admin organisation_admin].each do |user_role| context "for #{user_role} users" do setup do @current_user = FactoryBot.build(:"#{user_role}_user") @@ -17,7 +17,7 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase end end - %i[super_organisation_admin organisation_admin normal].each do |user_role| + %i[normal].each do |user_role| context "for #{user_role} users" do setup do @current_user = FactoryBot.build(:"#{user_role}_user") @@ -31,7 +31,7 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase end context "show?" do - %i[superadmin admin].each do |user_role| + %i[superadmin admin super_organisation_admin organisation_admin].each do |user_role| context "for #{user_role} users" do setup do @current_user = build(:"#{user_role}_user") @@ -43,7 +43,7 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase end end - %i[super_organisation_admin organisation_admin normal].each do |user_role| + %i[normal].each do |user_role| context "for #{user_role} users" do setup do @current_user = build(:"#{user_role}_user") @@ -86,17 +86,70 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase %i[superadmin admin].each do |user_role| context "for #{user_role} users" do setup do - @current_user = build(:"#{user_role}_user") - @application = build(:application) + @current_user = create(:"#{user_role}_user") + @application = create(:application) end - should "be permitted" do - assert permit?(@current_user, nil, :remove_signin_permission) + context "when the user has signin permission for the app" do + setup do + @current_user.grant_application_signin_permission(@application) + end + + should "be permitted" do + assert permit?(@current_user, @application, :remove_signin_permission) + end + end + + context "when the user does not have the signin permission for the app" do + should "be forbidden" do + assert forbid?(@current_user, @application, :remove_signin_permission) + end end end end - %i[super_organisation_admin organisation_admin normal].each do |user_role| + %i[super_organisation_admin organisation_admin].each do |user_role| + context "for #{user_role} users" do + setup do + @current_user = create(:"#{user_role}_user") + @application = create(:application) + end + + context "when the user has signin permission for the app" do + setup do + @current_user.grant_application_signin_permission(@application) + end + + context "and the application has delegatable permissions" do + setup do + @application.signin_permission.update!(delegatable: true) + end + + should "be permitted" do + assert permit?(@current_user, @application, :remove_signin_permission) + end + end + + context "and the application does not have delegatable permissions" do + setup do + @application.signin_permission.update!(delegatable: false) + end + + should "not be permitted" do + assert forbid?(@current_user, @application, :remove_signin_permission) + end + end + end + + context "when the user does not have the signin permission for the app" do + should "be forbidden" do + assert forbid?(@current_user, @application, :remove_signin_permission) + end + end + end + end + + %i[normal].each do |user_role| context "for #{user_role} users" do setup do @current_user = build(:"#{user_role}_user") @@ -110,7 +163,7 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase end context "#view_permissions?" do - %i[superadmin admin].each do |user_role| + %i[superadmin admin super_organisation_admin organisation_admin].each do |user_role| context "for #{user_role} users" do setup do @current_user = build(:"#{user_role}_user") @@ -122,7 +175,7 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase end end - %i[super_organisation_admin organisation_admin normal].each do |user_role| + %i[normal].each do |user_role| context "for #{user_role} users" do setup do @current_user = build(:"#{user_role}_user") From defd38936b39cd904bc025bb0d0c6665b9c6194a Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 21 Sep 2023 12:52:57 +0100 Subject: [PATCH 7/8] Don't display "Grant access" button to Publishing Managers Publishing Managers aren't allowed to grant themselves access to applications so we shouldn't show them this button. --- app/views/account/applications/index.html.erb | 10 ++++++---- .../account/applications_controller_test.rb | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb index 9036b1f5e..754eeb304 100644 --- a/app/views/account/applications/index.html.erb +++ b/app/views/account/applications/index.html.erb @@ -63,10 +63,12 @@ diff --git a/test/controllers/account/applications_controller_test.rb b/test/controllers/account/applications_controller_test.rb index f6d27d77f..be82c30c9 100644 --- a/test/controllers/account/applications_controller_test.rb +++ b/test/controllers/account/applications_controller_test.rb @@ -22,4 +22,18 @@ class Account::ApplicationsControllerTest < ActionController::TestCase assert_redirected_to "/account/applications" end end + + context "#index" do + context "logged in as a publishing manager" do + should "not display the button to grant access to an application" do + application = create(:application, name: "app-name") + sign_in create(:organisation_admin_user) + + get :index + + assert_select "tr td", text: "app-name" + assert_select "form[action='#{account_application_signin_permission_path(application)}']", count: 0 + end + end + end end From 036a2ee546ea6f3a08bedfa1641fbe68574f1d7e Mon Sep 17 00:00:00 2001 From: Chris Roos Date: Thu, 21 Sep 2023 14:30:49 +0100 Subject: [PATCH 8/8] Conditionally display the "Remove access" button Publishing Managers can only remove their access from applications that have delegatable permissions. We should only display the button if they're allowed to remove their access. --- app/views/account/applications/index.html.erb | 10 ++++++---- .../account/applications_controller_test.rb | 13 +++++++++++++ 2 files changed, 19 insertions(+), 4 deletions(-) diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb index 754eeb304..a3530ab6e 100644 --- a/app/views/account/applications/index.html.erb +++ b/app/views/account/applications/index.html.erb @@ -36,10 +36,12 @@ <% end %> diff --git a/test/controllers/account/applications_controller_test.rb b/test/controllers/account/applications_controller_test.rb index be82c30c9..8833a3ec5 100644 --- a/test/controllers/account/applications_controller_test.rb +++ b/test/controllers/account/applications_controller_test.rb @@ -34,6 +34,19 @@ class Account::ApplicationsControllerTest < ActionController::TestCase assert_select "tr td", text: "app-name" assert_select "form[action='#{account_application_signin_permission_path(application)}']", count: 0 end + + should "not display the button to remove access to an application" do + application = create(:application, name: "app-name") + application.signin_permission.update!(delegatable: false) + user = create(:organisation_admin_user, with_signin_permissions_for: [application]) + + sign_in user + + get :index + + assert_select "tr td", text: "app-name" + assert_select "a[href='#{delete_account_application_signin_permission_path(application)}']", count: 0 + end end end end
NameName Has this permission?
<%= application.name %> <%= application.description %> - <%= button_to account_application_signin_permission_path(application), - class: "govuk-button govuk-!-margin-0", - data: { module: "govuk-button" } do %> - Grant access to <%= application.name %> + <% if policy([:account, Doorkeeper::Application]).grant_signin_permission? %> + <%= button_to account_application_signin_permission_path(application), + class: "govuk-button govuk-!-margin-0", + data: { module: "govuk-button" } do %> + Grant access to <%= application.name %> + <% end %> <% end %>
- <%= link_to delete_account_application_signin_permission_path(application), - class: "govuk-button govuk-button--warning govuk-!-margin-0", - data: { module: "govuk-button" } do %> - Remove access to <%= application.name %> + <% if policy([:account, application]).remove_signin_permission? %> + <%= link_to delete_account_application_signin_permission_path(application), + class: "govuk-button govuk-button--warning govuk-!-margin-0", + data: { module: "govuk-button" } do %> + Remove access to <%= application.name %> + <% end %> <% end %>