Merge pull request #38 from github/configurable-member_filter-memberUid-support

Configurable posixGroup membership filter support

Author: mtodd

github-ldap.gemspec

@@ -2,7 +2,7 @@
 
 Gem::Specification.new do |spec|
   spec.name          = "github-ldap"
-  spec.version       = "1.3.1"
+  spec.version       = "1.3.2"
   spec.authors       = ["David Calavera"]
   spec.email         = ["david.calavera@gmail.com"]
   spec.description   = %q{Ldap authentication for humans}

lib/github/ldap.rb

@@ -23,7 +23,7 @@ class Ldap
     # Returns a Net::LDAP::Entry if the operation succeeded.
     def_delegator :@connection, :bind
 
-    attr_reader :uid, :virtual_attributes, :search_domains
+    attr_reader :uid, :search_domains, :virtual_attributes
 
     def initialize(options = {})
       @uid = options[:uid] || "sAMAccountName"
@@ -43,6 +43,9 @@ def initialize(options = {})
       # enable fallback recursive group search unless option is false
       @recursive_group_search_fallback = (options[:recursive_group_search_fallback] != false)
 
+      # enable posixGroup support unless option is false
+      @posix_support = (options[:posix_support] != false)
+
       # search_domains is a connection of bases to perform searches
       # when a base is not explicitly provided.
       @search_domains = Array(options[:search_domains])
@@ -58,6 +61,17 @@ def recursive_group_search_fallback?
       @recursive_group_search_fallback
     end
 
+    # Public - Whether membership checks should include posixGroup filter
+    # conditions on `memberUid`. Configurable since some LDAP servers don't
+    # handle unsupported attribute queries gracefully.
+    #
+    # Enable by passing :posix_support => true.
+    #
+    # Returns true, false, or nil (assumed false).
+    def posix_support_enabled?
+      @posix_support
+    end
+
     # Public - Utility method to check if the connection with the server can be stablished.
     # It tries to bind with the ldap auth default configuration.
     #

lib/github/ldap/domain.rb

@@ -66,7 +66,14 @@ def membership(user_entry, group_names)
           end
         else
           # fallback to non-recursive group membership search
-          filter = member_filter(user_entry) & group_filter(group_names)
+          filter = member_filter(user_entry)
+
+          # include memberUid filter if enabled and entry has a UID set
+          if @ldap.posix_support_enabled? && !user_entry[@ldap.uid].empty?
+            filter |= posix_member_filter(user_entry, @ldap.uid)
+          end
+
+          filter &= group_filter(group_names)
           search(filter: filter)
         end
       end

lib/github/ldap/filter.rb

@@ -20,26 +20,31 @@ def group_filter(group_names)
 
       # Filter to check group membership.
       #
-      # entry:    finds groups this Net::LDAP::Entry is a member of (optional)
-      # uid_attr: specifies the memberUid attribute to match with   (optional)
+      # entry: finds groups this Net::LDAP::Entry is a member of (optional)
       #
       # Returns a Net::LDAP::Filter.
-      def member_filter(entry = nil, uid_attr = @ldap.uid)
+      def member_filter(entry = nil)
         if entry
-          filter =
-            MEMBERSHIP_NAMES. map {|n| Net::LDAP::Filter.eq(n, entry.dn) }.
-                              reduce(:|)
-
-          if !entry[uid_attr].empty?
-            filter |=
-              entry[uid_attr].map { |uid| Net::LDAP::Filter.eq("memberUid", uid) }.
-                              reduce(:|)
-          end
-
-          filter
+          MEMBERSHIP_NAMES.
+            map {|n| Net::LDAP::Filter.eq(n, entry.dn) }.reduce(:|)
         else
-          (MEMBERSHIP_NAMES + %w(memberUid)).
-            map {|n| Net::LDAP::Filter.pres(n)}.reduce(:|)
+          MEMBERSHIP_NAMES.
+            map {|n| Net::LDAP::Filter.pres(n) }.        reduce(:|)
+        end
+      end
+
+      # Filter to check group membership for posixGroups.
+      #
+      # Used by Domain#membership when posix_support_enabled? is true.
+      #
+      # entry:    finds groups this Net::LDAP::Entry is a member of
+      # uid_attr: specifies the memberUid attribute to match with
+      #
+      # Returns a Net::LDAP::Filter or nil if no entry has no UID set.
+      def posix_member_filter(entry, uid_attr)
+        if !entry[uid_attr].empty?
+          entry[uid_attr].map { |uid| Net::LDAP::Filter.eq("memberUid", uid) }.
+                          reduce(:|)
         end
       end
 

