Don't default to signin permission

We need to fix delegatable permissions, and this will involve changes to
the `UserApplicationPermissionPolicy`. Currently everything is based on
the `signin` permission. This undoes that, updates the
`UserApplicationPermission.for` method so that `supported_permission` is
compulsory, and - for now - passes in the `signin` permission where this
is used. This will be iterated on in later commits to gradually fix
delagatable permissions

This required an update to the `UserApplicationPermissionPolicy` tests,
and I restructured them in the process

app/controllers/users/permissions_controller.rb

@@ -13,7 +13,7 @@ def show
   end
 
   def edit
-    authorize UserApplicationPermission.for(@user, @application)
+    authorize UserApplicationPermission.for(user: @user, supported_permission: @application.signin_permission)
 
     @shared_permissions_form_locals = {
       action: user_application_permissions_path(@user, @application),
@@ -38,7 +38,7 @@ def edit
   end
 
   def update
-    authorize UserApplicationPermission.for(@user, @application)
+    authorize UserApplicationPermission.for(user: @user, supported_permission: @application.signin_permission)
 
     selected_permission_ids = []
 

app/controllers/users/signin_permissions_controller.rb

@@ -5,7 +5,7 @@ class Users::SigninPermissionsController < ApplicationController
 
   def create
     application = Doorkeeper::Application.not_api_only.find(params[:application_id])
-    authorize UserApplicationPermission.for(@user, application)
+    authorize UserApplicationPermission.for(user: @user, supported_permission: application.signin_permission)
 
     params = { supported_permission_ids: @user.supported_permissions.map(&:id) + [application.signin_permission.id] }
     UserUpdate.new(@user, params, current_user, user_ip_address).call
@@ -14,11 +14,11 @@ def create
   end
 
   def delete
-    authorize UserApplicationPermission.for(@user, @application)
+    authorize UserApplicationPermission.for(user: @user, supported_permission: @application.signin_permission)
   end
 
   def destroy
-    authorize UserApplicationPermission.for(@user, @application)
+    authorize UserApplicationPermission.for(user: @user, supported_permission: @application.signin_permission)
 
     params = { supported_permission_ids: @user.supported_permissions.map(&:id) - [@application.signin_permission.id] }
     UserUpdate.new(@user, params, current_user, user_ip_address).call

app/helpers/application_table_helper.rb

@@ -14,7 +14,7 @@ def account_applications_grant_access_link(application)
   end
 
   def users_applications_grant_access_link(application, user)
-    if policy(UserApplicationPermission.for(user, application)).create?
+    if policy(UserApplicationPermission.for(user:, supported_permission: application.signin_permission)).create?
       grant_access_link(application, user)
     else
       ""
@@ -30,7 +30,7 @@ def account_applications_remove_access_link(application)
   end
 
   def users_applications_remove_access_link(application, user)
-    if policy(UserApplicationPermission.for(user, application)).delete?
+    if policy(UserApplicationPermission.for(user:, supported_permission: application.signin_permission)).delete?
       remove_access_link(application, user)
     else
       ""
@@ -48,7 +48,7 @@ def account_applications_permissions_link(application)
   end
 
   def users_applications_permissions_link(application, user)
-    if policy(UserApplicationPermission.for(user, application)).edit?
+    if policy(UserApplicationPermission.for(user:, supported_permission: application.signin_permission)).edit?
       update_permissions_link(application, user)
     else
       view_permissions_link(application, user)

app/models/user_application_permission.rb

@@ -8,8 +8,8 @@ class UserApplicationPermission < ApplicationRecord
 
   before_validation :assign_application_id
 
-  def self.for(user, application)
-    new(user:, application:)
+  def self.for(user:, supported_permission:, application: nil)
+    new(user:, supported_permission:, application: application || supported_permission.application)
   end
 
 private

app/policies/user_application_permission_policy.rb

@@ -1,15 +1,14 @@
 class UserApplicationPermissionPolicy < BasePolicy
   def create?
     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.signin_permission.delegatable?
+    current_user.publishing_manager? && current_user.has_access_to?(application) && supported_permission.delegatable?
   end
   alias_method :destroy?, :create?
   alias_method :delete?, :create?
   alias_method :update?, :create?
   alias_method :edit?, :create?
 
-  delegate :user, :application, to: :record
+  delegate :user, :application, :supported_permission, to: :record
 end

test/models/user_application_permission_test.rb

@@ -1,6 +1,37 @@
 require "test_helper"
 
 class UserApplicationPermissionTest < ActiveSupport::TestCase
+  context ".for" do
+    setup do
+      @user = build(:user)
+      @supported_permission = build(:supported_permission)
+      @application = build(:application, supported_permissions: [@supported_permission])
+    end
+
+    context "for a user, supported permission, and application" do
+      setup { @result = UserApplicationPermission.for(user: @user, supported_permission: @supported_permission, application: @application) }
+
+      should "return a UserApplicationPermission with the given associations" do
+        assert @result.instance_of?(UserApplicationPermission)
+        assert_equal @user, @result.user
+        assert_equal @supported_permission, @result.supported_permission
+        assert_equal @application, @result.application
+      end
+
+      should "not persist the UserApplicationPermission" do
+        assert_not @result.persisted?
+      end
+    end
+
+    context "without an application" do
+      should "infer the application from the supported permission" do
+        result = UserApplicationPermission.for(user: @user, supported_permission: @supported_permission)
+
+        assert_equal @application, result.application
+      end
+    end
+  end
+
   context "validations" do
     setup do
       @user = create(:user)

test/policies/user_application_permission_policy_test.rb

@@ -8,89 +8,57 @@ class UserApplicationPermissionPolicyTest < ActiveSupport::TestCase
   %i[create destroy delete update edit].each do |policy_method|
     context "#{policy_method}?" do
       setup do
-        @user = create(:user)
         @application = create(:application)
-        @signin_permission = @user.grant_application_signin_permission(@application)
-      end
-
-      should "be allowed for superadmins" do
-        current_user = build(:superadmin_user)
-
-        stub_policy(current_user, @user, edit?: true)
-
-        assert permit?(current_user, @signin_permission, policy_method)
-      end
-
-      should "be allowed for admins" do
-        current_user = build(:admin_user)
-
-        stub_policy(current_user, @user, edit?: true)
-
-        assert permit?(current_user, @signin_permission, policy_method)
-      end
-
-      should "be allowed for super org admins when they have access to the application" do
-        current_user = build(:super_organisation_admin_user)
-        current_user.grant_application_signin_permission(@application)
-
-        stub_policy(current_user, @user, edit?: true)
-
-        assert permit?(current_user, @signin_permission, policy_method)
-      end
-
-      should "not be allowed for super org admins when they don't have access to the application" do
-        current_user = build(:super_organisation_admin_user)
-
-        stub_policy(current_user, @user, edit?: true)
-
-        assert forbid?(current_user, @signin_permission, policy_method)
-      end
-
-      should "be allowed for org admins when they have access to the application" do
-        current_user = build(:organisation_admin_user)
-        current_user.grant_application_signin_permission(@application)
-
-        stub_policy(current_user, @user, edit?: true)
-
-        assert permit?(current_user, @signin_permission, policy_method)
-      end
-
-      should "not be allowed for org admins when they don't have access to the application" do
-        current_user = build(:organisation_admin_user)
-
-        stub_policy(current_user, @user, edit?: true)
+        @supported_permission = create(:supported_permission, application: @application, delegatable: true)
+        @current_user = create(:superadmin_user, :in_organisation)
+        @user = create(:user)
+        @user_application_permission = UserApplicationPermission.for(user: @user, supported_permission: @supported_permission, application: @application)
 
-        assert forbid?(current_user, @signin_permission, policy_method)
+        @current_user.grant_application_signin_permission(@application)
+        @user.grant_application_signin_permission(@application)
       end
 
-      context "when the signin permission is not delegatable" do
-        setup do
-          @application.signin_permission.update!(delegatable: false)
-        end
-
-        should "not be allowed for super org admins" do
-          current_user = build(:super_organisation_admin_user)
-
-          stub_policy(current_user, @user, edit?: true)
+      context "when the current user is allowed to edit the target user" do
+        setup { stub_policy(@current_user, @user, edit?: true) }
 
-          assert forbid?(current_user, @signin_permission, policy_method)
+        [Roles::Superadmin.name, Roles::Admin.name].each do |govuk_admin_role|
+          context "and the current user is a(n) #{govuk_admin_role}" do
+            setup { @current_user.update!(role: govuk_admin_role) }
+            should("be allowed") { assert permit?(@current_user, @user_application_permission, policy_method) }
+          end
         end
 
-        should "not be allowed for org admins" do
-          current_user = build(:organisation_admin_user)
-
-          stub_policy(current_user, @user, edit?: true)
-
-          assert forbid?(current_user, @signin_permission, policy_method)
+        [Roles::SuperOrganisationAdmin.name, Roles::OrganisationAdmin.name].each do |publishing_manager_role|
+          context "and the current user is a #{publishing_manager_role}" do
+            setup { @current_user.update!(role: publishing_manager_role) }
+
+            context "with access to the application and for a delegatable permission" do
+              should("be allowed") { assert permit?(@current_user, @user_application_permission, policy_method) }
+            end
+
+            context "without access to the application" do
+              setup do
+                UserApplicationPermission.find_by(
+                  user: @current_user,
+                  application: @application,
+                  supported_permission: @application.signin_permission,
+                ).destroy
+              end
+
+              should("not be allowed") { assert forbid?(@current_user, @user_application_permission, policy_method) }
+            end
+
+            context "for a non-delegatable permission" do
+              setup { @supported_permission.update!(delegatable: false) }
+              should("not be allowed") { assert forbid?(@current_user, @user_application_permission, policy_method) }
+            end
+          end
         end
       end
 
-      should "not be allowed if current user is not allowed to edit the target user" do
-        current_user = build(:user)
-
-        stub_policy(current_user, @user, edit?: false)
-
-        assert forbid?(current_user, @signin_permission, policy_method)
+      context "when the current user is not allowed to edit the target user" do
+        setup { stub_policy(@current_user, @user, edit?: false) }
+        should("not be allowed") { assert forbid?(@current_user, @user_application_permission, policy_method) }
       end
     end
   end