Use signed_id to reset password.

This is more secure, and also requires fewer columns. Issues ------ - Closes #59
stevepolitodesign · Jan 8, 2022 · 1098172 · 1098172
1 parent eb06349
commit 1098172
Show file tree

Hide file tree

Showing 12 changed files with 75 additions and 127 deletions.
diff --git a/README.md b/README.md
@@ -604,70 +604,37 @@ class UsersController < ApplicationController
 end
 ```
 
-## Step 9: Add Password Reset Columns to Users Table
+## Step 9: Add Password Reset Functionality
 
-1. Create migration.
-
-```bash
-rails g migration add_password_reset_token_to_users password_reset_token:string password_reset_sent_at:datetime
-```
-
-2. Update the migration.
-
-```ruby
-# db/migrate/[timestamp]_add_password_reset_token_to_users.rb
-class AddPasswordResetTokenToUsers < ActiveRecord::Migration[6.1]
-  def change
-    add_column :users, :password_reset_token, :string, null: false
-    add_column :users, :password_reset_sent_at, :datetime
-    add_index :users, :password_reset_token, unique: true
-  end
-end
-```
-
-> **What's Going On Here?**
->
-> - The `password_reset_token` column will store a random value created through the [has_secure_token](https://api.rubyonrails.org/classes/ActiveRecord/SecureToken/ClassMethods.html#method-i-has_secure_token) method when a record is saved. This will be used to identify users in a secure way when they need to reset their password. We add `null: false` to prevent empty values and also add a unique index to ensure that no two users will have the same `password_reset_token`. You can think of this as a secure alternative to the `id` column.
-> - The `password_reset_sent_at` column will be used to ensure a password reset link has not expired. This is an added layer of security to prevent a `password_reset_token` from being used multiple times.
-
-3. Run migration.
-
-```bash
-rails db:migrate
-```
-
-4. Update User Model.
+1. Update User Model.
 
 ```ruby
 # app/models/user.rb
 class User < ApplicationRecord
   ...
-  PASSWORD_RESET_TOKEN_EXPIRATION_IN_SECONDS = 10.minutes.to_i
-  ...
-  has_secure_token :password_reset_token
+  PASSWORD_RESET_TOKEN_EXPIRATION = 10.minutes
   ...
-  def password_reset_token_has_expired?
-    return true if password_reset_sent_at.nil?
-    (Time.current - password_reset_sent_at) >= User::PASSWORD_RESET_TOKEN_EXPIRATION_IN_SECONDS
+  def generate_password_reset_token
+    signed_id expires_in: PASSWORD_RESET_TOKEN_EXPIRATION, purpose: :reset_password
   end
-
+  ...
   def send_password_reset_email!
-    regenerate_password_reset_token
-    update_columns(password_reset_sent_at: Time.current)
-    UserMailer.password_reset(self).deliver_now
+    password_reset_token = generate_password_reset_token
+    UserMailer.password_reset(self, password_reset_token).deliver_now
   end
   ...
 end
 ```
 
-5. Update User Mailer.
+2. Update User Mailer.
 
 ```ruby
 # app/mailers/user_mailer.rb
 class UserMailer < ApplicationMailer
   ...
-  def password_reset(user)
+  def password_reset(user, password_reset_token)
     @user = user
+    @password_reset_token = password_reset_token
 
     mail to: @user.email, subject: "Password Reset Instructions"
   end
@@ -678,21 +645,20 @@ end
 <!-- app/views/user_mailer/password_reset.html.erb -->
 <h1>Password Reset Instructions</h1>
 
-<%= link_to "Click here to reset your password.", edit_password_url(@user.password_reset_token) %>
+<%= link_to "Click here to reset your password.", edit_password_url(@password_reset_token) %>
 ```
 
 ```text
 <!-- app/views/user_mailer/password_reset.text.erb -->
 Password Reset Instructions
 
