From 8cf06692f2bba812922ae3419e8633580718e6e2 Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Sat, 12 Sep 2026 00:10:41 -0400 Subject: [PATCH 1/3] Reuse successful LDAP base searches Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5b8ed060-ed02-4402-b5e1-af407bcb2f61 --- lib/entitlements/service/ldap.rb | 30 ++++++++++++--- spec/unit/entitlements/service/ldap_spec.rb | 42 ++++++++++++++++++++- 2 files changed, 64 insertions(+), 8 deletions(-) diff --git a/lib/entitlements/service/ldap.rb b/lib/entitlements/service/ldap.rb index cec36a7..c609f10 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 @@ -215,11 +225,19 @@ 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 + + # 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/spec/unit/entitlements/service/ldap_spec.rb b/spec/unit/entitlements/service/ldap_spec.rb index bcb9cb2..7f215dd 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 From 09c3c84599d66af3246a8e7213444135cdf36ade Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Mon, 14 Sep 2026 14:19:43 -0400 Subject: [PATCH 2/3] Invalidate LDAP caches after deletion Clear both known-existence and read caches after a successful LDAP delete so later checks re-read the directory. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f79283a-9bd3-46dd-8e33-676d145619c5 --- lib/entitlements/service/ldap.rb | 10 +++++- spec/unit/entitlements/service/ldap_spec.rb | 38 +++++++++++++++++++++ 2 files changed, 47 insertions(+), 1 deletion(-) diff --git a/lib/entitlements/service/ldap.rb b/lib/entitlements/service/ldap.rb index c609f10..067d164 100644 --- a/lib/entitlements/service/ldap.rb +++ b/lib/entitlements/service/ldap.rb @@ -190,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 @@ -233,6 +236,11 @@ 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. diff --git a/spec/unit/entitlements/service/ldap_spec.rb b/spec/unit/entitlements/service/ldap_spec.rb index 7f215dd..0976964 100644 --- a/spec/unit/entitlements/service/ldap_spec.rb +++ b/spec/unit/entitlements/service/ldap_spec.rb @@ -395,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) From 5ab1b9a9e8663ef4fbd21ecc1108dc87253989e2 Mon Sep 17 00:00:00 2001 From: Stephen Hosom Date: Mon, 14 Sep 2026 14:22:57 -0400 Subject: [PATCH 3/3] Bump entitlements-app to 1.2.3 Prepare the LDAP cache optimization for publication on merge. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3f79283a-9bd3-46dd-8e33-676d145619c5 --- Gemfile.lock | 2 +- lib/version.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) 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/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