Expanded configuration for the mailer

The current configuration options were missing some important keys for
ActiveMailer on some scenarios.

Fixes #1876
Fixes #1922

Signed-off-by: Miquel Sabaté Solà <[email protected]>

app/controllers/passwords_controller.rb

@@ -19,8 +19,9 @@ def create
     else
       redirect_to new_user_password_path, alert: resource.errors.full_messages, float: true
     end
-  rescue *::Portus::Errors::NET => e
-    msg = "#{e}: #{::Portus::Errors.message_from_exception(e)}"
+  rescue *::Portus::Errors::NET, ::Net::SMTPAuthenticationError => e
+    from = ::Portus::Errors.message_from_exception(e)
+    msg  = "#{e}: #{from if from}"
     Rails.logger.tagged("Mailer") { Rails.logger.info msg }
     redirect_to new_user_password_path,
                 alert: "Something went wrong. Check the configuration of Portus",

config/config.yml

@@ -3,7 +3,9 @@
 # (it will be ignored by git). For more info, you can read the dedicated page
 # here: http://port.us.org/docs/Configuring-Portus.html.
 
-# Settings for the Portus mailer.
+# Settings for the Portus mailer. It's strongly recommended to read the
+# following documentation link before configuring the mailer:
+#   http://port.us.org/docs/Configuring-Portus.html#email-configuration
 email:
   from: "[email protected]"
   name: "Portus"
@@ -16,10 +18,24 @@ email:
   smtp:
     enabled: false
     address: "smtp.example.com"
-    port: 587,
-    user_name: "[email protected]"
-    password: "password"
-    domain: "example.com"
+    port:    587,
+    domain:  "example.com"
+
+    ##
+    # SSL.
+
+    ssl_tls:              ""
+    enable_starttls_auto: false
+    openssl_verify_mode:  "none"
+    ca_path:              ""
+    ca_file:              ""
+
+    ##
+    # Authentication
+
+    user_name:      ""
+    password:       ""
+    authentication: "login"
 
 # If enabled, then the profile picture will be picked from the Gravatar
 # associated with each user. See: https://en.gravatar.com/

config/environments/development.rb

@@ -41,7 +41,10 @@
   # config.action_view.raise_on_missing_translations = true
 
   # Set this to true when debugging a mailer.
-  config.action_mailer.raise_delivery_errors = false
+  config.action_mailer.raise_delivery_errors = true
+  # Uncomment the following two lines to test the mailer in the real world.
+  # config.action_mailer.perform_deliveries = true
+  # config.action_mailer.raise_delivery_errors = true
 
   # Control which IP's have access to the console. In Dev mode we can allow all private networks
   config.web_console.whitelisted_ips = %w[127.0.0.1/1 ::1 10.0.0.0/8 172.16.0.0/12 192.168.0.0/16]

config/initializers/mail.rb

@@ -1,41 +1,25 @@
 # frozen_string_literal: true
 
-def check_email!(key)
-  value = APP_CONFIG["email"][key]
-  return if value.match?(Devise.email_regexp)
-  raise "Mail: bad config value for '#{key}'. '#{value}' is not a proper email..."
-end
+require "portus/mail"
 
 unless Rails.env.test?
-  check_email!("from")
-  check_email!("reply_to") if APP_CONFIG["email"]["reply_to"].present?
-
-  # If SMTP was set, then use it as the delivery method and configure it with the
-  # given config.
+  # In some weird cases APP_CONFIG is not even there. In these cases, just go
+  # back to sendmail.
+  if defined?(APP_CONFIG)
+    # Check that emails have the proper format.
+    mail = ::Portus::Mail::Utils.new(APP_CONFIG["email"])
+    mail.check_email_configuration!
 
-  if defined?(APP_CONFIG) && APP_CONFIG["email"]["smtp"]["enabled"]
-    Portus::Application.config.action_mailer.delivery_method = :smtp
-    smtp = APP_CONFIG["email"]["smtp"]
-    smtp_settings = {
-      address:              smtp["address"],
-      port:                 smtp["port"],
-      domain:               smtp["domain"],
-      enable_starttls_auto: false
-    }
-    if smtp["user_name"].blank?
-      Rails.logger.info "No smtp username supplied, not using smtp authentication"
+    # Fetch SMTP settings. On success, it will set SMTP as the delivery method,
+    # otherwise we fall back to sendmail.
+    settings = mail.smtp_settings
+    if settings
+      Portus::Application.config.action_mailer.delivery_method = :smtp
+      ActionMailer::Base.smtp_settings = settings
     else
