bugfix: 404 if no key matches, fixes #6789

I also separated the parsing of the hkp response from
FetchKeyInfo.

This way FetchKeyInfo has the EM specific code that has
sideeffects and the logic is in a class without sideeffects
and (almost) without state.

The only state we keep is the KeyInfo array that contains
all the information the server returns. This way we avoid
parsing the response multiple times.

3 files changed, 102 insertions, 52 deletions

diff --git a/lib/nickserver.rb b/lib/nickserver.rb
index 951dae9..eb7eddb 100644
--- a/lib/nickserver.rb
+++ b/lib/nickserver.rb
@@ -5,9 +5,10 @@ require "nickserver/email_address"
 
 require "nickserver/couch/fetch_key"
 
-require "nickserver/hkp/key_info"
-require "nickserver/hkp/fetch_key_info"
 require "nickserver/hkp/fetch_key"
+require "nickserver/hkp/fetch_key_info"
+require "nickserver/hkp/parse_key_info"
+require "nickserver/hkp/key_info"
 
 require "nickserver/server"
 require "nickserver/daemon"
diff --git a/lib/nickserver/hkp/fetch_key_info.rb b/lib/nickserver/hkp/fetch_key_info.rb
index 2cfff43..bc12488 100644
--- a/lib/nickserver/hkp/fetch_key_info.rb
+++ b/lib/nickserver/hkp/fetch_key_info.rb
@@ -8,23 +8,16 @@ module Nickserver; module HKP
   class FetchKeyInfo
     include EM::Deferrable
 
-    # for this regexp to work, the source text must end in a trailing "\n",
-    # which the output of sks does.
-    MATCH_PUB_KEY = /(^pub:.+?\n(^uid:.+?\n)+)/m
-
     def search(uid)
       # in practice, exact=on seems to have no effect
       params = {:op => 'vindex', :search => uid, :exact => 'on', :options => 'mr', :fingerprint => 'on'}
       EventMachine::HttpRequest.new(Config.hkp_url).get(:query => params).callback {|http|
-        if http.response_header.status != 200
-          self.fail http.response_header.status, "Could net fetch keyinfo."
+        parser = ParseKeyInfo.new http.response_header, http.response
+        keys = parser.keys(uid)
+        if keys.any?
+          self.succeed keys
         else
-          keys, errors = parse(uid, http.response)
-          if keys.empty?
-            self.fail 500, errors.join("\n")
-          else
-            self.succeed keys
-          end
+          self.fail parser.status(uid), parser.msg(uid)
         end
       }.errback {|http|
         self.fail 500, http.error
@@ -32,43 +25,6 @@ module Nickserver; module HKP
       self
     end
 
-    #
-    # input:
-    #  uid           -- uid to search for
-    #  vindex_result -- raw output from a vindex hkp query (machine readable)
-    #
-    # returns:
-    #   an array of:
-    #   [0] -- array of eligible keys (as HKPKeyInfo objects) matching uid.
-    #   [1] -- array of error messages
-    #
-    # keys are eliminated from eligibility for a number of reasons, including expiration,
-    # revocation, uid match, key length, and so on...
-    #
-    def parse(uid, vindex_result)
-      keys = []
-      errors = []
-      now = Time.now
-      vindex_result.scan(MATCH_PUB_KEY).each do |match|
-        key_info = KeyInfo.new(match[0])
-        if key_info.uids.include?(uid)
-          if key_info.keylen < 2048
-            errors << "Ignoring key #{key_info.keyid} for #{uid}: key length is too short."
-          elsif key_info.expired?
-            errors << "Ignoring key #{key_info.keyid} for #{uid}: key expired."
-          elsif key_info.revoked?
-            errors << "Ignoring key #{key_info.keyid} for #{uid}: key revoked."
-          elsif key_info.disabled?
-            errors << "Ignoring key #{key_info.keyid} for #{uid}: key disabled."
-          elsif key_info.expirationdate && key_info.expirationdate < now
-            errors << "Ignoring key #{key_info.keyid} for #{uid}: key expired"
-          else
-            keys << key_info
-          end
-        end
-      end
-      [keys, errors]
-    end
   end
 
-end; end
\ No newline at end of file
+end; end
diff --git a/lib/nickserver/hkp/parse_key_info.rb b/lib/nickserver/hkp/parse_key_info.rb
new file mode 100644
index 0000000..2bfcb7e
--- /dev/null
+++ b/lib/nickserver/hkp/parse_key_info.rb
@@ -0,0 +1,93 @@
+module Nickserver; module HKP
+  class ParseKeyInfo
+
+    # for this regexp to work, the source text must end in a trailing "\n",
+    # which the output of sks does.
+    MATCH_PUB_KEY = /(^pub:.+?\n(^uid:.+?\n)+)/m
+
+    #  header        -- header of the hkp response
+    #  vindex_result -- raw output from a vindex hkp query (machine readable)
+    def initialize(header, vindex_result)
+      @header = header
+      @vindex_result = vindex_result
+    end
+
+    def status(uid)
+      if header.status == 200 && keys(uid).any?
+        200
+      else
+        error_status(uid)
+      end
+    end
+
+    def keys(uid)
+      key_infos(uid).reject { |key| error_for_key(key) }
+    end
+
+    def msg(uid)
+      if errors(uid).any?
+        error_messages(uid).join "\n"
+      else
+        "Could not fetch keyinfo."
+      end
+    end
+
+    protected
+
+    attr_reader :header
+    attr_reader :vindex_result
+
+    def error_status(uid)
+      if header.status != 200
+        header.status
+      else
+        if errors(uid).any?
+          500
+        else
+          404
+        end
+      end
+    end
+
+    def errors(uid)
+      key_infos(uid).map{|key| error_for_key(key) }.compact
+    end
+
+    def error_messages(uid)
+      key_infos(uid).map do |key|
+        err = error_for_key(key)
+        error_message(uid, key, err)
+      end.compact
+    end
+
+    def key_infos(uid)
+      all_key_infos.select do |key_info|
+        key_info.uids.include?(uid)
+      end
+    end
+
+    def all_key_infos
+      @all_key_infos ||= vindex_result.scan(MATCH_PUB_KEY).map do |match|
+        KeyInfo.new(match[0])
+      end
+    end
+
+    def error_message(uid, key, err)
+      "Ignoring key #{key.keyid} for #{uid}: #{err}" if err
+    end
+
+    def error_for_key(key)
+      if key.keylen < 2048
+        "key length is too short."
+      elsif key.expired?
+        "key expired."
+      elsif key.revoked?
+        "key revoked."
+      elsif key.disabled?
+        "key disabled."
+      elsif key.expirationdate && key.expirationdate < Time.now
+        "key expired"
+      end
+    end
+  end
+end; end