mirror of
https://github.com/basecamp/once-campfire.git
synced 2026-10-28 16:36:48 +09:00
Check the current password whenever the saved password changes
The profile asked for the current password when the request's password
field was filled in. The check now follows the save itself: the model
checks the challenge whenever the password digest is about to change,
and the profile always hands it the challenge. Editing a name, avatar
or email without a new password still needs none, and scripts that
reset a password, which set no challenge, are unaffected.
Only a string counts as a challenge, and the rate limit counts every
non-empty one, so what's limited is exactly what gets checked.
(cherry picked from commit 4a580302a5)
This commit is contained in:
1 parent
6c5ceca6b5
commit
56e2a287ea
4 files changed
+58
-14
No files matched your search
@@ -2,7 +2,7 @@ class Users::ProfilesController < ApplicationController
|
||||
before_action :set_user
|
||||
|
||||
# Limited like signing in, so the current password can't be guessed here instead.
|
||||
rate_limit to: 10, within: 3.minutes, only: :update, if: :changing_password?,
|
||||
rate_limit to: 10, within: 3.minutes, only: :update, if: :answering_password_challenge?,
|
||||
with: -> { redirect_to user_profile_url, alert: "Too many attempts. Try again in a few minutes." }
|
||||
|
||||
def show
|
||||
@@ -25,19 +25,22 @@ class Users::ProfilesController < ApplicationController
|
||||
@user = Current.user
|
||||
end
|
||||
|
||||
# Always with the challenge, so the model checks it whenever the save would change the password, however the
|
||||
# request spelled the new one.
|
||||
def user_params
|
||||
permitted = params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact
|
||||
|
||||
if changing_password?
|
||||
permitted.merge(password_challenge: params[:user][:password_challenge].to_s)
|
||||
else
|
||||
permitted
|
||||
end
|
||||
params.require(:user).permit(:name, :avatar, :email_address, :password, :bio).compact
|
||||
.merge(password_challenge: password_challenge)
|
||||
end
|
||||
|
||||
# As has_secure_password reads it: an empty password leaves the current one alone.
|
||||
def changing_password?
|
||||
!params.dig(:user, :password).to_s.empty?
|
||||
# Only a string is a challenge, so the rate limit counts exactly what the model checks.
|
||||
def password_challenge
|
||||
challenge = params.dig(:user, :password_challenge)
|
||||
challenge.is_a?(String) ? challenge : ""
|
||||
end
|
||||
|
||||
# Guessing the current password means sending one, so this is what the rate limit counts.
|
||||
def answering_password_challenge?
|
||||
!password_challenge.empty?
|
||||
end
|
||||
|
||||
def update_notice
|
||||
@@ -47,7 +50,7 @@ class Users::ProfilesController < ApplicationController
|
||||
def update_alert
|
||||
if !@user.errors.include?(:password_challenge)
|
||||
@user.errors.full_messages.to_sentence
|
||||
elsif params[:user][:password_challenge].blank?
|
||||
elsif password_challenge.empty?
|
||||
"Enter your current password to change it."
|
||||
else
|
||||
"That isn't your current password."
|
||||
|
||||
Reference in new issue
Block a user