Commit e9ea5208 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'fj-174-better-ldap-connection-handling' into 'master'

Add better LDAP connection handling

See merge request gitlab-org/gitlab-ce!18039
parents 5d16fc67 ae84eaeb
...@@ -229,10 +229,6 @@ class ApplicationController < ActionController::Base ...@@ -229,10 +229,6 @@ class ApplicationController < ActionController::Base
@event_filter ||= EventFilter.new(filters) @event_filter ||= EventFilter.new(filters)
end end
def gitlab_ldap_access(&block)
Gitlab::Auth::LDAP::Access.open { |access| yield(access) }
end
# JSON for infinite scroll via Pager object # JSON for infinite scroll via Pager object
def pager_json(partial, count, locals = {}) def pager_json(partial, count, locals = {})
html = render_to_string( html = render_to_string(
......
---
title: Add better LDAP connection handling
merge_request: 18039
author:
type: fixed
...@@ -52,6 +52,8 @@ module Gitlab ...@@ -52,6 +52,8 @@ module Gitlab
block_user(user, 'does not exist anymore') block_user(user, 'does not exist anymore')
false false
end end
rescue LDAPConnectionError
false
end end
def adapter def adapter
......
...@@ -2,6 +2,9 @@ module Gitlab ...@@ -2,6 +2,9 @@ module Gitlab
module Auth module Auth
module LDAP module LDAP
class Adapter class Adapter
SEARCH_RETRY_FACTOR = [1, 1, 2, 3].freeze
MAX_SEARCH_RETRIES = Rails.env.test? ? 1 : SEARCH_RETRY_FACTOR.size.freeze
attr_reader :provider, :ldap attr_reader :provider, :ldap
def self.open(provider, &block) def self.open(provider, &block)
...@@ -16,7 +19,7 @@ module Gitlab ...@@ -16,7 +19,7 @@ module Gitlab
def initialize(provider, ldap = nil) def initialize(provider, ldap = nil)
@provider = provider @provider = provider
@ldap = ldap || Net::LDAP.new(config.adapter_options) @ldap = ldap || renew_connection_adapter
end end
def config def config
...@@ -47,8 +50,10 @@ module Gitlab ...@@ -47,8 +50,10 @@ module Gitlab
end end
def ldap_search(*args) def ldap_search(*args)
retries ||= 0
# Net::LDAP's `time` argument doesn't work. Use Ruby `Timeout` instead. # Net::LDAP's `time` argument doesn't work. Use Ruby `Timeout` instead.
Timeout.timeout(config.timeout) do Timeout.timeout(timeout_time(retries)) do
results = ldap.search(*args) results = ldap.search(*args)
if results.nil? if results.nil?
...@@ -63,16 +68,26 @@ module Gitlab ...@@ -63,16 +68,26 @@ module Gitlab
results results
end end
end end
rescue Net::LDAP::Error => error rescue Net::LDAP::Error, Timeout::Error => error
Rails.logger.warn("LDAP search raised exception #{error.class}: #{error.message}") retries += 1
[] error_message = connection_error_message(error)
rescue Timeout::Error
Rails.logger.warn("LDAP search timed out after #{config.timeout} seconds") Rails.logger.warn(error_message)
[]
if retries < MAX_SEARCH_RETRIES
renew_connection_adapter
retry
else
raise LDAPConnectionError, error_message
end
end end
private private
def timeout_time(retry_number)
SEARCH_RETRY_FACTOR[retry_number] * config.timeout
end
def user_options(fields, value, limit) def user_options(fields, value, limit)
options = { options = {
attributes: Gitlab::Auth::LDAP::Person.ldap_attributes(config), attributes: Gitlab::Auth::LDAP::Person.ldap_attributes(config),
...@@ -104,6 +119,18 @@ module Gitlab ...@@ -104,6 +119,18 @@ module Gitlab
filter filter
end end
end end
def connection_error_message(exception)
if exception.is_a?(Timeout::Error)
"LDAP search timed out after #{config.timeout} seconds"
else
"LDAP search raised exception #{exception.class}: #{exception.message}"
end
end
def renew_connection_adapter
@ldap = Net::LDAP.new(config.adapter_options)
end
end end
end end
end end
......
module Gitlab
module Auth
module LDAP
LDAPConnectionError = Class.new(StandardError)
end
end
end
...@@ -124,6 +124,9 @@ module Gitlab ...@@ -124,6 +124,9 @@ module Gitlab
Gitlab::Auth::LDAP::Person.find_by_uid(auth_hash.uid, adapter) || Gitlab::Auth::LDAP::Person.find_by_uid(auth_hash.uid, adapter) ||
Gitlab::Auth::LDAP::Person.find_by_email(auth_hash.uid, adapter) || Gitlab::Auth::LDAP::Person.find_by_email(auth_hash.uid, adapter) ||
Gitlab::Auth::LDAP::Person.find_by_dn(auth_hash.uid, adapter) Gitlab::Auth::LDAP::Person.find_by_dn(auth_hash.uid, adapter)
rescue Gitlab::Auth::LDAP::LDAPConnectionError
nil
end end
def ldap_config def ldap_config
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::Auth::LDAP::Access do describe Gitlab::Auth::LDAP::Access do
include LdapHelpers
let(:access) { described_class.new user } let(:access) { described_class.new user }
let(:user) { create(:omniauth_user) } let(:user) { create(:omniauth_user) }
...@@ -32,8 +34,10 @@ describe Gitlab::Auth::LDAP::Access do ...@@ -32,8 +34,10 @@ describe Gitlab::Auth::LDAP::Access do
end end
context 'when the user is found' do context 'when the user is found' do
let(:ldap_user) { Gitlab::Auth::LDAP::Person.new(Net::LDAP::Entry.new, 'ldapmain') }
before do before do
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(:ldap_user) allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
end end
context 'and the user is disabled via active directory' do context 'and the user is disabled via active directory' do
...@@ -120,6 +124,22 @@ describe Gitlab::Auth::LDAP::Access do ...@@ -120,6 +124,22 @@ describe Gitlab::Auth::LDAP::Access do
end end
end end
end end
context 'when the connection fails' do
before do
raise_ldap_connection_error
end
it 'does not block the user' do
access.allowed?
expect(user.ldap_blocked?).to be_falsey
end
it 'denies access' do
expect(access.allowed?).to be_falsey
end
end
end end
describe '#block_user' do describe '#block_user' do
......
...@@ -124,16 +124,36 @@ describe Gitlab::Auth::LDAP::Adapter do ...@@ -124,16 +124,36 @@ describe Gitlab::Auth::LDAP::Adapter do
context "when the search raises an LDAP exception" do context "when the search raises an LDAP exception" do
before do before do
allow(adapter).to receive(:renew_connection_adapter).and_return(ldap)
allow(ldap).to receive(:search) { raise Net::LDAP::Error, "some error" } allow(ldap).to receive(:search) { raise Net::LDAP::Error, "some error" }
allow(Rails.logger).to receive(:warn) allow(Rails.logger).to receive(:warn)
end end
it { is_expected.to eq [] } context 'retries the operation' do
before do
stub_const("#{described_class}::MAX_SEARCH_RETRIES", 3)
end
it 'as many times as MAX_SEARCH_RETRIES' do
expect(ldap).to receive(:search).exactly(3).times
expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
end
context 'when no more retries' do
before do
stub_const("#{described_class}::MAX_SEARCH_RETRIES", 1)
end
it 'logs the error' do it 'raises the exception' do
subject expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
expect(Rails.logger).to have_received(:warn).with( end
"LDAP search raised exception Net::LDAP::Error: some error")
it 'logs the error' do
expect { subject }.to raise_error(Gitlab::Auth::LDAP::LDAPConnectionError)
expect(Rails.logger).to have_received(:warn).with(
"LDAP search raised exception Net::LDAP::Error: some error")
end
end
end end
end end
end end
......
require 'spec_helper' require 'spec_helper'
describe Gitlab::Auth::OAuth::User do describe Gitlab::Auth::OAuth::User do
include LdapHelpers
let(:oauth_user) { described_class.new(auth_hash) } let(:oauth_user) { described_class.new(auth_hash) }
let(:gl_user) { oauth_user.gl_user } let(:gl_user) { oauth_user.gl_user }
let(:uid) { 'my-uid' } let(:uid) { 'my-uid' }
...@@ -38,10 +40,6 @@ describe Gitlab::Auth::OAuth::User do ...@@ -38,10 +40,6 @@ describe Gitlab::Auth::OAuth::User do
end end
describe '#save' do describe '#save' do
def stub_ldap_config(messages)
allow(Gitlab::Auth::LDAP::Config).to receive_messages(messages)
end
let(:provider) { 'twitter' } let(:provider) { 'twitter' }
describe 'when account exists on server' do describe 'when account exists on server' do
...@@ -269,20 +267,47 @@ describe Gitlab::Auth::OAuth::User do ...@@ -269,20 +267,47 @@ describe Gitlab::Auth::OAuth::User do
end end
context 'when an LDAP person is not found by uid' do context 'when an LDAP person is not found by uid' do
it 'tries to find an LDAP person by DN and adds the omniauth identity to the user' do it 'tries to find an LDAP person by email and adds the omniauth identity to the user' do
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(nil) allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(nil)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user) allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(ldap_user)
oauth_user.save
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash).to match_array(result_identities(dn, uid))
end
context 'when also not found by email' do
it 'tries to find an LDAP person by DN and adds the omniauth identity to the user' do
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_uid).and_return(nil)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_email).and_return(nil)
allow(Gitlab::Auth::LDAP::Person).to receive(:find_by_dn).and_return(ldap_user)
oauth_user.save
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash).to match_array(result_identities(dn, uid))
end
end
end
def result_identities(dn, uid)
[
{ provider: 'ldapmain', extern_uid: dn },
{ provider: 'twitter', extern_uid: uid }
]
end
context 'when there is an LDAP connection error' do
before do
raise_ldap_connection_error
end
it 'does not save the identity' do
oauth_user.save oauth_user.save
identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } } identities_as_hash = gl_user.identities.map { |id| { provider: id.provider, extern_uid: id.extern_uid } }
expect(identities_as_hash) expect(identities_as_hash).to match_array([{ provider: 'twitter', extern_uid: uid }])
.to match_array(
[
{ provider: 'ldapmain', extern_uid: dn },
{ provider: 'twitter', extern_uid: uid }
]
)
end end
end end
end end
...@@ -739,4 +764,19 @@ describe Gitlab::Auth::OAuth::User do ...@@ -739,4 +764,19 @@ describe Gitlab::Auth::OAuth::User do
expect(oauth_user.find_user).to eql gl_user expect(oauth_user.find_user).to eql gl_user
end end
end end
describe '#find_ldap_person' do
context 'when LDAP connection fails' do
before do
raise_ldap_connection_error
end
it 'returns nil' do
adapter = Gitlab::Auth::LDAP::Adapter.new('ldapmain')
hash = OmniAuth::AuthHash.new(uid: 'whatever', provider: 'ldapmain')
expect(oauth_user.send(:find_ldap_person, hash, adapter)).to be_nil
end
end
end
end end
...@@ -41,4 +41,9 @@ module LdapHelpers ...@@ -41,4 +41,9 @@ module LdapHelpers
entry entry
end end
def raise_ldap_connection_error
allow_any_instance_of(Gitlab::Auth::LDAP::Adapter)
.to receive(:ldap_search).and_raise(Gitlab::Auth::LDAP::LDAPConnectionError)
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment