Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Do not allow the user to mass assign their own password alongside other
parameters

Much thanks to Breno Vitório (@brenu) for the report!
  • Loading branch information
jhass committed Apr 27, 2022
1 parent 6ad4eb3 commit 1cfe003
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 29 deletions.
39 changes: 17 additions & 22 deletions app/controllers/users_controller.rb
Expand Up @@ -18,25 +18,17 @@ def privacy_settings
end

def update
password_changed = false
user_data = user_params
@user = current_user

if user_data
# change password
if params[:change_password]
password_changed = change_password(user_data)
else
update_user(user_data)
end
if params[:change_password] && user_password_params
password_changed = change_password(user_password_params)
return redirect_to new_user_session_path if password_changed
elsif user_params
update_user(user_params)
end

if password_changed
redirect_to new_user_session_path
else
set_email_preferences
render :edit
end
set_email_preferences
render :edit
end

def update_privacy_settings
Expand Down Expand Up @@ -137,13 +129,9 @@ def auth_token

private

# rubocop:disable Metrics/MethodLength
def user_params
params.fetch(:user).permit(
:email,
:current_password,
:password,
:password_confirmation,
:language,
:color_theme,
:disable_mail,
Expand All @@ -157,7 +145,14 @@ def user_params
email_preferences: UserPreference::VALID_EMAIL_TYPES.map(&:to_sym)
)
end
# rubocop:enable Metrics/MethodLength

def user_password_params
params.fetch(:user).permit(
:current_password,
:password,
:password_confirmation
)
end

def update_user(user_data)
if user_data[:email_preferences]
Expand All @@ -177,8 +172,8 @@ def update_user(user_data)
end
end

def change_password(user_data)
if @user.update_with_password(user_data)
def change_password(password_params)
if @user.update_with_password(password_params)
flash[:notice] = t("users.update.password_changed")
true
else
Expand Down
27 changes: 20 additions & 7 deletions spec/controllers/users_controller_spec.rb
Expand Up @@ -127,21 +127,34 @@
expect(response).to render_template('edit')
end

describe 'password updates' do
describe "password updates" do
let(:password_params) do
{:current_password => 'bluepin7',
:password => "foobaz",
:password_confirmation => "foobaz"}
{current_password: "bluepin7", password: "foobaz", password_confirmation: "foobaz"}
end

let(:params) do
{id: @user.id, user: password_params, change_password: 'Change Password'}
{id: @user.id, user: password_params, change_password: "Change Password"}
end

it "uses devise's update with password" do
expect(@user).to receive(:update_with_password).with(hash_including(password_params))
before do
allow(@controller).to receive(:current_user).and_return(@user)
allow(@user).to receive(:update_with_password)
allow(@user).to receive(:update_attributes)
end

it "uses devise's update with password" do
put :update, params: params

expect(@user).to have_received(:update_with_password).with(hash_including(password_params))
expect(@user).not_to have_received(:update_attributes).with(hash_including(password_params))
end

it "does not update the password without the change_password param" do
put :update, params: params.except(:change_password).deep_merge(user: {language: "de"})

expect(@user).not_to have_received(:update_with_password).with(hash_including(password_params))
expect(@user).not_to have_received(:update_attributes).with(hash_including(password_params))
expect(@user).to have_received(:update_attributes).with(hash_including(language: "de"))
end
end

Expand Down

0 comments on commit 1cfe003

Please sign in to comment.