Skip to content

Specific errors subclassing Net::LDAP::Error #183

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jan 16, 2015
5 changes: 2 additions & 3 deletions lib/net/ldap.rb
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ class LDAP
require 'net/ldap/instrumentation'
require 'net/ldap/connection'
require 'net/ldap/version'
require 'net/ldap/error'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 to putting this in it's own file


# == Quick-start for the Impatient
# === Quick Example of a user-authentication against an LDAP directory:
Expand Down Expand Up @@ -246,8 +247,6 @@ class LDAP
class Net::LDAP
include Net::LDAP::Instrumentation

class LdapError < StandardError; end

SearchScope_BaseObject = 0
SearchScope_SingleLevel = 1
SearchScope_WholeSubtree = 2
Expand Down Expand Up @@ -666,7 +665,7 @@ def open
# anything with the bind results. We then pass self to the caller's
# block, where he will execute his LDAP operations. Of course they will
# all generate auth failures if the bind was unsuccessful.
raise Net::LDAP::LdapError, "Open already in progress" if @open_connection
raise Net::LDAP::AlreadyOpenedError, "Open already in progress" if @open_connection

instrument "open.net_ldap" do |payload|
begin
Expand Down
54 changes: 27 additions & 27 deletions lib/net/ldap/connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,13 @@ def initialize(server)
begin
@conn = server[:socket] || TCPSocket.new(server[:host], server[:port])
rescue SocketError
raise Net::LDAP::LdapError, "No such address or other socket error."
raise Net::LDAP::Error, "No such address or other socket error."
rescue Errno::ECONNREFUSED
raise Net::LDAP::LdapError, "Server #{server[:host]} refused connection on port #{server[:port]}."
raise Net::LDAP::Error, "Server #{server[:host]} refused connection on port #{server[:port]}."
rescue Errno::EHOSTUNREACH => error
raise Net::LDAP::LdapError, "Host #{server[:host]} was unreachable (#{error.message})"
raise Net::LDAP::Error, "Host #{server[:host]} was unreachable (#{error.message})"
rescue Errno::ETIMEDOUT
raise Net::LDAP::LdapError, "Connection to #{server[:host]} timed out."
raise Net::LDAP::Error, "Connection to #{server[:host]} timed out."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we're making changes to this, I propose removing this rescue block entirely. The stdlib's SocketError is already descriptive about the specifics of the exception; We're not adding any additional information by wrapping it in our own custom error class. The change we would make is to document where the API may raise a SocketError.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jch definitely open to that, but let's tackle that separately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jch 's idea sounds good 👍 but I agree with @mtodd, the topic looks beyond the scope of this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤘

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the meantime, should we be using error classes for these errors? Or are we OK with keeping Net::LDAP::Error here?

end

if server[:encryption]
Expand All @@ -42,7 +42,7 @@ def close
end

def self.wrap_with_ssl(io, tls_options = {})
raise Net::LDAP::LdapError, "OpenSSL is unavailable" unless Net::LDAP::HasOpenSSL
raise Net::LDAP::NoOpenSSLError, "OpenSSL is unavailable" unless Net::LDAP::HasOpenSSL

ctx = OpenSSL::SSL::SSLContext.new

Expand All @@ -67,7 +67,7 @@ def self.wrap_with_ssl(io, tls_options = {})
# successfully-opened @conn instance variable, which is a TCP connection.
# Depending on the received arguments, we establish SSL, potentially
# replacing the value of @conn accordingly. Don't generate any errors here
# if no encryption is requested. DO raise Net::LDAP::LdapError objects if encryption
# if no encryption is requested. DO raise Net::LDAP::Error objects if encryption
# is requested and we have trouble setting it up. That includes if OpenSSL
# is not set up on the machine. (Question: how does the Ruby OpenSSL
# wrapper react in that case?) DO NOT filter exceptions raised by the
Expand Down Expand Up @@ -105,16 +105,16 @@ def setup_encryption(args)
pdu = queued_read(message_id)

