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

9 files changed, 95 insertions, 27 deletions

diff --git a/CHANGELOG b/CHANGELOG
index 04647ac901c..1993bea31ad 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -4,6 +4,7 @@ v 8.5.0 (unreleased)
   - Add "visibility" flag to GET /projects api endpoint
 
 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 a08181b910f..dc7cb9f667f 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: 20160119112418) do
+ActiveRecord::Schema.define(version: 20160119145451) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -854,6 +854,7 @@ ActiveRecord::Schema.define(version: 20160119112418) 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).