From 1f22baeeaf1f135b4b759dc0c9c23607cd56e2b8 Mon Sep 17 00:00:00 2001 From: Drew Blessing Date: Fri, 9 Sep 2016 10:20:33 -0500 Subject: [PATCH] Allow membership override when using LDAP group sync --- app/models/group.rb | 5 ++-- app/models/member.rb | 3 +- app/models/members/group_member.rb | 1 - ...906143504_add_ldap_attributes_to_member.rb | 19 ++++++++++++ db/schema.rb | 4 ++- lib/ee/gitlab/ldap/sync/group.rb | 17 +++++++---- spec/lib/ee/gitlab/ldap/sync/group_spec.rb | 29 ++++++++++++++++++- 7 files changed, 67 insertions(+), 11 deletions(-) create mode 100644 db/migrate/20160906143504_add_ldap_attributes_to_member.rb diff --git a/app/models/group.rb b/app/models/group.rb index 937cc60be2f20b..7493421bab7121 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -107,7 +107,7 @@ def avatar_url(size = nil) 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 @@ def add_users(user_ids, access_level, current_user: nil, skip_notification: fals access_level, current_user: current_user, skip_notification: skip_notification, - expires_at: expires_at + expires_at: expires_at, + ldap: ldap ) end end diff --git a/app/models/member.rb b/app/models/member.rb index 403818a44ea7f2..eff724b33609b6 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -75,7 +75,7 @@ def user_for_id(user_id) 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 @@ def add_user(members, user_id, access_level, current_user: nil, skip_notificatio member.access_level = access_level member.expires_at = expires_at member.skip_notification = skip_notification + member.ldap = ldap member.save end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 21c0cf370624aa..57d7f3d8bab92c 100644 --- a/app/models/members/group_member.rb +++ b/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 diff --git a/db/migrate/20160906143504_add_ldap_attributes_to_member.rb b/db/migrate/20160906143504_add_ldap_attributes_to_member.rb new file mode 100644 index 00000000000000..111903f3e91729 --- /dev/null +++ b/db/migrate/20160906143504_add_ldap_attributes_to_member.rb @@ -0,0 +1,19 @@ +# 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 diff --git a/db/schema.rb b/db/schema.rb index 3d139fc3dcf898..97c77577c83b65 100644 --- a/db/schema.rb +++ b/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 @@ 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 diff --git a/lib/ee/gitlab/ldap/sync/group.rb b/lib/ee/gitlab/ldap/sync/group.rb index 403e575eb95203..c7acd2ce8a6ff2 100644 --- a/lib/ee/gitlab/ldap/sync/group.rb +++ b/lib/ee/gitlab/ldap/sync/group.rb @@ -118,6 +118,11 @@ def update_existing_group_membership(group, access_levels) 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 @@ def update_existing_group_membership(group, access_levels) # 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 @@ def add_or_update_user_membership(user, group, access) 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 @@ def warn_cannot_remove_last_owner(user, group) 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 diff --git a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb index a7393555ec020d..6089ec47068d30 100644 --- a/spec/lib/ee/gitlab/ldap/sync/group_spec.rb +++ b/spec/lib/ee/gitlab/ldap/sync/group_spec.rb @@ -115,10 +115,11 @@ def execute 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 @@ -158,6 +159,32 @@ def execute 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 -- GitLab