diff --git a/app/controllers/account_activations_controller.rb b/app/controllers/account_activations_controller.rb index 21a37a66..2a08afa2 100644 --- a/app/controllers/account_activations_controller.rb +++ b/app/controllers/account_activations_controller.rb @@ -29,7 +29,7 @@ class AccountActivationsController < ApplicationController # 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 @@ class AccountActivationsController < ApplicationController 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 @@ class AccountActivationsController < ApplicationController 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 index 083bb1f5..be8e8090 100644 --- a/app/controllers/admins_controller.rb +++ b/app/controllers/admins_controller.rb @@ -133,9 +133,7 @@ class AdminsController < ApplicationController # 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 index 69a06833..59540023 100644 --- 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 @@ module Emailer 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 @@ module Emailer 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 @@ module Emailer 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 index 1b8098f6..36a84c83 100644 --- 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 @@ class PasswordResetsController < ApplicationController # 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 @@ class PasswordResetsController < ApplicationController 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 @@ class PasswordResetsController < ApplicationController 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 index 2771cf5a..d16b520b 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -88,10 +88,7 @@ class SessionsController < ApplicationController # 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 @@ class SessionsController < ApplicationController 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 index f463dff9..9ce870bb 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -58,9 +58,7 @@ class UsersController < ApplicationController # 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 index 466e7820..bdc25fa0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,7 +21,6 @@ require 'bbb_api' class User < ApplicationRecord include Deleteable - attr_accessor :reset_token, :activation_token after_create :setup_user before_save { email.try(:downcase!) } @@ -110,24 +109,28 @@ class User < ApplicationRecord # 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 - # 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) + def self.hash_token(token) + Digest::SHA2.hexdigest(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) + # Sets the password reset attributes. + def create_reset_digest + new_token = SecureRandom.urlsafe_base64 + update_attributes(reset_digest: User.hash_token(new_token), reset_sent_at: Time.zone.now) + new_token + end + + 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 @@ class User < ApplicationRecord 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 @@ class User < ApplicationRecord 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 index 8711ce41..6fd27e12 100644 --- a/spec/controllers/account_activations_controller_spec.rb +++ b/spec/controllers/account_activations_controller_spec.rb @@ -35,8 +35,7 @@ describe AccountActivationsController, type: :controller do 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 @@ describe AccountActivationsController, type: :controller do 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 @@ describe AccountActivationsController, type: :controller do 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 @@ describe AccountActivationsController, type: :controller do @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,8 +83,8 @@ describe AccountActivationsController, type: :controller do 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 @@ -96,8 +92,7 @@ describe AccountActivationsController, type: :controller do 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 index 647367c9..aeee3570 100644 --- a/spec/controllers/password_resets_controller_spec.rb +++ b/spec/controllers/password_resets_controller_spec.rb @@ -81,8 +81,6 @@ describe PasswordResetsController, type: :controller do 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 @@ describe PasswordResetsController, type: :controller do 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 @@ describe PasswordResetsController, type: :controller do 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 index 8046565e..051e3052 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -138,8 +138,13 @@ describe User, type: :model do 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