From f9f467227538df0ce2012df39dfdcf55fb260f94 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Mon, 4 Sep 2017 19:23:33 +0200
Subject: [PATCH 01/30] Send a confirmation email when the user adds a
 secondary email address. Utilizes the Devise `confirmable` capabilities.
 Issue #37385

---
 app/controllers/confirmations_controller.rb   |  5 +--
 app/controllers/profiles/emails_controller.rb |  5 +--
 app/mailers/emails/profile.rb                 |  6 ----
 app/models/email.rb                           |  3 ++
 app/services/notification_service.rb          |  7 ----
 ...firmation_instructions_secondary.html.haml |  8 +++++
 ...nfirmation_instructions_secondary.text.erb |  7 ++++
 .../confirmation_instructions.html.haml       | 31 +++++++++--------
 .../mailer/confirmation_instructions.text.erb |  4 +++
 app/views/notify/new_email_email.html.haml    | 10 ------
 app/views/notify/new_email_email.text.erb     |  7 ----
 app/views/profiles/emails/index.html.haml     |  2 ++
 .../feature-verify_secondary_emails.yml       |  5 +++
 config/routes/user.rb                         |  5 +++
 .../20170904092148_add_email_confirmation.rb  | 34 +++++++++++++++++++
 lib/api/users.rb                              |  2 --
 .../profiles/emails_controller_spec.rb        | 20 +++++++++++
 spec/mailers/emails/profile_spec.rb           | 25 --------------
 spec/services/notification_service_spec.rb    | 12 -------
 19 files changed, 109 insertions(+), 89 deletions(-)
 create mode 100644 app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
 create mode 100644 app/views/devise/mailer/_confirmation_instructions_secondary.text.erb
 delete mode 100644 app/views/notify/new_email_email.html.haml
 delete mode 100644 app/views/notify/new_email_email.text.erb
 create mode 100644 changelogs/unreleased/feature-verify_secondary_emails.yml
 create mode 100644 db/migrate/20170904092148_add_email_confirmation.rb
 create mode 100644 spec/controllers/profiles/emails_controller_spec.rb

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) }

From 47c6525a98d3b3d73301a9dd6b75464f2cff3a7b Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 6 Sep 2017 18:24:46 +0200
Subject: [PATCH 02/30] remove unnecessary devise_scope

---
 config/routes/user.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/config/routes/user.rb b/config/routes/user.rb
index 483d34ab4f3..c4b34402068 100644
--- a/config/routes/user.rb
+++ b/config/routes/user.rb
@@ -13,8 +13,6 @@ 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',

From b1e68922a462a4a9ddaa62adefb3bcf350c75798 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 6 Sep 2017 18:28:09 +0200
Subject: [PATCH 03/30] refactored the account confirmation into a partial. 
 Also made the text in the `.text` version consistent with the `.html` version

---
 ...onfirmation_instructions_account.html.haml | 15 ++++++++++++++
 ...confirmation_instructions_account.text.erb | 14 +++++++++++++
 ...firmation_instructions_secondary.html.haml |  2 +-
 ...nfirmation_instructions_secondary.text.erb |  4 ++--
 .../confirmation_instructions.html.haml       | 20 +++----------------
 .../mailer/confirmation_instructions.text.erb | 14 +++----------
 6 files changed, 38 insertions(+), 31 deletions(-)
 create mode 100644 app/views/devise/mailer/_confirmation_instructions_account.html.haml
 create mode 100644 app/views/devise/mailer/_confirmation_instructions_account.text.erb

diff --git a/app/views/devise/mailer/_confirmation_instructions_account.html.haml b/app/views/devise/mailer/_confirmation_instructions_account.html.haml
new file mode 100644
index 00000000000..a508b7537a2
--- /dev/null
+++ b/app/views/devise/mailer/_confirmation_instructions_account.html.haml
@@ -0,0 +1,15 @@
+- 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_account.text.erb b/app/views/devise/mailer/_confirmation_instructions_account.text.erb
new file mode 100644
index 00000000000..01f09aa763d
--- /dev/null
+++ b/app/views/devise/mailer/_confirmation_instructions_account.text.erb
@@ -0,0 +1,14 @@
+<% if @resource.unconfirmed_email.present? %>
+<%= @resource.unconfirmed_email %>,
+
+Use the link below to confirm your email address.
+<% else %>
+  <% if Gitlab.com? %>
+Thanks for signing up to GitLab!
+  <% else %>
+Welcome, <%= @resource.name %>!
+  <% end %>
+To get started, use the link below to confirm your account.
+<% end %>  
+
+<%= confirmation_url(@resource, confirmation_token: @token) %>
diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
index a716d98415c..83f5edbde7a 100644
--- a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
+++ b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
@@ -1,5 +1,5 @@
 #content
-  = email_default_heading("#{@resource.user.name}, you've added a secondary email!")
+  = email_default_heading("#{@resource.user.name}, you've added an additional 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)
diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb b/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb
index 2d8de854cf6..a3b28cb0b84 100644
--- a/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb
+++ b/app/views/devise/mailer/_confirmation_instructions_secondary.text.erb
@@ -1,6 +1,6 @@
-<%= @resource.user.name %>, you've added a secondary email!
+<%= @resource.user.name %>, you've added an additional email!
 
-You can confirm your email (<%= @resource.email %>) through the link below:
+Use the link below to confirm your email address (<%= @resource.email %>)
 
 <%= confirmation_url(@resource, confirmation_token: @token) %>
 
diff --git a/app/views/devise/mailer/confirmation_instructions.html.haml b/app/views/devise/mailer/confirmation_instructions.html.haml
index c9e13a636d7..a0fb687e152 100644
--- a/app/views/devise/mailer/confirmation_instructions.html.haml
+++ b/app/views/devise/mailer/confirmation_instructions.html.haml
@@ -1,18 +1,4 @@
-- if @resource.is_a?(Email)
-  = render partial: 'confirmation_instructions_secondary'
+- if @resource.is_a?(User)
+  = render partial: 'confirmation_instructions_account'
 - else
-  - 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)
+  = render partial: 'confirmation_instructions_secondary'
diff --git a/app/views/devise/mailer/confirmation_instructions.text.erb b/app/views/devise/mailer/confirmation_instructions.text.erb
index d4bfb3af76f..057bb0b23f9 100644
--- a/app/views/devise/mailer/confirmation_instructions.text.erb
+++ b/app/views/devise/mailer/confirmation_instructions.text.erb
@@ -1,13 +1,5 @@
-<% if @resource.is_a?(Email) %>
+<% if @resource.is_a?(User) %>
+<%= render partial: 'confirmation_instructions_account' %>
+<% else %>
 <%= render partial: 'confirmation_instructions_secondary' %>
-<% else %>
-Welcome, <%= @resource.name %>!
-
-<% if @resource.unconfirmed_email.present? %>
-You can confirm your email (<%= @resource.unconfirmed_email %>) through the link below:
-<% else %>
-You can confirm your account through the link below:
-<% end %>
-
-<%= confirmation_url(@resource, confirmation_token: @token) %>
 <% end %>

From a6045df701aaf21f9af09227564eddb891f5e3f2 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 6 Sep 2017 18:39:55 +0200
Subject: [PATCH 04/30] move #cta properly under #content

---
 .../_confirmation_instructions_account.html.haml       |  8 ++++----
 .../_confirmation_instructions_secondary.html.haml     | 10 +++++-----
 2 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/views/devise/mailer/_confirmation_instructions_account.html.haml b/app/views/devise/mailer/_confirmation_instructions_account.html.haml
index a508b7537a2..3090fc0a2c0 100644
--- a/app/views/devise/mailer/_confirmation_instructions_account.html.haml
+++ b/app/views/devise/mailer/_confirmation_instructions_account.html.haml
@@ -2,8 +2,8 @@
   #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)
+    #cta
+      = link_to 'Confirm your email address', confirmation_url(@resource, confirmation_token: @token)
 - else
   #content
     - if Gitlab.com?
@@ -11,5 +11,5 @@
     - 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)
+    #cta
+      = link_to 'Confirm your account', confirmation_url(@resource, confirmation_token: @token)
diff --git a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
index 83f5edbde7a..3d0a1f622a5 100644
--- a/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
+++ b/app/views/devise/mailer/_confirmation_instructions_secondary.html.haml
@@ -1,8 +1,8 @@
 #content
   = email_default_heading("#{@resource.user.name}, you've added an additional 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
+  #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

From ecdf851335ff10a6daade26cbf79bea8b897858e Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sat, 9 Sep 2017 10:29:21 +0200
Subject: [PATCH 05/30] changed from `if !` to `unless`

---
 app/views/profiles/emails/index.html.haml | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index a653df98bc1..f6f5a363194 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -47,6 +47,6 @@
               %span.label.label-info Public email
             - if email.email === current_user.notification_email
               %span.label.label-info Notification email
-            - if !email.confirmed?
+            - unless 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'

From 86698c960bb95e51bc6141b361ba3f1ee79abcdf Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sat, 9 Sep 2017 11:05:59 +0200
Subject: [PATCH 06/30] use `add_concurrent_index` to add the
 :confirmation_token index

---
 .../20170904092148_add_email_confirmation.rb  |  1 -
 ...0909090114_add_email_confirmation_index.rb | 36 +++++++++++++++++++
 2 files changed, 36 insertions(+), 1 deletion(-)
 create mode 100644 db/migrate/20170909090114_add_email_confirmation_index.rb

diff --git a/db/migrate/20170904092148_add_email_confirmation.rb b/db/migrate/20170904092148_add_email_confirmation.rb
index b4c574b6a99..b6ecf1dd9da 100644
--- a/db/migrate/20170904092148_add_email_confirmation.rb
+++ b/db/migrate/20170904092148_add_email_confirmation.rb
@@ -29,6 +29,5 @@ class AddEmailConfirmation < ActiveRecord::Migration
     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/db/migrate/20170909090114_add_email_confirmation_index.rb b/db/migrate/20170909090114_add_email_confirmation_index.rb
new file mode 100644
index 00000000000..e488f339247
--- /dev/null
+++ b/db/migrate/20170909090114_add_email_confirmation_index.rb
@@ -0,0 +1,36 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddEmailConfirmationIndex < 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!
+
+  # Not necessary to remove duplicates, as :confirmation_token is a new column
+  def up
+    add_concurrent_index :emails, :confirmation_token, unique: true
+  end
+
+  def down
+    remove_index :emails, :confirmation_token if index_exists?(:emails, :confirmation_token)
+  end
+end

From f5937926f22095b68ae8bf363e737730df1848ce Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sat, 9 Sep 2017 15:10:34 +0200
Subject: [PATCH 07/30] refactored the Spinach feature
 `features/profile/emails.feature` into an rspec feature

---
 spec/features/profiles/emails_spec.rb | 56 +++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)
 create mode 100644 spec/features/profiles/emails_spec.rb

diff --git a/spec/features/profiles/emails_spec.rb b/spec/features/profiles/emails_spec.rb
new file mode 100644
index 00000000000..98b8c607cb0
--- /dev/null
+++ b/spec/features/profiles/emails_spec.rb
@@ -0,0 +1,56 @@
+require 'rails_helper'
+
+feature 'Profile > Emails' do
+  let(:user) { create(:user) }
+
+  before do
+    login_as(user)
+  end
+
+  describe 'User adds an email' do
+    before do
+      visit profile_emails_path
+    end
+
+    scenario 'saves the new email' do
+      fill_in('Email', with: 'my@email.com')
+      click_button('Add email address')
+
+      expect(page).to have_content('my@email.com Unverified')
+      expect(page).to have_content('user1@example.org Verified')
+      expect(page).to have_content('Resend Confirmation Email')
+    end
+
+    scenario 'does not add a duplicate email' do
+      fill_in('Email', with: user.email)
+      click_button('Add email address')
+
+      email = user.emails.find_by(email: user.email)
+      expect(email).to be_nil
+      expect(page).to have_content('Email has already been taken')
+    end
+  end
+
+  scenario 'User removes email' do
+    user.emails.create(email: 'my@email.com')
+    visit profile_emails_path
+    expect(page).to have_content("my@email.com")
+
+    click_link('Remove')
+    expect(page).to_not have_content("my@email.com")
+  end
+
+  scenario 'User confirms email' do
+    email = user.emails.create(email: 'my@email.com')
+    visit profile_emails_path
+    expect(page).to have_content("my@email.com Unverified")
+
+    email.confirm
+    expect(email.confirmed?).to be_truthy
+
+    visit profile_emails_path
+    expect(page).to have_content("my@email.com Verified")
+  end
+
+  scenario ''
+end

From c56208f98028d10f8f2ab315ae52e9fcacc45399 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sat, 9 Sep 2017 15:18:40 +0200
Subject: [PATCH 08/30] refactored `_email_with_badge` partial from gpg into
 shared, so it can be used with the email profile page as well

---
 app/assets/stylesheets/pages/profile.scss                     | 4 ++--
 app/views/profiles/gpg_keys/_key.html.haml                    | 2 +-
 .../{profiles/gpg_keys => shared}/_email_with_badge.html.haml | 4 ++--
 3 files changed, 5 insertions(+), 5 deletions(-)
 rename app/views/{profiles/gpg_keys => shared}/_email_with_badge.html.haml (79%)

diff --git a/app/assets/stylesheets/pages/profile.scss b/app/assets/stylesheets/pages/profile.scss
index c5d6ff66dd6..8b1f4b75693 100644
--- a/app/assets/stylesheets/pages/profile.scss
+++ b/app/assets/stylesheets/pages/profile.scss
@@ -392,11 +392,11 @@ table.u2f-registrations {
   }
 }
 
