Commit 1ec106b853cf2b3aa7c8fa29e24d1f429df2f0a8
Exists in
spb-stable
and in
3 other branches
Merge branch 'ldap_connections'
Signed-off-by: Dmitriy Zaporozhets <dmitriy.zaporozhets@gmail.com> Conflicts: CHANGELOG
Showing
5 changed files
with
48 additions
and
17 deletions
Show diff stats
CHANGELOG
| @@ -18,6 +18,7 @@ v 6.7.0 | @@ -18,6 +18,7 @@ v 6.7.0 | ||
| 18 | - Add webhook when a new tag is pushed (Jeroen van Baarsen) | 18 | - Add webhook when a new tag is pushed (Jeroen van Baarsen) |
| 19 | - Add button for toggling inline comments in diff view | 19 | - Add button for toggling inline comments in diff view |
| 20 | - Add retry feature for repository import | 20 | - Add retry feature for repository import |
| 21 | + - Reuse the GitLab LDAP connection within each request | ||
| 21 | 22 | ||
| 22 | v 6.6.2 | 23 | v 6.6.2 |
| 23 | - Fix 500 error on branch/tag create or remove via UI | 24 | - Fix 500 error on branch/tag create or remove via UI |
app/controllers/application_controller.rb
| @@ -182,13 +182,15 @@ class ApplicationController < ActionController::Base | @@ -182,13 +182,15 @@ class ApplicationController < ActionController::Base | ||
| 182 | 182 | ||
| 183 | def ldap_security_check | 183 | def ldap_security_check |
| 184 | if current_user && current_user.requires_ldap_check? | 184 | if current_user && current_user.requires_ldap_check? |
| 185 | - if gitlab_ldap_access.allowed?(current_user) | ||
| 186 | - current_user.last_credential_check_at = Time.now | ||
| 187 | - current_user.save | ||
| 188 | - else | ||
| 189 | - sign_out current_user | ||
| 190 | - flash[:alert] = "Access denied for your LDAP account." | ||
| 191 | - redirect_to new_user_session_path | 185 | + gitlab_ldap_access do |access| |
| 186 | + if access.allowed?(current_user) | ||
| 187 | + current_user.last_credential_check_at = Time.now | ||
| 188 | + current_user.save | ||
| 189 | + else | ||
| 190 | + sign_out current_user | ||
| 191 | + flash[:alert] = "Access denied for your LDAP account." | ||
| 192 | + redirect_to new_user_session_path | ||
| 193 | + end | ||
| 192 | end | 194 | end |
| 193 | end | 195 | end |
| 194 | end | 196 | end |
| @@ -198,8 +200,8 @@ class ApplicationController < ActionController::Base | @@ -198,8 +200,8 @@ class ApplicationController < ActionController::Base | ||
| 198 | @event_filter ||= EventFilter.new(filters) | 200 | @event_filter ||= EventFilter.new(filters) |
| 199 | end | 201 | end |
| 200 | 202 | ||
| 201 | - def gitlab_ldap_access | ||
| 202 | - Gitlab::LDAP::Access.new | 203 | + def gitlab_ldap_access(&block) |
| 204 | + Gitlab::LDAP::Access.open { |access| block.call(access) } | ||
| 203 | end | 205 | end |
| 204 | 206 | ||
| 205 | # JSON for infinite scroll via Pager object | 207 | # JSON for infinite scroll via Pager object |
lib/gitlab/ldap/access.rb
| 1 | module Gitlab | 1 | module Gitlab |
| 2 | module LDAP | 2 | module LDAP |
| 3 | class Access | 3 | class Access |
| 4 | + attr_reader :adapter | ||
| 5 | + | ||
| 6 | + def self.open(&block) | ||
| 7 | + Gitlab::LDAP::Adapter.open do |adapter| | ||
| 8 | + block.call(self.new(adapter)) | ||
| 9 | + end | ||
| 10 | + end | ||
| 11 | + | ||
| 12 | + def initialize(adapter=nil) | ||
| 13 | + @adapter = adapter | ||
| 14 | + end | ||
| 15 | + | ||
| 4 | def allowed?(user) | 16 | def allowed?(user) |
| 5 | - !!Gitlab::LDAP::Person.find_by_dn(user.extern_uid) | 17 | + !!Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter) |
| 6 | rescue | 18 | rescue |
| 7 | false | 19 | false |
| 8 | end | 20 | end |
lib/gitlab/ldap/adapter.rb
| @@ -3,7 +3,17 @@ module Gitlab | @@ -3,7 +3,17 @@ module Gitlab | ||
| 3 | class Adapter | 3 | class Adapter |
| 4 | attr_reader :ldap | 4 | attr_reader :ldap |
| 5 | 5 | ||
| 6 | - def initialize | 6 | + def self.open(&block) |
| 7 | + Net::LDAP.open(adapter_options) do |ldap| | ||
| 8 | + block.call(self.new(ldap)) | ||
| 9 | + end | ||
| 10 | + end | ||
| 11 | + | ||
| 12 | + def self.config | ||
| 13 | + Gitlab.config.ldap | ||
| 14 | + end | ||
| 15 | + | ||
| 16 | + def self.adapter_options | ||
| 7 | encryption = config['method'].to_s == 'ssl' ? :simple_tls : nil | 17 | encryption = config['method'].to_s == 'ssl' ? :simple_tls : nil |
| 8 | 18 | ||
| 9 | options = { | 19 | options = { |
| @@ -23,8 +33,12 @@ module Gitlab | @@ -23,8 +33,12 @@ module Gitlab | ||
| 23 | if config['password'] || config['bind_dn'] | 33 | if config['password'] || config['bind_dn'] |
| 24 | options.merge!(auth_options) | 34 | options.merge!(auth_options) |
| 25 | end | 35 | end |
| 36 | + options | ||
| 37 | + end | ||
| 38 | + | ||
| 26 | 39 | ||
| 27 | - @ldap = Net::LDAP.new(options) | 40 | + def initialize(ldap=nil) |
| 41 | + @ldap = ldap || Net::LDAP.new(self.class.adapter_options) | ||
| 28 | end | 42 | end |
| 29 | 43 | ||
| 30 | def users(field, value) | 44 | def users(field, value) |
| @@ -65,7 +79,7 @@ module Gitlab | @@ -65,7 +79,7 @@ module Gitlab | ||
| 65 | private | 79 | private |
| 66 | 80 | ||
| 67 | def config | 81 | def config |
| 68 | - @config ||= Gitlab.config.ldap | 82 | + @config ||= self.class.config |
| 69 | end | 83 | end |
| 70 | end | 84 | end |
| 71 | end | 85 | end |
lib/gitlab/ldap/person.rb
| 1 | module Gitlab | 1 | module Gitlab |
| 2 | module LDAP | 2 | module LDAP |
| 3 | class Person | 3 | class Person |
| 4 | - def self.find_by_uid(uid) | ||
| 5 | - Gitlab::LDAP::Adapter.new.user(config.uid, uid) | 4 | + def self.find_by_uid(uid, adapter=nil) |
| 5 | + adapter ||= Gitlab::LDAP::Adapter.new | ||
| 6 | + adapter.user(config.uid, uid) | ||
| 6 | end | 7 | end |
| 7 | 8 | ||
| 8 | - def self.find_by_dn(dn) | ||
| 9 | - Gitlab::LDAP::Adapter.new.user('dn', dn) | 9 | + def self.find_by_dn(dn, adapter=nil) |
| 10 | + adapter ||= Gitlab::LDAP::Adapter.new | ||
| 11 | + adapter.user('dn', dn) | ||
| 10 | end | 12 | end |
| 11 | 13 | ||
| 12 | def initialize(entry) | 14 | def initialize(entry) |