From 8219679b1a42c974760b2ff357ba711e4e062a83 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Mon, 12 Aug 2024 15:18:08 +0100 Subject: [PATCH 1/3] Add grantable non-deleg. non-signin perms check This will be used to determine whether to show a notice to publishing managers about permissions they can't manage --- app/models/doorkeeper/application.rb | 4 ++ test/models/doorkeeper/application_test.rb | 50 ++++++++++++++++++++++ 2 files changed, 54 insertions(+) diff --git a/app/models/doorkeeper/application.rb b/app/models/doorkeeper/application.rb index 760d307cb..e6cebf5aa 100644 --- a/app/models/doorkeeper/application.rb +++ b/app/models/doorkeeper/application.rb @@ -67,6 +67,10 @@ def has_delegatable_non_signin_permissions_grantable_from_ui? (supported_permissions.delegatable.grantable_from_ui - [signin_permission]).any? end + def has_non_delegatable_non_signin_permissions_grantable_from_ui? + (supported_permissions.grantable_from_ui.where(delegatable: false) - [signin_permission]).any? + end + def url_without_path parsed_url = URI.parse(redirect_uri) "#{parsed_url.scheme}://#{parsed_url.host}:#{parsed_url.port}" diff --git a/test/models/doorkeeper/application_test.rb b/test/models/doorkeeper/application_test.rb index 99baeadf8..131a95836 100644 --- a/test/models/doorkeeper/application_test.rb +++ b/test/models/doorkeeper/application_test.rb @@ -263,6 +263,56 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase end end + context "has_non_delegatable_non_signin_permissions_grantable_from_ui?" do + should "return false if no permissions are non-delegatable" do + app = create( + :application, + with_delegatable_supported_permissions: %w[delegtable], + with_delegatable_supported_permissions_not_grantable_from_ui: %w[delegatable-non-grantable], + with_non_delegatable_supported_permissions: [], + with_non_delegatable_supported_permissions_not_grantable_from_ui: [], + ) + + assert_not app.has_non_delegatable_non_signin_permissions_grantable_from_ui? + end + + should "return false if no permissions are grantable from the UI" do + app = create( + :application, + with_delegatable_supported_permissions: [], + with_delegatable_supported_permissions_not_grantable_from_ui: %w[non-grantable], + with_non_delegatable_supported_permissions: [], + with_non_delegatable_supported_permissions_not_grantable_from_ui: %w[non-delegatable-non-grantable], + ) + + assert_not app.has_non_delegatable_non_signin_permissions_grantable_from_ui? + end + + should "return false if only the signin permission is non-delegatable and grantable from the UI" do + app = create( + :application, + with_delegatable_supported_permissions: %w[delegatable], + with_delegatable_supported_permissions_not_grantable_from_ui: %w[non-grantable], + with_non_delegatable_supported_permissions: [SupportedPermission::SIGNIN_NAME], + with_non_delegatable_supported_permissions_not_grantable_from_ui: %w[non-delegatable-non-grantable], + ) + + assert_not app.has_non_delegatable_non_signin_permissions_grantable_from_ui? + end + + should "return true if there are non-delegatable non-signin permissions grantable from the UI" do + app = create( + :application, + with_delegatable_supported_permissions: %w[delegatable], + with_delegatable_supported_permissions_not_grantable_from_ui: %w[non-grantable], + with_non_delegatable_supported_permissions: %w[yay!], + with_non_delegatable_supported_permissions_not_grantable_from_ui: %w[non-delegatable-non-grantable], + ) + + assert app.has_non_delegatable_non_signin_permissions_grantable_from_ui? + end + end + context ".all (default scope)" do setup do @app = create(:application) From 925846891aaea53ddede195ba5a714dc263ae858 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Mon, 12 Aug 2024 15:14:01 +0100 Subject: [PATCH 2/3] Add `notice_about_non_delegatable_permissions` This method will be used to let publishing managers on the edit permissions page that there are non-delegatable permissions that they are unable to see/manage on the page, but that they can still view all permissions if they'd like --- app/helpers/application_permissions_helper.rb | 21 +++++++ .../application_permissions_helper_test.rb | 57 +++++++++++++++++++ 2 files changed, 78 insertions(+) diff --git a/app/helpers/application_permissions_helper.rb b/app/helpers/application_permissions_helper.rb index 1588c8002..4be9e8351 100644 --- a/app/helpers/application_permissions_helper.rb +++ b/app/helpers/application_permissions_helper.rb @@ -22,4 +22,25 @@ def message_for_success(application_id, user = current_user) paragraph + list end + + def notice_about_non_delegatable_permissions(current_user, application, other_grantee = nil) + return nil if current_user.govuk_admin? + return nil unless application.has_non_delegatable_non_signin_permissions_grantable_from_ui? + + link = if other_grantee + link_to( + "view all the permissions #{other_grantee.name} has for #{application.name}", + user_application_permissions_path(other_grantee, application), + class: "govuk-link", + ) + else + link_to( + "view all the permissions you have for #{application.name}", + account_application_permissions_path(application), + class: "govuk-link", + ) + end + + "Below, you will only see permissions that you are authorised to manage. You can also #{link}.".html_safe + end end diff --git a/test/helpers/application_permissions_helper_test.rb b/test/helpers/application_permissions_helper_test.rb index 6205280e3..fc0cdc8d4 100644 --- a/test/helpers/application_permissions_helper_test.rb +++ b/test/helpers/application_permissions_helper_test.rb @@ -57,4 +57,61 @@ class ApplicationPermissionsHelperTest < ActionView::TestCase end end end + + context "#notice_about_non_delegatable_permissions" do + context "when the current user is a GOV.UK admin" do + setup do + @current_user = create(:user) + @current_user.expects(:govuk_admin?).returns(true) + end + + should "return nil" do + assert_nil notice_about_non_delegatable_permissions(@current_user, create(:application)) + end + end + + context "when the current user is not a GOV.UK admin" do + setup do + @current_user = create(:user, name: "Current User") + @current_user.expects(:govuk_admin?).returns(false) + @application = create(:application, name: "My First App") + end + + context "when the app has no non-delegatable non-signin permissions grantable from the UI" do + setup { @application.expects(:has_non_delegatable_non_signin_permissions_grantable_from_ui?).returns(false) } + + should "return nil" do + assert_nil notice_about_non_delegatable_permissions(@current_user, @application) + end + end + + context "when the app has some non-delegatable non-signin permissions grantable from the UI" do + setup { @application.expects(:has_non_delegatable_non_signin_permissions_grantable_from_ui?).returns(true) } + + context "without another user passed in as the grantee" do + should "infer the grantee as the current user and return a notice with a link to the account application permissions path" do + link = "view all the permissions you have for My First App" + + assert_equal( + "Below, you will only see permissions that you are authorised to manage. You can also #{link}.", + notice_about_non_delegatable_permissions(@current_user, @application), + ) + end + end + + context "with another user passed in as the grantee" do + should "return a notice with a link to the user application permissions path for the given grantee" do + grantee = create(:user, name: "Another User") + + link = "view all the permissions Another User has for My First App" + + assert_equal( + "Below, you will only see permissions that you are authorised to manage. You can also #{link}.", + notice_about_non_delegatable_permissions(@current_user, @application, grantee), + ) + end + end + end + end + end end From 2a8436cf3ce580fc83c6500b9b0ee921cf2a793c Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Mon, 12 Aug 2024 15:30:03 +0100 Subject: [PATCH 3/3] Show notice when there are non-delegatable perms This will be shown to publishing managers on the edit permissions page when there are non-delegatable non-signin permissions so that they are aware that they aren't seeing all permissions for the given app --- app/controllers/account/permissions_controller.rb | 2 ++ app/controllers/users/permissions_controller.rb | 2 ++ app/views/account/permissions/edit.html.erb | 8 ++++++++ app/views/users/permissions/edit.html.erb | 8 ++++++++ test/integration/account_applications_test.rb | 2 ++ test/integration/granting_permissions_test.rb | 4 ++++ 6 files changed, 26 insertions(+) diff --git a/app/controllers/account/permissions_controller.rb b/app/controllers/account/permissions_controller.rb index 9626c9835..1bf11d95c 100644 --- a/app/controllers/account/permissions_controller.rb +++ b/app/controllers/account/permissions_controller.rb @@ -3,6 +3,8 @@ class Account::PermissionsController < ApplicationController before_action :set_application before_action :set_permissions, only: %i[edit update] + include ApplicationPermissionsHelper + def show authorize [:account, @application], :view_permissions? diff --git a/app/controllers/users/permissions_controller.rb b/app/controllers/users/permissions_controller.rb index 44bea6b2d..562b9607a 100644 --- a/app/controllers/users/permissions_controller.rb +++ b/app/controllers/users/permissions_controller.rb @@ -4,6 +4,8 @@ class Users::PermissionsController < ApplicationController before_action :set_application before_action :set_permissions, only: %i[edit update] + include ApplicationPermissionsHelper + def show authorize [{ application: @application, user: @user }], :view_permissions?, policy_class: Users::ApplicationPolicy diff --git a/app/views/account/permissions/edit.html.erb b/app/views/account/permissions/edit.html.erb index 91029a119..d0af236fb 100644 --- a/app/views/account/permissions/edit.html.erb +++ b/app/views/account/permissions/edit.html.erb @@ -29,6 +29,14 @@ <% end %> <% end %> +<% if notice = notice_about_non_delegatable_permissions(current_user, @application, @user) %> +
+
+ <%= render "govuk_publishing_components/components/inset_text", { text: notice } %> +
+
+<% end %> + <%= render "shared/permissions_forms", { assigned_permissions: @assigned_permissions, unassigned_permission_options: @unassigned_permission_options, diff --git a/app/views/users/permissions/edit.html.erb b/app/views/users/permissions/edit.html.erb index 2b0c21253..7d5f59b81 100644 --- a/app/views/users/permissions/edit.html.erb +++ b/app/views/users/permissions/edit.html.erb @@ -34,6 +34,14 @@ <% end %> <% end %> +<% if notice = notice_about_non_delegatable_permissions(current_user, @application, @user) %> +
+
+ <%= render "govuk_publishing_components/components/inset_text", { text: notice } %> +
+
+<% end %> + <%= render "shared/permissions_forms", { assigned_permissions: @assigned_permissions, unassigned_permission_options: @unassigned_permission_options, diff --git a/test/integration/account_applications_test.rb b/test/integration/account_applications_test.rb index bd681e8dd..fe3765977 100644 --- a/test/integration/account_applications_test.rb +++ b/test/integration/account_applications_test.rb @@ -252,6 +252,8 @@ class AccountApplicationsTest < ActionDispatch::IntegrationTest assert page.has_field?("delegatable_perm") assert page.has_no_field?("non_delegatable_perm") + + assert_selector ".govuk-inset-text", text: "Below, you will only see permissions that you are authorised to manage. You can also view all the permissions you have for app-name." end should "not be able to grant permissions that are not grantable_from_ui" do diff --git a/test/integration/granting_permissions_test.rb b/test/integration/granting_permissions_test.rb index 85292b6ff..cf578e6fb 100644 --- a/test/integration/granting_permissions_test.rb +++ b/test/integration/granting_permissions_test.rb @@ -225,6 +225,8 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest assert page.has_field?("delegatable_perm") assert page.has_no_field?("non_delegatable_perm") + + assert_selector ".govuk-inset-text", text: "Below, you will only see permissions that you are authorised to manage. You can also view all the permissions #{@user.name} has for MyApp." end should "not be able to grant permissions that are not grantable_from_ui" do @@ -322,6 +324,8 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest assert page.has_field?("delegatable_perm") assert page.has_no_field?("non_delegatable_perm") + + assert_selector ".govuk-inset-text", text: "Below, you will only see permissions that you are authorised to manage. You can also view all the permissions #{@user.name} has for MyApp." end should "not be able to grant permissions that are not grantable_from_ui" do