-<%= edit_password_url(@user.password_reset_token) %>
+<%= edit_password_url(@password_reset_token) %>
 ```
 
 > **What's Going On Here?**
 >
-> - The `has_secure_token :password_reset_token` method is added to give us an [API](https://api.rubyonrails.org/classes/ActiveRecord/SecureToken/ClassMethods.html#method-i-has_secure_token) to work with the `password_reset_token` column.
-> - The `password_reset_token_has_expired?` method tells us if the password reset token is expired or not. This can be controlled by changing the value of the `PASSWORD_RESET_TOKEN_EXPIRATION_IN_SECONDS` constant. This will be useful when we build the password reset mailer.
-> - The `send_password_reset_email!` method will create a new `password_reset_token` and update the value of `password_reset_sent_at`. This is to ensure password reset links expire and cannot be reused. It will also send the password reset email to the user. We still need to build this.
+> - The `generate_password_reset_token` method creates a [signed_id](https://api.rubyonrails.org/classes/ActiveRecord/SignedId.html#method-i-signed_id) that will be used to securely identify the user. For added security, we ensure that this ID will expire in 10 minutes (this can be controlled with the `PASSWORD_RESET_TOKEN_EXPIRATION` constant) and give it an explicit purpose of `:reset_password`.
+> - The `send_password_reset_email!` method will create a new `password_reset_token`. This is to ensure password reset links expire and cannot be reused. It will also send the password reset email to the user.
 
 ## Step 10: Build Password Reset Forms
 
@@ -722,10 +688,10 @@ class PasswordsController < ApplicationController
   end
 
   def edit
-    @user = User.find_by(password_reset_token: params[:password_reset_token])
+    @user = User.find_signed(params[:password_reset_token], purpose: :reset_password)
     if @user.present? && @user.unconfirmed?
       redirect_to new_confirmation_path, alert: "You must confirm your email before you can sign in."
-    elsif @user.nil? || @user.password_reset_token_has_expired?
+    elsif @user.nil?
       redirect_to new_password_path, alert: "Invalid or expired token."
     end
   end
@@ -734,21 +700,18 @@ class PasswordsController < ApplicationController
   end
 
   def update
-    @user = User.find_by(password_reset_token: params[:password_reset_token])
+    @user = User.find_signed(params[:password_reset_token], purpose: :reset_password)
     if @user
       if @user.unconfirmed?
         redirect_to new_confirmation_path, alert: "You must confirm your email before you can sign in."
-      elsif @user.password_reset_token_has_expired?
-        redirect_to new_password_path, alert: "Incorrect email or password."
       elsif @user.update(password_params)
-        @user.regenerate_password_reset_token
-        redirect_to login_path, notice: "Signed in."
+        redirect_to login_path, notice: "Sign in."
       else
         flash.now[:alert] = @user.errors.full_messages.to_sentence
         render :edit, status: :unprocessable_entity
       end
     else
-      flash.now[:alert] = "Incorrect email or password."
+      flash.now[:alert] = "Invalid or expired token."
       render :new, status: :unprocessable_entity
     end
   end
@@ -797,7 +760,7 @@ end
 
 ```html+ruby
 <!-- app/views/passwords/edit.html.erb -->
-<%= form_with url: password_path(@user.password_reset_token), scope: :user, method: :put do |form| %>
+<%= form_with url: password_path(params[:password_reset_token]), scope: :user, method: :put do |form| %>
   <div>
     <%= form.label :password %>
     <%= form.password_field :password, required: true %>

diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb
@@ -16,10 +16,10 @@ def create
   end
 
   def edit
-    @user = User.find_by(password_reset_token: params[:password_reset_token])
+    @user = User.find_signed(params[:password_reset_token], purpose: :reset_password)
     if @user.present? && @user.unconfirmed?
       redirect_to new_confirmation_path, alert: "You must confirm your email before you can sign in."
