From 253641e4ecf1dac9cd52c3df1ff724f6ecccd3bc Mon Sep 17 00:00:00 2001 From: JDutil Date: Wed, 23 Oct 2019 15:32:53 -0600 Subject: [PATCH] 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. --- .../spree/admin/users_controller.rb | 4 +++ .../views/spree/admin/users/_form.html.erb | 22 +++++++++------- .../spree/admin/users_controller_spec.rb | 23 ++++++++++++++++ backend/spec/features/admin/users_spec.rb | 26 +++++++++++++------ .../spree/permission_sets/user_management.rb | 3 +++ core/lib/spree/permitted_attributes.rb | 6 ++--- .../permission_sets/user_management_spec.rb | 2 ++ 7 files changed, 65 insertions(+), 21 deletions(-) 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) }