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

Sync with intridea/omniauth-ldap

.travis.yml

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

Gemfile.lock

 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 @@ module OmniAuth
       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]
-      MUST_HAVE_KEYS = [:host, :port, :method, :uid, :base]
+      VALID_ADAPTER_CONFIGURATION_KEYS = [:host, :port, :method, :bind_dn, :password, :try_sasl, :sasl_mechanisms, :uid, :base, :allow_anonymous, :filter]
+      # 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 @@ module OmniAuth
       }
       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 @@ module OmniAuth
           :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 @@ module OmniAuth
         [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

 module OmniAuth
   module LDAP
-    VERSION = "1.0.3"
+    VERSION = "1.0.4"
   end
 end

lib/omniauth/strategies/ldap.rb

@@ -3,7 +3,6 @@ require 'omniauth'
 module OmniAuth
   module Strategies
     class LDAP
-      class MissingCredentialsError < StandardError; end
       include OmniAuth::Strategy
       @@config = {
         'name' => 'cn',
@@ -38,14 +37,10 @@ module OmniAuth
       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 => Net::LDAP::Filter.eq(@adaptor.uid, @options[:name_proc].call(request['username'])),:size => 1, :password => request['password'])
+          @ldap_user_info = @adaptor.bind_as(:filter => filter(@adaptor), :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 @@ module OmniAuth
         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 @@ module OmniAuth
         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 @@ module OmniAuth
         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 @@ require 'spec_helper'
 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 @@ describe "OmniAuth::LDAP::Adaptor" do
       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

spec/omniauth/strategies/ldap_spec.rb

@@ -12,7 +12,7 @@ describe "OmniAuth::Strategies::LDAP" do
   # :password => 'password'
   class MyLdapProvider < OmniAuth::Strategies::LDAP; end
-  def app
+  let(:app) do
     Rack::Builder.new {
       use OmniAuth::Test::PhonySession
       use MyLdapProvider, :name => 'ldap', :title => 'MyLdap Form', :host => '192.168.1.145', :base => 'dc=score, dc=local', :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')}
@@ -20,7 +20,7 @@ describe "OmniAuth::Strategies::LDAP" do
     }.to_app
   end
-  def session
+  let(:session) do
     last_request.env['rack.session']
   end
@@ -46,12 +46,12 @@ describe "OmniAuth::Strategies::LDAP" do
     it 'should have a label of the form title' do
       last_response.body.scan('MyLdap Form').size.should > 1
     end
-
   end
   describe 'post /auth/ldap/callback' do
     before(:each) do
-      @adaptor = mock(OmniAuth::LDAP::Adaptor, {:uid => 'ping'})
+      @adaptor = double(OmniAuth::LDAP::Adaptor, {:uid => 'ping'})
+      @adaptor.stub(:filter)
       OmniAuth::LDAP::Adaptor.stub(:new).and_return(@adaptor)
     end
@@ -60,10 +60,10 @@ describe "OmniAuth::Strategies::LDAP" do
         @adaptor.stub(:bind_as).and_return(false)
       end
-      it 'should raise MissingCredentialsError' do
+      it 'should fail with missing_credentials' do
         post('/auth/ldap/callback', {})
         last_response.should be_redirect
-        last_response.headers['Location'].should =~ %r{ldap_error}
+        last_response.headers['Location'].should =~ %r{missing_credentials}
       end
       it 'should redirect to error page' do
@@ -78,22 +78,167 @@ describe "OmniAuth::Strategies::LDAP" do
         last_response.should be_redirect
         last_response.headers['Location'].should =~ %r{ldap_error}
       end
+
+      context "when username is not preset" do
+        it 'should redirect to error page' do
+          post('/auth/ldap/callback', {})
+
+          last_response.should be_redirect
+          last_response.headers['Location'].should =~ %r{missing_credentials}
+        end
+      end
+
+      context "when username is empty" do
+        it 'should redirect to error page' do
+          post('/auth/ldap/callback', {:username => ""})
+
+          last_response.should be_redirect
+          last_response.headers['Location'].should =~ %r{missing_credentials}
+        end
+      end
+
+      context "when username is present" do
+        context "and password is not preset" do
+          it 'should redirect to error page' do
+            post('/auth/ldap/callback', {:username => "ping"})
+
+            last_response.should be_redirect
+            last_response.headers['Location'].should =~ %r{missing_credentials}
+          end
+        end
+
+        context "and password is empty" do
+          it 'should redirect to error page' do
+            post('/auth/ldap/callback', {:username => "ping", :password => ""})
+
+            last_response.should be_redirect
+            last_response.headers['Location'].should =~ %r{missing_credentials}
+          end
+        end
+      end
+
+      context "when username and password are present" do
+        context "and bind on LDAP server failed" do
+          it 'should redirect to error page' do
+            post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
+
+            last_response.should be_redirect
+            last_response.headers['Location'].should =~ %r{invalid_credentials}
+          end
+          context 'and filter is set' do
+            it 'should bind with filter' do
+              @adaptor.stub(:filter).and_return('uid=%{username}')
+              Net::LDAP::Filter.should_receive(:construct).with('uid=ping')
+              post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
+
+              last_response.should be_redirect
+              last_response.headers['Location'].should =~ %r{invalid_credentials}
+            end
+          end
+
+        end
+
+        context "and communication with LDAP server caused an exception" do
+          before :each do
+            @adaptor.stub(:bind_as).and_throw(Exception.new('connection_error'))
+          end
+
+          it 'should redirect to error page' do
+            post('/auth/ldap/callback', {:username => "ping", :password => "password"})
+
+            last_response.should be_redirect
+            last_response.headers['Location'].should =~ %r{ldap_error}
+          end
+        end
+      end
     end
     context 'success' do
       let(:auth_hash){ last_request.env['omniauth.auth'] }
+
       before(:each) do
-        @adaptor.stub(:bind_as).and_return({:dn => ['cn=ping, dc=intridea, dc=com'], :mail => ['ping@intridea.com'], :givenname => ['Ping'], :sn => ['Yu'],
-                                           :telephonenumber => ['555-555-5555'], :mobile => ['444-444-4444'], :uid => ['ping'], :title => ['dev'], :address =>[ 'k street'],
-                                           :l => ['Washington'], :st => ['DC'], :co => ["U.S.A"], :postofficebox => ['20001'], :wwwhomepage => ['www.intridea.com'],
-                                           :jpegphoto => ['http://www.intridea.com/ping.jpg'], :description => ['omniauth-ldap']})
+        @adaptor.stub(:filter)
+        @adaptor.stub(:bind_as).and_return(Net::LDAP::Entry.from_single_ldif_string(
+%Q{dn: cn=ping, dc=intridea, dc=com
+mail: ping@intridea.com
+givenname: Ping
+sn: Yu
+telephonenumber: 555-555-5555
+mobile: 444-444-4444
+uid: ping
+title: dev
+address: k street
+l: Washington
+st: DC
+co: U.S.A
+postofficebox: 20001
+wwwhomepage: www.intridea.com
+jpegphoto: http://www.intridea.com/ping.jpg
+description: omniauth-ldap
+}
+    ))
+      end
+
+      it 'should not redirect to error page' do
         post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
+        last_response.should_not be_redirect
       end
-      it 'should raise MissingCredentialsError' do
-        should_not raise_error OmniAuth::Strategies::LDAP::MissingCredentialsError
+      context 'and filter is set' do
+        it 'should bind with filter' do
+          @adaptor.stub(:filter).and_return('uid=%{username}')
+          Net::LDAP::Filter.should_receive(:construct).with('uid=ping')
+          post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
+
+          last_response.should_not be_redirect
+        end
       end
-      it 'should map user info' do
+
+      it 'should map user info to Auth Hash' do
+        post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
+        auth_hash.uid.should == 'cn=ping, dc=intridea, dc=com'
+        auth_hash.info.email.should == 'ping@intridea.com'
+        auth_hash.info.first_name.should == 'Ping'
+        auth_hash.info.last_name.should == 'Yu'
+        auth_hash.info.phone.should == '555-555-5555'
+        auth_hash.info.mobile.should == '444-444-4444'
+        auth_hash.info.nickname.should == 'ping'
+        auth_hash.info.title.should == 'dev'
+        auth_hash.info.location.should == 'k street, Washington, DC, U.S.A 20001'
+        auth_hash.info.url.should == 'www.intridea.com'
+        auth_hash.info.image.should == 'http://www.intridea.com/ping.jpg'
+        auth_hash.info.description.should == 'omniauth-ldap'
+      end
+    end
+    
+    context 'alternate fields' do
+      let(:auth_hash){ last_request.env['omniauth.auth'] }
+
+      before(:each) do
+        @adaptor.stub(:filter)
+        @adaptor.stub(:bind_as).and_return(Net::LDAP::Entry.from_single_ldif_string(
+%Q{dn: cn=ping, dc=intridea, dc=com
+userprincipalname: ping@intridea.com
+givenname: Ping
+sn: Yu
+telephonenumber: 555-555-5555
+mobile: 444-444-4444
+uid: ping
+title: dev
+address: k street
+l: Washington
+st: DC
+co: U.S.A
+postofficebox: 20001
+wwwhomepage: www.intridea.com
+jpegphoto: http://www.intridea.com/ping.jpg
+description: omniauth-ldap
+}
+    ))
+      end
+
+      it 'should map user info to Auth Hash' do
+        post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
         auth_hash.uid.should == 'cn=ping, dc=intridea, dc=com'
         auth_hash.info.email.should == 'ping@intridea.com'
         auth_hash.info.first_name.should == 'Ping'