-.gpg-email-badge {
+.email-badge {
   display: inline;
   margin-right: $gl-padding / 2;
 
-  .gpg-email-badge-email {
+  .email-badge-email {
     display: inline;
     margin-right: $gl-padding / 4;
   }
diff --git a/app/views/profiles/gpg_keys/_key.html.haml b/app/views/profiles/gpg_keys/_key.html.haml
index b04981f90e3..970e92aadaa 100644
--- a/app/views/profiles/gpg_keys/_key.html.haml
+++ b/app/views/profiles/gpg_keys/_key.html.haml
@@ -3,7 +3,7 @@
     = icon 'key', class: "settings-list-icon hidden-xs"
   .key-list-item-info
     - key.emails_with_verified_status.map do |email, verified|
-      = render partial: 'email_with_badge', locals: { email: email, verified: verified }
+      = render partial: 'shared/email_with_badge', locals: { email: email, verified: verified }
 
     .description
       %code= key.fingerprint
diff --git a/app/views/profiles/gpg_keys/_email_with_badge.html.haml b/app/views/shared/_email_with_badge.html.haml
similarity index 79%
rename from app/views/profiles/gpg_keys/_email_with_badge.html.haml
rename to app/views/shared/_email_with_badge.html.haml
index 5f7844584e1..b7bbc109238 100644
--- a/app/views/profiles/gpg_keys/_email_with_badge.html.haml
+++ b/app/views/shared/_email_with_badge.html.haml
@@ -2,7 +2,7 @@
 - css_classes << (verified ? 'verified': 'unverified')
 - text = verified ? 'Verified' : 'Unverified'
 
-.gpg-email-badge
-  .gpg-email-badge-email= email
+.email-badge
+  .email-badge-email= email
   %div{ class: css_classes }
     = text

From cf8a5bcaec99cc197ff556793febb8317e1db220 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sat, 9 Sep 2017 15:55:07 +0200
Subject: [PATCH 09/30] add verified/unverified labels to profile emails. added
 "Resend confirmation email" for unverified emails

---
 app/controllers/profiles/emails_controller.rb | 10 +++++++
 app/services/emails/confirm_service.rb        |  7 +++++
 app/views/profiles/emails/index.html.haml     | 13 +++++++---
 config/routes/profile.rb                      |  6 ++++-
 .../profiles/emails_controller_spec.rb        | 15 +++++++++++
 spec/features/profiles/emails_spec.rb         | 26 +++++++++++++++----
 spec/services/emails/confirm_service_spec.rb  | 16 ++++++++++++
 7 files changed, 83 insertions(+), 10 deletions(-)
 create mode 100644 app/services/emails/confirm_service.rb
 create mode 100644 spec/services/emails/confirm_service_spec.rb

diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 60426e4f14f..152b71e687d 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -24,6 +24,16 @@ class Profiles::EmailsController < Profiles::ApplicationController
     end
   end
 
+  def resend_confirmation_instructions
+    @email = current_user.emails.find(params[:id])
+    if @email && Emails::ConfirmService.new(current_user, email: @email.email).execute
+      flash[:notice] = "Confirmation email sent to #{@email.email}"
+    else
+      flash[:alert] = "There was a problem sending the confirmation email"
+    end
+    redirect_to profile_emails_url
+  end
+  
   private
 
   def email_params
diff --git a/app/services/emails/confirm_service.rb b/app/services/emails/confirm_service.rb
new file mode 100644
index 00000000000..45845ccecc5
--- /dev/null
+++ b/app/services/emails/confirm_service.rb
@@ -0,0 +1,7 @@
+module Emails
+  class ConfirmService < ::Emails::BaseService
+    def execute
+      Email.find_by_email!(@email).resend_confirmation_instructions
+    end
+  end
+end
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index f6f5a363194..1b02603543d 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -32,7 +32,7 @@
           All email addresses will be used to identify your commits.
     %ul.well-list
       %li
-        = @primary
+        = render partial: 'shared/email_with_badge', locals: { email: @primary, verified: current_user.confirmed? }
         %span.pull-right
           %span.label.label-success Primary email
           - if @primary === current_user.public_email
@@ -41,12 +41,17 @@
             %span.label.label-info Notification email
       - @emails.each do |email|
         %li
-          = email.email
+          = render partial: 'shared/email_with_badge', locals: { email: email.email, verified: email.confirmed? }
           %span.pull-right
             - if email.email === current_user.public_email
               %span.label.label-info Public email
             - if email.email === current_user.notification_email
               %span.label.label-info Notification email
             - unless 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'
+              - confirm_title = "#{email.confirmation_sent_at ? 'Resend' : 'Send'} confirmation email"
+              = link_to confirm_title, resend_confirmation_instructions_profile_email_path(email), method: :put, class: 'btn btn-sm btn-warning prepend-left-10'
+                
+            = link_to profile_email_path(email), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-sm btn-danger prepend-left-10' do
+              %span.sr-only Remove
+              = icon('trash')
+            
diff --git a/config/routes/profile.rb b/config/routes/profile.rb
index 3e4e6111ab8..ea4dd9b67ec 100644
--- a/config/routes/profile.rb
+++ b/config/routes/profile.rb
@@ -28,7 +28,11 @@ resource :profile, only: [:show, :update] do
         put :revoke
       end
     end
-    resources :emails, only: [:index, :create, :destroy]
+    resources :emails, only: [:index, :create, :destroy] do
+      member do
+        put :resend_confirmation_instructions
+      end
+    end
     resources :chat_names, only: [:index, :new, :create, :destroy] do
       collection do
         delete :deny
diff --git a/spec/controllers/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb
index 00862f12b7e..cf510aa5589 100644
--- a/spec/controllers/profiles/emails_controller_spec.rb
+++ b/spec/controllers/profiles/emails_controller_spec.rb
@@ -17,4 +17,19 @@ describe Profiles::EmailsController do
       expect(ActionMailer::Base.deliveries.last.subject).to match "Confirmation instructions"
     end
   end
+
+  describe '#resend_confirmation_instructions' do
+    let(:email_params) { {email: "add_email@example.com" } }
+
+    it 'resends an email confirmation' do
+      email = user.emails.create(email: 'add_email@example.com')
+      expect {put(:resend_confirmation_instructions, { id: email })}.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
+
+    it 'unable to resend an email confirmation' do
+      expect {put(:resend_confirmation_instructions, { id: 1 })}.to_not change { ActionMailer::Base.deliveries.size }
+    end
+  end
 end
diff --git a/spec/features/profiles/emails_spec.rb b/spec/features/profiles/emails_spec.rb
index 98b8c607cb0..a3e3ea4e7a2 100644
--- a/spec/features/profiles/emails_spec.rb
+++ b/spec/features/profiles/emails_spec.rb
@@ -17,8 +17,8 @@ feature 'Profile > Emails' do
       click_button('Add email address')
 
       expect(page).to have_content('my@email.com Unverified')
-      expect(page).to have_content('user1@example.org Verified')
-      expect(page).to have_content('Resend Confirmation Email')
+      expect(page).to have_content("#{user.email} Verified")
+      expect(page).to have_content('Resend confirmation email')
     end
 
     scenario 'does not add a duplicate email' do
@@ -43,14 +43,30 @@ feature 'Profile > Emails' do
   scenario 'User confirms email' do
     email = user.emails.create(email: 'my@email.com')
     visit profile_emails_path
-    expect(page).to have_content("my@email.com Unverified")
+    expect(page).to have_content("#{email.email} Unverified")
 
     email.confirm
     expect(email.confirmed?).to be_truthy
 
     visit profile_emails_path
-    expect(page).to have_content("my@email.com Verified")
+    expect(page).to have_content("#{email.email} Verified")
+  end
+  
+  scenario 'User re-sends confirmation email' do
+    email = user.emails.create(email: 'my@email.com')
+    visit profile_emails_path
+    
+    expect { click_link("Resend confirmation email") }.to change { ActionMailer::Base.deliveries.size }
+    expect(page).to have_content("Confirmation email sent to #{email.email}")
+  end
+  
+  scenario 'old unconfirmed emails show Send Confirmation button' do
+    email = user.emails.create(email: 'my@email.com')
+    email.update_attribute(:confirmation_sent_at, nil)
+    visit profile_emails_path
+
+    expect(page).to_not have_content('Resend confirmation email')
+    expect(page).to have_content('Send confirmation email')
   end
 
-  scenario ''
 end
diff --git a/spec/services/emails/confirm_service_spec.rb b/spec/services/emails/confirm_service_spec.rb
new file mode 100644
index 00000000000..2f348589136
--- /dev/null
+++ b/spec/services/emails/confirm_service_spec.rb
@@ -0,0 +1,16 @@
+require 'spec_helper'
+
+describe Emails::ConfirmService do
+  let(:user) { create(:user) }
+  let(:opts) { { email: 'new@email.com' } }
+
+  subject(:service) { described_class.new(user, opts) }
+
+  describe '#execute' do
+    it 'sends a confirmation email again' do
+      email = user.emails.create(email: opts[:email])
+      mail  = service.execute
+      expect(mail.subject).to eq('Confirmation instructions')
+    end
+  end
+end

From d2267beb8916de5003e06b8764f209508fca989f Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sat, 9 Sep 2017 18:32:27 +0200
Subject: [PATCH 10/30] tweaks for rubocop

---
 app/controllers/profiles/emails_controller.rb         |  2 +-
 db/migrate/20170904092148_add_email_confirmation.rb   |  4 ++--
 .../20170909090114_add_email_confirmation_index.rb    |  2 +-
 lib/gitlab/path_regex.rb                              |  1 +
 spec/controllers/profiles/emails_controller_spec.rb   |  7 +++----
 spec/features/profiles/emails_spec.rb                 | 11 +++++------
 spec/services/emails/confirm_service_spec.rb          |  2 +-
 7 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 152b71e687d..b5df4c9554b 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -33,7 +33,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
     end
     redirect_to profile_emails_url
   end
-  
+
   private
 
   def email_params
diff --git a/db/migrate/20170904092148_add_email_confirmation.rb b/db/migrate/20170904092148_add_email_confirmation.rb
index b6ecf1dd9da..17ff424b319 100644
--- a/db/migrate/20170904092148_add_email_confirmation.rb
+++ b/db/migrate/20170904092148_add_email_confirmation.rb
@@ -27,7 +27,7 @@ class AddEmailConfirmation < ActiveRecord::Migration
 
   def change
     add_column :emails, :confirmation_token, :string
-    add_column :emails, :confirmed_at, :datetime
-    add_column :emails, :confirmation_sent_at, :datetime
+    add_column :emails, :confirmed_at, :datetime_with_timezone
+    add_column :emails, :confirmation_sent_at, :datetime_with_timezone
   end
 end
diff --git a/db/migrate/20170909090114_add_email_confirmation_index.rb b/db/migrate/20170909090114_add_email_confirmation_index.rb
index e488f339247..a8c1023c482 100644
--- a/db/migrate/20170909090114_add_email_confirmation_index.rb
+++ b/db/migrate/20170909090114_add_email_confirmation_index.rb
@@ -31,6 +31,6 @@ class AddEmailConfirmationIndex < ActiveRecord::Migration
   end
 
   def down
-    remove_index :emails, :confirmation_token if index_exists?(:emails, :confirmation_token)
+    remove_concurrent_index :emails, :confirmation_token if index_exists?(:emails, :confirmation_token)
   end
 end
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index 7c02c9c5c48..b119cb823b0 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -30,6 +30,7 @@ module Gitlab
       ci
       dashboard
       deploy.html
+      emails
       explore
       favicon.ico
       files
diff --git a/spec/controllers/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb
index cf510aa5589..bdf040ba295 100644
--- a/spec/controllers/profiles/emails_controller_spec.rb
+++ b/spec/controllers/profiles/emails_controller_spec.rb
@@ -1,7 +1,6 @@
 require 'spec_helper'
 
 describe Profiles::EmailsController do
-
   let(:user) { create(:user) }
 
   before do
@@ -9,7 +8,7 @@ describe Profiles::EmailsController do
   end
 
   describe '#create' do
-    let(:email_params) { {email: "add_email@example.com" } }
+    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 }
@@ -19,7 +18,7 @@ describe Profiles::EmailsController do
   end
 
   describe '#resend_confirmation_instructions' do
-    let(:email_params) { {email: "add_email@example.com" } }
+    let(:email_params) { {email: "add_email@example.com"} }
 
     it 'resends an email confirmation' do
       email = user.emails.create(email: 'add_email@example.com')
@@ -29,7 +28,7 @@ describe Profiles::EmailsController do
     end
 
     it 'unable to resend an email confirmation' do
-      expect {put(:resend_confirmation_instructions, { id: 1 })}.to_not change { ActionMailer::Base.deliveries.size }
+      expect {put(:resend_confirmation_instructions, { id: 1 })}.not_to change { ActionMailer::Base.deliveries.size }
     end
   end
 end
diff --git a/spec/features/profiles/emails_spec.rb b/spec/features/profiles/emails_spec.rb
index a3e3ea4e7a2..3085e1ee7bc 100644
--- a/spec/features/profiles/emails_spec.rb
+++ b/spec/features/profiles/emails_spec.rb
@@ -37,7 +37,7 @@ feature 'Profile > Emails' do
     expect(page).to have_content("my@email.com")
 
     click_link('Remove')
-    expect(page).to_not have_content("my@email.com")
+    expect(page).not_to have_content("my@email.com")
   end
 
   scenario 'User confirms email' do
@@ -51,22 +51,21 @@ feature 'Profile > Emails' do
     visit profile_emails_path
     expect(page).to have_content("#{email.email} Verified")
   end
-  
+
   scenario 'User re-sends confirmation email' do
     email = user.emails.create(email: 'my@email.com')
     visit profile_emails_path
-    
+
     expect { click_link("Resend confirmation email") }.to change { ActionMailer::Base.deliveries.size }
     expect(page).to have_content("Confirmation email sent to #{email.email}")
   end
-  
+
   scenario 'old unconfirmed emails show Send Confirmation button' do
     email = user.emails.create(email: 'my@email.com')
     email.update_attribute(:confirmation_sent_at, nil)
     visit profile_emails_path
 
-    expect(page).to_not have_content('Resend confirmation email')
+    expect(page).not_to have_content('Resend confirmation email')
     expect(page).to have_content('Send confirmation email')
   end
-
 end
diff --git a/spec/services/emails/confirm_service_spec.rb b/spec/services/emails/confirm_service_spec.rb
index 2f348589136..21efe60266e 100644
--- a/spec/services/emails/confirm_service_spec.rb
+++ b/spec/services/emails/confirm_service_spec.rb
@@ -8,7 +8,7 @@ describe Emails::ConfirmService do
 
   describe '#execute' do
     it 'sends a confirmation email again' do
-      email = user.emails.create(email: opts[:email])
+      user.emails.create(email: opts[:email])
       mail  = service.execute
       expect(mail.subject).to eq('Confirmation instructions')
     end

From 4457ae827251904c28a30c3db06e05495a42b484 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Mon, 11 Sep 2017 18:31:49 +0200
Subject: [PATCH 11/30] updated schema.rb

---
 db/schema.rb | 54 ++++++++++++++++++++++++++++------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/db/schema.rb b/db/schema.rb
index 80ef91ec95d..861e369a695 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -32,8 +32,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.text "description", null: false
     t.string "header_logo"
     t.string "logo"
-    t.datetime_with_timezone "created_at", null: false
-    t.datetime_with_timezone "updated_at", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
     t.text "description_html"
     t.integer "cached_markdown_version"
   end
@@ -101,10 +101,6 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.text "help_page_text_html"
     t.text "shared_runners_text_html"
     t.text "after_sign_up_text_html"
-    t.integer "rsa_key_restriction", default: 0, null: false
-    t.integer "dsa_key_restriction", default: 0, null: false
-    t.integer "ecdsa_key_restriction", default: 0, null: false
-    t.integer "ed25519_key_restriction", default: 0, null: false
     t.boolean "housekeeping_enabled", default: true, null: false
     t.boolean "housekeeping_bitmaps_enabled", default: true, null: false
     t.integer "housekeeping_incremental_repack_period", default: 10, null: false
@@ -129,10 +125,14 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.boolean "prometheus_metrics_enabled", default: false, null: false
     t.boolean "help_page_hide_commercial_content", default: false
     t.string "help_page_support_url"
-    t.boolean "password_authentication_enabled"
     t.integer "performance_bar_allowed_group_id"
-    t.boolean "hashed_storage_enabled", default: false, null: false
+    t.boolean "password_authentication_enabled"
     t.boolean "project_export_enabled", default: true, null: false
+    t.boolean "hashed_storage_enabled", default: false, null: false
+    t.integer "rsa_key_restriction", default: 0, null: false
+    t.integer "dsa_key_restriction", default: 0, null: false
+    t.integer "ecdsa_key_restriction", default: 0, null: false
+    t.integer "ed25519_key_restriction", default: 0, null: false
     t.boolean "auto_devops_enabled", default: false, null: false
   end
 
@@ -275,8 +275,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "encrypted_value_iv"
     t.integer "group_id", null: false
     t.boolean "protected", default: false, null: false
-    t.datetime_with_timezone "created_at", null: false
-    t.datetime_with_timezone "updated_at", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
   end
 
   add_index "ci_group_variables", ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree
@@ -288,8 +288,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "encrypted_value_salt"
     t.string "encrypted_value_iv"
     t.integer "pipeline_schedule_id", null: false
-    t.datetime_with_timezone "created_at"
-    t.datetime_with_timezone "updated_at"
+    t.datetime "created_at"
+    t.datetime "updated_at"
   end
 
   add_index "ci_pipeline_schedule_variables", ["pipeline_schedule_id", "key"], name: "index_ci_pipeline_schedule_variables_on_schedule_id_and_key", unique: true, using: :btree
@@ -341,8 +341,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.integer "auto_canceled_by_id"
     t.integer "pipeline_schedule_id"
     t.integer "source"
-    t.integer "config_source"
     t.boolean "protected"
+    t.integer "config_source"
   end
 
   add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree
@@ -515,8 +515,12 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "email", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string "confirmation_token"
+    t.datetime "confirmed_at"
+    t.datetime "confirmation_sent_at"
   end
 
+  add_index "emails", ["confirmation_token"], name: "index_emails_on_confirmation_token", unique: true, using: :btree
   add_index "emails", ["email"], name: "index_emails_on_email", unique: true, using: :btree
   add_index "emails", ["user_id"], name: "index_emails_on_user_id", using: :btree
 
@@ -538,8 +542,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.integer "project_id"
     t.integer "author_id", null: false
     t.integer "target_id"
-    t.datetime_with_timezone "created_at", null: false
-    t.datetime_with_timezone "updated_at", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
     t.integer "action", limit: 2, null: false
     t.string "target_type"
   end
@@ -577,8 +581,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
   add_index "forked_project_links", ["forked_to_project_id"], name: "index_forked_project_links_on_forked_to_project_id", unique: true, using: :btree
 
   create_table "gpg_keys", force: :cascade do |t|
-    t.datetime_with_timezone "created_at", null: false
-    t.datetime_with_timezone "updated_at", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
     t.integer "user_id"
     t.binary "primary_keyid"
     t.binary "fingerprint"
@@ -590,8 +594,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
   add_index "gpg_keys", ["user_id"], name: "index_gpg_keys_on_user_id", using: :btree
 
   create_table "gpg_signatures", force: :cascade do |t|
-    t.datetime_with_timezone "created_at", null: false
-    t.datetime_with_timezone "updated_at", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
     t.integer "project_id"
     t.integer "gpg_key_id"
     t.binary "commit_sha"
@@ -789,8 +793,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
   add_index "members", ["user_id"], name: "index_members_on_user_id", using: :btree
 
   create_table "merge_request_diff_commits", id: false, force: :cascade do |t|
-    t.datetime_with_timezone "authored_date"
-    t.datetime_with_timezone "committed_date"
+    t.datetime "authored_date"
+    t.datetime "committed_date"
     t.integer "merge_request_diff_id", null: false
     t.integer "relative_order", null: false
     t.binary "sha", null: false
@@ -1113,8 +1117,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
 
   create_table "project_auto_devops", force: :cascade do |t|
     t.integer "project_id", null: false
-    t.datetime_with_timezone "created_at", null: false
-    t.datetime_with_timezone "updated_at", null: false
+    t.datetime "created_at", null: false
+    t.datetime "updated_at", null: false
     t.boolean "enabled"
     t.string "domain"
   end
@@ -1204,7 +1208,6 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "repository_storage", default: "default", null: false
     t.boolean "request_access_enabled", default: false, null: false
     t.boolean "has_external_wiki"
-    t.string "ci_config_path"
     t.boolean "lfs_enabled"
     t.text "description_html"
     t.boolean "only_allow_merge_if_all_discussions_are_resolved"
@@ -1212,8 +1215,9 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.integer "auto_cancel_pending_pipelines", default: 1, null: false
     t.string "import_jid"
     t.integer "cached_markdown_version"
-    t.text "delete_error"
     t.datetime "last_repository_updated_at"
+    t.string "ci_config_path"
+    t.text "delete_error"
     t.integer "storage_version", limit: 2
     t.boolean "resolve_outdated_diff_discussions"
   end

From a9b31786971d83c193a1430df7c5c4550ba5aa6b Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Mon, 11 Sep 2017 19:12:57 +0200
Subject: [PATCH 12/30] Make GPG signature verification work with non-primary
 email (#36959)

---
 app/models/user.rb                            | 15 +++--
 .../repository/gpg_signed_commits/index.md    |  4 +-
 spec/factories/emails.rb                      |  2 +
 spec/models/gpg_key_spec.rb                   | 13 ++++-
 spec/models/user_spec.rb                      | 56 ++++++++++++++-----
 5 files changed, 68 insertions(+), 22 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 09c9b3250eb..cdc7c8ad84a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -817,6 +817,17 @@ class User < ActiveRecord::Base
     all_emails
   end
 
+  def all_verified_emails
+    verified_emails = []
+    verified_emails << email if confirmed? && !temp_oauth_email?
+    verified_emails.concat(emails.select {|e| e.confirmed?}.map(&:email))
+    verified_emails
+  end
+
+  def verified_email?(email)
+    all_verified_emails.include?(email)
+  end
+
   def hook_attrs
     {
       name: name,
@@ -1041,10 +1052,6 @@ class User < ActiveRecord::Base
     ensure_rss_token!
   end
 
-  def verified_email?(email)
-    self.email == email
-  end
-
   def sync_attribute?(attribute)
     return true if ldap_user? && attribute == :email
 
diff --git a/doc/user/project/repository/gpg_signed_commits/index.md b/doc/user/project/repository/gpg_signed_commits/index.md
index dfe43c6b691..d4ef6a58a11 100644
--- a/doc/user/project/repository/gpg_signed_commits/index.md
+++ b/doc/user/project/repository/gpg_signed_commits/index.md
@@ -26,7 +26,7 @@ to be uploaded to GitLab. For a signature to be verified three conditions need
 to be met:
 
 1. The public key needs to be added your GitLab account
-1. One of the emails in the GPG key matches your **primary** email
+1. One of the emails in the GPG key matches a **verified** email address you use in GitLab
 1. The committer's email matches the verified email from the gpg key
 
 ## Generating a GPG key
@@ -94,7 +94,7 @@ started:
     ```
 
 1. Enter you real name, the email address to be associated with this key (should
-   match the primary email address you use in GitLab) and an optional comment
+   match a verified email address you use in GitLab) and an optional comment
    (press <kbd>Enter</kbd> to skip):
 
     ```
diff --git a/spec/factories/emails.rb b/spec/factories/emails.rb
index 8303861bcfe..c9ab87a15aa 100644
--- a/spec/factories/emails.rb
+++ b/spec/factories/emails.rb
@@ -2,5 +2,7 @@ FactoryGirl.define do
   factory :email do
     user
     email { generate(:email_alias) }
+
+    trait(:confirmed) { confirmed_at Time.now }
   end
 end
diff --git a/spec/models/gpg_key_spec.rb b/spec/models/gpg_key_spec.rb
index fadc8bfeb61..49c608b284f 100644
--- a/spec/models/gpg_key_spec.rb
+++ b/spec/models/gpg_key_spec.rb
@@ -88,12 +88,21 @@ describe GpgKey do
 
   describe '#emails_with_verified_status' do
     it 'email is verified if the user has the matching email' do
-      user = create :user, email: 'bette.cartwright@example.com'
+      user    = create :user, email: 'bette.cartwright@example.com'
       gpg_key = create :gpg_key, key: GpgHelpers::User2.public_key, user: user
+      email_unconfirmed = create :email, user: user
+      user.reload
 
       expect(gpg_key.emails_with_verified_status).to eq(
         'bette.cartwright@example.com' => true,
-        'bette.cartwright@example.net' => false
+        'bette.cartwright@example.net' => false        
+      )
+
+      email_confirmed = create :email, :confirmed, user: user, email: 'bette.cartwright@example.net'
+      user.reload
+      expect(gpg_key.emails_with_verified_status).to eq(
+        'bette.cartwright@example.com' => true,
+        'bette.cartwright@example.net' => true
       )
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index c1affa812aa..e047027adab 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1093,6 +1093,48 @@ describe User do
     end
   end
 
+  describe '#all_emails' do
+    let(:user) { create(:user) }
+
+    it 'returns all emails' do
+      email_confirmed   = create :email, user: user, confirmed_at: Time.now
+      email_unconfirmed = create :email, user: user
+      user.reload
+      expect(user.all_emails).to eq([user.email, email_unconfirmed.email, email_confirmed.email])
+    end
+  end
+
+  describe '#all_verified_emails' do
+    let(:user) { create(:user) }
+
+    it 'returns only confirmed emails' do
+      email_confirmed   = create :email, user: user, confirmed_at: Time.now
+      email_unconfirmed = create :email, user: user
+      user.reload
+      expect(user.all_verified_emails).to eq([user.email, email_confirmed.email])
+    end
+  end
+
+  describe '#verified_email?' do
+    let(:user) { create(:user) }
+
+    it 'returns true when the email is verified/confirmed' do
+      email_confirmed   = create :email, user: user, confirmed_at: Time.now
+      email_unconfirmed = create :email, user: user
+      user.reload
+
+      expect(user.verified_email?(user.email)).to be_truthy
+      expect(user.verified_email?(email_confirmed.email)).to be_truthy
+    end
+
+    it 'returns false when the email is not verified/confirmed' do
+      email_unconfirmed = create :email, user: user
+      user.reload
+
+      expect(user.verified_email?(email_unconfirmed.email)).to be_falsy
+    end
+  end
+
   describe '#requires_ldap_check?' do
     let(:user) { described_class.new }
 
@@ -2073,20 +2115,6 @@ describe User do
     end
   end
 
-  describe '#verified_email?' do
-    it 'returns true when the email is the primary email' do
-      user = build :user, email: 'email@example.com'
-
-      expect(user.verified_email?('email@example.com')).to be true
-    end
-
-    it 'returns false when the email is not the primary email' do
-      user = build :user, email: 'email@example.com'
-
-      expect(user.verified_email?('other_email@example.com')).to be false
-    end
-  end
-
   describe '#sync_attribute?' do
     let(:user) { described_class.new }
 

From 30e3a49e23f8ce0c2a07b13addc45951d7fc6719 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Tue, 12 Sep 2017 17:36:59 +0200
Subject: [PATCH 13/30] optimized the email services

---
 app/controllers/profiles/emails_controller.rb | 14 +++++++++-----
 app/services/emails/base_service.rb           |  2 +-
 app/services/emails/confirm_service.rb        |  4 ++--
 app/services/emails/destroy_service.rb        |  4 ++--
 spec/services/emails/confirm_service_spec.rb  |  7 +++----
 spec/services/emails/destroy_service_spec.rb  |  4 ++--
 6 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index b5df4c9554b..0cd5a7db098 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -1,4 +1,7 @@
 class Profiles::EmailsController < Profiles::ApplicationController
+
+  before_action :find_email, only: [:destroy, :resend_confirmation_instructions]
+
   def index
     @primary = current_user.email
     @emails = current_user.emails.order_id_desc
@@ -14,9 +17,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
   end
 
   def destroy
-    @email = current_user.emails.find(params[:id])
-
-    Emails::DestroyService.new(current_user, email: @email.email).execute
+    Emails::DestroyService.new(current_user).execute(@email)
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url, status: 302 }
@@ -25,8 +26,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
   end
 
   def resend_confirmation_instructions
-    @email = current_user.emails.find(params[:id])
-    if @email && Emails::ConfirmService.new(current_user, email: @email.email).execute
+    if Emails::ConfirmService.new(current_user).execute(@email)
       flash[:notice] = "Confirmation email sent to #{@email.email}"
     else
       flash[:alert] = "There was a problem sending the confirmation email"
@@ -39,4 +39,8 @@ class Profiles::EmailsController < Profiles::ApplicationController
   def email_params
     params.require(:email).permit(:email)
   end
+  
+  def find_email
+    @email = current_user.emails.find(params[:id])
+  end
 end
diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index ace49889097..227c87602fc 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -1,6 +1,6 @@
 module Emails
   class BaseService
-    def initialize(user, opts)
+    def initialize(user, opts = {})
       @user = user
       @email = opts[:email]
     end
diff --git a/app/services/emails/confirm_service.rb b/app/services/emails/confirm_service.rb
index 45845ccecc5..e764f18ddd0 100644
--- a/app/services/emails/confirm_service.rb
+++ b/app/services/emails/confirm_service.rb
@@ -1,7 +1,7 @@
 module Emails
   class ConfirmService < ::Emails::BaseService
-    def execute
-      Email.find_by_email!(@email).resend_confirmation_instructions
+    def execute(email_record)
+      email_record.resend_confirmation_instructions
     end
   end
 end
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index d586b9dfe0c..d29d7e69bde 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -1,7 +1,7 @@
 module Emails
   class DestroyService < ::Emails::BaseService
-    def execute
-      Email.find_by_email!(@email).destroy && update_secondary_emails!
+    def execute(email_record)
+      email_record.destroy && update_secondary_emails!
     end
 
     private
diff --git a/spec/services/emails/confirm_service_spec.rb b/spec/services/emails/confirm_service_spec.rb
index 21efe60266e..2b2c31e2521 100644
--- a/spec/services/emails/confirm_service_spec.rb
+++ b/spec/services/emails/confirm_service_spec.rb
@@ -2,14 +2,13 @@ require 'spec_helper'
 
 describe Emails::ConfirmService do
   let(:user) { create(:user) }
-  let(:opts) { { email: 'new@email.com' } }
 
-  subject(:service) { described_class.new(user, opts) }
+  subject(:service) { described_class.new(user) }
 
   describe '#execute' do
     it 'sends a confirmation email again' do
-      user.emails.create(email: opts[:email])
-      mail  = service.execute
+      email = user.emails.create(email: 'new@email.com')
+      mail  = service.execute(email)
       expect(mail.subject).to eq('Confirmation instructions')
     end
   end
diff --git a/spec/services/emails/destroy_service_spec.rb b/spec/services/emails/destroy_service_spec.rb
index 1f4294dd905..200e33a78cf 100644
--- a/spec/services/emails/destroy_service_spec.rb
+++ b/spec/services/emails/destroy_service_spec.rb
@@ -4,11 +4,11 @@ describe Emails::DestroyService do
   let!(:user) { create(:user) }
   let!(:email) { create(:email, user: user) }
 
-  subject(:service) { described_class.new(user, email: email.email) }
+  subject(:service) { described_class.new(user) }
 
   describe '#execute' do
     it 'removes an email' do
-      expect { service.execute }.to change { user.emails.count }.by(-1)
+      expect { service.execute(email) }.to change { user.emails.count }.by(-1)
     end
   end
 end

From 09726bdf446b4673d4f92bc9132a6e34f3a67160 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Tue, 12 Sep 2017 17:39:38 +0200
Subject: [PATCH 14/30] small cleanup changes based on feedback

---
 app/models/user.rb                                 | 14 +++++++++-----
 .../mailer/confirmation_instructions.html.haml     |  5 +----
 .../mailer/confirmation_instructions.text.erb      |  6 +-----
 config/routes/user.rb                              |  2 +-
 spec/models/user_spec.rb                           |  6 +++---
 5 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index cdc7c8ad84a..5e1355662b6 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -810,6 +810,10 @@ class User < ActiveRecord::Base
     avatar_path(args) || GravatarService.new.execute(email, size, scale, username: username)
   end
 
+  def primary_email_verified?
+    confirmed? && !temp_oauth_email?
+  end
+
   def all_emails
     all_emails = []
     all_emails << email unless temp_oauth_email?
@@ -817,15 +821,15 @@ class User < ActiveRecord::Base
     all_emails
   end
 
-  def all_verified_emails
+  def verified_emails
     verified_emails = []
-    verified_emails << email if confirmed? && !temp_oauth_email?
-    verified_emails.concat(emails.select {|e| e.confirmed?}.map(&:email))
+    verified_emails << email if primary_email_verified?
+    verified_emails.concat(emails.where.not(confirmed_at: nil).pluck(:email))
     verified_emails
   end
 
-  def verified_email?(email)
-    all_verified_emails.include?(email)
+  def verified_email?(check_email)
+    (email == check_email && primary_email_verified?) || verified_emails.include?(check_email)
   end
 
   def hook_attrs
diff --git a/app/views/devise/mailer/confirmation_instructions.html.haml b/app/views/devise/mailer/confirmation_instructions.html.haml
index a0fb687e152..d2685140913 100644
--- a/app/views/devise/mailer/confirmation_instructions.html.haml
+++ b/app/views/devise/mailer/confirmation_instructions.html.haml
@@ -1,4 +1 @@
-- if @resource.is_a?(User)
-  = render partial: 'confirmation_instructions_account'
-- else
-  = render partial: 'confirmation_instructions_secondary'
+= render partial: "confirmation_instructions_#{@resource.is_a?(User) ? 'account' : 'secondary'}"
\ No newline at end of file
diff --git a/app/views/devise/mailer/confirmation_instructions.text.erb b/app/views/devise/mailer/confirmation_instructions.text.erb
index 057bb0b23f9..05fddddf415 100644
--- a/app/views/devise/mailer/confirmation_instructions.text.erb
+++ b/app/views/devise/mailer/confirmation_instructions.text.erb
@@ -1,5 +1 @@
-<% if @resource.is_a?(User) %>
-<%= render partial: 'confirmation_instructions_account' %>
-<% else %>
-<%= render partial: 'confirmation_instructions_secondary' %>
-<% end %>
+<%= render partial: "confirmation_instructions_#{@resource.is_a?(User) ? 'account' : 'secondary'}" %>
\ No newline at end of file
diff --git a/config/routes/user.rb b/config/routes/user.rb
index c4b34402068..8ee79e96836 100644
--- a/config/routes/user.rb
+++ b/config/routes/user.rb
@@ -12,7 +12,7 @@ devise_scope :user do
 end
 
 # for secondary email confirmations
-devise_for   :emails, controllers: { confirmations: :confirmations }
+devise_for :emails, controllers: { confirmations: :confirmations }
 
 scope(constraints: { username: Gitlab::PathRegex.root_namespace_route_regex }) do
   scope(path: 'users/:username',
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index e047027adab..08f58e6d069 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1100,18 +1100,18 @@ describe User do
       email_confirmed   = create :email, user: user, confirmed_at: Time.now
       email_unconfirmed = create :email, user: user
       user.reload
-      expect(user.all_emails).to eq([user.email, email_unconfirmed.email, email_confirmed.email])
+      expect(user.all_emails).to match_array([user.email, email_unconfirmed.email, email_confirmed.email])
     end
   end
 
-  describe '#all_verified_emails' do
+  describe '#verified_emails' do
     let(:user) { create(:user) }
 
     it 'returns only confirmed emails' do
       email_confirmed   = create :email, user: user, confirmed_at: Time.now
       email_unconfirmed = create :email, user: user
       user.reload
-      expect(user.all_verified_emails).to eq([user.email, email_confirmed.email])
+      expect(user.verified_emails).to match_array([user.email, email_confirmed.email])
     end
   end
 

From b2d53791614da093a32fd7040371cf2054aa9817 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 13 Sep 2017 14:47:09 +0200
Subject: [PATCH 15/30] fix calls to Emails::DestroyService

---
 app/controllers/admin/users_controller.rb | 2 +-
 lib/api/users.rb                          | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb
index cbcef70e957..abf03622b17 100644
--- a/app/controllers/admin/users_controller.rb
+++ b/app/controllers/admin/users_controller.rb
@@ -155,7 +155,7 @@ class Admin::UsersController < Admin::ApplicationController
 
   def remove_email
     email = user.emails.find(params[:email_id])
-    success = Emails::DestroyService.new(user, email: email.email).execute
+    success = Emails::DestroyService.new(user).execute(email)
 
     respond_to do |format|
       if success
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 8b44639dd7d..992556c838b 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -366,7 +366,7 @@ module API
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user).execute(email)
         end
 
         user.update_secondary_emails!
@@ -689,7 +689,7 @@ module API
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user).execute(email)
         end
 
         current_user.update_secondary_emails!

From 85d2bf778a72f82b10f4bb6ebddc3cedf8ce4e4e Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 13 Sep 2017 15:02:27 +0200
Subject: [PATCH 16/30] when a primary email is replaced and added to the
 secondary emails list, make sure it stays confirmed

---
 app/models/user.rb                          |  7 ++--
 app/services/emails/create_service.rb       |  4 +--
 spec/models/user_spec.rb                    | 38 +++++++++++++++++++++
 spec/services/emails/create_service_spec.rb |  5 +++
 4 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 5e1355662b6..88dc5565a3a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -526,8 +526,11 @@ class User < ActiveRecord::Base
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
-      Emails::DestroyService.new(self, email: email).execute
-      Emails::CreateService.new(self, email: email_was).execute
+      Emails::DestroyService.new(self).execute(primary_email_record)
+
+      # the original primary email was confirmed, and we want that to carry over.  We don't
+      # have access to the original confirmation values at this point, so just set confirmed_at
+      Emails::CreateService.new(self, email: email_was).execute(confirmed_at: confirmed_at_was)
     end
   end
 
diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index b6491ee9804..051efd2b2c0 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -1,7 +1,7 @@
 module Emails
   class CreateService < ::Emails::BaseService
-    def execute
-      @user.emails.create(email: @email)
+    def execute(options = {})
+      @user.emails.create({email: @email}.merge(options))
     end
   end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 08f58e6d069..45f0144f68e 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -379,6 +379,44 @@ describe User do
         user.update_attributes!(email: 'shawnee.ritchie@denesik.com')
       end
     end
+
+    describe '#update_emails_with_primary_email' do
+      before do
+        @user = create(:user, email: 'primary@example.com').tap do |user|
+          user.skip_reconfirmation!
+        end
+        @secondary = create :email, email: 'secondary@example.com', user: @user
+        @user.reload
+      end
+
+      it 'gets called when email updated' do
+        expect(@user).to receive(:update_emails_with_primary_email)
+        @user.update_attributes!(email: 'new_primary@example.com')
+      end
+
+      it 'does not add old primary to secondary emails' do
+        @user.update_attributes!(email: 'new_primary@example.com')
+        @user.reload
+        expect(@user.emails.count).to eq 1
+        expect(@user.emails.first.email).to eq @secondary.email
+      end
+
+      it 'adds old primary to secondary emails if secondary is becoming a primary' do
+        @user.update_attributes!(email: @secondary.email)
+        @user.reload
+        expect(@user.emails.count).to eq 1
+        expect(@user.emails.first.email).to eq 'primary@example.com'
+      end
+
+      it 'transfers old confirmation values into new secondary' do
+        org_user = @user
+        @user.update_attributes!(email: @secondary.email)
+        @user.reload
+        expect(@user.emails.count).to eq 1
+        expect(@user.emails.first.confirmed_at).not_to eq nil
+      end
+    end
+
   end
 
   describe '#update_tracked_fields!', :clean_gitlab_redis_shared_state do
diff --git a/spec/services/emails/create_service_spec.rb b/spec/services/emails/create_service_spec.rb
index 641d5538de8..d028ee27a16 100644
--- a/spec/services/emails/create_service_spec.rb
+++ b/spec/services/emails/create_service_spec.rb
@@ -12,6 +12,11 @@ describe Emails::CreateService do
       expect(Email.where(opts)).not_to be_empty
     end
 
+    it 'creates an email with additional attributes' do
+      expect { service.execute(confirmation_token: 'abc') }.to change { Email.count }.by(1)
+      expect(Email.where(opts).first.confirmation_token).to eq 'abc'
+    end
+
     it 'has the right user association' do
       service.execute
 

From a2715aaa121dbef8d209ddc87993495280a7fc2f Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 13 Sep 2017 16:13:33 +0200
Subject: [PATCH 17/30] Return schema.rb to pre-rebase state This reverts
 commit 9f186db09d7f380cfbde4c0061be77c272518547.

---
 db/schema.rb | 54 ++++++++++++++++++++++++----------------------------
 1 file changed, 25 insertions(+), 29 deletions(-)

diff --git a/db/schema.rb b/db/schema.rb
index 861e369a695..80ef91ec95d 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -32,8 +32,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.text "description", null: false
     t.string "header_logo"
     t.string "logo"
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.text "description_html"
     t.integer "cached_markdown_version"
   end
@@ -101,6 +101,10 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.text "help_page_text_html"
     t.text "shared_runners_text_html"
     t.text "after_sign_up_text_html"
+    t.integer "rsa_key_restriction", default: 0, null: false
+    t.integer "dsa_key_restriction", default: 0, null: false
+    t.integer "ecdsa_key_restriction", default: 0, null: false
+    t.integer "ed25519_key_restriction", default: 0, null: false
     t.boolean "housekeeping_enabled", default: true, null: false
     t.boolean "housekeeping_bitmaps_enabled", default: true, null: false
     t.integer "housekeeping_incremental_repack_period", default: 10, null: false
@@ -125,14 +129,10 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.boolean "prometheus_metrics_enabled", default: false, null: false
     t.boolean "help_page_hide_commercial_content", default: false
     t.string "help_page_support_url"
-    t.integer "performance_bar_allowed_group_id"
     t.boolean "password_authentication_enabled"
-    t.boolean "project_export_enabled", default: true, null: false
+    t.integer "performance_bar_allowed_group_id"
     t.boolean "hashed_storage_enabled", default: false, null: false
-    t.integer "rsa_key_restriction", default: 0, null: false
-    t.integer "dsa_key_restriction", default: 0, null: false
-    t.integer "ecdsa_key_restriction", default: 0, null: false
-    t.integer "ed25519_key_restriction", default: 0, null: false
+    t.boolean "project_export_enabled", default: true, null: false
     t.boolean "auto_devops_enabled", default: false, null: false
   end
 
@@ -275,8 +275,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "encrypted_value_iv"
     t.integer "group_id", null: false
     t.boolean "protected", default: false, null: false
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
   end
 
   add_index "ci_group_variables", ["group_id", "key"], name: "index_ci_group_variables_on_group_id_and_key", unique: true, using: :btree
@@ -288,8 +288,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "encrypted_value_salt"
     t.string "encrypted_value_iv"
     t.integer "pipeline_schedule_id", null: false
-    t.datetime "created_at"
-    t.datetime "updated_at"
+    t.datetime_with_timezone "created_at"
+    t.datetime_with_timezone "updated_at"
   end
 
   add_index "ci_pipeline_schedule_variables", ["pipeline_schedule_id", "key"], name: "index_ci_pipeline_schedule_variables_on_schedule_id_and_key", unique: true, using: :btree
@@ -341,8 +341,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.integer "auto_canceled_by_id"
     t.integer "pipeline_schedule_id"
     t.integer "source"
-    t.boolean "protected"
     t.integer "config_source"
+    t.boolean "protected"
   end
 
   add_index "ci_pipelines", ["auto_canceled_by_id"], name: "index_ci_pipelines_on_auto_canceled_by_id", using: :btree
@@ -515,12 +515,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "email", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.string "confirmation_token"
-    t.datetime "confirmed_at"
-    t.datetime "confirmation_sent_at"
   end
 
-  add_index "emails", ["confirmation_token"], name: "index_emails_on_confirmation_token", unique: true, using: :btree
   add_index "emails", ["email"], name: "index_emails_on_email", unique: true, using: :btree
   add_index "emails", ["user_id"], name: "index_emails_on_user_id", using: :btree
 
@@ -542,8 +538,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.integer "project_id"
     t.integer "author_id", null: false
     t.integer "target_id"
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.integer "action", limit: 2, null: false
     t.string "target_type"
   end
@@ -581,8 +577,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
   add_index "forked_project_links", ["forked_to_project_id"], name: "index_forked_project_links_on_forked_to_project_id", unique: true, using: :btree
 
   create_table "gpg_keys", force: :cascade do |t|
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.integer "user_id"
     t.binary "primary_keyid"
     t.binary "fingerprint"
@@ -594,8 +590,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
   add_index "gpg_keys", ["user_id"], name: "index_gpg_keys_on_user_id", using: :btree
 
   create_table "gpg_signatures", force: :cascade do |t|
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.integer "project_id"
     t.integer "gpg_key_id"
     t.binary "commit_sha"
@@ -793,8 +789,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
   add_index "members", ["user_id"], name: "index_members_on_user_id", using: :btree
 
   create_table "merge_request_diff_commits", id: false, force: :cascade do |t|
-    t.datetime "authored_date"
-    t.datetime "committed_date"
+    t.datetime_with_timezone "authored_date"
+    t.datetime_with_timezone "committed_date"
     t.integer "merge_request_diff_id", null: false
     t.integer "relative_order", null: false
     t.binary "sha", null: false
@@ -1117,8 +1113,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
 
   create_table "project_auto_devops", force: :cascade do |t|
     t.integer "project_id", null: false
-    t.datetime "created_at", null: false
-    t.datetime "updated_at", null: false
+    t.datetime_with_timezone "created_at", null: false
+    t.datetime_with_timezone "updated_at", null: false
     t.boolean "enabled"
     t.string "domain"
   end
@@ -1208,6 +1204,7 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "repository_storage", default: "default", null: false
     t.boolean "request_access_enabled", default: false, null: false
     t.boolean "has_external_wiki"
+    t.string "ci_config_path"
     t.boolean "lfs_enabled"
     t.text "description_html"
     t.boolean "only_allow_merge_if_all_discussions_are_resolved"
@@ -1215,9 +1212,8 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.integer "auto_cancel_pending_pipelines", default: 1, null: false
     t.string "import_jid"
     t.integer "cached_markdown_version"
-    t.datetime "last_repository_updated_at"
-    t.string "ci_config_path"
     t.text "delete_error"
+    t.datetime "last_repository_updated_at"
     t.integer "storage_version", limit: 2
     t.boolean "resolve_outdated_diff_discussions"
   end

From 867d59c57f9b2501690b52671c9ca1ed54fb92d4 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 13 Sep 2017 16:18:48 +0200
Subject: [PATCH 18/30] updated schema.rb with correct changes

---
 db/schema.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/db/schema.rb b/db/schema.rb
index 80ef91ec95d..17f59145472 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -515,8 +515,12 @@ ActiveRecord::Schema.define(version: 20170921115009) do
     t.string "email", null: false
     t.datetime "created_at"
     t.datetime "updated_at"
+    t.string "confirmation_token"
+    t.datetime "confirmed_at"
+    t.datetime "confirmation_sent_at"
   end
 
+  add_index "emails", ["confirmation_token"], name: "index_emails_on_confirmation_token", unique: true, using: :btree
   add_index "emails", ["email"], name: "index_emails_on_email", unique: true, using: :btree
   add_index "emails", ["user_id"], name: "index_emails_on_user_id", using: :btree
 

From a000c2a1443beb7de0b5193f89916ccce57170c5 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Thu, 14 Sep 2017 15:36:30 +0200
Subject: [PATCH 19/30] moved devise_for :emails to live under `profiles`,
 removing the need for `emails` to be a top level route

---
 config/routes/profile.rb | 3 +++
 config/routes/user.rb    | 3 ---
 lib/gitlab/path_regex.rb | 1 -
 3 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/config/routes/profile.rb b/config/routes/profile.rb
index ea4dd9b67ec..ddc852f0132 100644
--- a/config/routes/profile.rb
+++ b/config/routes/profile.rb
@@ -1,3 +1,6 @@
+# for secondary email confirmations - uses the same confirmation controller as :users
+devise_for :emails, path: 'profile/emails', controllers: { confirmations: :confirmations }
+
 resource :profile, only: [:show, :update] do
   member do
     get :audit_log
diff --git a/config/routes/user.rb b/config/routes/user.rb
index 8ee79e96836..e682dcd6663 100644
--- a/config/routes/user.rb
+++ b/config/routes/user.rb
@@ -11,9 +11,6 @@ devise_scope :user do
   get '/users/almost_there' => 'confirmations#almost_there'
 end
 
-# for secondary email confirmations
-devise_for :emails, controllers: { confirmations: :confirmations }
-
 scope(constraints: { username: Gitlab::PathRegex.root_namespace_route_regex }) do
   scope(path: 'users/:username',
         as: :user,
diff --git a/lib/gitlab/path_regex.rb b/lib/gitlab/path_regex.rb
index b119cb823b0..7c02c9c5c48 100644
--- a/lib/gitlab/path_regex.rb
+++ b/lib/gitlab/path_regex.rb
@@ -30,7 +30,6 @@ module Gitlab
       ci
       dashboard
       deploy.html
-      emails
       explore
       favicon.ico
       files

From a32f1dddf2ed65f3e61ded56256da2996d6442bd Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Thu, 14 Sep 2017 16:18:32 +0200
Subject: [PATCH 20/30] fixes for rubocop

---
 app/controllers/profiles/emails_controller.rb       |  3 +--
 app/services/emails/create_service.rb               |  2 +-
 spec/controllers/profiles/emails_controller_spec.rb |  4 ++--
 spec/models/gpg_key_spec.rb                         |  8 ++++----
 spec/models/user_spec.rb                            | 10 ++++------
 5 files changed, 12 insertions(+), 15 deletions(-)

diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 0cd5a7db098..2c85606c271 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -1,5 +1,4 @@
 class Profiles::EmailsController < Profiles::ApplicationController
-
   before_action :find_email, only: [:destroy, :resend_confirmation_instructions]
 
   def index
@@ -39,7 +38,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
   def email_params
     params.require(:email).permit(:email)
   end
-  
+
   def find_email
     @email = current_user.emails.find(params[:id])
   end
diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index 051efd2b2c0..0f6a1e24f02 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -1,7 +1,7 @@
 module Emails
   class CreateService < ::Emails::BaseService
     def execute(options = {})
-      @user.emails.create({email: @email}.merge(options))
+      @user.emails.create({ email: @email }.merge(options))
     end
   end
 end
diff --git a/spec/controllers/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb
index bdf040ba295..69b34db0c2e 100644
--- a/spec/controllers/profiles/emails_controller_spec.rb
+++ b/spec/controllers/profiles/emails_controller_spec.rb
@@ -8,7 +8,7 @@ describe Profiles::EmailsController do
   end
 
   describe '#create' do
-    let(:email_params) { {email: "add_email@example.com"} }
+    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 }
@@ -18,7 +18,7 @@ describe Profiles::EmailsController do
   end
 
   describe '#resend_confirmation_instructions' do
-    let(:email_params) { {email: "add_email@example.com"} }
+    let(:email_params) { { email: "add_email@example.com" } }
 
     it 'resends an email confirmation' do
       email = user.emails.create(email: 'add_email@example.com')
diff --git a/spec/models/gpg_key_spec.rb b/spec/models/gpg_key_spec.rb
index 49c608b284f..067afc0182b 100644
--- a/spec/models/gpg_key_spec.rb
+++ b/spec/models/gpg_key_spec.rb
@@ -88,17 +88,17 @@ describe GpgKey do
 
   describe '#emails_with_verified_status' do
     it 'email is verified if the user has the matching email' do
-      user    = create :user, email: 'bette.cartwright@example.com'
+      user = create :user, email: 'bette.cartwright@example.com'
       gpg_key = create :gpg_key, key: GpgHelpers::User2.public_key, user: user
-      email_unconfirmed = create :email, user: user
+      create :email, user: user
       user.reload
 
       expect(gpg_key.emails_with_verified_status).to eq(
         'bette.cartwright@example.com' => true,
-        'bette.cartwright@example.net' => false        
+        'bette.cartwright@example.net' => false
       )
 
-      email_confirmed = create :email, :confirmed, user: user, email: 'bette.cartwright@example.net'
+      create :email, :confirmed, user: user, email: 'bette.cartwright@example.net'
       user.reload
       expect(gpg_key.emails_with_verified_status).to eq(
         'bette.cartwright@example.com' => true,
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 45f0144f68e..a230f72449a 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -409,14 +409,12 @@ describe User do
       end
 
       it 'transfers old confirmation values into new secondary' do
-        org_user = @user
         @user.update_attributes!(email: @secondary.email)
         @user.reload
         expect(@user.emails.count).to eq 1
         expect(@user.emails.first.confirmed_at).not_to eq nil
       end
     end
-
   end
 
   describe '#update_tracked_fields!', :clean_gitlab_redis_shared_state do
@@ -1146,8 +1144,8 @@ describe User do
     let(:user) { create(:user) }
 
     it 'returns only confirmed emails' do
-      email_confirmed   = create :email, user: user, confirmed_at: Time.now
-      email_unconfirmed = create :email, user: user
+      email_confirmed = create :email, user: user, confirmed_at: Time.now
+      create :email, user: user
       user.reload
       expect(user.verified_emails).to match_array([user.email, email_confirmed.email])
     end
@@ -1157,8 +1155,8 @@ describe User do
     let(:user) { create(:user) }
 
     it 'returns true when the email is verified/confirmed' do
-      email_confirmed   = create :email, user: user, confirmed_at: Time.now
-      email_unconfirmed = create :email, user: user
+      email_confirmed = create :email, user: user, confirmed_at: Time.now
+      create :email, user: user
       user.reload
 
       expect(user.verified_email?(user.email)).to be_truthy

From 442dbf6d4b1b50f9eccaeb5a62506c55daa0fc36 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Mon, 18 Sep 2017 14:50:32 +0200
Subject: [PATCH 21/30] when user verifies a secondary email, revalidate GPG
 signatures

---
 app/models/email.rb       |  7 +++++++
 spec/models/email_spec.rb | 17 +++++++++++++++++
 2 files changed, 24 insertions(+)

diff --git a/app/models/email.rb b/app/models/email.rb
index 6254d66cad9..085663a4fef 100644
--- a/app/models/email.rb
+++ b/app/models/email.rb
@@ -7,6 +7,8 @@ class Email < ActiveRecord::Base
   validates :email, presence: true, uniqueness: true, email: true
   validate :unique_email, if: ->(email) { email.email_changed? }
 
+  after_commit :update_invalid_gpg_signatures, if: -> { previous_changes.key?('confirmed_at') }
+
   devise :confirmable
   self.reconfirmable = false  # currently email can't be changed, no need to reconfirm
 
@@ -17,4 +19,9 @@ class Email < ActiveRecord::Base
   def unique_email
     self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
   end
+
+  # once email is confirmed, update the gpg signatures
+  def update_invalid_gpg_signatures
+    user.update_invalid_gpg_signatures if confirmed?
+  end
 end
diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb
index 1d6fabe48b1..8fca9db37ca 100644
--- a/spec/models/email_spec.rb
+++ b/spec/models/email_spec.rb
@@ -11,4 +11,21 @@ describe Email do
     expect(described_class.new(email: ' inFO@exAMPLe.com ').email)
       .to eq 'info@example.com'
   end
+
+  describe '#update_invalid_gpg_signatures' do
+    let(:user) do
+      create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
+        user.skip_reconfirmation!
+      end
+    end
+    let(:user) { create(:user) }
+
+    it 'synchronizes the gpg keys when the email is updated' do
+      email = user.emails.create(email: 'new@email.com')
+      expect(user).to receive(:update_invalid_gpg_signatures)
+      email.confirm
+      # email.save
+    end
+  end
+
 end

From ed99c899a28134e8d9a1a8a8c4677a6ee65bbd2b Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Mon, 18 Sep 2017 19:00:38 +0200
Subject: [PATCH 22/30] allow a verified secondary email to be use as the
 primary without a reconfirmation

---
 app/models/user.rb                           | 20 +++++++++++++++-----
 spec/controllers/profiles_controller_spec.rb | 14 ++++++++++++++
 spec/features/profiles/emails_spec.rb        |  2 +-
 spec/models/email_spec.rb                    |  2 --
 spec/models/user_spec.rb                     | 13 +++++++++++++
 5 files changed, 43 insertions(+), 8 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 88dc5565a3a..c75107472d6 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -161,15 +161,16 @@ class User < ActiveRecord::Base
   before_validation :sanitize_attrs
   before_validation :set_notification_email, if: :email_changed?
   before_validation :set_public_email, if: :public_email_changed?
-
-  after_update :update_emails_with_primary_email, if: :email_changed?
   before_save :ensure_authentication_token, :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: :external_changed?
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
+  before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record?}
   after_save :ensure_namespace_correct
-  after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') }
-  after_initialize :set_projects_limit
   after_destroy :post_destroy_hook
+  after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') }
+  after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') }
+
+  after_initialize :set_projects_limit
 
   # User's Layout preference
   enum layout: [:fixed, :fluid]
@@ -222,6 +223,11 @@ class User < ActiveRecord::Base
     end
   end
 
+  # see if the new email is already a verified secondary email 
+  def check_for_verified_email
+    skip_reconfirmation! if emails.find_by(email: self.email).try(:confirmed?)
+  end
+
   mount_uploader :avatar, AvatarUploader
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
@@ -523,14 +529,18 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
+  # note: the use of the Emails services will cause `saves` on the user object, running
+  # through the callbacks again and can have side effects, such as the `previous_changes`
+  # hash getting cleared.  
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record
+      previous_email = previous_changes[:email][0]
       Emails::DestroyService.new(self).execute(primary_email_record)
 
       # the original primary email was confirmed, and we want that to carry over.  We don't
       # have access to the original confirmation values at this point, so just set confirmed_at
-      Emails::CreateService.new(self, email: email_was).execute(confirmed_at: confirmed_at_was)
+      Emails::CreateService.new(self, email: previous_email).execute(confirmed_at: confirmed_at)
     end
   end
 
diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb
index b52b63e05a4..3fb631a6fac 100644
--- a/spec/controllers/profiles_controller_spec.rb
+++ b/spec/controllers/profiles_controller_spec.rb
@@ -15,6 +15,20 @@ describe ProfilesController do
       expect(user.unconfirmed_email).to eq('john@gmail.com')
     end
 
+    it "allows an email update without confirmation if existing verified email" do
+      user = create(:user)
+      email = create(:email, :confirmed, user: user, email: 'john@gmail.com')
+      sign_in(user)
+
+      put :update,
+          user: { email: "john@gmail.com", name: "John" }
+
+      user.reload
+
+      expect(response.status).to eq(302)
+      expect(user.unconfirmed_email).to eq nil
+    end
+
     it "ignores an email update from a user with an external email address" do
       stub_omniauth_setting(sync_profile_from_provider: ['ldap'])
       stub_omniauth_setting(sync_profile_attributes: true)
diff --git a/spec/features/profiles/emails_spec.rb b/spec/features/profiles/emails_spec.rb
index 3085e1ee7bc..11cc8aae6f3 100644
--- a/spec/features/profiles/emails_spec.rb
+++ b/spec/features/profiles/emails_spec.rb
@@ -4,7 +4,7 @@ feature 'Profile > Emails' do
   let(:user) { create(:user) }
 
   before do
-    login_as(user)
+    sign_in(user)
   end
 
   describe 'User adds an email' do
diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb
index 8fca9db37ca..8ae5ccd89ed 100644
--- a/spec/models/email_spec.rb
+++ b/spec/models/email_spec.rb
@@ -24,8 +24,6 @@ describe Email do
       email = user.emails.create(email: 'new@email.com')
       expect(user).to receive(:update_invalid_gpg_signatures)
       email.confirm
-      # email.save
     end
   end
-
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index a230f72449a..c6df8028072 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -359,6 +359,19 @@ describe User do
         expect(external_user.projects_limit).to be 0
       end
     end
+    
+    describe '#check_for_verified_email' do
+      let(:user)      { create(:user) }
+      let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user, ) }
+      
+      it 'allows a verfied secondary email to be used as the primary without needing reconfirmation' do
+        user.update_attributes!(email: secondary.email)
+        user.reload
+        expect(user.email).to eq secondary.email
+        expect(user.unconfirmed_email).to eq nil
+        expect(user.confirmed?).to be_truthy
+      end
+    end
   end
 
   describe 'after update hook' do

