From b1738a78ccf5768f92068a27255f9f69be1c3147 Mon Sep 17 00:00:00 2001
From: Azul <azul@riseup.net>
Date: Mon, 24 Jul 2017 09:55:28 +0200
Subject: [PATCH] fix: #3 handle domains without A-record
If a domain only has an mx record but no A record it
will trigger a ConnectionError when attempting the
nicknym key lookup.
We need to detect and handle this in the http adapter
already because once the exception is handled by Celluloid
our actor will be terminated.
So now we allow for handing a rescue option to the
adapter with a string that is checked for inclusion in
the error message. If the string is found the exception
will be caught and the adapter returns nil.
We only make use of this when checking the availability
of nicknym so far. That should be the only http request
going out.
---
lib/nickserver/adapters/http.rb | 15 +++++++++++++++
lib/nickserver/nicknym/source.rb | 3 ++-
test/functional/sample_test.rb | 5 +++++
test/remote/Readme.md | 15 +++++++--------
test/remote/nicknym_source_test.rb | 15 +++++++++++++--
test/support/http_stub_helper.rb | 27 +++++++++++++--------------
test/unit/nicknym/source_test.rb | 13 ++++++++++---
7 files changed, 65 insertions(+), 28 deletions(-)
diff --git a/lib/nickserver/adapters/http.rb b/lib/nickserver/adapters/http.rb
index 636aceb..eb77cc6 100644
--- a/lib/nickserver/adapters/http.rb
+++ b/lib/nickserver/adapters/http.rb
@@ -2,6 +2,19 @@ require 'nickserver/adapters'
require 'nickserver/config'
require 'http'
+# Nickserver::Adapters::Http
+#
+# Basic http adapter with ssl and minimal error handling.
+# Only implemented get requests so far.
+#
+# Error Handling:
+#
+# Pass a string as the 'rescue' option. If a ConnectionError occures
+# which includes the string passed it will be rescued and the request
+# will return nil. This allows handling the error inside the adapter so
+# that for the derived CelluloidHttp Adapter the actor does not get
+# killed.
+
module Nickserver::Adapters
class Http
@@ -9,6 +22,8 @@ module Nickserver::Adapters
url = HTTP::URI.parse url.to_s
response = get_with_auth url, params: options[:query]
return response.code, response.to_s
+ rescue HTTP::ConnectionError => e
+ raise unless options[:rescue] && e.to_s.include?(options[:rescue])
end
protected
diff --git a/lib/nickserver/nicknym/source.rb b/lib/nickserver/nicknym/source.rb
index 96945cb..f49547e 100644
--- a/lib/nickserver/nicknym/source.rb
+++ b/lib/nickserver/nicknym/source.rb
@@ -8,7 +8,8 @@ module Nickserver
PORT = 6425
def available_for?(domain)
- status, body = adapter.get "https://#{domain}/provider.json"
+ status, body = adapter.get "https://#{domain}/provider.json",
+ rescue: 'failed to connect: getaddrinfo'
status == 200 && provider_with_mx?(body)
end
diff --git a/test/functional/sample_test.rb b/test/functional/sample_test.rb
index 68127e1..4be5c26 100644
--- a/test/functional/sample_test.rb
+++ b/test/functional/sample_test.rb
@@ -24,6 +24,11 @@ class SampleTest < FunctionalTest
# Regression Tests
+ # #3 handle missing A records
+ def test_nicknym
+ assert_lookup_status 404, 'postmaster@cs.ucl.ac.uk'
+ end
+
def test_no_file_descriptors_leak
lookup 'test@mail.bitmask.net'
before = open_files_count
diff --git a/test/remote/Readme.md b/test/remote/Readme.md
index 957ea12..53f5e65 100644
--- a/test/remote/Readme.md
+++ b/test/remote/Readme.md
@@ -1,15 +1,14 @@
Integration tests for clients of remote services
================================================
-The tests in this directory are integration test with remote services. However
-we aims at testing the client side of the equation as that is what we control
-here.
+The tests in this directory are integration test with remote services.
+However we aims at testing the client side of the equation as that is
+what we control here.
-So unexpected server behavious should *crash* the test if we are not dealing
-with it properly yet and have no unit test for it.
+So unexpected server behavious should *crash* the test if we are not
+dealing with it properly yet and have no unit test for it.
-Server responses that we do not expect but handle in the code and test in unit
-tests make the test *skip*.
+Server responses that we do not expect but handle in the code and test
+in unit tests make the test *skip*.
The Behaviour we would normally expect should make the test *pass*
-
diff --git a/test/remote/nicknym_source_test.rb b/test/remote/nicknym_source_test.rb
index 4ca3033..b97f2b2 100644
--- a/test/remote/nicknym_source_test.rb
+++ b/test/remote/nicknym_source_test.rb
@@ -10,13 +10,24 @@ require 'nickserver/email_address'
class RemoteNicknymSourceTest < CelluloidTest
include HttpAdapterHelper
- def test_availablility_check
+ def test_available_for_mail
source.available_for? 'mail.bitmask.net'
- refute source.available_for? 'dl.bitmask.net' # not a provider
rescue HTTP::ConnectionError => e
skip e.to_s
end
+ # not a provider
+ def test_not_available
+ refute source.available_for? 'dl.bitmask.net'
+ rescue HTTP::ConnectionError => e
+ skip e.to_s
+ end
+
+ # cs.ucl.ac.uk only has an MX not an A-record
+ def test_not_available_without_a_record
+ refute source.available_for? 'cs.ucl.ac.uk'
+ end
+
def test_successful_query
response = source.query(email_with_key)
skip if response.status == 404
diff --git a/test/support/http_stub_helper.rb b/test/support/http_stub_helper.rb
index c9f2bfa..4e3d89b 100644
--- a/test/support/http_stub_helper.rb
+++ b/test/support/http_stub_helper.rb
@@ -10,32 +10,31 @@ module HttpStubHelper
end
def stub_nicknym_available_response(domain, response = {})
- stub_http_request :get, "https://#{domain}/provider.json",
- response: response
+ stub_http_get "https://#{domain}/provider.json",
+ response,
+ Hash
end
def stub_sks_vindex_reponse(uid, response = {})
- stub_http_request :get, config.hkp_url,
- query: {op: 'vindex', search: uid, exact: 'on', options: 'mr', fingerprint: 'on'},
- response: response
+ stub_http_get config.hkp_url, response,
+ query: {op: 'vindex', search: uid, exact: 'on', options: 'mr', fingerprint: 'on'}
end
def stub_sks_get_reponse(key_id, response = {})
- stub_http_request :get, config.hkp_url,
- query: {op: 'get', search: "0x"+key_id, exact: 'on', options: 'mr'},
- response: response
+ stub_http_get config.hkp_url, response,
+ query: {op: 'get', search: "0x"+key_id, exact: 'on', options: 'mr'}
end
def stub_couch_response(uid, response = {})
query = "\?key=#{"%22#{uid}%22"}&reduce=false"
- stub_http_request :get,
- /#{Regexp.escape(config.couch_url)}.*#{query}/,
- response: response
+ stub_http_get /#{Regexp.escape(config.couch_url)}.*#{query}/,
+ response
end
- def stub_http_request(verb, url, options = {})
- response = {status: 200, body: ""}.merge(options.delete(:response) || {})
- options = nil if options == {}
+ private
+
+ def stub_http_get(url, response, options = nil)
+ response = {status: 200, body: ""}.merge(response || {})
adapter.expect :get, [response[:status], response[:body]],
[url, options].compact
end
diff --git a/test/unit/nicknym/source_test.rb b/test/unit/nicknym/source_test.rb
index f8c9b60..b17f22b 100644
--- a/test/unit/nicknym/source_test.rb
+++ b/test/unit/nicknym/source_test.rb
@@ -1,4 +1,6 @@
require 'test_helper'
+require 'http'
+require 'json'
require 'nickserver/nicknym/source'
require 'nickserver/email_address'
@@ -20,6 +22,11 @@ class NicknymSourceTest < Minitest::Test
refute available_on?(200, 'blablabla')
end
+ # adapter rescues name resolution errors and returns nothing
+ def test_not_available_without_response
+ refute available_on?
+ end
+
def test_proxy_successful_query
assert proxies_query_response?(200, 'dummy body')
end
@@ -39,9 +46,9 @@ class NicknymSourceTest < Minitest::Test
adapter.verify
end
- def available_on?(status = 0, body = nil)
- adapter.expect :get, [status, body],
- ['https://remote.tld/provider.json']
+ def available_on?(*args)
+ adapter.expect :get, args,
+ ['https://remote.tld/provider.json', Hash]
available = source.available_for?('remote.tld')
adapter.verify
return available
--
GitLab