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.
This commit is contained in:
Martin Brennan 2024-07-18 10:33:14 +10:00 committed by GitHub
parent 9a0e8fc100
commit 48d13cb231
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
29 changed files with 470 additions and 323 deletions

View File

@ -17,6 +17,7 @@
<GroupSmtpEmailSettings
@group={{this.group}}
@smtpSettingsValid={{this.smtpSettingsValid}}
@onChangeSmtpSettingsValid={{this.onChangeSmtpSettingsValid}}
/>
{{/if}}

View File

@ -65,6 +65,11 @@ export default Component.extend({
);
},
@action
onChangeSmtpSettingsValid(valid) {
this.set("smtpSettingsValid", valid);
},
@action
smtpEnabledChange(event) {
if (

View File

@ -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));
}
<template>
<div class="group-smtp-email-settings">
<form class="groups-form form-horizontal group-smtp-form">
<div>
<div class="control-group">
<label for="username">{{i18n
"groups.manage.email.credentials.username"
}}</label>
<input
type="text"
name="username"
class="group-smtp-form__smtp-username"
value={{this.form.email_username}}
tabindex="1"
{{on "input" (withEventValue this.changeEmailUsername)}}
/>
</div>
<div class="control-group">
<label for="smtp_server">{{i18n
"groups.manage.email.credentials.smtp_server"
}}</label>
<input
type="text"
name="smtp_server"
class="group-smtp-form__smtp-server"
value={{this.form.smtp_server}}
tabindex="4"
{{on "input" (withEventValue this.changeSmtpServer)}}
/>
</div>
<div class="control-group">
<label for="smtp_ssl_mode">
{{i18n "groups.manage.email.credentials.smtp_ssl_mode"}}
</label>
<ComboBox
@content={{this.sslModes}}
@valueProperty="value"
@value={{this.form.smtp_ssl_mode}}
name="smtp_ssl_mode"
class="group-smtp-form__smtp-ssl-mode"
tabindex="6"
@onChange={{this.onChangeSslMode}}
/>
</div>
</div>
<div>
<div class="control-group">
<label for="password">{{i18n
"groups.manage.email.credentials.password"
}}</label>
<input
type="password"
name="password"
class="group-smtp-form__smtp-password"
value={{this.form.email_password}}
tabindex="2"
{{on "input" (withEventValue this.changeEmailPassword)}}
/>
</div>
<div class="control-group">
<label for="smtp_port">{{i18n
"groups.manage.email.credentials.smtp_port"
}}</label>
<input
type="text"
name="smtp_port"
class="group-smtp-form__smtp-port"
value={{this.form.smtp_port}}
tabindex="5"
{{on "input" (withEventValue this.changeSmtpPort)}}
/>
</div>
</div>
<div>
<div class="control-group">
<label for="from_alias">{{i18n
"groups.manage.email.settings.from_alias"
}}</label>
<input
type="text"
name="from_alias"
class="group-smtp-form__smtp-from-alias"
id="from_alias"
value={{this.form.email_from_alias}}
{{on "input" (withEventValue this.changeEmailFromAlias)}}
tabindex="3"
/>
<p>{{i18n "groups.manage.email.settings.from_alias_hint"}}</p>
</div>
</div>
</form>
<div class="control-group">
<div class="group-smtp-prefill-options">
{{i18n "groups.manage.email.prefill.title"}}
<ul>
<li>
<a
id="prefill_smtp_gmail"
href
{{on "click" (fn this.prefillSettings "gmail")}}
>{{i18n "groups.manage.email.prefill.gmail"}}</a>
</li>
<li>
<a
id="prefill_smtp_outlook"
href
{{on "click" (fn this.prefillSettings "outlook")}}
>{{i18n "groups.manage.email.prefill.outlook"}}</a>
</li>
<li>
<a
id="prefill_smtp_office365"
href
{{on "click" (fn this.prefillSettings "office365")}}
>{{i18n "groups.manage.email.prefill.office365"}}</a>
</li>
</ul>
</div>
</div>
<div class="control-group buttons">
<DButton
@disabled={{or this.missingSettings this.testingSettings}}
@action={{this.testSmtpSettings}}
@icon="cog"
@label="groups.manage.email.test_settings"
@title="groups.manage.email.settings_required"
tabindex="7"
class="btn-primary group-smtp-form__test-smtp-settings"
/>
<ConditionalLoadingSpinner
@size="small"
@condition={{this.testingSettings}}
/>
{{#if @smtpSettingsValid}}
<span class="group-smtp-form__smtp-settings-ok">
{{dIcon "check-circle"}}
{{i18n "groups.manage.email.smtp_settings_valid"}}
</span>
{{/if}}
</div>
{{#if @group.smtp_updated_at}}
<div class=".group-smtp-form__last-updated-details">
<small>
{{i18n "groups.manage.email.last_updated"}}
<strong>{{formatDate
@group.smtp_updated_at
leaveAgo="true"
}}</strong>
{{i18n "groups.manage.email.last_updated_by"}}
<LinkTo
@route="user"
@model={{@group.smtp_updated_by.username}}
>{{@group.smtp_updated_by.username}}</LinkTo>
</small>
</div>
{{/if}}
</div>
</template>
}

View File

@ -1,139 +0,0 @@
<div class="group-smtp-email-settings">
<form class="groups-form form-horizontal">
<div>
<div class="control-group">
<label for="username">{{i18n
"groups.manage.email.credentials.username"
}}</label>
<Input
@type="text"
name="username"
@value={{this.form.email_username}}
tabindex="1"
{{on "change" (action "resetSettingsValid")}}
/>
</div>
<div class="control-group">
<label for="smtp_server">{{i18n
"groups.manage.email.credentials.smtp_server"
}}</label>
<Input
@type="text"
name="smtp_server"
@value={{this.form.smtp_server}}
tabindex="4"
{{on "change" (action "resetSettingsValid")}}
/>
</div>
<label for="enable_ssl" class="groups-form__enable-ssl">
<Input
@type="checkbox"
@checked={{this.form.smtp_ssl}}
id="enable_ssl"
tabindex="6"
{{on "change" (action "resetSettingsValid")}}
/>
{{i18n "groups.manage.email.credentials.smtp_ssl"}}
</label>
</div>
<div>
<div class="control-group">
<label for="password">{{i18n
"groups.manage.email.credentials.password"
}}</label>
<Input
@type="password"
name="password"
@value={{this.form.email_password}}
tabindex="2"
{{on "change" (action "resetSettingsValid")}}
/>
</div>
<div class="control-group">
<label for="smtp_port">{{i18n
"groups.manage.email.credentials.smtp_port"
}}</label>
<Input
@type="text"
name="smtp_port"
@value={{this.form.smtp_port}}
tabindex="5"
{{on "change" (action "resetSettingsValid" this.form.smtp_port)}}
/>
</div>
</div>
<div>
<div class="control-group">
<label for="from_alias">{{i18n
"groups.manage.email.settings.from_alias"
}}</label>
<Input
@type="text"
name="from_alias"
id="from_alias"
@value={{this.form.email_from_alias}}
{{on "change" (action "resetSettingsValid")}}
tabindex="3"
/>
<p>{{i18n "groups.manage.email.settings.from_alias_hint"}}</p>
</div>
</div>
</form>
<div class="control-group">
<div class="group-smtp-prefill-options">
{{i18n "groups.manage.email.prefill.title"}}
<a
id="prefill_smtp_gmail"
href
{{on "click" (fn this.prefillSettings "gmail")}}
>{{i18n "groups.manage.email.prefill.gmail"}}</a>
</div>
</div>
<div class="control-group buttons">
<DButton
@disabled={{or this.missingSettings this.testingSettings}}
@action={{this.testSmtpSettings}}
@icon="cog"
@label="groups.manage.email.test_settings"
@title="groups.manage.email.settings_required"
tabindex="7"
class="btn-primary test-smtp-settings"
/>
<ConditionalLoadingSpinner
@size="small"
@condition={{this.testingSettings}}
/>
{{#if this.smtpSettingsValid}}
<span class="smtp-settings-ok">
{{d-icon "check-circle"}}
{{i18n "groups.manage.email.smtp_settings_valid"}}
</span>
{{/if}}
</div>
{{#if this.group.smtp_updated_at}}
<div class="group-email-last-updated-details for-smtp">
<small>
{{i18n "groups.manage.email.last_updated"}}
<strong>{{format-date
this.group.smtp_updated_at
leaveAgo="true"
}}</strong>
{{i18n "groups.manage.email.last_updated_by"}}
<LinkTo
@route="user"
@model={{this.group.smtp_updated_by.username}}
>{{this.group.smtp_updated_by.username}}</LinkTo>
</small>
</div>
{{/if}}
</div>

View File

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

View File

@ -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 = {

View File

@ -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];
}
}

View File

@ -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,

View File

@ -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(),

View File

@ -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;
}

View File

@ -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

View File

@ -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,

View File

@ -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
#

View File

@ -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,

View File

@ -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

View File

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

View File

@ -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

View File

@ -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

View File

@ -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

View File

@ -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};

View File

@ -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

View File

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

View File

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

View File

@ -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,

View File

@ -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,

View File

@ -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",

View File

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

View File

@ -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(

View File

@ -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,