diff --git a/Gemfile.lock b/Gemfile.lock index 56afec2..5189114 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,7 +1,7 @@ PATH remote: . specs: - entitlements-app (1.2.2) + entitlements-app (1.2.3) concurrent-ruby (~> 1.3, >= 1.3.1) dogstatsd-ruby (~> 5.7) faraday (~> 2.0) diff --git a/lib/entitlements/service/ldap.rb b/lib/entitlements/service/ldap.rb index cec36a7..067d164 100644 --- a/lib/entitlements/service/ldap.rb +++ b/lib/entitlements/service/ldap.rb @@ -17,9 +17,9 @@ class WTFError < RuntimeError; end # This keeps the schema happy. attr_reader :binddn, :person_dn_format - # Constructor-like object that ensures only one LDAP object (and hence connection) - # is made for a given LDAP server, for efficiency sake. Takes the same parameters as - # the constructor and returns the same object type. + # Constructor-like object that ensures only one LDAP service object is made for a + # given LDAP server. Takes the same parameters as the constructor and returns the + # same object type. # # addr - URL of LDAP server e.g. ldaps://ldap.example.net:636 # binddn - DN to bind with @@ -80,6 +80,8 @@ def initialize(addr:, binddn:, bindpw:, ca_file: ENV["LDAP_CACERT"], disable_ssl @ca_file = ca_file @disable_ssl_verification = disable_ssl_verification @person_dn_format = person_dn_format + @known_existing_dns = {} + @known_existing_dns_mutex = Mutex.new end # Read a single entry identified by its DN and return the value. Returns nil if @@ -119,7 +121,13 @@ def search(base:, filter: nil, attrs: "*", index: :dn, scope: Net::LDAP::SearchS downcased_attrs = attrs == "*" ? "*" : attrs.map { |a| a.downcase } result = {} - ldap.search(base: base, filter: filter, attributes: downcased_attrs, scope: scope, return_result: false) do |entry| + search_succeeded = ldap.search( + base: base, + filter: filter, + attributes: downcased_attrs, + scope: scope, + return_result: false + ) do |entry| result_key = index == :dn ? entry.dn : entry[index] unless result_key raise EntryError, "#{entry.dn} has no value for #{index.inspect}" @@ -132,6 +140,7 @@ def search(base:, filter: nil, attrs: "*", index: :dn, scope: Net::LDAP::SearchS result[result_key] = entry end + remember_existing_dn(base) if search_succeeded Entitlements.logger.debug "Completed search: #{result.keys.size} result(s)" @@ -145,6 +154,7 @@ def search(base:, filter: nil, attrs: "*", index: :dn, scope: Net::LDAP::SearchS # Returns true if the entry exists, false otherwise. Contract String => C::Bool def exists?(dn) + return true if known_existing_dn?(dn) read(dn).is_a?(Net::LDAP::Entry) end @@ -180,7 +190,10 @@ def delete(dn) ldap.delete(dn: dn) operation_result = ldap.get_operation_result - return true if operation_result["code"] == 0 + if operation_result["code"] == 0 + forget_dn(dn) + return true + end Entitlements.logger.error "Error deleting #{dn}: #{operation_result['message']}" false end @@ -215,11 +228,24 @@ def modify(dn, updates) attr_reader :addr, :bindpw - # The LDAP object is initialized and bound on demand the first time it's called. + def known_existing_dn?(dn) + @known_existing_dns_mutex.synchronize { @known_existing_dns.key?(dn) } + end + + def remember_existing_dn(dn) + @known_existing_dns_mutex.synchronize { @known_existing_dns[dn] = true } + end + + def forget_dn(dn) + @known_existing_dns_mutex.synchronize { @known_existing_dns.delete(dn) } + @dn_cache&.delete(dn) + end + + # The LDAP object is initialized and its credentials are validated on demand. # # Takes no arguments. # - # Returns a Net::LDAP object that is connected and bound. + # Returns a Net::LDAP object configured to connect and bind for each operation. Contract C::None => Net::LDAP def ldap @ldap ||= begin diff --git a/lib/version.rb b/lib/version.rb index 06191dc..cd6c58b 100644 --- a/lib/version.rb +++ b/lib/version.rb @@ -2,6 +2,6 @@ module Entitlements module Version - VERSION = "1.2.2" + VERSION = "1.2.3" end end diff --git a/spec/unit/entitlements/service/ldap_spec.rb b/spec/unit/entitlements/service/ldap_spec.rb index bcb9cb2..0976964 100644 --- a/spec/unit/entitlements/service/ldap_spec.rb +++ b/spec/unit/entitlements/service/ldap_spec.rb @@ -296,7 +296,7 @@ end describe "#exists?" do - it "returns false if the entry does not exist" do + it "returns true if the entry exists" do allow(entry1).to receive(:dn).and_return(dn1) expect(subject).to receive(:ldap).and_return(ldap) expect(ldap).to receive(:search) @@ -306,13 +306,51 @@ end - it "returns true if the entry exists" do + it "returns false if the entry does not exist" do allow(entry1).to receive(:dn).and_return(dn1) expect(subject).to receive(:ldap).and_return(ldap) expect(ldap).to receive(:search) .with(base: dn1, filter: nil, attributes: "*", scope: Net::LDAP::SearchScope_BaseObject, return_result: false) expect(subject.exists?(dn1)).to eq(false) end + + it "reuses the known existence of a successfully searched base" do + expect(subject).to receive(:ldap).and_return(ldap) + expect(ldap).to receive(:search) + .with( + base: dn1, + filter: nil, + attributes: "*", + scope: Net::LDAP::SearchScope_WholeSubtree, + return_result: false + ).and_return(true) + + expect(subject.search(base: dn1)).to eq({}) + expect(subject.exists?(dn1)).to eq(true) + end + + it "does not remember the base of a failed search" do + expect(subject).to receive(:ldap).twice.and_return(ldap) + expect(ldap).to receive(:search) + .with( + base: dn1, + filter: nil, + attributes: "*", + scope: Net::LDAP::SearchScope_WholeSubtree, + return_result: false + ).and_return(false) + expect(ldap).to receive(:search) + .with( + base: dn1, + filter: nil, + attributes: "*", + scope: Net::LDAP::SearchScope_BaseObject, + return_result: false + ).and_return(false) + + expect(subject.search(base: dn1)).to eq({}) + expect(subject.exists?(dn1)).to eq(false) + end end describe "#upsert" do @@ -357,6 +395,44 @@ expect(subject.delete(dn)).to eq(true) end + it "invalidates cached existence and reads after a successful delete" do + operation_result = { "code" => 0, "message" => ":tada:" } + allow(subject).to receive(:ldap).and_return(ldap) + allow(existing).to receive(:dn).and_return(dn) + + expect(ldap).to receive(:search) + .with( + base: dn, + filter: nil, + attributes: "*", + scope: Net::LDAP::SearchScope_WholeSubtree, + return_result: false + ).and_return(true) + expect(ldap).to receive(:search) + .with( + base: dn, + filter: nil, + attributes: "*", + scope: Net::LDAP::SearchScope_BaseObject, + return_result: false + ).and_yield(existing).and_return(true) + expect(ldap).to receive(:delete).with(dn: dn) + expect(ldap).to receive(:get_operation_result).and_return(operation_result) + expect(ldap).to receive(:search) + .with( + base: dn, + filter: nil, + attributes: "*", + scope: Net::LDAP::SearchScope_BaseObject, + return_result: false + ).and_return(false) + + expect(subject.search(base: dn)).to eq({}) + expect(subject.read(dn)).to eq(existing) + expect(subject.delete(dn)).to eq(true) + expect(subject.exists?(dn)).to eq(false) + end + it "returns false when the call fails" do operation_result = { "code" => 1, "message" => ":crying_cat_face:" } allow(subject).to receive(:ldap).and_return(ldap)