From 665c7876faadc41e1c7a77bb423b951cbcb020ce Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 20 Sep 2017 13:24:10 +0200
Subject: [PATCH 23/30] added email.confirmed scope and fix email comparison

---
 app/models/email.rb       |  4 +++-
 app/models/user.rb        |  5 +++--
 spec/models/email_spec.rb | 11 +++++++++++
 spec/models/user_spec.rb  |  2 +-
 4 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/app/models/email.rb b/app/models/email.rb
index 085663a4fef..041fbbfbe44 100644
--- a/app/models/email.rb
+++ b/app/models/email.rb
@@ -7,6 +7,8 @@ class Email < ActiveRecord::Base
   validates :email, presence: true, uniqueness: true, email: true
   validate :unique_email, if: ->(email) { email.email_changed? }
 
+  scope :confirmed, -> { where.not(confirmed_at: nil) }
+
   after_commit :update_invalid_gpg_signatures, if: -> { previous_changes.key?('confirmed_at') }
 
   devise :confirmable
@@ -19,7 +21,7 @@ class Email < ActiveRecord::Base
   def unique_email
     self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
   end
-
+  
   # once email is confirmed, update the gpg signatures
   def update_invalid_gpg_signatures
     user.update_invalid_gpg_signatures if confirmed?
diff --git a/app/models/user.rb b/app/models/user.rb
index c75107472d6..3be3b205ea8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -837,12 +837,13 @@ class User < ActiveRecord::Base
   def verified_emails
     verified_emails = []
     verified_emails << email if primary_email_verified?
