From 48d13cb231508ecaca02db1b9f66749bc31c70cf Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 18 Jul 2024 10:33:14 +1000 Subject: [PATCH] UX: Use a dropdown for SSL mode for group SMTP (#27932) Our old group SMTP SSL option was a checkbox, but this was not ideal because there are actually 3 different ways SSL can be used when sending SMTP: * None * SSL/TLS * STARTTLS We got around this before with specific overrides for Gmail, but it's not flexible enough and now people want to use other providers. It's best to be clear, though it is a technical detail. We provide a way to test the SMTP settings before saving them so there should be little chance of messing this up. This commit also converts GroupEmailSettings to a glimmer component. --- .../group-manage-email-settings.hbs | 1 + .../components/group-manage-email-settings.js | 5 + .../components/group-smtp-email-settings.gjs | 308 ++++++++++++++++++ .../components/group-smtp-email-settings.hbs | 139 -------- .../components/group-smtp-email-settings.js | 82 ----- .../discourse/app/lib/constants.js | 2 + .../lib/email-provider-default-settings.js | 24 +- .../javascripts/discourse/app/models/group.js | 2 +- .../group-manage-email-settings-test.js | 36 +- app/assets/stylesheets/common/base/group.scss | 23 +- app/controllers/groups_controller.rb | 31 +- app/mailers/group_smtp_mailer.rb | 3 +- app/models/group.rb | 10 +- app/serializers/group_show_serializer.rb | 2 +- app/services/email_settings_validator.rb | 6 - config/locales/client.en.yml | 8 +- ...40715073605_add_smtp_ssl_mode_to_groups.rb | 17 + .../20240717053710_drop_groups_smtp_ssl.rb | 13 + lib/smtp_provider_overrides.rb | 16 - lib/tasks/javascript.rake | 2 + .../spec/triggers/pm_created_spec.rb | 2 +- spec/fabricators/group_fabricator.rb | 4 +- spec/lib/email/receiver_spec.rb | 6 +- spec/mailers/group_smtp_mailer_spec.rb | 3 +- spec/models/group_spec.rb | 8 +- .../api/schemas/json/group_response.json | 6 +- spec/requests/groups_controller_spec.rb | 8 +- .../services/email_settings_validator_spec.rb | 24 -- spec/services/post_alerter_spec.rb | 2 +- 29 files changed, 470 insertions(+), 323 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/components/group-smtp-email-settings.gjs delete mode 100644 app/assets/javascripts/discourse/app/components/group-smtp-email-settings.hbs delete mode 100644 app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js create mode 100644 db/migrate/20240715073605_add_smtp_ssl_mode_to_groups.rb create mode 100644 db/post_migrate/20240717053710_drop_groups_smtp_ssl.rb diff --git a/app/assets/javascripts/discourse/app/components/group-manage-email-settings.hbs b/app/assets/javascripts/discourse/app/components/group-manage-email-settings.hbs index 38a3119d37a..0948b6e16d5 100644 --- a/app/assets/javascripts/discourse/app/components/group-manage-email-settings.hbs +++ b/app/assets/javascripts/discourse/app/components/group-manage-email-settings.hbs @@ -17,6 +17,7 @@ {{/if}} 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 index 8eaef3a7a03..38ef87ca910 100644 --- a/app/assets/javascripts/discourse/app/components/group-manage-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-manage-email-settings.js @@ -65,6 +65,11 @@ export default Component.extend({ ); }, + @action + onChangeSmtpSettingsValid(valid) { + this.set("smtpSettingsValid", valid); + }, + @action smtpEnabledChange(event) { if ( diff --git a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.gjs b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.gjs new file mode 100644 index 00000000000..193bf6eb8a4 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.gjs @@ -0,0 +1,308 @@ +import Component from "@glimmer/component"; +import { tracked } from "@glimmer/tracking"; +import { fn } from "@ember/helper"; +import { on } from "@ember/modifier"; +import { action } from "@ember/object"; +import { LinkTo } from "@ember/routing"; +import { inject as service } from "@ember/service"; +import { isEmpty } from "@ember/utils"; +import { TrackedObject } from "@ember-compat/tracked-built-ins"; +import { or } from "truth-helpers"; +import ConditionalLoadingSpinner from "discourse/components/conditional-loading-spinner"; +import DButton from "discourse/components/d-button"; +import formatDate from "discourse/helpers/format-date"; +import withEventValue from "discourse/helpers/with-event-value"; +import { ajax } from "discourse/lib/ajax"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import { GROUP_SMTP_SSL_MODES } from "discourse/lib/constants"; +import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings"; +import dIcon from "discourse-common/helpers/d-icon"; +import i18n from "discourse-common/helpers/i18n"; +import I18n from "I18n"; +import ComboBox from "select-kit/components/combo-box"; + +export default class GroupSmtpEmailSettings extends Component { + @service currentUser; + + @tracked smtpSettingsValid = false; + @tracked testingSettings = false; + + form = new TrackedObject({ + email_username: this.args.group.email_username, + email_password: this.args.group.email_password, + email_from_alias: this.args.group.email_from_alias, + smtp_server: this.args.group.smtp_server, + smtp_port: (this.args.group.smtp_port || "").toString(), + smtp_ssl_mode: this.args.group.smtp_ssl_mode || GROUP_SMTP_SSL_MODES.none, + }); + + get sslModes() { + return Object.keys(GROUP_SMTP_SSL_MODES).map((key) => { + return { + value: GROUP_SMTP_SSL_MODES[key], + name: I18n.t(`groups.manage.email.ssl_modes.${key}`), + }; + }); + } + + get missingSettings() { + if (!this.form) { + return true; + } + return [ + this.form.email_username, + this.form.email_password, + this.form.smtp_server, + this.form.smtp_port, + ].some((value) => isEmpty(value)); + } + + @action + changeSmtpSettingsValid(newValidValue) { + this.smtpSettingsValid = newValidValue; + this.args.onChangeSmtpSettingsValid(newValidValue); + } + + @action + onChangeSslMode(newMode) { + this.form.smtp_ssl_mode = newMode; + this.changeSmtpSettingsValid(false); + } + + @action + changeEmailUsername(newValue) { + this.form.email_username = newValue; + this.changeSmtpSettingsValid(false); + } + + @action + changeEmailPassword(newValue) { + this.form.email_password = newValue; + this.changeSmtpSettingsValid(false); + } + + @action + changeEmailFromAlias(newValue) { + this.form.email_from_alias = newValue; + this.changeSmtpSettingsValid(false); + } + + @action + changeSmtpServer(newValue) { + this.form.smtp_server = newValue; + this.changeSmtpSettingsValid(false); + } + + @action + changeSmtpPort(newValue) { + this.form.smtp_port = newValue; + this.changeSmtpSettingsValid(false); + } + + @action + prefillSettings(provider, event) { + event?.preventDefault(); + Object.assign(this.form, emailProviderDefaultSettings(provider, "smtp")); + } + + @action + testSmtpSettings() { + const settings = { + host: this.form.smtp_server, + port: this.form.smtp_port, + ssl_mode: this.form.smtp_ssl_mode, + username: this.form.email_username, + password: this.form.email_password, + }; + + this.testingSettings = true; + this.changeSmtpSettingsValid(false); + + return ajax(`/groups/${this.args.group.id}/test_email_settings`, { + type: "POST", + data: Object.assign(settings, { protocol: "smtp" }), + }) + .then(() => { + this.changeSmtpSettingsValid(true); + this.args.group.setProperties({ + smtp_server: this.form.smtp_server, + smtp_port: this.form.smtp_port, + smtp_ssl_mode: this.form.smtp_ssl_mode, + email_username: this.form.email_username, + email_from_alias: this.form.email_from_alias, + email_password: this.form.email_password, + }); + }) + .catch(popupAjaxError) + .finally(() => (this.testingSettings = false)); + } + + +} diff --git a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.hbs b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.hbs deleted file mode 100644 index a5f4ef7b9a7..00000000000 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.hbs +++ /dev/null @@ -1,139 +0,0 @@ -
-
-
-
- - -
- -
- - -
- - -
- -
-
- - -
- -
- - -
-
- -
-
- - -

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

-
-
-
- -
-
- {{i18n "groups.manage.email.prefill.title"}} - {{i18n "groups.manage.email.prefill.gmail"}} -
-
- -
- - - - - {{#if this.smtpSettingsValid}} - - {{d-icon "check-circle"}} - {{i18n "groups.manage.email.smtp_settings_valid"}} - - {{/if}} -
- - {{#if this.group.smtp_updated_at}} -
- - {{i18n "groups.manage.email.last_updated"}} - {{format-date - this.group.smtp_updated_at - leaveAgo="true" - }} - {{i18n "groups.manage.email.last_updated_by"}} - {{this.group.smtp_updated_by.username}} - -
- {{/if}} -
\ No newline at end of file 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 deleted file mode 100644 index 1f0ca0cfc66..00000000000 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js +++ /dev/null @@ -1,82 +0,0 @@ -import Component from "@ember/component"; -import EmberObject, { action } from "@ember/object"; -import { isEmpty } from "@ember/utils"; -import { ajax } from "discourse/lib/ajax"; -import { popupAjaxError } from "discourse/lib/ajax-error"; -import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings"; -import discourseComputed, { on } from "discourse-common/utils/decorators"; - -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, - email_from_alias: this.group.email_from_alias, - smtp_server: this.group.smtp_server, - smtp_port: (this.group.smtp_port || "").toString(), - smtp_ssl: this.group.smtp_ssl, - }) - ); - }, - - @action - prefillSettings(provider, event) { - event?.preventDefault(); - 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_from_alias: this.form.email_from_alias, - email_password: this.form.email_password, - }); - }) - .catch(popupAjaxError) - .finally(() => this.set("testingSettings", false)); - }, -}); diff --git a/app/assets/javascripts/discourse/app/lib/constants.js b/app/assets/javascripts/discourse/app/lib/constants.js index bbc9f9fae81..3dd3f53935f 100644 --- a/app/assets/javascripts/discourse/app/lib/constants.js +++ b/app/assets/javascripts/discourse/app/lib/constants.js @@ -69,6 +69,8 @@ export const AUTO_GROUPS = { }, }; +export const GROUP_SMTP_SSL_MODES = { none: 0, ssl_tls: 1, starttls: 2 }; + export const MAX_NOTIFICATIONS_LIMIT_PARAMS = 60; export const TOPIC_VISIBILITY_REASONS = { 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 index fa2939f6ace..1176623042b 100644 --- a/app/assets/javascripts/discourse/app/lib/email-provider-default-settings.js +++ b/app/assets/javascripts/discourse/app/lib/email-provider-default-settings.js @@ -1,3 +1,5 @@ +import { GROUP_SMTP_SSL_MODES } from "discourse/lib/constants"; + const GMAIL = { imap: { imap_server: "imap.gmail.com", @@ -7,7 +9,23 @@ const GMAIL = { smtp: { smtp_server: "smtp.gmail.com", smtp_port: "587", - smtp_ssl: true, + smtp_ssl_mode: GROUP_SMTP_SSL_MODES.starttls, + }, +}; + +const OUTLOOK = { + smtp: { + smtp_server: "smtp-mail.outlook.com", + smtp_port: "587", + smtp_ssl_mode: GROUP_SMTP_SSL_MODES.starttls, + }, +}; + +const OFFICE365 = { + smtp: { + smtp_server: "smtp.office365.com", + smtp_port: "587", + smtp_ssl_mode: GROUP_SMTP_SSL_MODES.starttls, }, }; @@ -18,5 +36,9 @@ export default function emailProviderDefaultSettings(provider, protocol) { switch (provider) { case "gmail": return GMAIL[protocol]; + case "office365": + return OFFICE365[protocol]; + case "outlook": + return OUTLOOK[protocol]; } } diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index e42de3e3585..2fc65d3b2b1 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -344,7 +344,7 @@ export default class Group extends RestModel { incoming_email: this.incoming_email, smtp_server: this.smtp_server, smtp_port: this.smtp_port, - smtp_ssl: this.smtp_ssl, + smtp_ssl_mode: this.smtp_ssl_mode, smtp_enabled: this.smtp_enabled, imap_server: this.imap_server, imap_port: this.imap_port, 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 index 48207de0850..9d8adb3fdd8 100644 --- 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 @@ -1,5 +1,6 @@ import { click, currentRouteName, fillIn, visit } from "@ember/test-helpers"; import { test } from "qunit"; +import { GROUP_SMTP_SSL_MODES } from "discourse/lib/constants"; import { acceptance, exists, @@ -85,22 +86,23 @@ acceptance( await click("#prefill_smtp_gmail"); assert.strictEqual( - query("input[name='smtp_server']").value, + query(".group-smtp-form__smtp-server").value, "smtp.gmail.com", "prefills SMTP server settings for gmail" ); assert.strictEqual( - query("input[name='smtp_port']").value, + query(".group-smtp-form__smtp-port").value, "587", "prefills SMTP port settings for gmail" ); - assert.ok( - exists("#enable_ssl:checked"), - "prefills SMTP ssl settings for gmail" + assert.strictEqual( + selectKit(".group-smtp-form__smtp-ssl-mode").header().value(), + GROUP_SMTP_SSL_MODES.starttls.toString(), + "prefills SSL mode to STARTTLS for gmail" ); assert.ok( - exists(".test-smtp-settings:disabled"), + exists(".group-smtp-form__test-smtp-settings:disabled"), "does not allow testing settings if not all fields are filled" ); @@ -108,9 +110,12 @@ acceptance( await fillIn('input[name="password"]', "password@gmail.com"); await fillIn("#from_alias", "akasomegroup@example.com"); - await click(".test-smtp-settings"); + await click(".group-smtp-form__test-smtp-settings"); - assert.ok(exists(".smtp-settings-ok"), "tested settings are ok"); + assert.ok( + exists(".group-smtp-form__smtp-settings-ok"), + "tested settings are ok" + ); await click(".group-manage-save"); @@ -141,7 +146,7 @@ acceptance( 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-smtp-form__test-smtp-settings"); await click(".group-manage-save"); assert.notOk( @@ -168,7 +173,7 @@ acceptance( "prefills IMAP port settings for gmail" ); assert.ok( - exists("#enable_ssl:checked"), + exists("#enable_ssl_imap:checked"), "prefills IMAP ssl settings for gmail" ); await click(".test-imap-settings"); @@ -239,7 +244,7 @@ acceptance( message_count: 2, smtp_server: "smtp.gmail.com", smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: GROUP_SMTP_SSL_MODES.starttls, smtp_enabled: true, smtp_updated_at: "2021-06-16T02:58:12.739Z", smtp_updated_by: { @@ -324,13 +329,6 @@ acceptance( ), "shows last updated imap details" ); - assert.ok(exists(".group-email-last-updated-details.for-smtp")); - assert.ok( - regex.test( - query(".group-email-last-updated-details.for-smtp").innerText.trim() - ), - "shows last updated smtp details" - ); }); } ); @@ -359,7 +357,7 @@ acceptance( 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-smtp-form__test-smtp-settings"); assert.strictEqual( query(".dialog-body").innerText.trim(), diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 2cffb6217fb..4a196868ee7 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -269,6 +269,25 @@ table.group-category-permissions { } } +.group-smtp-prefill-options { + ul { + display: inline; + margin: 0; + + li { + display: inline-block; + + &:before { + content: "|"; + } + + &:first-child:before { + content: ""; + } + } + } +} + .group-smtp-email-settings, .group-imap-email-settings { background-color: var(--primary-very-low); @@ -289,10 +308,6 @@ table.group-category-permissions { } } -.groups-form__enable-ssl { - margin-bottom: 1em; -} - .group-manage-email-additional-settings-wrapper { margin-top: 1em; } diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 01b0f4a2cee..b638b352088 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -715,7 +715,6 @@ class GroupsController < ApplicationController params.require(:host) params.require(:username) params.require(:password) - params.require(:ssl) group = Group.find(params[:group_id]) guardian.ensure_can_edit!(group) @@ -723,7 +722,6 @@ class GroupsController < ApplicationController 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 !%w[smtp imap].include?(params[:protocol]) @@ -734,13 +732,19 @@ class GroupsController < ApplicationController begin case params[:protocol] when "smtp" - enable_starttls_auto = false - settings.delete(:ssl) + raise Discourse::InvalidParameters if params[:ssl_mode].blank? + + settings.delete(:ssl_mode) + + if params[:ssl_mode].blank? || + !Group.smtp_ssl_modes.values.include?(params[:ssl_mode].to_i) + raise Discourse::InvalidParameters.new("SSL mode must be present and valid") + end final_settings = settings.merge( - enable_tls: enable_tls, - enable_starttls_auto: enable_starttls_auto, + enable_tls: params[:ssl_mode].to_i == Group.smtp_ssl_modes[:ssl_tls], + enable_starttls_auto: params[:ssl_mode].to_i == Group.smtp_ssl_modes[:starttls], ).permit(:host, :port, :username, :password, :enable_tls, :enable_starttls_auto, :debug) EmailSettingsValidator.validate_as_user( current_user, @@ -748,8 +752,17 @@ class GroupsController < ApplicationController **final_settings.to_h.symbolize_keys, ) when "imap" + raise Discourse::InvalidParameters if params[:ssl].blank? + final_settings = - settings.merge(ssl: enable_tls).permit(:host, :port, :username, :password, :ssl, :debug) + settings.merge(ssl: settings[:ssl] == "true").permit( + :host, + :port, + :username, + :password, + :ssl, + :debug, + ) EmailSettingsValidator.validate_as_user( current_user, "imap", @@ -812,7 +825,7 @@ class GroupsController < ApplicationController :incoming_email, :smtp_server, :smtp_port, - :smtp_ssl, + :smtp_ssl_mode, :smtp_enabled, :smtp_updated_by, :smtp_updated_at, @@ -890,7 +903,7 @@ class GroupsController < ApplicationController if should_clear_smtp attributes[:smtp_server] = nil - attributes[:smtp_ssl] = false + attributes[:smtp_ssl_mode] = false attributes[:smtp_port] = nil attributes[:email_username] = nil attributes[:email_password] = nil diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index 7b6cc26b44d..d1d7ef71762 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/app/mailers/group_smtp_mailer.rb @@ -18,7 +18,8 @@ class GroupSmtpMailer < ActionMailer::Base # NOTE: Might be better at some point to store this authentication method in the database # against the group. authentication: SmtpProviderOverrides.authentication_override(from_group.smtp_server), - enable_starttls_auto: from_group.smtp_ssl, + enable_starttls_auto: from_group.smtp_ssl_mode == Group.smtp_ssl_modes[:starttls], + enable_ssl: from_group.smtp_ssl_mode == Group.smtp_ssl_modes[:ssl_tls], return_response: true, open_timeout: GlobalSetting.group_smtp_open_timeout, read_timeout: GlobalSetting.group_smtp_read_timeout, diff --git a/app/models/group.rb b/app/models/group.rb index 445df74c5df..3c4b8ff78d0 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -3,7 +3,9 @@ require "net/imap" class Group < ActiveRecord::Base - self.ignored_columns = %w[flair_url] # TODO: Remove when 20240212034010_drop_deprecated_columns has been promoted to pre-deploy + # TODO: Remove flair_url when 20240212034010_drop_deprecated_columns has been promoted to pre-deploy + # TODO: Remove smtp_ssl when db/post_migrate/20240717053710_drop_groups_smtp_ssl has been promoted to pre-deploy + self.ignored_columns = %w[flair_url smtp_ssl] include HasCustomFields include AnonCacheInvalidator @@ -145,6 +147,10 @@ class Group < ActiveRecord::Base @visibility_levels = Enum.new(public: 0, logged_on_users: 1, members: 2, staff: 3, owners: 4) end + def self.smtp_ssl_modes + @visibility_levels = Enum.new(none: 0, ssl_tls: 1, starttls: 2) + end + def self.auto_groups_between(lower, upper) lower_group = Group::AUTO_GROUPS[lower.to_sym] upper_group = Group::AUTO_GROUPS[upper.to_sym] @@ -1292,7 +1298,6 @@ end # mentionable_level :integer default(0) # smtp_server :string # smtp_port :integer -# smtp_ssl :boolean # imap_server :string # imap_port :integer # imap_ssl :boolean @@ -1316,6 +1321,7 @@ end # imap_updated_at :datetime # imap_updated_by_id :integer # email_from_alias :string +# smtp_ssl_mode :integer default(0), not null # # Indexes # diff --git a/app/serializers/group_show_serializer.rb b/app/serializers/group_show_serializer.rb index f70c379a4a2..9b5015678ec 100644 --- a/app/serializers/group_show_serializer.rb +++ b/app/serializers/group_show_serializer.rb @@ -24,7 +24,7 @@ class GroupShowSerializer < BasicGroupSerializer admin_attributes :automatic_membership_email_domains, :smtp_server, :smtp_port, - :smtp_ssl, + :smtp_ssl_mode, :smtp_enabled, :smtp_updated_at, :smtp_updated_by, diff --git a/app/services/email_settings_validator.rb b/app/services/email_settings_validator.rb index f7227ce2a9c..0d2ee6899f3 100644 --- a/app/services/email_settings_validator.rb +++ b/app/services/email_settings_validator.rb @@ -62,9 +62,6 @@ class EmailSettingsValidator # Attempts to start an SMTP session and if that raises an error then it is # assumed the credentials or other settings are wrong. # - # For Gmail, the port should be 587, enable_starttls_auto should be true, - # and enable_tls should be false. - # # @param domain [String] - Used for HELO, should be the FQDN of the server sending the mail # localhost can be used in development mode. # See https://datatracker.ietf.org/doc/html/rfc788#section-4 @@ -83,9 +80,6 @@ class EmailSettingsValidator debug: Rails.env.development? ) begin - port, enable_tls, enable_starttls_auto = - SmtpProviderOverrides.ssl_override(host, port, enable_tls, enable_starttls_auto) - if enable_tls && enable_starttls_auto raise ArgumentError, "TLS and STARTTLS are mutually exclusive" end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index d1d4fe84480..ff700b26582 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -873,11 +873,17 @@ en: prefill: title: "Prefill with settings for:" gmail: "Gmail" + outlook: "Outlook.com" + office365: "Microsoft 365" + ssl_modes: + none: "None" + ssl_tls: "SSL/TLS" + starttls: "STARTTLS" credentials: title: "Credentials" smtp_server: "SMTP Server" smtp_port: "SMTP Port" - smtp_ssl: "Use SSL for SMTP" + smtp_ssl_mode: "SSL Mode" imap_server: "IMAP Server" imap_port: "IMAP Port" imap_ssl: "Use SSL for IMAP" diff --git a/db/migrate/20240715073605_add_smtp_ssl_mode_to_groups.rb b/db/migrate/20240715073605_add_smtp_ssl_mode_to_groups.rb new file mode 100644 index 00000000000..4de8b4b11a7 --- /dev/null +++ b/db/migrate/20240715073605_add_smtp_ssl_mode_to_groups.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true +class AddSmtpSslModeToGroups < ActiveRecord::Migration[7.1] + def up + add_column :groups, :smtp_ssl_mode, :integer, default: 0, null: false + + execute <<~SQL + UPDATE groups SET smtp_ssl_mode = (CASE WHEN smtp_ssl THEN 2 ELSE 0 END) + SQL + + Migration::ColumnDropper.mark_readonly(:groups, :smtp_ssl) + end + + def down + Migration::ColumnDropper.drop_readonly(:groups, :smtp_ssl) + remove_column :groups, :smtp_ssl_mode + end +end diff --git a/db/post_migrate/20240717053710_drop_groups_smtp_ssl.rb b/db/post_migrate/20240717053710_drop_groups_smtp_ssl.rb new file mode 100644 index 00000000000..3c4f2faf830 --- /dev/null +++ b/db/post_migrate/20240717053710_drop_groups_smtp_ssl.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class DropGroupsSmtpSsl < ActiveRecord::Migration[7.1] + DROPPED_COLUMNS ||= { groups: %i[smtp_ssl] } + + def up + DROPPED_COLUMNS.each { |table, columns| Migration::ColumnDropper.execute_drop(table, columns) } + end + + def down + raise ActiveRecord::IrreversibleMigration + end +end diff --git a/lib/smtp_provider_overrides.rb b/lib/smtp_provider_overrides.rb index 0263ba4e06f..6c33ee34ccd 100644 --- a/lib/smtp_provider_overrides.rb +++ b/lib/smtp_provider_overrides.rb @@ -9,20 +9,4 @@ class SmtpProviderOverrides return "login" if %w[smtp.office365.com smtp-mail.outlook.com].include?(host) GlobalSetting.smtp_authentication end - - def self.ssl_override(host, port, enable_tls, enable_starttls_auto) - # Certain mail servers act 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 %w[smtp.gmail.com smtp.office365.com smtp-mail.outlook.com].include?(host) - 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/lib/tasks/javascript.rake b/lib/tasks/javascript.rake index 0c23adb50a3..b06733c89f8 100644 --- a/lib/tasks/javascript.rake +++ b/lib/tasks/javascript.rake @@ -154,6 +154,8 @@ task "javascript:update_constants" => :environment do export const AUTO_GROUPS = #{auto_groups.to_json}; + export const GROUP_SMTP_SSL_MODES = #{Group.smtp_ssl_modes.to_json}; + export const MAX_NOTIFICATIONS_LIMIT_PARAMS = #{NotificationsController::INDEX_LIMIT}; export const TOPIC_VISIBILITY_REASONS = #{Topic.visibility_reasons.to_json}; diff --git a/plugins/automation/spec/triggers/pm_created_spec.rb b/plugins/automation/spec/triggers/pm_created_spec.rb index ec62517cd9c..5cd32144dec 100644 --- a/plugins/automation/spec/triggers/pm_created_spec.rb +++ b/plugins/automation/spec/triggers/pm_created_spec.rb @@ -164,7 +164,7 @@ describe "PMCreated" do incoming_email: "team@somesmtpaddress.com|suppor+team@bar.com", smtp_server: "smtp.test.com", smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], smtp_enabled: true, ) SiteSetting.email_in = true diff --git a/spec/fabricators/group_fabricator.rb b/spec/fabricators/group_fabricator.rb index c0a13a3f39d..bbc4d5df0b6 100644 --- a/spec/fabricators/group_fabricator.rb +++ b/spec/fabricators/group_fabricator.rb @@ -10,7 +10,7 @@ end Fabricator(:imap_group, from: :group) do smtp_server "smtp.ponyexpress.com" smtp_port 587 - smtp_ssl true + smtp_ssl_mode Group.smtp_ssl_modes[:starttls] smtp_enabled true imap_server "imap.ponyexpress.com" imap_port 993 @@ -26,7 +26,7 @@ end Fabricator(:smtp_group, from: :group) do smtp_server "smtp.ponyexpress.com" smtp_port 587 - smtp_ssl true + smtp_ssl_mode Group.smtp_ssl_modes[:starttls] smtp_enabled true email_username "discourseteam@ponyexpress.com" email_password "test" diff --git a/spec/lib/email/receiver_spec.rb b/spec/lib/email/receiver_spec.rb index aa8ed472fff..db28035f47a 100644 --- a/spec/lib/email/receiver_spec.rb +++ b/spec/lib/email/receiver_spec.rb @@ -740,7 +740,7 @@ RSpec.describe Email::Receiver do Please find some text file attached. [details="#{I18n.t("emails.incoming.attachments")}"] - + #{UploadMarkdown.new(upload).to_markdown} [/details] @@ -1223,7 +1223,7 @@ RSpec.describe Email::Receiver do incoming_email: "team@somesmtpaddress.com|support+team@bar.com", smtp_server: "smtp.test.com", smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], smtp_enabled: true, ) end @@ -1318,7 +1318,7 @@ RSpec.describe Email::Receiver do incoming_email: "team@somesmtpaddress.com|suppor+team@bar.com", smtp_server: "smtp.test.com", smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], smtp_enabled: true, ) process(:email_to_group_email_username_1) diff --git a/spec/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb index 5d5f1de2656..10ca7f1c6dd 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -10,7 +10,7 @@ RSpec.describe GroupSmtpMailer do full_name: "Testers Group", smtp_server: "smtp.gmail.com", smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], smtp_enabled: true, imap_server: "imap.gmail.com", imap_port: 993, @@ -128,6 +128,7 @@ RSpec.describe GroupSmtpMailer do password: "super$secret$password", authentication: GlobalSetting.smtp_authentication, enable_starttls_auto: true, + enable_ssl: false, return_response: true, open_timeout: GlobalSetting.group_smtp_open_timeout, read_timeout: GlobalSetting.group_smtp_read_timeout, diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb index 980879ff255..e467e7a894d 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1397,7 +1397,7 @@ RSpec.describe Group do it "enables smtp and records the change" do group.update( smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], smtp_server: "smtp.gmail.com", email_username: "test@gmail.com", email_password: "password", @@ -1414,7 +1414,7 @@ RSpec.describe Group do it "records the change for singular setting changes" do group.update( smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], smtp_server: "smtp.gmail.com", email_username: "test@gmail.com", email_password: "password", @@ -1448,7 +1448,7 @@ RSpec.describe Group do it "disables smtp and records the change" do group.update( smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], smtp_server: "smtp.gmail.com", email_username: "test@gmail.com", email_password: "password", @@ -1460,7 +1460,7 @@ RSpec.describe Group do group.update( smtp_port: nil, - smtp_ssl: false, + smtp_ssl_mode: Group.smtp_ssl_modes[:none], smtp_server: nil, email_username: nil, email_password: nil, diff --git a/spec/requests/api/schemas/json/group_response.json b/spec/requests/api/schemas/json/group_response.json index 9163e33f9dc..61cf8910c39 100644 --- a/spec/requests/api/schemas/json/group_response.json +++ b/spec/requests/api/schemas/json/group_response.json @@ -170,9 +170,9 @@ "null" ] }, - "smtp_ssl": { + "smtp_ssl_mode": { "type": [ - "string", + "integer", "null" ] }, @@ -363,7 +363,7 @@ "automatic_membership_email_domains", "smtp_server", "smtp_port", - "smtp_ssl", + "smtp_ssl_mode", "imap_server", "imap_port", "imap_ssl", diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 6d042a7f502..ad3b71fd2be 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -2766,6 +2766,7 @@ RSpec.describe GroupsController do let(:params) do { protocol: protocol, + ssl_mode: ssl_mode, ssl: ssl, port: port, host: host, @@ -2784,7 +2785,8 @@ RSpec.describe GroupsController do let(:username) { "test@gmail.com" } let(:password) { "password" } let(:domain) { nil } - let(:ssl) { true } + let(:ssl_mode) { Group.smtp_ssl_modes[:starttls] } + let(:ssl) { nil } let(:host) { "smtp.somemailsite.com" } let(:port) { 587 } @@ -2811,11 +2813,12 @@ RSpec.describe GroupsController do let(:password) { "password" } let(:domain) { nil } let(:ssl) { true } + let(:ssl_mode) { nil } 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)) + EmailSettingsValidator.expects(:validate_imap).with(has_entries(ssl: true)) post "/groups/#{group.id}/test_email_settings.json", params: params expect(response.status).to eq(200) end @@ -2844,6 +2847,7 @@ RSpec.describe GroupsController do let(:username) { "test@gmail.com" } let(:password) { "password" } let(:ssl) { true } + let(:ssl_mode) { nil } context "when the protocol is not accepted" do let(:protocol) { "sigma" } diff --git a/spec/services/email_settings_validator_spec.rb b/spec/services/email_settings_validator_spec.rb index ca41c0352d4..e7387889f6e 100644 --- a/spec/services/email_settings_validator_spec.rb +++ b/spec/services/email_settings_validator_spec.rb @@ -275,30 +275,6 @@ RSpec.describe EmailSettingsValidator do }.to raise_error(ArgumentError) end - it "corrects tls settings for gmail based on port 587" do - net_smtp_stub.expects(:enable_starttls_auto).once - net_smtp_stub.expects(:enable_tls).never - described_class.validate_smtp( - host: host, - port: 587, - username: username, - password: password, - domain: domain, - ) - end - - it "corrects tls settings for gmail based on port 465" do - net_smtp_stub.expects(:enable_starttls_auto).never - net_smtp_stub.expects(:enable_tls).once - described_class.validate_smtp( - host: host, - port: 465, - username: username, - password: password, - domain: domain, - ) - end - it "corrects authentication method to login for office365" do net_smtp_stub.expects(:start).with("office365.com", username, password, :login) described_class.validate_smtp( diff --git a/spec/services/post_alerter_spec.rb b/spec/services/post_alerter_spec.rb index 2667926580e..d858813d9f7 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -2408,7 +2408,7 @@ RSpec.describe PostAlerter do :group, smtp_server: "smtp.gmail.com", smtp_port: 587, - smtp_ssl: true, + smtp_ssl_mode: Group.smtp_ssl_modes[:starttls], imap_server: "imap.gmail.com", imap_port: 993, imap_ssl: true,