Merge branch 'ad_disabled_users' into 'master'

Block 'disabled' AD users over SSH The existing SSH access check for LDAP users ignored the ActiveDirectory 'disabled' flag.

Merge branch 'ad_disabled_users' into 'master'
Block 'disabled' AD users over SSH The existing SSH access check for LDAP users ignored the ActiveDirectory 'disabled' flag.
Dmitriy Zaporozhets
2 parents a107776e be1120e9
Showing 6 changed files with 101 additions and 4 deletions Show diff stats
lib/gitlab/git_access.rb
lib/gitlab/ldap/access.rb
lib/gitlab/ldap/adapter.rb
lib/gitlab/ldap/person.rb
spec/lib/gitlab/ldap/ldap_access_spec.rb
spec/lib/gitlab/ldap/ldap_adapter_spec.rb
@@ -66,8 +66,8 @@ module Gitlab
       if Gitlab.config.ldap.enabled
         if user.ldap_user?
           # Check if LDAP user exists and match LDAP user_filter
-          unless Gitlab::LDAP::Access.new.allowed?(user)
-            return false
+          Gitlab::LDAP::Access.open do |adapter|
+            return false unless adapter.allowed?(user)
           end
         end
       end
@@ -14,7 +14,11 @@ module Gitlab
       end
  
       def allowed?(user)
-        !!Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
+        if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
+          !Gitlab::LDAP::Person.active_directory_disabled?(user.extern_uid, adapter)
+        else
+          false
+        end
       rescue
         false
       end
@@ -64,7 +64,7 @@ module Gitlab
                              end
         end
  
-        entries = ldap.search(options).select do |entry|
+        entries = ldap_search(options).select do |entry|
           entry.respond_to? config.uid
         end
  
@@ -77,6 +77,26 @@ module Gitlab
         users(*args).first
       end
  
+      def dn_matches_filter?(dn, filter)
+        ldap_search(base: dn, filter: filter, scope: Net::LDAP::SearchScope_BaseObject, attributes: %w{dn}).any?
+      end
+
+      def ldap_search(*args)
+        results = ldap.search(*args)
+
+        if results.nil?
+          response = ldap.get_operation_result
+
+          unless response.code.zero?
+            Rails.logger.warn("LDAP search error: #{response.message}")
+          end
+
+          []
+        else
+          results
+        end
+      end
+
       private
  
       def config
 module Gitlab
   module LDAP
     class Person
+      # Active Directory-specific LDAP filter that checks if bit 2 of the
+      # userAccountControl attribute is set.
+      # Source: http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/
+      AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", "2")
+
       def self.find_by_uid(uid, adapter=nil)
         adapter ||= Gitlab::LDAP::Adapter.new
         adapter.user(config.uid, uid)
@@ -11,6 +16,11 @@ module Gitlab
         adapter.user('dn', dn)
       end
  
+      def self.active_directory_disabled?(dn, adapter=nil)
+        adapter ||= Gitlab::LDAP::Adapter.new
+        adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
+      end
+
       def initialize(entry)
         Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
         @entry = entry
@@ -0,0 +1,32 @@
+require 'spec_helper'
+
+describe Gitlab::LDAP::Access do
+  let(:access) { Gitlab::LDAP::Access.new }
+  let(:user) { create(:user) }
+
+  describe :allowed? do
+    subject { access.allowed?(user) }
+
+    context 'when the user cannot be found' do
+      before { Gitlab::LDAP::Person.stub(find_by_dn: nil) }
+
+      it { should be_false }
+    end
+
+    context 'when the user is found' do
+      before { Gitlab::LDAP::Person.stub(find_by_dn: :ldap_user) }
+
+      context 'and the Active Directory disabled flag is set' do
+        before { Gitlab::LDAP::Person.stub(active_directory_disabled?: true) }
+
+        it { should be_false }
+      end
+
+      context 'and the Active Directory disabled flag is not set' do
+        before { Gitlab::LDAP::Person.stub(active_directory_disabled?: false) }
+
+        it { should be_true }
+      end
+    end
+  end
+end
@@ -0,0 +1,31 @@
+require 'spec_helper'
+
+describe Gitlab::LDAP::Adapter do
+  let(:adapter) { Gitlab::LDAP::Adapter.new }
+
+  describe :dn_matches_filter? do
+    let(:ldap) { double(:ldap) }
+    subject { adapter.dn_matches_filter?(:dn, :filter) }
+    before { adapter.stub(ldap: ldap) }
+
+    context "when the search is successful" do
+      context "and the result is non-empty" do
+        before { ldap.stub(search: [:foo]) }
+
+        it { should be_true }
+      end
+
+      context "and the result is empty" do
+        before { ldap.stub(search: []) }
+
+        it { should be_false }
+      end
+    end
+
+    context "when the search encounters an error" do
+      before { ldap.stub(search: nil, get_operation_result: double(code: 1, message: 'some error')) }
+
+      it { should be_false }
+    end
+  end
+end
...	...	@@ -66,8 +66,8 @@ module Gitlab
66	66	if Gitlab.config.ldap.enabled
67	67	if user.ldap_user?
68	68	# Check if LDAP user exists and match LDAP user_filter
69		- unless Gitlab::LDAP::Access.new.allowed?(user)
70		- return false
	69	+ Gitlab::LDAP::Access.open do \|adapter\|
	70	+ return false unless adapter.allowed?(user)
