From d6f2a531aab1976ac38890ed03b51506da5c7500 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Apr 2024 15:34:24 +1000 Subject: [PATCH 1/5] Don't pass invalid auth method "None" to net-smtp It's our magic word to not pass username and password on. --- lib/spree/core/mail_settings.rb | 8 +++++++- spec/lib/spree/core/mail_settings_spec.rb | 2 +- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/lib/spree/core/mail_settings.rb b/lib/spree/core/mail_settings.rb index 2c8261b246..07d8dbee7a 100644 --- a/lib/spree/core/mail_settings.rb +++ b/lib/spree/core/mail_settings.rb @@ -38,7 +38,13 @@ module Spree { address: Config.mail_host, domain: Config.mail_domain, port: Config.mail_port, - authentication: Config.mail_auth_type } + authentication: } + end + + def authentication + return unless need_authentication? + + Config.mail_auth_type.presence end def need_authentication? diff --git a/spec/lib/spree/core/mail_settings_spec.rb b/spec/lib/spree/core/mail_settings_spec.rb index 4039addaf2..d1a2fc99b4 100644 --- a/spec/lib/spree/core/mail_settings_spec.rb +++ b/spec/lib/spree/core/mail_settings_spec.rb @@ -23,7 +23,7 @@ module Spree it { expect(ActionMailer::Base.smtp_settings[:address]).to eq "smtp.example.com" } it { expect(ActionMailer::Base.smtp_settings[:domain]).to eq "example.com" } it { expect(ActionMailer::Base.smtp_settings[:port]).to eq 123 } - it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq "None" } + it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq nil } it { expect(ActionMailer::Base.smtp_settings[:enable_starttls_auto]).to be_truthy } it "doesnt touch user name config" do From a41019fbffcf02484fc522f38eb761d4e60fa41d Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Apr 2024 15:41:15 +1000 Subject: [PATCH 2/5] Simplify SMTP auth method selection Instead of using the auth method name, let's just not supply username and password when we don't want to authenticate. The two affected servers AU and CA don't have credentials set anyway. This is compatible. The specs needed changing though. --- lib/spree/core/mail_settings.rb | 21 +++++-------- spec/lib/spree/core/mail_settings_spec.rb | 37 +++++++++-------------- 2 files changed, 21 insertions(+), 37 deletions(-) diff --git a/lib/spree/core/mail_settings.rb b/lib/spree/core/mail_settings.rb index 07d8dbee7a..0373f7fac7 100644 --- a/lib/spree/core/mail_settings.rb +++ b/lib/spree/core/mail_settings.rb @@ -20,18 +20,14 @@ module Spree private def mail_server_settings - settings = if need_authentication? - basic_settings.merge(user_credentials) - else - basic_settings - end + settings = basic_settings.merge(user_credentials) settings.merge(enable_starttls_auto: secure_connection?) end def user_credentials - { user_name: Config.smtp_username, - password: Config.smtp_password } + { user_name: Config.smtp_username.presence, + password: Config.smtp_password.presence } end def basic_settings @@ -42,13 +38,10 @@ module Spree end def authentication - return unless need_authentication? - - Config.mail_auth_type.presence - end - - def need_authentication? - Config.mail_auth_type != 'None' + # "None" is an option in the UI but not a real authentication type. + # We should remove it from our host configurations but I'm keeping + # this check for backwards-compatibility for now. + Config.mail_auth_type.presence unless Config.mail_auth_type == "None" end def secure_connection? diff --git a/spec/lib/spree/core/mail_settings_spec.rb b/spec/lib/spree/core/mail_settings_spec.rb index d1a2fc99b4..6a40495695 100644 --- a/spec/lib/spree/core/mail_settings_spec.rb +++ b/spec/lib/spree/core/mail_settings_spec.rb @@ -8,12 +8,12 @@ module Spree let!(:subject) { MailSettings.new } context "overrides appplication defaults" do - context "authentication method is none" do + context "authentication method is login" do before do Config.mail_host = "smtp.example.com" Config.mail_domain = "example.com" Config.mail_port = 123 - Config.mail_auth_type = MailSettings::SECURE_CONNECTION_TYPES[0] + Config.mail_auth_type = "login" Config.smtp_username = "schof" Config.smtp_password = "hellospree!" Config.secure_connection_type = "TLS" @@ -23,31 +23,22 @@ module Spree it { expect(ActionMailer::Base.smtp_settings[:address]).to eq "smtp.example.com" } it { expect(ActionMailer::Base.smtp_settings[:domain]).to eq "example.com" } it { expect(ActionMailer::Base.smtp_settings[:port]).to eq 123 } - it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq nil } + it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq "login" } it { expect(ActionMailer::Base.smtp_settings[:enable_starttls_auto]).to be_truthy } - - it "doesnt touch user name config" do - expect(ActionMailer::Base.smtp_settings[:user_name]).to be_nil - end - - it "doesnt touch password config" do - expect(ActionMailer::Base.smtp_settings[:password]).to be_nil - end - end - end - - context "when mail_auth_type is other than none" do - before do - Config.mail_auth_type = "login" - Config.smtp_username = "schof" - Config.smtp_password = "hellospree!" - subject.override! - end - - context "overrides user credentials" do it { expect(ActionMailer::Base.smtp_settings[:user_name]).to eq "schof" } it { expect(ActionMailer::Base.smtp_settings[:password]).to eq "hellospree!" } end + + context "authentication method is none" do + before do + Config.mail_auth_type = "None" + subject.override! + end + + it "doesn't store 'None' as auth method" do + expect(ActionMailer::Base.smtp_settings[:authentication]).to eq nil + end + end end end end From 7e2fcede61e8c8cfbc0f6e81c322efad834a6184 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Apr 2024 15:43:59 +1000 Subject: [PATCH 3/5] Further simplify mail options logic We were always adding this option anyway, so why not declare it to start with? --- lib/spree/core/mail_settings.rb | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/lib/spree/core/mail_settings.rb b/lib/spree/core/mail_settings.rb index 0373f7fac7..ebd62fc4c0 100644 --- a/lib/spree/core/mail_settings.rb +++ b/lib/spree/core/mail_settings.rb @@ -20,9 +20,7 @@ module Spree private def mail_server_settings - settings = basic_settings.merge(user_credentials) - - settings.merge(enable_starttls_auto: secure_connection?) + basic_settings.merge(user_credentials) end def user_credentials @@ -31,10 +29,13 @@ module Spree end def basic_settings - { address: Config.mail_host, + { + address: Config.mail_host, domain: Config.mail_domain, port: Config.mail_port, - authentication: } + authentication:, + enable_starttls_auto: secure_connection?, + } end def authentication From 1fc4270613acb80a00554d1158ab703bab6eb4d4 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Apr 2024 15:46:06 +1000 Subject: [PATCH 4/5] Remove indirection from MailSettings We can simply merge the option hashes now because they are not conditional anymore. Well, the magic `presence` method does the conditional logic for us now. --- lib/spree/core/mail_settings.rb | 11 ++--------- 1 file changed, 2 insertions(+), 9 deletions(-) diff --git a/lib/spree/core/mail_settings.rb b/lib/spree/core/mail_settings.rb index ebd62fc4c0..9824283521 100644 --- a/lib/spree/core/mail_settings.rb +++ b/lib/spree/core/mail_settings.rb @@ -20,21 +20,14 @@ module Spree private def mail_server_settings - basic_settings.merge(user_credentials) - end - - def user_credentials - { user_name: Config.smtp_username.presence, - password: Config.smtp_password.presence } - end - - def basic_settings { address: Config.mail_host, domain: Config.mail_domain, port: Config.mail_port, authentication:, enable_starttls_auto: secure_connection?, + user_name: Config.smtp_username.presence, + password: Config.smtp_password.presence, } end From 5cd53e0b5ee027b9f19e9378893f66522c02036b Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Tue, 16 Apr 2024 16:44:12 +1000 Subject: [PATCH 5/5] Clean up MailSettings spec Best viewed with whitespace ignored. --- spec/lib/spree/core/mail_settings_spec.rb | 62 ++++++++++------------- 1 file changed, 28 insertions(+), 34 deletions(-) diff --git a/spec/lib/spree/core/mail_settings_spec.rb b/spec/lib/spree/core/mail_settings_spec.rb index 6a40495695..3953c3266f 100644 --- a/spec/lib/spree/core/mail_settings_spec.rb +++ b/spec/lib/spree/core/mail_settings_spec.rb @@ -2,43 +2,37 @@ require 'spec_helper' -module Spree - module Core - describe MailSettings do - let!(:subject) { MailSettings.new } +describe Spree::Core::MailSettings do + context "overrides appplication defaults" do + context "authentication method is login" do + before do + Spree::Config.mail_host = "smtp.example.com" + Spree::Config.mail_domain = "example.com" + Spree::Config.mail_port = 123 + Spree::Config.mail_auth_type = "login" + Spree::Config.smtp_username = "schof" + Spree::Config.smtp_password = "hellospree!" + Spree::Config.secure_connection_type = "TLS" + subject.override! + end - context "overrides appplication defaults" do - context "authentication method is login" do - before do - Config.mail_host = "smtp.example.com" - Config.mail_domain = "example.com" - Config.mail_port = 123 - Config.mail_auth_type = "login" - Config.smtp_username = "schof" - Config.smtp_password = "hellospree!" - Config.secure_connection_type = "TLS" - subject.override! - end + it { expect(ActionMailer::Base.smtp_settings[:address]).to eq "smtp.example.com" } + it { expect(ActionMailer::Base.smtp_settings[:domain]).to eq "example.com" } + it { expect(ActionMailer::Base.smtp_settings[:port]).to eq 123 } + it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq "login" } + it { expect(ActionMailer::Base.smtp_settings[:enable_starttls_auto]).to be_truthy } + it { expect(ActionMailer::Base.smtp_settings[:user_name]).to eq "schof" } + it { expect(ActionMailer::Base.smtp_settings[:password]).to eq "hellospree!" } + end - it { expect(ActionMailer::Base.smtp_settings[:address]).to eq "smtp.example.com" } - it { expect(ActionMailer::Base.smtp_settings[:domain]).to eq "example.com" } - it { expect(ActionMailer::Base.smtp_settings[:port]).to eq 123 } - it { expect(ActionMailer::Base.smtp_settings[:authentication]).to eq "login" } - it { expect(ActionMailer::Base.smtp_settings[:enable_starttls_auto]).to be_truthy } - it { expect(ActionMailer::Base.smtp_settings[:user_name]).to eq "schof" } - it { expect(ActionMailer::Base.smtp_settings[:password]).to eq "hellospree!" } - end + context "authentication method is none" do + before do + Spree::Config.mail_auth_type = "None" + subject.override! + end - context "authentication method is none" do - before do - Config.mail_auth_type = "None" - subject.override! - end - - it "doesn't store 'None' as auth method" do - expect(ActionMailer::Base.smtp_settings[:authentication]).to eq nil - end - end + it "doesn't store 'None' as auth method" do + expect(ActionMailer::Base.smtp_settings[:authentication]).to eq nil end end end