Merge branch 'refactor/clarify-response' into 'master'

Refactor/clarify response

See merge request leap/nickserver!20

14 files changed, 84 insertions, 82 deletions

diff --git a/README.md b/README.md
index 50460c4..01eaa07 100644
--- a/README.md
+++ b/README.md
@@ -17,17 +17,18 @@ About nickserver:
 
 * Written in Ruby 2.1, licensed GPLv3
 * Lightweight and scalable (high concurrency, reasonable latency)
-* Uses asynchronous network IO for both server and client connections (via EventMachine)
+* Uses asynchronous network IO for both server and client connections
+  (via Celluloid IO)
 
 API
 ==================================
 
-You query the nickserver via HTTP. The API is very minimal:
+You send requests to nickserver via HTTP. The API is very minimal:
 
     curl -X POST -d address=alice@domain.org https://nicknym.domain.org:6425
 
-The response consists of a signed JSON document with fields for the available
-public keys corresponding to the address.
+The response consists of a JSON document with fields for the selected
+public key corresponding to the address.
 
 For more details, see https://leap.se/nicknym
 
@@ -40,8 +41,6 @@ Requirements
 Installation
 ==================================
 
-You have three fine options for installing nickserver:
-
 Install prerequisites
 
     $ sudo apt-get install ruby-dev libssl-dev
diff --git a/bin/nickserver b/bin/nickserver
index 4706d8b..0890def 100755
--- a/bin/nickserver
+++ b/bin/nickserver
@@ -5,7 +5,8 @@
 #
 
 def load_local_gem(dir_path = '../..')
-  base_directory = File.expand_path(dir_path, File.symlink?(__FILE__) ? File.readlink(__FILE__) : __FILE__)
+  actual_file = File.symlink?(__FILE__) ? File.readlink(__FILE__) : __FILE__
+  base_directory = File.expand_path(dir_path, actual_file)
   unless $LOAD_PATH.include? "#{base_directory}/lib"
     if File.exist?("#{base_directory}/Gemfile.lock")
       ENV['BUNDLE_GEMFILE'] ||= "#{base_directory}/Gemfile"
diff --git a/lib/nickserver/hkp/key_info.rb b/lib/nickserver/hkp/key_info.rb
index e1a9500..c1b1ad3 100644
--- a/lib/nickserver/hkp/key_info.rb
+++ b/lib/nickserver/hkp/key_info.rb
@@ -49,7 +49,7 @@ module Nickserver::Hkp
 
     def expirationdate
       expires = properties[4]
-      Time.at(expires.to_i)
+      expires && Time.at(expires.to_i)
     end
 
     def flags
diff --git a/lib/nickserver/hkp/source.rb b/lib/nickserver/hkp/source.rb
index d7c86a3..fe3c4a5 100644
--- a/lib/nickserver/hkp/source.rb
+++ b/lib/nickserver/hkp/source.rb
@@ -1,6 +1,6 @@
 require 'nickserver/source'
 require 'nickserver/response'
-require 'nickserver/hkp/response'
+require 'nickserver/key_response'
 require 'nickserver/hkp/client'
 require 'nickserver/hkp/parse_key_info'
 require 'nickserver/hkp/key_info'
@@ -30,7 +30,7 @@ module Nickserver::Hkp
     def get_key_by_fingerprint(fingerprint, nick = nil)
       status, response = client.get_key_by_fingerprint fingerprint
       if status == 200
-        Response.new nick, response
+        Nickserver::KeyResponse.new nick, response
       else
         Nickserver::Response.new status, 'HKP Request failed'
       end
diff --git a/lib/nickserver/hkp/response.rb b/lib/nickserver/key_response.rb
index 2cc69d3..438dfc3 100644
--- a/lib/nickserver/hkp/response.rb
+++ b/lib/nickserver/key_response.rb
@@ -1,5 +1,5 @@
-module Nickserver::Hkp
-  class Response
+module Nickserver
+  class KeyResponse
     attr_reader :status, :content
 
     def initialize(uid, key)
diff --git a/lib/nickserver/wkd/source.rb b/lib/nickserver/wkd/source.rb
index 43f0b2e..bf30c94 100644
--- a/lib/nickserver/wkd/source.rb
+++ b/lib/nickserver/wkd/source.rb
@@ -1,7 +1,7 @@
 require 'nickserver/source'
 require 'nickserver/response'
 require 'nickserver/wkd/url'
-require 'nickserver/hkp/response'
+require 'nickserver/key_response'
 
 module Nickserver::Wkd
   # Query the web key directory for a given email address
@@ -10,7 +10,7 @@ module Nickserver::Wkd
       url = Url.new(email)
       status, blob = adapter.get url
       if status == 200
-        Nickserver::Hkp::Response.new(email.to_s, armor_key(blob))
+        Nickserver::KeyResponse.new(email.to_s, armor_key(blob))
       end
     end
 
diff --git a/test/functional/bin_test.rb b/test/functional/bin_test.rb
index 3a4e6c7..4d9f904 100644
--- a/test/functional/bin_test.rb
+++ b/test/functional/bin_test.rb
@@ -33,7 +33,7 @@ class BinTest < Minitest::Test
 
   def assert_command_runs(command)
     out = run_command command
-    assert ($CHILD_STATUS.exitstatus == 0),
+    assert $CHILD_STATUS.exitstatus.zero?,
            "failed to run 'nickserver #{command}':\n #{out}"
   end
 
diff --git a/test/functional/sample_test.rb b/test/functional/sample_test.rb
index 412555e..1bfe8b5 100644
--- a/test/functional/sample_test.rb
+++ b/test/functional/sample_test.rb
@@ -32,7 +32,7 @@ class SampleTest < FunctionalTest
 
   # platform/#8674 handle nonexisting domains
   def test_nicknym_handles_missing_domain
-    assert_lookup_status 404, 'postmaster@now-dont-you-dare-register-this-domain.coop'
+    assert_lookup_status 404, 'postmaster@dont-you-dare-register-this.coop'
   end
 
   def test_no_file_descriptors_leak
@@ -63,7 +63,7 @@ class SampleTest < FunctionalTest
 
   def run_command(command)
     `#{command} 2>&1`.tap do |out|
-      assert ($CHILD_STATUS.exitstatus == 0),
+      assert $CHILD_STATUS.exitstatus.zero?,
              "failed to run '#{command}':\n #{out}"
     end
   end
diff --git a/test/integration/dispatcher_test.rb b/test/integration/dispatcher_test.rb
index 58aa972..1973e84 100644
--- a/test/integration/dispatcher_test.rb
+++ b/test/integration/dispatcher_test.rb
@@ -29,8 +29,9 @@ class Nickserver::DispatcherTest < Minitest::Test
   end
 
   def test_fingerprint_is_not_hex
-    handle fingerprint: ['X36E738D69173C13Z709E44F2F455E2824D18DDX']
-    assert_response error('Fingerprint invalid: X36E738D69173C13Z709E44F2F455E2824D18DDX')
+    fingerprint = 'X36E738D69173C13Z709E44F2F455E2824D18DDX'
+    handle fingerprint: [fingerprint]
+    assert_response error("Fingerprint invalid: #{fingerprint}")
   end
 
   def test_missing_domain
@@ -41,28 +42,32 @@ class Nickserver::DispatcherTest < Minitest::Test
   end
 
   def test_email_via_hkp
-    handle address: ['valid@email.tld'], headers: { 'Host' => 'http://nickserver.me' }
+    handle address: ['valid@email.tld'],
+           headers: { 'Host' => 'http://nickserver.me' }
     stub_nicknym_not_available
     hkp_source.expect :query, success, [Nickserver::EmailAddress]
     assert_response success
   end
 
   def test_email_via_hkp_nicknym_unreachable
