Fix delegatable permissions

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]
 
+  # make equivalent changes in this and related files as in the users permissions controller
+
   def show
     authorize [:account, @application], :view_permissions?
 

app/controllers/users/applications_controller.rb

@@ -3,14 +3,14 @@ class Users::ApplicationsController < ApplicationController
 
   def show
     user = User.find(params[:user_id])
-    authorize user, :edit?
+    authorize user, :edit? # move this to applications policy but defer to this policy there (similar to what UserApplicationPermissionPolicy is doing, but more in the form of the Account::ApplicationPolicy)?
 
     redirect_to user_applications_path(user)
   end
 
   def index
     @user = User.find(params[:user_id])
-    authorize @user, :edit?
+    authorize @user, :edit? # move this to applications policy but defer to this policy there (similar to what UserApplicationPermissionPolicy is doing, but more in the form of the Account::ApplicationPolicy)?
 
     @applications_with_signin = Doorkeeper::Application.not_api_only.can_signin(@user)
     @applications_without_signin = Doorkeeper::Application.not_api_only.without_signin_permission_for(@user)

app/controllers/users/permissions_controller.rb

@@ -58,6 +58,11 @@ def update
       selected_permission_ids: selected_permission_ids.map(&:to_i),
     ).build
 
+    # I think the `SupportedPermissionParameterFilter` in combination with the
+    # change to the `SupportedPermissionPolicy` will prevent removing
+    # non-delegatable permissions for publishing managers, so there may not need
+    # to be any further changes to this file, but we'll need to test this carefully
+
     UserUpdate.new(@user, { supported_permission_ids: }, current_user, user_ip_address).call
 
     if update_params[:add_more] == "true"
@@ -84,6 +89,11 @@ def set_application
   end
 
   def set_permissions
-    @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
+    # is this the best place to do this?
+    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/models/doorkeeper/application.rb

@@ -42,8 +42,12 @@ 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
@@ -53,6 +57,10 @@ def sorted_supported_permissions_grantable_from_ui(include_signin: true)
     end
   end
 
+  def has_delegatable_non_signin_permissions?
+    (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,11 @@ def remove_signin_permission?
         current_user.publishing_manager? && record.signin_permission.delegatable?
       )
   end
-  alias_method :edit_permissions?, :remove_signin_permission?
+
+  def edit_permissions?
+    return false unless current_user.has_access_to?(record)
+    return true if current_user.govuk_admin?
+
+    current_user.publishing_manager? && record.has_delegatable_non_signin_permissions?
+  end
 end

app/policies/supported_permission_policy.rb

@@ -5,7 +5,7 @@ def resolve
         scope.all
       elsif current_user.publishing_manager?
         app_ids = Pundit.policy_scope(current_user, :user_permission_manageable_application).pluck(:id)
-        scope.joins(:application).where(oauth_applications: { id: app_ids })
+        scope.joins(:application).delegatable.where(oauth_applications: { id: app_ids })
       else
         scope.none
       end

app/policies/user_permission_manageable_application_policy.rb

@@ -22,6 +22,14 @@ def resolve
       if current_user.govuk_admin?
         applications
       elsif current_user.publishing_manager?
+        # this policy is used in a few places - I think this might need further research
+        # to determine if it's being used correctly and if anything needs changing.
+        # On initial inspection, it looks like another area where the signin
+        # permission is too powerful/impactful and I'd expect
+        # `applications.can_signin(current_user)` to be more accurate here, with
+        # permissions-related filters applied elsewhere, e.g. the SupportedPermissionPolicy
+        # The change in the SupportedPermissionPolicy to filter by delegatable
+        # might be enough for our purposes though?
         applications.can_signin(current_user).with_signin_delegatable
       else
         applications.none

app/policies/users/application_policy.rb

@@ -16,9 +16,15 @@ def grant_signin_permission?
   end
 
   alias_method :remove_signin_permission?, :grant_signin_permission?
-  alias_method :edit_permissions?, :grant_signin_permission?
 
   def view_permissions?
     Pundit.policy(current_user, user).edit?
   end
+
+  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) && application.has_delegatable_non_signin_permissions?
+  end
 end

app/views/account/permissions/edit.html.erb

@@ -29,6 +29,9 @@
   <% end %>
 <% end %>
 