if pdu.nil? || pdu.app_tag != Net::LDAP::PDU::ExtendedResponse
raise Net::LDAP::LdapError, "no start_tls result"
raise Net::LDAP::NoStartTLSResultError, "no start_tls result"
end

if pdu.result_code.zero?
@conn = self.class.wrap_with_ssl(@conn, args[:tls_options])
else
raise Net::LDAP::LdapError, "start_tls failed: #{pdu.result_code}"
raise Net::LDAP::StartTlSError, "start_tls failed: #{pdu.result_code}"
end
else
raise Net::LDAP::LdapError, "unsupported encryption method #{args[:method]}"
raise Net::LDAP::EncMethodUnsupportedError, "unsupported encryption method #{args[:method]}"
end
end

Expand Down Expand Up @@ -225,7 +225,7 @@ def bind(auth)
elsif meth == :gss_spnego
bind_gss_spnego(auth)
else
raise Net::LDAP::LdapError, "Unsupported auth method (#{meth})"
raise Net::LDAP::AuthMethodUnsupportedError, "Unsupported auth method (#{meth})"
end
end
end
Expand All @@ -241,7 +241,7 @@ def bind_simple(auth)
["", ""]
end

raise Net::LDAP::LdapError, "Invalid binding information" unless (user && psw)
raise Net::LDAP::BindingInformationInvalidError, "Invalid binding information" unless (user && psw)

message_id = next_msgid
request = [
Expand All @@ -253,7 +253,7 @@ def bind_simple(auth)
pdu = queued_read(message_id)

if !pdu || pdu.app_tag != Net::LDAP::PDU::BindResult
raise Net::LDAP::LdapError, "no bind result"
raise Net::LDAP::NoBindResultError, "no bind result"
end

pdu
Expand Down Expand Up @@ -283,7 +283,7 @@ def bind_simple(auth)
def bind_sasl(auth)
mech, cred, chall = auth[:mechanism], auth[:initial_credential],
auth[:challenge_response]
raise Net::LDAP::LdapError, "Invalid binding information" unless (mech && cred && chall)
raise Net::LDAP::BindingInformationInvalidError, "Invalid binding information" unless (mech && cred && chall)

message_id = next_msgid

Expand All @@ -298,16 +298,16 @@ def bind_sasl(auth)
pdu = queued_read(message_id)

if !pdu || pdu.app_tag != Net::LDAP::PDU::BindResult
raise Net::LDAP::LdapError, "no bind result"
raise Net::LDAP::NoBindResultError, "no bind result"
end

return pdu unless pdu.result_code == Net::LDAP::ResultCodeSaslBindInProgress
raise Net::LDAP::LdapError, "sasl-challenge overflow" if ((n += 1) > MaxSaslChallenges)
raise Net::LDAP::SASLChallengeOverflowError, "sasl-challenge overflow" if ((n += 1) > MaxSaslChallenges)

cred = chall.call(pdu.result_server_sasl_creds)
}

raise Net::LDAP::LdapError, "why are we here?"
raise Net::LDAP::SASLChallengeOverflowError, "why are we here?"
end
private :bind_sasl

Expand All @@ -326,7 +326,7 @@ def bind_gss_spnego(auth)
require 'ntlm'

user, psw = [auth[:username] || auth[:dn], auth[:password]]
raise Net::LDAP::LdapError, "Invalid binding information" unless (user && psw)
raise Net::LDAP::BindingInformationInvalidError, "Invalid binding information" unless (user && psw)