-    elsif @user.nil? || @user.password_reset_token_has_expired?
+    elsif @user.nil?
       redirect_to new_password_path, alert: "Invalid or expired token."
     end
   end
@@ -28,21 +28,18 @@ def new
   end
 
   def update
-    @user = User.find_by(password_reset_token: params[:password_reset_token])
+    @user = User.find_signed(params[:password_reset_token], purpose: :reset_password)
     if @user
       if @user.unconfirmed?
         redirect_to new_confirmation_path, alert: "You must confirm your email before you can sign in."
-      elsif @user.password_reset_token_has_expired?
-        redirect_to new_password_path, alert: "Incorrect email or password."
       elsif @user.update(password_params)
-        @user.regenerate_password_reset_token
-        redirect_to login_path, notice: "Password updated."
+        redirect_to login_path, notice: "Sign in."
       else
         flash.now[:alert] = @user.errors.full_messages.to_sentence
         render :edit, status: :unprocessable_entity
       end
     else
-      flash.now[:alert] = "Incorrect email or password."
+      flash.now[:alert] = "Invalid or expired token."
       render :new, status: :unprocessable_entity
     end
   end

diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb
@@ -13,8 +13,9 @@ def confirmation(user, confirmation_token)
     mail to: @user.confirmable_email, subject: "Confirmation Instructions"
   end
 
-  def password_reset(user)
+  def password_reset(user, password_reset_token)
     @user = user
+    @password_reset_token = password_reset_token
 
     mail to: @user.email, subject: "Password Reset Instructions"
   end

diff --git a/app/models/user.rb b/app/models/user.rb
@@ -1,12 +1,11 @@
 class User < ApplicationRecord
   CONFIRMATION_TOKEN_EXPIRATION = 10.minutes
   MAILER_FROM_EMAIL = "no-reply@example.com"
-  PASSWORD_RESET_TOKEN_EXPIRATION_IN_SECONDS = 10.minutes.to_i
+  PASSWORD_RESET_TOKEN_EXPIRATION = 10.minutes
 
   attr_accessor :current_password
 
   has_secure_password
-  has_secure_token :password_reset_token
   has_secure_token :remember_token
   has_secure_token :session_token
 
@@ -58,9 +57,8 @@ def generate_confirmation_token
     signed_id expires_in: CONFIRMATION_TOKEN_EXPIRATION, purpose: :confirm_email
   end
 
-  def password_reset_token_has_expired?
-    return true if password_reset_sent_at.nil?
-    (Time.current - password_reset_sent_at) >= User::PASSWORD_RESET_TOKEN_EXPIRATION_IN_SECONDS
+  def generate_password_reset_token
+    signed_id expires_in: PASSWORD_RESET_TOKEN_EXPIRATION, purpose: :reset_password
   end
 
   def send_confirmation_email!
@@ -69,9 +67,8 @@ def send_confirmation_email!
   end
 
   def send_password_reset_email!
-    regenerate_password_reset_token
-    update_columns(password_reset_sent_at: Time.current)
-    UserMailer.password_reset(self).deliver_now
+    password_reset_token = generate_password_reset_token
+    UserMailer.password_reset(self, password_reset_token).deliver_now
   end
 
   def reconfirming?

diff --git a/app/views/passwords/edit.html.erb b/app/views/passwords/edit.html.erb
@@ -1,4 +1,4 @@
-<%= form_with url: password_path(@user.password_reset_token), scope: :user, method: :put do |form| %>
+<%= form_with url: password_path(params[:password_reset_token]), scope: :user, method: :put do |form| %>
   <div>
     <%= form.label :password %>
     <%= form.password_field :password, required: true %>

diff --git a/app/views/user_mailer/password_reset.html.erb b/app/views/user_mailer/password_reset.html.erb
@@ -1,3 +1,3 @@
 <h1>Password Reset Instructions</h1>
 
