From c60e25f71c460b11413c4f4a77492cbe2e2eb815 Mon Sep 17 00:00:00 2001 From: farhatahmad <35435341+farhatahmad@users.noreply.github.com> Date: Fri, 22 Feb 2019 16:47:02 -0500 Subject: [PATCH] GRN-56: Correctly implemented the account verification flow (#367) * Correctly implemented the account verification flow * Fixed issues with redirect locations --- .../account_activations_controller.rb | 76 +++++++++++++++ app/controllers/rooms_controller.rb | 18 +++- app/controllers/sessions_controller.rb | 6 +- app/controllers/users_controller.rb | 73 ++++++-------- app/helpers/account_activations_helper.rb | 20 ++++ app/models/user.rb | 32 ++++++- .../verify.html.erb | 9 +- .../shared/components/_resend_button.html.erb | 2 +- config/locales/en.yml | 10 +- config/routes.rb | 13 +-- .../20190206210049_add_activation_to_users.rb | 8 ++ db/schema.rb | 16 ++-- .../account_activations_controller_spec.rb | 95 +++++++++++++++++++ spec/controllers/rooms_controller_spec.rb | 18 ++++ spec/controllers/sessions_controller_spec.rb | 15 +++ spec/controllers/users_controller_spec.rb | 66 ------------- spec/factories.rb | 1 + 17 files changed, 337 insertions(+), 141 deletions(-) create mode 100644 app/controllers/account_activations_controller.rb create mode 100644 app/helpers/account_activations_helper.rb rename app/views/{users => account_activations}/verify.html.erb (75%) create mode 100644 db/migrate/20190206210049_add_activation_to_users.rb create mode 100644 spec/controllers/account_activations_controller_spec.rb diff --git a/app/controllers/account_activations_controller.rb b/app/controllers/account_activations_controller.rb new file mode 100644 index 00000000..cb7515c9 --- /dev/null +++ b/app/controllers/account_activations_controller.rb @@ -0,0 +1,76 @@ +# frozen_string_literal: true + +# BigBlueButton open source conferencing system - http://www.bigbluebutton.org/. +# +# Copyright (c) 2018 BigBlueButton Inc. and by respective authors (see below). +# +# This program is free software; you can redistribute it and/or modify it under the +# terms of the GNU Lesser General Public License as published by the Free Software +# Foundation; either version 3.0 of the License, or (at your option) any later +# version. +# +# BigBlueButton is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A +# PARTICULAR PURPOSE. See the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License along +# with BigBlueButton; if not, see . + +class AccountActivationsController < ApplicationController + before_action :ensure_unauthenticated + before_action :find_user + + # GET /account_activations + def show + render :verify + end + + # GET /account_activations/edit + def edit + if @user && !@user.email_verified? && @user.authenticated?(:activation, params[:token]) + @user.activate + + flash[:success] = I18n.t("verify.activated") + " " + I18n.t("verify.signin") + else + flash[:alert] = I18n.t("verify.invalid") + end + + redirect_to root_url + end + + # GET /account_activations/resend + def resend + if @user.email_verified + flash[:alert] = I18n.t("verify.already_verified") + else + begin + @user.send_activation_email(verification_link) + rescue => e + logger.error "Error in email delivery: #{e}" + flash[:alert] = I18n.t(params[:message], default: I18n.t("delivery_error")) + else + flash[:success] = I18n.t("email_sent") + end + end + + redirect_to(root_path) + end + + private + + def verification_link + request.base_url + edit_account_activation_path(token: @user.activation_token, email: @user.email) + end + + def ensure_unauthenticated + redirect_to current_user.main_room if current_user + end + + def email_params + params.require(:email).permit(:token) + end + + def find_user + @user = User.find_by!(email: params[:email], provider: "greenlight") + end +end diff --git a/app/controllers/rooms_controller.rb b/app/controllers/rooms_controller.rb index 3d737b80..a53435a6 100644 --- a/app/controllers/rooms_controller.rb +++ b/app/controllers/rooms_controller.rb @@ -18,9 +18,11 @@ class RoomsController < ApplicationController before_action :validate_accepted_terms, unless: -> { !Rails.configuration.terms } - before_action :validate_verified_email, unless: -> { !Rails.configuration.enable_email_verification } + before_action :validate_verified_email, except: [:show, :join], + unless: -> { !Rails.configuration.enable_email_verification } before_action :find_room, except: :create before_action :verify_room_ownership, except: [:create, :show, :join, :logout] + before_action :verify_room_owner_verified, only: [:show, :join] include RecordingsHelper META_LISTED = "gl-listed" @@ -240,7 +242,19 @@ class RoomsController < ApplicationController def validate_verified_email if current_user - redirect_to resend_path unless current_user.email_verified + redirect_to account_activation_path(current_user) unless current_user.email_verified + end + end + + def verify_room_owner_verified + unless @room.owner.email_verified + flash[:alert] = t("room.unavailable") + + if current_user + redirect_to current_user.main_room + else + redirect_to root_path + end end end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 690bbc1d..96128b7b 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -31,7 +31,11 @@ class SessionsController < ApplicationController if user && !user.greenlight_account? redirect_to root_path, alert: I18n.t("invalid_login_method") elsif user.try(:authenticate, session_params[:password]) - login(user) + if user.email_verified + login(user) + else + redirect_to(account_activation_path(email: user.email)) && return + end else redirect_to root_path, alert: I18n.t("invalid_credentials") end diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index d7c780ad..145b0fa1 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -30,19 +30,37 @@ class UsersController < ApplicationController @user = User.new(user_params) @user.provider = "greenlight" - if Rails.configuration.enable_email_verification && @user.save + # Check if user already exists + if User.exists?(email: user_params[:email], provider: @user.provider) + existing_user = User.find_by!(email: user_params[:email], provider: @user.provider) + if Rails.configuration.enable_email_verification && !existing_user.email_verified? + # User exists but is not verified + redirect_to(account_activation_path(email: existing_user.email)) && return + else + # User already exists and is verified + # Attempt to save so that the correct errors appear + @user.save + + render(:new) && return + end + elsif Rails.configuration.enable_email_verification && @user.save begin - UserMailer.verify_email(@user, verification_link(@user)).deliver - login(@user) + @user.send_activation_email(verification_link) rescue => e logger.error "Error in email delivery: #{e}" - mailer_delivery_fail + flash[:alert] = I18n.t(params[:message], default: I18n.t("delivery_error")) + else + flash[:success] = I18n.t("email_sent") end + + redirect_to(root_path) && return elsif @user.save + # User doesn't exist and email verification is turned off + @user.activate login(@user) else # Handle error on user creation. - render :new + render(:new) && return end end @@ -145,53 +163,16 @@ class UsersController < ApplicationController end end - # GET | POST /u/verify/confirm - def confirm - if !current_user || current_user.uid != params[:user_uid] - redirect_to '/404' - elsif current_user.email_verified - login(current_user) - elsif params[:email_verified] == "true" - current_user.update_attributes(email_verified: true) - login(current_user) - else - render 'verify' - end - end - - # GET /u/verify/resend - def resend - if !current_user - redirect_to '/404' - elsif current_user.email_verified - login(current_user) - elsif params[:email_verified] == "false" - begin - UserMailer.verify_email(current_user, verification_link(current_user)).deliver - render 'verify' - rescue => e - logger.error "Error in email delivery: #{e}" - mailer_delivery_fail - end - else - render 'verify' - end - end - private - def mailer_delivery_fail - redirect_to root_path, alert: I18n.t(params[:message], default: I18n.t("delivery_error")) - end - - def verification_link(user) - request.base_url + confirm_path(user.uid) - end - def find_user @user = User.find_by!(uid: params[:user_uid]) end + def verification_link + request.base_url + edit_account_activation_path(token: @user.activation_token, email: @user.email) + end + def ensure_unauthenticated redirect_to current_user.main_room if current_user end diff --git a/app/helpers/account_activations_helper.rb b/app/helpers/account_activations_helper.rb new file mode 100644 index 00000000..4cddbd77 --- /dev/null +++ b/app/helpers/account_activations_helper.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +# BigBlueButton open source conferencing system - http://www.bigbluebutton.org/. +# +# Copyright (c) 2018 BigBlueButton Inc. and by respective authors (see below). +# +# This program is free software; you can redistribute it and/or modify it under the +# terms of the GNU Lesser General Public License as published by the Free Software +# Foundation; either version 3.0 of the License, or (at your option) any later +# version. +# +# BigBlueButton is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A +# PARTICULAR PURPOSE. See the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License along +# with BigBlueButton; if not, see . + +module AccountActivationsHelper +end diff --git a/app/models/user.rb b/app/models/user.rb index dab25525..0f368d21 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -17,9 +17,10 @@ # with BigBlueButton; if not, see . class User < ApplicationRecord - attr_accessor :reset_token - after_create :initialize_main_room + attr_accessor :reset_token, :activation_token + after_create :create_home_room_if_verified before_save { email.try(:downcase!) } + before_create :create_activation_digest before_destroy :destroy_rooms @@ -94,6 +95,18 @@ class User < ApplicationRecord end end + # Activates an account and initialize a users main room + def activate + update_attribute(:email_verified, true) + update_attribute(:activated_at, Time.zone.now) + + initialize_main_room + end + + def send_activation_email(url) + UserMailer.verify_email(self, url).deliver + end + # Sets the password reset attributes. def create_reset_digest self.reset_token = User.new_token @@ -156,14 +169,27 @@ class User < ApplicationRecord private + def create_activation_digest + # Create the token and digest. + self.activation_token = User.new_token + self.activation_digest = User.digest(activation_token) + end + # Destory a users rooms when they are removed. def destroy_rooms rooms.destroy_all end + # Assigns the user a BigBlueButton id and a home room if verified + def create_home_room_if_verified + self.uid = "gl-#{(0...12).map { (65 + rand(26)).chr }.join.downcase}" + + initialize_main_room if email_verified + save + end + # Initializes a room for the user and assign a BigBlueButton user id. def initialize_main_room - self.uid = "gl-#{(0...12).map { (65 + rand(26)).chr }.join.downcase}" self.main_room = Room.create!(owner: self, name: I18n.t("home_room")) save end diff --git a/app/views/users/verify.html.erb b/app/views/account_activations/verify.html.erb similarity index 75% rename from app/views/users/verify.html.erb rename to app/views/account_activations/verify.html.erb index 49815d14..3fc03ff2 100644 --- a/app/views/users/verify.html.erb +++ b/app/views/account_activations/verify.html.erb @@ -20,13 +20,8 @@

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

-

Your account has not been verified yet.

- <% if Rails.configuration.enable_email_verification && params[:user_uid] == current_user.uid %> - <%= render "/shared/components/confirm_button" %> - <% else %> - <%= render "/shared/components/resend_button" %> - <% end %> - +

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

+ <%= render "/shared/components/resend_button" %>
diff --git a/app/views/shared/components/_resend_button.html.erb b/app/views/shared/components/_resend_button.html.erb index 914c85e6..0662bb7f 100644 --- a/app/views/shared/components/_resend_button.html.erb +++ b/app/views/shared/components/_resend_button.html.erb @@ -14,5 +14,5 @@ %>
- <%= button_to t("verify.resend"), resend_path, params: { email_verified: false }, class: "btn btn-primary btn-space" %> + <%= button_to t("verify.resend"), resend_email_path, params: { email_verified: false }, class: "btn btn-primary btn-space" %>
diff --git a/config/locales/en.yml b/config/locales/en.yml index 48bc9651..cc9c5ade 100755 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -178,6 +178,7 @@ en: sessions: Sessions settings: Room Settings start: Start + unavailable: This room is currently unavailable due to the owner's email not being verified. update_settings_error: There was an error updating the room settings update_settings_success: Room settings successfully updated wait: @@ -222,6 +223,11 @@ en: For details, see the %{href}. update: Update verify: - title: Verify your email - resend: Resend verification email accept: Verify + activated: Account verified! + already_verified: Account has already been verified + invalid: Invalid verification link + not_verified: Your account has not been verified yet. + resend: Resend verification email + signin: Please sign in to access your account. + title: Verify your email diff --git a/config/routes.rb b/config/routes.rb index f5c7b5d4..3f7be57b 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -34,14 +34,15 @@ Rails.application.routes.draw do # Password reset resources. resources :password_resets, only: [:new, :create, :edit, :update] + # Account activation resources + scope '/account_activations' do + get '/', to: 'account_activations#show', as: :account_activation + get '/edit', to: 'account_activations#edit', as: :edit_account_activation + get '/resend', to: 'account_activations#resend', as: :resend_email + end + # User resources. scope '/u' do - # Verification Routes - scope '/verify' do - match '/resend', to: 'users#resend', via: [:get, :post], as: :resend - match '/confirm/:user_uid', to: 'users#confirm', via: [:get, :post], as: :confirm - end - # Handles login of greenlight provider accounts. post '/login', to: 'sessions#create', as: :create_session diff --git a/db/migrate/20190206210049_add_activation_to_users.rb b/db/migrate/20190206210049_add_activation_to_users.rb new file mode 100644 index 00000000..771ecf04 --- /dev/null +++ b/db/migrate/20190206210049_add_activation_to_users.rb @@ -0,0 +1,8 @@ +# frozen_string_literal: true + +class AddActivationToUsers < ActiveRecord::Migration[5.0] + def change + add_column :users, :activation_digest, :string + add_column :users, :activated_at, :datetime + end +end diff --git a/db/schema.rb b/db/schema.rb index a2340fdb..adcdd5f8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20190122210632) do +ActiveRecord::Schema.define(version: 20190206210049) do create_table "rooms", force: :cascade do |t| t.integer "user_id" @@ -40,14 +40,16 @@ ActiveRecord::Schema.define(version: 20190122210632) do t.string "social_uid" t.string "image" t.string "password_digest" - t.boolean "accepted_terms", default: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.boolean "email_verified", default: false - t.string "language", default: "default" - t.string "role", default: "moderator" + t.boolean "accepted_terms", default: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.boolean "email_verified", default: false + t.string "language", default: "default" + t.string "role", default: "moderator" t.string "reset_digest" t.datetime "reset_sent_at" + t.string "activation_digest" + t.datetime "activated_at" t.index ["password_digest"], name: "index_users_on_password_digest", unique: true t.index ["room_id"], name: "index_users_on_room_id" end diff --git a/spec/controllers/account_activations_controller_spec.rb b/spec/controllers/account_activations_controller_spec.rb new file mode 100644 index 00000000..37bab271 --- /dev/null +++ b/spec/controllers/account_activations_controller_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +# BigBlueButton open source conferencing system - http://www.bigbluebutton.org/. +# +# Copyright (c) 2018 BigBlueButton Inc. and by respective authors (see below). +# +# This program is free software; you can redistribute it and/or modify it under the +# terms of the GNU Lesser General Public License as published by the Free Software +# Foundation; either version 3.0 of the License, or (at your option) any later +# version. +# +# BigBlueButton is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A +# PARTICULAR PURPOSE. See the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License along +# with BigBlueButton; if not, see . + +require "rails_helper" + +describe AccountActivationsController, type: :controller do + before { allow(Rails.configuration).to receive(:allow_user_signup).and_return(true) } + before { allow(Rails.configuration).to receive(:enable_email_verification).and_return(true) } + + describe "GET #show" do + it "redirects to main room if signed in" do + user = create(:user, provider: "greenlight") + @request.session[:user_id] = user.id + + get :show, params: { email: user.email } + + expect(response).to redirect_to(user.main_room) + end + + 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 } + + expect(response).to render_template(:verify) + end + end + + describe "GET #edit" 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.reload + + expect(@user.email_verified).to eq(true) + expect(flash[:success]).to be_present + expect(response).to redirect_to(root_path) + end + + it "does not activate 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: "fake_token" } + @user.reload + + expect(@user.email_verified).to eq(false) + expect(flash[:alert]).to be_present + expect(response).to redirect_to(root_path) + 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 } + + expect(flash[:alert]).to be_present + expect(response).to redirect_to(root_path) + end + end + + describe "GET #resend" 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) + 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") + + get :resend, params: { email: user.email } + + expect(flash[:alert]).to be_present + expect(response).to redirect_to(root_path) + end + end +end diff --git a/spec/controllers/rooms_controller_spec.rb b/spec/controllers/rooms_controller_spec.rb index 5270e1fb..904fd145 100644 --- a/spec/controllers/rooms_controller_spec.rb +++ b/spec/controllers/rooms_controller_spec.rb @@ -72,6 +72,15 @@ describe RoomsController, type: :controller do get :show, params: { room_uid: "non_existent" } end.to raise_error(ActiveRecord::RecordNotFound) end + + it "redirects to root if owner of room is not verified" do + @owner.update_attribute(:email_verified, false) + + post :show, params: { room_uid: @owner.main_room } + + expect(flash[:alert]).to be_present + expect(response).to redirect_to(root_path) + end end describe "POST #create" do @@ -151,6 +160,15 @@ describe RoomsController, type: :controller do expect(response).to redirect_to(@user.main_room.join_path(@owner.name, { user_is_moderator: true }, @owner.uid)) end + + it "redirects to root if owner of room is not verified" do + @owner.update_attribute(:email_verified, false) + + post :join, params: { room_uid: @room, join_name: @owner.name } + + expect(flash[:alert]).to be_present + expect(response).to redirect_to(root_path) + end end describe "DELETE #destroy" do diff --git a/spec/controllers/sessions_controller_spec.rb b/spec/controllers/sessions_controller_spec.rb index 9ec4f0c9..2faaa7f1 100644 --- a/spec/controllers/sessions_controller_spec.rb +++ b/spec/controllers/sessions_controller_spec.rb @@ -72,6 +72,21 @@ describe SessionsController, type: :controller do expect(@request.session[:user_id]).to be_nil end + + it "should not login user if account is not verified" do + @secondary_user = create(:user, email_verified: false, provider: "greenlight", + password: "example", password_confirmation: "example") + + post :create, params: { + session: { + email: @secondary_user.email, + password: "example", + }, + } + + expect(@request.session[:user_id]).to be_nil + expect(response).to redirect_to(account_activation_path(email: @secondary_user.email)) + end end describe "GET/POST #omniauth" do diff --git a/spec/controllers/users_controller_spec.rb b/spec/controllers/users_controller_spec.rb index e29f990b..68996ace 100644 --- a/spec/controllers/users_controller_spec.rb +++ b/spec/controllers/users_controller_spec.rb @@ -166,72 +166,6 @@ describe UsersController, type: :controller do end end - describe "GET | POST #resend" do - before { allow(Rails.configuration).to receive(:allow_user_signup).and_return(true) } - before { allow(Rails.configuration).to receive(:enable_email_verification).and_return(true) } - - it "redirects to main room if verified" do - params = random_valid_user_params - post :create, params: params - - u = User.find_by(name: params[:user][:name], email: params[:user][:email]) - u.email_verified = false - - get :resend - expect(response).to render_template(:verify) - end - - it "resend email upon click if unverified" do - params = random_valid_user_params - post :create, params: params - - u = User.find_by(name: params[:user][:name], email: params[:user][:email]) - u.email_verified = false - - expect { post :resend, params: { email_verified: false } }.to change { ActionMailer::Base.deliveries.count }.by(1) - expect(response).to render_template(:verify) - end - - it "should raise if there there is a delivery failure" do - params = random_valid_user_params - post :create, params: params - - u = User.find_by(name: params[:user][:name], email: params[:user][:email]) - u.email_verified = false - - expect do - post :resend, params: { email_verified: false } - raise Net::SMTPAuthenticationError - end.to raise_error { Net::SMTPAuthenticationError } - end - end - - describe "GET | POST #confirm" do - before { allow(Rails.configuration).to receive(:allow_user_signup).and_return(true) } - before { allow(Rails.configuration).to receive(:enable_email_verification).and_return(true) } - - it "redirects to main room if already verified" do - params = random_valid_user_params - post :create, params: params - - u = User.find_by(name: params[:user][:name], email: params[:user][:email]) - - post :confirm, params: { user_uid: u.uid, email_verified: true } - expect(response).to redirect_to(room_path(u.main_room)) - end - - it "renders confirmation pane if unverified" do - params = random_valid_user_params - post :create, params: params - - u = User.find_by(name: params[:user][:name], email: params[:user][:email]) - u.email_verified = false - - get :confirm, params: { user_uid: u.uid } - expect(response).to render_template(:verify) - end - end - describe "GET #recordings" do before do @user1 = create(:user) diff --git a/spec/factories.rb b/spec/factories.rb index e62cceb0..de8a3975 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -29,6 +29,7 @@ FactoryBot.define do password_confirmation { password } accepted_terms { true } email_verified { true } + activated_at { Time.zone.now } end factory :room do