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.

6 files changed, 101 insertions, 4 deletions

diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index eefdb1833fc..f3e781ac4e9 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.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
diff --git a/lib/gitlab/ldap/access.rb b/lib/gitlab/ldap/access.rb
index 8f492e5c012..4e48ff11871 100644
--- a/lib/gitlab/ldap/access.rb
+++ b/lib/gitlab/ldap/access.rb
@@ -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
diff --git a/lib/gitlab/ldap/adapter.rb b/lib/gitlab/ldap/adapter.rb
index 0777558d643..e36616f0e66 100644
--- a/lib/gitlab/ldap/adapter.rb
+++ b/lib/gitlab/ldap/adapter.rb
@@ -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
diff --git a/lib/gitlab/ldap/person.rb b/lib/gitlab/ldap/person.rb
index 06b17c58f8c..9ad6618bd46 100644
--- a/lib/gitlab/ldap/person.rb
+++ b/lib/gitlab/ldap/person.rb
@@ -1,6 +1,11 @@
 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
diff --git a/spec/lib/gitlab/ldap/ldap_access_spec.rb b/spec/lib/gitlab/ldap/ldap_access_spec.rb
new file mode 100644
index 00000000000..d8c107502ba
--- /dev/null
+++ b/spec/lib/gitlab/ldap/ldap_access_spec.rb
@@ -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
diff --git a/spec/lib/gitlab/ldap/ldap_adapter_spec.rb b/spec/lib/gitlab/ldap/ldap_adapter_spec.rb
new file mode 100644
index 00000000000..c3f07334431
--- /dev/null
+++ b/spec/lib/gitlab/ldap/ldap_adapter_spec.rb
@@ -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