From 4d364ec3bcd44c25afbf7f47808bd9acaeb0006f Mon Sep 17 00:00:00 2001 From: Jonathan Khoo Date: Sun, 23 Feb 2025 16:50:30 +1300 Subject: [PATCH 1/7] Remove some outdated references to nztrain.com --- app/views/user/admin_email.html.erb | 2 +- config/application.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/user/admin_email.html.erb b/app/views/user/admin_email.html.erb index 4a4cf38a..547d1bc5 100644 --- a/app/views/user/admin_email.html.erb +++ b/app/views/user/admin_email.html.erb @@ -13,7 +13,7 @@

<%= current_user.name %>
- Admin of nztrain.com + Admin of NZOI Training

diff --git a/config/application.rb b/config/application.rb index 803fd399..e0126e1f 100644 --- a/config/application.rb +++ b/config/application.rb @@ -52,7 +52,7 @@ class Application < Rails::Application :address => "smtp.gmail.com", :port => 587, :authentication => :plain, - :domain => 'nztrain.com', + :domain => 'nzoi.org.nz', :user_name => 'nztrain@gmail.com', # username and password set with higher priority in settings table :password => 'training site', :enable_starttls_auto => true From 2b4cfd6088ee95c45a4f893e79fb1ce92981367c Mon Sep 17 00:00:00 2001 From: Tom Levy Date: Mon, 3 Mar 2025 00:16:52 +1300 Subject: [PATCH 2/7] Remove hard-coded default SMTP user name and password Passwords should never be hard-coded in the source code for security. The default user name was somewhat useful for the official instance, but it's cleaner to avoid it. (Note: Now that the default value is blank, it's not clear that the setting "system/mailer/username" is supposed to be an email address. This will be addressed in a later commit that renames that setting to "system/mailer/email".) --- config/application.rb | 6 +++--- config/initializers/mailer.rb | 6 +++--- db/migrate/20120805061859_system_mailer_settings.rb | 4 ++-- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/config/application.rb b/config/application.rb index e0126e1f..b4009fea 100644 --- a/config/application.rb +++ b/config/application.rb @@ -2,7 +2,7 @@ require 'rubygems' require 'rails/all' -if defined?(Bundler) +if defined?(Bundler) # Require the gems listed in Gemfile, including any gems # you've limited to :test, :development, or :production. Bundler.require(:default, Rails.env) @@ -53,8 +53,8 @@ class Application < Rails::Application :port => 587, :authentication => :plain, :domain => 'nzoi.org.nz', - :user_name => 'nztrain@gmail.com', # username and password set with higher priority in settings table - :password => 'training site', + #:user_name => ..., # set in config/initializers/mailer.rb + #:password => ..., # set in config/initializers/mailer.rb :enable_starttls_auto => true } diff --git a/config/initializers/mailer.rb b/config/initializers/mailer.rb index 21fee7cc..b0541ac0 100644 --- a/config/initializers/mailer.rb +++ b/config/initializers/mailer.rb @@ -1,7 +1,7 @@ -# use db stored username and password for email if present +# use db stored username and password for email if ActiveRecord::Base.connection.table_exists?(Setting.table_name) - ActionMailer::Base.smtp_settings[:user_name] = Setting.find_by_key("system/mailer/username").value if Setting.find_by_key("system/mailer/username") - ActionMailer::Base.smtp_settings[:password] = Setting.find_by_key("system/mailer/password").value if Setting.find_by_key("system/mailer/password") + ActionMailer::Base.smtp_settings[:user_name] = Setting.find_by_key("system/mailer/username")&.value + ActionMailer::Base.smtp_settings[:password] = Setting.find_by_key("system/mailer/password")&.value end diff --git a/db/migrate/20120805061859_system_mailer_settings.rb b/db/migrate/20120805061859_system_mailer_settings.rb index 71bb3bed..96c0977d 100644 --- a/db/migrate/20120805061859_system_mailer_settings.rb +++ b/db/migrate/20120805061859_system_mailer_settings.rb @@ -1,7 +1,7 @@ class SystemMailerSettings < ActiveRecord::Migration def up - Setting.find_or_create_by!(key: "system/mailer/username", value: "nztrain@gmail.com") - Setting.find_or_create_by!(key: "system/mailer/password", value: "training site") + Setting.find_or_create_by!(key: "system/mailer/username") + Setting.find_or_create_by!(key: "system/mailer/password") end def down From f83eae2cadb0d4ce2f7bd25352167e73429a8d90 Mon Sep 17 00:00:00 2001 From: Tom Levy Date: Thu, 6 Mar 2025 22:39:24 +1300 Subject: [PATCH 3/7] Set default "from" email address globally Refactor the code to set the sender email address in one place instead of multiple places. This is in preparation for making the "from" address configurable. Note that `config.action_mailer.default()` doesn't work[1], so we must use `default_options=`. Also drop `:charset => "utf-8"`, because the default is already UTF-8 (and that line wasn't even doing anything due to [1]). [1] https://www.github.com/rails/rails/issues/54700 --- app/mailers/admin_mailer.rb | 2 -- config/application.rb | 2 +- config/initializers/devise.rb | 2 +- lib/development_mail_interceptor.rb | 2 +- 4 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/mailers/admin_mailer.rb b/app/mailers/admin_mailer.rb index 83911456..f503629f 100644 --- a/app/mailers/admin_mailer.rb +++ b/app/mailers/admin_mailer.rb @@ -1,6 +1,4 @@ class AdminMailer < ActionMailer::Base - default :from => "nztrain@gmail.com" - def custom_email(admin,user,subject,msgbody) @user = user @msgbody = msgbody diff --git a/config/application.rb b/config/application.rb index b4009fea..b1047a65 100644 --- a/config/application.rb +++ b/config/application.rb @@ -46,7 +46,7 @@ class Application < Rails::Application config.action_mailer.delivery_method = :smtp config.action_mailer.perform_deliveries = true config.action_mailer.raise_delivery_errors = true - config.action_mailer.default :charset => "utf-8" + config.action_mailer.default_options = { from: "nztrain@gmail.com" } ActionMailer::Base.smtp_settings = { :address => "smtp.gmail.com", diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 0b9c8d07..f2900a09 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -10,7 +10,7 @@ # Configure the e-mail address which will be shown in Devise::Mailer, # note that it will be overwritten if you use your own mailer class # with default "from" parameter. - config.mailer_sender = "nztrain@gmail.com" + #config.mailer_sender = ... # Configure the class responsible to send e-mails. # config.mailer = "Devise::Mailer" diff --git a/lib/development_mail_interceptor.rb b/lib/development_mail_interceptor.rb index 838a87c2..f5bf728b 100644 --- a/lib/development_mail_interceptor.rb +++ b/lib/development_mail_interceptor.rb @@ -1,7 +1,7 @@ class DevelopmentMailInterceptor def self.delivering_email(message) message.subject = "[Development] #{message.to} #{message.subject}" - message.to = "nztrain@gmail.com" + message.to = message.from end end From 4034cd7cded8b4d1f3d5707d3db74003c2699afc Mon Sep 17 00:00:00 2001 From: Tom Levy Date: Mon, 3 Mar 2025 00:35:15 +1300 Subject: [PATCH 4/7] Set "from" email address using settings table instead of hard-coding Previously the "from" address was hard-coded to nztrain@gmail.com and could not be configured. This commit uses the value of the "system/mailer/username" setting, which is the user name for SMTP authentication. It will be the same as the "from" email when using Gmail and in most other setups. (A later commit will rename that setting to "system/mailer/email".) * We provide a default email in the test environment, otherwise the specs fail. * We don't provide a default email in the production environment, because the admin must configure email properly in order for mail delivery to work. * We don't provide a default email in the development environment, because mail delivery will fail anyway (unless SMTP is configured properly). The admin will still be able to read the emails by looking in the development log. In the future we may wish to add a setting for overriding the "Reply-To" address; it could be useful for redirecting responses to an mailbox that's actually monitored (https://www.github.com/NZOI/nztrain/pull/272#issuecomment-2704959896). But for now we don't bother. --- config/application.rb | 1 - config/environments/test.rb | 2 ++ config/initializers/mailer.rb | 3 ++- 3 files changed, 4 insertions(+), 2 deletions(-) diff --git a/config/application.rb b/config/application.rb index b1047a65..3b2f5501 100644 --- a/config/application.rb +++ b/config/application.rb @@ -46,7 +46,6 @@ class Application < Rails::Application config.action_mailer.delivery_method = :smtp config.action_mailer.perform_deliveries = true config.action_mailer.raise_delivery_errors = true - config.action_mailer.default_options = { from: "nztrain@gmail.com" } ActionMailer::Base.smtp_settings = { :address => "smtp.gmail.com", diff --git a/config/environments/test.rb b/config/environments/test.rb index 05fc3f0d..22f41cfe 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -25,6 +25,8 @@ # ActionMailer::Base.deliveries array. config.action_mailer.delivery_method = :test + config.action_mailer.default_options = { from: "train@example.com" } + # Use SQL instead of Active Record's schema dumper when creating the test database. # This is necessary if your schema can't be completely dumped by the schema dumper, # like if you have constraints or database-specific column types diff --git a/config/initializers/mailer.rb b/config/initializers/mailer.rb index b0541ac0..c6be9eec 100644 --- a/config/initializers/mailer.rb +++ b/config/initializers/mailer.rb @@ -1,7 +1,8 @@ -# use db stored username and password for email +# use db-stored email address and password for sending email if ActiveRecord::Base.connection.table_exists?(Setting.table_name) ActionMailer::Base.smtp_settings[:user_name] = Setting.find_by_key("system/mailer/username")&.value ActionMailer::Base.smtp_settings[:password] = Setting.find_by_key("system/mailer/password")&.value + ActionMailer::Base.default from: Setting.find_by_key("system/mailer/username").value if Setting.find_by_key("system/mailer/username")&.value.present? end From 395b1f3c96145292a6863b0645502c07a6bda98a Mon Sep 17 00:00:00 2001 From: Tom Levy Date: Tue, 18 Mar 2025 19:21:48 +1300 Subject: [PATCH 5/7] Rename setting system/mailer/username to system/mailer/email It is now used for both SMTP authentication and for the "from" email address, so the term "email" is clearer and more accurate. --- config/initializers/mailer.rb | 4 ++-- ...0318053450_rename_setting_mailer_username_to_email.rb | 9 +++++++++ db/schema.rb | 2 +- 3 files changed, 12 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb diff --git a/config/initializers/mailer.rb b/config/initializers/mailer.rb index c6be9eec..05e325d6 100644 --- a/config/initializers/mailer.rb +++ b/config/initializers/mailer.rb @@ -1,8 +1,8 @@ # use db-stored email address and password for sending email if ActiveRecord::Base.connection.table_exists?(Setting.table_name) - ActionMailer::Base.smtp_settings[:user_name] = Setting.find_by_key("system/mailer/username")&.value + ActionMailer::Base.smtp_settings[:user_name] = Setting.find_by_key("system/mailer/email")&.value ActionMailer::Base.smtp_settings[:password] = Setting.find_by_key("system/mailer/password")&.value - ActionMailer::Base.default from: Setting.find_by_key("system/mailer/username").value if Setting.find_by_key("system/mailer/username")&.value.present? + ActionMailer::Base.default from: Setting.find_by_key("system/mailer/email").value if Setting.find_by_key("system/mailer/email")&.value.present? end diff --git a/db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb b/db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb new file mode 100644 index 00000000..55b2931a --- /dev/null +++ b/db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb @@ -0,0 +1,9 @@ +class RenameSettingMailerUsernameToEmail < ActiveRecord::Migration + def up + execute "UPDATE settings SET key = 'system/mailer/email' WHERE key = 'system/mailer/username'" + end + + def down + execute "UPDATE settings SET key = 'system/mailer/username' WHERE key = 'system/mailer/email'" + end +end diff --git a/db/schema.rb b/db/schema.rb index 0c980263..43d4326b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20230225054132) do +ActiveRecord::Schema.define(version: 20250318053450) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" From 02227f26df9f2714634e3a51c7d6814dd1ab558f Mon Sep 17 00:00:00 2001 From: Tom Levy Date: Tue, 18 Mar 2025 19:26:49 +1300 Subject: [PATCH 6/7] Create settings system/mailer/* using seeds instead of migration It's better to use seeds for adding initial data to the database. --- db/migrate/20120805061859_system_mailer_settings.rb | 9 --------- ...0318053450_rename_setting_mailer_username_to_email.rb | 2 ++ db/seeds.rb | 2 +- 3 files changed, 3 insertions(+), 10 deletions(-) delete mode 100644 db/migrate/20120805061859_system_mailer_settings.rb diff --git a/db/migrate/20120805061859_system_mailer_settings.rb b/db/migrate/20120805061859_system_mailer_settings.rb deleted file mode 100644 index 96c0977d..00000000 --- a/db/migrate/20120805061859_system_mailer_settings.rb +++ /dev/null @@ -1,9 +0,0 @@ -class SystemMailerSettings < ActiveRecord::Migration - def up - Setting.find_or_create_by!(key: "system/mailer/username") - Setting.find_or_create_by!(key: "system/mailer/password") - end - - def down - end -end diff --git a/db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb b/db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb index 55b2931a..7097256c 100644 --- a/db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb +++ b/db/migrate/20250318053450_rename_setting_mailer_username_to_email.rb @@ -1,4 +1,6 @@ class RenameSettingMailerUsernameToEmail < ActiveRecord::Migration + # this migration can be deleted after deploying + def up execute "UPDATE settings SET key = 'system/mailer/email' WHERE key = 'system/mailer/username'" end diff --git a/db/seeds.rb b/db/seeds.rb index a8524876..f8a05341 100644 --- a/db/seeds.rb +++ b/db/seeds.rb @@ -34,7 +34,7 @@ superadmin.users.push(rootuser) unless superadmin.users.include?(rootuser); end -["recaptcha/public_key","recaptcha/private_key"].each do |setting| +["system/mailer/email","system/mailer/password","recaptcha/public_key","recaptcha/private_key"].each do |setting| Setting.find_or_create_by(key: setting) end From d3d216cd11191062a11104f70b1900bf60aa79a2 Mon Sep 17 00:00:00 2001 From: Tom Levy Date: Tue, 18 Mar 2025 19:45:22 +1300 Subject: [PATCH 7/7] Set email sender's display name to "NZOI Training" Ideally it should be configurable, but we hard-code it for now like in other places for simplicity. Suggested-by: Jonathan Khoo --- config/initializers/mailer.rb | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/config/initializers/mailer.rb b/config/initializers/mailer.rb index 05e325d6..071c9f3d 100644 --- a/config/initializers/mailer.rb +++ b/config/initializers/mailer.rb @@ -1,8 +1,17 @@ # use db-stored email address and password for sending email +def email_address_with_name(address, name) + # can switch to ActionMailer::Base.email_address_with_name() once we upgrade to Rails 6 + Mail::Address.new.tap do |builder| + builder.address = address + builder.display_name = name.presence + end.to_s +end + if ActiveRecord::Base.connection.table_exists?(Setting.table_name) - ActionMailer::Base.smtp_settings[:user_name] = Setting.find_by_key("system/mailer/email")&.value + email_setting = Setting.find_by_key("system/mailer/email") + ActionMailer::Base.smtp_settings[:user_name] = email_setting&.value ActionMailer::Base.smtp_settings[:password] = Setting.find_by_key("system/mailer/password")&.value - ActionMailer::Base.default from: Setting.find_by_key("system/mailer/email").value if Setting.find_by_key("system/mailer/email")&.value.present? + ActionMailer::Base.default from: email_address_with_name(email_setting.value, "NZOI Training") if email_setting&.value.present? end