Merge pull request #10 from jhollingsworth/sync-intridea-omniauth-ldap

Sync with intridea/omniauth-ldap

Author: dzaporozhets

.travis.yml

@@ -2,5 +2,5 @@ branches:
   only:
     - 'master'
 rvm:
-  - 1.9.2
+  - 2.0.0
 script: "bundle exec rspec spec"

Gemfile.lock

@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    gitlab_omniauth-ldap (1.0.2)
+    gitlab_omniauth-ldap (1.0.4)
       net-ldap (~> 0.3.1)
       omniauth (~> 1.0)
       pyu-ruby-sasl (~> 0.0.3.1)

README.md

@@ -11,6 +11,10 @@ Use the LDAP strategy as a middleware in your application:
         :method => :plain,
         :base => 'dc=intridea, dc=com',
         :uid => 'sAMAccountName',
+        :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')},
+        :bind_dn => 'default_bind_dn',
+        # Or, alternatively:
+        #:filter => '(&(uid=%{username})(memberOf=cn=myapp-users,ou=groups,dc=example,dc=com))'
         :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')}
         :bind_dn => 'default_bind_dn'
         :password => 'password'
@@ -29,6 +33,9 @@ Allowed values of :method are: :plain, :ssl, :tls.
 :uid is the LDAP attribute name for the user name in the login form. 
   typically AD would be 'sAMAccountName' or 'UserPrincipalName', while OpenLDAP is 'uid'.
 
+:filter is the LDAP filter used to search the user entry. It can be used in place of :uid for more flexibility.
+  `%{username}` will be replaced by the user name processed by :name_proc.
+
 :name_proc allows you to match the user name entered with the format of the :uid attributes. 
   For example, value of 'sAMAccountName' in AD contains only the windows user name. If your user prefers using 
   email to login, a name_proc as above will trim the email string down to just the windows login name. 

gitlab_omniauth-ldap.gemspec

@@ -7,6 +7,7 @@ Gem::Specification.new do |gem|
   gem.description   = %q{A LDAP strategy for OmniAuth.}
   gem.summary       = %q{A LDAP strategy for OmniAuth.}
   gem.homepage      = "https://github.com/gitlabhq/omniauth-ldap"
+  gem.license       = "MIT"
 
   gem.add_runtime_dependency     'omniauth', '~> 1.0'
   gem.add_runtime_dependency     'net-ldap', '~> 0.3.1'

lib/omniauth-ldap/adaptor.rb

@@ -3,7 +3,6 @@
 require 'rack'
 require 'net/ldap'
 require 'net/ntlm'
-require 'uri'
 require 'sasl'
 require 'kconv'
 module OmniAuth
@@ -14,9 +13,10 @@ class ConfigurationError < StandardError; end
       class AuthenticationError < StandardError; end
       class ConnectionError < StandardError; end
 
-      VALID_ADAPTER_CONFIGURATION_KEYS = [:host, :port, :method, :bind_dn, :password, :try_sasl, :sasl_mechanisms, :uid, :base, :allow_anonymous]
+      VALID_ADAPTER_CONFIGURATION_KEYS = [:host, :port, :method, :bind_dn, :password, :try_sasl, :sasl_mechanisms, :uid, :base, :allow_anonymous, :filter]
 
-      MUST_HAVE_KEYS = [:host, :port, :method, :uid, :base]
+      # A list of needed keys. Possible alternatives are specified using sub-lists.
+      MUST_HAVE_KEYS = [:host, :port, :method, [:uid, :filter], :base]
 
       METHOD = {
         :ssl => :simple_tls,
@@ -25,11 +25,15 @@ class ConnectionError < StandardError; end
       }
 
       attr_accessor :bind_dn, :password
-      attr_reader :connection, :uid, :base, :auth
+      attr_reader :connection, :uid, :base, :auth, :filter
       def self.validate(configuration={})
         message = []
-        MUST_HAVE_KEYS.each do |name|
-           message << name if configuration[name].nil?
+        MUST_HAVE_KEYS.each do |names|
+          names = [names].flatten
+          missing_keys = names.select{|name| configuration[name].nil?}
+          if missing_keys == names
+            message << names.join(' or ')
+          end
         end
         raise ArgumentError.new(message.join(",") +" MUST be provided") unless message.empty?
       end
@@ -48,7 +52,6 @@ def initialize(configuration={})
           :encryption => method,
           :base => @base
         }
-        @uri = construct_uri(@host, @port, @method != :plain)
 
         @bind_method = @try_sasl ? :sasl : (@allow_anonymous||!@bind_dn||!@password ? :anonymous : :simple)
 
