Merge branch 'dm-ldap-email-readonly' into 'master'

app/models/identity.rb

@@ -14,11 +14,11 @@ class Identity < ActiveRecord::Base
   end
 
   def ldap?
-    provider.starts_with?('ldap')
+    Gitlab::OAuth::Provider.ldap_provider?(provider)
   end
 
   def self.normalize_uid(provider, uid)
-    if provider.to_s.starts_with?('ldap')
+    if Gitlab::OAuth::Provider.ldap_provider?(provider)
       Gitlab::LDAP::Person.normalize_dn(uid)
     else
       uid.to_s

app/models/user.rb

@@ -738,7 +738,7 @@ class User < ActiveRecord::Base
 
   def ldap_user?
     if identities.loaded?
-      identities.find { |identity| identity.provider.start_with?('ldap') && !identity.extern_uid.nil? }
+      identities.find { |identity| Gitlab::OAuth::Provider.ldap_provider?(identity.provider) && !identity.extern_uid.nil? }
     else
       identities.exists?(["provider LIKE ? AND extern_uid IS NOT NULL", "ldap%"])
     end

app/models/user_synced_attributes_metadata.rb

@@ -6,11 +6,11 @@ class UserSyncedAttributesMetadata < ActiveRecord::Base
   SYNCABLE_ATTRIBUTES = %i[name email location].freeze
 
   def read_only?(attribute)
-    Gitlab.config.omniauth.sync_profile_from_provider && synced?(attribute)
+    sync_profile_from_provider? && synced?(attribute)
   end
 
   def read_only_attributes
-    return [] unless Gitlab.config.omniauth.sync_profile_from_provider
+    return [] unless sync_profile_from_provider?
 
     SYNCABLE_ATTRIBUTES.select { |key| synced?(key) }
   end
@@ -22,4 +22,10 @@ class UserSyncedAttributesMetadata < ActiveRecord::Base
   def set_attribute_synced(attribute, value)
     write_attribute("#{attribute}_synced", value)
   end
+
+  private
+
+  def sync_profile_from_provider?
+    Gitlab::OAuth::Provider.sync_profile_from_provider?(provider)
+  end
 end

changelogs/unreleased/dm-ldap-email-readonly.yml

+---
+title: Make sure user email is read only when synced with LDAP
+merge_request: 15915
+author:
+type: fixed

config/gitlab.yml.example

@@ -383,6 +383,7 @@ production: &base
     # Sync user's profile from the specified Omniauth providers every time the user logs in (default: empty).
     # Define the allowed providers using an array, e.g. ["cas3", "saml", "twitter"],
     # or as true/false to allow all providers or none.
+    # When authenticating using LDAP, the user's email is always synced.
     # sync_profile_from_provider: []
 
     # Select which info to sync from the providers above. (default: email).

doc/integration/omniauth.md

@@ -229,16 +229,18 @@ In order to enable/disable an OmniAuth provider, go to Admin Area -> Settings ->
 ## Keep OmniAuth user profiles up to date
 
 You can enable profile syncing from selected OmniAuth providers and for all or for specific user information.
- 
+
+When authenticating using LDAP, the user's email is always synced.
+
  ```ruby
    gitlab_rails['sync_profile_from_provider'] = ['twitter', 'google_oauth2']
    gitlab_rails['sync_profile_attributes'] = ['name', 'email', 'location']
  ```
- 
+
  **For installations from source**
