From 03266730e8bb948360de214753e047c3b5779921 Mon Sep 17 00:00:00 2001 From: etiennevvv <59622352+etiennevvv@users.noreply.github.com> Date: Mon, 24 Feb 2020 13:05:09 -0500 Subject: [PATCH] GRN2-233: Made account activation & password reset links based on tokens only (#959) * GRN2-233: Hiding email in verification link and password reset link * updating tests * removing uid from email verificaiton link * GRN2-233: modifying test cases * GRN2-233: Removing uid from password reset link * GRN2-233: Removed email_params and fixed "authenticated?" method * GRN2-233: Fixed error when trying to sign in unverified * GRN2-233: Changed how activation tokens are generated --- .../account_activations_controller.rb | 7 ++--- app/controllers/concerns/emailer.rb | 4 +-- app/controllers/password_resets_controller.rb | 2 +- app/controllers/sessions_controller.rb | 5 +++- app/controllers/users_controller.rb | 2 ++ app/models/user.rb | 19 ++++-------- app/views/account_activations/show.html.erb | 2 +- .../account_activations_controller_spec.rb | 30 +++++++++---------- .../password_resets_controller_spec.rb | 14 ++++----- spec/controllers/sessions_controller_spec.rb | 3 +- 10 files changed, 42 insertions(+), 46 deletions(-) diff --git a/app/controllers/account_activations_controller.rb b/app/controllers/account_activations_controller.rb index cc859f09..21a37a66 100644 --- a/app/controllers/account_activations_controller.rb +++ b/app/controllers/account_activations_controller.rb @@ -51,6 +51,7 @@ class AccountActivationsController < ApplicationController flash[:alert] = I18n.t("verify.already_verified") else # Resend + @user.create_activation_token send_activation_email(@user) end @@ -60,14 +61,10 @@ class AccountActivationsController < ApplicationController private def find_user - @user = User.find_by!(email: params[:email], provider: @user_domain) + @user = User.find_by!(activation_digest: User.digest(params[:token]), provider: @user_domain) end def ensure_unauthenticated redirect_to current_user.main_room if current_user end - - def email_params - params.require(:email).permit(:email, :token) - end end diff --git a/app/controllers/concerns/emailer.rb b/app/controllers/concerns/emailer.rb index 9c35c01c..69a06833 100644 --- a/app/controllers/concerns/emailer.rb +++ b/app/controllers/concerns/emailer.rb @@ -125,7 +125,7 @@ module Emailer # 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, email: user.email) + edit_account_activation_url(token: user.activation_token) end def admin_emails @@ -140,7 +140,7 @@ module Emailer end def reset_link(user) - edit_password_reset_url(user.reset_token, email: user.email) + edit_password_reset_url(user.reset_token) end def invitation_link(token) diff --git a/app/controllers/password_resets_controller.rb b/app/controllers/password_resets_controller.rb index 1164e5b8..715594e0 100644 --- a/app/controllers/password_resets_controller.rb +++ b/app/controllers/password_resets_controller.rb @@ -68,7 +68,7 @@ class PasswordResetsController < ApplicationController private def find_user - @user = User.find_by(email: params[:email], provider: @user_domain) + @user = User.find_by(reset_digest: User.digest(params[:id]), provider: @user_domain) end def user_params diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 90272c46..23eef094 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -88,7 +88,10 @@ 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 - return redirect_to(account_activation_path(email: user.email)) unless user.activated? + unless user.activated? + user.create_activation_token + return redirect_to(account_activation_path(token: user.activation_token)) + end end login(user) diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index c6c31af6..9b6a2613 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -58,6 +58,8 @@ 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) redirect_to root_path diff --git a/app/models/user.rb b/app/models/user.rb index 7d61bf64..0d14ea3d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -21,7 +21,7 @@ require 'bbb_api' class User < ApplicationRecord include Deleteable - attr_accessor :reset_token + attr_accessor :reset_token, :activation_token after_create :setup_user before_save { email.try(:downcase!) } @@ -122,7 +122,7 @@ class User < ApplicationRecord def authenticated?(attribute, token) digest = send("#{attribute}_digest") return false if digest.nil? - BCrypt::Password.new(digest).is_password?(token) + digest == Digest::SHA256.base64digest(token) end # Return true if password reset link expires @@ -153,9 +153,9 @@ class User < ApplicationRecord social_uid.nil? end - def activation_token - # Create the token. - create_reset_activation_digest(User.new_token) + def create_activation_token + self.activation_token = User.new_token + update_attributes(activation_digest: User.digest(activation_token)) end def admin_of?(user) @@ -172,8 +172,7 @@ class User < ApplicationRecord end def self.digest(string) - cost = ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost - BCrypt::Password.create(string, cost: cost) + Digest::SHA256.base64digest(string) end # Returns a random token. @@ -250,12 +249,6 @@ class User < ApplicationRecord private - def create_reset_activation_digest(token) - # Create the digest and persist it. - update_attribute(:activation_digest, User.digest(token)) - token - end - # Destory a users rooms when they are removed. def destroy_rooms rooms.destroy_all diff --git a/app/views/account_activations/show.html.erb b/app/views/account_activations/show.html.erb index 6640bb79..c2ab1b52 100644 --- a/app/views/account_activations/show.html.erb +++ b/app/views/account_activations/show.html.erb @@ -22,7 +22,7 @@

<%= t("verify.not_verified") %>

- <%= button_to t("verify.resend"), resend_email_path, params: { email: params['email'], email_verified: false }, class: "btn btn-primary btn-space", "data-disable": "" %> + <%= button_to t("verify.resend"), resend_email_path, params: { token: params['token'], email_verified: false }, class: "btn btn-primary btn-space", "data-disable": "" %>
diff --git a/spec/controllers/account_activations_controller_spec.rb b/spec/controllers/account_activations_controller_spec.rb index 7fb4fcbd..8711ce41 100644 --- a/spec/controllers/account_activations_controller_spec.rb +++ b/spec/controllers/account_activations_controller_spec.rb @@ -27,7 +27,7 @@ describe AccountActivationsController, type: :controller do user = create(:user, provider: "greenlight") @request.session[:user_id] = user.id - get :show, params: { email: user.email } + get :show, params: { uid: user.uid } expect(response).to redirect_to(user.main_room) end @@ -35,7 +35,8 @@ 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") - get :show, params: { email: user.email } + user.create_activation_token + get :show, params: { token: user.activation_token } expect(response).to render_template(:show) end @@ -45,7 +46,8 @@ 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") - get :edit, params: { email: @user.email, token: @user.activation_token } + @user.create_activation_token + get :edit, params: { token: @user.activation_token } @user.reload expect(@user.email_verified).to eq(true) @@ -53,22 +55,17 @@ describe AccountActivationsController, type: :controller do expect(response).to redirect_to(signin_path) end - it "does not activate a user if they have the correct activation token" do + it "should not find user when given fake activation token" do @user = create(:user, email_verified: false, provider: "greenlight") - get :edit, params: { email: @user.email, token: "fake_token" } - @user.reload - - expect(@user.email_verified).to eq(false) - expect(flash[:alert]).to be_present - expect(response).to redirect_to(root_path) + expect { get :edit, params: { token: "fake_token" } }.to raise_error(ActiveRecord::RecordNotFound) end it "does not allow the user to click the verify link again" do @user = create(:user, provider: "greenlight") - get :edit, params: { email: @user.email, token: @user.activation_token } - + @user.create_activation_token + get :edit, params: { token: @user.activation_token } expect(flash[:alert]).to be_present expect(response).to redirect_to(root_path) end @@ -78,7 +75,8 @@ describe AccountActivationsController, type: :controller do @user.add_role :pending - get :edit, params: { email: @user.email, token: @user.activation_token } + @user.create_activation_token + get :edit, params: { token: @user.activation_token } expect(flash[:success]).to be_present expect(response).to redirect_to(root_path) @@ -89,7 +87,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") - expect { get :resend, params: { email: user.email } }.to change { ActionMailer::Base.deliveries.count }.by(1) + user.create_activation_token + expect { get :resend, params: { token: user.activation_token } }.to change { ActionMailer::Base.deliveries.count }.by(1) expect(flash[:success]).to be_present expect(response).to redirect_to(root_path) end @@ -97,7 +96,8 @@ describe AccountActivationsController, type: :controller do it "redirects a verified user to the root path" do user = create(:user, provider: "greenlight") - get :resend, params: { email: user.email } + user.create_activation_token + get :resend, params: { token: user.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 2853ac8c..647367c9 100644 --- a/spec/controllers/password_resets_controller_spec.rb +++ b/spec/controllers/password_resets_controller_spec.rb @@ -116,18 +116,14 @@ describe PasswordResetsController, type: :controller do it "updates attributes if the password update is a success" do user = create(:user, provider: "greenlight") - token = "reset_token" - - cost = ActiveModel::SecurePassword.min_cost ? BCrypt::Engine::MIN_COST : BCrypt::Engine.cost - user.reset_digest = BCrypt::Password.create(token, cost: cost) + 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) - controller.instance_variable_set(:@user, user) params = { - id: token, - email: user.email, + id: user.reset_token, user: { password: :password, password_confirmation: :password, @@ -135,6 +131,10 @@ describe PasswordResetsController, type: :controller do } patch :update, params: params + + user.reload + + expect(old_digest.eql?(user.password_digest)).to be false expect(response).to redirect_to(root_path) end end diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index c2e16b1a..38e078d1 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -143,7 +143,8 @@ describe SessionsController, type: :controller do } expect(@request.session[:user_id]).to be_nil - expect(response).to redirect_to(account_activation_path(email: @user3.email)) + # Expect to redirect to activation path since token is not known here + expect(response.location.start_with?(account_activation_url(token: ""))).to be true end it "should not login user if account is deleted" do