Sync email address from specified omniauth provider

12 files changed, 157 insertions, 22 deletions

diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 8cd1c47eb3f..72f34930ca8 100644
--- a/app/controllers/profiles_controller.rb
+++ b/app/controllers/profiles_controller.rb
@@ -9,7 +9,7 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def update
-    user_params.except!(:email) if @user.ldap_user?
+    user_params.except!(:email) if @user.external_email?
 
     respond_to do |format|
       if @user.update_attributes(user_params)
@@ -76,7 +76,7 @@ class ProfilesController < Profiles::ApplicationController
   end
 
   def user_params
-    params.require(:user).permit(
+    @user_params ||= params.require(:user).permit(
       :avatar,
       :bio,
       :email,
diff --git a/app/helpers/profiles_helper.rb b/app/helpers/profiles_helper.rb
new file mode 100644
index 00000000000..45238f12ac7
--- /dev/null
+++ b/app/helpers/profiles_helper.rb
@@ -0,0 +1,7 @@
+module ProfilesHelper
+  def email_provider_label
+    return unless current_user.external_email?
+
+    current_user.email_provider.present? ? Gitlab::OAuth::Provider.label_for(current_user.email_provider) : "LDAP"
+  end
+end
diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml
index 4a1438aa68e..fcfd350f0da 100644
--- a/app/views/profiles/show.html.haml
+++ b/app/views/profiles/show.html.haml
@@ -49,10 +49,10 @@
 
       .form-group
         = f.label :email, class: "label-light"
-        - if @user.ldap_user? && @user.ldap_email?
+        - if @user.external_email?
           = f.text_field :email, class: "form-control", required: true, readonly: true
           %span.help-block.light
-            Your email address was automatically set based on the LDAP server.
+            Your email address was automatically set based on your #{email_provider_label} account.
         - else
           - if @user.temp_oauth_email?
             = f.text_field :email, class: "form-control", required: true, value: nil
diff --git a/changelogs/unreleased/sync-email-from-omniauth.yml b/changelogs/unreleased/sync-email-from-omniauth.yml
new file mode 100644
index 00000000000..ed14a95a5f1
--- /dev/null
+++ b/changelogs/unreleased/sync-email-from-omniauth.yml
@@ -0,0 +1,4 @@
+---
+title: Sync email address from specified omniauth provider
+merge_request: 11268
+author: Robin Bobbitt
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index d2aeb66ebf6..0b33783869b 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -337,6 +337,10 @@ production: &base
     # showing GitLab's sign-in page (default: show the GitLab sign-in page)
     # auto_sign_in_with_provider: saml
 
+    # Sync user's email address from the specified Omniauth provider every time the user logs
+    # in (default: nil). And consequently make this field read-only.
+    # sync_email_from_provider: cas3
+
     # CAUTION!
     # This allows users to login without having a user account first. Define the allowed providers
     # using an array, e.g. ["saml", "twitter"], or as true/false to allow all providers or none.
diff --git a/config/initializers/1_settings.rb b/config/initializers/1_settings.rb
index 45ea2040d23..8ddf8e4d2e4 100644
--- a/config/initializers/1_settings.rb
+++ b/config/initializers/1_settings.rb
@@ -156,6 +156,7 @@ Settings.omniauth['external_providers'] = [] if Settings.omniauth['external_prov
 Settings.omniauth['block_auto_created_users'] = true if Settings.omniauth['block_auto_created_users'].nil?
 Settings.omniauth['auto_link_ldap_user'] = false if Settings.omniauth['auto_link_ldap_user'].nil?
 Settings.omniauth['auto_link_saml_user'] = false if Settings.omniauth['auto_link_saml_user'].nil?
+Settings.omniauth['sync_email_from_provider'] ||= nil
 
 Settings.omniauth['providers'] ||= []
 Settings.omniauth['cas3'] ||= Settingslogic.new({})
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 2d5e47a6f3b..5e299e26c54 100644
--- a/lib/gitlab/ldap/user.rb
+++ b/lib/gitlab/ldap/user.rb
@@ -41,11 +41,6 @@ module Gitlab
 
       def update_user_attributes
         if persisted?
-          if auth_hash.has_email?
-            gl_user.skip_reconfirmation!
-            gl_user.email = auth_hash.email
-          end
-
           # find_or_initialize_by doesn't update `gl_user.identities`, and isn't autosaved.
           identity = gl_user.identities.find { |identity|  identity.provider == auth_hash.provider }
           identity ||= gl_user.identities.build(provider: auth_hash.provider)
@@ -55,10 +50,6 @@ module Gitlab
           # For an existing identity with no change in DN, this line changes nothing.
           identity.extern_uid = auth_hash.uid
         end
-
-        gl_user.ldap_email = auth_hash.has_email?
-
-        gl_user
       end
 
       def changed?
@@ -69,6 +60,10 @@ module Gitlab
         ldap_config.block_auto_created_users
       end
 
+      def sync_email_from_provider?
+        true
+      end
+
       def allowed?
         Gitlab::LDAP::Access.allowed?(gl_user)
       end
diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb
index afd24b4dcc5..7307f8c2c87 100644
--- a/lib/gitlab/o_auth/user.rb
+++ b/lib/gitlab/o_auth/user.rb
@@ -12,6 +12,7 @@ module Gitlab
 
       def initialize(auth_hash)
         self.auth_hash = auth_hash
+        update_email
       end
 
       def persisted?
@@ -174,6 +175,22 @@ module Gitlab
         }
       end
 
+      def sync_email_from_provider?
+        auth_hash.provider.to_s == Gitlab.config.omniauth.sync_email_from_provider.to_s
+      end
+
+      def update_email
+        if auth_hash.has_email? && sync_email_from_provider?
+          if persisted?
+            gl_user.skip_reconfirmation!
+            gl_user.email = auth_hash.email
+          end
+
+          gl_user.external_email = true
+          gl_user.email_provider = auth_hash.provider
+        end
+      end
+
       def log
         Gitlab::AppLogger
       end
diff --git a/spec/controllers/profiles_controller_spec.rb b/spec/controllers/profiles_controller_spec.rb
new file mode 100644
index 00000000000..9d60dab12d1
--- /dev/null
+++ b/spec/controllers/profiles_controller_spec.rb
@@ -0,0 +1,31 @@
+require('spec_helper')
+
+describe ProfilesController do
+  describe "PUT update" do
+    it "allows an email update from a user without an external email address" do
+      user = create(:user)
+      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('john@gmail.com')
+    end
+
+    it "ignores an email update from a user with an external email address" do
+      ldap_user = create(:omniauth_user, external_email: true)
+      sign_in(ldap_user)
+
+      put :update,
+          user: { email: "john@gmail.com", name: "John" }
+
+      ldap_user.reload
+
+      expect(response.status).to eq(302)
+      expect(ldap_user.unconfirmed_email).not_to eq('john@gmail.com')
+    end
+  end
+end
diff --git a/spec/helpers/profiles_helper_spec.rb b/spec/helpers/profiles_helper_spec.rb
new file mode 100644
index 00000000000..b33b3f3a228
--- /dev/null
+++ b/spec/helpers/profiles_helper_spec.rb
@@ -0,0 +1,36 @@
+require 'rails_helper'
+
+describe ProfilesHelper do
+  describe '#email_provider_label' do
+    it "returns nil for users without external email" do
+      user = create(:user)
+      allow(helper).to receive(:current_user).and_return(user)
+
+      expect(helper.email_provider_label).to be_nil
+    end
+
+    it "returns omniauth provider label for users with external email" do
+      stub_cas_omniauth_provider
+      cas_user = create(:omniauth_user, provider: 'cas3', external_email: true, email_provider: 'cas3')
+      allow(helper).to receive(:current_user).and_return(cas_user)
+
+      expect(helper.email_provider_label).to eq('CAS')
+    end
+
+    it "returns 'LDAP' for users with external email but no email provider" do
+      ldap_user = create(:omniauth_user, external_email: true)
+      allow(helper).to receive(:current_user).and_return(ldap_user)
+
+      expect(helper.email_provider_label).to eq('LDAP')
+    end
+  end
+
+  def stub_cas_omniauth_provider
+    provider = OpenStruct.new(
+      'name' => 'cas3',
+      'label' => 'CAS'
+    )
+
+    stub_omniauth_setting(providers: [provider])
+  end
+end
diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb
index f4aab429931..a0eda685ca3 100644
--- a/spec/lib/gitlab/ldap/user_spec.rb
+++ b/spec/lib/gitlab/ldap/user_spec.rb
@@ -37,7 +37,7 @@ describe Gitlab::LDAP::User, lib: true do
     end
 
     it "does not mark existing ldap user as changed" do
-      create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain', ldap_email: true)
+      create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain', external_email: true, email_provider: 'ldapmain')
       expect(ldap_user.changed?).to be_falsey
     end
   end
@@ -141,8 +141,12 @@ describe Gitlab::LDAP::User, lib: true do
         expect(ldap_user.gl_user.email).to eq(info[:email])
       end
 
-      it "has ldap_email set to true" do
-        expect(ldap_user.gl_user.ldap_email?).to be(true)
+      it "has external_email set to true" do
+        expect(ldap_user.gl_user.external_email?).to be(true)
+      end
+
+      it "has email_provider set to provider" do
+        expect(ldap_user.gl_user.email_provider).to eql 'ldapmain'
       end
     end
 
@@ -155,8 +159,8 @@ describe Gitlab::LDAP::User, lib: true do
         expect(ldap_user.gl_user.temp_oauth_email?).to be(true)
       end
 
-      it "has ldap_email set to false" do
-        expect(ldap_user.gl_user.ldap_email?).to be(false)
+      it "has external_email set to false" do
+        expect(ldap_user.gl_user.external_email?).to be(false)
       end
     end
   end
diff --git a/spec/lib/gitlab/o_auth/user_spec.rb b/spec/lib/gitlab/o_auth/user_spec.rb
index 828c953197d..8943d1aa488 100644
--- a/spec/lib/gitlab/o_auth/user_spec.rb
+++ b/spec/lib/gitlab/o_auth/user_spec.rb
@@ -28,11 +28,11 @@ describe Gitlab::OAuth::User, lib: true do
     end
   end
 
-  describe '#save' do
-    def stub_omniauth_config(messages)
-      allow(Gitlab.config.omniauth).to receive_messages(messages)
-    end
+  def stub_omniauth_config(messages)
+    allow(Gitlab.config.omniauth).to receive_messages(messages)
+  end
 
+  describe '#save' do
     def stub_ldap_config(messages)
       allow(Gitlab::LDAP::Config).to receive_messages(messages)
     end
@@ -377,4 +377,40 @@ describe Gitlab::OAuth::User, lib: true do
       end
     end
   end
+
+  describe 'updating email' do
+    let!(:existing_user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'my-provider') }
+
+    before do
+      stub_omniauth_config(sync_email_from_provider: 'my-provider')
+    end
+
+    context "when provider sets an email" do
+      it "updates the user email" do
+        expect(gl_user.email).to eq(info_hash[:email])
+      end
+
+      it "has external_email set to true" do
+        expect(gl_user.external_email?).to be(true)
+      end
+
+      it "has email_provider set to provider" do
+        expect(gl_user.email_provider).to eql 'my-provider'
+      end
+    end
+
+    context "when provider doesn't set an email" do
+      before do
+        info_hash.delete(:email)
+      end
+
+      it "does not update the user email" do
+        expect(gl_user.email).not_to eq(info_hash[:email])
+      end
+
+      it "has external_email set to false" do
+        expect(gl_user.external_email?).to be(false)
+      end
+    end
+  end
 end