Add permission check for admins updating user passwords

For security purposes administrators should not be able to set a
users password. Only the accounts owner should be able to directly set
their password. administrators should only have the ability to send a
password reset email to the account owner. Otherwise someone working in
customer service or another role could take over a users account in
order to make illegal purchases with their stored credit card
information.

In order to maintain backwards compatibility, and leave more power in
control of the store owner this will leave the current admin role
behavior the same, but anyone creating custom roles will no longer
be able to update passwords unless they explicitly add a change
password permission.

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

@@ -131,6 +131,11 @@ def user_params
           attributes += [{ spree_role_ids: [] }]
         end
 
+        # Don't allow users to set another users password.
+        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

@@ -57,22 +57,23 @@
       </div>
     <% end %>
 
-  </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 %>
+    <div data-hook="admin_user_form_password_fields" class="col-6">
+      <% 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 %>
-    <% end %>
+    </div>
   </div>
 </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

@@ -162,14 +162,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'
 
@@ -217,6 +209,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/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 = [