GRN-56: Correctly implemented the account verification flow (#367)

* Correctly implemented the account verification flow

* Fixed issues with redirect locations

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 <http://www.gnu.org/licenses/>.
+
+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

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

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

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
+        @user.send_activation_email(verification_link)
-        login(@user)
       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

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 <http://www.gnu.org/licenses/>.
+
+module AccountActivationsHelper
+end

app/models/user.rb

@@ -17,9 +17,10 @@
 # with BigBlueButton; if not, see <http://www.gnu.org/licenses/>.
 
 class User < ApplicationRecord
-  attr_accessor :reset_token
+  attr_accessor :reset_token, :activation_token
-  after_create :initialize_main_room
+  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

app/views/account_activations/verify.html.erb

@@ -20,13 +20,8 @@
         <h3 class="card-title"><%= t("verify.title") %></h3>
       </div>
       <div class="card-body">
-          <p> Your account has not been verified yet. </p>
+        <p><%= t("verify.not_verified") %></p>
-          <% if Rails.configuration.enable_email_verification && params[:user_uid] == current_user.uid %>
+        <%= render "/shared/components/resend_button" %>
-            <%= render "/shared/components/confirm_button" %>
-          <% else %>
-            <%= render "/shared/components/resend_button" %>
-          <% end %>
-        </form>
       </div>
     </div>
   </div>

app/views/shared/components/_resend_button.html.erb

@@ -14,5 +14,5 @@
 %>
 
 <div class="btn-list text-right pt-8">
-    <%= 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" %>
 </div>

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

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
 

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

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.boolean  "accepted_terms",    default: false
-    t.datetime "created_at",                            null: false
+    t.datetime "created_at",                              null: false
-    t.datetime "updated_at",                            null: false
+    t.datetime "updated_at",                              null: false
-    t.boolean  "email_verified",  default: false
+    t.boolean  "email_verified",    default: false
-    t.string   "language",        default: "default"
+    t.string   "language",          default: "default"
-    t.string   "role",            default: "moderator"
+    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

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 <http://www.gnu.org/licenses/>.
+
+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

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

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

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)

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