-    verified_emails.concat(emails.where.not(confirmed_at: nil).pluck(:email))
+    verified_emails.concat(emails.confirmed.pluck(:email))
     verified_emails
   end
 
   def verified_email?(check_email)
-    (email == check_email && primary_email_verified?) || verified_emails.include?(check_email)
+    downcased = check_email.downcase
+    (email == downcased && primary_email_verified?) || emails.confirmed.where(email: downcased).exists?
   end
 
   def hook_attrs
diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb
index 8ae5ccd89ed..8e8ef411f23 100644
--- a/spec/models/email_spec.rb
+++ b/spec/models/email_spec.rb
@@ -26,4 +26,15 @@ describe Email do
       email.confirm
     end
   end
+  
+  describe 'scopes' do
+    let(:user) { create(:user) }
+
+    it 'scopes confirmed emails' do
+      create(:email, :confirmed, user: user)
+      expect(user.emails.count).to eq 1
+      expect(user.emails.unconfirmed.count).to eq 0
+      expect(user.emails.confirmed.count).to eq 1
+    end
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index c6df8028072..83addbb809e 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1173,7 +1173,7 @@ describe User do
       user.reload
 
       expect(user.verified_email?(user.email)).to be_truthy
-      expect(user.verified_email?(email_confirmed.email)).to be_truthy
+      expect(user.verified_email?(email_confirmed.email.titlecase)).to be_truthy
     end
 
     it 'returns false when the email is not verified/confirmed' do

From de9fb43f49689b947a3e7f3c47151e7d8b1dc30c Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 20 Sep 2017 13:24:42 +0200
Subject: [PATCH 24/30] fix to make gpg_keys_spec start passing

---
 spec/features/profiles/gpg_keys_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/features/profiles/gpg_keys_spec.rb b/spec/features/profiles/gpg_keys_spec.rb
index 623e4f341c5..b0f6848bc4b 100644
--- a/spec/features/profiles/gpg_keys_spec.rb
+++ b/spec/features/profiles/gpg_keys_spec.rb
@@ -4,7 +4,7 @@ feature 'Profile > GPG Keys' do
   let(:user) { create(:user, email: GpgHelpers::User2.emails.first) }
 
   before do
-    login_as(user)
+    sign_in(user)
   end
 
   describe 'User adds a key' do

From 945667abd85b3b832159acda3bf1e3886175da46 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 20 Sep 2017 13:33:43 +0200
Subject: [PATCH 25/30] more explantion why an `after_update` was replaced with
 an `after_commit`

---
 app/models/user.rb | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 3be3b205ea8..57f619a083f 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -529,9 +529,11 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
-  # note: the use of the Emails services will cause `saves` on the user object, running
+  # Note: the use of the Emails services will cause `saves` on the user object, running
   # through the callbacks again and can have side effects, such as the `previous_changes`