test/domain_test.rb

@@ -173,24 +173,18 @@ def self.test_server_options
   def setup
     @ldap = GitHub::Ldap.new(options)
     @domain = @ldap.domain("dc=github,dc=com")
-
-    @group = Net::LDAP::Entry._load("""
-dn: cn=enterprise-posix-devs,ou=groups,dc=github,dc=com
-cn: enterprise-posix-devs
-objectClass: posixGroup
-memberUid: benburkert
-memberUid: mtodd""")
+    @cn = "enterprise-posix-devs"
   end
 
   def test_membership_for_posixGroups
     assert user = @ldap.domain('uid=mtodd,ou=users,dc=github,dc=com').bind
 
-    assert @domain.is_member?(user, @group.cn),
-      "Expected `#{@group.cn.first}` to include the member `#{user.dn}`"
+    assert @domain.is_member?(user, [@cn]),
+      "Expected `#{@cn}` to include the member `#{user.dn}`"
   end
 end
 
-class GitHubLdapPosixGroupsTest < GitHub::Ldap::Test
+class GitHubLdapPosixGroupsWithoutRecursionTest < GitHub::Ldap::Test
   def self.test_server_options
     {
       custom_schemas: FIXTURES.join('posixGroup.schema.ldif'),
@@ -203,19 +197,41 @@ def self.test_server_options
   def setup
     @ldap = GitHub::Ldap.new(options)
     @domain = @ldap.domain("dc=github,dc=com")
+    @cn = "enterprise-posix-devs"
+  end
+
+  def test_membership_for_posixGroups
+    assert user = @ldap.domain('uid=mtodd,ou=users,dc=github,dc=com').bind
 
-    @group = Net::LDAP::Entry._load("""
-dn: cn=enterprise-posix-devs,ou=groups,dc=github,dc=com
-cn: enterprise-posix-devs
-objectClass: posixGroup
-memberUid: benburkert
-memberUid: mtodd""")
+    assert @domain.is_member?(user, [@cn]),
+      "Expected `#{@cn}` to include the member `#{user.dn}`"
+  end
+end
+
+# Specifically testing that this doesn't break when posixGroups are not
+# supported.
+class GitHubLdapWithoutPosixGroupsTest < GitHub::Ldap::Test
+  def self.test_server_options
+    {
+      custom_schemas: FIXTURES.join('posixGroup.schema.ldif'),
+      user_fixtures: FIXTURES.join('github-with-posixGroups.ldif').to_s,
+      # so we test the test the non-recursive group membership search
+      recursive_group_search_fallback: false,
+      # explicitly disable posixGroup support (even if the schema supports it)
+      posix_support:                   false
+    }
+  end
+
+  def setup
+    @ldap = GitHub::Ldap.new(options)
+    @domain = @ldap.domain("dc=github,dc=com")
+    @cn = "enterprise-posix-devs"
   end
 
   def test_membership_for_posixGroups
     assert user = @ldap.domain('uid=mtodd,ou=users,dc=github,dc=com').bind
 
-    assert @domain.is_member?(user, @group.cn),
-      "Expected `#{@group.cn.first}` to include the member `#{user.dn}`"
+    refute @domain.is_member?(user, [@cn]),
+      "Expected `#{@cn}` to not include the member `#{user.dn}`"
   end
 end

test/filter_test.rb

@@ -24,18 +24,22 @@ def setup
   end
 
   def test_member_present
-    assert_equal "(|(|(member=*)(uniqueMember=*))(memberUid=*))", @subject.member_filter.to_s
+    assert_equal "(|(member=*)(uniqueMember=*))", @subject.member_filter.to_s
   end
 
   def test_member_equal
-    assert_equal "(|(|(member=#{@me})(uniqueMember=#{@me}))(memberUid=#{@uid}))",
+    assert_equal "(|(member=#{@me})(uniqueMember=#{@me}))",
                  @subject.member_filter(@entry).to_s
   end
 
-  def test_member_without_uid
+  def test_posix_member_without_uid
     @entry.uid = nil
-    assert_equal "(|(member=#{@me})(uniqueMember=#{@me}))",
-                 @subject.member_filter(@entry).to_s
+    assert_nil @subject.posix_member_filter(@entry, @ldap.uid)
+  end
+
+  def test_posix_member_equal
+    assert_equal "(memberUid=#{@uid})",
+                 @subject.posix_member_filter(@entry, @ldap.uid).to_s
   end
 
   def test_groups_reduced