-      auth_settings = {
-        user_name:            smtp["user_name"],
-        password:             smtp["password"],
-        authentication:       :login,
-        enable_starttls_auto: true
-      }
-      smtp_settings = smtp_settings.merge(auth_settings)
+      Portus::Application.config.action_mailer.delivery_method = :sendmail
     end
-    ActionMailer::Base.smtp_settings = smtp_settings
   else
-    # If SMTP is not enabled, then go for sendmail.
     Portus::Application.config.action_mailer.delivery_method = :sendmail
   end
 end

lib/portus/mail.rb

@@ -0,0 +1,91 @@
+# frozen_string_literal: true
+
+module Portus
+  # Mail implements a set of utilities for mailing purposes.
+  module Mail
+    # ConfigurationError is raised when the given configuration has semantic
+    # problems (e.g. malformed emails).
+    class ConfigurationError < StandardError; end
+
+    # Utils is a set of utility methods for mails.
+    class Utils
+      # config contains only the email configuration (i.e. APP_CONFIG["email"]
+      # instead of APP_CONFIG directly).
+      def initialize(config)
+        @config = config
+      end
+
+      # check_email_configuration! raises a ::Portus::Mail::ConfigurationError
+      # when any of the relevant emails is badly formatted.
+      def check_email_configuration!
+        check_email!("from")
+        check_email!("reply_to") if @config["reply_to"].present?
+      end
+
+      # Returns a hash with the SMTP settings to be used by the mailer.
+      def smtp_settings
+        smtp = @config["smtp"]
+        return unless smtp["enabled"]
+
+        {
+          address: smtp["address"],
+          port:    smtp["port"],
+          domain:  smtp["domain"]
+        }.merge(ssl_settings).merge(authentication_settings)
+      end
+
+      protected
+
+      # Returns the SMTP settings around SSL.
+      def ssl_settings
+        {
+          enable_starttls_auto: @config["smtp"]["enable_starttls_auto"],
+          openssl_verify_mode:  @config["smtp"]["openssl_verify_mode"]
+        }.merge(ssl_tls).merge(ca)
+      end
+
+      # Returns a hash with either SSL or TLS enabled if the configuration
+      # specifies it. It returns an empty hash when no SSL/TLS has been
+      # configured.
+      def ssl_tls
+        if @config["smtp"]["ssl_tls"] == "ssl"
+          { ssl: true }
+        elsif @config["smtp"]["ssl_tls"] == "tls"
+          { tls: true }
+        else
+          {}
+        end
+      end
+
+      # Returns a hash with the `ca_path` and the `ca_file` options as specified
+      # in the configuration.
+      def ca
+        {}.tap do |hsh|
+          hsh[:ca_path] = @config["smtp"]["ca_path"] if @config["smtp"]["ca_path"]
+          hsh[:ca_file] = @config["smtp"]["ca_file"] if @config["smtp"]["ca_file"]
+        end
+      end
+
+      # Returns a hash with the authentication settings as specified in the
+      # configuration. It returns an empty hash if the `user_name` field has
+      # been left blank.
+      def authentication_settings
+        return {} if @config["smtp"]["user_name"].blank?
+        {
+          user_name:      @config["smtp"]["user_name"],
+          password:       @config["smtp"]["password"],
+          authentication: @config["smtp"]["authentication"]
+        }
+      end
+
+      # check_email! raises an error when the given configuration key has a
+      # badly formatted value.
+      def check_email!(key)
+        value = @config[key]
+        return if value.match?(Devise.email_regexp)
+        raise ConfigurationError,
+              "Mail: bad config value for '#{key}'. '#{value}' is not a proper email..."
+      end
+    end
+  end
+end

spec/controllers/passwords_controller_spec.rb

@@ -45,6 +45,14 @@
     expect(response.status).to eq 302
   end
 
+  it "redirects on ::Net::SMTPAuthenticationError" do
+    allow(User).to receive(:send_reset_password_instructions) do
+      raise ::Net::SMTPAuthenticationError, "error"
+    end
+    post :create, "user" => { "email" => @user.email }
+    expect(response.status).to eq 302
+  end
+
   describe "LDAP support is enabled" do
     before do
       APP_CONFIG["ldap"]["enabled"] = true

spec/lib/portus/mail_spec.rb

