new ConfirmationService that handles checking conditions and confirming

an email address

6 files changed, 137 insertions, 3 deletions

diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index bc0948cd3fb..bd365aed86b 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -1,4 +1,21 @@
 class ConfirmationsController < Devise::ConfirmationsController
+
+  # GET /resource/confirmation?confirmation_token=abcdef
+  # overridden from Devise
+  # we now allow duplicate unconfirmed emails to be added, to prevent
+  # malicious individuals from blocking the valid owners of an email 
+  def show
+    resource = ConfirmationService.new(resource_class, params[:confirmation_token]).execute
+    yield resource if block_given?
+
+    if resource.errors.empty?
+      set_flash_message!(:notice, :confirmed)
+      respond_with_navigational(resource){ redirect_to after_confirmation_path_for(resource_name, resource) }
+    else
+      respond_with_navigational(resource.errors, status: :unprocessable_entity){ render :new }
+    end
+  end
+
   def almost_there
     flash[:notice] = nil
     render layout: "devise_empty"
diff --git a/app/models/email.rb b/app/models/email.rb
index 513c997f27c..e8c6f3e5586 100644
--- a/app/models/email.rb
+++ b/app/models/email.rb
@@ -13,7 +13,8 @@ class Email < ActiveRecord::Base
   validates_uniqueness_of :email, conditions: -> { where.not(confirmed_at: nil) }
   validate :unique_email
 
-  scope :confirmed, -> { where.not(confirmed_at: nil) }
+  scope :confirmed,   -> { where.not(confirmed_at: nil) }
+  scope :unconfirmed, -> { where(confirmed_at: nil) }
 
   after_commit :update_invalid_gpg_signatures, if: -> { previous_changes.key?('confirmed_at') }
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 6c9349ed9dd..167212c904d 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -230,6 +230,8 @@ class User < ActiveRecord::Base
   scope :todo_authors, ->(user_id, state) { where(id: Todo.where(user_id: user_id, state: state).select(:author_id)) }
   scope :order_recent_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('last_sign_in_at', 'DESC')) }
   scope :order_oldest_sign_in, -> { reorder(Gitlab::Database.nulls_last_order('last_sign_in_at', 'ASC')) }
+  scope :confirmed,   -> { where.not(confirmed_at: nil) }
+  scope :unconfirmed, -> { where(confirmed_at: nil) }
 
   def self.with_two_factor
     joins("LEFT OUTER JOIN u2f_registrations AS u2f ON u2f.user_id = users.id")
@@ -436,7 +438,7 @@ class User < ActiveRecord::Base
   end
 
   def skip_confirmation=(bool)
-    skip_confirmation! if bool
+    # skip_confirmation! if bool
   end
 
   def generate_reset_token
@@ -509,8 +511,10 @@ class User < ActiveRecord::Base
     end
   end
 
+  # user can specify an email that someone else has added as a secondary,
+  # as long as it hasn't been confirmed yet
   def unique_email
-    if !emails.exists?(email: email) && Email.exists?(email: email)
+    if !emails.exists?(email: email) && Email.confirmed.exists?(email: email)
       errors.add(:email, 'has already been taken')
     end
   end