- 
+
  ```yaml
    omniauth:
      sync_profile_from_provider: ['twitter', 'google_oauth2']
-     sync_profile_claims_from_provider: ['email', 'location']
- ```
\ No newline at end of file
+     sync_profile_attributes: ['email', 'location']
+ ```

lib/gitlab/ldap/user.rb

@@ -36,10 +36,6 @@ module Gitlab
         ldap_config.block_auto_created_users
       end
 
-      def sync_profile_from_provider?
-        true
-      end
-
       def allowed?
         Gitlab::LDAP::Access.allowed?(gl_user)
       end

lib/gitlab/o_auth/provider.rb

@@ -19,6 +19,18 @@ module Gitlab
         name.to_s.start_with?('ldap')
       end
 
+      def self.sync_profile_from_provider?(provider)
+        return true if ldap_provider?(provider)
+
+        providers = Gitlab.config.omniauth.sync_profile_from_provider
+
+        if providers.is_a?(Array)
+          providers.include?(provider)
+        else
+          providers
+        end
+      end
+
       def self.config_for(name)
         name = name.to_s
         if ldap_provider?(name)

lib/gitlab/o_auth/user.rb

@@ -12,7 +12,7 @@ module Gitlab
 
       def initialize(auth_hash)
         self.auth_hash = auth_hash
-        update_profile if sync_profile_from_provider?
+        update_profile
         add_or_update_user_identities
       end
 
@@ -195,29 +195,31 @@ module Gitlab
       end
 
       def sync_profile_from_provider?
-        providers = Gitlab.config.omniauth.sync_profile_from_provider
-
-        if providers.is_a?(Array)
-          providers.include?(auth_hash.provider)
-        else
-          providers
-        end
+        Gitlab::OAuth::Provider.sync_profile_from_provider?(auth_hash.provider)
       end
 
       def update_profile
-        user_synced_attributes_metadata = gl_user.user_synced_attributes_metadata || gl_user.build_user_synced_attributes_metadata
-
-        UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key|
-          if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key)
-            gl_user[key] = auth_hash.public_send(key) # rubocop:disable GitlabSecurity/PublicSend
-            user_synced_attributes_metadata.set_attribute_synced(key, true)
-          else
-            user_synced_attributes_metadata.set_attribute_synced(key, false)
+        return unless sync_profile_from_provider? || creating_linked_ldap_user?
+
+        metadata = gl_user.user_synced_attributes_metadata || gl_user.build_user_synced_attributes_metadata
+
+        if sync_profile_from_provider?
+          UserSyncedAttributesMetadata::SYNCABLE_ATTRIBUTES.each do |key|
+            if auth_hash.has_attribute?(key) && gl_user.sync_attribute?(key)
+              gl_user[key] = auth_hash.public_send(key) # rubocop:disable GitlabSecurity/PublicSend
+              metadata.set_attribute_synced(key, true)
+            else
+              metadata.set_attribute_synced(key, false)
+            end
           end
+
+          metadata.provider = auth_hash.provider
         end
 
-        user_synced_attributes_metadata.provider = auth_hash.provider
-        gl_user.user_synced_attributes_metadata = user_synced_attributes_metadata
+        if creating_linked_ldap_user? && gl_user.email == ldap_person.email.first
+          metadata.set_attribute_synced(:email, true)
+          metadata.provider = ldap_person.provider
+        end
       end
 
       def log

spec/lib/gitlab/ldap/user_spec.rb

@@ -38,7 +38,6 @@ describe Gitlab::LDAP::User do
 
     it "does not mark existing ldap user as changed" do
       create(:omniauth_user, email: 'john@example.com', extern_uid: 'uid=john smith,ou=people,dc=example,dc=com', provider: 'ldapmain')
-      ldap_user.gl_user.user_synced_attributes_metadata(provider: 'ldapmain', email: true)
       expect(ldap_user.changed?).to be_falsey
     end
   end
@@ -144,11 +143,15 @@ describe Gitlab::LDAP::User do
         expect(ldap_user.gl_user.email).to eq(info[:email])
       end
 
-      it "has user_synced_attributes_metadata email set to true" do
+      it "has email set as synced" do
         expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
       end
 
-      it "has synced_attribute_provider set to ldapmain" do
+      it "has email set as read-only" do
+        expect(ldap_user.gl_user.read_only_attribute?(:email)).to be_truthy
+      end
+
+      it "has synced attributes provider set to ldapmain" do
         expect(ldap_user.gl_user.user_synced_attributes_metadata.provider).to eql 'ldapmain'
       end
     end
@@ -162,9 +165,13 @@ describe Gitlab::LDAP::User do
         expect(ldap_user.gl_user.temp_oauth_email?).to be_truthy
       end
 
-      it "has synced attribute email set to false" do
+      it "has email set as not synced" do
         expect(ldap_user.gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
       end
+
+      it "does not have email set as read-only" do
+        expect(ldap_user.gl_user.read_only_attribute?(:email)).to be_falsey
+      end
     end
   end
 

spec/lib/gitlab/o_auth/user_spec.rb

@@ -202,11 +202,13 @@ describe Gitlab::OAuth::User do
             end
 
             context "and no account for the LDAP user" do
-              it "creates a user with dual LDAP and omniauth identities" do
+              before do
                 allow(Gitlab::LDAP::Person).to receive(:find_by_uid).and_return(ldap_user)
 
                 oauth_user.save
+              end
 
+              it "creates a user with dual LDAP and omniauth identities" do
                 expect(gl_user).to be_valid
                 expect(gl_user.username).to eql uid
                 expect(gl_user.email).to eql 'johndoe@example.com'
@@ -219,6 +221,18 @@ describe Gitlab::OAuth::User do
                   ]
                 )
               end
+
+              it "has email set as synced" do
+                expect(gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
+              end
+
+              it "has email set as read-only" do
+                expect(gl_user.read_only_attribute?(:email)).to be_truthy
+              end
+
+              it "has synced attributes provider set to ldapmain" do
+                expect(gl_user.user_synced_attributes_metadata.provider).to eql 'ldapmain'
+              end
             end
 
             context "and LDAP user has an account already" do
@@ -440,11 +454,15 @@ describe Gitlab::OAuth::User do
         expect(gl_user.email).to eq(info_hash[:email])
       end
 
-      it "has external_attributes set to true" do
-        expect(gl_user.user_synced_attributes_metadata).not_to be_nil
+      it "has email set as synced" do
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be_truthy
+      end
+
+      it "has email set as read-only" do
+        expect(gl_user.read_only_attribute?(:email)).to be_truthy
       end
 
-      it "has attributes_provider set to my-provider" do
+      it "has synced attributes provider set to my-provider" do
         expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
       end
     end
@@ -458,10 +476,13 @@ describe Gitlab::OAuth::User do
         expect(gl_user.email).not_to eq(info_hash[:email])
       end
 
-      it "has user_synced_attributes_metadata set to nil" do
-        expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
+      it "has email set as not synced" do
         expect(gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
       end
+
+      it "does not have email set as read-only" do
+        expect(gl_user.read_only_attribute?(:email)).to be_falsey
+      end
     end
   end
 
@@ -508,11 +529,15 @@ describe Gitlab::OAuth::User do
         expect(gl_user.email).to eq(info_hash[:email])
       end
 
-      it "has email_synced_attribute set to true" do
+      it "has email set as synced" do
         expect(gl_user.user_synced_attributes_metadata.email_synced).to be(true)
       end
 
-      it "has my-provider as attributes_provider" do
+      it "has email set as read-only" do
+        expect(gl_user.read_only_attribute?(:email)).to be_truthy
+      end
+
+      it "has synced attributes provider set to my-provider" do
         expect(gl_user.user_synced_attributes_metadata.provider).to eql 'my-provider'
       end
     end
@@ -524,7 +549,14 @@ describe Gitlab::OAuth::User do
 
       it "does not update the user email" do
         expect(gl_user.email).not_to eq(info_hash[:email])
-        expect(gl_user.user_synced_attributes_metadata.email_synced).to be(false)
+      end
+
+      it "has email set as not synced" do
+        expect(gl_user.user_synced_attributes_metadata.email_synced).to be_falsey
+      end
+
+      it "does not have email set as read-only" do
+        expect(gl_user.read_only_attribute?(:email)).to be_falsey
       end
     end
   end