Merge branch 'fix_ldap_group_sync' into 'master'

Downgrade group member role if LDAP dictates

Fixes #170 

If a group link was updated to set a lower maximum role/access level, 
a user's role was not downgraded. Similarly, if the user was moved to
another group where their role should be lowered, their access was not
downgraded. For some reason we were preferring the higher of LDAP or
GitLab access even if LDAP said it should be lower. After this change,
what LDAP says, wins. 

If a user is a member of multiple LDAP groups that are linked with the
same GitLab group, they still receive the highest of all roles.

See merge request !123

CHANGELOG

@@ -27,6 +27,7 @@ v 8.4.0 (unreleased)
   - Add API support for looking up a user by username (Stan Hu)
   - Add project permissions to all project API endpoints (Stan Hu)
   - Link to milestone in "Milestone changed" system note
+  - LDAP Group Sync: Allow group role downgradegit
   - Only allow group/project members to mention `@all`
   - Expose Git's version in the admin area (Trey Davis)
   - Add "Frequently used" category to emoji picker

doc/integration/ldap.md

@@ -156,18 +156,22 @@ If multiple LDAP email attributes are present, e.g. `mail: foo@bar.com` and `ema
 
 ## LDAP group synchronization (GitLab Enterprise Edition)
 
-LDAP group synchronization in GitLab Enterprise Edition allows you to synchronize the members of a GitLab group with one or more LDAP groups.
+LDAP group synchronization in GitLab Enterprise Edition allows you to
+synchronize the members of a GitLab group with one or more LDAP groups.
 
 ### Setting up LDAP group synchronization
 
-Before enabling group synchronization, you need to make sure that the `group_base` field is set in your LDAP settings on
-your `gitlab.rb` or `gitlab.yml` file. This setting will tell GitLab where to look for groups within your LDAP server.
+Before enabling group synchronization, you need to make sure that the
+`group_base` field is set in your LDAP settings on your `gitlab.rb` or
+`gitlab.yml` file. This setting will tell GitLab where to look for groups
+within your LDAP server.
 
 ```
 group_base: 'OU=groups,DC=example,DC=com'
 ```
 
-Suppose we want to synchronize the GitLab group 'example group' with the LDAP group 'Engineering'.
+Suppose we want to synchronize the GitLab group 'example group' with the LDAP
+group 'Engineering'.
 
 1. As an owner, go to the group settings page for 'example group'.
 
@@ -179,18 +183,25 @@ As an admin you can also go to the group edit page in the admin area.
 
 2. Enter 'Engineering' as the LDAP Common Name (CN) in the 'LDAP Group cn' field.
 
-3. Enter a default group access level in the 'LDAP Access' field; let's say Developer.
+3. Enter a default group access level in the 'LDAP Access' field; let's say
+Developer.
 
 ![LDAP group settings filled in](ldap/select_group_cn_engineering.png)
 
 4. Click 'Add synchronization' to add the new LDAP group link.
 
-Now every time a member of the 'Engineering' LDAP group signs in, they automatically become a Developer-level member of the 'example group' GitLab group. Users who are already signed in will see the change in membership after up to one hour.
+Now every time a member of the 'Engineering' LDAP group signs in, they
+automatically become a Developer-level member of the 'example group' GitLab
+group. Users who are already signed in will see the change in membership after
+up to one hour.
 
 ### Synchronizing with more than one LDAP group (GitLab EE 7.3 and newer)
 
-If you want to add the members of LDAP group to your GitLab group you can add an additional LDAP group link.
-If you have two LDAP group links, e.g. 'cn=Engineering' at level 'Developer' and 'cn=QA' at level 'Reporter', and user Jane belongs to both the 'Engineering' and 'QA' LDAP groups, she will get the _highest_ access level of the two, namely 'Developer'.
+If you have two LDAP group links, and a user belongs to both LDAP groups, they
+will receive the _highest_ access level of the two. For example, suppose you
+have configured group sync for the 'Engineering' group at level 'Developer' and
+'QA' group at level 'Reporter'. User 'Jane' belongs to both the 'Engineering' and
+'QA' LDAP groups so she will receive the 'Developer' role.
 
 ![Two linked LDAP groups](ldap/two_linked_ldap_groups.png)
 

lib/gitlab/ldap/access.rb

@@ -164,7 +164,7 @@ module Gitlab
         end
       end
 
-      # Loop throug all ldap conneted groups, and update the users link with it
+      # Loop through all ldap connected groups, and update the users link with it
       #
       # We documented what sort of queries an LDAP server can expect from
       # GitLab EE in doc/integration/ldap.md. Please remember to update that
@@ -174,7 +174,7 @@ module Gitlab
           active_group_links = group.ldap_group_links.where(cn: cns_with_access)
 
           if active_group_links.any?
-            group.add_users([user.id], fetch_group_access(group, user, active_group_links), skip_notification: true)
+            group.add_users([user.id], active_group_links.maximum(:group_access), skip_notification: true)
           elsif group.last_owner?(user)
             logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
           else
@@ -221,16 +221,6 @@ module Gitlab
           where(ldap_group_links: { provider: provider })
       end
 
-      # Get the group_access for a give user.
-      # Always respect the current level, never downgrade it.
-      def fetch_group_access(group, user, active_group_links)
-        current_access_level = group.group_members.where(user_id: user).maximum(:access_level)
-        max_group_access_level = active_group_links.maximum(:group_access)
-
-        # TODO: Test if nil value of current_access_level in handled properly
-        [current_access_level, max_group_access_level].compact.max
-      end
-
       def logger
         Rails.logger
       end

spec/lib/gitlab/ldap/access_spec.rb

@@ -328,19 +328,19 @@ objectclass: posixGroup
       end
     end
 
-    context "existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER" do
+    context 'existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER' do
       before do
         gitlab_group_1.group_members.masters.create(user_id: user.id)
         gitlab_group_1.ldap_group_links.create({
           cn: 'ldap-group1', group_access: Gitlab::Access::DEVELOPER, provider: 'ldapmain' })
       end
 
-      it "keeps the users master access for group 1" do
-        expect { access.update_ldap_group_links }.not_to \
-          change{ gitlab_group_1.has_master?(user) }
+      it 'downgrades the users access' do
+        expect { access.update_ldap_group_links }.to \
+          change{ gitlab_group_1.has_master?(user) }.from(true).to(false)
       end
 
-      it "doesn't send a notification email" do
+      it 'does not send a notification email' do
         expect { access.update_ldap_group_links }.not_to \
           change { ActionMailer::Base.deliveries }
       end