Merge pull request #1109 from texpert/fix-updated-ajax

Fix updated_ajax action to permit only legit params

Author: texpert

app/controllers/camaleon_cms/admin/users_controller.rb

@@ -52,10 +52,15 @@ def update
       def updated_ajax
         @user = current_site.users.find(params[:user_id])
         update_session = current_user_is?(@user)
-        @user.update(params.require(:password).permit!)
-        render inline: @user.errors.full_messages.join(', ')
+        attrs = params.require(:password).permit(%i[password password_confirmation])
+        @user.update(password: attrs.require(:password), password_confirmation: attrs.require(:password_confirmation))
+
+        return render inline: @user.errors.full_messages.join(', '), status: :unprocessable_entity if @user.errors.any?
+
         # keep user logged in when changing their own password
         update_auth_token_in_cookie @user.auth_token if update_session && @user.saved_change_to_password_digest?
+      rescue ActionController::ParameterMissing => e
+        render inline: "ERROR: #{e.class.name}, #{e.message}", status: :bad_request
       end
 
       def update_auth_token_in_cookie(token)

spec/requests/admin/users_controller/updated_ajax_spec.rb

@@ -0,0 +1,87 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe 'updated_ajax request', type: :request do
+  init_site
+
+  let(:current_site) { Cama::Site.first.decorate }
+  let(:current_user) { create(:user_admin, site: current_site, password: 'secret', password_confirmation: 'secret') }
+
+  before do
+    allow_any_instance_of(CamaleonCms::AdminController).to receive(:cama_authenticate)
+    allow_any_instance_of(CamaleonCms::Admin::UsersController).to receive(:validate_role).and_return(true)
+  end
+
+  context 'when receiving correct params' do
+    it "updates user's password" do
+      expect(current_user.authenticate('secret')).to be_truthy
+      expect(current_user.authenticate('new password')).to be_falsey
+
+      patch "/admin/users/#{current_user.id}/updated_ajax",
+            params: { password: { password: 'new password', password_confirmation: 'new password' } }
+
+      expect(response.status).to eql(204)
+      expect(response.body).to eql('')
+      expect(current_user.reload.authenticate('secret')).to be_falsey
+      expect(current_user.reload.authenticate('new password')).to be_truthy
+    end
+  end
+
+  context 'when receiving incorrect params' do
+    context 'when wrong password confirmation' do
+      it "doesn't update user's password and return error" do
+        expect(current_user.authenticate('secret')).to be_truthy
+
+        patch "/admin/users/#{current_user.id}/updated_ajax",
+              params: { password: { password: 'new password', password_confirmation: 'old password' } }
+
+        expect(response.status).to eql(422)
+        expect(response.body).to eql("Password confirmation doesn't match Password")
+        expect(current_user.reload.authenticate('secret')).to be_truthy
+      end
+    end
+
+    context 'when missing password confirmation' do
+      it "doesn't update user's password" do
+        expect(current_user.authenticate('secret')).to be_truthy
+
+        patch "/admin/users/#{current_user.id}/updated_ajax", params: { password: { password: 'new password' } }
+
+        expect(response.status).to eql(400)
+        expect(response.body).to start_with(
+          'ERROR: ActionController::ParameterMissing, param is missing or the value is empty'
+        )
+        expect(response.body).to include('password_confirmation')
+        expect(current_user.reload.authenticate('secret')).to be_truthy
+        expect(current_user.reload.authenticate('new password')).to be_falsey
+      end
+    end
+
+    context 'when passing unpermitted params' do
+      it 'ignores the unpermitted param' do
+        expect(current_user.authenticate('secret')).to be_truthy
+
+        # Changing this to false, because the receiver is not only yielded to the blocks, but also passed as an
+        # unexpected additional argument to the `originall.call`
+        RSpec::Mocks.configuration.yield_receiver_to_any_instance_implementation_blocks = false
+
+        allow_any_instance_of(CamaleonCms::User).to receive(:update).and_call_original
+
+        expect_any_instance_of(CamaleonCms::User)
+          .to receive(:update).with(password: 'new password', password_confirmation: 'new password')
+
+        patch "/admin/users/#{current_user.id}/updated_ajax",
+              params: { password: { password: 'new password', password_confirmation: 'new password', role: 'admin' } }
+
+        expect(response.status).to eql(204)
+        expect(response.body).to eql('')
+        expect(current_user.reload.authenticate('secret')).to be_falsey
+        expect(current_user.reload.authenticate('new password')).to be_truthy
+
+        # returning the default configuration
+        RSpec::Mocks.configuration.yield_receiver_to_any_instance_implementation_blocks = false
+      end
+    end
+  end
+end

spec/spec_helper.rb

@@ -1,5 +1,6 @@
 ENV['RAILS_ENV'] ||= 'test'
 
+require 'pathname'
 CAMALEON_CMS_ROOT = Pathname.new(__FILE__).join('../..')
 
 require File.expand_path('dummy/config/environment.rb', __dir__)