@@ -140,10 +143,6 @@ def sasl_bind_setup_gss_spnego(options)
         [Net::NTLM::Message::Type1.new.serialize, nego]
       end
 
-      def construct_uri(host, port, ssl)
-        protocol = ssl ? "ldaps" : "ldap"
-        URI.parse("#{protocol}://#{host}:#{port}").to_s
-      end
     end
   end
 end

lib/omniauth-ldap/version.rb

@@ -1,5 +1,5 @@
 module OmniAuth
   module LDAP
-    VERSION = "1.0.3"
+    VERSION = "1.0.4"
   end
 end

lib/omniauth/strategies/ldap.rb

@@ -3,7 +3,6 @@
 module OmniAuth
   module Strategies
     class LDAP
-      class MissingCredentialsError < StandardError; end
       include OmniAuth::Strategy
       @@config = {
         'name' => 'cn',
@@ -38,14 +37,10 @@ def request_phase
       def callback_phase
         @adaptor = OmniAuth::LDAP::Adaptor.new @options
 
+        return fail!(:missing_credentials) if missing_credentials?
         begin
-          # GITLAB security patch
-          # Dont allow blank password for ldap auth
-          if request['username'].nil? || request['username'].empty? || request['password'].nil? || request['password'].empty?
-            raise MissingCredentialsError.new("Missing login credentials")
-          end
+          @ldap_user_info = @adaptor.bind_as(:filter => filter(@adaptor), :size => 1, :password => request['password'])
 
-          @ldap_user_info = @adaptor.bind_as(:filter => Net::LDAP::Filter.eq(@adaptor.uid, @options[:name_proc].call(request['username'])),:size => 1, :password => request['password'])
           return fail!(:invalid_credentials) if !@ldap_user_info
 
           @user_info = self.class.map_user(@@config, @ldap_user_info)
@@ -55,6 +50,14 @@ def callback_phase
         end
       end
 
+      def filter adaptor
+        if adaptor.filter and !adaptor.filter.empty?
+          Net::LDAP::Filter.construct(adaptor.filter % {username: @options[:name_proc].call(request['username'])})
+        else
+          Net::LDAP::Filter.eq(adaptor.uid, @options[:name_proc].call(request['username']))
+        end
+      end
+
       uid {
         @user_info["uid"]
       }
@@ -70,14 +73,14 @@ def self.map_user(mapper, object)
         mapper.each do |key, value|
           case value
           when String
-            user[key] = object[value.downcase.to_sym].first if object[value.downcase.to_sym]
+            user[key] = object[value.downcase.to_sym].first if object.respond_to? value.downcase.to_sym
           when Array
-            value.each {|v| (user[key] = object[v.downcase.to_sym].first; break;) if object[v.downcase.to_sym]}
+            value.each {|v| (user[key] = object[v.downcase.to_sym].first; break;) if object.respond_to? v.downcase.to_sym}
           when Hash
             value.map do |key1, value1|
               pattern = key1.dup
               value1.each_with_index do |v,i|
-                part = ''; v.collect(&:downcase).collect(&:to_sym).each {|v1| (part = object[v1].first; break;) if object[v1]}
+                part = ''; v.collect(&:downcase).collect(&:to_sym).each {|v1| (part = object[v1].first; break;) if object.respond_to? v1}
                 pattern.gsub!("%#{i}",part||'')
               end
               user[key] = pattern
@@ -86,6 +89,12 @@ def self.map_user(mapper, object)
         end
         user
       end
+
+      protected
+
+      def missing_credentials?
+        request['username'].nil? or request['username'].empty? or request['password'].nil? or request['password'].empty?
+      end # missing_credentials?
     end
   end
 end

spec/omniauth-ldap/adaptor_spec.rb

@@ -2,13 +2,13 @@
 describe "OmniAuth::LDAP::Adaptor" do
 
   describe 'initialize' do
-
     it 'should throw exception when must have field is not set' do
       #[:host, :port, :method, :bind_dn]
-      lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain'})}.should raise_error(ArgumentError)      
+      lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain'})}.should raise_error(ArgumentError)
     end
+
     it 'should throw exception when method is not supported' do
-      lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'myplain', uid: 'uid', port: 389, base: 'dc=com'})}.should raise_error(OmniAuth::LDAP::Adaptor::ConfigurationError)      
+      lambda { OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'myplain', uid: 'uid', port: 389, base: 'dc=com'})}.should raise_error(OmniAuth::LDAP::Adaptor::ConfigurationError)
     end
 
     it 'should setup ldap connection with anonymous' do