-<%= link_to "Click here to reset your password.", edit_password_url(@user.password_reset_token) %>
+<%= link_to "Click here to reset your password.", edit_password_url(@password_reset_token) %>
diff --git a/app/views/user_mailer/password_reset.text.erb b/app/views/user_mailer/password_reset.text.erb
@@ -1,3 +1,3 @@
 Password Reset Instructions
 
-<%= edit_password_url(@user.password_reset_token) %>
+<%= edit_password_url(@password_reset_token) %>
diff --git a/db/migrate/20211123101413_add_password_reset_token_to_users.rb b/db/migrate/20211123101413_add_password_reset_token_to_users.rb
diff --git a/db/schema.rb b/db/schema.rb
diff --git a/test/controllers/passwords_controller_test.rb b/test/controllers/passwords_controller_test.rb
@@ -6,24 +6,23 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "should get edit" do
-    @confirmed_user.send_password_reset_email!
+    password_reset_token = @confirmed_user.generate_password_reset_token
 
-    get edit_password_path(@confirmed_user.password_reset_token)
+    get edit_password_path(password_reset_token)
     assert_response :ok
   end
 
   test "should redirect from edit if password link expired" do
-    @confirmed_user.send_password_reset_email!
+    password_reset_token = @confirmed_user.generate_password_reset_token
 
     travel_to 601.seconds.from_now
-    get edit_password_path(@confirmed_user.password_reset_token)
+    get edit_password_path(password_reset_token)
 
     assert_redirected_to new_password_path
     assert_not_nil flash[:alert]
   end
 
   test "should redirect from edit if password link is incorrect" do
-    travel_to 601.seconds.from_now
     get edit_password_path("not_a_real_token")
 
     assert_redirected_to new_password_path
@@ -32,16 +31,20 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest
 
   test "should redirect from edit if user is not confirmed" do
     @confirmed_user.update!(confirmed_at: nil)
-    get edit_password_path(@confirmed_user.password_reset_token)
+    password_reset_token = @confirmed_user.generate_password_reset_token
+
+    get edit_password_path(password_reset_token)
 
     assert_redirected_to new_confirmation_path
     assert_not_nil flash[:alert]
   end
 
   test "should redirect from edit if user is authenticated" do
+    password_reset_token = @confirmed_user.generate_password_reset_token
+
     login @confirmed_user
 
-    get edit_password_path(@confirmed_user.password_reset_token)
+    get edit_password_path(password_reset_token)
     assert_redirected_to root_path
   end
 
@@ -71,25 +74,23 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "should update password" do
-    @confirmed_user.send_password_reset_email!
+    password_reset_token = @confirmed_user.generate_password_reset_token
 
-    assert_changes "@confirmed_user.reload.password_reset_token" do
-      put password_path(@confirmed_user.password_reset_token), params: {
-        user: {
-          password: "password",
-          password_confirmation: "password"
-        }
+    put password_path(password_reset_token), params: {
+      user: {
+        password: "password",
+        password_confirmation: "password"
       }
-    end
+    }
 
     assert_redirected_to login_path
     assert_not_nil flash[:notice]
   end
 
   test "should handle errors" do
-    @confirmed_user.send_password_reset_email!
+    password_reset_token = @confirmed_user.generate_password_reset_token
 
-    put password_path(@confirmed_user.password_reset_token), params: {
+    put password_path(password_reset_token), params: {
       user: {
         password: "password",
         password_confirmation: "password_that_does_not_match"
@@ -100,9 +101,11 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest
   end
 
   test "should not update password if authenticated" do
+    password_reset_token = @confirmed_user.generate_password_reset_token
+
     login @confirmed_user
 
-    put password_path(@confirmed_user.password_reset_token), params: {
+    put password_path(password_reset_token), params: {
       user: {
         password: "password",
         password_confirmation: "password"
@@ -113,4 +116,4 @@ class PasswordsControllerTest < ActionDispatch::IntegrationTest
     get new_password_path
     assert_redirected_to root_path
   end
-end
+end