diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index 920fc86dbed..72d5cb4c0d2 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/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 diff --git a/backend/app/views/spree/admin/users/_form.html.erb b/backend/app/views/spree/admin/users/_form.html.erb index 719f9dc8d82..2f1682b4350 100644 --- a/backend/app/views/spree/admin/users/_form.html.erb +++ b/backend/app/views/spree/admin/users/_form.html.erb @@ -60,18 +60,20 @@
- <%= 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 %>
diff --git a/backend/spec/controllers/spree/admin/users_controller_spec.rb b/backend/spec/controllers/spree/admin/users_controller_spec.rb index 27d9319a7dc..7a111d7efe8 100644 --- a/backend/spec/controllers/spree/admin/users_controller_spec.rb +++ b/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') diff --git a/backend/spec/features/admin/users_spec.rb b/backend/spec/features/admin/users_spec.rb index 7d9bafddb32..8fd1f088571 100644 --- a/backend/spec/features/admin/users_spec.rb +++ b/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 diff --git a/core/lib/spree/permission_sets/user_management.rb b/core/lib/spree/permission_sets/user_management.rb index 298ff2f89e3..90a65ca0319 100644 --- a/core/lib/spree/permission_sets/user_management.rb +++ b/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 diff --git a/core/lib/spree/permitted_attributes.rb b/core/lib/spree/permitted_attributes.rb index 2ed3106567c..27c43a30fa9 100644 --- a/core/lib/spree/permitted_attributes.rb +++ b/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 = [ diff --git a/core/spec/models/spree/permission_sets/user_management_spec.rb b/core/spec/models/spree/permission_sets/user_management_spec.rb index 0a84acafc94..5210ced5e90 100644 --- a/core/spec/models/spree/permission_sets/user_management_spec.rb +++ b/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) }