From e38196df6fd8860ccdf79f416db31cb0694fca31 Mon Sep 17 00:00:00 2001 From: Mac Siri Date: Mon, 14 Jun 2021 10:29:43 -0400 Subject: [PATCH] Create Settings::SMTP (#13943) * Fix typo * Add SMTP configs to Settings::General * Expand Settings's show page * Expand Settings::General's constants * Move smtp_enabled? logic to ApplicationMailer * Apply suggestions from code review Co-authored-by: Ridhwana * Add missing descriptions and placeholders * Remove production guard clause * Change delivery_method to a callback * Create Settings::SMTP * Run migration * Move constants * Remove SMTP from Settings::General * Create SMTPSettingsController * Add back guard clause * Change which perform_deliveries configuration to use from * Update config/environments/production.rb * Rename migration to singular * Run migration again * Fix name * Alphabetize and add validation for authentication * Move settings and enabled? logic to Settings::SMTP * Change after_action to before_action * Fix broken spec * Fix broken spec * Create SMTP specs * Provide default for port * Create spec for ApplicationMailer * Fix broken spec * Move smtp_enabled? * Search and replace Co-authored-by: Ridhwana --- .../settings/smtp_settings_controller.rb | 9 +++++ app/lib/constants/settings/smtp.rb | 33 +++++++++++++++ app/mailers/application_mailer.rb | 12 ++++++ app/models/forem_instance.rb | 2 +- app/models/settings/smtp.rb | 40 +++++++++++++++++++ app/views/admin/settings/show.html.erb | 23 +++++++++++ config/environments/production.rb | 21 +--------- config/initializers/inflections.rb | 6 +-- config/routes/admin.rb | 1 + .../20210609164958_create_settings_smtp.rb | 16 ++++++++ db/schema.rb | 10 ++++- spec/mailers/application_mailer_spec.rb | 27 +++++++++++++ spec/mailers/notify_mailer_spec.rb | 4 ++ spec/models/follow_spec.rb | 1 + spec/models/forem_instance_spec.rb | 19 +++++++++ spec/models/message_spec.rb | 1 + spec/models/settings/smtp_spec.rb | 40 +++++++++++++++++++ spec/requests/admin/chat_channel_spec.rb | 1 + spec/requests/admin/feedback_messages_spec.rb | 1 + spec/requests/admin/users_spec.rb | 1 + spec/requests/feedback_messages_spec.rb | 4 ++ spec/requests/user/user_settings_spec.rb | 1 + spec/services/exporter/service_spec.rb | 7 +++- spec/system/admin/user_email_tools_spec.rb | 1 + .../organizations/delete_worker_spec.rb | 1 + spec/workers/users/delete_worker_spec.rb | 4 ++ 26 files changed, 260 insertions(+), 26 deletions(-) create mode 100644 app/controllers/admin/settings/smtp_settings_controller.rb create mode 100644 app/lib/constants/settings/smtp.rb create mode 100644 app/models/settings/smtp.rb create mode 100644 db/migrate/20210609164958_create_settings_smtp.rb create mode 100644 spec/mailers/application_mailer_spec.rb create mode 100644 spec/models/settings/smtp_spec.rb diff --git a/app/controllers/admin/settings/smtp_settings_controller.rb b/app/controllers/admin/settings/smtp_settings_controller.rb new file mode 100644 index 000000000..8f80230a0 --- /dev/null +++ b/app/controllers/admin/settings/smtp_settings_controller.rb @@ -0,0 +1,9 @@ +module Admin + module Settings + class SMTPSettingsController < Admin::Settings::BaseController + def authorization_resource + ::Settings::SMTP + end + end + end +end diff --git a/app/lib/constants/settings/smtp.rb b/app/lib/constants/settings/smtp.rb new file mode 100644 index 000000000..bbc550565 --- /dev/null +++ b/app/lib/constants/settings/smtp.rb @@ -0,0 +1,33 @@ +module Constants + module Settings + module SMTP + DETAILS = { + address: { + description: "Address of the remote mail server", + placeholder: "ie. smtp.gmail.com" + }, + port: { + description: "The port that your mail server runs on", + placeholder: "25" + }, + authentication: { + description: " If your mail server requires authentication, " \ + "you need to specify the authentication type here", + placeholder: "ie. plain, login, or cram_md5" + }, + user_name: { + description: "If your mail server requires authentication, copy the username from your server", + placeholder: "" + }, + password: { + description: "If your mail server requires authentication, copy the password from your server", + placeholder: "" + }, + domain: { + description: "If you need to specify a HELO domain, you can do it here", + placeholder: "" + } + }.freeze + end + end +end diff --git a/app/mailers/application_mailer.rb b/app/mailers/application_mailer.rb index db4337163..ba669a9a7 100644 --- a/app/mailers/application_mailer.rb +++ b/app/mailers/application_mailer.rb @@ -8,6 +8,8 @@ class ApplicationMailer < ActionMailer::Base helper AuthenticationHelper before_action :use_custom_host + before_action :set_delivery_options + before_action :set_perform_deliveries default( from: -> { email_from }, @@ -36,4 +38,14 @@ class ApplicationMailer < ActionMailer::Base def use_custom_host ActionMailer::Base.default_url_options[:host] = Settings::General.app_domain end + + def set_perform_deliveries + self.perform_deliveries = ForemInstance.smtp_enabled? + end + + protected + + def set_delivery_options + self.smtp_settings = Settings::SMTP.settings + end end diff --git a/app/models/forem_instance.rb b/app/models/forem_instance.rb index 1da2c2008..249150558 100644 --- a/app/models/forem_instance.rb +++ b/app/models/forem_instance.rb @@ -25,6 +25,6 @@ class ForemInstance end def self.smtp_enabled? - Rails.configuration.action_mailer.perform_deliveries + (Settings::SMTP.user_name.present? && Settings::SMTP.password.present?) || ENV["SENDGRID_API_KEY"].present? end end diff --git a/app/models/settings/smtp.rb b/app/models/settings/smtp.rb new file mode 100644 index 000000000..ff70cb0b2 --- /dev/null +++ b/app/models/settings/smtp.rb @@ -0,0 +1,40 @@ +module Settings + class SMTP < RailsSettings::Base + self.table_name = :settings_smtp + + OPTIONS = %i[address port authentication user_name password domain].freeze + + # The configuration is cached, change this if you want to force update + # the cache, or call Settings::Smtp.clear_cache + cache_prefix { "v1" } + + field :address, type: :string, default: ApplicationConfig["SMTP_ADDRESS"] + field :authentication, type: :string, default: ApplicationConfig["SMTP_AUTHENTICATION"], + validates: { inclusion: %w[plain login cram_md5] } + field :domain, type: :string, default: ApplicationConfig["SMTP_DOMAIN"] + field :password, type: :string, default: ApplicationConfig["SMTP_PASSWORD"] + field :port, type: :integer, default: ApplicationConfig["SMTP_PORT"].presence || 25 + field :user_name, type: :string, default: ApplicationConfig["SMTP_USER_NAME"] + + class << self + def settings + return sendgrid_settings if ENV["SENDGRID_API_KEY"].present? + + keys.index_with { |k| public_send(k) }.symbolize_keys + end + + private + + def sendgrid_settings + { + address: "smtp.sendgrid.net", + port: 587, + authentication: :plain, + user_name: "apikey", + password: ENV["SENDGRID_API_KEY"], + domain: ::Settings::General.app_domain + } + end + end + end +end diff --git a/app/views/admin/settings/show.html.erb b/app/views/admin/settings/show.html.erb index 275a4a412..533ac029f 100644 --- a/app/views/admin/settings/show.html.erb +++ b/app/views/admin/settings/show.html.erb @@ -1036,6 +1036,29 @@ <% end %> + <%= form_for(Settings::SMTP.new, url: admin_settings_smtp_settings_path) do |f| %> +
+ <%= render partial: "admin/shared/card_header", + locals: { header: "SMTP Settings", state: "collapse", target: "smtpSettingsBodyContainer", expanded: false } %> +
+
+ <% Settings::SMTP::OPTIONS.each do |config_key| %> +
+ <%= admin_config_label config_key %> + <%= admin_config_description Constants::Settings::SMTP::DETAILS[config_key][:description] %> + <%= f.text_field config_key, + class: "crayons-textfield", + value: Settings::SMTP.public_send(config_key), + placeholder: Constants::Settings::SMTP::DETAILS[config_key][:placeholder] %> + +
+ <% end %> + <%= render "form_submission", f: f %> +
+
+
+ <% end %> + <%= form_for(Settings::General.new, url: admin_settings_general_settings_path) do |f| %>
<%= render partial: "admin/shared/card_header", diff --git a/config/environments/production.rb b/config/environments/production.rb index 9b39682ff..b693317d4 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -127,27 +127,8 @@ Rails.application.configure do protocol = ENV["APP_PROTOCOL"] || "http://" config.action_mailer.delivery_method = :smtp - config.action_mailer.perform_deliveries = ENV["SMTP_PASWORD"].present? || ENV["SENDGRID_API_KEY"].present? + config.action_mailer.perform_deliveries = false config.action_mailer.default_url_options = { host: protocol + ENV["APP_DOMAIN"].to_s } - ActionMailer::Base.smtp_settings = if ENV["SENDGRID_API_KEY"].present? - { - address: "smtp.sendgrid.net", - port: 587, - authentication: :plain, - user_name: "apikey", - password: ENV["SENDGRID_API_KEY"], - domain: ENV["APP_DOMAIN"] - } - else - { - address: ENV["SMTP_ADDRESS"], - port: ENV["SMTP_PORT"], - authentication: ENV["SMTP_AUTHENTICATION"], - user_name: ENV["SMTP_USER_NAME"], - password: ENV["SMTP_PASSWORD"], - domain: ENV["SMTP_DOMAIN"] - } - end if ENV["HEROKU_APP_URL"].present? && ENV["HEROKU_APP_URL"] != ENV["APP_DOMAIN"] config.middleware.use Rack::HostRedirect, diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb index ac033bf9d..67012a382 100644 --- a/config/initializers/inflections.rb +++ b/config/initializers/inflections.rb @@ -11,6 +11,6 @@ # end # These inflection rules are supported but not enabled by default: -# ActiveSupport::Inflector.inflections(:en) do |inflect| -# inflect.acronym 'RESTful' -# end +ActiveSupport::Inflector.inflections(:en) do |inflect| + inflect.acronym "SMTP" +end diff --git a/config/routes/admin.rb b/config/routes/admin.rb index 536e1d5c4..d737fdc4c 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -21,6 +21,7 @@ namespace :admin do resources :general_settings, only: [:create] resources :mandatory_settings, only: [:create] resources :rate_limits, only: [:create] + resources :smtp_settings, only: [:create] resources :user_experiences, only: [:create] end namespace :users do diff --git a/db/migrate/20210609164958_create_settings_smtp.rb b/db/migrate/20210609164958_create_settings_smtp.rb new file mode 100644 index 000000000..e0bc272b3 --- /dev/null +++ b/db/migrate/20210609164958_create_settings_smtp.rb @@ -0,0 +1,16 @@ +class CreateSettingsSMTP < ActiveRecord::Migration[6.1] + def self.up + create_table :settings_smtp do |t| + t.string :var, null: false + t.text :value, null: true + + t.timestamps + end + + add_index :settings_smtp, :var, unique: true + end + + def self.down + drop_table :settings_smtp + end +end diff --git a/db/schema.rb b/db/schema.rb index b7c671be4..4d64f3364 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2021_06_09_103939) do +ActiveRecord::Schema.define(version: 2021_06_09_164958) do # These are extensions that must be enabled in order to support this database enable_extension "citext" @@ -1118,6 +1118,14 @@ ActiveRecord::Schema.define(version: 2021_06_09_103939) do t.index ["var"], name: "index_settings_rate_limits_on_var", unique: true end + create_table "settings_smtp", force: :cascade do |t| + t.datetime "created_at", precision: 6, null: false + t.datetime "updated_at", precision: 6, null: false + t.text "value" + t.string "var", null: false + t.index ["var"], name: "index_settings_smtp_on_var", unique: true + end + create_table "settings_user_experiences", force: :cascade do |t| t.datetime "created_at", precision: 6, null: false t.datetime "updated_at", precision: 6, null: false diff --git a/spec/mailers/application_mailer_spec.rb b/spec/mailers/application_mailer_spec.rb new file mode 100644 index 000000000..58c262432 --- /dev/null +++ b/spec/mailers/application_mailer_spec.rb @@ -0,0 +1,27 @@ +require "rails_helper" + +RSpec.describe ApplicationMailer, type: :mailer do + let(:user) { create(:user) } + let(:email) { VerificationMailer.with(user_id: user.id).account_ownership_verification_email } + + describe "#set_perform_deliveries" do + it "changes perform_deliveries from true to false if smtp is not enabled" do + expect { email.deliver_now }.to change(described_class, :perform_deliveries).from(true).to(false) + end + + it "changes perform_deliveries from false to true if smtp is enabled" do + described_class.perform_deliveries = false + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) + + expect { email.deliver_now }.to change(described_class, :perform_deliveries).from(false).to(true) + end + end + + describe "#set_delivery_options" do + it "evoked Settings::SMTP.settings during callback" do + allow(Settings::SMTP).to receive(:settings) + email.deliver_now + expect(Settings::SMTP).to have_received(:settings) + end + end +end diff --git a/spec/mailers/notify_mailer_spec.rb b/spec/mailers/notify_mailer_spec.rb index 5ca4f4545..5146d8723 100644 --- a/spec/mailers/notify_mailer_spec.rb +++ b/spec/mailers/notify_mailer_spec.rb @@ -260,6 +260,8 @@ RSpec.describe NotifyMailer, type: :mailer do end it "tracks the feedback message ID after delivery" do + user # Instantiate user to pre-generate welcome email + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) assert_emails 1 do email.deliver_now end @@ -268,6 +270,8 @@ RSpec.describe NotifyMailer, type: :mailer do end it "tracks the email_type as the UTM campaign" do + user # Instantiate user to pre-generate welcome email + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) assert_emails 1 do email.deliver_now end diff --git a/spec/models/follow_spec.rb b/spec/models/follow_spec.rb index cbb76b594..04e0fbbfa 100644 --- a/spec/models/follow_spec.rb +++ b/spec/models/follow_spec.rb @@ -81,6 +81,7 @@ RSpec.describe Follow, type: :model do end it "sends an email notification" do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) user_2.update_column(:email_follower_notifications, true) expect do Sidekiq::Testing.inline! do diff --git a/spec/models/forem_instance_spec.rb b/spec/models/forem_instance_spec.rb index be77406ef..95f1e80f9 100644 --- a/spec/models/forem_instance_spec.rb +++ b/spec/models/forem_instance_spec.rb @@ -76,4 +76,23 @@ RSpec.describe ForemInstance, type: :model do expect(described_class.dev_to?).to be(false) end end + + describe ".smtp_enabled?" do + it "return false when no credential is provided" do + expect(described_class.smtp_enabled?).to be(false) + end + + it "returns true if user_name and password are present" do + allow(Settings::SMTP).to receive(:user_name).and_return("something") + allow(Settings::SMTP).to receive(:password).and_return("something") + + expect(described_class.smtp_enabled?).to be(true) + end + + it "returns true if sendgrid api key is available" do + ENV["SENDGRID_API_KEY"] = "something" + expect(described_class.smtp_enabled?).to be(true) + ENV["SENDGRID_API_KEY"] = nil + end + end end diff --git a/spec/models/message_spec.rb b/spec/models/message_spec.rb index 119e77613..04b54bb7a 100644 --- a/spec/models/message_spec.rb +++ b/spec/models/message_spec.rb @@ -122,6 +122,7 @@ RSpec.describe Message, type: :model do context "when callbacks are triggered after create" do before do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) chat_channel.add_users([user, user2]) end diff --git a/spec/models/settings/smtp_spec.rb b/spec/models/settings/smtp_spec.rb new file mode 100644 index 000000000..2829c0b0b --- /dev/null +++ b/spec/models/settings/smtp_spec.rb @@ -0,0 +1,40 @@ +require "rails_helper" + +RSpec.describe Settings::SMTP do + describe "::settings" do + it "use default sendgrid config if SENDGRID_API_KEY is available" do + key = "something" + domain = "test.com" + allow(ApplicationConfig).to receive(:[]).with("SENDGRID_API_KEY").and_return(key) + ENV["SENDGRID_API_KEY"] = "something" + allow(Settings::General).to receive(:app_domain).and_return(domain) + + expect(described_class.settings).to eq({ + address: "smtp.sendgrid.net", + port: 587, + authentication: :plain, + user_name: "apikey", + password: key, + domain: domain + }) + ENV["SENDGRID_API_KEY"] = nil + end + + it "uses Settings::SMTP config if SENDGRID_API_KEY is not available" do + described_class.address = "smtp.google.com" + described_class.port = 25 + described_class.authentication = "plain" + described_class.user_name = "username" + described_class.password = "password" + + expect(described_class.settings).to eq({ + address: "smtp.google.com", + port: 25, + authentication: "plain", + user_name: "username", + password: "password", + domain: "" + }) + end + end +end diff --git a/spec/requests/admin/chat_channel_spec.rb b/spec/requests/admin/chat_channel_spec.rb index 2c5c3d96c..15c7439e3 100644 --- a/spec/requests/admin/chat_channel_spec.rb +++ b/spec/requests/admin/chat_channel_spec.rb @@ -8,6 +8,7 @@ RSpec.describe "/admin/apps/chat_channels", type: :request do around { |example| perform_enqueued_jobs(&example) } it "creates chat_channel for with users as moderator" do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) user.add_role(:super_admin) sign_in user expect do diff --git a/spec/requests/admin/feedback_messages_spec.rb b/spec/requests/admin/feedback_messages_spec.rb index 94d1da1da..941230076 100644 --- a/spec/requests/admin/feedback_messages_spec.rb +++ b/spec/requests/admin/feedback_messages_spec.rb @@ -86,6 +86,7 @@ RSpec.describe "/admin/moderation/reports", type: :request do end it "creates a new email message with the same params" do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) post send_email_admin_reports_path, params: send_email_params expect(EmailMessage.last.attributes).to include(email_message_attributes) diff --git a/spec/requests/admin/users_spec.rb b/spec/requests/admin/users_spec.rb index 3088d517c..8ce2e88e2 100644 --- a/spec/requests/admin/users_spec.rb +++ b/spec/requests/admin/users_spec.rb @@ -121,6 +121,7 @@ RSpec.describe "/admin/users", type: :request do describe "POST /admin/users/:id/verify_email_ownership" do it "allows a user to verify email ownership" do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) post verify_email_ownership_admin_user_path(user.id), params: { user_id: user.id } path = verify_email_authorizations_path( diff --git a/spec/requests/feedback_messages_spec.rb b/spec/requests/feedback_messages_spec.rb index 12fa8504e..d3418b1ab 100644 --- a/spec/requests/feedback_messages_spec.rb +++ b/spec/requests/feedback_messages_spec.rb @@ -3,6 +3,10 @@ require "rails_helper" RSpec.describe "feedback_messages", type: :request do let(:user) { create(:user) } + before do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) + end + describe "POST /feedback_messages" do def mock_recaptcha_verification # rubocop:disable RSpec/AnyInstance diff --git a/spec/requests/user/user_settings_spec.rb b/spec/requests/user/user_settings_spec.rb index 6dc442999..c4f8f197c 100644 --- a/spec/requests/user/user_settings_spec.rb +++ b/spec/requests/user/user_settings_spec.rb @@ -321,6 +321,7 @@ RSpec.describe "UserSettings", type: :request do end it "sends an email" do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) expect do sidekiq_perform_enqueued_jobs do send_request diff --git a/spec/services/exporter/service_spec.rb b/spec/services/exporter/service_spec.rb index 7a08e5f46..92598f488 100644 --- a/spec/services/exporter/service_spec.rb +++ b/spec/services/exporter/service_spec.rb @@ -8,7 +8,11 @@ RSpec.describe Exporter::Service, type: :service do let(:other_user_article) { create(:article, user: other_user) } before do - ActionMailer::Base.deliveries.clear + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) + end + + after do + ApplicationMailer.deliveries.clear end def valid_instance(user) @@ -71,6 +75,7 @@ RSpec.describe Exporter::Service, type: :service do context "when emailing the user" do it "delivers one email" do service = valid_instance(article.user) + ApplicationMailer.deliveries.clear service.export(article.user.email) expect(ActionMailer::Base.deliveries.count).to eq(1) end diff --git a/spec/system/admin/user_email_tools_spec.rb b/spec/system/admin/user_email_tools_spec.rb index 82cafceb8..2188419fc 100644 --- a/spec/system/admin/user_email_tools_spec.rb +++ b/spec/system/admin/user_email_tools_spec.rb @@ -8,6 +8,7 @@ RSpec.describe "Admin Email Tools within User management", type: :system do context "when looking at a user with recorded emails in the admin panel" do before do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) sign_in admin visit admin_user_path(user) diff --git a/spec/workers/organizations/delete_worker_spec.rb b/spec/workers/organizations/delete_worker_spec.rb index 7ea2ab555..f33dea905 100644 --- a/spec/workers/organizations/delete_worker_spec.rb +++ b/spec/workers/organizations/delete_worker_spec.rb @@ -38,6 +38,7 @@ RSpec.describe Organizations::DeleteWorker, type: :worker do end it "sends the notification" do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) expect do worker.perform(org.id, user.id) end.to change(ActionMailer::Base.deliveries, :count).by(1) diff --git a/spec/workers/users/delete_worker_spec.rb b/spec/workers/users/delete_worker_spec.rb index 1bb1e744a..ddf772ce2 100644 --- a/spec/workers/users/delete_worker_spec.rb +++ b/spec/workers/users/delete_worker_spec.rb @@ -6,6 +6,10 @@ RSpec.describe Users::DeleteWorker, type: :worker do let(:mailer) { double } let(:message_delivery) { double } + before do + allow(ForemInstance).to receive(:smtp_enabled?).and_return(true) + end + describe "#perform" do let!(:user) { create(:user) } let(:delete) { Users::Delete }