71	71	end
72	72	end
73	73	end
...	...
...	...	@@ -14,7 +14,11 @@ module Gitlab
14	14	end
15	15
16	16	def allowed?(user)
17		- !!Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
	17	+ if Gitlab::LDAP::Person.find_by_dn(user.extern_uid, adapter)
	18	+ !Gitlab::LDAP::Person.active_directory_disabled?(user.extern_uid, adapter)
	19	+ else
	20	+ false
	21	+ end
18	22	rescue
19	23	false
20	24	end
...	...
...	...	@@ -64,7 +64,7 @@ module Gitlab
64	64	end
65	65	end
66	66
67		- entries = ldap.search(options).select do \|entry\|
	67	+ entries = ldap_search(options).select do \|entry\|
68	68	entry.respond_to? config.uid
69	69	end
70	70
...	...	@@ -77,6 +77,26 @@ module Gitlab
77	77	users(*args).first
78	78	end
79	79
	80	+ def dn_matches_filter?(dn, filter)
	81	+ ldap_search(base: dn, filter: filter, scope: Net::LDAP::SearchScope_BaseObject, attributes: %w{dn}).any?
	82	+ end
	83	+
	84	+ def ldap_search(*args)
	85	+ results = ldap.search(*args)
	86	+
	87	+ if results.nil?
	88	+ response = ldap.get_operation_result
	89	+
	90	+ unless response.code.zero?
	91	+ Rails.logger.warn("LDAP search error: #{response.message}")
	92	+ end
	93	+
	94	+ []
	95	+ else
	96	+ results
	97	+ end
	98	+ end
	99	+
80	100	private
81	101
82	102	def config
...	...
1	1	module Gitlab
2	2	module LDAP
3	3	class Person
	4	+ # Active Directory-specific LDAP filter that checks if bit 2 of the
	5	+ # userAccountControl attribute is set.
	6	+ # Source: http://ctogonewild.com/2009/09/03/bitmask-searches-in-ldap/
	7	+ AD_USER_DISABLED = Net::LDAP::Filter.ex("userAccountControl:1.2.840.113556.1.4.803", "2")
	8	+
4	9	def self.find_by_uid(uid, adapter=nil)
5	10	adapter \|\|= Gitlab::LDAP::Adapter.new
6	11	adapter.user(config.uid, uid)
...	...	@@ -11,6 +16,11 @@ module Gitlab
11	16	adapter.user('dn', dn)
12	17	end
13	18
	19	+ def self.active_directory_disabled?(dn, adapter=nil)
	20	+ adapter \|\|= Gitlab::LDAP::Adapter.new
	21	+ adapter.dn_matches_filter?(dn, AD_USER_DISABLED)
	22	+ end
	23	+
14	24	def initialize(entry)
15	25	Rails.logger.debug { "Instantiating #{self.class.name} with LDIF:\n#{entry.to_ldif}" }
16	26	@entry = entry
...	...
...	...	@@ -0,0 +1,32 @@
	1	+require 'spec_helper'
	2	+
	3	+describe Gitlab::LDAP::Access do
	4	+ let(:access) { Gitlab::LDAP::Access.new }
	5	+ let(:user) { create(:user) }
	6	+
	7	+ describe :allowed? do
	8	+ subject { access.allowed?(user) }
	9	+
	10	+ context 'when the user cannot be found' do
	11	+ before { Gitlab::LDAP::Person.stub(find_by_dn: nil) }
	12	+
	13	+ it { should be_false }
	14	+ end
	15	+
	16	+ context 'when the user is found' do
	17	+ before { Gitlab::LDAP::Person.stub(find_by_dn: :ldap_user) }
	18	+
	19	+ context 'and the Active Directory disabled flag is set' do
	20	+ before { Gitlab::LDAP::Person.stub(active_directory_disabled?: true) }
	21	+
	22	+ it { should be_false }
	23	+ end
	24	+
	25	+ context 'and the Active Directory disabled flag is not set' do
	26	+ before { Gitlab::LDAP::Person.stub(active_directory_disabled?: false) }
	27	+
	28	+ it { should be_true }
	29	+ end
	30	+ end
	31	+ end
	32	+end
...	...
...	...	@@ -0,0 +1,31 @@
	1	+require 'spec_helper'
	2	+
	3	+describe Gitlab::LDAP::Adapter do
	4	+ let(:adapter) { Gitlab::LDAP::Adapter.new }
	5	+
	6	+ describe :dn_matches_filter? do
	7	+ let(:ldap) { double(:ldap) }
	8	+ subject { adapter.dn_matches_filter?(:dn, :filter) }
	9	+ before { adapter.stub(ldap: ldap) }
	10	+
	11	+ context "when the search is successful" do
	12	+ context "and the result is non-empty" do
	13	+ before { ldap.stub(search: [:foo]) }
	14	+
	15	+ it { should be_true }
	16	+ end
	17	+
	18	+ context "and the result is empty" do
	19	+ before { ldap.stub(search: []) }
	20	+
	21	+ it { should be_false }
	22	+ end
	23	+ end
	24	+
	25	+ context "when the search encounters an error" do
	26	+ before { ldap.stub(search: nil, get_operation_result: double(code: 1, message: 'some error')) }
	27	+
	28	+ it { should be_false }
	29	+ end
	30	+ end
	31	+end
...	...