BREAKING CHANGE: Stop authorization adapters assigning attributes on create and update, just check for permission instead

Newly created object still gets attributes assigned.
Closes #3120

Author: mshibuya

lib/rails_admin/config/actions/edit.rb

@@ -25,9 +25,7 @@ class Edit < RailsAdmin::Config::Actions::Base
               sanitize_params_for!(request.xhr? ? :modal : :update)
 
               @object.set_attributes(params[@abstract_model.param_key])
-              @authorization_adapter && @authorization_adapter.attributes_for(:update, @abstract_model).each do |name, value|
-                @object.send("#{name}=", value)
-              end
+              @authorization_adapter && @authorization_adapter.authorize(:update, @abstract_model, @object)
               changes = @object.changes
               if @object.save
                 @auditing_adapter && @auditing_adapter.update_object(@object, @abstract_model, _current_user, changes)

lib/rails_admin/config/actions/new.rb

@@ -36,9 +36,7 @@ class New < RailsAdmin::Config::Actions::Base
               sanitize_params_for!(request.xhr? ? :modal : :create)
 
               @object.set_attributes(params[@abstract_model.param_key])
-              @authorization_adapter && @authorization_adapter.attributes_for(:create, @abstract_model).each do |name, value|
-                @object.send("#{name}=", value)
-              end
+              @authorization_adapter && @authorization_adapter.authorize(:create, @abstract_model, @object)
 
               if @object.save
                 @auditing_adapter && @auditing_adapter.create_object(@object, @abstract_model, _current_user)

spec/integration/authorization/cancancan_spec.rb

@@ -132,6 +132,13 @@ def initialize(user)
       expect(@player).to be_suspended # suspended is inherited behavior based on permission
     end
 
+    it 'POST /admin/player/new with unauthorized attribute value should raise access denied' do
+      visit new_path(model_name: 'player')
+      fill_in 'player[name]', with: 'Jackie Robinson'
+      uncheck 'player[suspended]'
+      expect { click_button 'Save' }.to raise_error(CanCan::AccessDenied)
+    end
+
     it 'GET /admin/player/1/edit should raise access denied' do
       @player = FactoryBot.create :player
       expect { visit edit_path(model_name: 'player', id: @player.id) }.to raise_error(CanCan::AccessDenied)
@@ -163,6 +170,13 @@ def initialize(user)
       expect { visit edit_path(model_name: 'player', id: @player.id) }.to raise_error(CanCan::AccessDenied)
     end
 
+    it 'PUT /admin/player/new with unauthorized attribute value should raise access denied' do
+      @player = FactoryBot.create :player
+      visit edit_path(model_name: 'player', id: @player.id)
+      check 'player[retired]'
+      expect { click_button 'Save' }.to raise_error(CanCan::AccessDenied)
+    end
+
     it 'GET /admin/player/1/delete should raise access denied' do
       @player = FactoryBot.create :player
       expect { visit delete_path(model_name: 'player', id: @player.id) }.to raise_error(CanCan::AccessDenied)

spec/integration/authorization/pundit_spec.rb

@@ -97,6 +97,32 @@
     end
   end
 
+  describe 'with create and read player role' do
+    before do
+      @user.update(roles: [:admin, :read_player, :create_player])
+    end
+
+    it 'POST /admin/player/new with unauthorized attribute value should raise access denied' do
+      visit new_path(model_name: 'player')
+      fill_in 'player[name]', with: 'Jackie Robinson'
+      uncheck 'player[suspended]'
+      expect { click_button 'Save' }.to raise_error(Pundit::NotAuthorizedError)
+    end
+  end
+
+  describe 'with update and read player role' do
+    before do
+      @user.update(roles: [:admin, :read_player, :update_player])
+    end
+
+    it 'PUT /admin/player/new with unauthorized attribute value should raise access denied' do
+      @player = FactoryBot.create :player
+      visit edit_path(model_name: 'player', id: @player.id)
+      check 'player[retired]'
+      expect { click_button 'Save' }.to raise_error(Pundit::NotAuthorizedError)
+    end
+  end
+
   context 'when ApplicationController already has pundit_user' do
     let(:admin) { FactoryBot.create :user, roles: [:admin] }
     before do

spec/policies.rb

@@ -33,10 +33,12 @@ def index?
   def new?
     user.roles.include? :admin
   end
+  alias create? new?
 
   def edit?
     user.roles.include? :admin
   end
+  alias update? edit?
 
   def export?
     user.roles.include? :admin
@@ -49,12 +51,16 @@ def rails_admin_index?
 
 class PlayerPolicy < ApplicationPolicy
   def new?
-    (user.roles.include?(:create_player) || user.roles.include?(:admin) || user.roles.include?(:manage_player))
+    (user.roles.include?(:manage_player) ||
+      (user.roles.include?(:create_player) && (!record.is_a?(Player) || record.suspended)))
   end
+  alias create? new?
 
   def edit?
-    (user.roles.include? :manage_player)
+    (user.roles.include?(:manage_player) ||
+      (user.roles.include?(:update_player) && (!record.is_a?(Player) || !record.retired)))
   end
+  alias update? edit?
 
   def destroy?
     (user.roles.include? :manage_player)