Fix delegatable permissions

app/controllers/account/permissions_controller.rb

@@ -14,6 +14,11 @@ def show
   def edit
     authorize [:account, @application], :edit_permissions?
 
+    if @permissions.empty?
+      flash[:alert] = "No permissions found for #{@application.name} that you are authorised to manage."
+      return redirect_to account_applications_path
+    end
+
     @shared_permissions_form_locals = {
       action: account_application_permissions_path(@application),
       application: @application,
@@ -79,6 +84,10 @@ def set_application
   end
 
   def set_permissions
-    @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
+    if current_user.govuk_admin?
+      @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
+    elsif current_user.publishing_manager?
+      @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false, only_delegatable: true)
+    end
   end
 end

app/controllers/users/permissions_controller.rb

@@ -15,6 +15,11 @@ def show
   def edit
     authorize [{ application: @application, user: @user }], :edit_permissions?, policy_class: Users::ApplicationPolicy
 
+    if @permissions.empty?
+      flash[:alert] = "No permissions found for #{@application.name} that you are authorised to manage."
+      return redirect_to user_applications_path(@user)
+    end
+
     @shared_permissions_form_locals = {
       action: user_application_permissions_path(@user, @application),
       application: @application,
@@ -84,6 +89,10 @@ def set_application
   end
 
   def set_permissions
-    @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
+    if current_user.govuk_admin?
+      @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
+    elsif current_user.publishing_manager?
+      @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false, only_delegatable: true)
+    end
   end
 end

app/helpers/application_table_helper.rb

@@ -101,7 +101,11 @@ def view_permissions_link(application, user = nil)
   end
 
   def update_permissions_link(application, user = nil)
-    return "" if application.sorted_supported_permissions_grantable_from_ui(include_signin: false).none?
+    if current_user.govuk_admin?
+      return "" unless application.has_non_signin_permissions_grantable_from_ui?
+    elsif current_user.publishing_manager?
+      return "" unless application.has_delegatable_non_signin_permissions_grantable_from_ui?
+    end
 
     path = if user.nil?
              edit_account_application_permissions_path(application)

app/models/doorkeeper/application.rb

@@ -42,17 +42,31 @@ def signin_permission
     supported_permissions.signin.first
   end
 
-  def sorted_supported_permissions_grantable_from_ui(include_signin: true)
-    sorted_permissions = supported_permissions.grantable_from_ui.order(:name)
+  def sorted_supported_permissions_grantable_from_ui(include_signin: true, only_delegatable: false)
+    sorted_permissions = if only_delegatable
+                           supported_permissions.grantable_from_ui.delegatable.order(:name)
+                         else
+                           supported_permissions.grantable_from_ui.order(:name)
+                         end
     sorted_permissions_without_signin = sorted_permissions - [signin_permission]
 
-    if include_signin
-      ([signin_permission] + sorted_permissions_without_signin).compact
-    else
+    return sorted_permissions_without_signin unless include_signin
+
+    if only_delegatable && !signin_permission.delegatable
       sorted_permissions_without_signin
+    else
+      ([signin_permission] + sorted_permissions_without_signin).compact
     end
   end
 
+  def has_non_signin_permissions_grantable_from_ui?
+    (supported_permissions.grantable_from_ui - [signin_permission]).any?
+  end
+
+  def has_delegatable_non_signin_permissions_grantable_from_ui?
+    (supported_permissions.delegatable.grantable_from_ui - [signin_permission]).any?
+  end
+
   def url_without_path
     parsed_url = URI.parse(redirect_uri)
     "#{parsed_url.scheme}://#{parsed_url.host}:#{parsed_url.port}"

app/policies/account/application_policy.rb

@@ -17,5 +17,8 @@ def remove_signin_permission?
         current_user.publishing_manager? && record.signin_permission.delegatable?
       )
   end
-  alias_method :edit_permissions?, :remove_signin_permission?
+
+  def edit_permissions?
+    current_user.has_access_to?(record) && (current_user.govuk_admin? || current_user.publishing_manager?)
+  end
 end

app/policies/supported_permission_policy.rb

@@ -4,7 +4,10 @@ def resolve
       if current_user.govuk_admin?
         scope.all
       elsif current_user.publishing_manager?
