Skip to content

Commit

Permalink
Show view permissions link even when updatable
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@yndajas
yndajas committed Jul 12, 2024
1 parent fa1d788 commit fe9cd78
Show file tree
Hide file tree
Showing 7 changed files with 95 additions and 50 deletions.
25 changes: 13 additions & 12 deletions app/helpers/application_table_helper.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion app/views/account/applications/index.html.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
])
},
Expand Down
2 changes: 1 addition & 1 deletion app/views/users/applications/index.html.erb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
])
},
Expand Down
22 changes: 11 additions & 11 deletions docs/access_and_permissions.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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

Expand All @@ -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
Expand Down
34 changes: 27 additions & 7 deletions test/controllers/account/applications_controller_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
13 changes: 7 additions & 6 deletions test/controllers/users/applications_controller_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
47 changes: 35 additions & 12 deletions test/helpers/application_table_helper_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down

0 comments on commit fe9cd78

Please sign in to comment.