Merge branch 'ldap_external_users' into 'master'

Allow LDAP to handle external users, based on users' group memberships

Fixes gitlab-org/gitlab-ce#4009

These MR adds functionality to the `LdapGroupSync` feature that allows
it to receive an array of group names, in order for the members of these
groups to be marked as external users.

See merge request !432

CHANGELOG-EE

@@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date.
 
 v 8.9.0 (unreleased)
   - Fix nil user handling in UpdateMirrorService
+  - Allow LDAP to mark users as external based on their group membership. !432
 
 v 8.8.3
   - Add standard web hook headers to Jenkins CI post. !374

config/gitlab.yml.example

@@ -351,6 +351,12 @@ production: &base
         #
         admin_group: ''
 
+        # LDAP group of users who should be marked as external users in GitLab
+        #
+        #   Ex. ['Contractors', 'Interns']
+        #
+        external_groups: []
+
         # Name of attribute which holds a ssh public key of the user object.
         # If false or nil, SSH key syncronisation will be disabled.
         #

config/initializers/1_settings.rb

@@ -157,6 +157,7 @@ if Settings.ldap['enabled'] || Rails.env.test?
     server['attributes'] = {} if server['attributes'].nil?
     server['provider_name'] ||= "ldap#{key}".downcase
     server['provider_class'] = OmniAuth::Utils.camelize(server['provider_name'])
+    server['external_groups'] = [] if server['external_groups'].nil?
     Settings.ldap['servers'][key] = server
   end
 end

doc/administration/auth/ldap.md

@@ -146,6 +146,14 @@ main: # 'main' is the GitLab 'provider ID' of this LDAP server
     #
     admin_group: ''
 
+    # An array of CNs of groups containing users that should be considered external
+    #
+    #   Ex. ['interns', 'contractors']
+    #
+    #   Note: Not `cn=interns` or the full DN
+    #
+    external_groups: []
+
     # The LDAP attribute containing a user's public SSH key
     #
     #   Ex. ssh_public_key
@@ -184,6 +192,28 @@ production:
         # snip...
 ```
 
+### External Groups
+
+>**Note:** External Groups configuration is only available in GitLab EE Version
+8.9 and above.
+
+Using the `external_groups` setting will allow you to mark all users belonging
+to these groups as [external users](../../permissions/). Group membership is
+checked periodically through the `LdapGroupSync` background task.
+
+**Configuration**
+
+```yaml
+# An array of CNs of groups containing users that should be considered external
+#
+#   Ex. ['interns', 'contractors']
+#
+#   Note: Not `cn=interns` or the full DN
+#
+external_groups: []
+```
+
+
 ## Using an LDAP filter to limit access to your GitLab server
 
 If you want to limit all GitLab access to a subset of the LDAP users on your

lib/gitlab/ldap/config.rb

@@ -101,6 +101,10 @@ module Gitlab
         options['timeout'].to_i
       end
 
+      def external_groups
+        options['external_groups']
+      end
+
       protected
 
       def base_config

lib/gitlab/ldap/group_sync.rb

@@ -49,6 +49,14 @@ module Gitlab
           logger.debug { "No `admin_group` configured for '#{provider}' provider. Skipping" }
         end
 
+        if external_groups.empty?
+          logger.debug { "No `external_groups` configured for '#{provider}' provider. Skipping" }
+        else
+          logger.debug { "Syncing external users for '#{provider}' provider" }
+          sync_external_users
+          logger.debug { "Finished syncing external users for '#{provider}' provider" }
+        end
+
         nil
       end
 
@@ -121,6 +129,36 @@ module Gitlab
         end
       end
 
+      # Update external users based on the specified external groups CN
+      def sync_external_users
+        current_external_users = ::User.external.with_provider(provider)
+        verified_external_users = []
+
+        external_groups.each do |group|
+          group_dns = dns_for_group_cn(group)
+
+          group_dns.each do |member_dn|
+            user = Gitlab::LDAP::User.find_by_uid_and_provider(member_dn, provider)
+
+            if user.present?
+              user.external = true
+              user.save
+              verified_external_users << user
+            else
+              logger.debug do
+                <<-MSG.strip_heredoc.tr("\n", ' ')
+                  #{self.class.name}: User with DN `#{member_dn}` should be marked as
+                  external but there is no user in GitLab with that identity.
+                  Membership will be updated once the user signs in for the first time.
+                MSG
+              end
+            end
+          end
+        end
+
+        update_external_permissions(current_external_users, verified_external_users)
+      end
+
       private
 
       # Cache LDAP group member DNs so we don't query LDAP groups more than once.
@@ -151,6 +189,10 @@ module Gitlab
         config.admin_group
       end
 
+      def external_groups
+        config.external_groups
+      end
+
       def ldap_group_member_dns(ldap_group_cn)
         ldap_group = Gitlab::LDAP::Group.find_by_cn(ldap_group_cn, adapter)
         unless ldap_group.present?
@@ -277,6 +319,16 @@ module Gitlab
         end
       end
 
+      def update_external_permissions(users, verified)
+        # Restore normal access to users no longer found in the external groups
+        users.each do |user|
+          unless verified.include?(user)
+            user.external = false
+            user.save
+          end
+        end
+      end
+
       def add_new_members(group, access_levels)
         logger.debug { "Adding new members to '#{group.name}' group" }
 

spec/lib/gitlab/ldap/group_sync_spec.rb

@@ -45,8 +45,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
     let(:group1) { create(:group) }
     let(:group2) { create(:group) }
     let(:ldap_group1) do
-      Net::LDAP::Entry.from_single_ldif_string(
-        <<-EOS.strip_heredoc
+      Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
           dn: cn=ldap_group1,ou=groups,dc=example,dc=com
           cn: ldap_group1
           description: LDAP Group 1
@@ -56,7 +55,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
           objectclass: top
           objectclass: groupOfNames
         EOS
-      )
     end
 
     context 'with all functionality against one LDAP group type' do
@@ -183,8 +181,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
             Gitlab::LDAP::GroupSync.new('ldapsecondary', adapter)
           end
           let(:ldap_secondary_group1) do
-            Net::LDAP::Entry.from_single_ldif_string(
-              <<-EOS.strip_heredoc
+            Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
                 dn: cn=ldap_secondary_group1,ou=groups,dc=example,dc=com
                 cn: ldap_secondary_group1
                 description: LDAP Group 1
@@ -194,7 +191,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
                 objectclass: top
                 objectclass: groupOfNames
               EOS
-            )
           end
           let(:user_w_multiple_ids) { create(:user) }
 
@@ -247,8 +243,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
 
       context 'when access level spillover could happen' do
         it 'does not erroneously add users' do
-          ldap_group2 = Net::LDAP::Entry.from_single_ldif_string(
-            <<-EOS.strip_heredoc
+          ldap_group2 = Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
               dn: cn=ldap_group2,ou=groups,dc=example,dc=com
               cn: ldap_group2
               description: LDAP Group 2
@@ -257,7 +252,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
               objectclass: top
               objectclass: groupOfNames
             EOS
-          )
 
           allow_any_instance_of(Gitlab::LDAP::Group)
             .to receive(:adapter).and_return(adapter)
@@ -332,8 +326,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
       context 'with groupOfNames style LDAP group' do
         let(:ldap_group) do
           Gitlab::LDAP::Group.new(
-            Net::LDAP::Entry.from_single_ldif_string(
-              <<-EOS.strip_heredoc
+            Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
                 dn: cn=ldap_group1,ou=groups,dc=example,dc=com
                 cn: ldap_group1
                 description: LDAP Group 1
@@ -341,7 +334,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
                 objectclass: top
                 objectclass: groupOfNames
               EOS
-            )
           )
         end
 
@@ -356,8 +348,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
       context 'with posixGroup style LDAP group' do
         let(:ldap_group) do
           Gitlab::LDAP::Group.new(
-            Net::LDAP::Entry.from_single_ldif_string(
-              <<-EOS.strip_heredoc
+            Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
                 dn: cn=ldap_group1,ou=groups,dc=example,dc=com
                 cn: ldap_group1
                 description: LDAP Group 1
@@ -365,7 +356,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
                 objectclass: top
                 objectclass: posixGroup
               EOS
-            )
           )
         end
         let(:ldap_user) do
@@ -453,8 +443,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
       context 'with groupOfUniqueNames style LDAP group' do
         let(:ldap_group) do
           Gitlab::LDAP::Group.new(
-            Net::LDAP::Entry.from_single_ldif_string(
-              <<-EOS.strip_heredoc
+            Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
                 dn: cn=ldap_group1,ou=groups,dc=example,dc=com
                 cn: ldap_group1
                 description: LDAP Group 1
@@ -462,7 +451,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
                 objectclass: top
                 objectclass: groupOfUniqueNames
               EOS
-            )
           )
         end
 
@@ -476,15 +464,13 @@ describe Gitlab::LDAP::GroupSync, lib: true do
       context 'with an empty LDAP group' do
         let(:ldap_group) do
           Gitlab::LDAP::Group.new(
-            Net::LDAP::Entry.from_single_ldif_string(
-              <<-EOS.strip_heredoc
+            Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
                 dn: cn=ldap_group1,ou=groups,dc=example,dc=com
                 cn: ldap_group1
                 description: LDAP Group 1
                 objectclass: top
                 objectclass: groupOfUniqueNames
               EOS
-            )
           )
         end
 
@@ -498,8 +484,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
       context 'with uid=username member format' do
         let(:ldap_group) do
           Gitlab::LDAP::Group.new(
-            Net::LDAP::Entry.from_single_ldif_string(
-              <<-EOS.strip_heredoc
+            Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
                 dn: cn=ldap_group1,ou=groups,dc=example,dc=com
                 cn: ldap_group1
                 member: uid=#{user1.username}
@@ -507,7 +492,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
                 objectclass: top
                 objectclass: groupOfUniqueNames
               EOS
-            )
           )
         end
         let(:ldap_user) do
