Merge pull request #3394 from JDutil/fixes-438

Add permission check for admins updating user passwords

backend/app/controllers/spree/admin/users_controller.rb

@@ -131,6 +131,10 @@ def user_params
           attributes += [{ spree_role_ids: [] }]
         end
 
+        unless can? :update_password, @user
+          attributes -= [:password, :password_confirmation]
+        end
+
         params.require(:user).permit(attributes)
       end
 

backend/app/views/spree/admin/users/_form.html.erb

@@ -60,18 +60,20 @@
   </div>
 
   <div data-hook="admin_user_form_password_fields" class="col-6">
-    <%= f.field_container :password do %>
-      <%= f.label :password %>
-      <%= f.password_field :password, class: 'fullwidth' %>
-      <%= f.error_message_on :password %>
-    <% end %>
+    <% if can? :update_password, @user %>
+      <%= f.field_container :password do %>
+        <%= f.label :password %>
+        <%= f.password_field :password, class: 'fullwidth' %>
+        <%= f.error_message_on :password %>
+      <% end %>
 
 
-    <% if f.object.respond_to?(:password_confirmation) %>
-      <%= f.field_container :password do %>
-        <%= f.label :password_confirmation %>
-        <%= f.password_field :password_confirmation, class: 'fullwidth' %>
-        <%= f.error_message_on :password_confirmation %>
+      <% if f.object.respond_to?(:password_confirmation) %>
+        <%= f.field_container :password do %>
+          <%= f.label :password_confirmation %>
+          <%= f.password_field :password_confirmation, class: 'fullwidth' %>
+          <%= f.error_message_on :password_confirmation %>
+        <% end %>
       <% end %>
     <% end %>
   </div>

backend/spec/controllers/spree/admin/users_controller_spec.rb

@@ -202,6 +202,29 @@ def user
       end
     end
 
+    context "allowed to update passwords" do
+      it "can change password of a user" do
+        expect {
+          put :update, params: { id: user.id, user: { password: "diff123", password_confirmation: "diff123" } }
+        }.to_not raise_error
+      end
+    end
+
+    context "not allowed to update passwords" do
+      stub_authorization! do |_user|
+        can [:admin, :update], Spree.user_class
+      end
+
+      it "cannot change password of a user" do
+        allow(ActionController::Parameters).
+          to receive(:action_on_unpermitted_parameters).and_return(:raise)
+
+        expect {
+          put :update, params: { id: user.id, user: { password: "diff123", password_confirmation: "diff123" } }
+        }.to raise_error(ActionController::UnpermittedParameters)
+      end
+    end
+
     it "can update ship_address attributes" do
       post :update, params: { id: user.id, user: { ship_address_attributes: valid_address_attributes } }
       expect(user.reload.ship_address.city).to eq('New York')

backend/spec/features/admin/users_spec.rb

@@ -158,14 +158,6 @@
       expect(page).to have_field('user_email', with: 'a@example.com99')
     end
 
-    it 'can edit the user password' do
-      fill_in 'user_password', with: 'welcome'
-      fill_in 'user_password_confirmation', with: 'welcome'
-      click_button 'Update'
-
-      expect(page).to have_text 'Account updated'
-    end
-
     it 'can edit user roles' do
       click_link 'Account'
 
@@ -213,6 +205,24 @@
       expect(user_a.reload.bill_address.address1).to eq "1313 Mockingbird Ln"
     end
 
+    it 'can edit the user password' do
+      fill_in 'user_password', with: 'welcome'
+      fill_in 'user_password_confirmation', with: 'welcome'
+      click_button 'Update'
+
+      expect(page).to have_text 'Account updated'
+    end
+
+    context 'without password permissions' do
+      custom_authorization! do |_user|
+        cannot [:update_password], Spree.user_class
+      end
+
+      it 'cannot edit the user password' do
+        expect(page).to_not have_text 'Password'
+      end
+    end
+
     context 'invalid entry' do
       around do |example|
         ::AlwaysInvalidUser = Class.new(Spree.user_class) do

core/lib/spree/permission_sets/user_management.rb

@@ -11,6 +11,9 @@ def activate!
         can :update_email, Spree.user_class do |user|
           user.spree_roles.none?
         end
+        can :update_password, Spree.user_class do |user|
+          user.spree_roles.none?
+        end
 
         cannot [:delete, :destroy], Spree.user_class
         can :manage, Spree::StoreCredit

core/lib/spree/permitted_attributes.rb

@@ -117,10 +117,10 @@ module PermittedAttributes
       :meta_description, :meta_keywords, :meta_title, :child_index
     ]
 
-    # intentionally leaving off email here to prevent privilege escalation
+    # Intentionally leaving off email here to prevent privilege escalation
     # by changing a user with higher priveleges' email to one a lower-priveleged
-    # admin owns. creating a user with an email is handled separate at the
-    # controller level
+    # admin owns. Creating a user with an email is handled separate at the
+    # controller level.
     @@user_attributes = [:password, :password_confirmation]
 
     @@variant_attributes = [

core/spec/models/spree/permission_sets/user_management_spec.rb

@@ -25,11 +25,13 @@
     context 'when the user does not have a role' do
       let(:user) { create(:user) }
       it { is_expected.to be_able_to(:update_email, user) }
+      it { is_expected.to be_able_to(:update_password, user) }
     end
 
     context 'when the user has a role' do
       let(:user) { create(:user, spree_roles: [create(:role)]) }
       it { is_expected.not_to be_able_to(:update_email, user) }
+      it { is_expected.not_to be_able_to(:update_password, user) }
     end
 
     it { is_expected.not_to be_able_to(:delete, Spree.user_class) }