From 6fdb8e8d69ce4a5df26a313e70469beea4731af7 Mon Sep 17 00:00:00 2001 From: Thibaut Gorioux Date: Mon, 29 Apr 2024 22:44:24 +0200 Subject: [PATCH] Allow a self-hosted user to configure their SMTP settings directly from within the UI (#682) * Add setting fields to model * Allow to configure SMTP settings * Normalize locales * Cleanup locales * Remove 'coming soon' * fix test * Reset credentials * Reset development config * Check smtp spelling * Use post instead of get method * TLS ENV variable is more descriptive * Rework application mailer * Follow rails convention for mailer action params * Reset schema.rb to main * Test WIP * Add test for controller and mailer * Move tests from controller to model * Custom error message if settings are not all present * Comment smtp config in development env * Add default tls enabled value * Rubocop * Fix controller test * Reset credentials * Normalize locales * Test * fix test * Fix application mailer test that fails randomly * Error flash message instead of notice * Rework application mailer tests --- .env.example | 5 ++- .../settings/hostings_controller.rb | 21 ++++++++- app/mailers/application_mailer.rb | 14 +++++- app/mailers/notification_mailer.rb | 5 +++ app/models/setting.rb | 14 ++++++ app/views/settings/hostings/show.html.erb | 13 +++--- config/application.rb | 2 + config/environments/development.rb | 14 +++++- config/environments/production.rb | 2 +- .../locales/views/notification_mailer/en.yml | 7 +++ config/locales/views/settings/en.yml | 12 +++-- config/routes.rb | 4 +- .../settings/hostings_controller_test.rb | 34 ++++++++++++++ test/mailers/application_mailer_test.rb | 45 +++++++++++++++++++ test/models/setting_test.rb | 23 ++++++++++ 15 files changed, 200 insertions(+), 15 deletions(-) create mode 100644 app/mailers/notification_mailer.rb create mode 100644 config/locales/views/notification_mailer/en.yml create mode 100644 test/mailers/application_mailer_test.rb create mode 100644 test/models/setting_test.rb diff --git a/.env.example b/.env.example index 9fcb7ecf..bcc1cda4 100644 --- a/.env.example +++ b/.env.example @@ -9,7 +9,10 @@ SMTP_ADDRESS= SMTP_PORT=465 SMTP_USERNAME= SMTP_PASSWORD= -TLS=true +SMTP_TLS_ENABLED=true + +# Email Configuration +EMAIL_SENDER= # Database Configuration DB_HOST=localhost # May need to be changed to `DB_HOST=db` if using devcontainer diff --git a/app/controllers/settings/hostings_controller.rb b/app/controllers/settings/hostings_controller.rb index 2bae3b56..b64a926d 100644 --- a/app/controllers/settings/hostings_controller.rb +++ b/app/controllers/settings/hostings_controller.rb @@ -17,6 +17,24 @@ class Settings::HostingsController < ApplicationController end end + def send_test_email + unless Setting.smtp_settings_populated? + flash[:error] = t(".missing_smtp_setting_error") + render(:show, status: :unprocessable_entity) + return + end + + begin + NotificationMailer.with(user: Current.user).test_email.deliver_now + rescue => _e + flash[:error] = t(".error") + render :show, status: :unprocessable_entity + return + end + + redirect_to settings_hosting_path, notice: t(".success") + end + private def all_updates_valid? @errors = ActiveModel::Errors.new(Setting) @@ -37,12 +55,13 @@ class Settings::HostingsController < ApplicationController end def hosting_params - permitted_params = params.require(:setting).permit(:render_deploy_hook, :upgrades_mode) + permitted_params = params.require(:setting).permit(:render_deploy_hook, :upgrades_mode, :email_sender, :app_domain, :smtp_host, :smtp_port, :smtp_username, :smtp_password) result = {} result[:upgrades_mode] = permitted_params[:upgrades_mode] == "manual" ? "manual" : "auto" if permitted_params.key?(:upgrades_mode) result[:render_deploy_hook] = permitted_params[:render_deploy_hook] if permitted_params.key?(:render_deploy_hook) result[:upgrades_target] = permitted_params[:upgrades_mode] unless permitted_params[:upgrades_mode] == "manual" if permitted_params.key?(:upgrades_mode) + result.merge!(permitted_params.slice(:email_sender, :app_domain, :smtp_host, :smtp_port, :smtp_username, :smtp_password)) result end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index 3c34c814..2ed82df8 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -1,4 +1,16 @@ class ApplicationMailer < ActionMailer::Base - default from: "from@example.com" layout "mailer" + + after_action :set_self_host_settings, if: -> { ENV["SELF_HOSTING_ENABLED"] == "true" } + + private + + def set_self_host_settings + mail.from = Setting.email_sender + mail.delivery_method.settings.merge!({ address: Setting.smtp_host, + port: Setting.smtp_port, + user_name: Setting.smtp_username, + password: Setting.smtp_password, + tls: ENV.fetch("SMTP_TLS_ENABLED", "true") == "true" }) + end end diff --git a/app/mailers/notification_mailer.rb b/app/mailers/notification_mailer.rb new file mode 100644 index 00000000..025aa8e9 --- /dev/null +++ b/app/mailers/notification_mailer.rb @@ -0,0 +1,5 @@ +class NotificationMailer < ApplicationMailer + def test_email + mail(to: params[:user].email, subject: t(".test_email_subject"), body: t(".test_email_body")) + end +end diff --git a/app/models/setting.rb b/app/models/setting.rb index 4e73bef2..88cfdad1 100644 --- a/app/models/setting.rb +++ b/app/models/setting.rb @@ -16,4 +16,18 @@ class Setting < RailsSettings::Base type: :string, default: ENV.fetch("UPGRADES_TARGET", "release"), validates: { inclusion: { in: %w[release commit] } } + + field :app_domain, type: :string, default: ENV["APP_DOMAIN"] + field :email_sender, type: :string, default: ENV["EMAIL_SENDER"] + + scope :smtp_settings do + field :smtp_host, type: :string, read_only: true, default: ENV["SMTP_ADDRESS"] + field :smtp_port, type: :string, read_only: true, default: ENV["SMTP_PORT"] + field :smtp_username, type: :string, read_only: true, default: ENV["SMTP_USERNAME"] + field :smtp_password, type: :string, read_only: true, default: ENV["SMTP_PASSWORD"] + end + + def self.smtp_settings_populated? + Setting.defined_fields.select { |f| f.scope == :smtp_settings }.map(&:read).all?(&:present?) + end end diff --git a/app/views/settings/hostings/show.html.erb b/app/views/settings/hostings/show.html.erb index fe883d7d..2e8dd0ae 100644 --- a/app/views/settings/hostings/show.html.erb +++ b/app/views/settings/hostings/show.html.erb @@ -51,11 +51,12 @@

<%= t(".smtp_settings.description") %>

- <%= form.text_field :smtp_domain, disabled: true, label: t(".smtp_settings.domain"), placeholder: t(".smtp_settings.domain_placeholder") %> - <%= form.text_field :smtp_host, disabled: true, label: t(".smtp_settings.host"), placeholder: t(".smtp_settings.host_placeholder") %> - <%= form.number_field :smtp_port, disabled: true, label: t(".smtp_settings.port"), placeholder: t(".smtp_settings.port_placeholder") %> - <%= form.text_field :smtp_username, disabled: true, label: t(".smtp_settings.username"), placeholder: t(".smtp_settings.username_placeholder") %> - <%= form.password_field :smtp_password, disabled: true, label: t(".smtp_settings.password"), placeholder: t(".smtp_settings.password_placeholder") %> + <%= form.text_field :email_sender, label: t(".email_sender"), placeholder: t(".email_sender_placeholder"), value: Setting.email_sender, data: { "auto-submit-form-target" => "auto" } %> + <%= form.text_field :app_domain, label: t(".domain"), placeholder: t(".domain_placeholder"), value: Setting.app_domain, data: { "auto-submit-form-target" => "auto" } %> + <%= form.text_field :smtp_host, label: t(".smtp_settings.host"), placeholder: t(".smtp_settings.host_placeholder"), value: Setting.smtp_host, data: { "auto-submit-form-target" => "auto" } %> + <%= form.number_field :smtp_port, label: t(".smtp_settings.port"), placeholder: t(".smtp_settings.port_placeholder"), value: Setting.smtp_port, data: { "auto-submit-form-target" => "auto" } %> + <%= form.text_field :smtp_username, label: t(".smtp_settings.username"), placeholder: t(".smtp_settings.username_placeholder"), value: Setting.smtp_username, data: { "auto-submit-form-target" => "auto" } %> + <%= form.password_field :smtp_password, label: t(".smtp_settings.password"), placeholder: t(".smtp_settings.password_placeholder"), value: Setting.smtp_password, data: { "auto-submit-form-target" => "auto" } %>
@@ -68,7 +69,7 @@
- + <%= link_to t(".smtp_settings.send_test_email_button"), send_test_email_settings_hosting_path, data: { turbo_method: :post }, class:"bg-gray-50 text-gray-900 text-sm font-medium rounded-lg px-3 py-2" %>
diff --git a/config/application.rb b/config/application.rb index 861d75dd..d2dc125b 100644 --- a/config/application.rb +++ b/config/application.rb @@ -23,5 +23,7 @@ module Maybe # # config.time_zone = "Central Time (US & Canada)" # config.eager_load_paths << Rails.root.join("extras") + + config.action_mailer.default_options = { from: ENV["MAILER_SENDER"] } end end diff --git a/config/environments/development.rb b/config/environments/development.rb index ef4f7037..3f760a76 100644 --- a/config/environments/development.rb +++ b/config/environments/development.rb @@ -38,10 +38,22 @@ Rails.application.configure do # Don't care if the mailer can't send. config.action_mailer.raise_delivery_errors = false + config.action_mailer.delivery_method = :letter_opener + + # Uncomment to send emails in development + # config.action_mailer.raise_delivery_errors = true + # config.action_mailer.delivery_method = :smtp + # config.action_mailer.smtp_settings = { + # address: ENV["SMTP_ADDRESS"], + # port: ENV["SMTP_PORT"], + # user_name: ENV["SMTP_USERNAME"], + # password: ENV["SMTP_PASSWORD"], + # tls: ENV.fetch("SMTP_TLS_ENABLED", "true") == "true" + # } + config.action_mailer.perform_caching = false - config.action_mailer.delivery_method = :letter_opener config.action_mailer.perform_deliveries = true config.action_mailer.default_url_options = { host: "localhost", port: 3000 } diff --git a/config/environments/production.rb b/config/environments/production.rb index baac7932..b40ce3b7 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -74,7 +74,7 @@ Rails.application.configure do port: ENV["SMTP_PORT"], user_name: ENV["SMTP_USERNAME"], password: ENV["SMTP_PASSWORD"], - tls: ENV["TLS"] == "true" + tls: ENV["SMTP_TLS_ENABLED"] == "true" } # Ignore bad email addresses and do not raise email delivery errors. diff --git a/config/locales/views/notification_mailer/en.yml b/config/locales/views/notification_mailer/en.yml new file mode 100644 index 00000000..c15894c1 --- /dev/null +++ b/config/locales/views/notification_mailer/en.yml @@ -0,0 +1,7 @@ +--- +en: + notification_mailer: + test_email: + test_email_body: Congratulation ! Connection to the SMTP server is now correctly + configured. + test_email_subject: SMTP settings verified ! diff --git a/config/locales/views/settings/en.yml b/config/locales/views/settings/en.yml index 8a0c2c89..6837a387 100644 --- a/config/locales/views/settings/en.yml +++ b/config/locales/views/settings/en.yml @@ -2,7 +2,15 @@ en: settings: hostings: + send_test_email: + error: 'Configuration error: Test email could not be sent' + missing_smtp_setting_error: Ensure that all smtp settings are filled in + success: Test email has been sent successfully show: + domain: App Domain + domain_placeholder: mydomain.com + email_sender: Email Sender + email_sender_placeholder: user@mydomain.com general_settings_title: General Settings page_title: Self-Hosting provider_settings: @@ -13,8 +21,6 @@ en: smtp_settings: description: Configure outgoing mail server settings for notifications and alerts - domain: App Domain - domain_placeholder: mydomain.com host: SMTP Host host_placeholder: smtp.gmail.com password: Password @@ -24,7 +30,7 @@ en: send_test_email: Send Test Email send_test_email_button: Send Test Email send_test_email_description: Verify SMTP settings by sending a test email - title: SMTP Email Configuration (coming soon...) + title: SMTP Email Configuration username: Username username_placeholder: username@gmail.com upgrades: diff --git a/config/routes.rb b/config/routes.rb index 971bdfb5..0133e25e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -15,7 +15,9 @@ Rails.application.routes.draw do resource :preferences, only: %i[show update] resource :notifications, only: %i[show update] resource :billing, only: %i[show update] - resource :hosting, only: %i[show update] + resource :hosting, only: %i[show update] do + post :send_test_email, on: :collection + end resource :security, only: %i[show update] end diff --git a/test/controllers/settings/hostings_controller_test.rb b/test/controllers/settings/hostings_controller_test.rb index 09743ad1..cb283e29 100644 --- a/test/controllers/settings/hostings_controller_test.rb +++ b/test/controllers/settings/hostings_controller_test.rb @@ -44,4 +44,38 @@ class Settings::HostingsControllerTest < ActionDispatch::IntegrationTest assert_equal "release", Setting.upgrades_target assert_equal NEW_RENDER_DEPLOY_HOOK, Setting.render_deploy_hook end + + test " #send_test_email if smtp settings are populated try to send an email and redirect with notice" do + Setting.stubs(:smtp_settings_populated?).returns(true) + + test_email_mock = mock + test_email_mock.expects(:deliver_now) + + mailer_mock = mock + mailer_mock.expects(:test_email).returns(test_email_mock) + + NotificationMailer.expects(:with).with(user: users(:family_admin)).returns(mailer_mock) + + post send_test_email_settings_hosting_path + assert_response :found + assert controller.flash[:notice].present? + end + + test "#send_test_email with one blank smtp setting" do + Setting.stubs(:smtp_settings_populated?).returns(false) + NotificationMailer.expects(:with).never + + post send_test_email_settings_hosting_path + assert_response :unprocessable_entity + assert controller.flash[:error].present? + end + + test "#send_test_email when sending the email raise an error" do + Setting.stubs(:smtp_settings_populated?).returns(true) + NotificationMailer.stubs(:with).raises(StandardError) + + post send_test_email_settings_hosting_path + assert_response :unprocessable_entity + assert controller.flash[:error].present? + end end diff --git a/test/mailers/application_mailer_test.rb b/test/mailers/application_mailer_test.rb new file mode 100644 index 00000000..243fc6e7 --- /dev/null +++ b/test/mailers/application_mailer_test.rb @@ -0,0 +1,45 @@ +require "test_helper" + +class ApplicationMailerTest < ActionMailer::TestCase + setup do + class TestMailer < ApplicationMailer + def test_email + mail(to: "testto@email.com", from: "testfrom@email.com", subject: "Test email subject", body: "Test email body") + end + end + end + + test "should use self host settings when self host enabled" do + ENV["SELF_HOSTING_ENABLED"] = "true" + + smtp_host = "smtp.example.com" + smtp_port = 466 + smtp_username = "user@example.com" + smtp_password = "password" + email_sender = "notification@example.com" + + smtp_settings_from_settings = { address: smtp_host, + port: smtp_port, + user_name: smtp_username, + password: smtp_password } + + Setting.stubs(:smtp_host).returns(smtp_host) + Setting.stubs(:smtp_port).returns(smtp_port) + Setting.stubs(:smtp_username).returns(smtp_username) + Setting.stubs(:smtp_password).returns(smtp_password) + Setting.stubs(:email_sender).returns(email_sender) + + TestMailer.test_email.deliver_now + assert_emails 1 + assert_equal smtp_settings_from_settings, ActionMailer::Base.deliveries.first.delivery_method.settings.slice(:address, :port, :user_name, :password) + end + + test "should use regular env settings when self host disabled" do + ENV["SELF_HOSTING_ENABLED"] = "false" + + TestMailer.test_email.deliver_now + + assert_emails 1 + assert_nil ActionMailer::Base.deliveries.first.delivery_method.settings[:address] + end +end diff --git a/test/models/setting_test.rb b/test/models/setting_test.rb new file mode 100644 index 00000000..a205b8ce --- /dev/null +++ b/test/models/setting_test.rb @@ -0,0 +1,23 @@ +require "test_helper" + +class AccountTest < ActiveSupport::TestCase + test "#send_test_email return true if all smtp settings are populated" do + Setting.smtp_host = "smtp.example.com" + Setting.smtp_port = 466 + Setting.smtp_username = "user@example.com" + Setting.smtp_password = "notification@example.com" + Setting.email_sender = "password" + + assert Setting.smtp_settings_populated? + end + + test "#send_test_email return false if one smtp settings is not populated" do + Setting.smtp_host = "" + Setting.smtp_port = 466 + Setting.smtp_username = "user@example.com" + Setting.smtp_password = "notification@example.com" + Setting.email_sender = "password" + + assert_not Setting.smtp_settings_populated? + end +end