Commit b9fca478 authored by Robert Speicher's avatar 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
parents 20cc4bc4 4f444556
...@@ -4,6 +4,7 @@ v 8.5.0 (unreleased) ...@@ -4,6 +4,7 @@ v 8.5.0 (unreleased)
- Add "visibility" flag to GET /projects api endpoint - Add "visibility" flag to GET /projects api endpoint
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