-    handle address: ['valid@email.tld'], headers: { 'Host' => 'http://nickserver.me' }
+    handle address: ['valid@email.tld'],
+           headers: { 'Host' => 'http://nickserver.me' }
     stub_nicknym_raises
     hkp_source.expect :query, success, [Nickserver::EmailAddress]
     assert_response success
   end
 
   def test_email_not_found_hkp_nicknym_unreachable
-    handle address: ['valid@email.tld'], headers: { 'Host' => 'http://nickserver.me' }
+    handle address: ['valid@email.tld'],
+           headers: { 'Host' => 'http://nickserver.me' }
     stub_nicknym_raises
     hkp_source.expect :query, nil, [Nickserver::EmailAddress]
     assert_response http_connection_error
   end
 
   def test_email_via_nicknym
-    handle address: ['valid@email.tld'], headers: { 'Host' => 'http://nickserver.me' }
+    handle address: ['valid@email.tld'],
+           headers: { 'Host' => 'http://nickserver.me' }
     nicknym_source.expect :available_for?, true, [String]
     nicknym_source.expect :query, success, [Nickserver::EmailAddress]
     assert_response success
diff --git a/test/integration/hkp_test.rb b/test/integration/hkp_test.rb
index c12588c..f6675e9 100644
--- a/test/integration/hkp_test.rb
+++ b/test/integration/hkp_test.rb
@@ -33,7 +33,7 @@ class HkpTest < Minitest::Test
     stubbing_http do
       uid = 'leaping_lemur@leap.se'
       stub_sks_vindex_reponse(uid, status: 404)
-      assert_response_status_for_uid uid, 404
+      assert_nil response_for_uid(uid)
     end
   end
 
@@ -41,7 +41,7 @@ class HkpTest < Minitest::Test
     stubbing_http do
       uid = 'leaping_lemur@leap.se'
       stub_sks_vindex_reponse(uid, status: 200)
-      assert_response_status_for_uid uid, 404
+      assert_nil response_for_uid(uid)
     end
   end
 
@@ -51,10 +51,9 @@ class HkpTest < Minitest::Test
     stubbing_http do
       stub_sks_vindex_reponse(uid, body: file_content(:leap_vindex_result))
       stub_sks_get_reponse(key_id, body: file_content(:leap_public_key))
-      assert_response_for_uid(uid) do |response|
-        content = JSON.parse response.content
-        assert_equal file_content(:leap_public_key), content['openpgp']
-      end
+      response = response_for_uid(uid)
+      content = JSON.parse response.content
+      assert_equal file_content(:leap_public_key), content['openpgp']
     end
   end
 
@@ -65,7 +64,7 @@ class HkpTest < Minitest::Test
     stubbing_http do
       stub_sks_vindex_reponse(uid, body: file_content(:leap_vindex_result))
       stub_sks_get_reponse(key_id, status: 404)
-      assert_response_status_for_uid uid, 404
+      assert_equal 404, response_for_uid(uid).status
     end
   end
 
@@ -74,29 +73,20 @@ class HkpTest < Minitest::Test
 
     stubbing_http do
       stub_sks_vindex_reponse(uid, body: file_content(:short_key_vindex_result))
-      assert_response_status_for_uid uid, 500
+      assert_equal 500, response_for_uid(uid).status
     end
   end
 
   protected
 
-  def assert_response_status_for_uid(uid, status)
-    assert_response_for_uid(uid) do |response|
-      assert_equal status, response.status
-    end
-  end
-
-  def assert_response_for_uid(uid)
-    Nickserver::Hkp::Source.new(adapter).query uid do |response|
-      yield response
-    end
+  def response_for_uid(uid)
+    Nickserver::Hkp::Source.new(adapter).query uid
   end
 
   def assert_key_info_for_uid(uid)
-    Nickserver::Hkp::Source.new(adapter).search uid do |status, keys|
-      assert_equal 200, status
-      yield keys
-    end
+    status, keys = Nickserver::Hkp::Source.new(adapter).search uid
+    assert_equal 200, status
+    yield keys
   end
 
   def fetch_key_info(body_source, uid, &block)
diff --git a/test/integration/nickserver_test.rb b/test/integration/nickserver_test.rb
index fb9b952..832a68c 100644
--- a/test/integration/nickserver_test.rb
+++ b/test/integration/nickserver_test.rb
@@ -19,13 +19,14 @@ require 'json'
 # (2) We actually start the Reelserver on 127.0.0.1 and talk to it via http.
 #     In order to run the Reelserver properly this is a celluloid test.
 #
-# (3) the "Host" header for requests to nickserver must be set (or Config.domain set)
+# (3) the "Host" header for requests to nickserver must be set
+#     (or Config.domain set)
 #
 
 class NickserverTest < CelluloidTest
   include HttpStubHelper
 
-  def test_GET_key_by_email_address_served_via_SKS
+  def test_key_by_email_address_from_sks
     uid    = 'cloudadmin@leap.se'
     key_id = 'E818C478D3141282F7590D29D041EB11B1647490'
     stub_nicknym_available_response 'leap.se', status: 404
@@ -34,51 +35,43 @@ class NickserverTest < CelluloidTest
 
     start do
       params = { query: { 'address' => uid } }
-      get(params) do |response|
-        assert_equal file_content(:leap_public_key), JSON.parse(response.to_s)['openpgp']
-      end
+      assert_responds_to params, key: :leap_public_key
     end
   end
 
-  def test_GET_key_by_fingerprint_served_via_SKS
+  def test_key_by_fingerprint_from_sks
     fingerprint = 'E818C478D3141282F7590D29D041EB11B1647490'
     stub_sks_get_reponse(fingerprint, body: file_content(:leap_public_key))
 
     start do
       params = { query: { 'fingerprint' => fingerprint } }
-      get(params) do |response|
-        assert_equal file_content(:leap_public_key), JSON.parse(response.to_s)['openpgp']
-      end
+      assert_responds_to params, key: :leap_public_key
     end
   end
 
-  def test_GET_served_via_couch_not_found
+  def test_couch_user_not_found
     domain = 'example.org'
     uid    = 'bananas@' + domain
     stub_couch_response(uid, status: 404) do
       start do
         params = { query: { 'address' => uid }, head: { 'Host' => domain } }
-        get(params) do |response|
-          assert_equal 404, response.code
-        end
+        assert_responds_to params, code: 404
       end
     end
   end
 
-  def test_GET_served_via_couch_empty_results
+  def test_couch_empty_results
     domain = 'example.org'
     uid    = 'stompy@' + domain
     stub_couch_response(uid, body: file_content(:empty_couchdb_result)) do
       start do
         params = { query: { 'address' => uid }, head: { host: domain } }
-        get(params) do |response|
-          assert_equal 404, response.code
-        end
+        assert_responds_to params, code: 404
       end
     end
   end
 
-  def test_GET_served_via_couch_success
+  def test_couch_success_response
     domain = 'example.org'
     uid    = 'blue@' + domain
     stub_couch_response(uid, body: file_content(:blue_couchdb_result)) do
@@ -91,7 +84,7 @@ class NickserverTest < CelluloidTest
     end
   end
 
-  def test_GET_empty
+  def test_empty_get
     start do
       get({}) do |response|
         assert_equal "404 Not Found\n", response.to_s
@@ -113,6 +106,16 @@ class NickserverTest < CelluloidTest
     server.terminate if server && server.alive?
   end
 
+  def assert_responds_to(params, key: nil, code: nil)
+    get(params) do |response|
+      assert_equal code, response.code if code
+      if key
+        assert_equal file_content(key),
+                     JSON.parse(response.to_s)['openpgp']
+      end
+    end
+  end
+
   #
   # http GET requests to nickserver
   #