@@ -552,8 +536,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
       context 'with invalid DNs in the LDAP group' do
         let(:ldap_group) do
           Gitlab::LDAP::Group.new(
-            Net::LDAP::Entry.from_single_ldif_string(
-              <<-EOS.strip_heredoc
+            Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
                 dn: cn=ldap_group1,ou=groups,dc=example,dc=com
                 cn: ldap_group1
                 member:
@@ -563,7 +546,6 @@ describe Gitlab::LDAP::GroupSync, lib: true do
                 objectclass: top
                 objectclass: groupOfUniqueNames
               EOS
-            )
           )
         end
 
@@ -591,8 +573,7 @@ describe Gitlab::LDAP::GroupSync, lib: true do
     let(:user3) { create(:user) }
 
     let(:admin_group) do
-      Net::LDAP::Entry.from_single_ldif_string(
-        <<-EOS.strip_heredoc
+      Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
           dn: cn=admin_group,ou=groups,dc=example,dc=com
           cn: admin_group
           description: Admin Group
@@ -601,14 +582,11 @@ describe Gitlab::LDAP::GroupSync, lib: true do
           objectclass: top
           objectclass: groupOfNames
         EOS
-      )
     end
 
     before do
-      user1.admin = true
-      user1.save
-      user3.admin = true
-      user3.save
+      user1.update_attribute(:admin, true)
+      user3.update_attribute(:admin, true)
 
       allow_any_instance_of(Gitlab::LDAP::Group)
         .to receive(:adapter).and_return(adapter)
@@ -646,4 +624,86 @@ describe Gitlab::LDAP::GroupSync, lib: true do
         .not_to change { User.admins.where(id: user3.id).any? }
     end
   end
+
+  describe '#sync_external_users' do
+    let(:user1) { create(:user) }
+    let(:user2) { create(:user) }
+    let(:user3) { create(:user) }
+    let(:user4) { create(:user) }
+
+    let(:external_group1) do
+      Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
+          dn: cn=external_group1,ou=groups,dc=example,dc=com
+          cn: external_group1
+          description: External Group 1
+          gidnumber: 42
+          uniqueMember: uid=#{user1.username},ou=users,dc=example,dc=com
+          objectclass: top
+          objectclass: groupOfNames
+      EOS
+    end
+
+    let(:external_group2) do
+      Net::LDAP::Entry.from_single_ldif_string(<<-EOS.strip_heredoc)
+        dn: cn=external_group2,ou=groups,dc=example,dc=com
+        cn: external_group2
+        description: External Group 2
+        gidnumber: 42
+        uniqueMember: uid=#{user2.username},ou=users,dc=example,dc=com
+        objectclass: top
+        objectclass: groupOfNames
+      EOS
+    end
+
+    before do
+      user3.update_attribute(:external, true)
+      user4.update_attribute(:external, true)
+
+      allow_any_instance_of(Gitlab::LDAP::Group)
+        .to receive(:adapter).and_return(adapter)
+      allow(Gitlab::LDAP::Group)
+        .to receive(:find_by_cn)
+              .with('external_group1', kind_of(Gitlab::LDAP::Adapter))
+              .and_return(Gitlab::LDAP::Group.new(external_group1))
+      allow(Gitlab::LDAP::Group)
+        .to receive(:find_by_cn)
+              .with('external_group2', kind_of(Gitlab::LDAP::Adapter))
+              .and_return(Gitlab::LDAP::Group.new(external_group2))
+
+      user1.identities.create(
+        provider: 'ldapmain',
+        extern_uid: "uid=#{user1.username},ou=users,dc=example,dc=com"
+      )
+      user2.identities.create(
+        provider: 'ldapmain',
+        extern_uid: "uid=#{user2.username},ou=users,dc=example,dc=com"
+      )
+      user4.identities.create(
+        provider: 'ldapmain',
+        extern_uid: "uid=#{user4.username},ou=users,dc=example,dc=com"
+      )
+
+      allow(group_sync).to receive_messages(external_groups: %w(external_group1 external_group2))
+    end
+
+    it 'adds user1 as external user' do
+      expect { group_sync.sync_external_users }
+        .to change { User.external.where(id: user1.id).any? }.from(false).to(true)
+    end
+
+    it 'adds user2 as external user' do
+      expect { group_sync.sync_external_users }
+        .to change { User.external.where(id: user2.id).any? }.from(false).to(true)
+    end
+
+    it 'removes users that are not in the LDAP group' do
+      expect { group_sync.sync_external_users }
+        .to change { User.external.where(id: user4.id).any? }.from(true).to(false)
+    end
+
+    it 'leaves as external users that do not have the LDAP provider' do
+      expect { group_sync.sync_external_users }
+        .not_to change { User.external.where(id: user3.id).any? }
+    end
+  end
 end