nego = proc { |challenge|
t2_msg = NTLM::Message.parse(challenge)
Expand Down Expand Up @@ -412,10 +412,10 @@ def search(args = nil)
sort = args.fetch(:sort_controls, false)

# arg validation
raise Net::LDAP::LdapError, "search base is required" unless base
raise Net::LDAP::LdapError, "invalid search-size" unless size >= 0
raise Net::LDAP::LdapError, "invalid search scope" unless Net::LDAP::SearchScopes.include?(scope)
raise Net::LDAP::LdapError, "invalid alias dereferencing value" unless Net::LDAP::DerefAliasesArray.include?(deref)
raise ArgumentError, "search base is required" unless base
raise ArgumentError, "invalid search-size" unless size >= 0
raise ArgumentError, "invalid search scope" unless Net::LDAP::SearchScopes.include?(scope)
raise ArgumentError, "invalid alias dereferencing value" unless Net::LDAP::DerefAliasesArray.include?(deref)

# arg transforms
filter = Net::LDAP::Filter.construct(filter) if filter.is_a?(String)
Expand Down Expand Up @@ -527,7 +527,7 @@ def search(args = nil)
end
break
else
raise Net::LDAP::LdapError, "invalid response-type in search: #{pdu.app_tag}"
raise Net::LDAP::ResponseTypeInvalidError, "invalid response-type in search: #{pdu.app_tag}"
end
end

Expand Down Expand Up @@ -625,7 +625,7 @@ def modify(args)
pdu = queued_read(message_id)

if !pdu || pdu.app_tag != Net::LDAP::PDU::ModifyResponse
raise Net::LDAP::LdapError, "response missing or invalid"
raise Net::LDAP::ResponseMissingOrInvalidError, "response missing or invalid"
end

pdu
Expand All @@ -639,7 +639,7 @@ def modify(args)
# to the error message and the matched-DN returned by the server.
#++
def add(args)
add_dn = args[:dn] or raise Net::LDAP::LdapError, "Unable to add empty DN"
add_dn = args[:dn] or raise Net::LDAP::EmptyDNError, "Unable to add empty DN"
add_attrs = []
a = args[:attributes] and a.each { |k, v|
add_attrs << [ k.to_s.to_ber, Array(v).map { |m| m.to_ber}.to_ber_set ].to_ber_sequence
Expand All @@ -652,7 +652,7 @@ def add(args)
pdu = queued_read(message_id)

if !pdu || pdu.app_tag != Net::LDAP::PDU::AddResponse
raise Net::LDAP::LdapError, "response missing or invalid"
raise Net::LDAP::ResponseMissingError, "response missing or invalid"
end

pdu
Expand All @@ -675,7 +675,7 @@ def rename(args)
pdu = queued_read(message_id)

if !pdu || pdu.app_tag != Net::LDAP::PDU::ModifyRDNResponse
raise Net::LDAP::LdapError.new "response missing or invalid"
raise Net::LDAP::ResponseMissingOrInvalidError.new "response missing or invalid"
end

pdu
Expand All @@ -694,7 +694,7 @@ def delete(args)
pdu = queued_read(message_id)

if !pdu || pdu.app_tag != Net::LDAP::PDU::DeleteResponse
raise Net::LDAP::LdapError, "response missing or invalid"
raise Net::LDAP::ResponseMissingOrInvalidError, "response missing or invalid"
end

pdu
Expand Down
2 changes: 1 addition & 1 deletion lib/net/ldap/entry.rb
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@ def from_single_ldif_string(ldif)

return nil if ds.empty?

raise Net::LDAP::LdapError, "Too many LDIF entries" unless ds.size == 1
raise Net::LDAP::EntryOverflowError, "Too many LDIF entries" unless ds.size == 1

entry = ds.to_entries.first

Expand Down
38 changes: 38 additions & 0 deletions lib/net/ldap/error.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
class Net::LDAP
class LdapError < StandardError
def message
"Deprecation warning: Net::LDAP::LdapError is no longer used. Use Net::LDAP::Error or rescue one of it's subclasses. \n" + super
end
end

class Error < StandardError; end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for switching the name from LdapError to just Error. Does anyone have strong opinions on having a backwards compatibility placeholder?

class LdapError < Error
  def message
    "Deprecation warning: Net::LDAP::LdapError is no longer used. Use Net::LDAP::Error or rescue one of it's subclasses. [link to this file]\n" + super
  end
end

We're pre 1.0, there's no obligation for us to do this. I'm fine with either because it's an easy error to fix if anyone runs into it and we can document it in History.md

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jch I support the rename and keeping around the backwards compatible constant, though the semantics of the backwards compatible constant is a bit trickier than inheriting from Error. What needs to be backwards compatible is this:

begin
  # something that could fail
rescue Net::LDAP::LdapError => error
  # handle error
end

The problem with the above is that it wouldn't actually catch anything that inherits from Net::LDAP::Error, only the specific Net::LDAP::LdapError subclass. Instead, I think we can just have a constant assignment that would work as expected:

class Net::LDAP
  class Error < StandardError; end
  LdapError = Error
end

begin
rescue Net::LDAP::LdapError
  # should work
end

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtodd good eye. What about having Error inherit from LdapError for a few point releases so that we can add a deprecation warning?

class Net::LDAP
  class LdapError < StandardError
    def message
      "Deprecation warning... " + super
    end
  end

  class Error < LdapError; end
end

This allows us to eventually remove LdapError.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would work!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


class AlreadyOpenedError < Error; end
class SocketError < Error; end
class ConnectionRefusedError < Error; end
class NoOpenSSLError < Error; end
class NoStartTLSResultError < Error; end
class NoSearchBaseError < Error; end
class StartTLSError < Error; end
class EncryptionUnsupportedError < Error; end
class EncMethodUnsupportedError < Error; end
class AuthMethodUnsupportedError < Error; end
class BindingInformationInvalidError < Error; end
class NoBindResultError < Error; end
class SASLChallengeOverflowError < Error; end
class SearchSizeInvalidError < Error; end
class SearchScopeInvalidError < Error; end
class ResponseTypeInvalidError < Error; end
class ResponseMissingOrInvalidError < Error; end
class EmptyDNError < Error; end
class HashTypeUnsupportedError < Error; end
class OperatorError < Error; end
class SubstringFilterError < Error; end
class SearchFilterError < Error; end
class BERInvalidError < Error; end
class SearchFilterTypeUnknownError < Error; end
class BadAttributeError < Error; end
class FilterTypeUnknownError < Error; end
class FilterSyntaxInvalidError < Error; end
class EntryOverflowError < Error; end
end
16 changes: 8 additions & 8 deletions lib/net/ldap/filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class Net::LDAP::Filter

def initialize(op, left, right) #:nodoc:
unless FilterTypes.include?(op)
raise Net::LDAP::LdapError, "Invalid or unsupported operator #{op.inspect} in LDAP Filter."
raise Net::LDAP::OperatorError, "Invalid or unsupported operator #{op.inspect} in LDAP Filter."
end
@op = op
@left = left
Expand Down Expand Up @@ -290,7 +290,7 @@ def parse_ber(ber)
ber.last.each { |b|
case b.ber_identifier
when 0x80 # context-specific primitive 0, SubstringFilter "initial"
raise Net::LDAP::LdapError, "Unrecognized substring filter; bad initial value." if str.length > 0
raise Net::LDAP::SubstringFilterError, "Unrecognized substring filter; bad initial value." if str.length > 0
str += escape(b)
when 0x81 # context-specific primitive 0, SubstringFilter "any"
str += "*#{escape(b)}"
Expand All @@ -309,7 +309,7 @@ def parse_ber(ber)
# call to_s to get rid of the BER-identifiedness of the incoming string.
present?(ber.to_s)
when 0xa9 # context-specific constructed 9, "extensible comparison"
raise Net::LDAP::LdapError, "Invalid extensible search filter, should be at least two elements" if ber.size<2
raise Net::LDAP::SearchFilterError, "Invalid extensible search filter, should be at least two elements" if ber.size < 2

# Reassembles the extensible filter parts
# (["sn", "2.4.6.8.10", "Barbara Jones", '1'])
Expand All @@ -330,7 +330,7 @@ def parse_ber(ber)

ex(attribute, value)
else
raise Net::LDAP::LdapError, "Invalid BER tag-value (#{ber.ber_identifier}) in search filter."
raise Net::LDAP::BERInvalidError, "Invalid BER tag-value (#{ber.ber_identifier}) in search filter."
end
end

Expand All @@ -357,7 +357,7 @@ def parse_ldap_filter(obj)
when 0xa3 # equalityMatch. context-specific constructed 3.
eq(obj[0], obj[1])
else
raise Net::LDAP::LdapError, "Unknown LDAP search-filter type: #{obj.ber_identifier}"
raise Net::LDAP::SearchFilterTypeUnknownError, "Unknown LDAP search-filter type: #{obj.ber_identifier}"
end
end
end
Expand Down Expand Up @@ -532,7 +532,7 @@ def to_ber
seq = []

unless @left =~ /^([-;\w]*)(:dn)?(:(\w+|[.\w]+))?$/
raise Net::LDAP::LdapError, "Bad attribute #{@left}"
raise Net::LDAP::BadAttributeError, "Bad attribute #{@left}"
end
type, dn, rule = $1, $2, $4

Expand Down Expand Up @@ -639,7 +639,7 @@ def match(entry)
l = entry[@left] and l = Array(l) and l.index(@right)
end
else
raise Net::LDAP::LdapError, "Unknown filter type in match: #{@op}"
raise Net::LDAP::FilterTypeUnknownError, "Unknown filter type in match: #{@op}"
end
end

Expand Down Expand Up @@ -671,7 +671,7 @@ def parse(ldap_filter_string)
def initialize(str)
require 'strscan' # Don't load strscan until we need it.
@filter = parse(StringScanner.new(str))
raise Net::LDAP::LdapError, "Invalid filter syntax." unless @filter
raise Net::LDAP::FilterSyntaxInvalidError, "Invalid filter syntax." unless @filter
end

##
Expand Down
10 changes: 5 additions & 5 deletions lib/net/ldap/password.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ class << self
def generate(type, str)
case type
when :md5
attribute_value = '{MD5}' + Base64.encode64(Digest::MD5.digest(str)).chomp!
attribute_value = '{MD5}' + Base64.encode64(Digest::MD5.digest(str)).chomp!
when :sha
attribute_value = '{SHA}' + Base64.encode64(Digest::SHA1.digest(str)).chomp!
attribute_value = '{SHA}' + Base64.encode64(Digest::SHA1.digest(str)).chomp!
when :ssha
salt = SecureRandom.random_bytes(16)
attribute_value = '{SSHA}' + Base64.encode64(Digest::SHA1.digest(str + salt) + salt).chomp!
salt = SecureRandom.random_bytes(16)
attribute_value = '{SSHA}' + Base64.encode64(Digest::SHA1.digest(str + salt) + salt).chomp!
else
raise Net::LDAP::LdapError, "Unsupported password-hash type (#{type})"
raise Net::LDAP::HashTypeUnsupportedError, "Unsupported password-hash type (#{type})"
end
return attribute_value
end
Expand Down
4 changes: 2 additions & 2 deletions test/test_filter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ def test_bug_7534_rfc2254
end

def test_invalid_filter_string
assert_raises(Net::LDAP::LdapError) { Filter.from_rfc2254("") }
assert_raises(Net::LDAP::FilterSyntaxInvalidError) { Filter.from_rfc2254("") }
end

def test_invalid_filter
assert_raises(Net::LDAP::LdapError) {
assert_raises(Net::LDAP::OperatorError) {
# This test exists to prove that our constructor blocks unknown filter
# types. All filters must be constructed using helpers.
Filter.__send__(:new, :xx, nil, nil)
Expand Down
4 changes: 2 additions & 2 deletions test/test_ldap_connection.rb
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@

class TestLDAPConnection < Test::Unit::TestCase
def test_unresponsive_host
assert_raise Net::LDAP::LdapError do
assert_raise Net::LDAP::Error do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
end
end

def test_blocked_port
flexmock(TCPSocket).should_receive(:new).and_raise(SocketError)
assert_raise Net::LDAP::LdapError do
assert_raise Net::LDAP::Error do
Net::LDAP::Connection.new(:host => 'test.mocked.com', :port => 636)
end
end
Expand Down