-  # hash getting cleared.  
+  # hash and `_was` variables getting munged.
+  # By using an `after_commit` instead of `after_update`, we avoid the recursive callback
+  # scenario, though it then requires us to use the `previous_changes` hash
   def update_emails_with_primary_email
     primary_email_record = emails.find_by(email: email)
     if primary_email_record

From d7d335c05b9ae359b72f59c31bbe5ed059df7f52 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Wed, 20 Sep 2017 14:51:27 +0200
Subject: [PATCH 26/30] add primary email as a secondary email whenever the
 primary is changed

---
 app/models/user.rb        | 12 +++++-------
 spec/models/email_spec.rb |  4 ++--
 spec/models/user_spec.rb  | 10 +++++-----
 3 files changed, 12 insertions(+), 14 deletions(-)

diff --git a/app/models/user.rb b/app/models/user.rb
index 57f619a083f..0f6144f9250 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -535,15 +535,13 @@ class User < ActiveRecord::Base
   # By using an `after_commit` instead of `after_update`, we avoid the recursive callback
   # scenario, though it then requires us to use the `previous_changes` hash
   def update_emails_with_primary_email
+    previous_email = previous_changes[:email][0]  # grab this before the DestroyService is called
     primary_email_record = emails.find_by(email: email)
-    if primary_email_record
-      previous_email = previous_changes[:email][0]
-      Emails::DestroyService.new(self).execute(primary_email_record)
+    Emails::DestroyService.new(self).execute(primary_email_record) if primary_email_record
 
