Merge branch 'digitalmoksha/gitlab-ce-feature/verify_secondary_emails' into 'master'

Send a confirmation email when the user adds a secondary email address

Closes #37385, #28621, and #36959

See merge request gitlab-org/gitlab-ce!14703

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

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(current_user, user: user, email: email.email).execute
+    success = Emails::DestroyService.new(current_user, user: user).execute(email)
 
     respond_to do |format|
       if success

app/controllers/confirmations_controller.rb

@@ -10,13 +10,14 @@ class ConfirmationsController < Devise::ConfirmationsController
     users_almost_there_path
   end
 
-  def after_confirmation_path_for(resource_name, resource)
-    if signed_in?(resource_name)
+  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(resource)
     else
       Gitlab::AppLogger.info("Email Confirmed: username=#{resource.username} email=#{resource.email} ip=#{request.remote_ip}")
       flash[:notice] += " Please sign in."
-      new_session_path(resource_name)
+      new_session_path(:user)
     end
   end
 

app/controllers/profiles/emails_controller.rb

@@ -1,15 +1,14 @@
 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
 
   def create
     @email = Emails::CreateService.new(current_user, email_params.merge(user: current_user)).execute
-
-    if @email.errors.blank?
-      NotificationService.new.new_email(@email)
-    else
+    unless @email.errors.blank?
       flash[:alert] = @email.errors.full_messages.first
     end
 
@@ -17,9 +16,7 @@ class Profiles::EmailsController < Profiles::ApplicationController
   end
 
   def destroy
-    @email = current_user.emails.find(params[:id])
-
-    Emails::DestroyService.new(current_user, user: current_user, email: @email.email).execute
+    Emails::DestroyService.new(current_user, user: current_user).execute(@email)
 
     respond_to do |format|
       format.html { redirect_to profile_emails_url, status: 302 }
@@ -27,9 +24,23 @@ class Profiles::EmailsController < Profiles::ApplicationController
     end
   end
 
+  def resend_confirmation_instructions
+    if Emails::ConfirmService.new(current_user, user: current_user).execute(@email)
+      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
     params.require(:email).permit(:email)
   end
+
+  def find_email
+    @email = current_user.emails.find(params[:id])
+  end
 end

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)
 

app/models/email.rb

@@ -7,6 +7,13 @@ 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
+  self.reconfirmable = false  # currently email can't be changed, no need to reconfirm
+
   def email=(value)
     write_attribute(:email, value.downcase.strip)
   end
@@ -14,4 +21,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

app/models/user.rb

@@ -163,15 +163,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]
@@ -525,12 +526,24 @@ 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.
+  # 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
-      Emails::DestroyService.new(self, user: self, email: email).execute
-      Emails::CreateService.new(self, user: self, email: email_was).execute
-    end
+    Emails::DestroyService.new(self, user: 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, user: self, email: previous_email).execute(confirmed_at: confirmed_at)
   end
 
   def update_invalid_gpg_signatures
@@ -816,6 +829,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?
@@ -823,6 +840,18 @@ class User < ActiveRecord::Base
     all_emails
   end
 
