Fix Account::PermissionsController#update

By duplicating the tests and implementation we've recently added for
`Users::PermissionsController#update`.

We discussed extracting a method to remove the duplication between the
two `build_user_update_params` methods but couldn't agree on where the
responsibility should live, so we've decided to leave it for now.

Note the change to `PunditHelpers#stub_policy` where we've updated it to
handle namespaced policies.

Co-authored-by: Chris Lowis <chris.lowis@gofreerange.com>

app/controllers/account/permissions_controller.rb

@@ -3,6 +3,7 @@ class Account::PermissionsController < ApplicationController
 
   before_action :authenticate_user!
   before_action :set_application
+  before_action :set_permissions
 
   def show
     authorize [:account, @application], :view_permissions?
@@ -14,24 +15,38 @@ def show
 
   def edit
     authorize [:account, @application], :edit_permissions?
-
-    @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
   end
 
   def update
     authorize [:account, @application], :edit_permissions?
 
-    permission_ids_for_other_applications = current_user.supported_permissions.excluding_application(@application).pluck(:id)
-    user_update_params = { supported_permission_ids: permission_ids_for_other_applications + params[:application][:supported_permission_ids] }
-    UserUpdate.new(current_user, user_update_params, current_user, user_ip_address).call
+    UserUpdate.new(current_user, build_user_update_params, current_user, user_ip_address).call
 
     flash[:application_id] = @application.id
     redirect_to account_applications_path
   end
 
 private
 
+  def update_params
+    params.require(:application).permit(supported_permission_ids: [])
+  end
+
   def set_application
     @application = Doorkeeper::Application.not_api_only.find(params[:application_id])
   end
+
+  def set_permissions
+    @permissions = @application.sorted_supported_permissions_grantable_from_ui(include_signin: false)
+  end
+
+  def build_user_update_params
+    permissions_user_has = current_user.supported_permissions.pluck(:id)
+    updatable_permissions_for_this_app = @permissions.pluck(:id)
+    selected_permissions = update_params[:supported_permission_ids].map(&:to_i)
+    permissions_to_add = updatable_permissions_for_this_app.intersection(selected_permissions)
+    permissions_to_remove = updatable_permissions_for_this_app.difference(selected_permissions)
+
+    { supported_permission_ids: (permissions_user_has + permissions_to_add - permissions_to_remove).sort }
+  end
 end

test/controllers/account/permissions_controller_test.rb

@@ -133,7 +133,7 @@ class Account::PermissionsControllerTest < ActionController::TestCase
 
       patch :update, params: { application_id: application.id, application: { supported_permission_ids: [new_permission.id] } }
 
-      assert_equal %w[new], user.permissions_for(application)
+      assert_equal %w[new signin], user.permissions_for(application)
       assert_redirected_to account_applications_path
     end
 
@@ -147,10 +147,106 @@ class Account::PermissionsControllerTest < ActionController::TestCase
 
       patch :update, params: { application_id: application.id, application: { supported_permission_ids: [new_permission.id] } }
 
-      assert_equal %w[new], user.permissions_for(application)
+      assert_equal %w[new signin], user.permissions_for(application)
       assert_equal %w[other], user.permissions_for(other_application)
     end
 
+    should "prevent permissions being added for apps that the current user does not have access to" do
+      application1 = create(:application)
+      application2 = create(:application, with_supported_permissions: %w[app2-permission])
+
+      current_user = create(:organisation_admin_user)
+      current_user.grant_application_signin_permission(application1)
+      sign_in current_user
+
+      stub_policy current_user, [:account, Doorkeeper::Application], index?: true
+      stub_policy current_user, [:account, application1], edit_permissions?: true
+
+      app2_permission = application2.supported_permissions.find_by!(name: "app2-permission")
+
+      patch :update, params: { application_id: application1, application: { supported_permission_ids: [app2_permission.id] } }
+
+      current_user.reload
+
+      assert_equal [application1.signin_permission], current_user.supported_permissions
+    end
+
+    should "not remove the signin permission from the app when updating other permissions" do
+      application = create(:application, with_supported_permissions: %w[other])
+
+      current_user = create(:admin_user)
+      current_user.grant_application_signin_permission(application)
+      sign_in current_user
+
+      stub_policy current_user, [:account, application], edit_permissions?: true
+
+      other_permission = application.supported_permissions.find_by(name: "other")
+
+      patch :update, params: { application_id: application.id, application: { supported_permission_ids: [other_permission.id] } }
+
+      current_user.reload
+      assert_same_elements [application.signin_permission, other_permission], current_user.supported_permissions
+    end
+
+    should "not remove permissions the user already has that are not grantable from ui" do
+      application = create(:application, with_supported_permissions: %w[other], with_supported_permissions_not_grantable_from_ui: %w[not_from_ui])
+
+      current_user = create(:admin_user)
+      current_user.grant_application_signin_permission(application)
+      current_user.grant_application_permission(application, "not_from_ui")
+
+      sign_in current_user
+
+      stub_policy current_user, [:account, application], edit_permissions?: true
+
+      other_permission = application.supported_permissions.find_by(name: "other")
+      not_from_ui_permission = application.supported_permissions.find_by(name: "not_from_ui")
+
+      patch :update, params: { application_id: application.id, application: { supported_permission_ids: [other_permission.id] } }
+
+      current_user.reload
+
+      assert_same_elements [other_permission, application.signin_permission, not_from_ui_permission], current_user.supported_permissions
+    end
+
+    should "prevent permissions being added for other apps" do
+      other_application = create(:application, with_supported_permissions: %w[other])
+      application = create(:application)
+
+      current_user = create(:admin_user)
+      current_user.grant_application_signin_permission(application)
+
+      sign_in current_user
+
+      stub_policy current_user, [:account, application], edit_permissions?: true
+
+      other_permission = other_application.supported_permissions.find_by(name: "other")
+
+      patch :update, params: { application_id: application.id, application: { supported_permission_ids: [other_permission.id] } }
+
+      current_user.reload
+
+      assert_equal [application.signin_permission], current_user.supported_permissions
+    end
+
+    should "prevent permissions being added that are not grantable from the ui" do
+      application = create(:application, with_supported_permissions_not_grantable_from_ui: %w[not_from_ui])
+
+      current_user = create(:admin_user)
+      current_user.grant_application_signin_permission(application)
+      sign_in current_user
+
+      stub_policy current_user, [:account, application], edit_permissions?: true
+
+      not_from_ui_permission = application.supported_permissions.find_by(name: "not_from_ui")
+
+      patch :update, params: { application_id: application.id, application: { supported_permission_ids: [not_from_ui_permission.id] } }
+
+      current_user.reload
+
+      assert_equal [application.signin_permission], current_user.supported_permissions
+    end
+
     should "assign the application id to the application_id flash" do
       application = create(:application, with_supported_permissions: %w[new])
       user = create(:admin_user, with_permissions: { application => [SupportedPermission::SIGNIN_NAME] })

test/support/pundit_helpers.rb

@@ -1,6 +1,7 @@
 module PunditHelpers
   def stub_policy(current_user, record, method_and_return_value)
     policy_class = Pundit::PolicyFinder.new(record).policy
+    record = record.last if record.is_a?(Array)
     policy = stub_everything("policy", method_and_return_value).responds_like_instance_of(policy_class)
     policy_class.stubs(:new).with(current_user, record).returns(policy)
   end