From 964da218173db007fefe6357e96292f5545c513e Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 28 May 2021 09:28:18 +1000 Subject: [PATCH] FEATURE: Improve group email settings UI (#13083) This overhauls the user interface for the group email settings management, aiming to make it a lot easier to test the settings entered and confirm they are correct before proceeding. We do this by forcing the user to test the settings before they can be saved to the database. It also includes some quality of life improvements around setting up IMAP and SMTP for our first supported provider, GMail. This PR does not remove the old group email config, that will come in a subsequent PR. This is related to https://meta.discourse.org/t/imap-support-for-group-inboxes/160588 so read that if you would like more backstory. ### UI Both site settings of `enable_imap` and `enable_smtp` must be true to test this. You must enable SMTP first to enable IMAP. You can prefill the SMTP settings with GMail configuration. To proceed with saving these settings you must test them, which is handled by the EmailSettingsValidator. If there is an issue with the configuration or credentials a meaningful error message should be shown. IMAP settings must also be validated when IMAP is enabled, before saving. When saving IMAP, we fetch the mailboxes for that account and populate them. This mailbox must be selected and saved for IMAP to work (the feature acts as though it is disabled until the mailbox is selected and saved): ### Database & Backend This adds several columns to the Groups table. The purpose of this change is to make it much more explicit that SMTP/IMAP is enabled for a group, rather than relying on settings not being null. Also included is an UPDATE query to backfill these columns. These columns are automatically filled when updating the group. For GMail, we now filter the mailboxes returned. This is so users cannot use a mailbox like Sent or Trash for syncing, which would generally be disastrous. There is a new group endpoint for testing email settings. This may be useful in the future for other places in our UI, at which point it can be extracted to a more generic endpoint or module to be included. --- app/assets/javascripts/application.js | 1 + .../components/group-imap-email-settings.js | 91 +++++++ .../components/group-manage-email-settings.js | 116 +++++++++ .../components/group-manage-save-button.js | 9 + .../components/group-smtp-email-settings.js | 82 +++++++ .../discourse/app/lib/ajax-error.js | 6 + .../lib/email-provider-default-settings.js | 22 ++ .../javascripts/discourse/app/models/group.js | 2 + .../app/routes/group-manage-email.js | 3 +- .../components/group-imap-email-settings.hbs | 80 ++++++ .../group-manage-email-settings.hbs | 38 +++ .../components/group-manage-save-button.hbs | 4 +- .../components/group-smtp-email-settings.hbs | 58 +++++ .../app/templates/group/manage/email.hbs | 5 +- .../group-manage-email-settings-test.js | 230 ++++++++++++++++++ .../tests/fixtures/group-fixtures.js | 6 +- app/assets/stylesheets/common/base/alert.scss | 4 + app/assets/stylesheets/common/base/group.scss | 24 ++ app/assets/stylesheets/mobile/group.scss | 17 ++ app/controllers/groups_controller.rb | 82 ++++++- app/mailers/group_smtp_mailer.rb | 2 +- app/models/group.rb | 69 ++++-- app/serializers/group_show_serializer.rb | 9 + .../email_settings_exception_handler.rb | 128 ++++++++++ app/services/email_settings_validator.rb | 91 ++++--- app/services/post_alerter.rb | 8 +- config/locales/client.en.yml | 21 +- config/locales/server.en.yml | 2 + config/routes.rb | 1 + ...ated_enable_imap_smtp_columns_for_group.rb | 35 +++ lib/imap/providers/generic.rb | 13 +- lib/imap/providers/gmail.rb | 9 + spec/fabricators/group_fabricator.rb | 2 + spec/lib/imap/providers/gmail_spec.rb | 19 ++ spec/mailers/group_smtp_mailer_spec.rb | 6 +- spec/models/group_spec.rb | 109 +++++++-- .../api/schemas/json/group_response.json | 30 +++ spec/requests/groups_controller_spec.rb | 112 +++++++++ .../email_settings_exception_handler_spec.rb | 100 ++++++++ .../services/email_settings_validator_spec.rb | 101 ++------ spec/services/post_alerter_spec.rb | 3 +- 41 files changed, 1564 insertions(+), 186 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/components/group-imap-email-settings.js create mode 100644 app/assets/javascripts/discourse/app/components/group-manage-email-settings.js create mode 100644 app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js create mode 100644 app/assets/javascripts/discourse/app/lib/email-provider-default-settings.js create mode 100644 app/assets/javascripts/discourse/app/templates/components/group-imap-email-settings.hbs create mode 100644 app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs create mode 100644 app/assets/javascripts/discourse/app/templates/components/group-smtp-email-settings.hbs create mode 100644 app/assets/javascripts/discourse/tests/acceptance/group-manage-email-settings-test.js create mode 100644 app/services/email_settings_exception_handler.rb create mode 100644 db/migrate/20210517061815_add_dedicated_enable_imap_smtp_columns_for_group.rb create mode 100644 spec/services/email_settings_exception_handler_spec.rb diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js index a42435af3a9..0e9654e0ddb 100644 --- a/app/assets/javascripts/application.js +++ b/app/assets/javascripts/application.js @@ -22,6 +22,7 @@ //= require ./discourse/app/lib/offset-calculator //= require ./discourse/app/lib/lock-on //= require ./discourse/app/lib/url +//= require ./discourse/app/lib/email-provider-default-settings //= require ./discourse/app/lib/debounce //= require ./discourse/app/lib/quote //= require ./discourse/app/lib/key-value-store diff --git a/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js new file mode 100644 index 00000000000..012c1178691 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js @@ -0,0 +1,91 @@ +import Component from "@ember/component"; +import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings"; +import { isEmpty } from "@ember/utils"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import discourseComputed, { on } from "discourse-common/utils/decorators"; +import EmberObject, { action } from "@ember/object"; +import { ajax } from "discourse/lib/ajax"; + +export default Component.extend({ + tagName: "", + form: null, + + @discourseComputed( + "group.email_username", + "group.email_password", + "form.imap_server", + "form.imap_port" + ) + missingSettings(email_username, email_password, imap_server, imap_port) { + return [ + email_username, + email_password, + imap_server, + imap_port, + ].some((value) => isEmpty(value)); + }, + + @discourseComputed("group.imap_mailboxes") + mailboxes(imapMailboxes) { + if (!imapMailboxes) { + return []; + } + return imapMailboxes.map((mailbox) => ({ name: mailbox, value: mailbox })); + }, + + @discourseComputed("group.imap_mailbox_name", "mailboxes.length") + mailboxSelected(mailboxName, mailboxesSize) { + return mailboxesSize === 0 || !isEmpty(mailboxName); + }, + + @action + resetSettingsValid() { + this.set("imapSettingsValid", false); + }, + + @on("init") + _fillForm() { + this.set( + "form", + EmberObject.create({ + imap_server: this.group.imap_server, + imap_port: (this.group.imap_port || "").toString(), + imap_ssl: this.group.imap_ssl, + }) + ); + }, + + @action + prefillSettings(provider) { + this.form.setProperties(emailProviderDefaultSettings(provider, "imap")); + }, + + @action + testImapSettings() { + const settings = { + host: this.form.imap_server, + port: this.form.imap_port, + ssl: this.form.imap_ssl, + username: this.group.email_username, + password: this.group.email_password, + }; + + this.set("testingSettings", true); + this.set("imapSettingsValid", false); + + return ajax(`/groups/${this.group.id}/test_email_settings`, { + type: "POST", + data: Object.assign(settings, { protocol: "imap" }), + }) + .then(() => { + this.set("imapSettingsValid", true); + this.group.setProperties({ + imap_server: this.form.imap_server, + imap_port: this.form.imap_port, + imap_ssl: this.form.imap_ssl, + }); + }) + .catch(popupAjaxError) + .finally(() => this.set("testingSettings", false)); + }, +}); diff --git a/app/assets/javascripts/discourse/app/components/group-manage-email-settings.js b/app/assets/javascripts/discourse/app/components/group-manage-email-settings.js new file mode 100644 index 00000000000..ffa586b32a3 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/group-manage-email-settings.js @@ -0,0 +1,116 @@ +import Component from "@ember/component"; +import { isEmpty } from "@ember/utils"; +import discourseComputed, { on } from "discourse-common/utils/decorators"; +import I18n from "I18n"; +import bootbox from "bootbox"; +import { action } from "@ember/object"; + +export default Component.extend({ + tagName: "", + + imapSettingsValid: false, + smtpSettingsValid: false, + + @on("init") + _determineSettingsValid() { + this.set( + "imapSettingsValid", + this.group.imap_enabled && this.group.imap_server + ); + this.set( + "smtpSettingsValid", + this.group.smtp_enabled && this.group.smtp_server + ); + }, + + @discourseComputed( + "emailSettingsValid", + "group.smtp_enabled", + "group.imap_enabled" + ) + enableImapSettings(emailSettingsValid, smtpEnabled, imapEnabled) { + return smtpEnabled && (emailSettingsValid || imapEnabled); + }, + + @discourseComputed( + "smtpSettingsValid", + "imapSettingsValid", + "group.smtp_enabled", + "group.imap_enabled" + ) + emailSettingsValid( + smtpSettingsValid, + imapSettingsValid, + smtpEnabled, + imapEnabled + ) { + return ( + (!smtpEnabled || smtpSettingsValid) && (!imapEnabled || imapSettingsValid) + ); + }, + + _anySmtpFieldsFilled() { + return [ + this.group.smtp_server, + this.group.smtp_port, + this.group.email_username, + this.group.email_password, + ].some((value) => !isEmpty(value)); + }, + + _anyImapFieldsFilled() { + return [this.group.imap_server, this.group.imap_port].some( + (value) => !isEmpty(value) + ); + }, + + @action + smtpEnabledChange(event) { + if ( + !event.target.checked && + this.group.smtp_enabled && + this._anySmtpFieldsFilled() + ) { + bootbox.confirm( + I18n.t("groups.manage.email.smtp_disable_confirm"), + (result) => { + if (!result) { + this.group.set("smtp_enabled", true); + } else { + this.group.set("imap_enabled", false); + } + } + ); + } + + this.group.set("smtp_enabled", event.target.checked); + }, + + @action + imapEnabledChange(event) { + if ( + !event.target.checked && + this.group.imap_enabled && + this._anyImapFieldsFilled() + ) { + bootbox.confirm( + I18n.t("groups.manage.email.imap_disable_confirm"), + (result) => { + if (!result) { + this.group.set("imap_enabled", true); + } + } + ); + } + + this.group.set("imap_enabled", event.target.checked); + }, + + @action + afterSave() { + // reload the group to get the updated imap_mailboxes + this.store.find("group", this.group.name).then(() => { + this._determineSettingsValid(); + }); + }, +}); diff --git a/app/assets/javascripts/discourse/app/components/group-manage-save-button.js b/app/assets/javascripts/discourse/app/components/group-manage-save-button.js index 0a92d4bd843..82cc182b7d7 100644 --- a/app/assets/javascripts/discourse/app/components/group-manage-save-button.js +++ b/app/assets/javascripts/discourse/app/components/group-manage-save-button.js @@ -7,6 +7,7 @@ import { popupAutomaticMembershipAlert } from "discourse/controllers/groups-new" export default Component.extend({ saving: null, + disabled: false, @discourseComputed("saving") savingText(saving) { @@ -15,6 +16,10 @@ export default Component.extend({ actions: { save() { + if (this.beforeSave) { + this.beforeSave(); + } + this.set("saving", true); const group = this.model; @@ -31,6 +36,10 @@ export default Component.extend({ } this.set("saved", true); + + if (this.afterSave) { + this.afterSave(); + } }) .catch(popupAjaxError) .finally(() => this.set("saving", false)); diff --git a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js new file mode 100644 index 00000000000..d9758d2c463 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -0,0 +1,82 @@ +import Component from "@ember/component"; +import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings"; +import { isEmpty } from "@ember/utils"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import discourseComputed, { on } from "discourse-common/utils/decorators"; +import EmberObject, { action } from "@ember/object"; +import { ajax } from "discourse/lib/ajax"; + +export default Component.extend({ + tagName: "", + form: null, + + @discourseComputed( + "form.email_username", + "form.email_password", + "form.smtp_server", + "form.smtp_port" + ) + missingSettings(email_username, email_password, smtp_server, smtp_port) { + return [ + email_username, + email_password, + smtp_server, + smtp_port, + ].some((value) => isEmpty(value)); + }, + + @action + resetSettingsValid() { + this.set("smtpSettingsValid", false); + }, + + @on("init") + _fillForm() { + this.set( + "form", + EmberObject.create({ + email_username: this.group.email_username, + email_password: this.group.email_password, + smtp_server: this.group.smtp_server, + smtp_port: (this.group.smtp_port || "").toString(), + smtp_ssl: this.group.smtp_ssl, + }) + ); + }, + + @action + prefillSettings(provider) { + this.form.setProperties(emailProviderDefaultSettings(provider, "smtp")); + }, + + @action + testSmtpSettings() { + const settings = { + host: this.form.smtp_server, + port: this.form.smtp_port, + ssl: this.form.smtp_ssl, + username: this.form.email_username, + password: this.form.email_password, + }; + + this.set("testingSettings", true); + this.set("smtpSettingsValid", false); + + return ajax(`/groups/${this.group.id}/test_email_settings`, { + type: "POST", + data: Object.assign(settings, { protocol: "smtp" }), + }) + .then(() => { + this.set("smtpSettingsValid", true); + this.group.setProperties({ + smtp_server: this.form.smtp_server, + smtp_port: this.form.smtp_port, + smtp_ssl: this.form.smtp_ssl, + email_username: this.form.email_username, + email_password: this.form.email_password, + }); + }) + .catch(popupAjaxError) + .finally(() => this.set("testingSettings", false)); + }, +}); diff --git a/app/assets/javascripts/discourse/app/lib/ajax-error.js b/app/assets/javascripts/discourse/app/lib/ajax-error.js index 0f3feba41ca..34a2b3604b4 100644 --- a/app/assets/javascripts/discourse/app/lib/ajax-error.js +++ b/app/assets/javascripts/discourse/app/lib/ajax-error.js @@ -1,4 +1,5 @@ import I18n from "I18n"; +import { isTesting } from "discourse-common/config/environment"; import bootbox from "bootbox"; export function extractError(error, defaultMessage) { @@ -69,6 +70,11 @@ export function popupAjaxError(error) { } bootbox.alert(extractError(error)); + // in testing mode we want to be able to test these ajax popup messages + if (isTesting()) { + return; + } + error._discourse_displayed = true; // We re-throw in a catch to not swallow the exception diff --git a/app/assets/javascripts/discourse/app/lib/email-provider-default-settings.js b/app/assets/javascripts/discourse/app/lib/email-provider-default-settings.js new file mode 100644 index 00000000000..fa2939f6ace --- /dev/null +++ b/app/assets/javascripts/discourse/app/lib/email-provider-default-settings.js @@ -0,0 +1,22 @@ +const GMAIL = { + imap: { + imap_server: "imap.gmail.com", + imap_port: "993", + imap_ssl: true, + }, + smtp: { + smtp_server: "smtp.gmail.com", + smtp_port: "587", + smtp_ssl: true, + }, +}; + +export default function emailProviderDefaultSettings(provider, protocol) { + provider = provider.toLowerCase(); + protocol = protocol.toLowerCase(); + + switch (provider) { + case "gmail": + return GMAIL[protocol]; + } +} diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index 429dcd05953..23a8e54f26a 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -217,10 +217,12 @@ const Group = RestModel.extend({ smtp_server: this.smtp_server, smtp_port: this.smtp_port, smtp_ssl: this.smtp_ssl, + smtp_enabled: this.smtp_enabled, imap_server: this.imap_server, imap_port: this.imap_port, imap_ssl: this.imap_ssl, imap_mailbox_name: this.imap_mailbox_name, + imap_enabled: this.imap_enabled, email_username: this.email_username, email_password: this.email_password, flair_icon: null, diff --git a/app/assets/javascripts/discourse/app/routes/group-manage-email.js b/app/assets/javascripts/discourse/app/routes/group-manage-email.js index 1ba2f5d5b51..5f8a26e43b7 100644 --- a/app/assets/javascripts/discourse/app/routes/group-manage-email.js +++ b/app/assets/javascripts/discourse/app/routes/group-manage-email.js @@ -5,7 +5,8 @@ export default DiscourseRoute.extend({ showFooter: true, beforeModel() { - if (!this.siteSettings.enable_imap && !this.siteSettings.enable_smtp) { + // cannot configure IMAP without SMTP being enabled + if (!this.siteSettings.enable_smtp) { return this.transitionTo("group.manage.profile"); } }, diff --git a/app/assets/javascripts/discourse/app/templates/components/group-imap-email-settings.hbs b/app/assets/javascripts/discourse/app/templates/components/group-imap-email-settings.hbs new file mode 100644 index 00000000000..2c6025d4ac4 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/components/group-imap-email-settings.hbs @@ -0,0 +1,80 @@ +
+
+
+
+ + {{input type="text" name="imap_server" value=form.imap_server tabindex="8" onChange=(action "resetSettingsValid")}} +
+ + +
+ +
+
+ + {{input type="text" name="imap_port" value=form.imap_port tabindex="9" onChange=(action "resetSettingsValid" form.imap_port)}} +
+
+ +
+
+ {{#if mailboxes}} + + {{combo-box name="imap_mailbox_name" + value=group.imap_mailbox_name + valueProperty="value" + content=mailboxes + none="groups.manage.email.mailboxes.disabled" + tabindex="10" + onChange=(action (mut group.imap_mailbox_name)) + }} + {{/if}} +
+ +
+
+ +
+
+ {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}} +
+
+ + {{#unless mailboxSelected}} +
+ {{i18n "groups.manage.email.imap_mailbox_not_selected"}} +
+ {{/unless}} + +
+ {{d-button + disabled=(or missingSettings testingSettings) + class="btn-primary test-imap-settings" + action=(action "testImapSettings") + icon="cog" + label="groups.manage.email.test_settings" + tabindex="12" + title="groups.manage.email.settings_required" + }} + + {{conditional-loading-spinner size="small" condition=testingSettings}} + + {{#if imapSettingsValid}} + + {{d-icon "check-circle"}} {{i18n "groups.manage.email.imap_settings_valid"}} + + {{/if}} +
+ +
+

{{i18n "groups.manage.email.imap_additional_settings"}}

+ +

{{i18n "groups.manage.email.settings.allow_unknown_sender_topic_replies_hint"}}

+
+
diff --git a/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs b/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs new file mode 100644 index 00000000000..97f08c53483 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs @@ -0,0 +1,38 @@ +
+

{{i18n "groups.manage.email.smtp_title"}}

+

{{i18n "groups.manage.email.smtp_instructions"}}

+ + + + {{#if group.smtp_enabled}} + {{group-smtp-email-settings group=group smtpSettingsValid=smtpSettingsValid}} + {{/if}} + + {{#if siteSettings.enable_imap}} +
+
+ +

{{i18n "groups.manage.email.imap_title"}}

+

+ {{html-safe (i18n "groups.manage.email.imap_instructions")}} +

+ +
{{i18n "groups.manage.email.imap_alpha_warning"}}
+ + + + {{#if group.imap_enabled}} + {{group-imap-email-settings group=group imapSettingsValid=imapSettingsValid}} + {{/if}} +
+ {{/if}} + +
+ {{group-manage-save-button model=group disabled=(not emailSettingsValid) beforeSave=beforeSave afterSave=afterSave tabindex="14"}} +
diff --git a/app/assets/javascripts/discourse/app/templates/components/group-manage-save-button.hbs b/app/assets/javascripts/discourse/app/templates/components/group-manage-save-button.hbs index 11b4167c70f..d4b358d00f6 100644 --- a/app/assets/javascripts/discourse/app/templates/components/group-manage-save-button.hbs +++ b/app/assets/javascripts/discourse/app/templates/components/group-manage-save-button.hbs @@ -1,6 +1,6 @@ -
+
{{d-button action=(action "save") - disabled=saving + disabled=(or disabled saving) class="btn btn-primary group-manage-save" translatedLabel=savingText }} diff --git a/app/assets/javascripts/discourse/app/templates/components/group-smtp-email-settings.hbs b/app/assets/javascripts/discourse/app/templates/components/group-smtp-email-settings.hbs new file mode 100644 index 00000000000..fbe3f88b5cb --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/components/group-smtp-email-settings.hbs @@ -0,0 +1,58 @@ +
+
+
+
+ + {{input type="text" name="username" value=form.email_username tabindex="1" onChange=(action "resetSettingsValid")}} +
+ +
+ + {{input type="text" name="smtp_server" value=form.smtp_server tabindex="3" onChange=(action "resetSettingsValid")}} +
+ + +
+ +
+
+ + {{input type="password" name="password" value=form.email_password tabindex="2" onChange=(action "resetSettingsValid")}} +
+ +
+ + {{input type="text" name="smtp_port" value=form.smtp_port tabindex="4" onChange=(action "resetSettingsValid" form.smtp_port)}} +
+
+
+ +
+
+ {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}} +
+
+ +
+ {{d-button + disabled=(or missingSettings testingSettings) + class="btn-primary test-smtp-settings" + action=(action "testSmtpSettings") + icon="cog" + label="groups.manage.email.test_settings" + tabindex="6" + title="groups.manage.email.settings_required" + }} + + {{conditional-loading-spinner size="small" condition=testingSettings}} + + {{#if smtpSettingsValid}} + + {{d-icon "check-circle"}} {{i18n "groups.manage.email.smtp_settings_valid"}} + + {{/if}} +
+
diff --git a/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs b/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs index 6ef5aada789..394dd2df081 100644 --- a/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs +++ b/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs @@ -1,4 +1 @@ -
- {{groups-form-email-fields model=model}} - {{group-manage-save-button model=model}} -
+{{group-manage-email-settings group=model}} diff --git a/app/assets/javascripts/discourse/tests/acceptance/group-manage-email-settings-test.js b/app/assets/javascripts/discourse/tests/acceptance/group-manage-email-settings-test.js new file mode 100644 index 00000000000..963f51ae171 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/acceptance/group-manage-email-settings-test.js @@ -0,0 +1,230 @@ +import { acceptance, queryAll } from "discourse/tests/helpers/qunit-helpers"; +import selectKit from "discourse/tests/helpers/select-kit-helper"; +import { click, currentRouteName, fillIn, visit } from "@ember/test-helpers"; +import I18n from "I18n"; +import { test } from "qunit"; + +acceptance("Managing Group Email Settings - SMTP Disabled", function (needs) { + needs.user(); + + test("When SiteSetting.enable_smtp is false", async function (assert) { + await visit("/g/discourse/manage/email"); + assert.equal( + currentRouteName(), + "group.manage.profile", + "it redirects to the group profile page" + ); + }); +}); + +acceptance( + "Managing Group Email Settings - SMTP Enabled, IMAP Disabled", + function (needs) { + needs.user(); + needs.settings({ enable_smtp: true }); + + test("When SiteSetting.enable_smtp is true but SiteSetting.enable_imap is false", async function (assert) { + await visit("/g/discourse/manage/email"); + assert.equal( + currentRouteName(), + "group.manage.email", + "it redirects to the group email page" + ); + assert.notOk( + exists(".group-manage-email-imap-wrapper"), + "does not show IMAP settings" + ); + }); + } +); + +acceptance( + "Managing Group Email Settings - SMTP and IMAP Enabled", + function (needs) { + needs.user(); + needs.settings({ enable_smtp: true, enable_imap: true }); + + needs.pretender((server, helper) => { + server.post("/groups/47/test_email_settings", () => { + return helper.response({ + success: "OK", + }); + }); + server.put("/groups/47", () => { + return helper.response({ + success: "OK", + }); + }); + }); + + test("enabling SMTP, testing, and saving", async function (assert) { + await visit("/g/discourse/manage/email"); + assert.ok( + exists("#enable_imap:disabled"), + "IMAP is disabled until SMTP settings are valid" + ); + + await click("#enable_smtp"); + assert.ok(exists(".group-smtp-email-settings")); + + await click("#prefill_smtp_gmail"); + assert.equal( + queryAll("input[name='smtp_server']").val(), + "smtp.gmail.com", + "prefills SMTP server settings for gmail" + ); + assert.equal( + queryAll("input[name='smtp_port']").val(), + "587", + "prefills SMTP port settings for gmail" + ); + assert.ok( + exists("#enable_ssl:checked"), + "prefills SMTP ssl settings for gmail" + ); + + assert.ok( + exists(".test-smtp-settings:disabled"), + "does not allow testing settings if not all fields are filled" + ); + + await fillIn('input[name="username"]', "myusername@gmail.com"); + await fillIn('input[name="password"]', "password@gmail.com"); + await click(".test-smtp-settings"); + + assert.ok(exists(".smtp-settings-ok"), "tested settings are ok"); + + await click(".group-manage-save"); + + assert.equal( + queryAll(".group-manage-save-button > span").text(), + "Saved!" + ); + + assert.notOk( + exists("#enable_imap:disabled"), + "IMAP is able to be enabled now that SMTP is saved" + ); + + await click("#enable_smtp"); + assert.equal( + queryAll(".modal-body").text(), + I18n.t("groups.manage.email.smtp_disable_confirm"), + "shows a confirm dialogue warning SMTP settings will be wiped" + ); + + await click(".modal-footer .btn.btn-primary"); + }); + + test("enabling IMAP, testing, and saving", async function (assert) { + await visit("/g/discourse/manage/email"); + + await click("#enable_smtp"); + await click("#prefill_smtp_gmail"); + await fillIn('input[name="username"]', "myusername@gmail.com"); + await fillIn('input[name="password"]', "password@gmail.com"); + await click(".test-smtp-settings"); + await click(".group-manage-save"); + + assert.notOk( + exists("#enable_imap:disabled"), + "IMAP is able to be enabled now that IMAP is saved" + ); + + await click("#enable_imap"); + + assert.ok( + exists(".test-imap-settings:disabled"), + "does not allow testing settings if not all fields are filled" + ); + + await click("#prefill_imap_gmail"); + assert.equal( + queryAll("input[name='imap_server']").val(), + "imap.gmail.com", + "prefills IMAP server settings for gmail" + ); + assert.equal( + queryAll("input[name='imap_port']").val(), + "993", + "prefills IMAP port settings for gmail" + ); + assert.ok( + exists("#enable_ssl:checked"), + "prefills IMAP ssl settings for gmail" + ); + await click(".test-imap-settings"); + + assert.ok(exists(".imap-settings-ok"), "tested settings are ok"); + + await click(".group-manage-save"); + + assert.equal( + queryAll(".group-manage-save-button > span").text(), + "Saved!" + ); + + assert.ok( + exists(".imap-no-mailbox-selected"), + "shows a message saying no IMAP mailbox is selected" + ); + + await selectKit( + ".control-group.group-imap-mailboxes .combo-box" + ).expand(); + await selectKit( + ".control-group.group-imap-mailboxes .combo-box" + ).selectRowByValue("All Mail"); + await click(".group-manage-save"); + + assert.notOk( + exists(".imap-no-mailbox-selected"), + "no longer shows a no mailbox selected message" + ); + + await click("#enable_imap"); + assert.equal( + queryAll(".modal-body").text(), + I18n.t("groups.manage.email.imap_disable_confirm"), + "shows a confirm dialogue warning IMAP settings will be wiped" + ); + await click(".modal-footer .btn.btn-primary"); + }); + } +); + +acceptance( + "Managing Group Email Settings - SMTP and IMAP Enabled - Email Test Invalid", + function (needs) { + needs.user(); + needs.settings({ enable_smtp: true, enable_imap: true }); + + needs.pretender((server, helper) => { + server.post("/groups/47/test_email_settings", () => { + return helper.response(422, { + success: false, + errors: [ + "There was an issue with the SMTP credentials provided, check the username and password and try again.", + ], + }); + }); + }); + + test("enabling IMAP, testing, and saving", async function (assert) { + await visit("/g/discourse/manage/email"); + + await click("#enable_smtp"); + await click("#prefill_smtp_gmail"); + await fillIn('input[name="username"]', "myusername@gmail.com"); + await fillIn('input[name="password"]', "password@gmail.com"); + await click(".test-smtp-settings"); + + assert.equal( + queryAll(".modal-body").text(), + "There was an issue with the SMTP credentials provided, check the username and password and try again.", + "shows a dialogue with the error message from the server" + ); + await click(".modal-footer .btn.btn-primary"); + }); + } +); diff --git a/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js b/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js index 615fe750864..04e15383ace 100644 --- a/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js +++ b/app/assets/javascripts/discourse/tests/fixtures/group-fixtures.js @@ -48,7 +48,11 @@ export default { messageable: true, can_see_members: true, has_messages: true, - message_count: 2 + message_count: 2, + imap_mailboxes: [ + "All Mail", + "Important" + ] }, extras: { visible_group_names: ["discourse"] diff --git a/app/assets/stylesheets/common/base/alert.scss b/app/assets/stylesheets/common/base/alert.scss index 64211b17522..8ade16c7188 100644 --- a/app/assets/stylesheets/common/base/alert.scss +++ b/app/assets/stylesheets/common/base/alert.scss @@ -23,6 +23,10 @@ background-color: var(--danger-low); color: var(--primary); } + &.alert-warning { + background-color: var(--highlight-low); + color: var(--primary); + } &.alert-info { background-color: var(--tertiary-low); color: var(--primary); diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index a8c0577ed2c..c39e6bee8fe 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -241,3 +241,27 @@ table.group-category-permissions { } } } + +.group-smtp-email-settings, +.group-imap-email-settings { + .groups-form { + display: grid; + grid-template-columns: 1fr 3fr; + margin-bottom: 0; + + &.groups-form-imap { + grid-template-columns: 1fr 1fr 2fr; + } + } + + background-color: var(--primary-very-low); + padding: 1em; + margin: 1em 0; + border: 1px solid var(--primary-low); + + .group-imap-mailboxes { + .combo-box { + width: 50%; + } + } +} diff --git a/app/assets/stylesheets/mobile/group.scss b/app/assets/stylesheets/mobile/group.scss index d54562b81a8..7053aa3ee46 100644 --- a/app/assets/stylesheets/mobile/group.scss +++ b/app/assets/stylesheets/mobile/group.scss @@ -47,3 +47,20 @@ margin-top: 0.5em; } } + +.group-smtp-email-settings, +.group-imap-email-settings { + .groups-form { + grid-template-columns: 1fr; + + input[type="text"], + input[type="password"], + .group-imap-mailboxes .combo-box { + width: 100%; + } + + &.groups-form-imap { + grid-template-columns: 1fr; + } + } +} diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 69dff9cf26d..3d6e6262bc3 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -10,7 +10,8 @@ class GroupsController < ApplicationController :histories, :request_membership, :search, - :new + :new, + :test_email_settings ] skip_before_action :preload_json, :check_xhr, only: [:posts_feed, :mentions_feed] @@ -150,8 +151,13 @@ class GroupsController < ApplicationController group = Group.find(params[:id]) guardian.ensure_can_edit!(group) unless guardian.can_admin_group?(group) - if group.update(group_params(automatic: group.automatic)) + params_with_permitted = group_params(automatic: group.automatic) + clear_disabled_email_settings(group, params_with_permitted) + + if group.update(params_with_permitted) GroupActionLogger.new(current_user, group, skip_guardian: true).log_change_group_settings + group.record_email_setting_changes!(current_user) + group.expire_imap_mailbox_cache if guardian.can_see?(group) render json: success_json @@ -580,6 +586,52 @@ class GroupsController < ApplicationController render_serialized(category_groups.sort_by { |category_group| category_group.category.name }, CategoryGroupSerializer) end + def test_email_settings + params.require(:group_id) + params.require(:protocol) + params.require(:port) + params.require(:host) + params.require(:username) + params.require(:password) + params.require(:ssl) + + group = Group.find(params[:group_id]) + guardian.ensure_can_edit!(group) + + RateLimiter.new(current_user, "group_test_email_settings", 5, 1.minute).performed! + + settings = params.except(:group_id, :protocol) + enable_tls = settings[:ssl] == "true" + email_host = params[:host] + + if !["smtp", "imap"].include?(params[:protocol]) + raise Discourse::InvalidParameters.new("Valid protocols to test are smtp and imap") + end + + hijack do + begin + case params[:protocol] + when "smtp" + enable_starttls_auto = false + settings.delete(:ssl) + + final_settings = settings.merge(enable_tls: enable_tls, enable_starttls_auto: enable_starttls_auto) + .permit(:host, :port, :username, :password, :enable_tls, :enable_starttls_auto, :debug) + EmailSettingsValidator.validate_as_user(current_user, "smtp", **final_settings.to_h.symbolize_keys) + when "imap" + final_settings = settings.merge(ssl: enable_tls) + .permit(:host, :port, :username, :password, :ssl, :debug) + EmailSettingsValidator.validate_as_user(current_user, "imap", **final_settings.to_h.symbolize_keys) + end + rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS, StandardError => err + return render_json_error( + EmailSettingsExceptionHandler.friendly_exception_message(err, email_host) + ) + end + render json: success_json + end + end + private def group_params(automatic: false) @@ -620,10 +672,16 @@ class GroupsController < ApplicationController :smtp_server, :smtp_port, :smtp_ssl, + :smtp_enabled, + :smtp_updated_by, + :smtp_updated_at, :imap_server, :imap_port, :imap_ssl, :imap_mailbox_name, + :imap_enabled, + :imap_updated_by, + :imap_updated_at, :email_username, :email_password, :primary_group, @@ -678,4 +736,24 @@ class GroupsController < ApplicationController end users end + + def clear_disabled_email_settings(group, params_with_permitted) + should_clear_imap = group.imap_enabled && params_with_permitted.key?(:imap_enabled) && params_with_permitted[:imap_enabled] == "false" + should_clear_smtp = group.smtp_enabled && params_with_permitted.key?(:smtp_enabled) && params_with_permitted[:smtp_enabled] == "false" + + if should_clear_imap || should_clear_smtp + params_with_permitted[:imap_server] = nil + params_with_permitted[:imap_ssl] = false + params_with_permitted[:imap_port] = nil + params_with_permitted[:imap_mailbox_name] = "" + end + + if should_clear_smtp + params_with_permitted[:smtp_server] = nil + params_with_permitted[:smtp_ssl] = false + params_with_permitted[:smtp_port] = nil + params_with_permitted[:email_username] = nil + params_with_permitted[:email_password] = nil + end + end end diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index 58fcbc4a1b8..f8510deae34 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/app/mailers/group_smtp_mailer.rb @@ -48,7 +48,7 @@ class GroupSmtpMailer < ActionMailer::Base group_name: from_group.name, allow_reply_by_email: true, only_reply_by_email: true, - use_from_address_for_reply_to: from_group.imap_enabled?, + use_from_address_for_reply_to: SiteSetting.enable_imap && from_group.imap_enabled?, private_reply: post.topic.private_message?, participants: participants(post), include_respond_instructions: true, diff --git a/app/models/group.rb b/app/models/group.rb index 16c47fd1967..4aad4ea7c80 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -34,6 +34,8 @@ class Group < ActiveRecord::Base has_many :group_tag_notification_defaults, dependent: :destroy belongs_to :flair_upload, class_name: 'Upload' + belongs_to :smtp_updated_by, class_name: 'User' + belongs_to :imap_updated_by, class_name: 'User' has_and_belongs_to_many :web_hooks @@ -59,6 +61,10 @@ class Group < ActiveRecord::Base def expire_cache ApplicationSerializer.expire_cache_fragment!("group_names") SvgSprite.expire_cache + expire_imap_mailbox_cache + end + + def expire_imap_mailbox_cache Discourse.cache.delete("group_imap_mailboxes_#{self.id}") end @@ -88,6 +94,22 @@ class Group < ActiveRecord::Base AUTO_GROUP_IDS = Hash[*AUTO_GROUPS.to_a.flatten.reverse] STAFF_GROUPS = [:admins, :moderators, :staff] + IMAP_SETTING_ATTRIBUTES = [ + "imap_server", + "imap_port", + "imap_ssl", + "email_username", + "email_password" + ] + + SMTP_SETTING_ATTRIBUTES = [ + "imap_server", + "imap_port", + "imap_ssl", + "email_username", + "email_password" + ] + ALIAS_LEVELS = { nobody: 0, only_admins: 1, @@ -112,7 +134,7 @@ class Group < ActiveRecord::Base validates :mentionable_level, inclusion: { in: ALIAS_LEVELS.values } validates :messageable_level, inclusion: { in: ALIAS_LEVELS.values } - scope :with_imap_configured, -> { where.not(imap_mailbox_name: '') } + scope :with_imap_configured, -> { where(imap_enabled: true).where.not(imap_mailbox_name: '') } scope :visible_groups, Proc.new { |user, order, opts| groups = self.order(order || "name ASC") @@ -281,6 +303,23 @@ class Group < ActiveRecord::Base end end + def record_email_setting_changes!(user) + if (self.previous_changes.keys & IMAP_SETTING_ATTRIBUTES).any? + self.imap_updated_at = Time.zone.now + self.imap_updated_by_id = user.id + end + + if (self.previous_changes.keys & SMTP_SETTING_ATTRIBUTES).any? + self.smtp_updated_at = Time.zone.now + self.smtp_updated_by_id = user.id + end + + self.smtp_enabled = [self.smtp_port, self.smtp_server, self.email_password, self.email_username].all?(&:present?) + self.imap_enabled = [self.imap_port, self.imap_server, self.email_password, self.email_username].all?(&:present?) + + self.save + end + def incoming_email_validator return if self.automatic || self.incoming_email.blank? @@ -796,10 +835,7 @@ class Group < ActiveRecord::Base end def imap_mailboxes - return [] if self.imap_server.blank? || - self.email_username.blank? || - self.email_password.blank? || - !SiteSetting.enable_imap + return [] if !self.imap_enabled || !SiteSetting.enable_imap Discourse.cache.fetch("group_imap_mailboxes_#{self.id}", expires_in: 30.minutes) do Rails.logger.info("[IMAP] Refreshing mailboxes list for group #{self.name}") @@ -810,7 +846,7 @@ class Group < ActiveRecord::Base self.imap_config ) imap_provider.connect! - mailboxes = imap_provider.list_mailboxes + mailboxes = imap_provider.filter_mailboxes(imap_provider.list_mailboxes_with_attributes) imap_provider.disconnect! update_columns(imap_last_error: nil) @@ -833,11 +869,6 @@ class Group < ActiveRecord::Base } end - def imap_enabled? - return false if !SiteSetting.enable_imap - imap_config.values.compact.length == imap_config.keys.length - end - def email_username_regex user, domain = email_username.split('@') if user.present? && domain.present? @@ -1042,10 +1073,6 @@ end # membership_request_template :text # messageable_level :integer default(0) # mentionable_level :integer default(0) -# publish_read_state :boolean default(FALSE), not null -# members_visibility_level :integer default(0), not null -# flair_icon :string -# flair_upload_id :integer # smtp_server :string # smtp_port :integer # smtp_ssl :boolean @@ -1057,10 +1084,20 @@ end # imap_last_uid :integer default(0), not null # email_username :string # email_password :string +# publish_read_state :boolean default(FALSE), not null +# members_visibility_level :integer default(0), not null # imap_last_error :text # imap_old_emails :integer # imap_new_emails :integer -# allow_unknown_sender_topic_replies :boolean default(FALSE) +# flair_icon :string +# flair_upload_id :integer +# allow_unknown_sender_topic_replies :boolean default(FALSE), not null +# smtp_enabled :boolean default(FALSE) +# smtp_updated_at :datetime +# smtp_updated_by_id :integer +# imap_enabled :boolean default(FALSE) +# imap_updated_at :datetime +# imap_updated_by_id :integer # # Indexes # diff --git a/app/serializers/group_show_serializer.rb b/app/serializers/group_show_serializer.rb index 1b0bdf8b789..9cdf1f842d3 100644 --- a/app/serializers/group_show_serializer.rb +++ b/app/serializers/group_show_serializer.rb @@ -12,15 +12,24 @@ class GroupShowSerializer < BasicGroupSerializer end end + has_one :smtp_updated_by, embed: :object, serializer: BasicUserSerializer + has_one :imap_updated_by, embed: :object, serializer: BasicUserSerializer + admin_attributes :automatic_membership_email_domains, :smtp_server, :smtp_port, :smtp_ssl, + :smtp_enabled, + :smtp_updated_at, + :smtp_updated_by, :imap_server, :imap_port, :imap_ssl, :imap_mailbox_name, :imap_mailboxes, + :imap_enabled, + :imap_updated_at, + :imap_updated_by, :email_username, :email_password, :imap_last_error, diff --git a/app/services/email_settings_exception_handler.rb b/app/services/email_settings_exception_handler.rb new file mode 100644 index 00000000000..ddb3fb908b8 --- /dev/null +++ b/app/services/email_settings_exception_handler.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'net/imap' +require 'net/smtp' +require 'net/pop' + +class EmailSettingsExceptionHandler + EXPECTED_EXCEPTIONS = [ + Net::POPAuthenticationError, + Net::IMAP::NoResponseError, + Net::IMAP::Error, + Net::SMTPAuthenticationError, + Net::SMTPServerBusy, + Net::SMTPSyntaxError, + Net::SMTPFatalError, + Net::SMTPUnknownError, + Net::OpenTimeout, + Net::ReadTimeout, + SocketError, + Errno::ECONNREFUSED + ] + + class GenericProvider + def initialize(exception) + @exception = exception + end + + def message + case @exception + when Net::POPAuthenticationError + net_pop_authentication_error + when Net::IMAP::NoResponseError + net_imap_no_response_error + when Net::IMAP::Error + net_imap_unhandled_error + when Net::SMTPAuthenticationError + net_smtp_authentication_error + when Net::SMTPServerBusy + net_smtp_server_busy + when Net::SMTPSyntaxError, Net::SMTPFatalError, Net::SMTPUnknownError + net_smtp_unhandled_error + when SocketError, Errno::ECONNREFUSED + socket_connection_error + when Net::OpenTimeout, Net::ReadTimeout + net_timeout_error + else + unhandled_error + end + end + + private + + def net_pop_authentication_error + I18n.t("email_settings.pop3_authentication_error") + end + + def net_imap_no_response_error + # Most of IMAP's errors are lumped under the NoResponseError, including invalid + # credentials errors, because it is raised when a "NO" response is + # raised from the IMAP server https://datatracker.ietf.org/doc/html/rfc3501#section-7.1.2 + # + # Generally, it should be fairly safe to just return the error message as is. + if @exception.message.match(/Invalid credentials/) + I18n.t("email_settings.imap_authentication_error") + else + I18n.t("email_settings.imap_no_response_error", message: @exception.message.gsub(" (Failure)", "")) + end + end + + def net_imap_unhandled_error + I18n.t("email_settings.imap_unhandled_error", message: @exception.message) + end + + def net_smtp_authentication_error + I18n.t("email_settings.smtp_authentication_error") + end + + def net_smtp_server_busy + I18n.t("email_settings.smtp_server_busy_error") + end + + def net_smtp_unhandled_error + I18n.t("email_settings.smtp_unhandled_error", message: @exception.message) + end + + def socket_connection_error + I18n.t("email_settings.connection_error") + end + + def net_timeout_error + I18n.t("email_settings.timeout_error") + end + + def unhandled_error + I18n.t("email_settings.unhandled_error", message: @exception.message) + 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 + end +end diff --git a/app/services/email_settings_validator.rb b/app/services/email_settings_validator.rb index 190d8720e2e..238b5869627 100644 --- a/app/services/email_settings_validator.rb +++ b/app/services/email_settings_validator.rb @@ -12,52 +12,13 @@ require 'net/pop' # # or for specific host preset # EmailSettingsValidator.validate_imap(**{ username: "test@gmail.com", password: "test" }.merge(Email.gmail_imap_settings)) # -# rescue *EmailSettingsValidator::FRIENDLY_EXCEPTIONS => err -# EmailSettingsValidator.friendly_exception_message(err) +# rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS => err +# EmailSettingsExceptionHandler.friendly_exception_message(err, host) # end class EmailSettingsValidator - EXPECTED_EXCEPTIONS = [ - Net::POPAuthenticationError, - Net::IMAP::NoResponseError, - Net::SMTPAuthenticationError, - Net::SMTPServerBusy, - Net::SMTPSyntaxError, - Net::SMTPFatalError, - Net::SMTPUnknownError, - Net::OpenTimeout, - Net::ReadTimeout, - SocketError, - Errno::ECONNREFUSED - ] - - def self.friendly_exception_message(exception) - case exception - when Net::POPAuthenticationError - I18n.t("email_settings.pop3_authentication_error") - when Net::IMAP::NoResponseError - - # Most of IMAP's errors are lumped under the NoResponseError, including invalid - # credentials errors, because it is raised when a "NO" response is - # raised from the IMAP server https://datatracker.ietf.org/doc/html/rfc3501#section-7.1.2 - # - # Generally, it should be fairly safe to just return the error message as is. - if exception.message.match(/Invalid credentials/) - I18n.t("email_settings.imap_authentication_error") - else - I18n.t("email_settings.imap_no_response_error", message: exception.message.gsub(" (Failure)", "")) - end - when Net::SMTPAuthenticationError - I18n.t("email_settings.smtp_authentication_error") - when Net::SMTPServerBusy - I18n.t("email_settings.smtp_server_busy_error") - when Net::SMTPSyntaxError, Net::SMTPFatalError, Net::SMTPUnknownError - I18n.t("email_settings.smtp_unhandled_error", message: exception.message) - when SocketError, Errno::ECONNREFUSED - I18n.t("email_settings.connection_error") - when Net::OpenTimeout, Net::ReadTimeout - I18n.t("email_settings.timeout_error") - else - I18n.t("email_settings.unhandled_error", message: exception.message) + def self.validate_as_user(user, protocol, **kwargs) + DistributedMutex.synchronize("validate_#{protocol}_#{user.id}", validity: 10) do + self.public_send("validate_#{protocol}", **kwargs) end end @@ -74,7 +35,7 @@ class EmailSettingsValidator password:, ssl: SiteSetting.pop3_polling_ssl, openssl_verify: SiteSetting.pop3_polling_openssl_verify, - debug: false + debug: Rails.env.development? ) begin pop3 = Net::POP3.new(host, port) @@ -113,16 +74,20 @@ class EmailSettingsValidator def self.validate_smtp( host:, port:, - domain:, username:, password:, + domain: nil, authentication: GlobalSetting.smtp_authentication, enable_starttls_auto: GlobalSetting.smtp_enable_start_tls, enable_tls: GlobalSetting.smtp_force_tls, openssl_verify_mode: GlobalSetting.smtp_openssl_verify_mode, - debug: false + debug: Rails.env.development? ) begin + port, enable_tls, enable_starttls_auto = provider_specific_ssl_overrides( + host, port, enable_tls, enable_starttls_auto + ) + if enable_tls && enable_starttls_auto raise ArgumentError, "TLS and STARTTLS are mutually exclusive" end @@ -131,6 +96,17 @@ class EmailSettingsValidator raise ArgumentError, "Invalid authentication method. Must be plain, login, or cram_md5." end + if domain.blank? + if Rails.env.development? + domain = "localhost" + else + + # Because we are using the SMTP settings here to send emails, + # the domain should just be the TLD of the host. + domain = MiniSuffix.domain(host) + end + end + smtp = Net::SMTP.new(host, port) # These SSL options are cribbed from the Mail gem, which is used internally @@ -150,6 +126,9 @@ class EmailSettingsValidator smtp.enable_starttls_auto(ssl_context) if enable_starttls_auto smtp.enable_tls(ssl_context) if enable_tls + smtp.open_timeout = 5 + smtp.read_timeout = 5 + smtp.start(domain, username, password, authentication.to_sym) smtp.finish rescue => err @@ -168,7 +147,7 @@ class EmailSettingsValidator port:, username:, password:, - open_timeout: 10, + open_timeout: 5, ssl: true, debug: false ) @@ -188,4 +167,20 @@ class EmailSettingsValidator end raise err end + + def self.provider_specific_ssl_overrides(host, port, enable_tls, enable_starttls_auto) + # Gmail acts weirdly if you do not use the correct combinations of + # TLS settings based on the port, we clean these up here for the user. + if host == "smtp.gmail.com" + if port.to_i == 587 + enable_starttls_auto = true + enable_tls = false + elsif port.to_i == 465 + enable_starttls_auto = false + enable_tls = true + end + end + + [port, enable_tls, enable_starttls_auto] + end end diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 77ef4146025..2958cea2986 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -579,13 +579,7 @@ class PostAlerter def group_notifying_via_smtp(post) return nil if !SiteSetting.enable_smtp || post.post_type != Post.types[:regular] - - post.topic.allowed_groups - .where.not(smtp_server: nil) - .where.not(smtp_port: nil) - .where.not(email_username: nil) - .where.not(email_password: nil) - .first + post.topic.allowed_groups.where(smtp_enabled: true).first end def notify_pm_users(post, reply_to_user, notified) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d0975397ed7..6e7dd772d4b 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -692,6 +692,25 @@ en: email: title: "Email" status: "Synchronized %{old_emails} / %{total_emails} emails via IMAP." + enable_smtp: "Enable SMTP" + enable_imap: "Enable IMAP" + test_settings: "Test Settings" + save_settings: "Save Settings" + settings_required: "All settings are required, please fill in all fields before validation." + smtp_settings_valid: "SMTP settings valid." + smtp_title: "SMTP" + smtp_instructions: "When you enable SMTP for the group, all outbound emails sent from the group's inbox will be sent via the SMTP settings specified here instead of the mail server configured for other emails sent by your forum." + imap_title: "IMAP" + imap_additional_settings: "Additional Settings" + imap_instructions: "When you enable IMAP for the group, emails are synced between the group inbox and the provided IMAP server and mailbox. SMTP must be enabled with valid and tested credentials before IMAP can be enabled. The email username and password used for SMTP will be used for IMAP. For more information see feature announcement on Discourse Meta." + imap_alpha_warning: "Warning: This is an alpha-stage feature. Only Gmail is officially supported. Use at your own risk!" + imap_settings_valid: "IMAP settings valid." + smtp_disable_confirm: "If you disable SMTP, all SMTP and IMAP settings will be reset and the associated functionality will be disabled. Are you sure you want to continue?" + imap_disable_confirm: "If you disable IMAP all IMAP settings will be reset and the associated functionality will be disabled. Are you sure you want to continue?" + imap_mailbox_not_selected: "You must select a Mailbox for this IMAP configuration or no mailboxes will be synced!" + prefill: + title: "Prefill with settings for:" + gmail: "GMail" credentials: title: "Credentials" smtp_server: "SMTP Server" @@ -709,7 +728,7 @@ en: mailboxes: synchronized: "Synchronized Mailbox" none_found: "No mailboxes were found in this email account." - disabled: "disabled" + disabled: "Disabled" membership: title: Membership access: Access diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 84c2faa36eb..6db98692d13 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -994,8 +994,10 @@ en: 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_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}" connection_error: "There was an issue connecting with the server, check the server name and port and try again." timeout_error: "Connection to the server timed out, check the server name and port and try again." unhandled_error: "Unhandled error when testing email settings. %{message}" diff --git a/config/routes.rb b/config/routes.rb index 7a88938102c..88f8ad353d2 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -568,6 +568,7 @@ Discourse::Application.routes.draw do get 'mentionable' get 'messageable' get 'logs' => 'groups#histories' + post 'test_email_settings' collection do get "check-name" => 'groups#check_name' diff --git a/db/migrate/20210517061815_add_dedicated_enable_imap_smtp_columns_for_group.rb b/db/migrate/20210517061815_add_dedicated_enable_imap_smtp_columns_for_group.rb new file mode 100644 index 00000000000..4375ba648ce --- /dev/null +++ b/db/migrate/20210517061815_add_dedicated_enable_imap_smtp_columns_for_group.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +class AddDedicatedEnableImapSmtpColumnsForGroup < ActiveRecord::Migration[6.1] + def up + add_column :groups, :smtp_enabled, :boolean, default: false + add_column :groups, :smtp_updated_at, :datetime, null: true + add_column :groups, :smtp_updated_by_id, :integer, null: true + + add_column :groups, :imap_enabled, :boolean, default: false + add_column :groups, :imap_updated_at, :datetime, null: true + add_column :groups, :imap_updated_by_id, :integer, null: true + + DB.exec(<<~SQL) + UPDATE groups SET smtp_enabled = true, smtp_updated_at = NOW(), smtp_updated_by_id = -1 + WHERE smtp_port IS NOT NULL AND smtp_server IS NOT NULL AND email_username IS NOT NULL AND email_password IS NOT NULL AND + smtp_server != '' AND email_username != '' AND email_password != '' + SQL + + DB.exec(<<~SQL) + UPDATE groups SET imap_enabled = true, imap_updated_at = NOW(), imap_updated_by_id = -1 + WHERE imap_port IS NOT NULL AND imap_server IS NOT NULL AND email_username IS NOT NULL AND email_password IS NOT NULL AND + imap_server != '' AND email_username != '' AND email_password != '' + SQL + end + + def down + remove_column :groups, :smtp_enabled + remove_column :groups, :smtp_updated_at + remove_column :groups, :smtp_updated_by_id + + remove_column :groups, :imap_enabled + remove_column :groups, :imap_updated_at + remove_column :groups, :imap_updated_by_id + end +end diff --git a/lib/imap/providers/generic.rb b/lib/imap/providers/generic.rb index e331c32f6ce..ac18ca9f467 100644 --- a/lib/imap/providers/generic.rb +++ b/lib/imap/providers/generic.rb @@ -143,6 +143,11 @@ module Imap end def list_mailboxes(attr_filter = nil) + # Lists all the mailboxes but just returns the names. + list_mailboxes_with_attributes(attr_filter).map(&:name) + end + + def list_mailboxes_with_attributes(attr_filter = nil) # Basically, list all mailboxes in the root of the server. # ref: https://tools.ietf.org/html/rfc3501#section-6.3.8 imap.list('', '*').reject do |m| @@ -162,11 +167,15 @@ module Imap else true end - end.map do |m| - m.name end end + def filter_mailboxes(mailboxes) + # we do not want to filter out any mailboxes for generic providers, + # because we do not know what they are ahead of time + mailboxes + end + def archive(uid) # do nothing by default, just removing the Inbox label should be enough end diff --git a/lib/imap/providers/gmail.rb b/lib/imap/providers/gmail.rb index 8400cc53960..7ac51f73042 100644 --- a/lib/imap/providers/gmail.rb +++ b/lib/imap/providers/gmail.rb @@ -120,6 +120,15 @@ module Imap { trash_uid_validity: open_trash_mailbox, email_uids_to_trash: email_uids_to_trash } end + # Some mailboxes are just not useful or advisable to sync with. This is + # used for the dropdown in the UI where we allow the user to select the + # IMAP mailbox to sync with. + def filter_mailboxes(mailboxes_with_attributes) + mailboxes_with_attributes.reject do |mb| + (mb.attr & [:Drafts, :Sent, :Junk, :Flagged, :Trash]).any? + end.map(&:name) + end + private def apply_gmail_patch(imap) diff --git a/spec/fabricators/group_fabricator.rb b/spec/fabricators/group_fabricator.rb index 9f04918300b..da0ac73d7f4 100644 --- a/spec/fabricators/group_fabricator.rb +++ b/spec/fabricators/group_fabricator.rb @@ -13,12 +13,14 @@ Fabricator(:imap_group, from: :group) do smtp_server "smtp.ponyexpress.com" smtp_port 587 smtp_ssl true + smtp_enabled true imap_server "imap.ponyexpress.com" imap_port 993 imap_ssl true imap_mailbox_name "All Mail" imap_uid_validity 0 imap_last_uid 0 + imap_enabled true email_username "discourseteam@ponyexpress.com" email_password "test" end diff --git a/spec/lib/imap/providers/gmail_spec.rb b/spec/lib/imap/providers/gmail_spec.rb index dc6880e2a21..014cd1bc1ea 100644 --- a/spec/lib/imap/providers/gmail_spec.rb +++ b/spec/lib/imap/providers/gmail_spec.rb @@ -70,4 +70,23 @@ RSpec.describe Imap::Providers::Gmail do provider.archive(main_uid) end end + + describe "#filter_mailboxes" do + it "filters down the gmail mailboxes to only show the relevant ones" do + mailboxes_with_attr = [ + Net::IMAP::MailboxList.new([:Hasnochildren], "/", "INBOX"), + Net::IMAP::MailboxList.new([:All, :Hasnochildren], "/", "[Gmail]/All Mail"), + Net::IMAP::MailboxList.new([:Drafts, :Hasnochildren], "/", "[Gmail]/Drafts"), + Net::IMAP::MailboxList.new([:Hasnochildren, :Important], "/", "[Gmail]/Important"), + Net::IMAP::MailboxList.new([:Hasnochildren, :Sent], "/", "[Gmail]/Sent Mail"), + Net::IMAP::MailboxList.new([:Hasnochildren, :Junk], "/", "[Gmail]/Spam"), + Net::IMAP::MailboxList.new([:Flagged, :Hasnochildren], "/", "[Gmail]/Starred"), + Net::IMAP::MailboxList.new([:Hasnochildren, :Trash], "/", "[Gmail]/Trash") + ] + + expect(provider.filter_mailboxes(mailboxes_with_attr)).to match_array([ + "INBOX", "[Gmail]/All Mail", "[Gmail]/Important" + ]) + end + end end diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb index 4be00abf5d1..5858418a239 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -11,9 +11,11 @@ describe GroupSmtpMailer do smtp_server: 'smtp.gmail.com', smtp_port: 587, smtp_ssl: true, + smtp_enabled: true, imap_server: 'imap.gmail.com', imap_port: 993, imap_ssl: true, + imap_enabled: true, email_username: 'bugs@gmail.com', email_password: 'super$secret$password' ) @@ -96,9 +98,7 @@ describe GroupSmtpMailer do context "when IMAP is disabled for the group" do before do - group.update( - imap_server: nil - ) + group.update(imap_enabled: false) end it "uses the reply key based reply to address" do diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index b2d04acf3c1..07e7c982650 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -995,6 +995,7 @@ describe Group do imap_server: "imap.gmail.com", imap_port: 993, imap_ssl: true, + imap_enabled: true, email_username: "test@gmail.com", email_password: "testPassword1!" ) @@ -1003,6 +1004,7 @@ describe Group do def enable_imap SiteSetting.enable_imap = true @mocked_imap_provider.stubs(:connect!) + @mocked_imap_provider.stubs(:list_mailboxes_with_attributes).returns([stub(attr: [], name: "Inbox")]) @mocked_imap_provider.stubs(:list_mailboxes).returns(["Inbox"]) @mocked_imap_provider.stubs(:disconnect!) end @@ -1011,20 +1013,6 @@ describe Group do Discourse.redis.del("group_imap_mailboxes_#{group.id}") end - describe "#imap_enabled?" do - it "returns true if imap is configured and enabled for the site" do - mock_imap - configure_imap - enable_imap - expect(group.imap_enabled?).to eq(true) - end - - it "returns false if imap is configured and not enabled for the site" do - configure_imap - expect(group.imap_enabled?).to eq(false) - end - end - describe "#imap_mailboxes" do it "returns an empty array if group imap is not configured" do expect(group.imap_mailboxes).to eq([]) @@ -1194,4 +1182,97 @@ describe Group do expect(TagUser.lookup(user, :tracking).pluck(:tag_id)).to contain_exactly(tag1.id, tag2.id) end end + + describe "email setting changes" do + it "enables smtp and records the change" do + group.update( + smtp_port: 587, + smtp_ssl: true, + smtp_server: "smtp.gmail.com", + email_username: "test@gmail.com", + email_password: "password", + ) + + group.record_email_setting_changes!(user) + group.reload + + expect(group.smtp_enabled).to eq(true) + expect(group.smtp_updated_at).not_to eq(nil) + expect(group.smtp_updated_by).to eq(user) + end + + it "enables imap and records the change" do + group.update( + imap_port: 587, + imap_ssl: true, + imap_server: "imap.gmail.com", + email_username: "test@gmail.com", + email_password: "password", + ) + + group.record_email_setting_changes!(user) + group.reload + + expect(group.imap_enabled).to eq(true) + expect(group.imap_updated_at).not_to eq(nil) + expect(group.imap_updated_by).to eq(user) + end + + it "disables smtp and records the change" do + group.update( + smtp_port: 587, + smtp_ssl: true, + smtp_server: "smtp.gmail.com", + email_username: "test@gmail.com", + email_password: "password", + smtp_updated_by: user + ) + + group.record_email_setting_changes!(user) + group.reload + + group.update( + smtp_port: nil, + smtp_ssl: false, + smtp_server: nil, + email_username: nil, + email_password: nil, + ) + + group.record_email_setting_changes!(user) + group.reload + + expect(group.smtp_enabled).to eq(false) + expect(group.smtp_updated_at).not_to eq(nil) + expect(group.smtp_updated_by).to eq(user) + end + + it "disables imap and records the change" do + group.update( + imap_port: 587, + imap_ssl: true, + imap_server: "imap.gmail.com", + email_username: "test@gmail.com", + email_password: "password", + ) + + group.record_email_setting_changes!(user) + group.reload + + group.update( + imap_port: nil, + imap_ssl: false, + imap_server: nil, + email_username: nil, + email_password: nil + ) + + group.record_email_setting_changes!(user) + group.reload + + expect(group.imap_enabled).to eq(false) + expect(group.imap_updated_at).not_to eq(nil) + expect(group.imap_updated_by).to eq(user) + end + end end diff --git a/spec/requests/api/schemas/json/group_response.json b/spec/requests/api/schemas/json/group_response.json index b4bf0f15cc9..7ba352070db 100644 --- a/spec/requests/api/schemas/json/group_response.json +++ b/spec/requests/api/schemas/json/group_response.json @@ -140,6 +140,21 @@ "null" ] }, + "smtp_updated_at": { + "type": [ + "string", + "null" + ] + }, + "smtp_updated_by": { + "type": [ + "object", + "null" + ] + }, + "smtp_enabled": { + "type": "boolean" + }, "smtp_server": { "type": [ "string", @@ -158,6 +173,21 @@ "null" ] }, + "imap_enabled": { + "type": "boolean" + }, + "imap_updated_at": { + "type": [ + "string", + "null" + ] + }, + "imap_updated_by": { + "type": [ + "object", + "null" + ] + }, "imap_server": { "type": [ "string", diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index fd5adca9fe5..8a3d46868e4 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -1997,4 +1997,116 @@ describe GroupsController do ) end end + + describe "#test_email_settings" do + let(:params) do + { + protocol: protocol, + ssl: ssl, + port: port, + host: host, + username: username, + password: password + } + end + + before do + sign_in(user) + group.group_users.where(user: user).last.update(owner: user) + end + + context "validating smtp" do + let(:protocol) { "smtp" } + let(:username) { "test@gmail.com" } + let(:password) { "password" } + let(:domain) { nil } + let(:ssl) { true } + let(:host) { "smtp.somemailsite.com" } + let(:port) { 587 } + + context "when an error is raised" do + before do + EmailSettingsValidator.expects(:validate_smtp).raises(Net::SMTPAuthenticationError, "Invalid credentials") + end + it "uses the friendly error message functionality to return the message to the user" 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")) + end + end + end + + context "validating imap" do + let(:protocol) { "imap" } + let(:username) { "test@gmail.com" } + let(:password) { "password" } + let(:domain) { nil } + let(:ssl) { true } + let(:host) { "imap.somemailsite.com" } + let(:port) { 993 } + + it "validates with the correct TLS settings" do + EmailSettingsValidator.expects(:validate_imap).with( + has_entry(ssl: true) + ) + post "/groups/#{group.id}/test_email_settings.json", params: params + expect(response.status).to eq(200) + end + + context "when an error is raised" do + before do + EmailSettingsValidator.expects(:validate_imap).raises( + Net::IMAP::NoResponseError, stub(data: stub(text: "Invalid credentials")) + ) + end + it "uses the friendly error message functionality to return the message to the user" 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.imap_authentication_error")) + end + end + end + + describe "global param validation and rate limit" do + let(:protocol) { "smtp" } + let(:host) { "smtp.gmail.com" } + let(:port) { 587 } + let(:username) { "test@gmail.com" } + let(:password) { "password" } + let(:ssl) { true } + + context "when the protocol is not accepted" do + let(:protocol) { "sigma" } + it "raises an invalid params error" do + post "/groups/#{group.id}/test_email_settings.json", params: params + expect(response.status).to eq(400) + expect(response.parsed_body["errors"].first).to match(/Valid protocols to test are smtp and imap/) + end + end + + context "user does not have access to the group" do + before do + group.group_users.destroy_all + end + it "errors if the user does not have access to the group" do + post "/groups/#{group.id}/test_email_settings.json", params: params + + expect(response.status).to eq(403) + end + end + + context "rate limited" do + it "rate limits anon searches per user" do + RateLimiter.enable + RateLimiter.clear_all! + + 5.times do + post "/groups/#{group.id}/test_email_settings.json", params: params + end + post "/groups/#{group.id}/test_email_settings.json", params: params + expect(response.status).to eq(429) + end + end + end + end end diff --git a/spec/services/email_settings_exception_handler_spec.rb b/spec/services/email_settings_exception_handler_spec.rb new file mode 100644 index 00000000000..56873a76bb0 --- /dev/null +++ b/spec/services/email_settings_exception_handler_spec.rb @@ -0,0 +1,100 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe EmailSettingsExceptionHandler do + describe "#friendly_exception_message" do + it "formats a Net::POPAuthenticationError" do + exception = Net::POPAuthenticationError.new("invalid credentials") + expect(subject.class.friendly_exception_message(exception, "pop.test.com")).to eq( + I18n.t("email_settings.pop3_authentication_error") + ) + end + + it "formats a Net::IMAP::NoResponseError for invalid credentials" do + exception = Net::IMAP::NoResponseError.new(stub(data: stub(text: "Invalid credentials"))) + expect(subject.class.friendly_exception_message(exception, "imap.test.com")).to eq( + I18n.t("email_settings.imap_authentication_error") + ) + end + + it "formats a general Net::IMAP::NoResponseError" do + exception = Net::IMAP::NoResponseError.new(stub(data: stub(text: "NO bad problem (Failure)"))) + expect(subject.class.friendly_exception_message(exception, "imap.test.com")).to eq( + I18n.t("email_settings.imap_no_response_error", message: "NO bad problem") + ) + 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(subject.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(subject.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("Application-specific password required") + expect(subject.class.friendly_exception_message(exception, "smtp.gmail.com")).to eq( + I18n.t("email_settings.authentication_error_gmail_app_password") + ) + end + + it "formats a Net::SMTPServerBusy" do + exception = Net::SMTPServerBusy.new("call me maybe later") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.smtp_server_busy_error") + ) + end + + it "formats a Net::SMTPSyntaxError, Net::SMTPFatalError, and Net::SMTPUnknownError" do + exception = Net::SMTPSyntaxError.new("bad syntax") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.smtp_unhandled_error", message: exception.message) + ) + exception = Net::SMTPFatalError.new("fatal") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.smtp_unhandled_error", message: exception.message) + ) + exception = Net::SMTPUnknownError.new("unknown") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.smtp_unhandled_error", message: exception.message) + ) + end + + it "formats a SocketError and Errno::ECONNREFUSED" do + exception = SocketError.new("bad socket") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.connection_error") + ) + exception = Errno::ECONNREFUSED.new("no thanks") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.connection_error") + ) + end + + it "formats a Net::OpenTimeout and Net::ReadTimeout error" do + exception = Net::OpenTimeout.new("timed out") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.timeout_error") + ) + exception = Net::ReadTimeout.new("timed out") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.timeout_error") + ) + end + + it "formats unhandled errors" do + exception = StandardError.new("unknown") + expect(subject.class.friendly_exception_message(exception, "smtp.test.com")).to eq( + I18n.t("email_settings.unhandled_error", message: exception.message) + ) + end + end +end diff --git a/spec/services/email_settings_validator_spec.rb b/spec/services/email_settings_validator_spec.rb index fb688b758eb..1de20827692 100644 --- a/spec/services/email_settings_validator_spec.rb +++ b/spec/services/email_settings_validator_spec.rb @@ -6,87 +6,6 @@ RSpec.describe EmailSettingsValidator do let(:username) { "kwest@gmail.com" } let(:password) { "mbdtf" } - describe "#friendly_exception_message" do - it "formats a Net::POPAuthenticationError" do - exception = Net::POPAuthenticationError.new("invalid credentials") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.pop3_authentication_error") - ) - end - - it "formats a Net::IMAP::NoResponseError for invalid credentials" do - exception = Net::IMAP::NoResponseError.new(stub(data: stub(text: "Invalid credentials"))) - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.imap_authentication_error") - ) - end - - it "formats a general Net::IMAP::NoResponseError" do - exception = Net::IMAP::NoResponseError.new(stub(data: stub(text: "NO bad problem (Failure)"))) - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.imap_no_response_error", message: "NO bad problem") - ) - end - - it "formats a Net::SMTPAuthenticationError" do - exception = Net::SMTPAuthenticationError.new("invalid credentials") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.smtp_authentication_error") - ) - end - - it "formats a Net::SMTPServerBusy" do - exception = Net::SMTPServerBusy.new("call me maybe later") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.smtp_server_busy_error") - ) - end - - it "formats a Net::SMTPSyntaxError, Net::SMTPFatalError, and Net::SMTPUnknownError" do - exception = Net::SMTPSyntaxError.new("bad syntax") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.smtp_unhandled_error", message: exception.message) - ) - exception = Net::SMTPFatalError.new("fatal") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.smtp_unhandled_error", message: exception.message) - ) - exception = Net::SMTPUnknownError.new("unknown") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.smtp_unhandled_error", message: exception.message) - ) - end - - it "formats a SocketError and Errno::ECONNREFUSED" do - exception = SocketError.new("bad socket") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.connection_error") - ) - exception = Errno::ECONNREFUSED.new("no thanks") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.connection_error") - ) - end - - it "formats a Net::OpenTimeout and Net::ReadTimeout error" do - exception = Net::OpenTimeout.new("timed out") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.timeout_error") - ) - exception = Net::ReadTimeout.new("timed out") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.timeout_error") - ) - end - - it "formats unhandled errors" do - exception = StandardError.new("unknown") - expect(subject.class.friendly_exception_message(exception)).to eq( - I18n.t("email_settings.unhandled_error", message: exception.message) - ) - end - end - describe "#validate_imap" do let(:host) { "imap.gmail.com" } let(:port) { 993 } @@ -175,6 +94,8 @@ RSpec.describe EmailSettingsValidator do obj.stubs(:finish).returns(true) obj.stubs(:enable_tls).returns(true) obj.stubs(:enable_starttls_auto).returns(true) + obj.stubs(:open_timeout=) + obj.stubs(:read_timeout=) obj end @@ -199,12 +120,12 @@ RSpec.describe EmailSettingsValidator do it "uses the correct ssl verify params for enable_tls if those settings are enabled" do net_smtp_stub.expects(:enable_tls) - expect { subject.class.validate_smtp(host: host, port: port, username: username, password: password, domain: domain, openssl_verify_mode: "peer", enable_tls: true, enable_starttls_auto: false) }.not_to raise_error + expect { subject.class.validate_smtp(host: host, port: 465, username: username, password: password, domain: domain, openssl_verify_mode: "peer", enable_tls: true, enable_starttls_auto: false) }.not_to raise_error end it "uses the correct ssl verify params for enable_starttls_auto if those settings are enabled" do net_smtp_stub.expects(:enable_starttls_auto) - expect { subject.class.validate_smtp(host: host, port: port, username: username, password: password, domain: domain, openssl_verify_mode: "peer", enable_tls: false, enable_starttls_auto: true) }.not_to raise_error + expect { subject.class.validate_smtp(host: host, port: 587, username: username, password: password, domain: domain, openssl_verify_mode: "peer", enable_tls: false, enable_starttls_auto: true) }.not_to raise_error end it "raises an ArgumentError if both enable_tls is true and enable_starttls_auto is true" do @@ -214,5 +135,19 @@ RSpec.describe EmailSettingsValidator do it "raises an ArgumentError if a bad authentication method is used" do expect { subject.class.validate_smtp(host: host, port: port, username: username, password: password, domain: domain, authentication: :rubber_stamp) }.to raise_error(ArgumentError) end + + context "when the domain is not provided" do + let(:domain) { nil } + it "gets the domain from the host" do + net_smtp_stub.expects(:start).with("gmail.com", username, password, :plain) + subject.class.validate_smtp(host: host, port: port, username: username, password: password, enable_tls: true, enable_starttls_auto: false) + end + + it "uses localhost when in development mode" do + Rails.env.stubs(:development?).returns(true) + net_smtp_stub.expects(:start).with("localhost", username, password, :plain) + subject.class.validate_smtp(host: host, port: port, username: username, password: password, enable_tls: true, enable_starttls_auto: false) + end + end end end diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index a07ff600540..609100ad275 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1304,7 +1304,8 @@ describe PostAlerter do smtp_server: "imap.gmail.com", smtp_port: 587, email_username: "discourse@example.com", - email_password: "discourse@example.com" + email_password: "discourse@example.com", + smtp_enabled: true ) end