Merge branch 'ldap_override_membership' into 'master'

Allow membership override when using LDAP group sync

Backend part of #343 

Adds an `ldap` and `override` attribute to `Member` so we can track which members are managed by LDAP and if/when they're overridden.

- `Member#ldap?` will tell you whether it's an LDAP member 
- `Member#override` should be set to `true` if someone adds an override. Sync will then ignore it, until `Member#override` is set back to `false`.

## Performance testing

All tests resulted in approximately 119,000 total members being created. 


-  ✅ 19 minutes - An 'update' sync (where all members already existed) updating the `ldap` flag once.
- ✅ 79 minutes - A full, fresh sync before changes
- ✅ 76 minutes - A full, fresh sync after changes

Although the before time was actually higher than the after time, I think there's a standard deviation in there that accounts for it. The important thing is that it's not drastically different. 

See merge request !717

app/models/group.rb

@@ -107,7 +107,7 @@ class Group < Namespace
     end
   end
 
-  def add_users(user_ids, access_level, current_user: nil, skip_notification: false, expires_at: nil)
+  def add_users(user_ids, access_level, current_user: nil, skip_notification: false, expires_at: nil, ldap: false)
     user_ids.each do |user_id|
       Member.add_user(
         self.group_members,
@@ -115,7 +115,8 @@ class Group < Namespace
         access_level,
         current_user: current_user,
         skip_notification: skip_notification,
-        expires_at: expires_at
+        expires_at: expires_at,
+        ldap: ldap
       )
     end
   end

app/models/member.rb

@@ -75,7 +75,7 @@ class Member < ActiveRecord::Base
       user
     end
 
-    def add_user(members, user_id, access_level, current_user: nil, skip_notification: false, expires_at: nil)
+    def add_user(members, user_id, access_level, current_user: nil, skip_notification: false, expires_at: nil, ldap: false)
       user = user_for_id(user_id)
 
       # `user` can be either a User object or an email to be invited
@@ -91,6 +91,7 @@ class Member < ActiveRecord::Base
         member.access_level = access_level
         member.expires_at = expires_at
         member.skip_notification = skip_notification
+        member.ldap = ldap
 
         member.save
       end

app/models/members/group_member.rb

@@ -9,7 +9,6 @@ class GroupMember < Member
   default_scope { where(source_type: SOURCE_TYPE) }
 
   scope :with_ldap_dn, -> { joins(user: :identities).where("identities.provider LIKE ?", 'ldap%') }
-  scope :select_access_level_and_user, -> { select(:access_level, :user_id) }
   scope :with_identity_provider, ->(provider) do
     joins(user: :identities).where(identities: { provider: provider })
   end

db/migrate/20160906143504_add_ldap_attributes_to_member.rb

+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLdapAttributesToMember < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
+    add_column_with_default :members, :ldap, :boolean, default: false, allow_null: false
+    add_column_with_default :members, :override, :boolean, default: false, allow_null: false
+  end
+
+  def down
+    remove_column :members, :ldap
+    remove_column :members, :override
+  end
+end

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: 20160902122721) do
+ActiveRecord::Schema.define(version: 20160906143504) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -630,6 +630,8 @@ ActiveRecord::Schema.define(version: 20160902122721) do
     t.datetime "invite_accepted_at"
     t.datetime "requested_at"
     t.date     "expires_at"
+    t.boolean  "ldap",               default: false, null: false
+    t.boolean  "override",           default: false, null: false
   end
 
   add_index "members", ["access_level"], name: "index_members_on_access_level", using: :btree

lib/ee/gitlab/ldap/sync/group.rb

@@ -118,6 +118,11 @@ module EE
 
               desired_access = access_levels[member_dn]
 
+              # Skip validations and callbacks. We have a limited set of attrs
+              # due to the `select` lookup, and we need to be efficient.
+              # Low risk, because the member should already be valid.
+              member.update_column(:ldap, true) unless member.ldap?
+
               # Don't do anything if the user already has the desired access level
               if member.access_level == desired_access
                 access_levels.delete(member_dn)
@@ -127,10 +132,12 @@ module EE
               # Check and update the access level. If `desired_access` is `nil`
               # we need to delete the user from the group.
               if desired_access.present?
-                add_or_update_user_membership(user, group, desired_access)
-
-                # Delete this entry from the hash now that we've acted on it
+                # Delete this entry from the hash now that we're acting on it
                 access_levels.delete(member_dn)
+
+                next if member.ldap? && member.override?
+
+                add_or_update_user_membership(user, group, desired_access)
               elsif group.last_owner?(user)
                 warn_cannot_remove_last_owner(user, group)
               else
@@ -175,7 +182,7 @@ module EE
               else
                 # If you pass the user object, instead of just user ID,
                 # it saves an extra user database query.
-                group.add_users([user], access, skip_notification: true)
+                group.add_users([user], access, skip_notification: true, ldap: true)
               end
             end
           end
@@ -191,7 +198,7 @@ module EE
           end
 
           def select_and_preload_group_members(group)
-            group.members.select_access_level_and_user
+            group.members.select(:id, :access_level, :user_id, :ldap, :override)
               .with_identity_provider(provider).preload(:user)
           end
 

spec/lib/ee/gitlab/ldap/sync/group_spec.rb

@@ -134,10 +134,11 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
           sync_group.update_permissions
         end
 
-        it 'adds new members' do
+        it 'adds new members and sets ldap attribute to true' do
           sync_group.update_permissions
 
           expect(group.members.pluck(:user_id)).to include(user.id)
+          expect(group.members.find_by(user_id: user.id).ldap?).to be_truthy
         end
 
         it 'converts an existing membership access request to a real member' do
@@ -177,6 +178,32 @@ describe EE::Gitlab::LDAP::Sync::Group, lib: true do
           expect(group.members.find_by(user_id: user.id).access_level)
             .to eq(::Gitlab::Access::DEVELOPER)
         end
+
+        it 'sets an existing member ldap attribute to true' do
+          group.add_users(
+            [user],
+            ::Gitlab::Access::DEVELOPER,
+            skip_notification: true
+          )
+
+          sync_group.update_permissions
+
+          expect(group.members.find_by(user_id: user.id).ldap?).to be_truthy
+        end
+
+        it 'does not alter an ldap member that has a permission override' do
+          group.members.create(
+            user: user,
+            access_level: ::Gitlab::Access::MASTER,
+            ldap: true,
+            override: true
+          )
+
+          sync_group.update_permissions
+
+          expect(group.members.find_by(user_id: user.id).access_level)
+            .to eq(::Gitlab::Access::MASTER)
+        end
       end
 
       context 'when existing user is no longer in LDAP group' do