Merge branch 'fix_ldap_user_removed' into 'master'

Remove user from groups when they are removed from LDAP

Fixes #159 

Currently, we only update user permissions if they come back from LDAP
as a valid user. I refactored some things in the `access.rb` file so we
update permissions no matter what. Now the user is removed from groups
and admin access is revoked, in addition to the previous behavior of
blocking the user.

See merge request !124

CHANGELOG

@@ -12,6 +12,7 @@ v 8.4.0 (unreleased)
   - Add user's last used IP addresses to admin page (Stan Hu)
   - Add housekeeping function to project settings page
   - The default GitLab logo now acts as a loading indicator
+  - LDAP group sync: Remove user from group when they are removed from LDAP
   - Fix caching issue where build status was not updating in project dashboard (Stan Hu)
   - Accept 2xx status codes for successful Web hook triggers (Stan Hu)
   - Fix missing date of month in network graph when commits span a month (Stan Hu)

doc/workflow/groups.md

@@ -70,28 +70,28 @@ gitlab_rails['gitlab_default_can_create_group'] = false
 # line in /home/git/gitlab/config/gitlab.yml
 ```
 
-## Lock project membership to members of the group
+## Lock project membership to members of this group
 
-In GitLab Enterprise Edition it is possible to lock membership in project to the level of members in group.
+In GitLab Enterprise Edition it is possible to lock membership in project to the
+level of members in group.
 
-This allows group owner to lock down any new project membership to any of the projects within the group allowing tighter control over project membership.
+This allows group owner to lock down any new project membership to any of the
+projects within the group allowing tighter control over project membership.
 
-To enable this feature, navigate to group settings page, select `Member lock` and `Save group`.
+To enable this feature, navigate to group settings page, select `Member lock`
+and `Save group`.
 
 ![Checkbox for membership lock](groups/membership_lock.png)
 
-This will disable the option for all users who previously had permissions to operate project memberships so no new users can be added. Furthermore, any request to add new user to project through API will not be possible.
+This will disable the option for all users who previously had permissions to
+operate project memberships so no new users can be added. Furthermore, any
+request to add new user to project through API will not be possible.
 
-## Namespaces in groups
+## Prevent projects in this group from sharing a project with another group
 
-By default, groups only get 20 namespaces at a time because the API results are paginated.
+In GitLab Enterprise it is possible to prevent projects in a group from sharing
+a project with another group. This allows for tighter control over project
+access.
 
-To get more (up to 100), pass the following as an argument to the API call:
-```
-/groups?per_page=100
-```
-
-And to switch pages add: 
-```
-/groups?per_page=100&page=2
-```
+To enable this feature, navigate to the group settings page. Select `Share with
+group lock` and save the group.

lib/gitlab/ldap/access.rb

@@ -16,9 +16,11 @@ module Gitlab
 
       def self.allowed?(user)
         self.open(user) do |access|
+          # Whether user is allowed, or not, we should update
+          # permissions to keep things clean
+          access.update_permissions
           if access.allowed?
-            access.update_permissions
-            access.update_email
+            access.update_user
             user.last_credential_check_at = Time.now
             user.save
             true
@@ -67,22 +69,15 @@ module Gitlab
         @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
       end
 
-      def update_permissions
-        if sync_ssh_keys?
-          update_ssh_keys
-        end
-
+      def update_user
+        update_email
+        update_ssh_keys if sync_ssh_keys?
         update_kerberos_identity if import_kerberos_identities?
+      end
 
-        # Skip updating group permissions
-        # if instance does not use group_base setting
-        return true unless group_base.present?
-
-        update_ldap_group_links
-
-        if admin_group.present?
-          update_admin_status
-        end
+      def update_permissions
+        update_ldap_group_links if group_base.present?
+        update_admin_status if admin_group.present?
       end
 
       # Update user ssh keys if they changed in LDAP
@@ -191,6 +186,7 @@ module Gitlab
 
       # returns a collection of cn strings to which the user has access
       def cns_with_access
+        return [] unless ldap_user.present?
         @ldap_groups_with_access ||= ldap_groups.select do |ldap_group|
           ldap_group.has_member?(ldap_user)
         end.map(&:cn)

spec/lib/gitlab/ldap/access_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::LDAP::Access, lib: true do
   let(:access) { Gitlab::LDAP::Access.new user }
   let(:user) { create(:omniauth_user) }
 
-  describe :allowed? do
+  describe '#allowed?' do
     subject { access.allowed? }
 
     context 'when the user cannot be found' do
@@ -40,7 +40,7 @@ describe Gitlab::LDAP::Access, lib: true do
         end
       end
 
-      context 'and has no disabled flag in active diretory' do
+      context 'and has no disabled flag in active directory' do
         before do
           allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false)
         end
@@ -71,7 +71,7 @@ describe Gitlab::LDAP::Access, lib: true do
         end
       end
 
-      context 'withoud ActiveDirectory enabled' do
+      context 'without ActiveDirectory enabled' do
         before do
           allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
           allow_any_instance_of(Gitlab::LDAP::Config).to receive(:active_directory).and_return(false)
@@ -82,41 +82,67 @@ describe Gitlab::LDAP::Access, lib: true do
     end
   end
 
-  describe :update_permissions do
-    subject { access.update_permissions }
+  describe '#update_user' do
+    subject { access.update_user }
+    let(:entry) do
+      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com")
+    end
+    before do
+      allow(access).to(
+        receive_messages(
+          ldap_user: Gitlab::LDAP::Person.new(entry, user.ldap_identity.provider)
+        )
+      )
+    end
+
+    it 'updates email address' do
+      expect(access).to receive(:update_email).once
+
+      subject
+    end
 
-    it "syncs ssh keys if enabled by configuration" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: 'sshpublickey', import_kerberos_identities?: false)
+    it 'syncs ssh keys if enabled by configuration' do
+      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: true)
       expect(access).to receive(:update_ssh_keys).once
 
       subject
     end
 
-    it "does update group permissions with a group base configured" do
-      allow(access).to receive_messages(group_base: 'my-group-base', sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'update_kerberos_identity' do
+      allow(access).to receive_messages(import_kerberos_identities?: true)
+      expect(access).to receive(:update_kerberos_identity).once
+
+      subject
+    end
+  end
+
+  describe '#update_permissions' do
+    subject { access.update_permissions }
+
+    it 'does update group permissions with a group base configured' do
+      allow(access).to receive_messages(group_base: 'my-group-base')
       expect(access).to receive(:update_ldap_group_links)
 
       subject
     end
 
-    it "does not update group permissions without a group base configured" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'does not update group permissions without a group base configured' do
+      allow(access).to receive_messages(group_base: '')
       expect(access).not_to receive(:update_ldap_group_links)
 
       subject
     end
 
-    it "does update admin group permissions if admin group is configured" do
-      allow(access).to receive_messages(admin_group: 'my-admin-group', update_ldap_group_links: nil, sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'does update admin group permissions if admin group is configured' do
+      allow(access).to receive_messages(admin_group: 'my-admin-group')
       expect(access).to receive(:update_admin_status)
 
       subject
     end
 
-    it "does update Kerberos identities if Kerberos is enabled and the LDAP server is Active Directory" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: true)
-
-      expect(access).to receive(:update_kerberos_identity)
+    it 'does not update admin status when admin group is not configured' do
+      allow(access).to receive_messages(admin_group: '')
+      expect(access).not_to receive(:update_admin_status)
 
       subject
     end
@@ -451,17 +477,33 @@ objectclass: posixGroup
       ]
     end
 
-    let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, user.ldap_identity.provider) }
-
     before do
-      allow(access).to receive_messages(ldap_user: ldap_user)
-      allow(ldap_user).to receive(:uid) { 'user1' }
       allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
+      allow(access).to receive_messages(ldap_groups: ldap_groups)
     end
 
-    it "only returns ldap cns to which the user has access" do
-      allow(access).to receive_messages(ldap_groups: ldap_groups)
-      expect(access.cns_with_access).to eql ['group1']
+    context 'when the LDAP user exists' do
+      let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, user.ldap_identity.provider) }
+
+      before do
+        allow(access).to receive_messages(ldap_user: ldap_user)
+        allow(ldap_user).to receive(:uid) { 'user1' }
+      end
+
+      it 'only returns ldap cns to which the user has access' do
+        expect(access.cns_with_access).to eq(['group1'])
+      end
+    end
+
+    context 'when the LADP user does not exist' do
+      let(:ldap_user) { nil }
+      before do
+        allow(access).to receive_messages(ldap_user: ldap_user)
+      end
+
+      it 'returns an empty array' do
+        expect(access.cns_with_access).to eq([])
+      end
     end
   end
 end