Merge branch 'sync-email-from-omniauth' into 'master'

Sync email address from specified omniauth provider

See merge request !11268

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,

app/helpers/profiles_helper.rb

+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

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

changelogs/unreleased/sync-email-from-omniauth.yml

+---
+title: Sync email address from specified omniauth provider
+merge_request: 11268
+author: Robin Bobbitt

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.

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

db/migrate/20170531202042_rename_users_ldap_email_to_external_email.rb

+class RenameUsersLdapEmailToExternalEmail < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    rename_column_concurrently :users, :ldap_email, :external_email
+  end
+
+  def down
+    cleanup_concurrent_column_rename :users, :external_email, :ldap_email
+  end
+end

db/migrate/20170603200744_add_email_provider_to_users.rb

+class AddEmailProviderToUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def change
+    add_column :users, :email_provider, :string
+  end
+end

db/post_migrate/20170531203055_cleanup_users_ldap_email_rename.rb

+class CleanupUsersLdapEmailRename < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def up
+    cleanup_concurrent_column_rename :users, :ldap_email, :external_email
+  end
+
+  def down
+    rename_column_concurrently :users, :external_email, :ldap_email
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20170526185921) do
+ActiveRecord::Schema.define(version: 20170603200744) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1415,7 +1415,6 @@ ActiveRecord::Schema.define(version: 20170526185921) do
     t.boolean "hide_project_limit", default: false
     t.string "unlock_token"
     t.datetime "otp_grace_period_started_at"
-    t.boolean "ldap_email", default: false, null: false
     t.boolean "external", default: false
     t.string "incoming_email_token"
     t.string "organization"
@@ -1426,6 +1425,8 @@ ActiveRecord::Schema.define(version: 20170526185921) do
     t.boolean "notified_of_own_activity"
     t.string "preferred_language"
     t.string "rss_token"
+    t.boolean "external_email", default: false, null: false
+    t.string "email_provider"
   end
 
   add_index "users", ["admin"], name: "index_users_on_admin", using: :btree

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

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

spec/controllers/profiles_controller_spec.rb

+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

spec/helpers/profiles_helper_spec.rb

+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

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

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