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