-      # the original primary email was confirmed, and we want that to carry over.  We don't
-      # have access to the original confirmation values at this point, so just set confirmed_at
-      Emails::CreateService.new(self, email: previous_email).execute(confirmed_at: confirmed_at)
-    end
+    # the original primary email was confirmed, and we want that to carry over.  We don't
+    # have access to the original confirmation values at this point, so just set confirmed_at
+    Emails::CreateService.new(self, email: previous_email).execute(confirmed_at: confirmed_at)
   end
 
   def update_invalid_gpg_signatures
diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb
index 8e8ef411f23..7c88b544d3c 100644
--- a/spec/models/email_spec.rb
+++ b/spec/models/email_spec.rb
@@ -32,8 +32,8 @@ describe Email do
 
     it 'scopes confirmed emails' do
       create(:email, :confirmed, user: user)
-      expect(user.emails.count).to eq 1
-      expect(user.emails.unconfirmed.count).to eq 0
+      create(:email, user: user)
+      expect(user.emails.count).to eq 2
       expect(user.emails.confirmed.count).to eq 1
     end
   end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 83addbb809e..839b1f5da79 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -374,7 +374,7 @@ describe User do
     end
   end
 
-  describe 'after update hook' do
+  describe 'after commit hook' do
     describe '.update_invalid_gpg_signatures' do
       let(:user) do
         create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
@@ -388,7 +388,7 @@ describe User do
       end
 
       it 'synchronizes the gpg keys when the email is updated' do
-        expect(user).to receive(:update_invalid_gpg_signatures)
+        expect(user).to receive(:update_invalid_gpg_signatures).at_most(:twice)
         user.update_attributes!(email: 'shawnee.ritchie@denesik.com')
       end
     end
@@ -407,11 +407,11 @@ describe User do
         @user.update_attributes!(email: 'new_primary@example.com')
       end
 
-      it 'does not add old primary to secondary emails' do
+      it 'adds old primary to secondary emails when secondary is a new email ' do
         @user.update_attributes!(email: 'new_primary@example.com')
         @user.reload
-        expect(@user.emails.count).to eq 1
-        expect(@user.emails.first.email).to eq @secondary.email
+        expect(@user.emails.count).to eq 2
+        expect(@user.emails.pluck(:email)).to match_array([@secondary.email, 'primary@example.com'])
       end
 
       it 'adds old primary to secondary emails if secondary is becoming a primary' do

From d97b577a1bee80bd79209d2e6d501d1cd93debcc Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sun, 24 Sep 2017 19:52:49 +0200
Subject: [PATCH 27/30] must now set the devise default scope (since we now
 have an :email scope) and rubocop fixes

---
 app/models/email.rb                          | 2 +-
 app/models/user.rb                           | 2 +-
 config/initializers/devise.rb                | 2 +-
 spec/controllers/profiles_controller_spec.rb | 2 +-
 spec/models/email_spec.rb                    | 2 +-
 spec/models/user_spec.rb                     | 6 +++---
 6 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/models/email.rb b/app/models/email.rb
index 041fbbfbe44..384f38f2db7 100644
--- a/app/models/email.rb
+++ b/app/models/email.rb
@@ -21,7 +21,7 @@ class Email < ActiveRecord::Base
   def unique_email
     self.errors.add(:email, 'has already been taken') if User.exists?(email: self.email)
   end
-  
+
   # once email is confirmed, update the gpg signatures
   def update_invalid_gpg_signatures
     user.update_invalid_gpg_signatures if confirmed?
diff --git a/app/models/user.rb b/app/models/user.rb
index 0f6144f9250..c6b2baea8e8 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -223,7 +223,7 @@ class User < ActiveRecord::Base
     end
   end
 
-  # see if the new email is already a verified secondary email 
+  # see if the new email is already a verified secondary email
   def check_for_verified_email
     skip_reconfirmation! if emails.find_by(email: self.email).try(:confirmed?)
   end
diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb
index 3aed2136f1b..2ca02c16497 100644
--- a/config/initializers/devise.rb
+++ b/config/initializers/devise.rb
@@ -175,7 +175,7 @@ Devise.setup do |config|
 
   # Configure the default scope given to Warden. By default it's the first
   # devise role declared in your routes (usually :user).
-  # config.default_scope = :user
+  config.default_scope = :user  # now have an :email scope as well, so set the default
 
   # Configure sign_out behavior.
   # Sign_out action can be scoped (i.e. /users/sign_out affects only :user scope).
diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb
index 3fb631a6fac..ce5040ff02b 100644
--- a/spec/controllers/profiles_controller_spec.rb
+++ b/spec/controllers/profiles_controller_spec.rb
@@ -17,7 +17,7 @@ describe ProfilesController do
 
     it "allows an email update without confirmation if existing verified email" do
       user = create(:user)
-      email = create(:email, :confirmed, user: user, email: 'john@gmail.com')
+      create(:email, :confirmed, user: user, email: 'john@gmail.com')
       sign_in(user)
 
       put :update,
diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb
index 7c88b544d3c..3fd3fe03be5 100644
--- a/spec/models/email_spec.rb
+++ b/spec/models/email_spec.rb
@@ -26,7 +26,7 @@ describe Email do
       email.confirm
     end
   end
-  
+
   describe 'scopes' do
     let(:user) { create(:user) }
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 839b1f5da79..9ed155b0fa0 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -359,11 +359,11 @@ describe User do
         expect(external_user.projects_limit).to be 0
       end
     end
-    
+
     describe '#check_for_verified_email' do
       let(:user)      { create(:user) }
-      let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user, ) }
-      
+      let(:secondary) { create(:email, :confirmed, email: 'secondary@example.com', user: user) }
+
       it 'allows a verfied secondary email to be used as the primary without needing reconfirmation' do
         user.update_attributes!(email: secondary.email)
         user.reload

From 39fbac868197442053d058023206f84f63473551 Mon Sep 17 00:00:00 2001
From: Brett Walker <brett@digitalmoksha.com>
Date: Sun, 24 Sep 2017 22:21:00 +0200
Subject: [PATCH 28/30] fix whitespace

---
 app/views/devise/mailer/confirmation_instructions.html.haml | 2 +-
 app/views/profiles/emails/index.html.haml                   | 3 +--
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/app/views/devise/mailer/confirmation_instructions.html.haml b/app/views/devise/mailer/confirmation_instructions.html.haml
index d2685140913..50ee7b53d8f 100644
--- a/app/views/devise/mailer/confirmation_instructions.html.haml
+++ b/app/views/devise/mailer/confirmation_instructions.html.haml
@@ -1 +1 @@
-= render partial: "confirmation_instructions_#{@resource.is_a?(User) ? 'account' : 'secondary'}"
\ No newline at end of file
+= render partial: "confirmation_instructions_#{@resource.is_a?(User) ? 'account' : 'secondary'}"
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index 1b02603543d..b0dc8c526e1 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -50,8 +50,7 @@
             - unless email.confirmed?
               - confirm_title = "#{email.confirmation_sent_at ? 'Resend' : 'Send'} confirmation email"
               = link_to confirm_title, resend_confirmation_instructions_profile_email_path(email), method: :put, class: 'btn btn-sm btn-warning prepend-left-10'
-                
+
             = link_to profile_email_path(email), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-sm btn-danger prepend-left-10' do
               %span.sr-only Remove
               = icon('trash')
-            

From ed3c25e4294bfc75f7167ed63e30561e39ac3a97 Mon Sep 17 00:00:00 2001
From: Alexandra <brett@digitalmoksha.com>
Date: Sun, 1 Oct 2017 17:07:26 +0200
Subject: [PATCH 29/30] spacing and small optimisations

---
 app/controllers/confirmations_controller.rb        |  2 +-
 app/controllers/profiles/emails_controller.rb      |  3 ++-
 app/models/user.rb                                 | 14 +++++++-------
 app/services/emails/base_service.rb                |  5 ++---
 app/services/emails/confirm_service.rb             |  4 ++--
 app/services/emails/create_service.rb              |  4 ++--
 app/services/emails/destroy_service.rb             |  4 ++--
 app/views/profiles/emails/index.html.haml          |  6 +++---
 .../controllers/profiles/emails_controller_spec.rb |  7 ++++---
 spec/models/email_spec.rb                          |  3 +++
 10 files changed, 28 insertions(+), 24 deletions(-)

diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index 51c26b9c17e..62ee9470704 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -10,7 +10,7 @@ class ConfirmationsController < Devise::ConfirmationsController
     users_almost_there_path
   end
 
-  def after_confirmation_path_for(resource_name, resource)
+  def after_confirmation_path_for(_resource_name, resource)
     # incoming resource can either be a :user or an :email
     if signed_in?(:user)
       after_sign_in_path_for(resource)
diff --git a/app/controllers/profiles/emails_controller.rb b/app/controllers/profiles/emails_controller.rb
index 2c85606c271..3ffe5729c2c 100644
--- a/app/controllers/profiles/emails_controller.rb
+++ b/app/controllers/profiles/emails_controller.rb
@@ -2,7 +2,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
   before_action :find_email, only: [:destroy, :resend_confirmation_instructions]
 
   def index
-    @primary = current_user.email
+    @primary_email = current_user.email
     @emails = current_user.emails.order_id_desc
   end
 
@@ -30,6 +30,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
     else
       flash[:alert] = "There was a problem sending the confirmation email"
     end
+
     redirect_to profile_emails_url
   end
 
diff --git a/app/models/user.rb b/app/models/user.rb
index c6b2baea8e8..7a9561a04ff 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -164,7 +164,7 @@ class User < ActiveRecord::Base
   before_save :ensure_authentication_token, :ensure_incoming_email_token
   before_save :ensure_user_rights_and_limits, if: :external_changed?
   before_save :skip_reconfirmation!, if: ->(user) { user.email_changed? && user.read_only_attribute?(:email) }
-  before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record?}
+  before_save :check_for_verified_email, if: ->(user) { user.email_changed? && !user.new_record? }
   after_save :ensure_namespace_correct
   after_destroy :post_destroy_hook
   after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') }
@@ -223,11 +223,6 @@ class User < ActiveRecord::Base
     end
   end
 
-  # see if the new email is already a verified secondary email
-  def check_for_verified_email
-    skip_reconfirmation! if emails.find_by(email: self.email).try(:confirmed?)
-  end
-
   mount_uploader :avatar, AvatarUploader
   has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
 
@@ -529,6 +524,11 @@ class User < ActiveRecord::Base
     errors.add(:public_email, "is not an email you own") unless all_emails.include?(public_email)
   end
 
+  # see if the new email is already a verified secondary email
+  def check_for_verified_email
+    skip_reconfirmation! if emails.confirmed.where(email: self.email).any?
+  end
+
   # Note: the use of the Emails services will cause `saves` on the user object, running
   # through the callbacks again and can have side effects, such as the `previous_changes`
   # hash and `_was` variables getting munged.
@@ -843,7 +843,7 @@ class User < ActiveRecord::Base
 
   def verified_email?(check_email)
     downcased = check_email.downcase
-    (email == downcased && primary_email_verified?) || emails.confirmed.where(email: downcased).exists?
+    email == downcased ? primary_email_verified? : emails.confirmed.where(email: downcased).exists?
   end
 
   def hook_attrs
diff --git a/app/services/emails/base_service.rb b/app/services/emails/base_service.rb
index 227c87602fc..caddbf505fe 100644
--- a/app/services/emails/base_service.rb
+++ b/app/services/emails/base_service.rb
@@ -1,8 +1,7 @@
 module Emails
   class BaseService
-    def initialize(user, opts = {})
-      @user = user
-      @email = opts[:email]
+    def initialize(user, params = {})
+      @user, @params = user, params.dup
     end
   end
 end
diff --git a/app/services/emails/confirm_service.rb b/app/services/emails/confirm_service.rb
index e764f18ddd0..b5301bf2b82 100644
--- a/app/services/emails/confirm_service.rb
+++ b/app/services/emails/confirm_service.rb
@@ -1,7 +1,7 @@
 module Emails
   class ConfirmService < ::Emails::BaseService
-    def execute(email_record)
-      email_record.resend_confirmation_instructions
+    def execute(email)
+      email.resend_confirmation_instructions
     end
   end
 end
diff --git a/app/services/emails/create_service.rb b/app/services/emails/create_service.rb
index 0f6a1e24f02..94a841af7c3 100644
--- a/app/services/emails/create_service.rb
+++ b/app/services/emails/create_service.rb
@@ -1,7 +1,7 @@
 module Emails
   class CreateService < ::Emails::BaseService
-    def execute(options = {})
-      @user.emails.create({ email: @email }.merge(options))
+    def execute(extra_params = {})
+      @user.emails.create(@params.merge(extra_params))
     end
   end
 end
diff --git a/app/services/emails/destroy_service.rb b/app/services/emails/destroy_service.rb
index d29d7e69bde..aef863bd9d1 100644
--- a/app/services/emails/destroy_service.rb
+++ b/app/services/emails/destroy_service.rb
@@ -1,7 +1,7 @@
 module Emails
   class DestroyService < ::Emails::BaseService
-    def execute(email_record)
-      email_record.destroy && update_secondary_emails!
+    def execute(email)
+      email.destroy && update_secondary_emails!
     end
 
     private
diff --git a/app/views/profiles/emails/index.html.haml b/app/views/profiles/emails/index.html.haml
index b0dc8c526e1..df1df4f5d72 100644
--- a/app/views/profiles/emails/index.html.haml
+++ b/app/views/profiles/emails/index.html.haml
@@ -32,12 +32,12 @@
           All email addresses will be used to identify your commits.
     %ul.well-list
       %li