@@ -17,54 +17,59 @@
       adaptor.connection.host.should == '192.168.1.145'
       adaptor.connection.port.should == 389
       adaptor.connection.base.should == 'dc=intridea, dc=com'
-      adaptor.connection.instance_variable_get('@auth').should == {:method => :anonymous, :username => nil, :password => nil}      
+      adaptor.connection.instance_variable_get('@auth').should == {:method => :anonymous, :username => nil, :password => nil}
     end
+
     it 'should setup ldap connection with simple' do
       adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', bind_dn: 'bind_dn', password: 'password'})
       adaptor.connection.should_not == nil
       adaptor.connection.host.should == '192.168.1.145'
       adaptor.connection.port.should == 389
       adaptor.connection.base.should == 'dc=intridea, dc=com'
-      adaptor.connection.instance_variable_get('@auth').should == {:method => :simple, :username => 'bind_dn', :password => 'password'}      
-    end    
+      adaptor.connection.instance_variable_get('@auth').should == {:method => :simple, :username => 'bind_dn', :password => 'password'}
+    end
+
     it 'should setup ldap connection with sasl-md5' do
       adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', try_sasl: true, sasl_mechanisms: ["DIGEST-MD5"], bind_dn: 'bind_dn', password: 'password'})
       adaptor.connection.should_not == nil
       adaptor.connection.host.should == '192.168.1.145'
       adaptor.connection.port.should == 389
       adaptor.connection.base.should == 'dc=intridea, dc=com'
-      adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl      
-      adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'DIGEST-MD5'      
-      adaptor.connection.instance_variable_get('@auth')[:initial_credential].should == ''      
-      adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil      
+      adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl
+      adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'DIGEST-MD5'
+      adaptor.connection.instance_variable_get('@auth')[:initial_credential].should == ''
+      adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil
     end
+
     it 'should setup ldap connection with sasl-gss' do
       adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', try_sasl: true, sasl_mechanisms: ["GSS-SPNEGO"], bind_dn: 'bind_dn', password: 'password'})
       adaptor.connection.should_not == nil
       adaptor.connection.host.should == '192.168.1.145'
       adaptor.connection.port.should == 389
       adaptor.connection.base.should == 'dc=intridea, dc=com'
-      adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl      
-      adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'GSS-SPNEGO'      
-      adaptor.connection.instance_variable_get('@auth')[:initial_credential].should =~ /^NTLMSSP/      
-      adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil      
+      adaptor.connection.instance_variable_get('@auth')[:method].should == :sasl
+      adaptor.connection.instance_variable_get('@auth')[:mechanism].should == 'GSS-SPNEGO'
+      adaptor.connection.instance_variable_get('@auth')[:initial_credential].should =~ /^NTLMSSP/
+      adaptor.connection.instance_variable_get('@auth')[:challenge_response].should_not be_nil
     end
   end
-  
+
   describe 'bind_as' do
     let(:args) { {:filter => Net::LDAP::Filter.eq('sAMAccountName', 'username'), :password => 'password', :size => 1} }
     let(:rs) { Struct.new(:dn).new('new dn') }
+
     it 'should bind simple' do
       adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.126", method: 'plain', base: 'dc=score, dc=local', port: 389, uid: 'sAMAccountName', bind_dn: 'bind_dn', password: 'password'})
       adaptor.connection.should_receive(:open).and_yield(adaptor.connection)
-      adaptor.connection.should_receive(:search).with(args).and_return([rs])      
+      adaptor.connection.should_receive(:search).with(args).and_return([rs])
       adaptor.connection.should_receive(:bind).with({:username => 'new dn', :password => args[:password], :method => :simple}).and_return(true)
       adaptor.bind_as(args).should == rs
     end
+
     it 'should bind sasl' do
       adaptor = OmniAuth::LDAP::Adaptor.new({host: "192.168.1.145", method: 'plain', base: 'dc=intridea, dc=com', port: 389, uid: 'sAMAccountName', try_sasl: true, sasl_mechanisms: ["GSS-SPNEGO"], bind_dn: 'bind_dn', password: 'password'})
       adaptor.connection.should_receive(:open).and_yield(adaptor.connection)
-      adaptor.connection.should_receive(:search).with(args).and_return([rs])      
+      adaptor.connection.should_receive(:search).with(args).and_return([rs])
       adaptor.connection.should_receive(:bind).and_return(true)
       adaptor.bind_as(args).should == rs
     end