diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index af86699f3..97f15278a 100644 --- a/app/controllers/passwords_controller.rb +++ b/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", diff --git a/config/config.yml b/config/config.yml index 0b37fcee1..fb7cbb0f7 100644 --- a/config/config.yml +++ b/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: "portus@example.com" name: "Portus" @@ -16,10 +18,24 @@ email: smtp: enabled: false address: "smtp.example.com" - port: 587, - user_name: "username@example.com" - 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/ diff --git a/config/environments/development.rb b/config/environments/development.rb index de135b332..957d52837 100644 --- a/config/environments/development.rb +++ b/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] diff --git a/config/initializers/mail.rb b/config/initializers/mail.rb index d4fab86e2..fa23aab6f 100644 --- a/config/initializers/mail.rb +++ b/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 diff --git a/lib/portus/mail.rb b/lib/portus/mail.rb new file mode 100644 index 000000000..ecc1719c5 --- /dev/null +++ b/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 diff --git a/spec/controllers/passwords_controller_spec.rb b/spec/controllers/passwords_controller_spec.rb index 7cb018deb..41b18230b 100644 --- a/spec/controllers/passwords_controller_spec.rb +++ b/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 diff --git a/spec/lib/portus/mail_spec.rb b/spec/lib/portus/mail_spec.rb new file mode 100644 index 000000000..bc9c89158 --- /dev/null +++ b/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" => "lala@example.com", + "name" => "test", + "smtp" => { "enabled": false } + }.freeze + end + + let(:basic) do + { + "from" => "lala@example.com", + "name" => "test", + "smtp" => { + "enabled" => true, + "address" => "address@example.com", + "port" => 567, + "domain" => "example.com", + "enable_starttls_auto" => false, + "openssl_verify_mode" => "none" + } + }.freeze + end + + let(:authentication) do + { + "from" => "lala@example.com", + "name" => "test", + "smtp" => { + "enabled" => true, + "address" => "address@example.com", + "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" => "lala@example.com", + "name" => "test", + "smtp" => { + "enabled" => true, + "address" => "address@example.com", + "port" => 567, + "domain" => "example.com", + "enable_starttls_auto" => true, + "openssl_verify_mode" => "peer", + "ssl_tls" => "tls" + } + }.freeze + end + + let(:notls_ca) do + { + "from" => "lala@example.com", + "name" => "test", + "smtp" => { + "enabled" => true, + "address" => "address@example.com", + "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" => "lala@example.com", + "name" => "test", + "smtp" => { + "enabled" => true, + "address" => "address@example.com", + "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" => "lal@ex.org", "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" => "lal@ex.org", "reply_to" => "lal@ex.org" } + 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