Merge pull request ruby-ldap#2 from partnerpedia/better_response_message

Better response message

Author: ivar

lib/net/ldap.rb

@@ -629,11 +629,10 @@ def search(args = {})
         yield entry if block_given?
       }
     else
-      @result = 0
       begin
         conn = Net::LDAP::Connection.new(:host => @host, :port => @port,
                                          :encryption => @encryption)
-        if (@result = conn.bind(args[:auth] || @auth)) == 0
+        if (@result = conn.bind(args[:auth] || @auth)).result_code == 0
           @result = conn.search(args) { |entry|
             result_set << entry if result_set
             yield entry if block_given?
@@ -645,9 +644,9 @@ def search(args = {})
     end
 
     if return_result_set
-      @result == 0 ? result_set : nil
+      (!@result.nil? && @result.result_code == 0) ? result_set : nil
     else
-      @result == 0
+      @result
     end
   end
 
@@ -721,7 +720,7 @@ def bind(auth = @auth)
       end
     end
 
-    @result == 0
+    @result
   end
 
   # #bind_as is for testing authentication credentials.
@@ -816,14 +815,14 @@ def add(args)
       begin
         conn = Connection.new(:host => @host, :port => @port,
                               :encryption => @encryption)
-        if (@result = conn.bind(args[:auth] || @auth)) == 0
+        if (@result = conn.bind(args[:auth] || @auth)).result_code == 0
           @result = conn.add(args)
         end
       ensure
         conn.close if conn
       end
     end
-    @result == 0
+    @result
   end
 
   # Modifies the attribute values of a particular entry on the LDAP
@@ -914,14 +913,15 @@ def modify(args)
       begin
         conn = Connection.new(:host => @host, :port => @port,
                               :encryption => @encryption)
-        if (@result = conn.bind(args[:auth] || @auth)) == 0
+        if (@result = conn.bind(args[:auth] || @auth)).result_code == 0
           @result = conn.modify(args)
         end
       ensure
         conn.close if conn
       end
     end
-    @result == 0
+
+    @result
   end
 
   # Add a value to an attribute. Takes the full DN of the entry to modify,
@@ -985,14 +985,14 @@ def rename(args)
       begin
         conn = Connection.new(:host => @host, :port => @port,
                               :encryption => @encryption)
-        if (@result = conn.bind(args[:auth] || @auth)) == 0
+        if (@result = conn.bind(args[:auth] || @auth)).result_code == 0
           @result = conn.rename(args)
         end
       ensure
         conn.close if conn
       end
     end
-    @result == 0
+    @result
   end
   alias_method :modify_rdn, :rename
 
@@ -1013,14 +1013,14 @@ def delete(args)
       begin
         conn = Connection.new(:host => @host, :port => @port,
                               :encryption => @encryption)
-        if (@result = conn.bind(args[:auth] || @auth)) == 0
+        if (@result = conn.bind(args[:auth] || @auth)).result_code == 0
           @result = conn.delete(args)
         end
       ensure
         conn.close
       end
     end
-    @result == 0
+    @result
   end
 
   # Delete an entry from the LDAP directory along with all subordinate entries.
@@ -1250,7 +1250,7 @@ def bind_simple(auth)
 
     (be = @conn.read_ber(Net::LDAP::AsnSyntax) and pdu = Net::LDAP::PDU.new(be)) or raise Net::LDAP::LdapError, "no bind result"
 
-    pdu.result_code
+    pdu
   end
 
   #--
@@ -1288,7 +1288,7 @@ def bind_sasl(auth)
       @conn.write request_pkt
 
       (be = @conn.read_ber(Net::LDAP::AsnSyntax) and pdu = Net::LDAP::PDU.new(be)) or raise Net::LDAP::LdapError, "no bind result"
-      return pdu.result_code unless pdu.result_code == 14 # saslBindInProgress
+      return pdu unless pdu.result_code == 14 # saslBindInProgress
       raise Net::LDAP::LdapError, "sasl-challenge overflow" if ((n += 1) > MaxSaslChallenges)
 
       cred = chall.call(pdu.result_server_sasl_creds)
@@ -1374,7 +1374,7 @@ def search(args = {})
     # to do a root-DSE record search and not do a paged search if the LDAP
     # doesn't support it. Yuck.
     rfc2696_cookie = [126, ""]
-    result_code = 0
+    result_pdu = nil
     n_results = 0
 
     loop {
@@ -1413,7 +1413,7 @@ def search(args = {})
       pkt = [next_msgid.to_ber, request, controls].to_ber_sequence
       @conn.write pkt
 
-      result_code = 0
+      result_pdu = nil
       controls = []
 
       while (be = @conn.read_ber(Net::LDAP::AsnSyntax)) && (pdu = Net::LDAP::PDU.new(be))
@@ -1430,7 +1430,7 @@ def search(args = {})
             end
           end
         when 5 # search-result
-          result_code = pdu.result_code
+          result_pdu = pdu
           controls = pdu.result_controls
           break
         else
@@ -1449,7 +1449,7 @@ def search(args = {})
       # of type OCTET STRING, covered in the default syntax supported by
       # read_ber, so I guess we're ok.
       more_pages = false
-      if result_code == 0 and controls
+      if result_pdu.result_code == 0 and controls
         controls.each do |c|
           if c.oid == Net::LDAP::LDAPControls::PAGED_RESULTS
             # just in case some bogus server sends us more than 1 of these.
@@ -1468,7 +1468,7 @@ def search(args = {})
       break unless more_pages
     } # loop
 
-    result_code
+    result_pdu || OpenStruct.new(:status => :failure, :result_code => 1, :message => "Invalid search")
   end
 
   MODIFY_OPERATIONS = { #:nodoc:
@@ -1508,7 +1508,8 @@ def modify(args)
     @conn.write pkt
 
     (be = @conn.read_ber(Net::LDAP::AsnSyntax)) && (pdu = Net::LDAP::PDU.new(be)) && (pdu.app_tag == 7) or raise Net::LDAP::LdapError, "response missing or invalid"
-    pdu.result_code
+
+    pdu
   end
 
   #--
@@ -1529,8 +1530,12 @@ def add(args)
     pkt = [next_msgid.to_ber, request].to_ber_sequence
     @conn.write pkt
 
-    (be = @conn.read_ber(Net::LDAP::AsnSyntax)) && (pdu = Net::LDAP::PDU.new(be)) && (pdu.app_tag == 9) or raise Net::LDAP::LdapError, "response missing or invalid"
-    pdu.result_code
+    (be = @conn.read_ber(Net::LDAP::AsnSyntax)) &&
+      (pdu = Net::LDAP::PDU.new(be)) &&
+      (pdu.app_tag == 9) or
+      raise Net::LDAP::LdapError, "response missing or invalid"
+
+    pdu
   end
 
   #--
@@ -1551,7 +1556,8 @@ def rename args
     (be = @conn.read_ber(Net::LDAP::AsnSyntax)) &&
     (pdu = Net::LDAP::PDU.new( be )) && (pdu.app_tag == 13) or
     raise Net::LDAP::LdapError.new( "response missing or invalid" )
-    pdu.result_code
+
+    pdu
   end
 
   #--
@@ -1565,6 +1571,7 @@ def delete(args)
     @conn.write pkt
 
     (be = @conn.read_ber(Net::LDAP::AsnSyntax)) && (pdu = Net::LDAP::PDU.new(be)) && (pdu.app_tag == 11) or raise Net::LDAP::LdapError, "response missing or invalid"
-    pdu.result_code
+
+    pdu
   end
 end # class Connection

lib/net/ldap/pdu.rb

@@ -112,6 +112,10 @@ def result
     @ldap_result || {}
   end
 
+  def error_message
+    result[:errorMessage] || ""
+  end
+
   ##
   # This returns an LDAP result code taken from the PDU, but it will be nil
   # if there wasn't a result code. That can easily happen depending on the
@@ -120,6 +124,18 @@ def result_code(code = :resultCode)
     @ldap_result and @ldap_result[code]
   end
 
+  def status
+    result_code == 0 ? :success : :failure
+  end
+
+  def success?
+    status == :success
+  end
+
+  def failure?
+    !success?
+  end
+
   ##
   # Return serverSaslCreds, which are only present in BindResponse packets.
   #--

spec/unit/ldap/search_spec.rb

@@ -3,8 +3,7 @@
 describe Net::LDAP, "search method" do
   class FakeConnection
     def search(args)
-      error_code = 1
-      return error_code
+      OpenStruct.new(:result_code => 1, :message => "error")
     end
   end
 
@@ -22,8 +21,8 @@ def search(args)
 
   context "when :return_result => false" do
     it "should return false upon error" do
-      success = @connection.search(:return_result => false)
-      success.should == false
+      result = @connection.search(:return_result => false)
+      result.result_code.should == 1
     end
   end
 

spec/unit/ldap_spec.rb

@@ -7,11 +7,11 @@
         flexmock(TCPSocket).
           should_receive(:new).and_raise(Errno::ECONNREFUSED)
       end
-      
+
       it "should raise LdapError" do
         lambda {
           Net::LDAP::Connection.new(
-            :server => 'test.mocked.com', 
+            :server => 'test.mocked.com',
             :port   => 636)
         }.should raise_error(Net::LDAP::LdapError)
       end
@@ -21,11 +21,11 @@
         flexmock(TCPSocket).
           should_receive(:new).and_raise(SocketError)
       end
-      
+
       it "should raise LdapError" do
         lambda {
           Net::LDAP::Connection.new(
-            :server => 'test.mocked.com', 
+            :server => 'test.mocked.com',
             :port   => 636)
         }.should raise_error(Net::LDAP::LdapError)
       end
@@ -35,14 +35,44 @@
         flexmock(TCPSocket).
           should_receive(:new).and_raise(NameError)
       end
-      
+
       it "should rethrow the exception" do
         lambda {
           Net::LDAP::Connection.new(
-            :server => 'test.mocked.com', 
+            :server => 'test.mocked.com',
             :port   => 636)
         }.should raise_error(NameError)
       end
     end
   end
-end
+
+  context "populate error messages" do
+    before do
+      @tcp_socket = flexmock(:connection)
+      @tcp_socket.should_receive(:write)
+      flexmock(TCPSocket).should_receive(:new).and_return(@tcp_socket)
+    end
+
+    subject { Net::LDAP::Connection.new(:server => 'test.mocked.com', :port => 636) }
+
+    it "should get back error messages if operation fails" do
+      ber = Net::BER::BerIdentifiedArray.new([53, "", "The provided password value was rejected by a password validator:  The provided password did not contain enough characters from the character set 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'.  The minimum number of characters from that set that must be present in user passwords is 1"])
+      ber.ber_identifier = 7
+      @tcp_socket.should_receive(:read_ber).and_return([2, ber])
+
+      result = subject.modify(:dn => "1", :operations => [[:replace, "mail", "[email protected]"]])
+      result.should be_failure
+      result.error_message.should == "The provided password value was rejected by a password validator:  The provided password did not contain enough characters from the character set 'ABCDEFGHIJKLMNOPQRSTUVWXYZ'.  The minimum number of characters from that set that must be present in user passwords is 1"
+    end
+
+    it "shouldn't get back error messages if operation succeeds" do
+      ber = Net::BER::BerIdentifiedArray.new([0, "", ""])
+      ber.ber_identifier = 7
+      @tcp_socket.should_receive(:read_ber).and_return([2, ber])
+
+      result = subject.modify(:dn => "1", :operations => [[:replace, "mail", "[email protected]"]])
+      result.should be_success
+      result.error_message.should == ""
+    end
+  end
+end