+<%# add a note here for publishing managers about the filtered list %>
+<%# link to view permissions page %>
+
 <%= render "shared/permissions_forms", {
   assigned_permissions: @assigned_permissions,
   unassigned_permission_options: @unassigned_permission_options,

app/views/users/permissions/edit.html.erb

@@ -34,6 +34,9 @@
   <% end %>
 <% end %>
 
+<%# add a note here for publishing managers about the filtered list %>
+<%# link to view permissions page %>
+
 <%= render "shared/permissions_forms", {
   assigned_permissions: @assigned_permissions,
   unassigned_permission_options: @unassigned_permission_options,

docs/access_and_permissions.md

@@ -38,8 +38,8 @@ 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      | ❌            | ❌             | ✅                | ✅                |
 
 ### Dependencies by route
 
@@ -150,8 +150,8 @@ 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      | ❌            | ❌             | ✅                | ✅                |
 
 ##### Without access to the app
 

test/integration/account_applications_test.rb

@@ -160,7 +160,8 @@ class AccountApplicationsTest < ActionDispatch::IntegrationTest
     context "as a #{user_role}" do
       should "support granting self app-specific permissions" do
         user = create(:"#{user_role}_user")
-        application = create(:application, name: "app-name", description: "app-description", with_non_delegatable_supported_permissions: %w[perm1 perm2])
+        # do we need a test for delegability here?
+        application = create(:application, name: "app-name", description: "app-description", with_delegatable_supported_permissions: %w[perm1 perm2])
         application.signin_permission.update!(delegatable: true)
         user.grant_application_signin_permission(application)
         user.grant_application_permission(application, "perm1")

test/integration/granting_permissions_test.rb

@@ -40,6 +40,37 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest
       assert_not_includes @user.permissions_for(app), "never"
     end
 
+    should "be able to manage permissions when there are no delegatable permissions" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+
+      assert_selector ".govuk-link", text: "Update permissions for MyApp"
+    end
+
+    should "be able to grant delegatable and non-delegatable permissions" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_delegatable_supported_permissions: %w[delegatable_perm],
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+      click_link "Update permissions for MyApp"
+
+      assert page.has_field?("delegatable_perm")
+      assert page.has_field?("non_delegatable_perm")
+    end
+
     should "not be able to grant permissions that are not grantable_from_ui" do
       app = create(
         :application,
@@ -97,6 +128,37 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest
       assert_not_includes @user.permissions_for(app), "never"
     end
 
+    should "be able to manage permissions when there are no delegatable permissions" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+
+      assert_selector ".govuk-link", text: "Update permissions for MyApp"
+    end
+
+    should "be able to grant delegatable and non-delegatable permissions" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_delegatable_supported_permissions: %w[delegatable_perm],
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+      click_link "Update permissions for MyApp"
+
+      assert page.has_field?("delegatable_perm")
+      assert page.has_field?("non_delegatable_perm")
+    end
+
     should "not be able to grant permissions that are not grantable_from_ui" do
       app = create(
         :application,
@@ -175,6 +237,38 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest
       assert_not_includes @user.permissions_for(app), "never"
     end
 
+    should "not be able to manage permissions when there are no delegatable permissions" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @super_org_admin.grant_application_signin_permission(app)
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+      assert_no_selector ".govuk-link", text: "Update permissions for MyApp"
+    end
+
+    should "not be able to grant permissions that are non-delegatable" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_delegatable_supported_permissions: %w[delegatable_perm],
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @super_org_admin.grant_application_signin_permission(app)
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+      click_link "Update permissions for MyApp"
+
+      assert page.has_field?("delegatable_perm")
+      assert page.has_no_field?("non_delegatable_perm")
+    end
+
     should "not be able to grant permissions that are not grantable_from_ui" do
       app = create(
         :application,
@@ -254,6 +348,38 @@ class GrantingPermissionsTest < ActionDispatch::IntegrationTest
       assert_not_includes @user.permissions_for(app), "never"
     end
 
+    should "not be able to manage permissions when there are no delegatable permissions" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @organisation_admin.grant_application_signin_permission(app)
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+      assert_no_selector ".govuk-link", text: "Update permissions for MyApp"
+    end
+
+    should "not be able to grant permissions that are non-delegatable" do
+      app = create(
+        :application,
+        name: "MyApp",
+        with_delegatable_supported_permissions: %w[delegatable_perm],
+        with_non_delegatable_supported_permissions: %w[non_delegatable_perm],
+      )
+      @organisation_admin.grant_application_signin_permission(app)
+      @user.grant_application_signin_permission(app)
+
+      visit edit_user_path(@user)
+      click_link "Manage permissions"
+      click_link "Update permissions for MyApp"
+
+      assert page.has_field?("delegatable_perm")
+      assert page.has_no_field?("non_delegatable_perm")
+    end
+
     should "not be able to grant permissions that are not grantable_from_ui" do
       app = create(
         :application,

test/models/doorkeeper/application_test.rb

@@ -124,6 +124,27 @@ class Doorkeeper::ApplicationTest < ActiveSupport::TestCase
     end
   end
 
+  context "has_delegatable_non_signin_permissions?" do
+    setup do
+      @app = create(:application, with_non_delegatable_supported_permissions: ["perm-1", "perm-2", SupportedPermission::SIGNIN_NAME])
+    end
+
+    should "return false if no permissions are delegatable" do
+      assert_empty @app.supported_permissions.delegatable
+      assert_not @app.has_delegatable_non_signin_permissions?
+    end
+
+    should "return false if only the signin permission is delegatable" do
+      @app.signin_permission.update!(delegatable: true)
+      assert_not @app.has_delegatable_non_signin_permissions?
+    end
+
+    should "return true if any non-signin permissions are delegatable" do
+      @app.supported_permissions.find_by(name: "perm-1").update!(delegatable: true)
+      assert @app.has_delegatable_non_signin_permissions?
+    end
+  end
+
   context ".all (default scope)" do
     setup do
       @app = create(:application)

test/policies/account/application_policy_test.rb

@@ -230,19 +230,19 @@ class Account::ApplicationPolicyTest < ActiveSupport::TestCase
             @current_user.grant_application_signin_permission(@application)
           end
 
-          context "and the application has delegatable permissions" do
+          context "and the application has delegatable non-signin permissions" do
             setup do
-              @application.signin_permission.update!(delegatable: true)
+              @application.stubs(:has_delegatable_non_signin_permissions?).returns(true)
             end
 
             should "be permitted" do
               assert permit?(@current_user, @application, :edit_permissions)
             end
           end
 
-          context "and the application does not have delegatable permissions" do
+          context "and the application does not have delegatable non-signin permissions" do
             setup do
-              @application.signin_permission.update!(delegatable: false)
+              @application.stubs(:has_delegatable_non_signin_permissions?).returns(false)
             end
 
             should "not be permitted" do