@@ -130,7 +133,8 @@ class NickserverTest < CelluloidTest
   #
   # http request to nickserver
   #
-  # this works because http requests to 127.0.0.1 are not stubbed, but requests to other domains are.
+  # this works because http requests to 127.0.0.1 are not stubbed, but
+  # requests to other domains are.
   #
   def request(method, options = {})
     response = HTTP
diff --git a/test/remote/hkp_source_test.rb b/test/remote/hkp_source_test.rb
index 8232dce..469941b 100644
--- a/test/remote/hkp_source_test.rb
+++ b/test/remote/hkp_source_test.rb
@@ -7,32 +7,27 @@ class RemoteHkpSourceTest < CelluloidTest
   include HttpAdapterHelper
 
   def test_key_info
-    uid = 'elijah@riseup.net'
-    assert_key_info_for_uid uid do |keys|
-      assert_equal 1, keys.size
-      assert keys.first.keyid =~ /00440025$/
-    end
+    assert_key_found 'elijah@riseup.net', /00440025$/
   end
 
   def test_tls_validation
-    hkp_url = 'https://keys.mayfirst.org/pks/lookup'
-    ca_file = file_path('mayfirst-ca.pem')
-
     config.stub(:hkp_url, hkp_url) do
-      # config.stub(:hkp_ca_file, file_path('autistici-ca.pem')) do
       config.stub(:hkp_ca_file, ca_file) do
         assert File.exist?(Nickserver::Config.hkp_ca_file)
-        uid = 'elijah@riseup.net'
-        assert_key_info_for_uid uid do |keys|
-          assert_equal 1, keys.size
-          assert keys.first.keyid =~ /00440025$/
-        end
+        assert_key_found 'elijah@riseup.net', /00440025$/
       end
     end
   end
 
   protected
 
+  def assert_key_found(uid, fingerprint_regexp)
+    assert_key_info_for_uid uid do |keys|
+      assert_equal 1, keys.size
+      assert keys.first.keyid =~ fingerprint_regexp
+    end
+  end
+
   def assert_key_info_for_uid(uid)
     status, keys = source.search uid
     assert_equal 200, status
@@ -44,4 +39,12 @@ class RemoteHkpSourceTest < CelluloidTest
   def source
     Nickserver::Hkp::Source.new adapter
   end
+
+  def hkp_url
+    'https://keys.mayfirst.org/pks/lookup'
+  end
+
+  def ca_file
+    file_path('mayfirst-ca.pem')
+  end
 end
diff --git a/test/remote/nicknym_source_test.rb b/test/remote/nicknym_source_test.rb
index 6fff1f6..ef74653 100644
--- a/test/remote/nicknym_source_test.rb
+++ b/test/remote/nicknym_source_test.rb
@@ -63,6 +63,6 @@ class RemoteNicknymSourceTest < CelluloidTest
   end
 
   def email_without_key
-    Nickserver::EmailAddress.new('pleaseneverusethisemailweuseittotest@mail.bitmask.net')
+    Nickserver::EmailAddress.new('neverusethisweuseittotest@mail.bitmask.net')
   end
 end
diff --git a/test/remote/wkd_source_test.rb b/test/remote/wkd_source_test.rb
index 1ed7ea5..46a6bbb 100644
--- a/test/remote/wkd_source_test.rb
+++ b/test/remote/wkd_source_test.rb
@@ -27,8 +27,8 @@ class RemoteWkdSourceTest < CelluloidTest
 
   def assert_pgp_key_in(response)
     json = JSON.parse response.content
-    assert_equal email_with_key.to_s, json["address"]
-    refute_empty json["openpgp"]
+    assert_equal email_with_key.to_s, json['address']
+    refute_empty json['openpgp']
     assert_equal file_content('dewey.pgp.asc'), json['openpgp']
   end