Reformatted specs and reworked way to propagate InvalidCredentials error to application

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,7 +37,7 @@ def request_phase
       def callback_phase
         @adaptor = OmniAuth::LDAP::Adaptor.new @options
 
-        raise MissingCredentialsError.new("Missing login credentials") if request['username'].nil? || request['password'].nil?
+        return fail!(:missing_credentials) if missing_credentials?
         begin
           @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
@@ -81,6 +80,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

spec/omniauth/strategies/ldap_spec.rb

@@ -1,24 +1,25 @@
 require 'spec_helper'
 describe "OmniAuth::Strategies::LDAP" do
-  # :title => "My LDAP", 
+  # :title => "My LDAP",
   # :host => '10.101.10.1',
   # :port => 389,
   # :method => :plain,
   # :base => 'dc=intridea, dc=com',
   # :uid => 'sAMAccountName',
   # :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')}
   # :bind_dn => 'default_bind_dn'
-  # :password => 'password'  
-  class MyLdapProvider < OmniAuth::Strategies::LDAP; end        
-  def app
+  # :password => 'password'
+  class MyLdapProvider < OmniAuth::Strategies::LDAP; end
+
+  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(/@.*$/,'')} 
+      use MyLdapProvider, :name => 'ldap', :title => 'MyLdap Form', :host => '192.168.1.145', :base => 'dc=score, dc=local', :name_proc => Proc.new {|name| name.gsub(/@.*$/,'')}
       run lambda { |env| [404, {'Content-Type' => 'text/plain'}, [env.key?('omniauth.auth').to_s]] }
     }.to_app
   end
 
-  def session
+  let(:session) do
     last_request.env['rack.session']
   end
 
@@ -44,48 +45,108 @@ def session
     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'})
       OmniAuth::LDAP::Adaptor.stub(:new).and_return(@adaptor)
     end
+
     context 'failure' do
-    before(:each) do
-      @adaptor.stub(:bind_as).and_return(false)
-    end
-      it 'should raise MissingCredentialsError' do
-        lambda{post('/auth/ldap/callback', {})}.should raise_error OmniAuth::Strategies::LDAP::MissingCredentialsError
+      before(:each) do
+        @adaptor.stub(:bind_as).and_return(false)
       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{invalid_credentials}
+
+      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
-      it 'should redirect to error page when there is exception' do        
-        @adaptor.stub(:bind_as).and_throw(Exception.new('connection_error'))
-        post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
-        last_response.should be_redirect
-        last_response.headers['Location'].should =~ %r{ldap_error}
+
+      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
+        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 => ['[email protected]'], :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(:bind_as).and_return({
+                                             :dn   => ['cn=ping, dc=intridea, dc=com'],
+                                             :mail => ['[email protected]'],
+                                             :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']})
+
         post('/auth/ldap/callback', {:username => 'ping', :password => 'password'})
       end
-      
-      it 'should raise MissingCredentialsError' do
-        should_not raise_error OmniAuth::Strategies::LDAP::MissingCredentialsError
+
+      it 'should not redirect to error page' do
+        last_response.should_not be_redirect
       end
-      it 'should map user info' do
+
+      it 'should map user info to Auth Hash' do
         auth_hash.uid.should == 'cn=ping, dc=intridea, dc=com'
         auth_hash.info.email.should == '[email protected]'
         auth_hash.info.first_name.should == 'Ping'