From 00608a19c68c1e7a81fa56a6da6a53576c476472 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 16 Jul 2024 09:14:17 +1000 Subject: [PATCH] FIX: Show the SMTP authentication error for group UI (#27914) Originally in 964da218173db007fefe6357e96292f5545c513e we hid the SMTPAuthenticationError message except in very specific cases. However this message often contains helpful information from the mail provider, for example here is a response from Office365: > 535 5.7.139 Authentication unsuccessful, user is locked by your organization's security defaults policy. Contact your administrator. So, we will show the error message in the modal UI instead of supressing it with a generic message to be more helpful. --- .../email_settings_exception_handler.rb | 41 ++++++------------- config/locales/server.en.yml | 3 +- spec/requests/groups_controller_spec.rb | 2 +- .../email_settings_exception_handler_spec.rb | 20 +-------- .../group_email_credentials_spec.rb | 18 +++++++- 5 files changed, 32 insertions(+), 52 deletions(-) diff --git a/app/services/email_settings_exception_handler.rb b/app/services/email_settings_exception_handler.rb index b85a9aaa95b..24f7f8fbc5e 100644 --- a/app/services/email_settings_exception_handler.rb +++ b/app/services/email_settings_exception_handler.rb @@ -75,7 +75,18 @@ class EmailSettingsExceptionHandler end def net_smtp_authentication_error - I18n.t("email_settings.smtp_authentication_error") + # Generally SMTP authentication errors are due to invalid credentials, + # and most common mail servers provide more detailed error messages, + # so it should be safe to return the error message as is. + # + # Example: Office 365 returns: + # + # 535 5.7.139 Authentication unsuccessful, user is locked by your organization's security defaults policy. Contact your administrator. + # + # Example: Gmail returns: + # + # Application-specific password required. Learn more at https://support.google.com/accounts/answer/185833 + I18n.t("email_settings.smtp_authentication_error", message: @exception.message) end def net_smtp_server_busy @@ -99,33 +110,7 @@ class EmailSettingsExceptionHandler end end - class GmailProvider < GenericProvider - def net_smtp_authentication_error - # Gmail requires use of application-specific passwords when 2FA is enabled and return - # a special error message calling this out. - if @exception.message.match(/Application-specific password required/) - I18n.t("email_settings.authentication_error_gmail_app_password") - else - super - end - end - - def net_imap_no_response_error - # Gmail requires use of application-specific passwords when 2FA is enabled and return - # a special error message calling this out. - if @exception.message.match(/Application-specific password required/) - I18n.t("email_settings.authentication_error_gmail_app_password") - else - super - end - end - end - def self.friendly_exception_message(exception, host) - if host.include?("gmail.com") - EmailSettingsExceptionHandler::GmailProvider.new(exception).message - else - EmailSettingsExceptionHandler::GenericProvider.new(exception).message - end + EmailSettingsExceptionHandler::GenericProvider.new(exception).message end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 5de98d755b9..5b11bcaa28b 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1123,8 +1123,7 @@ en: pop3_authentication_error: "There was an issue with the POP3 credentials provided, check the username and password and try again." imap_authentication_error: "There was an issue with the IMAP credentials provided, check the username and password and try again." imap_no_response_error: "An error occurred when communicating with the IMAP server. %{message}" - smtp_authentication_error: "There was an issue with the SMTP credentials provided, check the username and password and try again." - authentication_error_gmail_app_password: 'Application-specific password required. Learn more at this Google Help article' + smtp_authentication_error: "There was an issue with the SMTP credentials provided, check the username and password and try again. %{message}" smtp_server_busy_error: "The SMTP server is currently busy, try again later." smtp_unhandled_error: "There was an unhandled error when communicating with the SMTP server. %{message}" imap_unhandled_error: "There was an unhandled error when communicating with the IMAP server. %{message}" diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index e25aa3a96e5..6d042a7f502 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -2799,7 +2799,7 @@ RSpec.describe GroupsController do post "/groups/#{group.id}/test_email_settings.json", params: params expect(response.status).to eq(422) expect(response.parsed_body["errors"]).to include( - I18n.t("email_settings.smtp_authentication_error"), + I18n.t("email_settings.smtp_authentication_error", message: "Invalid credentials"), ) end end diff --git a/spec/services/email_settings_exception_handler_spec.rb b/spec/services/email_settings_exception_handler_spec.rb index 1baa084abbc..1fe25376bbc 100644 --- a/spec/services/email_settings_exception_handler_spec.rb +++ b/spec/services/email_settings_exception_handler_spec.rb @@ -23,28 +23,10 @@ RSpec.describe EmailSettingsExceptionHandler do ) end - it "formats a general Net::IMAP::NoResponseError with application-specific password Gmail error" do - exception = - Net::IMAP::NoResponseError.new( - stub(data: stub(text: "NO Application-specific password required")), - ) - expect(described_class.friendly_exception_message(exception, "imap.gmail.com")).to eq( - I18n.t("email_settings.authentication_error_gmail_app_password"), - ) - end - it "formats a Net::SMTPAuthenticationError" do exception = Net::SMTPAuthenticationError.new("invalid credentials") expect(described_class.friendly_exception_message(exception, "smtp.test.com")).to eq( - I18n.t("email_settings.smtp_authentication_error"), - ) - end - - it "formats a Net::SMTPAuthenticationError with application-specific password Gmail error" do - exception = - Net::SMTPAuthenticationError.new(nil, message: "Application-specific password required") - expect(described_class.friendly_exception_message(exception, "smtp.gmail.com")).to eq( - I18n.t("email_settings.authentication_error_gmail_app_password"), + I18n.t("email_settings.smtp_authentication_error", message: "invalid credentials"), ) end diff --git a/spec/services/problem_check/group_email_credentials_spec.rb b/spec/services/problem_check/group_email_credentials_spec.rb index 2c2ea73a70f..c8551e4dfb7 100644 --- a/spec/services/problem_check/group_email_credentials_spec.rb +++ b/spec/services/problem_check/group_email_credentials_spec.rb @@ -45,7 +45,14 @@ RSpec.describe ProblemCheck::GroupEmailCredentials do target: group2.id, priority: "high", message: - "There was an issue with the email credentials for the group . No emails will be sent from the group inbox until this problem is addressed. There was an issue with the SMTP credentials provided, check the username and password and try again.", + I18n.t( + "dashboard.problem.group_email_credentials", + base_path: Discourse.base_path, + group_name: group2.name, + group_full_name: group2.full_name, + error: + I18n.t("email_settings.smtp_authentication_error", message: "bad credentials"), + ), ), ) end @@ -63,7 +70,14 @@ RSpec.describe ProblemCheck::GroupEmailCredentials do target: group3.id, priority: "high", message: - "There was an issue with the email credentials for the group . No emails will be sent from the group inbox until this problem is addressed. There was an issue with the IMAP credentials provided, check the username and password and try again.", + I18n.t( + "dashboard.problem.group_email_credentials", + base_path: Discourse.base_path, + group_name: group3.name, + group_full_name: group3.full_name, + error: + I18n.t("email_settings.imap_authentication_error", message: "bad credentials"), + ), ), ) end