+  def verified_emails
+    verified_emails = []
+    verified_emails << email if primary_email_verified?
+    verified_emails.concat(emails.confirmed.pluck(:email))
+    verified_emails
+  end
+
+  def verified_email?(check_email)
+    downcased = check_email.downcase
+    email == downcased ? primary_email_verified? : emails.confirmed.where(email: downcased).exists?
+  end
+
   def hook_attrs
     {
       name: name,
@@ -1047,10 +1076,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
 

app/services/emails/base_service.rb

@@ -1,9 +1,8 @@
 module Emails
   class BaseService
-    def initialize(current_user, opts)
-      @current_user = current_user
-      @user = opts.delete(:user)
-      @email = opts[:email]
+    def initialize(current_user, params = {})
+      @current_user, @params = current_user, params.dup
+      @user = params.delete(:user)
     end
   end
 end

app/services/emails/confirm_service.rb

@@ -0,0 +1,7 @@
+module Emails
+  class ConfirmService < ::Emails::BaseService
+    def execute(email)
+      email.resend_confirmation_instructions
+    end
+  end
+end

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(extra_params = {})
+      @user.emails.create(@params.merge(extra_params))
     end
   end
 end

app/services/emails/destroy_service.rb

@@ -1,7 +1,7 @@
 module Emails
   class DestroyService < ::Emails::BaseService
-    def execute
-      update_secondary_emails! if Email.find_by_email!(@email).destroy
+    def execute(email)
+      email.destroy && update_secondary_emails!
     end
 
     private

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

app/views/devise/mailer/_confirmation_instructions_account.html.haml

@@ -0,0 +1,16 @@
+- confirmation_link = 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_link
+- 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_link

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

app/views/devise/mailer/_confirmation_instructions_secondary.html.haml

@@ -0,0 +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

app/views/devise/mailer/_confirmation_instructions_secondary.text.erb

@@ -0,0 +1,7 @@
+<%= @resource.user.name %>, you've added an additional email!
+
+Use the link below to confirm your email address (<%= @resource.email %>)
+
+<%= confirmation_url(@resource, confirmation_token: @token) %>
+
+If this email was added in error, you can remove it here: <%= profile_emails_url %>

app/views/devise/mailer/confirmation_instructions.html.haml

@@ -1,16 +1 @@
-- confirmation_link = 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 confirmation_link, confirmation_link
-- 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 confirmation_link, confirmation_link
+= render partial: "confirmation_instructions_#{@resource.is_a?(User) ? 'account' : 'secondary'}"

app/views/devise/mailer/confirmation_instructions.text.erb

@@ -1,9 +1 @@
-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) %>
+<%= render partial: "confirmation_instructions_#{@resource.is_a?(User) ? 'account' : 'secondary'}" %>

app/views/notify/new_email_email.html.haml

@@ -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

app/views/notify/new_email_email.text.erb

@@ -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 %>

app/views/profiles/emails/index.html.haml

@@ -32,19 +32,25 @@
           All email addresses will be used to identify your commits.
     %ul.well-list
       %li
-        = @primary
+        = 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
-          = 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
-            = link_to 'Remove', profile_email_path(email), data: { confirm: 'Are you sure?'}, method: :delete, class: 'btn btn-sm btn-warning prepend-left-10'
+            - 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')

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

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

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

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

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
@@ -28,7 +31,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

db/migrate/20170904092148_add_email_confirmation.rb

@@ -0,0 +1,33 @@
+# 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_with_timezone
+    add_column :emails, :confirmation_sent_at, :datetime_with_timezone
+  end
+end

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_concurrent_index :emails, :confirmation_token if index_exists?(:emails, :confirmation_token)
+  end
+end

db/schema.rb

