GRN2-xx: Restructured email verification and password reset (bigblueb…

…utton#1444) * Restructured email verification and password reset * Fixed issue with password reset Co-authored-by: Jesus Federico <[email protected]>
pbdco · Apr 29, 2020 · 2830210 · 2830210
1 parent 8f3ba8a
commit 2830210
Show file tree

Hide file tree

Showing 10 changed files with 44 additions and 79 deletions.
diff --git a/app/controllers/account_activations_controller.rb b/app/controllers/account_activations_controller.rb
@@ -29,7 +29,7 @@ def show
   # GET /account_activations/edit
   def edit
     # If the user exists and is not verified and provided the correct token
-    if @user && !@user.activated? && @user.authenticated?(:activation, params[:token])
+    if @user && !@user.activated?
       # Verify user
       @user.activate
 
@@ -51,8 +51,7 @@ def resend
       flash[:alert] = I18n.t("verify.already_verified")
     else
       # Resend
-      @user.create_activation_token
-      send_activation_email(@user)
+      send_activation_email(@user, @user.create_activation_token)
     end
 
     redirect_to root_path
@@ -61,7 +60,7 @@ def resend
   private
 
   def find_user
-    @user = User.find_by!(activation_digest: User.digest(params[:token]), provider: @user_domain)
+    @user = User.find_by!(activation_digest: User.hash_token(params[:token]), provider: @user_domain)
   end
 
   def ensure_unauthenticated

diff --git a/app/controllers/admins_controller.rb b/app/controllers/admins_controller.rb
@@ -133,9 +133,7 @@ def invite
 
   # GET /admins/reset
   def reset
-    @user.create_reset_digest
-
-    send_password_reset_email(@user)
+    send_password_reset_email(@user, @user.create_reset_digest)
 
     if session[:prev_url].present?
       redirect_path = session[:prev_url]

diff --git a/app/controllers/concerns/emailer.rb b/app/controllers/concerns/emailer.rb
@@ -20,11 +20,11 @@ module Emailer
   extend ActiveSupport::Concern
 
   # Sends account activation email.
-  def send_activation_email(user)
+  def send_activation_email(user, token)
     begin
       return unless Rails.configuration.enable_email_verification
 
-      UserMailer.verify_email(user, user_verification_link(user), @settings).deliver
+      UserMailer.verify_email(user, user_verification_link(token), @settings).deliver
     rescue => e
       logger.error "Support: Error in email delivery: #{e}"
       flash[:alert] = I18n.t(params[:message], default: I18n.t("delivery_error"))
@@ -34,11 +34,11 @@ def send_activation_email(user)
   end
 
   # Sends password reset email.
-  def send_password_reset_email(user)
+  def send_password_reset_email(user, token)
     begin
       return unless Rails.configuration.enable_email_verification
 
-      UserMailer.password_reset(user, reset_link(user), @settings).deliver_now
+      UserMailer.password_reset(user, reset_link(token), @settings).deliver_now
     rescue => e
       logger.error "Support: Error in email delivery: #{e}"
       flash[:alert] = I18n.t(params[:message], default: I18n.t("delivery_error"))
@@ -124,8 +124,8 @@ def send_invite_user_signup_email(user)
   private
 
   # Returns the link the user needs to click to verify their account
-  def user_verification_link(user)
-    edit_account_activation_url(token: user.activation_token)
+  def user_verification_link(token)
+    edit_account_activation_url(token: token)
   end
 
   def admin_emails
@@ -139,8 +139,8 @@ def admin_emails
     admins.collect(&:email).join(",")
   end
 
-  def reset_link(user)
-    edit_password_reset_url(user.reset_token)
+  def reset_link(token)
+    edit_password_reset_url(token)
   end
 
   def invitation_link(token)

diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb
@@ -21,7 +21,6 @@ class PasswordResetsController < ApplicationController
 
   before_action :disable_password_reset, unless: -> { Rails.configuration.enable_email_verification }
   before_action :find_user, only: [:edit, :update]
-  before_action :valid_user, only: [:edit, :update]
   before_action :check_expiration, only: [:edit, :update]
 
   # POST /password_resets/new
@@ -34,8 +33,7 @@ def create
       # Check if user exists and throw an error if he doesn't
       @user = User.find_by!(email: params[:password_reset][:email].downcase, provider: @user_domain)
 
-      @user.create_reset_digest
-      send_password_reset_email(@user)
+      send_password_reset_email(@user, @user.create_reset_digest)
       redirect_to root_path
     rescue
       # User doesn't exist
@@ -68,7 +66,7 @@ def update
   private
 
   def find_user
-    @user = User.find_by(reset_digest: User.digest(params[:id]), provider: @user_domain)
+    @user = User.find_by(reset_digest: User.hash_token(params[:id]), provider: @user_domain)
   end
 
   def user_params
@@ -80,14 +78,6 @@ def check_expiration
     redirect_to new_password_reset_url, alert: I18n.t("expired_reset_token") if @user.password_reset_expired?
   end
 
-  # Confirms a valid user.
-  def valid_user
-    unless @user.authenticated?(:reset, params[:id])
-      @user&.activate unless @user&.activated?
-      redirect_to root_url
-    end
-  end
-
   # Redirects to 404 if emails are not enabled
   def disable_password_reset
     redirect_to '/404'

diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb
@@ -88,10 +88,7 @@ def create
       # Check that the user is a Greenlight account
       return redirect_to(root_path, alert: I18n.t("invalid_login_method")) unless user.greenlight_account?
       # Check that the user has verified their account
-      unless user.activated?
-        user.create_activation_token
-        return redirect_to(account_activation_path(token: user.activation_token))
-      end
+      return redirect_to(account_activation_path(token: user.create_activation_token)) unless user.activated?
     end
 
     login(user)
@@ -247,8 +244,7 @@ def switch_account_to_local(user)
     logger.info "Switching social account to local account for #{user.uid}"
 
     # Send the user a reset password email
-    user.create_reset_digest
-    send_password_reset_email(user)
+    send_password_reset_email(user, user.create_reset_digest)
 
     # Overwrite the flash with a more descriptive message if successful
     flash[:success] = I18n.t("reset_password.auth_change") if flash[:success].present?

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
@@ -58,9 +58,7 @@ def create
     # Sign in automatically if email verification is disabled or if user is already verified.
     login(@user) && return if !Rails.configuration.enable_email_verification || @user.email_verified
 
-    @user.create_activation_token
-
-    send_activation_email(@user)
+    send_activation_email(@user, @user.create_activation_token)
 
     redirect_to root_path
   end

diff --git a/app/models/user.rb b/app/models/user.rb
@@ -21,7 +21,6 @@
 class User < ApplicationRecord
   include Deleteable
 
-  attr_accessor :reset_token, :activation_token
   after_create :setup_user
 
   before_save { email.try(:downcase!) }
@@ -110,24 +109,28 @@ def ordered_rooms
 
   # Activates an account and initialize a users main room
   def activate
-    update_attributes(email_verified: true, activated_at: Time.zone.now)
+    update_attributes(email_verified: true, activated_at: Time.zone.now, activation_digest: nil)
   end
 
   def activated?
     Rails.configuration.enable_email_verification ? email_verified : true
   end
 
+  def self.hash_token(token)
+    Digest::SHA2.hexdigest(token)
+  end
+
   # Sets the password reset attributes.
   def create_reset_digest
-    self.reset_token = User.new_token
-    update_attributes(reset_digest: User.digest(reset_token), reset_sent_at: Time.zone.now)
+    new_token = SecureRandom.urlsafe_base64
+    update_attributes(reset_digest: User.hash_token(new_token), reset_sent_at: Time.zone.now)
+    new_token
   end
 
-  # Returns true if the given token matches the digest.
-  def authenticated?(attribute, token)
-    digest = send("#{attribute}_digest")
-    return false if digest.nil?
-    digest == Digest::SHA256.base64digest(token)
+  def create_activation_token
+    new_token = SecureRandom.urlsafe_base64
+    update_attributes(activation_digest: User.hash_token(new_token))
+    new_token
   end
 
   # Return true if password reset link expires
@@ -158,11 +161,6 @@ def greenlight_account?
     social_uid.nil?
   end
 
-  def create_activation_token
-    self.activation_token = User.new_token
-    update_attributes(activation_digest: User.digest(activation_token))
-  end
-
   def admin_of?(user, permission)
     has_correct_permission = highest_priority_role.get_permission(permission) && id != user.id
 
@@ -171,15 +169,6 @@ def admin_of?(user, permission)
     has_correct_permission && provider == user.provider && !user.has_role?(:super_admin)
   end
 
-  def self.digest(string)
-    Digest::SHA256.base64digest(string)
-  end
-
-  # Returns a random token.
-  def self.new_token
-    SecureRandom.urlsafe_base64
-  end
-
   # role functions
   def highest_priority_role
     roles.min_by(&:priority)

diff --git a/spec/controllers/account_activations_controller_spec.rb b/spec/controllers/account_activations_controller_spec.rb
@@ -35,8 +35,7 @@
     it "renders the verify view if the user is not signed in and is not verified" do
       user = create(:user, email_verified: false,  provider: "greenlight")
 
-      user.create_activation_token
-      get :show, params: { token: user.activation_token }
+      get :show, params: { token: user.create_activation_token }
 
       expect(response).to render_template(:show)
     end
@@ -46,8 +45,7 @@
     it "activates a user if they have the correct activation token" do
       @user = create(:user, email_verified: false, provider: "greenlight")
 
-      @user.create_activation_token
-      get :edit, params: { token: @user.activation_token }
+      get :edit, params: { token: @user.create_activation_token }
       @user.reload
 
       expect(@user.email_verified).to eq(true)
@@ -64,8 +62,7 @@
     it "does not allow the user to click the verify link again" do
       @user = create(:user, provider: "greenlight")
 
-      @user.create_activation_token
-      get :edit, params: { token: @user.activation_token }
+      get :edit, params: { token: @user.create_activation_token }
       expect(flash[:alert]).to be_present
       expect(response).to redirect_to(root_path)
     end
@@ -75,8 +72,7 @@
 
       @user.add_role :pending
 
-      @user.create_activation_token
-      get :edit, params: { token: @user.activation_token }
+      get :edit, params: { token: @user.create_activation_token }
 
       expect(flash[:success]).to be_present
       expect(response).to redirect_to(root_path)
@@ -87,17 +83,16 @@
     it "resends the email to the current user if the resend button is clicked" do
       user = create(:user, email_verified: false, provider: "greenlight")
 
-      user.create_activation_token
-      expect { get :resend, params: { token: user.activation_token } }.to change { ActionMailer::Base.deliveries.count }.by(1)
+      expect { get :resend, params: { token: user.create_activation_token } }
+        .to change { ActionMailer::Base.deliveries.count }.by(1)
       expect(flash[:success]).to be_present
       expect(response).to redirect_to(root_path)
     end
 
     it "redirects a verified user to the root path" do
       user = create(:user, provider: "greenlight")
 
-      user.create_activation_token
-      get :resend, params: { token: user.activation_token }
+      get :resend, params: { token: user.create_activation_token }
 
       expect(flash[:alert]).to be_present
       expect(response).to redirect_to(root_path)

diff --git a/spec/controllers/password_resets_controller_spec.rb b/spec/controllers/password_resets_controller_spec.rb
@@ -81,8 +81,6 @@ def random_valid_user_params
     context "valid user" do
       it "reloads page with notice if password is empty" do
         token = "reset_token"
-
-        allow(controller).to receive(:valid_user).and_return(nil)
         allow(controller).to receive(:check_expiration).and_return(nil)
 
         params = {
@@ -99,7 +97,6 @@ def random_valid_user_params
       it "reloads page with notice if password is confirmation doesn't match" do
         token = "reset_token"
 
-        allow(controller).to receive(:valid_user).and_return(nil)
         allow(controller).to receive(:check_expiration).and_return(nil)
 
         params = {
@@ -116,14 +113,12 @@ def random_valid_user_params
 
       it "updates attributes if the password update is a success" do
         user = create(:user, provider: "greenlight")
-        user.create_reset_digest
         old_digest = user.password_digest
 
-        allow(controller).to receive(:valid_user).and_return(nil)
         allow(controller).to receive(:check_expiration).and_return(nil)
 
         params = {
-          id: user.reset_token,
+          id: user.create_reset_digest,
           user: {
             password: :password,
             password_confirmation: :password,

diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
@@ -138,8 +138,13 @@
     it 'creates token and respective reset digest' do
       user = create(:user)
 
-      reset_digest_success = user.create_reset_digest
-      expect(reset_digest_success).to eq(true)
+      expect(user.create_reset_digest).to be_truthy
+    end
+
+    it 'correctly verifies the token' do
+      user = create(:user)
+      token = user.create_reset_digest
+      expect(User.exists?(reset_digest: User.hash_token(token))).to be true
     end
 
     it 'verifies if password reset link expired' do