Fix mx records resolver issues (#29)

truemail-rb · Apr 8, 2019 · 49b5737 · 49b5737
1 parent 23a02e6
commit 49b5737
Show file tree

Hide file tree

Showing 11 changed files with 190 additions and 35 deletions.
diff --git a/.github/ISSUE_TEMPLATE.md b/.github/ISSUE_TEMPLATE.md
@@ -0,0 +1,17 @@
+<!-- Thanks for helping to make Truemail better! Before submit your issue, please make sure to check the following boxes by putting an x in the [ ] (don't: [x ], [ x], do: [x]) -->
+
+### New Issue Checklist
+
+- [ ] I have updated truemail to the latest version
+- [ ] I have read the [Contribution Guidelines](CONTRIBUTING.md)
+- [ ] I have read the [documentation](README.md)
+- [ ] I have searched for [existing GitHub issues](https://github.com/rubygarage/truemail/issues)
+
+### Issue Description
+<!-- Please include what's happening, expected behavior, and any relevant code samples -->
+
+##### Complete output when running truemail, including the stack trace and command used
+
+<details>
+  <pre>[INSERT OUTPUT HERE]</pre>
+</details>
diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md
@@ -0,0 +1,46 @@
+# PR Details
+
+<!-- Provide a general summary of your changes in the Title above -->
+
+## Description
+
+<!--- Describe your changes in detail -->
+
+## Related Issue
+
+<!--- This project only accepts pull requests related to open issues -->
+<!--- If suggesting a new feature or change, please discuss it in an issue first -->
+<!--- If fixing a bug, there should be an issue describing it with steps to reproduce -->
+<!--- Please link to the issue here: -->
+
+## Motivation and Context
+
+<!--- Why is this change required? What problem does it solve? -->
+
+## How Has This Been Tested
+
+<!--- Please describe in detail how you tested your changes. -->
+<!--- Include details of your testing environment, and the tests you ran to -->
+<!--- see how your change affects other areas of the code, etc. -->
+
+## Types of changes
+
+<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
+
+- [ ] Docs change / refactoring / dependency upgrade
+- [ ] Bug fix (non-breaking change which fixes an issue)
+- [ ] New feature (non-breaking change which adds functionality)
+- [ ] Breaking change (fix or feature that would cause existing functionality to change)
+
+## Checklist
+
+<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
+<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
+
+- [ ] My code follows the code style of this project
+- [ ] My change requires a change to the documentation
+- [ ] I have updated the documentation accordingly
+- [ ] I have read the [**CONTRIBUTING** document](CONTRIBUTING.md)
+- [ ] I have added tests to cover my changes
+- [ ] I have run `bundle exec rspec` from the root directory to see all new and existing tests pass
+- [ ] I have run `rubocop` and `reek` to ensure the code style is valid
diff --git a/.reek.yml b/.reek.yml
@@ -27,7 +27,7 @@ detectors:
     exclude:
       - Truemail::Validate::Smtp::Request#compose_from
       - Truemail::Validator#select_validation_type
-      - Truemail::Validate::Mx#mx_records
+      - Truemail::Validate::Mx#null_mx?
 
   ControlParameter:
     exclude:

diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
@@ -0,0 +1,43 @@
+# Contributing to Truemail
+
+Please take a moment to review this document in order to make the contribution process easy and effective for everyone involved.
+
+Following these guidelines helps to communicate that you respect the time of the developers managing and developing this open source project. In return, they should reciprocate that respect in addressing your issue or assessing patches and features.
+
+## Using the issue tracker
+
+The issue tracker is the preferred channel for [bug reports](#bugs), [features requests](#features) and [submitting pull requests](#pull-requests).
+
+<a name="bugs"></a>
+## Bug/issue reports
+
+A bug is a _demonstrable problem_ that is caused by the code in the repository.
+Good bug reports are extremely helpful - thank you!
+
+Guidelines for bug reports:
+
+1. **Use the GitHub issue search** &mdash; check if the issue has already been reported.
+2. **Check if the issue has been fixed** &mdash; try to reproduce it using the latest `master` or development branch in the repository.
+
+A good bug report shouldn't leave others needing to chase you up for more information. Please try to be as detailed as possible in your report. What is your environment? What steps will reproduce the issue? What would you expect to be the outcome? All these details will help people to fix any potential bugs.
+
+<a name="features"></a>
+## Feature requests
+
+Feature requests are welcome. But take a moment to find out whether your idea fits with the scope and aims of the project. It's up to *you* to make a strong case to convince the project's developers of the merits of this feature. Please provide as much detail and context as possible.
+
+<a name="pull-requests"></a>
+## Pull requests
+
+Good pull requests - patches, improvements, new features - are a fantastic help. They should remain focused in scope and avoid containing unrelated commits.
+
+**Please ask first** before embarking on any significant pull request (e.g. implementing features, refactoring code, porting to a different language), otherwise you risk spending a lot of time working on something that the project's developers might not want to merge into the project.
+
+Please adhere to the coding conventions used throughout a project (indentation, accurate comments, etc.) and any other requirements (such as test coverage). Not all features proposed will be added but we are open to having a conversation about a feature you are championing.
+
+Guidelines for pull requests:
+
+1. Fork the repo.
+2. Run the tests. This is to make sure your starting point works. Tests can be run via ```rspec```
+3. Create a new branch and make your changes. This includes tests for features!
+4. Push to your fork and submit a pull request.
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -1,7 +1,7 @@
 PATH
   remote: .
   specs:
-    truemail (0.1.5)
+    truemail (0.1.6)
 
 GEM
   remote: https://rubygems.org/

diff --git a/README.md b/README.md
@@ -1,6 +1,6 @@
 # Truemail
 
-[![Maintainability](https://api.codeclimate.com/v1/badges/657aa241399927dcd2e2/maintainability)](https://codeclimate.com/github/rubygarage/truemail/maintainability) [![Test Coverage](https://api.codeclimate.com/v1/badges/657aa241399927dcd2e2/test_coverage)](https://codeclimate.com/github/rubygarage/truemail/test_coverage) [![Gem Version](https://badge.fury.io/rb/truemail.svg)](https://badge.fury.io/rb/truemail) [![CircleCI](https://circleci.com/gh/rubygarage/truemail/tree/master.svg?style=svg)](https://circleci.com/gh/rubygarage/truemail/tree/master)
+[![Maintainability](https://api.codeclimate.com/v1/badges/657aa241399927dcd2e2/maintainability)](https://codeclimate.com/github/rubygarage/truemail/maintainability) [![Test Coverage](https://api.codeclimate.com/v1/badges/657aa241399927dcd2e2/test_coverage)](https://codeclimate.com/github/rubygarage/truemail/test_coverage) [![Gem Version](https://badge.fury.io/rb/truemail.svg)](https://badge.fury.io/rb/truemail) [![CircleCI](https://circleci.com/gh/rubygarage/truemail/tree/master.svg?style=svg)](https://circleci.com/gh/rubygarage/truemail/tree/master) [![Contributor Covenant](https://img.shields.io/badge/Contributor%20Covenant-v1.4%20adopted-ff69b4.svg)](CODE_OF_CONDUCT.md)
 
 The Truemail gem helps you validate emails by regex pattern, presence of domain mx-records, and real existence of email account on a current email server.
 
@@ -170,7 +170,7 @@ require 'truemail'
 
 Truemail.configure do |config|
   config.verifier_email = '[email protected]'
-  config.config.email_pattern = /regex_pattern/
+  config.email_pattern = /regex_pattern/
 end
 
 Truemail.validate('[email protected]', with: :regex)
@@ -195,7 +195,7 @@ Validation by MX records is the second validation level. It uses Regex validatio
 [Regex validation] -> [MX validation]
 ```
 
-Truemail MX validation performs strictly following the [RFC 5321](https://tools.ietf.org/html/rfc5321#section-5) standard.
+Please note, Truemail MX validator not performs strict compliance of the [RFC 5321](https://tools.ietf.org/html/rfc5321#section-5) standard for best validation outcome.
 
 Example of usage:
 
@@ -419,15 +419,19 @@ end
 
 ## Contributing
 
-Bug reports and pull requests are welcome on GitHub at https://github.com/rubygarage/truemail. This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the [Contributor Covenant](http://contributor-covenant.org) code of conduct.
+Bug reports and pull requests are welcome on GitHub at https://github.com/rubygarage/truemail. This project is intended to be a safe, welcoming space for collaboration, and contributors are expected to adhere to the [Contributor Covenant](http://contributor-covenant.org) code of conduct. Please check the [open tikets](https://github.com/rubygarage/truemail/issues). Be shure to follow Contributor Code of Conduct below and our [Contributing Guidelines](CONTRIBUTING.md).
 
 ## License
 
 The gem is available as open source under the terms of the [MIT License](https://opensource.org/licenses/MIT).
 
 ## Code of Conduct
 
-Everyone interacting in the Truemail project’s codebases, issue trackers, chat rooms and mailing lists is expected to follow the [code of conduct](https://github.com/[USERNAME]/truemail/blob/master/CODE_OF_CONDUCT.md).
+Everyone interacting in the Truemail project’s codebases, issue trackers, chat rooms and mailing lists is expected to follow the [code of conduct](CODE_OF_CONDUCT.md).
+
+## Versioning
+
+Truemail uses [Semantic Versioning 2.0.0](https://semver.org)
 
 ---
 <a href="https://rubygarage.org/"><img src="https://rubygarage.s3.amazonaws.com/assets/assets/rg_color_logo_horizontal-919afc51a81d2e40cb6a0b43ee832e3fcd49669d06785156d2d16fd0d799f89e.png" alt="RubyGarage Logo" width="415" height="128"></a>

diff --git a/lib/truemail/validate/base.rb b/lib/truemail/validate/base.rb
@@ -22,6 +22,10 @@ def success(condition)
       def add_error(message)
         result.errors[self.class.name.split('::').last.downcase.to_sym] = message
       end
+
+      def mail_servers
+        result.mail_servers
+      end
     end
   end
 end
diff --git a/lib/truemail/validate/mx.rb b/lib/truemail/validate/mx.rb
@@ -6,12 +6,13 @@ class Mx < Truemail::Validate::Base
       require 'resolv'
 
       ERROR = 'target host(s) not found'
+      NULL_MX_RECORD = 'null_mx_record'
 
       def run
         return false unless Truemail::Validate::Regex.check(result)
         result.domain = result.email[Truemail::RegexConstant::REGEX_DOMAIN_FROM_EMAIL, 1]
-        return true if success(mx_lookup)
-        add_error(Truemail::Validate::Mx::ERROR)
+        return true if success(mx_lookup && domain_not_include_null_mx)
+        mail_servers.clear && add_error(Truemail::Validate::Mx::ERROR)
         false
       end
 
@@ -28,17 +29,28 @@ def mx_lookup
       end
 
       def fetch_target_hosts(hosts)
-        result.mail_servers.push(*hosts)
+        mail_servers.push(*hosts)
+      end
+
+      def null_mx?(domain_mx_records)
+        mx_record = domain_mx_records.first
+        domain_mx_records.one? && mx_record.preference.zero? && mx_record.exchange.to_s.empty?
+      end
+
+      def domain_not_include_null_mx
+        !mail_servers.include?(Truemail::Validate::Mx::NULL_MX_RECORD)
       end
 
       def mx_records(domain)
-        Resolv::DNS.new.getresources(domain, Resolv::DNS::Resource::IN::MX).sort_by(&:preference).map do |mx_record|
-          Resolv.getaddress(mx_record.exchange.to_s)
-        end
+        domain_mx_records = Resolv::DNS.new.getresources(domain, Resolv::DNS::Resource::IN::MX)
+        return [Truemail::Validate::Mx::NULL_MX_RECORD] if null_mx?(domain_mx_records)
+        domain_mx_records.sort_by(&:preference).map do |mx_record|
+          Resolv.getaddresses(mx_record.exchange.to_s)
+        end.flatten
       end
 
       def mail_servers_found?
-        !result.mail_servers.empty?
+        !mail_servers.empty?
       end
 
       def hosts_from_mx_records?

diff --git a/lib/truemail/validate/smtp.rb b/lib/truemail/validate/smtp.rb
@@ -29,10 +29,6 @@ def request
         smtp_results.last
       end
 
-      def mail_servers
-        result.mail_servers
-      end
-
       def attempts
         @attempts ||=
           mail_servers.one? ? { attempts: Truemail.configuration.connection_attempts } : {}

diff --git a/lib/truemail/version.rb b/lib/truemail/version.rb
@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
 module Truemail
-  VERSION = '0.1.5'
+  VERSION = '0.1.6'
 end
diff --git a/spec/truemail/validate/mx_spec.rb b/spec/truemail/validate/mx_spec.rb
@@ -8,6 +8,7 @@
 
   describe 'defined constants' do
     specify { expect(described_class).to be_const_defined(:ERROR) }
+    specify { expect(described_class).to be_const_defined(:NULL_MX_RECORD) }
   end
 
   describe '.check' do
@@ -42,26 +43,58 @@
       context 'when mx records found' do
         let(:mx_records_file) { "#{File.expand_path('../../', __dir__)}/support/objects/mx_records.yml" }
 
-        before do
-          allow(Resolv::DNS).to receive_message_chain(:new, :getresources).and_return(mx_records_object)
-          allow(Resolv).to receive(:getaddress).and_return(host_address)
-        end
+        before { allow(Resolv::DNS).to receive_message_chain(:new, :getresources).and_return(mx_records_object) }
 
-        specify do
-          expect(mx_validator_instance).to receive(:hosts_from_mx_records?).and_call_original
-          expect(mx_validator_instance).not_to receive(:hosts_from_cname_records?)
-          expect(mx_validator_instance).not_to receive(:host_from_a_record?)
+        context 'with null mx' do
+          let(:target_mx_record) { mx_records_object.first }
 
-          expect { mx_validator }
-            .to change(result_instance, :domain)
-            .from(nil).to(email[Truemail::RegexConstant::REGEX_EMAIL_PATTERN, 3])
-            .and change(result_instance, :mail_servers)
-            .from([]).to(mail_servers_by_ip)
-            .and not_change(result_instance, :success)
+          before do
+            allow(mx_validator_instance).to receive(:hosts_from_mx_records?).and_call_original
+            allow(mx_validator_instance).to receive(:mx_records).and_call_original
+            allow(mx_validator_instance).to receive(:null_mx?).and_call_original
+
+            allow(mx_records_object).to receive(:one?).and_return(true)
+            allow(target_mx_record).to receive_message_chain(:preference, :zero?).and_return(true)
+            allow(target_mx_record).to receive_message_chain(:exchange, :to_s, :empty?).and_return(true)
+          end
+
+          specify do
+            expect(mx_validator_instance).not_to receive(:hosts_from_cname_records?)
+            expect(mx_validator_instance).not_to receive(:host_from_a_record?)
+
+            expect { mx_validator }
+              .to change(result_instance, :domain)
+              .from(nil).to(email[Truemail::RegexConstant::REGEX_EMAIL_PATTERN, 3])
+              .and not_change(result_instance, :mail_servers)
+              .and change(result_instance, :success).from(true).to(false)
+          end
+
+          it 'returns false' do
+            expect(mx_validator).to be(false)
+          end
         end
 
-        it 'returns true' do
-          expect(mx_validator).to be(true)
+        context 'without null mx' do
+          before { allow(Resolv).to receive(:getaddresses).and_return([host_address]) }
+
+          specify do
+            expect(mx_validator_instance).to receive(:hosts_from_mx_records?).and_call_original
+            expect(mx_validator_instance).to receive(:mx_records).and_call_original
+            expect(mx_validator_instance).to receive(:null_mx?).and_call_original
+            expect(mx_validator_instance).not_to receive(:hosts_from_cname_records?)
+            expect(mx_validator_instance).not_to receive(:host_from_a_record?)
+
+            expect { mx_validator }
+              .to change(result_instance, :domain)
+              .from(nil).to(email[Truemail::RegexConstant::REGEX_EMAIL_PATTERN, 3])
+              .and change(result_instance, :mail_servers)
+              .from([]).to(mail_servers_by_ip)
+              .and not_change(result_instance, :success)
+          end
+
+          it 'returns true' do
+            expect(mx_validator).to be(true)
+          end
         end
       end