From fe9cd78001a2fe648c7032cf636530e0cd5ce4a2 Mon Sep 17 00:00:00 2001 From: Ynda Jas Date: Thu, 27 Jun 2024 18:47:59 +0100 Subject: [PATCH] Show view permissions link even when updatable Previously on the applications pages the view permissions link wouldn't show if the user could edit permissions. However, we'll soon fix delegatable permissions, which will mean that publishing managers will only see delegatable permissions on the update permissions page. It would be useful for them to be able to see all permissions, even if they can't grant non-delegatable ones, so we should show the view link regardless of update authorisation --- app/helpers/application_table_helper.rb | 25 +++++----- app/views/account/applications/index.html.erb | 2 +- app/views/users/applications/index.html.erb | 2 +- docs/access_and_permissions.md | 22 ++++----- .../account/applications_controller_test.rb | 34 +++++++++++--- .../users/applications_controller_test.rb | 13 ++--- test/helpers/application_table_helper_test.rb | 47 ++++++++++++++----- 7 files changed, 95 insertions(+), 50 deletions(-) diff --git a/app/helpers/application_table_helper.rb b/app/helpers/application_table_helper.rb index b9d7ba3d35..b36fafd561 100644 --- a/app/helpers/application_table_helper.rb +++ b/app/helpers/application_table_helper.rb @@ -37,22 +37,23 @@ def users_applications_remove_access_link(application, user) end end - def account_applications_permissions_link(application) - if policy([:account, application]).edit_permissions? - update_permissions_link(application) - elsif policy([:account, application]).view_permissions? - view_permissions_link(application) - else - "" - end + def account_applications_permissions_links(application) + links = [] + + links << view_permissions_link(application) if policy([:account, application]).view_permissions? + links << update_permissions_link(application) if policy([:account, application]).edit_permissions? + + safe_join(links) end - def users_applications_permissions_link(application, user) + def users_applications_permissions_links(application, user) + links = [view_permissions_link(application, user)] + if policy(UserApplicationPermission.for(user:, supported_permission: application.signin_permission)).edit? - update_permissions_link(application, user) - else - view_permissions_link(application, user) + links << update_permissions_link(application, user) end + + safe_join(links) end def api_users_applications_permissions_link(application, user) diff --git a/app/views/account/applications/index.html.erb b/app/views/account/applications/index.html.erb index 729f6d9030..e8623ec645 100644 --- a/app/views/account/applications/index.html.erb +++ b/app/views/account/applications/index.html.erb @@ -37,7 +37,7 @@ { text: application.name }, { text: application.description }, { text: wrap_links_in_actions_markup([ - account_applications_permissions_link(application), + account_applications_permissions_links(application), account_applications_remove_access_link(application) ]) }, diff --git a/app/views/users/applications/index.html.erb b/app/views/users/applications/index.html.erb index b2f80df6c1..5f63b19d97 100644 --- a/app/views/users/applications/index.html.erb +++ b/app/views/users/applications/index.html.erb @@ -42,7 +42,7 @@ { text: application.name }, { text: application.description }, { text: wrap_links_in_actions_markup([ - users_applications_permissions_link(application, @user), + users_applications_permissions_links(application, @user), users_applications_remove_access_link(application, @user) ]) }, diff --git a/docs/access_and_permissions.md b/docs/access_and_permissions.md index 2e73fef6e7..2e16a55a75 100644 --- a/docs/access_and_permissions.md +++ b/docs/access_and_permissions.md @@ -29,16 +29,16 @@ In this section, the granter and grantee are the same user: this is about managi | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions | |-------------------------|--------------|---------------|------------------|------------------| -| None | ✅ | ✅ | ✅ | ❌ | -| `signin` | ✅ | ✅ | ✅ | ❌ | -| Another permission | ✅ | ✅ | ✅ | ❌ | +| None | ✅ | ✅ | ✅ | ✅ | +| `signin` | ✅ | ✅ | ✅ | ✅ | +| Another permission | ✅ | ✅ | ✅ | ✅ | #### As a publishing manager | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions | |-------------------------|--------------|---------------|------------------|------------------| | None | ❌ | ❌ | ❌ | ✅ | -| `signin` | ❌ | ✅ | ✅ | ❌ | +| `signin` | ❌ | ✅ | ✅ | ✅ | | Another permission | ❌ | ❌ | ❌ | ✅ | ### Dependencies by route @@ -131,17 +131,17 @@ In this section, the granter and grantee are different users: this is about mana | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions | |-------------------------|--------------|---------------|------------------|------------------| -| None | ✅ | ✅ | ✅ | ❌ | -| `signin` | ✅ | ✅ | ✅ | ❌ | -| Another permission | ✅ | ✅ | ✅ | ❌ | +| None | ✅ | ✅ | ✅ | ✅ | +| `signin` | ✅ | ✅ | ✅ | ✅ | +| Another permission | ✅ | ✅ | ✅ | ✅ | ##### Without access to the app | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions | |-------------------------|--------------|---------------|------------------|------------------| -| None | ✅ | ✅ | ✅ | ❌ | -| `signin` | ✅ | ✅ | ✅ | ❌ | -| Another permission | ✅ | ✅ | ✅ | ❌ | +| None | ✅ | ✅ | ✅ | ✅ | +| `signin` | ✅ | ✅ | ✅ | ✅ | +| Another permission | ✅ | ✅ | ✅ | ✅ | #### As a publishing manager @@ -150,7 +150,7 @@ In this section, the granter and grantee are different users: this is about mana | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions | |-------------------------|--------------|---------------|------------------|------------------| | None | ❌ | ❌ | ❌ | ✅ | -| `signin` | ✅ | ✅ | ✅ | ❌ | +| `signin` | ✅ | ✅ | ✅ | ✅ | | Another permission | ❌ | ❌ | ❌ | ✅ | ##### Without access to the app diff --git a/test/controllers/account/applications_controller_test.rb b/test/controllers/account/applications_controller_test.rb index 91ba528750..36f17f69e6 100644 --- a/test/controllers/account/applications_controller_test.rb +++ b/test/controllers/account/applications_controller_test.rb @@ -126,28 +126,48 @@ class Account::ApplicationsControllerTest < ActionController::TestCase end context "editing permissions" do - should "not display any permissions links when the app only has the signin permission" do - stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true + context "when the app only has the signin permission" do + should "only display a link to view permissions when authorised to view or edit permissions" do + stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true - get :index + get :index + + assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0 + assert_select "a[href='#{account_application_permissions_path(@application)}']" + end + + should "not display links to view or edit permissions when not authorised to view permissions" do + stub_policy @user, [:account, @application], view_permissions?: false, edit_permissions?: true + + get :index - assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0 - assert_select "a[href='#{account_application_permissions_path(@application)}']", count: 0 + assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0 + assert_select "a[href='#{account_application_permissions_path(@application)}']", count: 0 + end end context "when the app has non-signin permissions" do setup { create(:supported_permission, application: @application) } - should "display a link to edit permissions when authorised to edit permissions" do + should "display links to view and edit permissions when authorised to view and edit permissions" do stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true get :index + assert_select "a[href='#{edit_account_application_permissions_path(@application)}']" + assert_select "a[href='#{account_application_permissions_path(@application)}']" + end + + should "only display a link to edit permissions when authorised to edit but not view permissions" do + stub_policy @user, [:account, @application], view_permissions?: false, edit_permissions?: true + + get :index + assert_select "a[href='#{edit_account_application_permissions_path(@application)}']" assert_select "a[href='#{account_application_permissions_path(@application)}']", count: 0 end - should "display a link to view permissions when not authorised to view but not edit permissions" do + should "only display a link to view permissions when not authorised to edit permissions" do stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: false get :index diff --git a/test/controllers/users/applications_controller_test.rb b/test/controllers/users/applications_controller_test.rb index 5e39ebf586..0ac45f0ba7 100644 --- a/test/controllers/users/applications_controller_test.rb +++ b/test/controllers/users/applications_controller_test.rb @@ -136,33 +136,34 @@ class Users::ApplicationsControllerTest < ActionController::TestCase context "editing permissions" do setup { @user_application_permission = stub_user_application_permission(@user, @application) } - should "not display any permissions links when the app only has the signin permission" do + should "only display a link to view permissions when the app only has the signin permission" do stub_policy @current_user, @user_application_permission, edit?: true get :index, params: { user_id: @user } - assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", count: 0 + assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", text: "Update permissions for app-name", count: 0 + assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name" end context "when the app has non-signin permissions" do setup { create(:supported_permission, application: @application) } - should "display a link to edit permissions when authorised to edit permissions" do + should "display links to view and edit permissions when authorised to edit permissions" do stub_policy @current_user, @user_application_permission, edit?: true get :index, params: { user_id: @user } assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", text: "Update permissions for app-name" - assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name", count: 0 + assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name" end - should "display a link to view permissions when not authorised to edit permissions" do + should "only display a link to view permissions when not authorised to edit permissions" do stub_policy @current_user, @user_application_permission, edit?: false get :index, params: { user_id: @user } - assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name" assert_select "a[href='#{edit_user_application_permissions_path(@user, @application)}']", text: "Update permissions for app-name", count: 0 + assert_select "a[href='#{user_application_permissions_path(@user, @application)}']", text: "View permissions for app-name" end end end diff --git a/test/helpers/application_table_helper_test.rb b/test/helpers/application_table_helper_test.rb index 9e05952262..ea9cfd7336 100644 --- a/test/helpers/application_table_helper_test.rb +++ b/test/helpers/application_table_helper_test.rb @@ -97,47 +97,70 @@ class ApplicationTableHelperTest < ActionView::TestCase end end - context "#account_applications_permissions_link" do + context "#account_applications_permissions_links" do setup do @user = build(:user) stubs(:current_user).returns(@user) @application = create(:application, with_supported_permissions: %w[permission]) end - should "generate an update link when the user can edit permissions" do - stub_policy @user, [:account, @application], edit_permissions?: true - assert_includes account_applications_permissions_link(@application), "Update permissions" + should "generate both view and update links when the user can both view and edit permissions" do + stub_policy @user, [:account, @application], view_permissions?: true, edit_permissions?: true + + result = account_applications_permissions_links(@application) + + assert_includes result, "View permissions" + assert_includes result, "Update permissions" end - should "generate a view link when the user can view permissions" do + should "only generate a view link when the user can only view permissions" do stub_policy @user, [:account, @application], view_permissions?: true - assert_includes account_applications_permissions_link(@application), "View permissions" + + result = account_applications_permissions_links(@application) + + assert_includes result, "View permissions" + assert_not_includes result, "Update permissions" + end + + should "only generate an update link when the user can only edit permissions" do + stub_policy @user, [:account, @application], edit_permissions?: true + + result = account_applications_permissions_links(@application) + + assert_not_includes result, "View permissions" + assert_includes result, "Update permissions" end should "return an empty string when the user can do neither" do stub_policy @user, [:account, @application] - assert account_applications_permissions_link(@application).empty? + assert account_applications_permissions_links(@application).empty? end end - context "#users_applications_permissions_link" do + context "#users_applications_permissions_links" do setup do @application = create(:application, with_supported_permissions: %w[permission]) @grantee = create(:user) end - should "generate an update link when the user can edit permissions" do + should "generate both view and update links when the user can edit permissions" do granter = create(:superadmin_user) stubs(:current_user).returns(granter) - assert_includes users_applications_permissions_link(@application, @grantee), "Update permissions" + result = users_applications_permissions_links(@application, @grantee) + + assert_includes result, "View permissions" + assert_includes result, "Update permissions" end - should "generate a view link when the user cannot edit permissions" do + should "only generate a view link when the user cannot edit permissions" do granter = create(:user) stubs(:current_user).returns(granter) - assert_includes users_applications_permissions_link(@application, @grantee), "View permissions" + result = users_applications_permissions_links(@application, @grantee) + + assert_includes result, "View permissions" + assert_not_includes result, "Update permissions" end end