Send a confirmation email when the user adds a secondary email address. Utilizes the Devise `confirmable` capabilities. Issue #37385

19 files changed, 109 insertions, 89 deletions

diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index 306afb65f10..51c26b9c17e 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -11,11 +11,12 @@ class ConfirmationsController < Devise::ConfirmationsController
   end
 
   def after_confirmation_path_for(resource_name, resource)
-    if signed_in?(resource_name)
+    # incoming resource can either be a :user or an :email
+    if signed_in?(:user)
       after_sign_in_path_for(resource)
     else
       flash[:notice] += " Please sign in."
-      new_session_path(resource_name)
+      new_session_path(:user)
     end
   end
 end
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index ddb67d1c4d1..60426e4f14f 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -6,10 +6,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
 
   def create
     @email = Emails::CreateService.new(current_user, email_params).execute
-
-    if @email.errors.blank?
-      NotificationService.new.new_email(@email)
-    else
+    unless @email.errors.blank?
       flash[:alert] = @email.errors.full_messages.first
     end
 
diff --git a/app/mailers/emails/profile.rb b/app/mailers/emails/profile.rb
index c401030e34a..4f5edeb9bda 100644
--- a/app/mailers/emails/profile.rb
+++ b/app/mailers/emails/profile.rb
@@ -7,12 +7,6 @@ module Emails
       mail(to: @user.notification_email, subject: subject("Account was created for you"))
     end
 
-    def new_email_email(email_id)
-      @email = Email.find(email_id)
-      @current_user = @user = @email.user
-      mail(to: @user.notification_email, subject: subject("Email was added to your account"))
-    end
-
     def new_ssh_key_email(key_id)
       @key = Key.find_by(id: key_id)
 
diff --git a/app/models/email.rb b/app/models/email.rb
index 826d4f16edb..6254d66cad9 100644
--- a/app/models/email.rb
+++ b/app/models/email.rb
@@ -7,6 +7,9 @@ class Email < ActiveRecord::Base
   validates :email, presence: true, uniqueness: true, email: true
   validate :unique_email, if: ->(email) { email.email_changed? }
 
+  devise :confirmable
+  self.reconfirmable = false  # currently email can't be changed, no need to reconfirm
+
   def email=(value)
     write_attribute(:email, value.downcase.strip)
   end
diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb
index e2a80db06a6..8d5da459882 100644
--- a/app/services/notification_service.rb
+++ b/app/services/notification_service.rb
@@ -31,13 +31,6 @@ class NotificationService
     end
   end
 
-  # Always notify user about email added to profile
-  def new_email(email)
-    if email.user&.can?(:receive_notifications)
-      mailer.new_email_email(email.id).deliver_later
-    end
-  end
-
   # When create an issue we should send an email to:
   #
   #  * issue assignee if their notification level is not Disabled
diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
new file mode 100644
index 00000000000..a716d98415c
--- /dev/null
+++ b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
@@ -0,0 +1,8 @@
+#content
+  = email_default_heading("#{@resource.user.name}, you've added a secondary email!")
+  %p Click the link below to confirm your email address (#{@resource.email})
+#cta
+  = link_to 'Confirm your email address', confirmation_url(@resource, confirmation_token: @token)
+%p
+  If this email was added in error, you can remove it here:
+  = link_to "Emails", profile_emails_url
diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb b/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb
new file mode 100644
index 00000000000..2d8de854cf6
--- /dev/null
+++ b/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb
@@ -0,0 +1,7 @@
+<%= @resource.user.name %>, you've added a secondary email!
+
+You can confirm your email (<%= @resource.email %>) through the link below:
+
+<%= confirmation_url(@resource, confirmation_token: @token) %>
+
+If this email was added in error, you can remove it here: <%= profile_emails_url %>
diff --git a/app/views/devise/mailer/confirmation_instructions.html.haml b/app/views/devise/mailer/confirmation_instructions.html.haml
index a508b7537a2..c9e13a636d7 100644
--- a/app/views/devise/mailer/confirmation_instructions.html.haml
+++ b/app/views/devise/mailer/confirmation_instructions.html.haml
@@ -1,15 +1,18 @@
-- if @resource.unconfirmed_email.present?
-  #content
-    = email_default_heading(@resource.unconfirmed_email)
-    %p Click the link below to confirm your email address.
-  #cta
-    = link_to 'Confirm your email address', confirmation_url(@resource, confirmation_token: @token)
+- if @resource.is_a?(Email)
+  = render partial: 'confirmation_instructions_secondary'
 - else
-  #content
-    - if Gitlab.com?
-      = email_default_heading('Thanks for signing up to GitLab!')
-    - else
-      = email_default_heading("Welcome, #{@resource.name}!")
-    %p To get started, click the link below to confirm your account.
-  #cta
-    = link_to 'Confirm your account', confirmation_url(@resource, confirmation_token: @token)
+  - if @resource.unconfirmed_email.present?
+    #content
+      = email_default_heading(@resource.unconfirmed_email)
+      %p Click the link below to confirm your email address.
+    #cta
+      = link_to 'Confirm your email address', confirmation_url(@resource, confirmation_token: @token)
+  - else
+    #content
+      - if Gitlab.com?
+        = email_default_heading('Thanks for signing up to GitLab!')
+      - else
+        = email_default_heading("Welcome, #{@resource.name}!")
+      %p To get started, click the link below to confirm your account.
+    #cta
+      = link_to 'Confirm your account', confirmation_url(@resource, confirmation_token: @token)
diff --git a/app/views/devise/mailer/confirmation_instructions.text.erb b/app/views/devise/mailer/confirmation_instructions.text.erb
index 9f76edb76a4..d4bfb3af76f 100644
--- a/app/views/devise/mailer/confirmation_instructions.text.erb
+++ b/app/views/devise/mailer/confirmation_instructions.text.erb
@@ -1,3 +1,6 @@
+<% if @resource.is_a?(Email) %>
+<%= render partial: 'confirmation_instructions_secondary' %>
+<% else %>
 Welcome, <%= @resource.name %>!
 
 <% if @resource.unconfirmed_email.present? %>
@@ -7,3 +10,4 @@ You can confirm your account through the link below:
 <% end %>
 
 <%= confirmation_url(@resource, confirmation_token: @token) %>
+<% end %>
diff --git a/app/views/notify/new_email_email.html.haml b/app/views/notify/new_email_email.html.haml
deleted file mode 100644
index 4a0448a573c..00000000000
--- a/app/views/notify/new_email_email.html.haml
+++ /dev/null
@@ -1,10 +0,0 @@
-%p
-  Hi #{@user.name}!
-%p
-  A new email was added to your account:
-%p
-  email:
-  %code= @email.email
-%p
-  If this email was added in error, you can remove it here:
-  = link_to "Emails", profile_emails_url
diff --git a/app/views/notify/new_email_email.text.erb b/app/views/notify/new_email_email.text.erb
deleted file mode 100644
index 51cba99ad0d..00000000000
--- a/app/views/notify/new_email_email.text.erb
+++ /dev/null
@@ -1,7 +0,0 @@
-Hi <%= @user.name %>!
-
-A new email was added to your account:
-
-email.................. <%= @email.email %>
-
-If this email was added in error, you can remove it here: <%= profile_emails_url %>
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index 612ecbbb96a..a653df98bc1 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -47,4 +47,6 @@
               %span.label.label-info Public email
             - if email.email === current_user.notification_email
               %span.label.label-info Notification email
+            - if !email.confirmed?
+              %span.label.label-warning Unconfirmed
             = link_to 'Remove', profile_email_path(email), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-sm btn-warning prepend-left-10'
diff --git a/changelogs/unreleased/feature-verify_secondary_emails.yml b/changelogs/unreleased/feature-verify_secondary_emails.yml
new file mode 100644
index 00000000000..e1ecc527f85
--- /dev/null
+++ b/changelogs/unreleased/feature-verify_secondary_emails.yml
@@ -0,0 +1,5 @@
+---
+title: A confirmation email is now sent when adding a secondary email address
+merge_request:
+author: digitalmoksha
+type: added
diff --git a/config/routes/user.rb b/config/routes/user.rb
index e682dcd6663..483d34ab4f3 100644
--- a/config/routes/user.rb
+++ b/config/routes/user.rb
@@ -11,6 +11,11 @@ devise_scope :user do
   get '/users/almost_there' => 'confirmations#almost_there'
 end
 
+# for secondary email confirmations
+devise_for   :emails, controllers: { confirmations: :confirmations }
+devise_scope :email do
+end
+
 scope(constraints: { username: Gitlab::PathRegex.root_namespace_route_regex }) do
   scope(path: 'users/:username',
         as: :user,
diff --git a/db/migrate/20170904092148_add_email_confirmation.rb b/db/migrate/20170904092148_add_email_confirmation.rb
new file mode 100644
index 00000000000..b4c574b6a99
--- /dev/null
+++ b/db/migrate/20170904092148_add_email_confirmation.rb
@@ -0,0 +1,34 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddEmailConfirmation < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  # When a migration requires downtime you **must** uncomment the following
+  # constant and define a short and easy to understand explanation as to why the
+  # migration requires downtime.
+  # DOWNTIME_REASON = ''
+
+  # When using the methods "add_concurrent_index", "remove_concurrent_index" or
+  # "add_column_with_default" you must disable the use of transactions
+  # as these methods can not run in an existing transaction.
+  # When using "add_concurrent_index" or "remove_concurrent_index" methods make sure
+  # that either of them is the _only_ method called in the migration,
+  # any other changes should go in a separate migration.
+  # This ensures that upon failure _only_ the index creation or removing fails
+  # and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    add_column :emails, :confirmation_token, :string
+    add_column :emails, :confirmed_at, :datetime
+    add_column :emails, :confirmation_sent_at, :datetime
+    add_index  :emails, :confirmation_token, unique: true
+  end
+end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index bdebda58d3f..8b44639dd7d 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -329,7 +329,6 @@ module API
         email = Emails::CreateService.new(user, declared_params(include_missing: false)).execute
 
         if email.errors.blank?
-          NotificationService.new.new_email(email)
           present email, with: Entities::Email
         else
           render_validation_error!(email)
@@ -675,7 +674,6 @@ module API
         email = Emails::CreateService.new(current_user, declared_params).execute
 
         if email.errors.blank?
-          NotificationService.new.new_email(email)
           present email, with: Entities::Email
         else
           render_validation_error!(email)
diff --git a/spec/controllers/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb
new file mode 100644
index 00000000000..00862f12b7e
--- /dev/null
+++ b/spec/controllers/profiles/emails_controller_spec.rb
@@ -0,0 +1,20 @@
+require 'spec_helper'
+
+describe Profiles::EmailsController do
+
+  let(:user) { create(:user) }
+
+  before do
+    sign_in(user)
+  end
+
+  describe '#create' do
+    let(:email_params) { {email: "add_email@example.com" } }
+
+    it 'sends an email confirmation' do
+      expect {post(:create, { email: email_params })}.to change { ActionMailer::Base.deliveries.size }
+      expect(ActionMailer::Base.deliveries.last.to).to eq [email_params[:email]]
+      expect(ActionMailer::Base.deliveries.last.subject).to match "Confirmation instructions"
+    end
+  end
+end
diff --git a/spec/mailers/emails/profile_spec.rb b/spec/mailers/emails/profile_spec.rb
index 09e5094cf84..1f7be415e35 100644
--- a/spec/mailers/emails/profile_spec.rb
+++ b/spec/mailers/emails/profile_spec.rb
@@ -120,29 +120,4 @@ describe Emails::Profile do
       it { expect { Notify.new_gpg_key_email('foo') }.not_to raise_error }
     end
   end
-
-  describe 'user added email' do
-    let(:email) { create(:email) }
-
-    subject { Notify.new_email_email(email.id) }
-
-    it_behaves_like 'it should not have Gmail Actions links'
-    it_behaves_like 'a user cannot unsubscribe through footer link'
-
-    it 'is sent to the new user' do
-      is_expected.to deliver_to email.user.email
-    end
-
-    it 'has the correct subject' do
-      is_expected.to have_subject /^Email was added to your account$/i
-    end
-
-    it 'contains the new email address' do
-      is_expected.to have_body_text /#{email.email}/
-    end
-
-    it 'includes a link to emails page' do
-      is_expected.to have_body_text /#{profile_emails_path}/
-    end
-  end
 end
diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb
index f4b36eb7eeb..b64ca5be8fc 100644
--- a/spec/services/notification_service_spec.rb
+++ b/spec/services/notification_service_spec.rb
@@ -105,18 +105,6 @@ describe NotificationService, :mailer do
     end
   end
 
-  describe 'Email' do
-    describe '#new_email' do
-      let!(:email) { create(:email) }
-
-      it { expect(notification.new_email(email)).to be_truthy }
-
-      it 'sends email to email owner' do
-        expect { notification.new_email(email) }.to change { ActionMailer::Base.deliveries.size }.by(1)
-      end
-    end
-  end
-
   describe 'Notes' do
     context 'issue note' do
       let(:project) { create(:project, :private) }