Move method to User

5 files changed, 22 insertions, 15 deletions

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 15fee9948ec..1f55b18e0b1 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 3098d49d58a..505a547d8ec 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 f801e8b60b3..0ed4c357926 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 90d5996757f..da4435c7308 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 46ac5825fd1..d1b42c1f9b9 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