diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 15fee9948ec1515813e8717abbb177c8007cb459..1f55b18e0b160b8c6c11f5eaa2202f9b03d20d79 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -246,7 +246,7 @@ class ApplicationController < ActionController::Base def ldap_security_check if current_user && current_user.requires_ldap_check? - return unless Gitlab::LDAP::Access.try_lock_user(current_user) + return unless current_user.try_obtain_ldap_lease unless Gitlab::LDAP::Access.allowed?(current_user) sign_out current_user diff --git a/app/models/user.rb b/app/models/user.rb index 3098d49d58a033825ff76ce992b984c43c69b3e4..505a547d8ec6a91848aae989c17d5f99112b416a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -612,6 +612,13 @@ class User < ActiveRecord::Base end end + def try_obtain_ldap_lease + # After obtaining this lease LDAP checks will be blocked for 600 seconds + # (10 minutes) for this user. + lease = Gitlab::ExclusiveLease.new("user_ldap_check:#{id}", timeout: 600) + lease.try_obtain + end + def solo_owned_groups @solo_owned_groups ||= owned_groups.select do |group| group.owners == [self] diff --git a/lib/gitlab/exclusive_lease.rb b/lib/gitlab/exclusive_lease.rb index f801e8b60b3835cc3d4fb386382b141792bdb06b..0ed4c35792616062ea77af594115ae651820d683 100644 --- a/lib/gitlab/exclusive_lease.rb +++ b/lib/gitlab/exclusive_lease.rb @@ -8,14 +8,25 @@ module Gitlab # servers. It is a 'cheap' alternative to using SQL queries and updates: # you do not need to change the SQL schema to start using # ExclusiveLease. + # + # It is important to choose the timeout wisely. If the timeout is very + # high (1 hour) then the throughput of your operation gets very low (at + # most once an hour). If the timeout is lower than how long your + # operation may take then you cannot count on exclusivity. For example, + # if the timeout is 10 seconds and you do an operation which may take 20 + # seconds then two overlapping operations may hold a lease at the + # same time. + # class ExclusiveLease - def initialize(key, timeout) + def initialize(key, timeout:) @key, @timeout = key, timeout end # Try to obtain the lease. Return true on succes, # false if the lease is already taken. def try_obtain + # This is expected to be atomic because we are talking to a + # single-threaded Redis server. !!redis.set(redis_key, redis_value, nx: true, ex: @timeout) end diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb index 90d5996757f6c9efdfd7725116dafc94b5f3e8e4..da4435c7308d5a761aac91697bad5a65d8699b3b 100644 --- a/lib/gitlab/ldap/access.rb +++ b/lib/gitlab/ldap/access.rb @@ -7,16 +7,6 @@ module Gitlab class Access attr_reader :provider, :user - # This timeout acts as a throttle on LDAP user checks. Its value of 600 - # seconds (10 minutes) means that after calling try_lock_user for user - # janedoe, no new LDAP checks can start for that user for the next 10 - # minutes. - LEASE_TIMEOUT = 600 - - def self.try_lock_user(user) - Gitlab::ExclusiveLease.new("user_ldap_check:#{user.id}", LEASE_TIMEOUT).try_obtain - end - def self.open(user, &block) Gitlab::LDAP::Adapter.open(user.ldap_identity.provider) do |adapter| block.call(self.new(user, adapter)) diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 46ac5825fd1ad7d550a26100aed3a9726b2c3bd0..d1b42c1f9b97a0c45bd0bfde190b129e6ec1aa9f 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -3,9 +3,8 @@ module Gitlab def self.allowed?(user) return false if user.blocked? - if user.requires_ldap_check? && Gitlab::LDAP::Access.try_lock_user(user) - return Gitlab::LDAP::Access.allowed?(user) - end + if user.requires_ldap_check? && user.try_obtain_ldap_lease + return false unless Gitlab::LDAP::Access.allowed?(user) end true