-        scope.joins(:application).where(oauth_applications: { id: publishing_manager_manageable_application_ids })
+        scope
+          .delegatable
+          .joins(:application)
+          .where(oauth_applications: { id: publishing_manager_manageable_application_ids })
       else
         scope.none
       end
@@ -17,7 +20,6 @@ def publishing_manager_manageable_application_ids
         .not_api_only
         .includes(:supported_permissions)
         .can_signin(current_user)
-        .with_signin_delegatable
         .pluck(:id)
     end
   end

app/policies/users/application_policy.rb

@@ -23,5 +23,11 @@ def grant_signin_permission?
   end
 
   alias_method :remove_signin_permission?, :grant_signin_permission?
-  alias_method :edit_permissions?, :grant_signin_permission?
+
+  def edit_permissions?
+    return false unless Pundit.policy(current_user, user).edit?
+    return true if current_user.govuk_admin?
+
+    current_user.publishing_manager? && current_user.has_access_to?(application)
+  end
 end

docs/access_and_permissions.md

@@ -38,8 +38,10 @@ 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      | ❌            | ❌             | ❌                | ✅                |
+| `signin`                | ❌            | ✅             | ❌                | ✅                |
+| Another permission      | ❌            | ❌             | ✅*               | ✅                |
+
+\* only delegatable non-signin permissions
 
 ### Dependencies by route
 
@@ -98,8 +100,8 @@ These dependencies determine whether a user can:
 
 ```mermaid
 flowchart TD
-    A(Account::PermissionsController#edit) --authorize [:account, @application], :edit_permissions?--> B(Account::ApplicationPolicy#edit_permissions?)
-    A --@application.sorted_supported_permissions_grantable_from_ui(include_signin: false)--> C(Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui)
+    A(Account::PermissionsController#edit) --@application.sorted_supported_permissions_grantable_from_ui( [...] )--> B(Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui)
+    A --authorize [:account, @application], :edit_permissions?--> C(Account::ApplicationPolicy#edit_permissions?) 
 ```
 
 #### Account permissions update
@@ -111,11 +113,12 @@ These dependencies determine whether a user can:
 
 ```mermaid
 flowchart TD
-    A(Account::PermissionsController#update) --authorize [:account, @application], :edit_permissions?--> B(Account::ApplicationPolicy#edit_permissions?)
-    A --UserUpdatePermissionBuilder.new( [...] ).build--> C(UserUpdatePermissionBuilder#build)
-    A --UserUpdate.new(current_user, { supported_permission_ids: }, current_user, user_ip_address).call--> D(UserUpdate#call)
-    D --SupportedPermissionParameterFilter.new(current_user, user, user_params) [...] .filtered_supported_permission_ids--> E(SupportedPermissionParameterFilter#filtered_supported_permission_ids)
-    E --Pundit.policy_scope(current_user, SupportedPermission)--> F("SupportedPermissionPolicy (scope)")
+    A(Account::PermissionsController#update) [email protected]_supported_permissions_grantable_from_ui( [...] )--> B(Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui)
+    A --authorize [:account, @application], :edit_permissions?--> C(Account::ApplicationPolicy#edit_permissions?)
+    A --UserUpdatePermissionBuilder.new( [...] ).build--> D(UserUpdatePermissionBuilder#build)
+    A --UserUpdate.new(current_user, { supported_permission_ids: }, current_user, user_ip_address).call--> E(UserUpdate#call)
+    E --SupportedPermissionParameterFilter.new(current_user, user, user_params) [...] .filtered_supported_permission_ids--> F(SupportedPermissionParameterFilter#filtered_supported_permission_ids)
+    D --Pundit.policy_scope(current_user, SupportedPermission)--> G("SupportedPermissionPolicy (scope)")
 ```
 
 ## For another existing user
@@ -126,15 +129,7 @@ In this section, the granter and grantee are different users: this is about mana
 
 #### As a GOV.UK admin
 
-##### With access to the app
-
-| Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions |
-|-------------------------|--------------|---------------|------------------|------------------|
-| None                    | ✅            | ✅             | ✅                | ✅                |
-| `signin`                | ✅            | ✅             | ✅                | ✅                |
-| Another permission      | ✅            | ✅             | ✅                | ✅                |
-
-##### Without access to the app
+##### With or without access to the app
 
 | Delegatable permissions | Grant access | Revoke access | Edit permissions | View permissions |
 |-------------------------|--------------|---------------|------------------|------------------|
