diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb
index 8cd1c47eb3f915d71adba2bea2a78a890ef3b0f9..72f34930ca8d8d8ffebbfdba18254d9bdd2c7258 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 0000000000000000000000000000000000000000..45238f12ac7d1ae9598d33e15a3ebf5d2e3013f6
--- /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 4a1438aa68e92dba2f15829a2dce5eb9ff69922f..fcfd350f0da1acef09062f5f3bf57bfcb48f4ac5 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 0000000000000000000000000000000000000000..ed14a95a5f11f6b513fa5fe7cdc7de63673fc860
--- /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 d2aeb66ebf61b4fe09a5c7b59b777e66cb285c96..0b33783869b7b514e3ae738853b3b3468de17e05 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 45ea2040d23f06af85195149944047718d447f17..8ddf8e4d2e42745579c55c68baaca52a52e20e94 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/db/migrate/20170531202042_rename_users_ldap_email_to_external_email.rb b/db/migrate/20170531202042_rename_users_ldap_email_to_external_email.rb
new file mode 100644
index 0000000000000000000000000000000000000000..470c3b8166c4cb1c2482885fbc0d4196e57580b5
--- /dev/null
+++ b/db/migrate/20170531202042_rename_users_ldap_email_to_external_email.rb
@@ -0,0 +1,15 @@
+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
diff --git a/db/migrate/20170603200744_add_email_provider_to_users.rb b/db/migrate/20170603200744_add_email_provider_to_users.rb
new file mode 100644
index 0000000000000000000000000000000000000000..ed90af9aadc63bf322a8b08cc8443f76f40423a8
--- /dev/null
+++ b/db/migrate/20170603200744_add_email_provider_to_users.rb
@@ -0,0 +1,9 @@
+class AddEmailProviderToUsers < ActiveRecord::Migration
+ include Gitlab::Database::MigrationHelpers
+
+ DOWNTIME = false
+
+ def change
+ add_column :users, :email_provider, :string
+ end
+end
diff --git a/db/post_migrate/20170531203055_cleanup_users_ldap_email_rename.rb b/db/post_migrate/20170531203055_cleanup_users_ldap_email_rename.rb
new file mode 100644
index 0000000000000000000000000000000000000000..15edb402b86a5bb38aa0e34c26c7c2ea6cefd3a4
--- /dev/null
+++ b/db/post_migrate/20170531203055_cleanup_users_ldap_email_rename.rb
@@ -0,0 +1,15 @@
+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
diff --git a/db/schema.rb b/db/schema.rb
index 400b01f73d82f0af66a55a67953c50eb4aa64918..83172a92b49025e3df7aba3b4a99593148291849 100644
--- a/db/schema.rb
+++ b/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
diff --git a/lib/gitlab/ldap/user.rb b/lib/gitlab/ldap/user.rb
index 2d5e47a6f3b7e648b0ae9bdddaa0327463066b52..5e299e26c54058ce94a7b48f8957edd67eac4691 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 afd24b4dcc5569e6862f6c76d1c81ab2f924730a..7307f8c2c871156a49d30832b9e7c87c6a43b9c1 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 0000000000000000000000000000000000000000..9d60dab12d106861279564ad828b69680db366ba
--- /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 0000000000000000000000000000000000000000..b33b3f3a228e086d527cc05e52cf00b0da317915
--- /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 f4aab4299315a0c27fe04e85dc0a750091aad374..a0eda685ca34e0847065c55d20b5325c3d670223 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 828c953197ded139247d247e2a4db311edde826d..8943d1aa4888ea2fc51519d5128446c1ef56cdaa 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