Commit a3a008f6 authored by Robert Speicher's avatar Robert Speicher Committed by Robert Speicher

Merge branch 'user-ldap-email' into 'master'

Allow LDAP users to change their email if it was not set by the LDAP
server

Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/3054

See merge request !2502
parent 6d0bf9d0
Please view this file on the master branch, on stable branches it's out of date. Please view this file on the master branch, on stable branches it's out of date.
v 8.4.0 (unreleased) v 8.4.0 (unreleased)
- Allow LDAP users to change their email if it was not set by the LDAP server
- Ensure Gravatar host looks like an actual host - Ensure Gravatar host looks like an actual host
- Consider re-assign as a mention from a notification point of view - Consider re-assign as a mention from a notification point of view
- Add pagination headers to already paginated API resources - Add pagination headers to already paginated API resources
......
...@@ -664,7 +664,10 @@ class User < ActiveRecord::Base ...@@ -664,7 +664,10 @@ class User < ActiveRecord::Base
end end
def all_emails def all_emails
[self.email, *self.emails.map(&:email)] all_emails = []
all_emails << self.email unless self.temp_oauth_email?
all_emails.concat(self.emails.map(&:email))
all_emails
end end
def hook_attrs def hook_attrs
......
...@@ -21,10 +21,10 @@ ...@@ -21,10 +21,10 @@
.form-group .form-group
= f.label :email, class: "control-label" = f.label :email, class: "control-label"
.col-sm-10 .col-sm-10
- if @user.ldap_user? - if @user.ldap_user? && @user.ldap_email?
= f.text_field :email, class: "form-control", required: true, readonly: true = f.text_field :email, class: "form-control", required: true, readonly: true
%span.help-block.light %span.help-block.light
Email is read-only for LDAP user Your email address was automatically set based on the LDAP server.
- else - else
- if @user.temp_oauth_email? - if @user.temp_oauth_email?
= f.text_field :email, class: "form-control", required: true, value: nil = f.text_field :email, class: "form-control", required: true, value: nil
......
class AddLdapEmailToUsers < ActiveRecord::Migration
def up
add_column :users, :ldap_email, :boolean, default: false, null: false
if Gitlab::Database.mysql?
execute %{
UPDATE users, identities
SET users.ldap_email = TRUE
WHERE identities.user_id = users.id
AND users.email LIKE 'temp-email-for-oauth%'
AND identities.provider LIKE 'ldap%'
AND identities.extern_uid IS NOT NULL
}
else
execute %{
UPDATE users
SET ldap_email = TRUE
FROM identities
WHERE identities.user_id = users.id
AND users.email LIKE 'temp-email-for-oauth%'
AND identities.provider LIKE 'ldap%'
AND identities.extern_uid IS NOT NULL
}
end
end
def down
remove_column :users, :ldap_email
end
end
...@@ -11,7 +11,7 @@ ...@@ -11,7 +11,7 @@
# #
# It's strongly recommended that you check this file into your version control system. # It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20160119112418) do ActiveRecord::Schema.define(version: 20160119145451) do
# These are extensions that must be enabled in order to support this database # These are extensions that must be enabled in order to support this database
enable_extension "plpgsql" enable_extension "plpgsql"
...@@ -854,6 +854,7 @@ ActiveRecord::Schema.define(version: 20160119112418) do ...@@ -854,6 +854,7 @@ ActiveRecord::Schema.define(version: 20160119112418) do
t.boolean "hide_project_limit", default: false t.boolean "hide_project_limit", default: false
t.string "unlock_token" t.string "unlock_token"
t.datetime "otp_grace_period_started_at" t.datetime "otp_grace_period_started_at"
t.boolean "ldap_email", default: false, null: false
end end
add_index "users", ["admin"], name: "index_users_on_admin", using: :btree add_index "users", ["admin"], name: "index_users_on_admin", using: :btree
......
...@@ -30,28 +30,31 @@ module Gitlab ...@@ -30,28 +30,31 @@ module Gitlab
end end
def find_by_uid_and_provider def find_by_uid_and_provider
self.class.find_by_uid_and_provider( self.class.find_by_uid_and_provider(auth_hash.uid, auth_hash.provider)
auth_hash.uid, auth_hash.provider)
end end
def find_by_email def find_by_email
::User.find_by(email: auth_hash.email.downcase) ::User.find_by(email: auth_hash.email.downcase) if auth_hash.has_email?
end end
def update_user_attributes def update_user_attributes
return unless persisted? if persisted?
if auth_hash.has_email?
gl_user.skip_reconfirmation!
gl_user.email = auth_hash.email
end
gl_user.skip_reconfirmation! # find_or_initialize_by doesn't update `gl_user.identities`, and isn't autosaved.
gl_user.email = auth_hash.email identity = gl_user.identities.find { |identity| identity.provider == auth_hash.provider }
identity ||= gl_user.identities.build(provider: auth_hash.provider)
# find_or_initialize_by doesn't update `gl_user.identities`, and isn't autosaved. # For a new identity set extern_uid to the LDAP DN
identity = gl_user.identities.find { |identity| identity.provider == auth_hash.provider } # For an existing identity with matching email but changed DN, update the DN.
identity ||= gl_user.identities.build(provider: auth_hash.provider) # For an existing identity with no change in DN, this line changes nothing.
identity.extern_uid = auth_hash.uid
end
# For a new user set extern_uid to the LDAP DN gl_user.ldap_email = auth_hash.has_email?
# For an existing user with matching email but changed DN, update the DN.
# For an existing user with no change in DN, this line changes nothing.
identity.extern_uid = auth_hash.uid
gl_user gl_user
end end
......
...@@ -32,6 +32,10 @@ module Gitlab ...@@ -32,6 +32,10 @@ module Gitlab
@password ||= Gitlab::Utils.force_utf8(Devise.friendly_token[0, 8].downcase) @password ||= Gitlab::Utils.force_utf8(Devise.friendly_token[0, 8].downcase)
end end
def has_email?
get_info(:email).present?
end
private private
def info def info
...@@ -46,8 +50,8 @@ module Gitlab ...@@ -46,8 +50,8 @@ module Gitlab
def username_and_email def username_and_email
@username_and_email ||= begin @username_and_email ||= begin
username = get_info(:username) || get_info(:nickname) username = get_info(:username).presence || get_info(:nickname).presence
email = get_info(:email) email = get_info(:email).presence
username ||= generate_username(email) if email username ||= generate_username(email) if email
email ||= generate_temporarily_email(username) if username email ||= generate_temporarily_email(username) if username
......
...@@ -111,7 +111,7 @@ module Gitlab ...@@ -111,7 +111,7 @@ module Gitlab
def block_after_signup? def block_after_signup?
if creating_linked_ldap_user? if creating_linked_ldap_user?
ldap_config.block_auto_created_users ldap_config.block_auto_created_users
else else
Gitlab.config.omniauth.block_auto_created_users Gitlab.config.omniauth.block_auto_created_users
end end
end end
...@@ -135,16 +135,16 @@ module Gitlab ...@@ -135,16 +135,16 @@ module Gitlab
def user_attributes def user_attributes
# Give preference to LDAP for sensitive information when creating a linked account # Give preference to LDAP for sensitive information when creating a linked account
if creating_linked_ldap_user? if creating_linked_ldap_user?
username = ldap_person.username username = ldap_person.username.presence
email = ldap_person.email.first email = ldap_person.email.first.presence
else
username = auth_hash.username
email = auth_hash.email
end end
username ||= auth_hash.username
email ||= auth_hash.email
name = auth_hash.name name = auth_hash.name
name = ::Namespace.clean_path(username) if name.strip.empty? name = ::Namespace.clean_path(username) if name.strip.empty?
{ {
name: name, name: name,
username: ::Namespace.clean_path(username), username: ::Namespace.clean_path(username),
......
...@@ -37,7 +37,7 @@ describe Gitlab::LDAP::User, lib: true do ...@@ -37,7 +37,7 @@ describe Gitlab::LDAP::User, lib: true do
end end
it "dont marks existing ldap user as changed" do it "dont marks existing ldap user as changed" do
create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain') create(:omniauth_user, email: 'john@example.com', extern_uid: 'my-uid', provider: 'ldapmain', ldap_email: true)
expect(ldap_user.changed?).to be_falsey expect(ldap_user.changed?).to be_falsey
end end
end end
...@@ -110,6 +110,32 @@ describe Gitlab::LDAP::User, lib: true do ...@@ -110,6 +110,32 @@ describe Gitlab::LDAP::User, lib: true do
end end
end end
describe 'updating email' do
context "when LDAP sets an email" do
it "has a real email" 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)
end
end
context "when LDAP doesn't set an email" do
before do
info.delete(:email)
end
it "has a temp email" 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)
end
end
end
describe 'blocking' do describe 'blocking' do
def configure_block(value) def configure_block(value)
allow_any_instance_of(Gitlab::LDAP::Config). allow_any_instance_of(Gitlab::LDAP::Config).
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment