diff options
| -rw-r--r-- | CHANGELOG | 1 | ||||
| -rw-r--r-- | app/models/user.rb | 5 | ||||
| -rw-r--r-- | app/views/profiles/show.html.haml | 4 | ||||
| -rw-r--r-- | db/migrate/20160119145451_add_ldap_email_to_users.rb | 30 | ||||
| -rw-r--r-- | db/schema.rb | 3 | ||||
| -rw-r--r-- | lib/gitlab/ldap/user.rb | 29 | ||||
| -rw-r--r-- | lib/gitlab/o_auth/auth_hash.rb | 8 | ||||
| -rw-r--r-- | lib/gitlab/o_auth/user.rb | 14 | ||||
| -rw-r--r-- | spec/lib/gitlab/ldap/user_spec.rb | 28 |
9 files changed, 95 insertions, 27 deletions
diff --git a/CHANGELOG b/CHANGELOG index 207ddf1dd64..dce909089f1 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -3,6 +3,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.5.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 - Consider re-assign as a mention from a notification point of view - Add pagination headers to already paginated API resources diff --git a/app/models/user.rb b/app/models/user.rb index 592468933ed..4214f01f6a4 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -664,7 +664,10 @@ class User < ActiveRecord::Base end 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 def hook_attrs diff --git a/app/views/profiles/show.html.haml b/app/views/profiles/show.html.haml index 9459d8a6295..add9a00138b 100644 --- a/app/views/profiles/show.html.haml +++ b/app/views/profiles/show.html.haml @@ -21,10 +21,10 @@ .form-group = f.label :email, class: "control-label" .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 %span.help-block.light - Email is read-only for LDAP user + Your email address was automatically set based on the LDAP server. - else - if @user.temp_oauth_email? = f.text_field :email, class: "form-control", required: true, value: nil diff --git a/db/migrate/20160119145451_add_ldap_email_to_users.rb b/db/migrate/20160119145451_add_ldap_email_to_users.rb new file mode 100644 index 00000000000..654d31ab15a --- /dev/null +++ b/db/migrate/20160119145451_add_ldap_email_to_users.rb @@ -0,0 +1,30 @@ +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 diff --git a/db/schema.rb b/db/schema.rb index 9045135dd9a..46811904024 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: 20160113111034) do +ActiveRecord::Schema.define(version: 20160119145451) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -850,6 +850,7 @@ ActiveRecord::Schema.define(version: 20160113111034) 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 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 aef08c97d1d..e044f0ecc6d 100644 --- a/lib/gitlab/ldap/user.rb +++ b/lib/gitlab/ldap/user.rb @@ -30,28 +30,31 @@ module Gitlab end def find_by_uid_and_provider - self.class.find_by_uid_and_provider( - auth_hash.uid, auth_hash.provider) + self.class.find_by_uid_and_provider(auth_hash.uid, auth_hash.provider) end 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 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! - gl_user.email = auth_hash.email + # 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) - # 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) + # For a new identity set extern_uid to the LDAP DN + # For an existing identity with matching email but changed DN, update the DN. + # 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 - # 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.ldap_email = auth_hash.has_email? gl_user end diff --git a/lib/gitlab/o_auth/auth_hash.rb b/lib/gitlab/o_auth/auth_hash.rb index ba31599432b..36e5c2670bb 100644 --- a/lib/gitlab/o_auth/auth_hash.rb +++ b/lib/gitlab/o_auth/auth_hash.rb @@ -32,6 +32,10 @@ module Gitlab @password ||= Gitlab::Utils.force_utf8(Devise.friendly_token[0, 8].downcase) end + def has_email? + get_info(:email).present? + end + private def info @@ -46,8 +50,8 @@ module Gitlab def username_and_email @username_and_email ||= begin - username = get_info(:username) || get_info(:nickname) - email = get_info(:email) + username = get_info(:username).presence || get_info(:nickname).presence + email = get_info(:email).presence username ||= generate_username(email) if email email ||= generate_temporarily_email(username) if username diff --git a/lib/gitlab/o_auth/user.rb b/lib/gitlab/o_auth/user.rb index e3d2cc65a8f..d87a72f7ba3 100644 --- a/lib/gitlab/o_auth/user.rb +++ b/lib/gitlab/o_auth/user.rb @@ -111,7 +111,7 @@ module Gitlab def block_after_signup? if creating_linked_ldap_user? ldap_config.block_auto_created_users - else + else Gitlab.config.omniauth.block_auto_created_users end end @@ -135,16 +135,16 @@ module Gitlab def user_attributes # Give preference to LDAP for sensitive information when creating a linked account if creating_linked_ldap_user? - username = ldap_person.username - email = ldap_person.email.first - else - username = auth_hash.username - email = auth_hash.email + username = ldap_person.username.presence + email = ldap_person.email.first.presence end + username ||= auth_hash.username + email ||= auth_hash.email + name = auth_hash.name name = ::Namespace.clean_path(username) if name.strip.empty? - + { name: name, username: ::Namespace.clean_path(username), diff --git a/spec/lib/gitlab/ldap/user_spec.rb b/spec/lib/gitlab/ldap/user_spec.rb index 1e755259dae..03199a2523e 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 "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 end end @@ -110,6 +110,32 @@ describe Gitlab::LDAP::User, lib: true do 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 def configure_block(value) allow_any_instance_of(Gitlab::LDAP::Config). |