diff options
author | Douwe Maan <douwe@gitlab.com> | 2016-03-10 21:30:28 +0300 |
---|---|---|
committer | Douwe Maan <douwe@gitlab.com> | 2016-03-10 21:30:28 +0300 |
commit | 57142014e8424717cda16de9fd8c549c9b402c37 (patch) | |
tree | 1e5ed8efcb3eb2f8ceefccaf243524964bfb19b4 | |
parent | 8ecdc0a7de05d81247f2892c7d7e2850ea5f811d (diff) | |
parent | 7b2bf4ce7589f73bf98a754fecb89d4f92d410b3 (diff) |
Merge branch 'expiring-lock' into 'master'
Use an exclusive lease for LDAP checks
Fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/14134
Merge https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/3140 first.
Only perform LDAP checks for a user after acquiring a lease. Checks
during user login do not require a lease.
The lease has a fixed expire time of 10 minutes.
See merge request !3143
-rw-r--r-- | app/controllers/application_controller.rb | 2 | ||||
-rw-r--r-- | app/models/user.rb | 7 | ||||
-rw-r--r-- | lib/gitlab/exclusive_lease.rb | 41 | ||||
-rw-r--r-- | lib/gitlab/user_access.rb | 2 | ||||
-rw-r--r-- | spec/lib/gitlab/exclusive_lease_spec.rb | 21 |
5 files changed, 72 insertions, 1 deletions
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fb74919ea23..1f55b18e0b1 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -246,6 +246,8 @@ class ApplicationController < ActionController::Base def ldap_security_check if current_user && current_user.requires_ldap_check? + return unless current_user.try_obtain_ldap_lease + unless Gitlab::LDAP::Access.allowed?(current_user) sign_out current_user flash[:alert] = "Access denied for your LDAP account." 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 new file mode 100644 index 00000000000..2ef50286b1d --- /dev/null +++ b/lib/gitlab/exclusive_lease.rb @@ -0,0 +1,41 @@ +module Gitlab + # This class implements an 'exclusive lease'. We call it a 'lease' + # because it has a set expiry time. We call it 'exclusive' because only + # one caller may obtain a lease for a given key at a time. The + # implementation is intended to work across GitLab processes and across + # 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 for the same + # key at the same time. + # + class ExclusiveLease + def initialize(key, timeout:) + @key, @timeout = key, timeout + end + + # Try to obtain the lease. Return true on success, + # false if the lease is already taken. + def try_obtain + # Performing a single SET is atomic + !!redis.set(redis_key, '1', nx: true, ex: @timeout) + end + + private + + def redis + # Maybe someday we want to use a connection pool... + @redis ||= Redis.new(url: Gitlab::RedisConfig.url) + end + + def redis_key + "gitlab:exclusive_lease:#{@key}" + end + end +end diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb index 4885baf9526..d1b42c1f9b9 100644 --- a/lib/gitlab/user_access.rb +++ b/lib/gitlab/user_access.rb @@ -3,7 +3,7 @@ module Gitlab def self.allowed?(user) return false if user.blocked? - if user.requires_ldap_check? + if user.requires_ldap_check? && user.try_obtain_ldap_lease return false unless Gitlab::LDAP::Access.allowed?(user) end diff --git a/spec/lib/gitlab/exclusive_lease_spec.rb b/spec/lib/gitlab/exclusive_lease_spec.rb new file mode 100644 index 00000000000..fbdb7ea34ac --- /dev/null +++ b/spec/lib/gitlab/exclusive_lease_spec.rb @@ -0,0 +1,21 @@ +require 'spec_helper' + +describe Gitlab::ExclusiveLease do + it 'cannot obtain twice before the lease has expired' do + lease = Gitlab::ExclusiveLease.new(unique_key, timeout: 3600) + expect(lease.try_obtain).to eq(true) + expect(lease.try_obtain).to eq(false) + end + + it 'can obtain after the lease has expired' do + timeout = 1 + lease = Gitlab::ExclusiveLease.new(unique_key, timeout: timeout) + lease.try_obtain # start the lease + sleep(2 * timeout) # lease should have expired now + expect(lease.try_obtain).to eq(true) + end + + def unique_key + SecureRandom.hex(10) + end +end |