-        = render partial: 'shared/email_with_badge', locals: { email: @primary, verified: current_user.confirmed? }
+        = render partial: 'shared/email_with_badge', locals: { email: @primary_email, verified: current_user.confirmed? }
         %span.pull-right
           %span.label.label-success Primary email
-          - if @primary === current_user.public_email
+          - if @primary_email === current_user.public_email
             %span.label.label-info Public email
-          - if @primary === current_user.notification_email
+          - if @primary_email === current_user.notification_email
             %span.label.label-info Notification email
       - @emails.each do |email|
         %li
diff --git a/spec/controllers/profiles/emails_controller_spec.rb b/spec/controllers/profiles/emails_controller_spec.rb
index 69b34db0c2e..ecf14aad54f 100644
--- a/spec/controllers/profiles/emails_controller_spec.rb
+++ b/spec/controllers/profiles/emails_controller_spec.rb
@@ -11,7 +11,7 @@ describe Profiles::EmailsController 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 { 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
@@ -22,13 +22,14 @@ describe Profiles::EmailsController do
 
     it 'resends an email confirmation' do
       email = user.emails.create(email: 'add_email@example.com')
-      expect {put(:resend_confirmation_instructions, { id: email })}.to change { ActionMailer::Base.deliveries.size }
+
+      expect { put(:resend_confirmation_instructions, { id: email }) }.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
 
     it 'unable to resend an email confirmation' do
-      expect {put(:resend_confirmation_instructions, { id: 1 })}.not_to change { ActionMailer::Base.deliveries.size }
+      expect { put(:resend_confirmation_instructions, { id: 1 }) }.not_to change { ActionMailer::Base.deliveries.size }
     end
   end
 end
diff --git a/spec/models/email_spec.rb b/spec/models/email_spec.rb
index 3fd3fe03be5..b32dd31ae6d 100644
--- a/spec/models/email_spec.rb
+++ b/spec/models/email_spec.rb
@@ -22,7 +22,9 @@ describe Email do
 
     it 'synchronizes the gpg keys when the email is updated' do
       email = user.emails.create(email: 'new@email.com')
+
       expect(user).to receive(:update_invalid_gpg_signatures)
+
       email.confirm
     end
   end
@@ -33,6 +35,7 @@ describe Email do
     it 'scopes confirmed emails' do
       create(:email, :confirmed, user: user)
       create(:email, user: user)
+
       expect(user.emails.count).to eq 2
       expect(user.emails.confirmed.count).to eq 1
     end

From 782c017ca0c85311373d53d49d3b9af642b6a85d Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@selenight.nl>
Date: Thu, 5 Oct 2017 12:58:21 +0200
Subject: [PATCH 30/30] Make sure spec expectations are in their own paragraphs

---
 spec/models/user_spec.rb | 52 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index e0ac8483189..9f517e4af72 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -405,12 +405,14 @@ describe User do
 
       it 'gets called when email updated' do
         expect(@user).to receive(:update_emails_with_primary_email)
+
         @user.update_attributes!(email: 'new_primary@example.com')
       end
 
       it 'adds old primary to secondary emails when secondary is a new email ' do
         @user.update_attributes!(email: 'new_primary@example.com')
         @user.reload
+
         expect(@user.emails.count).to eq 2
         expect(@user.emails.pluck(:email)).to match_array([@secondary.email, 'primary@example.com'])
       end
@@ -418,6 +420,7 @@ describe User do
       it 'adds old primary to secondary emails if secondary is becoming a primary' do
         @user.update_attributes!(email: @secondary.email)
         @user.reload
+
         expect(@user.emails.count).to eq 1
         expect(@user.emails.first.email).to eq 'primary@example.com'
       end
@@ -425,6 +428,7 @@ describe User do
       it 'transfers old confirmation values into new secondary' do
         @user.update_attributes!(email: @secondary.email)
         @user.reload
+
         expect(@user.emails.count).to eq 1
         expect(@user.emails.first.confirmed_at).not_to eq nil
       end
@@ -516,6 +520,7 @@ describe User do
   describe '#generate_password' do
     it "does not generate password by default" do
       user = create(:user, password: 'abcdefghe')
+
       expect(user.password).to eq('abcdefghe')
     end
   end
@@ -523,6 +528,7 @@ describe User do
   describe 'authentication token' do
     it "has authentication token" do
       user = create(:user)
+
       expect(user.authentication_token).not_to be_blank
     end
   end
@@ -530,6 +536,7 @@ describe User do
   describe 'ensure incoming email token' do
     it 'has incoming email token' do
       user = create(:user)
+
       expect(user.incoming_email_token).not_to be_blank
     end
   end
@@ -572,6 +579,7 @@ describe User do
     it 'ensures an rss token on read' do
       user = create(:user, rss_token: nil)
       rss_token = user.rss_token
+
       expect(rss_token).not_to be_blank
       expect(user.reload.rss_token).to eq rss_token
     end
@@ -682,6 +690,7 @@ describe User do
 
     it "blocks user" do
       user.block
+
       expect(user.blocked?).to be_truthy
     end
   end
@@ -1015,6 +1024,7 @@ describe User do
 
     it 'is case-insensitive' do
       user = create(:user, username: 'JohnDoe')
+
       expect(described_class.find_by_username('JOHNDOE')).to eq user
     end
   end
@@ -1027,6 +1037,7 @@ describe User do
 
     it 'is case-insensitive' do
       user = create(:user, username: 'JohnDoe')
+
       expect(described_class.find_by_username!('JOHNDOE')).to eq user
     end
   end
@@ -1116,11 +1127,13 @@ describe User do
 
     it 'is true if avatar is image' do
       user.update_attribute(:avatar, 'uploads/avatar.png')
+
       expect(user.avatar_type).to be_truthy
     end
 
     it 'is false if avatar is html page' do
       user.update_attribute(:avatar, 'uploads/avatar.html')
+
       expect(user.avatar_type).to eq(['only images allowed'])
     end
   end
@@ -1150,6 +1163,7 @@ describe User do
       email_confirmed   = create :email, user: user, confirmed_at: Time.now
       email_unconfirmed = create :email, user: user
       user.reload
+
       expect(user.all_emails).to match_array([user.email, email_unconfirmed.email, email_confirmed.email])
     end
   end
@@ -1161,6 +1175,7 @@ describe User do
       email_confirmed = create :email, user: user, confirmed_at: Time.now
       create :email, user: user
       user.reload
+
       expect(user.verified_emails).to match_array([user.email, email_confirmed.email])
     end
   end
@@ -1192,6 +1207,7 @@ describe User do
       # Create a condition which would otherwise cause 'true' to be returned
       allow(user).to receive(:ldap_user?).and_return(true)
       user.last_credential_check_at = nil
+
       expect(user.requires_ldap_check?).to be_falsey
     end
 
@@ -1202,6 +1218,7 @@ describe User do
 
       it 'is false for non-LDAP users' do
         allow(user).to receive(:ldap_user?).and_return(false)
+
         expect(user.requires_ldap_check?).to be_falsey
       end
 
@@ -1212,11 +1229,13 @@ describe User do
 
         it 'is true when the user has never had an LDAP check before' do
           user.last_credential_check_at = nil
+
           expect(user.requires_ldap_check?).to be_truthy
         end
 
         it 'is true when the last LDAP check happened over 1 hour ago' do
           user.last_credential_check_at = 2.hours.ago
+
           expect(user.requires_ldap_check?).to be_truthy
         end
       end
@@ -1227,16 +1246,19 @@ describe User do
     describe '#ldap_user?' do
       it 'is true if provider name starts with ldap' do
         user = create(:omniauth_user, provider: 'ldapmain')
+
         expect(user.ldap_user?).to be_truthy
       end
 
       it 'is false for other providers' do
         user = create(:omniauth_user, provider: 'other-provider')
+
         expect(user.ldap_user?).to be_falsey
       end
 
       it 'is false if no extern_uid is provided' do
         user = create(:omniauth_user, extern_uid: nil)
+
         expect(user.ldap_user?).to be_falsey
       end
     end
@@ -1244,6 +1266,7 @@ describe User do
     describe '#ldap_identity' do
       it 'returns ldap identity' do
         user = create :omniauth_user
+
         expect(user.ldap_identity.provider).not_to be_empty
       end
     end
@@ -1253,6 +1276,7 @@ describe User do
 
       it 'blocks user flaging the action caming from ldap' do
         user.ldap_block
+
         expect(user.blocked?).to be_truthy
         expect(user.ldap_blocked?).to be_truthy
       end
@@ -1325,18 +1349,22 @@ describe User do
       expect(user.starred?(project2)).to be_falsey
 
       star1 = UsersStarProject.create!(project: project1, user: user)
+
       expect(user.starred?(project1)).to be_truthy
       expect(user.starred?(project2)).to be_falsey
 
       star2 = UsersStarProject.create!(project: project2, user: user)
+
       expect(user.starred?(project1)).to be_truthy
       expect(user.starred?(project2)).to be_truthy
 
       star1.destroy
+
       expect(user.starred?(project1)).to be_falsey
       expect(user.starred?(project2)).to be_truthy
 
       star2.destroy
+
       expect(user.starred?(project1)).to be_falsey
       expect(user.starred?(project2)).to be_falsey
     end
@@ -1348,9 +1376,13 @@ describe User do
       project = create(:project, :public)
 
       expect(user.starred?(project)).to be_falsey
+
       user.toggle_star(project)
+
       expect(user.starred?(project)).to be_truthy
+
       user.toggle_star(project)
+
       expect(user.starred?(project)).to be_falsey
     end
   end
@@ -1529,9 +1561,11 @@ describe User do
       user    = create(:user)
 
       member = group.add_developer(user)
+
       expect(user.authorized_projects).to include(project)
 
       member.destroy
+
       expect(user.authorized_projects).not_to include(project)
     end
 
@@ -1552,9 +1586,11 @@ describe User do
       project = create(:project, :private, namespace: user1.namespace)
 
       project.team << [user2, Gitlab::Access::DEVELOPER]
+
       expect(user2.authorized_projects).to include(project)
 
       project.destroy
+
       expect(user2.authorized_projects).not_to include(project)
     end
 
@@ -1564,9 +1600,11 @@ describe User do
       user    = create(:user)
 
       group.add_developer(user)
+
       expect(user.authorized_projects).to include(project)
 
       group.destroy
+
       expect(user.authorized_projects).not_to include(project)
     end
   end
@@ -2110,7 +2148,9 @@ describe User do
 
       it 'creates the namespace' do
         expect(user.namespace).to be_nil
+
         user.save!
+
         expect(user.namespace).not_to be_nil
       end
     end
@@ -2131,11 +2171,13 @@ describe User do
 
           it 'updates the namespace name' do
             user.update_attributes!(username: new_username)
+
             expect(user.namespace.name).to eq(new_username)
           end
 
           it 'updates the namespace path' do
             user.update_attributes!(username: new_username)
+
             expect(user.namespace.path).to eq(new_username)
           end
 
@@ -2149,6 +2191,7 @@ describe User do
 
             it 'adds the namespace errors to the user' do
               user.update_attributes(username: new_username)
+
               expect(user.errors.full_messages.first).to eq('Namespace name has already been taken')
             end
           end
@@ -2171,36 +2214,43 @@ describe User do
     context 'oauth user' do
       it 'returns true if name can be synced' do
         stub_omniauth_setting(sync_profile_attributes: %w(name location))
+
         expect(user.sync_attribute?(:name)).to be_truthy
       end
 
       it 'returns true if email can be synced' do
         stub_omniauth_setting(sync_profile_attributes: %w(name email))
+
         expect(user.sync_attribute?(:email)).to be_truthy
       end
 
       it 'returns true if location can be synced' do
         stub_omniauth_setting(sync_profile_attributes: %w(location email))
+
         expect(user.sync_attribute?(:email)).to be_truthy
       end
 
       it 'returns false if name can not be synced' do
         stub_omniauth_setting(sync_profile_attributes: %w(location email))
+
         expect(user.sync_attribute?(:name)).to be_falsey
       end
 
       it 'returns false if email can not be synced' do
         stub_omniauth_setting(sync_profile_attributes: %w(location email))
+
         expect(user.sync_attribute?(:name)).to be_falsey
       end
 
       it 'returns false if location can not be synced' do
         stub_omniauth_setting(sync_profile_attributes: %w(location email))
+
         expect(user.sync_attribute?(:name)).to be_falsey
       end
 
       it 'returns true for all syncable attributes if all syncable attributes can be synced' do
         stub_omniauth_setting(sync_profile_attributes: true)
+
         expect(user.sync_attribute?(:name)).to be_truthy
         expect(user.sync_attribute?(:email)).to be_truthy
         expect(user.sync_attribute?(:location)).to be_truthy
@@ -2216,6 +2266,7 @@ describe User do
     context 'ldap user' do
       it 'returns true for email if ldap user' do
         allow(user).to receive(:ldap_user?).and_return(true)
+
         expect(user.sync_attribute?(:name)).to be_falsey
         expect(user.sync_attribute?(:email)).to be_truthy
         expect(user.sync_attribute?(:location)).to be_falsey
@@ -2224,6 +2275,7 @@ describe User do
       it 'returns true for email and location if ldap user and location declared as syncable' do
         allow(user).to receive(:ldap_user?).and_return(true)
         stub_omniauth_setting(sync_profile_attributes: %w(location))
+
         expect(user.sync_attribute?(:name)).to be_falsey
         expect(user.sync_attribute?(:email)).to be_truthy
         expect(user.sync_attribute?(:location)).to be_truthy