@@ -0,0 +1,163 @@
+# frozen_string_literal: true
+
+require "rails_helper"
+
+describe ::Portus::Mail::Utils do
+  let(:no_smtp) do
+    {
+      "from" => "[email protected]",
+      "name" => "test",
+      "smtp" => { "enabled": false }
+    }.freeze
+  end
+
+  let(:basic) do
+    {
+      "from" => "[email protected]",
+      "name" => "test",
+      "smtp" => {
+        "enabled"              => true,
+        "address"              => "[email protected]",
+        "port"                 => 567,
+        "domain"               => "example.com",
+        "enable_starttls_auto" => false,
+        "openssl_verify_mode"  => "none"
+      }
+    }.freeze
+  end
+
+  let(:authentication) do
+    {
+      "from" => "[email protected]",
+      "name" => "test",
+      "smtp" => {
+        "enabled"              => true,
+        "address"              => "[email protected]",
+        "port"                 => 567,
+        "domain"               => "example.com",
+        "enable_starttls_auto" => false,
+        "openssl_verify_mode"  => "none",
+        "user_name"            => "mssola",
+        "password"             => "password",
+        "authentication"       => "login"
+      }
+    }.freeze
+  end
+
+  let(:tls_noca) do
+    {
+      "from" => "[email protected]",
+      "name" => "test",
+      "smtp" => {
+        "enabled"              => true,
+        "address"              => "[email protected]",
+        "port"                 => 567,
+        "domain"               => "example.com",
+        "enable_starttls_auto" => true,
+        "openssl_verify_mode"  => "peer",
+        "ssl_tls"              => "tls"
+      }
+    }.freeze
+  end
+
+  let(:notls_ca) do
+    {
+      "from" => "[email protected]",
+      "name" => "test",
+      "smtp" => {
+        "enabled"              => true,
+        "address"              => "[email protected]",
+        "port"                 => 567,
+        "domain"               => "example.com",
+        "enable_starttls_auto" => true,
+        "openssl_verify_mode"  => "peer",
+        "ca_path"              => "/lala",
+        "ca_file"              => "/lala"
+      }
+    }.freeze
+  end
+
+  let(:ssl_ca) do
+    {
+      "from" => "[email protected]",
+      "name" => "test",
+      "smtp" => {
+        "enabled"              => true,
+        "address"              => "[email protected]",
+        "port"                 => 567,
+        "domain"               => "example.com",
+        "enable_starttls_auto" => true,
+        "openssl_verify_mode"  => "peer",
+        "ca_path"              => "/lala",
+        "ca_file"              => "/lala",
+        "ssl_tls"              => "ssl"
+      }
+    }.freeze
+  end
+
+  describe "#check_email_configuration!" do
+    it "raises an exception on malformed 'from'" do
+      expect do
+        described_class.new("from" => "!").check_email_configuration!
+      end.to raise_error(::Portus::Mail::ConfigurationError)
+    end
+
+    it "raises an exception on malformed 'reply_to'" do
+      expect do
+        described_class.new("from" => "[email protected]", "reply_to" => "!").check_email_configuration!
+      end.to raise_error(::Portus::Mail::ConfigurationError)
+    end
+
+    it "does not raise an exception when everything is alright" do
+      expect do
+        hsh = { "from" => "[email protected]", "reply_to" => "[email protected]" }
+        described_class.new(hsh).check_email_configuration!
+      end.not_to raise_error
+    end
+  end
+
+  describe "#smtp_settings" do
+    it "returns nil when disabled" do
+      res = described_class.new(no_smtp).smtp_settings
+      expect(res).to be_nil
+    end
+
+    it "returns a basic smtp configuration" do
+      res = described_class.new(basic).smtp_settings
+      %i[address port domain enable_starttls_auto openssl_verify_mode].each do |key|
+        expect(res[key]).not_to be_nil
+      end
+    end
+
+    it "returns a configuration with authentication" do
+      res = described_class.new(authentication).smtp_settings
+      %i[address port domain enable_starttls_auto openssl_verify_mode
+         user_name password authentication].each do |key|
+        expect(res[key]).not_to be_nil
+      end
+    end
+
+    it "returns a configuration with SSL (ssl/tls and no ca)" do
+      res = described_class.new(tls_noca).smtp_settings
+      %i[address port domain enable_starttls_auto openssl_verify_mode tls].each do |key|
+        expect(res[key]).not_to be_nil
+      end
+    end
+
+    it "returns a configuration with SSL (ssl/tls and ca)" do
+      res = described_class.new(ssl_ca).smtp_settings
+      %i[address port domain enable_starttls_auto openssl_verify_mode ssl
+         ca_file ca_path].each do |key|
+        expect(res[key]).not_to be_nil
+      end
+    end
+
+    it "returns a configuration with SSL (no ssl/tls and ca)" do
+      res = described_class.new(notls_ca).smtp_settings
+      %i[address port domain enable_starttls_auto openssl_verify_mode
+         ca_file ca_path].each do |key|
+        expect(res[key]).not_to be_nil
+      end
+    end
+  end
+end