From 20fe5eceb885b35d469f3201dbdcf84afc88daf1 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 4 Jan 2022 10:14:33 +1000 Subject: [PATCH] FEATURE: Scheduled group email credential problem check (#15396) This commit adds a check that runs regularly as per 2d68e5d942fa9312655d3d5abacf15d8a9fca948 which tests the credentials of groups with SMTP or IMAP enabled. If any issues are found with those credentials a high priority problem is added to the admin dashboard. This commit also formats the admin dashboard differently if there are high priority problems, bringing them to the top of the list and highlighting them. The problem will be cleared if the issue is fixed before the next problem check, or if the group's settings are updated with a valid credential. --- .../addon/controllers/admin-dashboard.js | 22 ++++- .../components/dashboard-problems.hbs | 29 +++++-- .../admin/addon/templates/dashboard.hbs | 3 +- .../stylesheets/common/admin/dashboard.scss | 17 +++- app/controllers/groups_controller.rb | 1 + app/models/admin_dashboard_data.rb | 16 +++- app/models/group.rb | 1 + config/locales/client.en.yml | 1 + config/locales/server.en.yml | 1 + lib/group_email_credentials_check.rb | 60 +++++++++++++ .../lib/group_email_credentials_check_spec.rb | 86 +++++++++++++++++++ 11 files changed, 223 insertions(+), 14 deletions(-) create mode 100644 lib/group_email_credentials_check.rb create mode 100644 spec/lib/group_email_credentials_check_spec.rb diff --git a/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js b/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js index c7291f1a763..004df68daa7 100644 --- a/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js +++ b/app/assets/javascripts/admin/addon/controllers/admin-dashboard.js @@ -13,9 +13,14 @@ export default Controller.extend({ exceptionController: controller("exception"), showVersionChecks: setting("version_checks"), - @discourseComputed("problems.length") - foundProblems(problemsLength) { - return this.currentUser.get("admin") && (problemsLength || 0) > 0; + @discourseComputed( + "lowPriorityProblems.length", + "highPriorityProblems.length" + ) + foundProblems(lowPriorityProblemsLength, highPriorityProblemsLength) { + const problemsLength = + lowPriorityProblemsLength + highPriorityProblemsLength; + return this.currentUser.admin && problemsLength > 0; }, visibleTabs: computed("siteSettings.dashboard_visible_tabs", function () { @@ -92,7 +97,16 @@ export default Controller.extend({ }); AdminDashboard.fetchProblems() - .then((model) => this.set("problems", model.problems)) + .then((model) => { + this.set( + "highPriorityProblems", + model.problems.filterBy("priority", "high") + ); + this.set( + "lowPriorityProblems", + model.problems.filterBy("priority", "low") + ); + }) .finally(() => this.set("loadingProblems", false)); }, diff --git a/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs b/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs index cd74918a73a..755ec62c793 100644 --- a/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs +++ b/app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs @@ -1,17 +1,34 @@ {{#if foundProblems}}
-

- {{d-icon "heart"}} - {{i18n "admin.dashboard.problems_found"}} -

+ {{#if highPriorityProblems.length}} +

+ {{d-icon "exclamation-triangle"}} + {{i18n "admin.dashboard.critical_problems_found"}} +

+ {{else}} +

+ {{d-icon "heart"}} + {{i18n "admin.dashboard.problems_found"}} +

+ {{/if}}
{{#conditional-loading-section isLoading=loadingProblems}} -
+ {{#if highPriorityProblems.length}} +
+
    + {{#each highPriorityProblems as |problem|}} +
  • {{html-safe problem.message}}
    • + {{/each}} +
+
+ {{/if}} + +
    - {{#each problems as |problem|}} + {{#each lowPriorityProblems as |problem|}}
  • {{html-safe problem.message}}
    • {{/each}}
diff --git a/app/assets/javascripts/admin/addon/templates/dashboard.hbs b/app/assets/javascripts/admin/addon/templates/dashboard.hbs index 95f26205e7c..07e1ab205d6 100644 --- a/app/assets/javascripts/admin/addon/templates/dashboard.hbs +++ b/app/assets/javascripts/admin/addon/templates/dashboard.hbs @@ -11,7 +11,8 @@ {{dashboard-problems loadingProblems=loadingProblems foundProblems=foundProblems - problems=problems + lowPriorityProblems=lowPriorityProblems + highPriorityProblems=highPriorityProblems problemsTimestamp=problemsTimestamp refreshProblems=(action "refreshProblems") }} diff --git a/app/assets/stylesheets/common/admin/dashboard.scss b/app/assets/stylesheets/common/admin/dashboard.scss index 955cf91cdc6..b9b84d7063c 100644 --- a/app/assets/stylesheets/common/admin/dashboard.scss +++ b/app/assets/stylesheets/common/admin/dashboard.scss @@ -237,8 +237,21 @@ .dashboard-problems { margin-bottom: 2em; - .problem-messages ul { - margin: 0 0 0 1.25em; + .problem-messages { + margin-bottom: 1.25em; + + &.priority-high { + background-color: var(--danger-low); + border: 1px solid var(--danger-medium); + } + + ul { + margin: 0 0 0 1.25em; + + li.dashboard-problem { + padding: 0.5em 0.5em; + } + } } .d-icon-exclamation-triangle { diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 6a2d6d37d11..2ca448e3237 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -174,6 +174,7 @@ class GroupsController < ApplicationController group.record_email_setting_changes!(current_user) group.expire_imap_mailbox_cache update_existing_users(group.group_users, notification_level, categories, tags) if params[:update_existing_users] == "true" + AdminDashboardData.clear_found_problem("group_#{group.id}_email_credentials") if guardian.can_see?(group) render json: success_json diff --git a/app/models/admin_dashboard_data.rb b/app/models/admin_dashboard_data.rb index 84748dcbcb9..831dc099dc8 100644 --- a/app/models/admin_dashboard_data.rb +++ b/app/models/admin_dashboard_data.rb @@ -124,7 +124,21 @@ class AdminDashboardData end def self.register_default_scheduled_problem_checks - # TODO (martin) Add group SMTP check here + add_scheduled_problem_check(:group_smtp_credentials) do + problems = GroupEmailCredentialsCheck.run + problems.map do |p| + problem_message = I18n.t( + "dashboard.group_email_credentials_warning", + { + base_path: Discourse.base_path, + group_name: p[:group_name], + group_full_name: p[:group_full_name], + error: p[:message] + } + ) + Problem.new(problem_message, priority: "high", identifier: "group_#{p[:group_id]}_email_credentials") + end + end end def self.execute_scheduled_checks diff --git a/app/models/group.rb b/app/models/group.rb index ebb703e88cd..a53d7e3ce37 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -138,6 +138,7 @@ class Group < ActiveRecord::Base validates :messageable_level, inclusion: { in: ALIAS_LEVELS.values } scope :with_imap_configured, -> { where(imap_enabled: true).where.not(imap_mailbox_name: '') } + scope :with_smtp_configured, -> { where(smtp_enabled: true) } scope :visible_groups, Proc.new { |user, order, opts| groups = self.order(order || "name ASC") diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index bac834ae433..1a05c0a89b5 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -4005,6 +4005,7 @@ en: installed_version: "Installed" latest_version: "Latest" problems_found: "Some advice based on your current site settings" + critical_problems_found: "You have some high priority problems that must be addressed" new_features: title: "🎁 New Features" dismiss: "Dismiss" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 84b2d10a97a..74bd406a87e 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1436,6 +1436,7 @@ en: description: "Top 10 users who have had the likes from a wide range of people." dashboard: + group_email_credentials_warning: 'There was an issue with the email credentials for the group %{group_full_name}. No emails will send from the group inbox until this problem is addressed. %{error}' rails_env_warning: "Your server is running in %{env} mode." host_names_warning: "Your config/database.yml file is using the default localhost hostname. Update it to use your site's hostname." sidekiq_warning: 'Sidekiq is not running. Many tasks, like sending emails, are executed asynchronously by sidekiq. Please ensure at least one sidekiq process is running. Learn about Sidekiq here.' diff --git a/lib/group_email_credentials_check.rb b/lib/group_email_credentials_check.rb new file mode 100644 index 00000000000..753429ed820 --- /dev/null +++ b/lib/group_email_credentials_check.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +## +# If group SMTP or IMAP has been configured, we want to make sure the +# credentials are always valid otherwise emails will not be sending out +# from group inboxes. This check is run as part of scheduled AdminDashboardData +# problem checks, and if any credentials have issues they will show up on +# the admin dashboard as a high priority issue. +class GroupEmailCredentialsCheck + def self.run + errors = [] + + if SiteSetting.enable_smtp + Group.with_smtp_configured.find_each do |group| + errors << try_validate(group) do + EmailSettingsValidator.validate_smtp( + host: group.smtp_server, + port: group.smtp_port, + username: group.email_username, + password: group.email_password + ) + end + end + end + + if SiteSetting.enable_imap + Group.with_imap_configured.find_each do |group| + errors << try_validate(group) do + EmailSettingsValidator.validate_imap( + host: group.smtp_server, + port: group.smtp_port, + username: group.email_username, + password: group.email_password + ) + end + end + end + + errors.compact + end + + def self.try_validate(group, &blk) + begin + blk.call + nil + rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS => err + { + group_id: group.id, + group_name: group.name, + group_full_name: group.full_name, + message: EmailSettingsExceptionHandler.friendly_exception_message(err, group.smtp_server) + } + rescue => err + Discourse.warn_exception( + err, message: "Unexpected error when checking SMTP credentials for group #{group.id} (#{group.name})." + ) + nil + end + end +end diff --git a/spec/lib/group_email_credentials_check_spec.rb b/spec/lib/group_email_credentials_check_spec.rb new file mode 100644 index 00000000000..152fc4a1af3 --- /dev/null +++ b/spec/lib/group_email_credentials_check_spec.rb @@ -0,0 +1,86 @@ +# frozen_string_literal: true + +require 'rails_helper' +require 'net/smtp' +require 'net/imap' + +describe GroupEmailCredentialsCheck do + fab!(:group1) { Fabricate(:group) } + fab!(:group2) { Fabricate(:smtp_group) } + fab!(:group3) { Fabricate(:imap_group) } + + describe "#run" do + it "does nothing if SMTP is disabled for the site" do + expect_no_validate_any + SiteSetting.enable_smtp = false + expect(described_class.run).to eq([]) + end + + context "with smtp and imap enabled for the site" do + before do + SiteSetting.enable_smtp = true + SiteSetting.enable_imap = true + end + + it "does nothing if no groups have smtp enabled" do + expect_no_validate_any + group2.update!(smtp_enabled: false) + group3.update!(smtp_enabled: false, imap_enabled: false) + expect(described_class.run).to eq([]) + end + + it "returns an error message and the group ID if the group's SMTP settings error" do + EmailSettingsValidator.expects(:validate_smtp).raises( + Net::SMTPAuthenticationError.new("bad credentials") + ).then.returns(true).at_least_once + EmailSettingsValidator.stubs(:validate_imap).returns(true) + + expect(described_class.run).to eq([ + { + group_full_name: group2.full_name, + group_name: group2.name, + group_id: group2.id, + message: I18n.t("email_settings.smtp_authentication_error") + } + ]) + end + + it "returns an error message and the group ID if the group's IMAP settings error" do + EmailSettingsValidator.stubs(:validate_smtp).returns(true) + EmailSettingsValidator.expects(:validate_imap).raises( + Net::IMAP::NoResponseError.new(stub(data: stub(text: "Invalid credentials"))) + ).once + + expect(described_class.run).to eq([ + { + group_full_name: group3.full_name, + group_name: group3.name, + group_id: group3.id, + message: I18n.t("email_settings.imap_authentication_error") + } + ]) + end + + it "returns no imap errors if imap is disabled for the site" do + SiteSetting.enable_imap = false + EmailSettingsValidator.stubs(:validate_smtp).returns(true) + EmailSettingsValidator.expects(:validate_imap).never + + expect(described_class.run).to eq([]) + end + end + end + + def expect_no_validate_imap + EmailSettingsValidator.expects(:validate_imap).never + end + + def expect_no_validate_smtp + EmailSettingsValidator.expects(:validate_smtp).never + end + + def expect_no_validate_any + expect_no_validate_smtp + expect_no_validate_imap + end +end