Reuse successful LDAP base searches

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)

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

lib/version.rb

@@ -2,6 +2,6 @@
 
 module Entitlements
   module Version
-    VERSION = "1.2.2"
+    VERSION = "1.2.3"
   end
 end

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)