FEATURE: Scheduled group email credential problem check (#15396)

This commit adds a check that runs regularly as per
2d68e5d942 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.

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));
   },
 

app/assets/javascripts/admin/addon/templates/components/dashboard-problems.hbs

@@ -1,17 +1,34 @@
 {{#if foundProblems}}
   <div class="section dashboard-problems">
     <div class="section-title">
-      <h2>
-        {{d-icon "heart"}}
-        {{i18n "admin.dashboard.problems_found"}}
-      </h2>
+      {{#if highPriorityProblems.length}}
+        <h2>
+          {{d-icon "exclamation-triangle"}}
+          {{i18n "admin.dashboard.critical_problems_found"}}
+        </h2>
+      {{else}}
+        <h2>
+          {{d-icon "heart"}}
+          {{i18n "admin.dashboard.problems_found"}}
+        </h2>
+      {{/if}}
     </div>
 
     <div class="section-body">
       {{#conditional-loading-section isLoading=loadingProblems}}
-        <div class="problem-messages">
+        {{#if highPriorityProblems.length}}
+          <div class="problem-messages priority-high">
+            <ul>
+              {{#each highPriorityProblems as |problem|}}
+                <li class={{concat "dashboard-problem " "priority-" problem.priority}}>{{html-safe problem.message}}</li>
+              {{/each}}
+            </ul>
+          </div>
+        {{/if}}
+
+        <div class="problem-messages priority-low">
           <ul>
-            {{#each problems as |problem|}}
+            {{#each lowPriorityProblems as |problem|}}
               <li class={{concat "dashboard-problem " "priority-" problem.priority}}>{{html-safe problem.message}}</li>
             {{/each}}
           </ul>

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")
 }}

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 {

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

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

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")

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"

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 <a href="%{base_path}/g/%{group_name}/manage/email">%{group_full_name}</a>. 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. <a href="https://github.com/mperham/sidekiq" target="_blank">Learn about Sidekiq here</a>.'

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

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