diff --git a/app/services/confirmation_service.rb b/app/services/confirmation_service.rb
new file mode 100644
index 00000000000..7a1b2db35a5
--- /dev/null
+++ b/app/services/confirmation_service.rb
@@ -0,0 +1,31 @@
+# Users and Emails can contain unconfirmed duplicates.  When one is confirmed,
+# remove any duplicates (don't remove duplicate User emails, just leave unconfirmed)
+class ConfirmationService
+
+  def initialize(resource_class, confirmation_token)
+    @resource_class, @confirmation_token = resource_class, confirmation_token
+  end
+
+  def execute
+    resource = @resource_class.find_first_by_auth_conditions(confirmation_token: @confirmation_token)
+    if resource
+      email = resource.email
+
+      # verify email being confirmed is not already confirmed elsewhere
+      unless Email.confirmed.exists?(email: email) || User.confirmed.exists?(email: email)
+        resource = @resource_class.confirm_by_token(@confirmation_token)
+        if resource.errors.empty?
+          # remove any duplicate emails from the emails table
+          Email.unconfirmed.where(email: email).each do |email|
+            email.destroy
+          end
+
+          # nothing to do about the duplicate user emails, just leave unconfirmed
+        end
+      else
+        resource.errors.add(:base, 'This email address was confirmed by someone else')
+      end
+    end
+    resource
+  end
+end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index fb03e320734..70734253277 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -266,6 +266,20 @@ describe User do
           expect(user).to be_valid
         end
       end
+      
+      context 'email added by another user as a secondary' do
+        it 'email should be allowed if it is not confirmed yet' do
+          create(:email, email: 'secondary@example.com')
+          user = build(:user, email: 'secondary@example.com')
+          expect(user).to be_valid
+        end
+
+        it 'email should not be allowed if it is confirmed' do
+          create(:email, :confirmed, email: 'secondary@example.com')
+          user = build(:user, email: 'secondary@example.com')
+          expect(user).not_to be_valid
+        end
+      end
     end
   end
 
diff --git a/spec/services/confirmation_service_spec.rb b/spec/services/confirmation_service_spec.rb
new file mode 100644
index 00000000000..66c82ef93ac
--- /dev/null
+++ b/spec/services/confirmation_service_spec.rb
@@ -0,0 +1,67 @@
+require 'spec_helper'
+
+describe ConfirmationService do
+
+  describe '#execute' do
+    let(:user)  { create(:user) }
+    let(:user2) { create(:user) }
+
+    context 'confirming secondary email' do
+      it 'removes secondary email duplicates' do
+        user.emails.create(email: 'new@email.com', confirmation_token: 'token_1')
+        user2.emails.create(email: 'new@email.com')
+        
+        expect(Email.where(email: 'new@email.com').count).to eq 2
+
+        service  = described_class.new(Email, 'token_1')
+        resource = service.execute
+
+        expect(resource.errors.empty?).to be_truthy
+        expect(Email.where(email: 'new@email.com').count).to eq 1
+        expect(Email.confirmed.count).to eq 1
+      end
+
+      it 'does not confirm with a confirmed user with same email' do
+        user.update_attribute(:confirmed_at, nil)
+        user2.emails.create(email: user.email, confirmation_token: 'token_1')
+        user.update_attribute(:confirmed_at, Time.now)
+        
+        expect(User.confirmed.count).to eq 2
+        expect(Email.confirmed.count).to eq 0
+
+        service  = described_class.new(Email, 'token_1')
+        resource = service.execute
+
+        expect(resource.errors.empty?).to be_falsy
+        expect(User.confirmed.count).to eq 2
+        expect(Email.confirmed.count).to eq 0
+      end
+
+      it 'does not confirm with a confirmed secondary with same email' do
+        user.emails.create(email: 'new@email.com', confirmation_token: 'token_1')
+        user2.emails.create(email: 'new@email.com', confirmed_at: Time.now)
+        
+        expect(Email.confirmed.count).to eq 1
+
+        service  = described_class.new(Email, 'token_1')
+        resource = service.execute
+
+        expect(resource.errors.empty?).to be_falsy
+        expect(Email.confirmed.count).to eq 1
+      end
+    end
+    
+    context 'confirming user email' do
+      it 'removes secondary email duplicates' do
+        user.update_attributes(confirmed_at: nil, confirmation_token: 'token_1')
+        user2.emails.create(email: user.email)
+        
+        service  = described_class.new(User, 'token_1')
+        resource = service.execute
+
+        expect(resource.errors.empty?).to be_truthy
+        expect(Email.count).to eq 0
+      end
+    end
+  end
+end