@@ -149,8 +144,10 @@ 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      | ❌            | ❌             | ❌                | ✅                |
+| `signin`                | ✅            | ✅             | ❌                | ✅                |
+| Another permission      | ❌            | ❌             | ✅*               | ✅                |
+
+\* only delegatable non-signin permissions
 
 ##### Without access to the app
 
@@ -224,9 +221,9 @@ These dependencies determine whether a user can:
 
 ```mermaid
 flowchart TD
-    A(Users::PermissionsController#edit) --authorize [{ application: @application, user: @user }], :edit_permissions?, policy_class: Users::ApplicationPolicy--> B(Users::ApplicationPolicy#edit_permissions?)
-    B --Pundit.policy(current_user, user).edit?--> C(UserPolicy#edit?)
-    A --@application.sorted_supported_permissions_grantable_from_ui(include_signin: false)--> D(Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui)
+    A(Users::PermissionsController#edit) --@application.sorted_supported_permissions_grantable_from_ui( [...] )--> B(Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui)
+    A --authorize [{ application: @application, user: @user }], :edit_permissions?, policy_class: Users::ApplicationPolicy--> C(Users::ApplicationPolicy#edit_permissions?)
+    C --Pundit.policy(current_user, user).edit?--> D(UserPolicy#edit?)
 ```
 
 #### Users permissions update
@@ -238,12 +235,13 @@ These dependencies determine whether a user can:
 
 ```mermaid
 flowchart TD
-    A(Users::PermissionsController#update) --authorize [{ application: @application, user: @user }], :edit_permissions?, policy_class: Users::ApplicationPolicy--> B(Users::ApplicationPolicy#edit_permissions?)
-    B --Pundit.policy(current_user, user).edit?--> C(UserPolicy#edit?)
-    A --UserUpdatePermissionBuilder.new( [...] ).build--> D(UserUpdatePermissionBuilder#build)
-    A --UserUpdate.new(@user, { supported_permission_ids: }, current_user, user_ip_address).call--> E(UserUpdate#call)
-    E --SupportedPermissionParameterFilter.new(current_user, user, user_params) [...] .filtered_supported_permission_ids--> F(SupportedPermissionParameterFilter#filtered_supported_permission_ids)
-    F --Pundit.policy_scope(current_user, SupportedPermission)--> G("SupportedPermissionPolicy (scope)")
+    A(Users::PermissionsController#update) [email protected]_supported_permissions_grantable_from_ui( [...] )--> B(Doorkeeper::Application#sorted_supported_permissions_grantable_from_ui)
+    A --authorize [{ application: @application, user: @user }], :edit_permissions?, policy_class: Users::ApplicationPolicy--> C(Users::ApplicationPolicy#edit_permissions?)
+    C --Pundit.policy(current_user, user).edit?--> D(UserPolicy#edit?)
+    A --UserUpdatePermissionBuilder.new( [...] ).build--> E(UserUpdatePermissionBuilder#build)
+    A --UserUpdate.new(@user, { supported_permission_ids: }, current_user, user_ip_address).call--> F(UserUpdate#call)
+    F --SupportedPermissionParameterFilter.new(current_user, user, user_params) [...] .filtered_supported_permission_ids--> G(SupportedPermissionParameterFilter#filtered_supported_permission_ids)
+    G --Pundit.policy_scope(current_user, SupportedPermission)--> H("SupportedPermissionPolicy (scope)")
 ```
 
 ## For a new user

test/controllers/account/applications_controller_test.rb

@@ -127,22 +127,28 @@ class Account::ApplicationsControllerTest < ActionController::TestCase
 
         context "editing permissions" do
           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
+            %w[govuk_admin publishing_manager].each do |role_group|
+              context "as a #{role_group}" do
+                setup { @user.stubs(:"#{role_group}?").returns(true) }
 
-              get :index
+                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
 
-              assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0
-              assert_select "a[href='#{account_application_permissions_path(@application)}']"
-            end
+                  get :index
 
-            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
+                  assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0
+                  assert_select "a[href='#{account_application_permissions_path(@application)}']"
+                end
 
-              get :index
+                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
 
-              assert_select "a[href='#{edit_account_application_permissions_path(@application)}']", count: 0
-              assert_select "a[href='#{account_application_permissions_path(@application)}']", count: 0
+                  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
+                end
+              end
             end
           end