@@ -514,8 +514,12 @@ ActiveRecord::Schema.define(version: 20171004121444) 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
 

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):
 
     ```

lib/api/users.rb

@@ -331,7 +331,6 @@ module API
         email = Emails::CreateService.new(current_user, declared_params(include_missing: false).merge(user: user)).execute
 
         if email.errors.blank?
-          NotificationService.new.new_email(email)
           present email, with: Entities::Email
         else
           render_validation_error!(email)
@@ -369,10 +368,8 @@ module API
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, user: user, email: email.email).execute
+          Emails::DestroyService.new(current_user, user: user).execute(email)
         end
-
-        user.update_secondary_emails!
       end
 
       desc 'Delete a user. Available only for admins.' do
@@ -677,7 +674,6 @@ module API
         email = Emails::CreateService.new(current_user, declared_params.merge(user: current_user)).execute
 
         if email.errors.blank?
-          NotificationService.new.new_email(email)
           present email, with: Entities::Email
         else
           render_validation_error!(email)
@@ -693,10 +689,8 @@ module API
         not_found!('Email') unless email
 
         destroy_conditionally!(email) do |email|
-          Emails::DestroyService.new(current_user, user: current_user, email: email.email).execute
+          Emails::DestroyService.new(current_user, user: current_user).execute(email)
         end
-
-        current_user.update_secondary_emails!
       end
 
       desc 'Get a list of user activities'

spec/controllers/profiles/emails_controller_spec.rb

@@ -0,0 +1,35 @@
+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
+
+  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 }) }.not_to change { ActionMailer::Base.deliveries.size }
+    end
+  end
+end

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

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

spec/features/profiles/emails_spec.rb

@@ -0,0 +1,71 @@
+require 'rails_helper'
+
+feature 'Profile > Emails' do
+  let(:user) { create(:user) }
+
+  before do
+    sign_in(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("#{user.email} 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).not_to 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("#{email.email} Unverified")
+
+    email.confirm
+    expect(email.confirmed?).to be_truthy
+
+    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).not_to have_content('Resend confirmation email')
+    expect(page).to have_content('Send confirmation email')
+  end
+end

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

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

spec/models/email_spec.rb

@@ -11,4 +11,33 @@ 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
+    end
+  end
+
+  describe 'scopes' do
+    let(:user) { create(:user) }
+
+    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
+  end
 end

spec/models/gpg_key_spec.rb

@@ -90,11 +90,20 @@ describe GpgKey do
     it 'email is verified if the user has the matching email' do
       user = create :user, email: 'bette.cartwright@example.com'
       gpg_key = create :gpg_key, key: GpgHelpers::User2.public_key, 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
       )
+
+      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
 

spec/models/user_spec.rb

@@ -360,9 +360,22 @@ 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
+  describe 'after commit hook' do
     describe '.update_invalid_gpg_signatures' do
       let(:user) do
         create(:user, email: 'tula.torphy@abshire.ca').tap do |user|
@@ -376,10 +389,50 @@ 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
+
+    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 '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
+
+      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
+        @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
@@ -467,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
@@ -474,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
@@ -481,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
@@ -523,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
@@ -633,6 +690,7 @@ describe User do
 
     it "blocks user" do
       user.block
+
       expect(user.blocked?).to be_truthy
     end
   end
@@ -966,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
@@ -978,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
@@ -1067,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
@@ -1094,6 +1156,50 @@ 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 match_array([user.email, email_unconfirmed.email, email_confirmed.email])
+    end
+  end
+
+  describe '#verified_emails' do
+    let(:user) { create(:user) }
+
+    it 'returns only confirmed emails' 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
+
+  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
+      create :email, user: user
+      user.reload
+
+      expect(user.verified_email?(user.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
+      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 }
 
@@ -1101,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
 
@@ -1111,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
 
@@ -1121,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
@@ -1136,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
@@ -1153,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
@@ -1162,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
@@ -1234,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
@@ -1257,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
@@ -1438,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
 
@@ -1461,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
 
@@ -1473,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
@@ -2019,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
@@ -2040,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
 
@@ -2058,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
@@ -2074,56 +2208,49 @@ 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 }
 
     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
@@ -2139,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
@@ -2147,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

spec/services/emails/confirm_service_spec.rb

@@ -0,0 +1,15 @@
+require 'spec_helper'
+
+describe Emails::ConfirmService do
+  let(:user) { create(:user) }
+
+  subject(:service) { described_class.new(user) }
+
+  describe '#execute' do
+    it 'sends a confirmation email again' do
+      email = user.emails.create(email: 'new@email.com')
+      mail  = service.execute(email)
+      expect(mail.subject).to eq('Confirmation instructions')
+    end
+  end
+end

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
 

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, user: user, email: email.email) }
+  subject(:service) { described_class.new(user, user: 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

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