From 5827a5d5e0e474c1c63ab896a8728f66fffa4925 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 14 May 2021 06:38:21 +1000 Subject: [PATCH 01/21] WIP commit --- .../components/group-manage-email-settings.js | 9 ++++ .../components/group-smtp-email-settings.js | 44 +++++++++++++++++ .../app/routes/group-manage-email.js | 2 +- .../group-manage-email-settings.hbs | 13 +++++ .../components/group-smtp-email-settings.hbs | 47 +++++++++++++++++++ .../app/templates/group/manage/email.hbs | 2 + app/assets/stylesheets/common/base/group.scss | 13 +++++ config/locales/client.en.yml | 8 ++++ 8 files changed, 137 insertions(+), 1 deletion(-) 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/templates/components/group-manage-email-settings.hbs create mode 100644 app/assets/javascripts/discourse/app/templates/components/group-smtp-email-settings.hbs 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 00000000000000..b99e7f02c8db8d --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/group-manage-email-settings.js @@ -0,0 +1,9 @@ +import Component from "@ember/component"; + +export default Component.extend({ + tagName: "", + + group: null, + + isSmtpEnabled: true, +}); 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 00000000000000..c9714c2b66e04f --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -0,0 +1,44 @@ +import Component from "@ember/component"; +import { on } from "discourse-common/utils/decorators"; +import { action } from "@ember/object"; + +export default Component.extend({ + tagName: "", + group: null, + + form: null, + smtpSettingsNotOk: true, + + @on("init") + _fillForm() { + this.set("form", { + username: this.group.email_username, + password: this.group.email_password, + smtp_server: this.group.smtp_server, + smtp_port: this.group.smtp_port, + smtp_ssl: this.group.smtp_ssl, + }); + }, + + @action + prefillSettings(provider) { + let providerDetails = null; + switch (provider) { + case "gmail": + providerDetails = { + server: "smtp.gmail.com", + port: "587", + ssl: true, + }; + } + + if (providerDetails) { + this.set("form.smtp_server", providerDetails.server); + this.set("form.smtp_port", providerDetails.port); + this.set("form.smtp_ssl", providerDetails.ssl); + } + }, + + @action + testSmtpSettings() {}, +}); 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 1ba2f5d5b5142a..f85a645311ac6e 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,7 @@ export default DiscourseRoute.extend({ showFooter: true, beforeModel() { - if (!this.siteSettings.enable_imap && !this.siteSettings.enable_smtp) { + if (!this.siteSettings.enable_imap || !this.siteSettings.enable_smtp) { return this.transitionTo("group.manage.profile"); } }, 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 00000000000000..78f5ba85bef2e2 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs @@ -0,0 +1,13 @@ +
+

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

+

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

+ + + + {{#if isSmtpEnabled}} + {{group-smtp-email-settings group=group}} + {{/if}} +
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 00000000000000..5438b80ad64672 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/components/group-smtp-email-settings.hbs @@ -0,0 +1,47 @@ +
+
+
+
+ + {{input type="text" name="username" value=form.email_username}} +
+ +
+ + {{input type="text" name="smtp_server" value=form.smtp_server}} +
+ + +
+ +
+
+ + {{input type="password" name="password" value=form.email_password}} +
+ +
+ + {{input type="text" name="smtp_port" value=form.smtp_port}} +
+
+
+ +
+
+ {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}} +
+
+ +
+ {{d-button + class="btn-primary" + action=(action "testSmtpSettings") + icon="cog" + label="groups.manage.email.test_settings" + }} +
+
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 6ef5aada78984d..912cb40dec5f57 100644 --- a/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs +++ b/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs @@ -1,3 +1,5 @@ +{{group-manage-email-settings group=model}} +
{{groups-form-email-fields model=model}} {{group-manage-save-button model=model}} diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index a8c0577ed2c5bb..6a0448d8dab28f 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -241,3 +241,16 @@ table.group-category-permissions { } } } + +.group-smtp-email-settings { + .groups-form { + display: grid; + grid-template-columns: 1fr 3fr; + margin-bottom: 0; + } + + background-color: var(--primary-very-low); + padding: 1em; + margin-top: 1em; + border: 1px solid var(--primary-low); +} diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 5308b7038b4230..64a2c8e6138ca0 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -687,6 +687,14 @@ 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" + 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." + prefill: + title: "Prefill with settings for:" + gmail: "GMail" credentials: title: "Credentials" smtp_server: "SMTP Server" From 9cd0d846877e3a5786765133faa0173ab1f0efe6 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 14 May 2021 12:31:47 +1000 Subject: [PATCH 02/21] WIP commit for group controller --- app/controllers/groups_controller.rb | 23 ++++++++++++- app/services/email_settings_validator.rb | 13 ++++++- config/routes.rb | 1 + spec/requests/groups_controller_spec.rb | 43 ++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 2 deletions(-) diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 9dc1c43a40154a..00c5b3cf1dd1c8 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] @@ -570,6 +571,26 @@ def permissions 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(:settings) + + group = Group.find(params[:group_id]) + guardian.ensure_can_edit!(group) + + RateLimiter.new(current_user, "group_test_email_settings", 5, 1.minute).performed! + + case params[:protocol] + when "smtp" + EmailSettingsValidator.validate_imap(**params[:settings]) + when "imap" + EmailSettingsValidator.validate_imap(**params[:settings]) + else + raise Discourse::InvalidParameters.new("Valid protocols to test are smtp and imap") + end + end + private def group_params(automatic: false) diff --git a/app/services/email_settings_validator.rb b/app/services/email_settings_validator.rb index 190d8720e2e2fd..97639828a04063 100644 --- a/app/services/email_settings_validator.rb +++ b/app/services/email_settings_validator.rb @@ -113,9 +113,9 @@ def self.validate_pop3( 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, @@ -131,6 +131,17 @@ def self.validate_smtp( 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 diff --git a/config/routes.rb b/config/routes.rb index 159c6e794dc39c..8a8c3634c10528 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -568,6 +568,7 @@ get 'mentionable' get 'messageable' get 'logs' => 'groups#histories' + post 'test_email_settings' collection do get "check-name" => 'groups#check_name' diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 1f35f3dad2ef2b..e230f017cc9f51 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -2002,4 +2002,47 @@ def expect_type_to_return_right_groups(type, expected_group_ids) ) end end + + describe "#test_email_settings" do + let(:protocol) { "smtp" } + let(:settings) do + { + ssl: true, + port: 587, + host: "smtp.gmail.com", + username: "test@gmail.com", + password: "password" + } + end + let(:params) { { settings: settings, protocol: protocol } } + + before do + sign_in(user) + group.group_users.where(user: user).last.update(owner: user) + 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 From cd79a7961af47e7d83cdbf615bce672b96270a18 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 18 May 2021 15:38:09 +1000 Subject: [PATCH 03/21] More progress. Group SMTP settings interface now done. --- .../components/group-manage-email-settings.js | 68 ++++++++- .../components/group-manage-save-button.js | 10 ++ .../components/group-smtp-email-settings.js | 47 ++++++- .../javascripts/discourse/app/models/group.js | 26 ++++ .../group-manage-email-settings.hbs | 7 +- .../components/group-manage-save-button.hbs | 2 +- .../components/group-smtp-email-settings.hbs | 17 ++- app/controllers/groups_controller.rb | 57 +++++++- app/mailers/group_smtp_mailer.rb | 2 +- app/models/group.rb | 40 +++++- app/serializers/group_show_serializer.rb | 9 ++ app/services/email_settings_validator.rb | 19 ++- app/services/post_alerter.rb | 8 +- config/locales/client.en.yml | 5 + config/locales/server.en.yml | 1 + ...ated_enable_imap_smtp_columns_for_group.rb | 35 +++++ spec/fabricators/group_fabricator.rb | 2 + spec/mailers/group_smtp_mailer_spec.rb | 6 +- spec/models/group_spec.rb | 107 ++++++++++++-- spec/requests/groups_controller_spec.rb | 131 +++++++++++++++--- .../services/email_settings_validator_spec.rb | 14 ++ spec/services/post_alerter_spec.rb | 3 +- 22 files changed, 544 insertions(+), 72 deletions(-) create mode 100644 db/migrate/20210517061815_add_dedicated_enable_imap_smtp_columns_for_group.rb 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 b99e7f02c8db8d..c35c66138da649 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 @@ -1,9 +1,75 @@ import Component from "@ember/component"; +import I18n from "I18n"; +import bootbox from "bootbox"; +import { action } from "@ember/object"; export default Component.extend({ tagName: "", group: null, - isSmtpEnabled: true, + @action + smtpEnabledChange(event) { + if (!event.target.checked && this.group.smtp_enabled) { + bootbox.confirm( + I18n.t("groups.manage.email.smtp_disable_confirm"), + (result) => { + this.clearAllEmailSettingsOnSave = result; + + if (!result) { + this.group.set("smtp_enabled", true); + } + } + ); + } + + if (event.target.checked) { + this.clearAllEmailSettingsOnSave = false; + } + + this.group.set("smtp_enabled", event.target.checked); + }, + + @action + imapEnabledChange(event) { + if (!event.target.checked && this.group.imap_enabled) { + bootbox.confirm( + I18n.t("groups.manage.email.imap_disable_confirm"), + (result) => { + this.clearImapEmailSettingsOnSave = result; + + if (!result) { + this.group.set("imap_enabled", true); + } + } + ); + } + + if (event.target.checked) { + this.clearImapEmailSettingsOnSave = false; + } + + this.group.set("imap_enabled", event.target.checked); + }, + + @action + beforeSave() { + if (this.clearAllEmailSettingsOnSave) { + this.group.setProperties({ + smtp_port: null, + smtp_server: null, + email_username: null, + email_password: null, + imap_server: null, + imap_port: null, + }); + } + + if (this.clearImapEmailSettingsOnSave) { + this.group.setProperties({ + imap_server: null, + imap_port: null, + }); + } + }, }); 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 0a92d4bd843b0d..fcfa0604f5c8f1 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,14 +7,24 @@ import { popupAutomaticMembershipAlert } from "discourse/controllers/groups-new" export default Component.extend({ saving: null, + disabled: false, @discourseComputed("saving") savingText(saving) { return saving ? I18n.t("saving") : I18n.t("save"); }, + @discourseComputed("saving", "disabled") + isDisabled(saving, disabled) { + return saving || disabled; + }, + actions: { save() { + if (this.beforeSave) { + this.beforeSave(); + } + this.set("saving", true); const group = this.model; 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 index c9714c2b66e04f..7a5c172a64a74f 100644 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -1,19 +1,22 @@ import Component from "@ember/component"; +import I18n from "I18n"; +import bootbox from "bootbox"; +import { isEmpty } from "@ember/utils"; +import { popupAjaxError } from "discourse/lib/ajax-error"; import { on } from "discourse-common/utils/decorators"; import { action } from "@ember/object"; +import { ajax } from "discourse/lib/ajax"; export default Component.extend({ tagName: "", group: null, - form: null, - smtpSettingsNotOk: true, @on("init") _fillForm() { this.set("form", { - username: this.group.email_username, - password: this.group.email_password, + email_username: this.group.email_username, + email_password: this.group.email_password, smtp_server: this.group.smtp_server, smtp_port: this.group.smtp_port, smtp_ssl: this.group.smtp_ssl, @@ -40,5 +43,39 @@ export default Component.extend({ }, @action - testSmtpSettings() {}, + testSmtpSettings() { + let 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, + }; + + for (const setting in settings) { + if (isEmpty(settings[setting])) { + return bootbox.alert(I18n.t("groups.manage.email.settings_required")); + } + } + + this.set("testingSettings", true); + this.set("group.smtpSettingsValid", false); + + return ajax(`/groups/${this.group.id}/test_email_settings`, { + type: "POST", + data: Object.assign(settings, { protocol: "smtp" }), + }) + .then(() => { + this.set("group.smtpSettingsValid", true); + this.setProperties({ + "group.smtp_server": this.form.smtp_server, + "group.smtp_port": this.form.smtp_port, + "group.smtp_ssl": this.form.smtp_ssl, + "group.email_username": this.form.email_username, + "group.email_password": this.form.email_password, + }); + }) + .catch(popupAjaxError) + .finally(() => this.set("testingSettings", false)); + }, }); diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index 429dcd05953e71..55edb157ee6bca 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -34,6 +34,32 @@ const Group = RestModel.extend({ return automatic ? "automatic" : "custom"; }, + @discourseComputed( + "smtpSettingsValid", + "imapSettingsValid", + "smtp_enabled", + "imap_enabled" + ) + emailSettingsValid( + smtpSettingsValid, + imapSettingsValid, + smtpEnabled, + imapEnabled + ) { + let smtpOk = true; + let imapOk = true; + + if (smtpEnabled) { + smtpOk = smtpSettingsValid; + } + + if (imapEnabled) { + imapOk = imapSettingsValid; + } + + return smtpOk && imapOk; + }, + findMembers(params, refresh) { if (isEmpty(this.name) || !this.can_see_members) { return Promise.reject(); 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 index 78f5ba85bef2e2..51943006fba92f 100644 --- 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 @@ -3,11 +3,14 @@

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

- {{#if isSmtpEnabled}} + {{#if group.smtp_enabled}} {{group-smtp-email-settings group=group}} {{/if}} + +
+ {{group-manage-save-button model=group disabled=(not group.emailSettingsValid) beforeSave=beforeSave}} 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 11b4167c70f316..6794ddd969f682 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=isDisabled 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 index 5438b80ad64672..3a71d0a8362e00 100644 --- 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 @@ -3,16 +3,16 @@
- {{input type="text" name="username" value=form.email_username}} + {{input type="text" name="username" value=form.email_username tabindex="1"}}
- {{input type="text" name="smtp_server" value=form.smtp_server}} + {{input type="text" name="smtp_server" value=form.smtp_server tabindex="3"}}
@@ -20,12 +20,12 @@
- {{input type="password" name="password" value=form.email_password}} + {{input type="password" name="password" value=form.email_password tabindex="2"}}
- {{input type="text" name="smtp_port" value=form.smtp_port}} + {{input type="text" name="smtp_port" value=form.smtp_port tabindex="4"}}
@@ -38,10 +38,17 @@
{{d-button + disabled=testingSettings class="btn-primary" action=(action "testSmtpSettings") icon="cog" label="groups.manage.email.test_settings" }} + + {{conditional-loading-spinner size="small" condition=testingSettings}} + + {{#if group.smtpSettingsValid}} + {{d-icon "check-circle"}} {{i18n "groups.manage.email.smtp_settings_valid"}} + {{/if}}
diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 00c5b3cf1dd1c8..a11da46f6382e8 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -153,6 +153,7 @@ def update if group.update(group_params(automatic: group.automatic)) GroupActionLogger.new(current_user, group, skip_guardian: true).log_change_group_settings + group.record_email_setting_changes!(current_user) if guardian.can_see?(group) render json: success_json @@ -574,21 +575,57 @@ def permissions def test_email_settings params.require(:group_id) params.require(:protocol) - params.require(:settings) + 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! - case params[:protocol] - when "smtp" - EmailSettingsValidator.validate_imap(**params[:settings]) - when "imap" - EmailSettingsValidator.validate_imap(**params[:settings]) - else + settings = params.except(:group_id, :protocol) + enable_tls = settings[:ssl] == "true" + + if !["smtp", "imap"].include?(params[:protocol]) raise Discourse::InvalidParameters.new("Valid protocols to test are smtp and imap") end + + begin + case params[:protocol] + when "smtp" + enable_starttls_auto = false + settings.delete(:ssl) + + # 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 settings[:host] == "smtp.gmail.com" + if settings[:port].to_i == 587 + enable_starttls_auto = true + enable_tls = false + elsif settings[:port].to_i == 465 + enable_starttls_auto = false + enable_tls = true + end + end + + final_settings = settings.merge(enable_tls: enable_tls, enable_starttls_auto: enable_starttls_auto, debug: true) + .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, debug: true) + .permit(:host, :port, :username, :password, :ssl, :debug) + EmailSettingsValidator.validate_as_user(current_user, "imap", **final_settings.to_h.symbolize_keys) + end + rescue *EmailSettingsValidator::EXPECTED_EXCEPTIONS => err + return render_json_error( + EmailSettingsValidator.friendly_exception_message(err) + ) + end + + render json: success_json end private @@ -631,10 +668,16 @@ def group_params(automatic: false) :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, diff --git a/app/mailers/group_smtp_mailer.rb b/app/mailers/group_smtp_mailer.rb index 58fcbc4a1b80d2..f8510deae34e5a 100644 --- a/app/mailers/group_smtp_mailer.rb +++ b/app/mailers/group_smtp_mailer.rb @@ -48,7 +48,7 @@ def send_mail(from_group, to_address, post) 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 16c47fd19670fe..b0d4fefb883c3d 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 @@ -88,6 +90,22 @@ def remove_review_groups 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, @@ -281,6 +299,23 @@ def cook_bio 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? @@ -833,11 +868,6 @@ def imap_config } 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? diff --git a/app/serializers/group_show_serializer.rb b/app/serializers/group_show_serializer.rb index 1b0bdf8b789106..9cdf1f842d3057 100644 --- a/app/serializers/group_show_serializer.rb +++ b/app/serializers/group_show_serializer.rb @@ -12,15 +12,24 @@ def self.admin_attributes(*attrs) 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_validator.rb b/app/services/email_settings_validator.rb index 97639828a04063..320a91cc623c3b 100644 --- a/app/services/email_settings_validator.rb +++ b/app/services/email_settings_validator.rb @@ -30,6 +30,12 @@ class EmailSettingsValidator Errno::ECONNREFUSED ] + def self.validate_as_user(user, protocol, **kwargs) + DistributedMutex.synchronize("validate_#{protocol}_#{user.id}", validity: 10) do + self.send("validate_#{protocol}", **kwargs) + end + end + def self.friendly_exception_message(exception) case exception when Net::POPAuthenticationError @@ -47,7 +53,13 @@ def self.friendly_exception_message(exception) I18n.t("email_settings.imap_no_response_error", message: exception.message.gsub(" (Failure)", "")) end when Net::SMTPAuthenticationError - I18n.t("email_settings.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.smtp_authentication_error_gmail_app_password") + else + I18n.t("email_settings.smtp_authentication_error") + end when Net::SMTPServerBusy I18n.t("email_settings.smtp_server_busy_error") when Net::SMTPSyntaxError, Net::SMTPFatalError, Net::SMTPUnknownError @@ -161,6 +173,9 @@ def self.validate_smtp( 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 @@ -179,7 +194,7 @@ def self.validate_imap( port:, username:, password:, - open_timeout: 10, + open_timeout: 5, ssl: true, debug: false ) diff --git a/app/services/post_alerter.rb b/app/services/post_alerter.rb index 77ef4146025bf6..2958cea298641e 100644 --- a/app/services/post_alerter.rb +++ b/app/services/post_alerter.rb @@ -579,13 +579,7 @@ def notify_users(users, type, post, opts = {}) 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 64a2c8e6138ca0..5b23b06eb54c07 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -690,8 +690,13 @@ en: 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." + 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 all IMAP settings will be reset and the associated functionality will be disabled. Are you sure you want to continue?" prefill: title: "Prefill with settings for:" gmail: "GMail" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index feb1bc07557aba..6283bd8e968fff 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -991,6 +991,7 @@ 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." + smtp_authentication_error_gmail_app_password: "Application-specific password required. Learn more at https://support.google.com/accounts/answer/185833" 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}" connection_error: "There was an issue connecting with the server, check the server name and port and try again." 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 00000000000000..4375ba648ce727 --- /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/spec/fabricators/group_fabricator.rb b/spec/fabricators/group_fabricator.rb index 9f04918300bc15..da0ac73d7f483f 100644 --- a/spec/fabricators/group_fabricator.rb +++ b/spec/fabricators/group_fabricator.rb @@ -13,12 +13,14 @@ 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/mailers/group_smtp_mailer_spec.rb b/spec/mailers/group_smtp_mailer_spec.rb index 4be00abf5d11ed..5858418a239286 100644 --- a/spec/mailers/group_smtp_mailer_spec.rb +++ b/spec/mailers/group_smtp_mailer_spec.rb @@ -11,9 +11,11 @@ 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 @@ 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 b2d04acf3c1361..2bef7dfa65ec24 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1011,20 +1011,6 @@ def enable_imap 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 +1180,97 @@ def enable_imap 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/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index e230f017cc9f51..a862cda2dc7caa 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -2004,44 +2004,139 @@ def expect_type_to_return_right_groups(type, expected_group_ids) end describe "#test_email_settings" do - let(:protocol) { "smtp" } - let(:settings) do + let(:params) do { - ssl: true, - port: 587, - host: "smtp.gmail.com", - username: "test@gmail.com", - password: "password" + protocol: protocol, + ssl: ssl, + port: port, + host: host, + username: username, + password: password } end - let(:params) { { settings: settings, protocol: protocol } } before do sign_in(user) group.group_users.where(user: user).last.update(owner: user) end - context "user does not have access to the group" do - before do - group.group_users.destroy_all + 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 "for port 465 and smtp.gmail.com" do + let(:host) { "smtp.gmail.com" } + let(:port) { 465 } + + it "validates with the correct TLS settings" do + EmailSettingsValidator.expects(:validate_smtp).with( + all_of(has_entry(enable_tls: true), has_entry(enable_starttls_auto: false)) + ) + post "/groups/#{group.id}/test_email_settings.json", params: params + expect(response.status).to eq(200) + end + end + + context "for port 587 and smtp.gmail.com" do + let(:host) { "smtp.gmail.com" } + let(:port) { 587 } + + it "validates with the correct TLS settings" do + EmailSettingsValidator.expects(:validate_smtp).with( + all_of(has_entry(enable_tls: false), has_entry(enable_starttls_auto: true)) + ) + post "/groups/#{group.id}/test_email_settings.json", params: params + expect(response.status).to eq(200) + end + end + + 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 - it "errors if the user does not have access to the group" do + 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 - expect(response.status).to eq(403) + 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 - context "rate limited" do - it "rate limits anon searches per user" do - RateLimiter.enable - RateLimiter.clear_all! + 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 - 5.times do + 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 diff --git a/spec/services/email_settings_validator_spec.rb b/spec/services/email_settings_validator_spec.rb index fb688b758eb535..240df4c2285b58 100644 --- a/spec/services/email_settings_validator_spec.rb +++ b/spec/services/email_settings_validator_spec.rb @@ -214,5 +214,19 @@ 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 86ca47e0187b3e..3be77cb60bf2a6 100644 --- a/spec/services/post_alerter_spec.rb +++ b/spec/services/post_alerter_spec.rb @@ -1304,7 +1304,8 @@ def set_topic_notification_level(user, topic, level_name) 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 From fcc986c0b83928b8afef188b646bfce4a64f3afe Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 May 2021 11:43:58 +1000 Subject: [PATCH 04/21] IMAP and SMTP UI interactions and clearing now working well --- .../components/group-imap-email-settings.js | 92 +++++++++++++++++++ .../components/group-manage-email-settings.js | 39 +++++++- .../components/group-smtp-email-settings.js | 15 +++ .../javascripts/discourse/app/models/group.js | 8 ++ .../components/group-imap-email-settings.hbs | 53 +++++++++++ .../group-manage-email-settings.hbs | 18 +++- .../components/group-smtp-email-settings.hbs | 10 +- app/assets/stylesheets/common/base/alert.scss | 4 + app/assets/stylesheets/common/base/group.scss | 4 +- app/models/group.rb | 16 +++- config/locales/client.en.yml | 4 + 11 files changed, 246 insertions(+), 17 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/components/group-imap-email-settings.js create mode 100644 app/assets/javascripts/discourse/app/templates/components/group-imap-email-settings.hbs 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 00000000000000..22d7666512cab3 --- /dev/null +++ b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js @@ -0,0 +1,92 @@ +import Component from "@ember/component"; +import { later } from "@ember/runloop"; +import I18n from "I18n"; +import bootbox from "bootbox"; +import { isEmpty } from "@ember/utils"; +import { popupAjaxError } from "discourse/lib/ajax-error"; +import { on } from "discourse-common/utils/decorators"; +import { action } from "@ember/object"; +import { ajax } from "discourse/lib/ajax"; + +export default Component.extend({ + tagName: "", + group: null, + form: null, + + @action + resetSettingsValid() { + if (this.initializing) { + return; + } + this.set("group.imapSettingsValid", false); + }, + + @on("init") + _fillForm() { + this.initializing = true; + this.set("form", { + imap_server: this.group.imap_server, + imap_port: this.group.imap_port, + imap_ssl: this.group.imap_ssl, + }); + + later(() => { + this.set("group.imapSettingsValid", this.group.imap_enabled); + this.initializing = false; + }); + }, + + @action + prefillSettings(provider) { + let providerDetails = null; + switch (provider) { + case "gmail": + providerDetails = { + server: "imap.gmail.com", + port: "993", + ssl: true, + }; + } + + if (providerDetails) { + this.set("form.imap_server", providerDetails.server); + this.set("form.imap_port", providerDetails.port); + this.set("form.imap_ssl", providerDetails.ssl); + } + }, + + @action + testImapSettings() { + let 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, + }; + + for (const setting in settings) { + if (isEmpty(settings[setting])) { + return bootbox.alert(I18n.t("groups.manage.email.settings_required")); + } + } + + this.set("testingSettings", true); + this.set("group.imapSettingsValid", false); + + return ajax(`/groups/${this.group.id}/test_email_settings`, { + type: "POST", + data: Object.assign(settings, { protocol: "imap" }), + }) + .then(() => { + this.set("group.imapSettingsValid", true); + this.setProperties({ + "group.imap_server": this.form.imap_server, + "group.imap_port": this.form.imap_port, + "group.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 index c35c66138da649..b4e323402efffd 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 @@ -1,4 +1,5 @@ import Component from "@ember/component"; +import discourseComputed from "discourse-common/utils/decorators"; import I18n from "I18n"; import bootbox from "bootbox"; import { action } from "@ember/object"; @@ -7,6 +8,34 @@ export default Component.extend({ tagName: "", group: null, + clearImapEmailSettingsOnSave: false, + clearAllEmailSettingsOnSave: false, + + @discourseComputed( + "group.emailSettingsValid", + "group.smtp_enabled", + "group.imap_enabled" + ) + enableImapSettings(emailSettingsValid, smtpEnabled, imapEnabled) { + return smtpEnabled && (emailSettingsValid || imapEnabled); + }, + + @discourseComputed( + "group.emailSettingsValid", + "clearImapEmailSettingsOnSave", + "clearAllEmailSettingsOnSave" + ) + disableSaveButton( + emailSettingsValid, + clearImapEmailSettingsOnSave, + clearAllEmailSettingsOnSave + ) { + return ( + !emailSettingsValid && + !clearImapEmailSettingsOnSave && + !clearAllEmailSettingsOnSave + ); + }, @action smtpEnabledChange(event) { @@ -14,17 +43,19 @@ export default Component.extend({ bootbox.confirm( I18n.t("groups.manage.email.smtp_disable_confirm"), (result) => { - this.clearAllEmailSettingsOnSave = result; + this.set("clearAllEmailSettingsOnSave", result); if (!result) { this.group.set("smtp_enabled", true); + } else { + this.group.set("imap_enabled", false); } } ); } if (event.target.checked) { - this.clearAllEmailSettingsOnSave = false; + this.set("clearAllEmailSettingsOnSave", false); } this.group.set("smtp_enabled", event.target.checked); @@ -36,7 +67,7 @@ export default Component.extend({ bootbox.confirm( I18n.t("groups.manage.email.imap_disable_confirm"), (result) => { - this.clearImapEmailSettingsOnSave = result; + this.set("clearImapEmailSettingsOnSave", result); if (!result) { this.group.set("imap_enabled", true); @@ -46,7 +77,7 @@ export default Component.extend({ } if (event.target.checked) { - this.clearImapEmailSettingsOnSave = false; + this.set("clearImapEmailSettingsOnSave", false); } this.group.set("imap_enabled", event.target.checked); 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 index 7a5c172a64a74f..c4e938c4c874b4 100644 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -1,4 +1,5 @@ import Component from "@ember/component"; +import { later } from "@ember/runloop"; import I18n from "I18n"; import bootbox from "bootbox"; import { isEmpty } from "@ember/utils"; @@ -12,8 +13,17 @@ export default Component.extend({ group: null, form: null, + @action + resetSettingsValid() { + if (this.initializing) { + return; + } + this.set("group.smtpSettingsValid", false); + }, + @on("init") _fillForm() { + this.initializing = true; this.set("form", { email_username: this.group.email_username, email_password: this.group.email_password, @@ -21,6 +31,11 @@ export default Component.extend({ smtp_port: this.group.smtp_port, smtp_ssl: this.group.smtp_ssl, }); + + later(() => { + this.set("group.smtpSettingsValid", this.group.smtp_enabled); + this.initializing = false; + }); }, @action diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index 55edb157ee6bca..dc679d0218fc44 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -49,6 +49,14 @@ const Group = RestModel.extend({ let smtpOk = true; let imapOk = true; + if (smtpSettingsValid === undefined) { + smtpSettingsValid = false; + } + + if (imapSettingsValid === undefined) { + imapSettingsValid = false; + } + if (smtpEnabled) { smtpOk = smtpSettingsValid; } 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 00000000000000..65e40764f0d784 --- /dev/null +++ b/app/assets/javascripts/discourse/app/templates/components/group-imap-email-settings.hbs @@ -0,0 +1,53 @@ +
+
+
+
+ + {{input type="text" name="imap_server" value=form.imap_server tabindex="3" onChange=(action "resetSettingsValid")}} +
+ + +
+ +
+
+ + {{input type="text" name="imap_port" value=form.imap_port tabindex="4" onChange=(action "resetSettingsValid")}} +
+
+
+ +
+
+ {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}} +
+
+ +
+ {{d-button + disabled=testingSettings + class="btn-primary" + action=(action "testImapSettings") + icon="cog" + label="groups.manage.email.test_settings" + }} + + {{conditional-loading-spinner size="small" condition=testingSettings}} + + {{#if group.imapSettingsValid}} + {{d-icon "check-circle"}} {{i18n "groups.manage.email.imap_settings_valid"}} + {{/if}} +
+ +
+

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 index 51943006fba92f..7df0453a1a2021 100644 --- 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 @@ -11,6 +11,22 @@ {{group-smtp-email-settings group=group}} {{/if}} +

{{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}} + {{/if}} +
- {{group-manage-save-button model=group disabled=(not group.emailSettingsValid) beforeSave=beforeSave}} + {{group-manage-save-button model=group disabled=disableSaveButton beforeSave=beforeSave}} 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 index 3a71d0a8362e00..6248c2692b994a 100644 --- 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 @@ -3,16 +3,16 @@
- {{input type="text" name="username" value=form.email_username tabindex="1"}} + {{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"}} + {{input type="text" name="smtp_server" value=form.smtp_server tabindex="3" onChange=(action "resetSettingsValid")}}
@@ -20,12 +20,12 @@
- {{input type="password" name="password" value=form.email_password tabindex="2"}} + {{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"}} + {{input type="text" name="smtp_port" value=form.smtp_port tabindex="4" onChange=(action "resetSettingsValid")}}
diff --git a/app/assets/stylesheets/common/base/alert.scss b/app/assets/stylesheets/common/base/alert.scss index 64211b17522754..8ade16c7188d07 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 6a0448d8dab28f..a1d22f58e36112 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -242,7 +242,7 @@ table.group-category-permissions { } } -.group-smtp-email-settings { +.group-smtp-email-settings, .group-imap-email-settings { .groups-form { display: grid; grid-template-columns: 1fr 3fr; @@ -251,6 +251,6 @@ table.group-category-permissions { background-color: var(--primary-very-low); padding: 1em; - margin-top: 1em; + margin: 1em 0; border: 1px solid var(--primary-low); } diff --git a/app/models/group.rb b/app/models/group.rb index b0d4fefb883c3d..d33a412340ca97 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -1072,10 +1072,6 @@ def enqueue_update_mentions_job # 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 @@ -1087,10 +1083,20 @@ def enqueue_update_mentions_job # 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/config/locales/client.en.yml b/config/locales/client.en.yml index 5b23b06eb54c07..1acce7248e68f6 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -695,6 +695,10 @@ en: 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_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 https://meta.discourse.org/t/imap-support-for-group-inboxes/160588." + 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 all IMAP settings will be reset and the associated functionality will be disabled. Are you sure you want to continue?" prefill: From 0f3b35f137f21b5bc938a47b14f273125fdae6ed Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 May 2021 11:58:30 +1000 Subject: [PATCH 05/21] Fix scss lint --- app/assets/stylesheets/common/base/group.scss | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index a1d22f58e36112..7770f09e64e87c 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -242,7 +242,8 @@ table.group-category-permissions { } } -.group-smtp-email-settings, .group-imap-email-settings { +.group-smtp-email-settings, +.group-imap-email-settings { .groups-form { display: grid; grid-template-columns: 1fr 3fr; From 67f9069ff3d3c084ee90d8d1f3494e80963767b4 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 May 2021 13:11:39 +1000 Subject: [PATCH 06/21] Fix specs --- .../api/schemas/json/group_response.json | 30 +++++++++++++++++++ .../services/email_settings_validator_spec.rb | 2 ++ 2 files changed, 32 insertions(+) diff --git a/spec/requests/api/schemas/json/group_response.json b/spec/requests/api/schemas/json/group_response.json index b4bf0f15cc9557..7ba352070db85a 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/services/email_settings_validator_spec.rb b/spec/services/email_settings_validator_spec.rb index 240df4c2285b58..d7eab70e804bf2 100644 --- a/spec/services/email_settings_validator_spec.rb +++ b/spec/services/email_settings_validator_spec.rb @@ -175,6 +175,8 @@ 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 From 630751ed461899d0a5c836162eff9657288c9ca6 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 May 2021 14:21:06 +1000 Subject: [PATCH 07/21] IMAP mailbox selection and filtering --- .../components/group-imap-email-settings.js | 21 +++++++++++++++-- .../components/group-manage-email-settings.js | 9 ++++++++ .../components/group-manage-save-button.js | 4 ++++ .../components/group-smtp-email-settings.js | 5 +++- .../components/group-imap-email-settings.hbs | 23 ++++++++++++++++++- .../group-manage-email-settings.hbs | 4 +++- .../app/templates/group/manage/email.hbs | 5 ---- app/assets/stylesheets/common/base/group.scss | 10 ++++++++ app/controllers/groups_controller.rb | 1 + app/models/group.rb | 13 ++++++----- config/locales/client.en.yml | 3 ++- lib/imap/providers/generic.rb | 13 +++++++++-- lib/imap/providers/gmail.rb | 9 ++++++++ spec/models/group_spec.rb | 1 + 14 files changed, 102 insertions(+), 19 deletions(-) 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 index 22d7666512cab3..b4d58cf97f9318 100644 --- a/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js @@ -4,7 +4,7 @@ import I18n from "I18n"; import bootbox from "bootbox"; import { isEmpty } from "@ember/utils"; import { popupAjaxError } from "discourse/lib/ajax-error"; -import { on } from "discourse-common/utils/decorators"; +import discourseComputed, { on } from "discourse-common/utils/decorators"; import { action } from "@ember/object"; import { ajax } from "discourse/lib/ajax"; @@ -13,6 +13,20 @@ export default Component.extend({ group: null, form: null, + @discourseComputed("group.imap_mailboxes") + mailboxes(imapMailboxes) { + return imapMailboxes.map((mailbox) => ({ name: mailbox, value: mailbox })); + }, + + @discourseComputed("group.imap_mailbox_name", "mailboxes.length") + mailboxSelected(mailboxName, mailboxesSize) { + if (mailboxesSize === 0) { + return true; + } + + return !isEmpty(mailboxName); + }, + @action resetSettingsValid() { if (this.initializing) { @@ -31,7 +45,10 @@ export default Component.extend({ }); later(() => { - this.set("group.imapSettingsValid", this.group.imap_enabled); + this.set( + "group.imapSettingsValid", + this.group.imap_enabled && this.form.imap_server + ); this.initializing = 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 index b4e323402efffd..dd267e8c29141d 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 @@ -83,16 +83,24 @@ export default Component.extend({ 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); + }, + @action beforeSave() { if (this.clearAllEmailSettingsOnSave) { this.group.setProperties({ smtp_port: null, + smtp_ssl: false, smtp_server: null, email_username: null, email_password: null, imap_server: null, imap_port: null, + imap_ssl: false, }); } @@ -100,6 +108,7 @@ export default Component.extend({ this.group.setProperties({ imap_server: null, imap_port: null, + imap_ssl: false, }); } }, 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 fcfa0604f5c8f1..0c297b1085bfd5 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 @@ -41,6 +41,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 index c4e938c4c874b4..14220c6eb59dc2 100644 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -33,7 +33,10 @@ export default Component.extend({ }); later(() => { - this.set("group.smtpSettingsValid", this.group.smtp_enabled); + this.set( + "group.smtpSettingsValid", + this.group.smtp_enabled && this.form.smtp_server + ); this.initializing = false; }); }, 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 index 65e40764f0d784..51746c9c76b207 100644 --- 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 @@ -1,5 +1,5 @@
-
+
@@ -18,6 +18,21 @@ {{input type="text" name="imap_port" value=form.imap_port tabindex="4" onChange=(action "resetSettingsValid")}}
+ +
+
+ {{#if mailboxes}} + + {{combo-box name="imap_mailbox_name" + value=group.imap_mailbox_name + valueProperty="value" + content=mailboxes + none="groups.manage.email.mailboxes.disabled" + onChange=(action (mut group.imap_mailbox_name))}} + {{/if}} +
+ +
@@ -26,6 +41,12 @@
+ {{#unless mailboxSelected}} +
+ {{i18n "groups.manage.email.imap_mailbox_not_selected"}} +
+ {{/unless}} +
{{d-button disabled=testingSettings 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 index 7df0453a1a2021..09c8bb5cb4a3a3 100644 --- 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 @@ -11,6 +11,8 @@ {{group-smtp-email-settings group=group}} {{/if}} +
+

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

{{html-safe (i18n "groups.manage.email.imap_instructions")}} @@ -28,5 +30,5 @@ {{/if}}
- {{group-manage-save-button model=group disabled=disableSaveButton beforeSave=beforeSave}} + {{group-manage-save-button model=group disabled=disableSaveButton beforeSave=beforeSave afterSave=afterSave}}

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 912cb40dec5f57..394dd2df081bd2 100644 --- a/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs +++ b/app/assets/javascripts/discourse/app/templates/group/manage/email.hbs @@ -1,6 +1 @@ {{group-manage-email-settings group=model}} - -
- {{groups-form-email-fields model=model}} - {{group-manage-save-button model=model}} -
diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 7770f09e64e87c..c39e6bee8fedda 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/app/assets/stylesheets/common/base/group.scss @@ -248,10 +248,20 @@ table.group-category-permissions { 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/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index a11da46f6382e8..9317af87f3e579 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -618,6 +618,7 @@ def test_email_settings final_settings = settings.merge(ssl: enable_tls, debug: true) .permit(:host, :port, :username, :password, :ssl, :debug) EmailSettingsValidator.validate_as_user(current_user, "imap", **final_settings.to_h.symbolize_keys) + group.expire_imap_mailbox_cache end rescue *EmailSettingsValidator::EXPECTED_EXCEPTIONS => err return render_json_error( diff --git a/app/models/group.rb b/app/models/group.rb index d33a412340ca97..4aad4ea7c80b5d 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -61,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 @@ -130,7 +134,7 @@ def self.visibility_levels 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") @@ -831,10 +835,7 @@ def set_default_notifications 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}") @@ -845,7 +846,7 @@ def imap_mailboxes 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) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 1acce7248e68f6..2cda18d6df283c 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -701,6 +701,7 @@ en: 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 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" @@ -721,7 +722,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/lib/imap/providers/generic.rb b/lib/imap/providers/generic.rb index e331c32f6ce5ab..ac18ca9f467da0 100644 --- a/lib/imap/providers/generic.rb +++ b/lib/imap/providers/generic.rb @@ -143,6 +143,11 @@ def tag_to_label(tag) 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 @@ def list_mailboxes(attr_filter = nil) 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 8400cc5396096a..7ac51f730422b7 100644 --- a/lib/imap/providers/gmail.rb +++ b/lib/imap/providers/gmail.rb @@ -120,6 +120,15 @@ def trash_move(uid) { 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/models/group_spec.rb b/spec/models/group_spec.rb index 2bef7dfa65ec24..1c4ff7f0bf01d9 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -995,6 +995,7 @@ def configure_imap imap_server: "imap.gmail.com", imap_port: 993, imap_ssl: true, + imap_enabled: true, email_username: "test@gmail.com", email_password: "testPassword1!" ) From 785380d9d80694d1fe2c3c508550e97619043261 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 May 2021 14:27:37 +1000 Subject: [PATCH 08/21] Hide IMAP settings if it is not enabled for the site --- .../app/routes/group-manage-email.js | 3 ++- .../group-manage-email-settings.hbs | 26 ++++++++++--------- 2 files changed, 16 insertions(+), 13 deletions(-) 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 f85a645311ac6e..5f8a26e43b73e5 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-manage-email-settings.hbs b/app/assets/javascripts/discourse/app/templates/components/group-manage-email-settings.hbs index 09c8bb5cb4a3a3..f07fcd71007eeb 100644 --- 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 @@ -11,22 +11,24 @@ {{group-smtp-email-settings group=group}} {{/if}} -
+ {{#if siteSettings.enable_imap}} +
-

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

-

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

+

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

+

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

-
{{i18n "groups.manage.email.imap_alpha_warning"}}
+
{{i18n "groups.manage.email.imap_alpha_warning"}}
- + - {{#if group.imap_enabled}} - {{group-imap-email-settings group=group}} + {{#if group.imap_enabled}} + {{group-imap-email-settings group=group}} + {{/if}} {{/if}}
From 433a838657881bc8d9e9caca7cbdcc53a578938f Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 May 2021 15:12:02 +1000 Subject: [PATCH 09/21] Fix spec and add filter_mailboxes spec --- .../components/group-manage-email-settings.js | 2 ++ spec/lib/imap/providers/gmail_spec.rb | 19 +++++++++++++++++++ spec/models/group_spec.rb | 1 + 3 files changed, 22 insertions(+) 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 dd267e8c29141d..65ce04bef056c6 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 @@ -101,6 +101,7 @@ export default Component.extend({ imap_server: null, imap_port: null, imap_ssl: false, + imap_mailbox_name: null, }); } @@ -109,6 +110,7 @@ export default Component.extend({ imap_server: null, imap_port: null, imap_ssl: false, + imap_mailbox_name: null, }); } }, diff --git a/spec/lib/imap/providers/gmail_spec.rb b/spec/lib/imap/providers/gmail_spec.rb index dc6880e2a2196b..014cd1bc1ea477 100644 --- a/spec/lib/imap/providers/gmail_spec.rb +++ b/spec/lib/imap/providers/gmail_spec.rb @@ -70,4 +70,23 @@ 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/models/group_spec.rb b/spec/models/group_spec.rb index 1c4ff7f0bf01d9..07e7c982650ce0 100644 --- a/spec/models/group_spec.rb +++ b/spec/models/group_spec.rb @@ -1004,6 +1004,7 @@ def configure_imap 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 From cb6ac05ad7e1f353cf33a19882bfde8f0c2dd3eb Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Thu, 20 May 2021 16:49:05 +1000 Subject: [PATCH 10/21] Start group email acceptance tests --- .../components/group-imap-email-settings.hbs | 2 +- .../group-manage-email-settings.hbs | 28 ++++---- .../components/group-smtp-email-settings.hbs | 2 +- .../group-manage-email-settings-test.js | 72 +++++++++++++++++++ 4 files changed, 89 insertions(+), 15 deletions(-) create mode 100644 app/assets/javascripts/discourse/tests/acceptance/group-manage-email-settings-test.js 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 index 51746c9c76b207..c621c89ba18910 100644 --- 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 @@ -37,7 +37,7 @@
- {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}} + {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}}
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 index f07fcd71007eeb..ded63c4ea6844f 100644 --- 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 @@ -12,23 +12,25 @@ {{/if}} {{#if siteSettings.enable_imap}} -
+
+
-

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

-

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

+

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

+

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

-
{{i18n "groups.manage.email.imap_alpha_warning"}}
+
{{i18n "groups.manage.email.imap_alpha_warning"}}
- + - {{#if group.imap_enabled}} - {{group-imap-email-settings group=group}} - {{/if}} + {{#if group.imap_enabled}} + {{group-imap-email-settings group=group}} + {{/if}} +
{{/if}}
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 index 6248c2692b994a..d3f2dfec306781 100644 --- 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 @@ -32,7 +32,7 @@
- {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}} + {{i18n "groups.manage.email.prefill.title"}} {{i18n "groups.manage.email.prefill.gmail"}}
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 00000000000000..c6c26a283adf81 --- /dev/null +++ b/app/assets/javascripts/discourse/tests/acceptance/group-manage-email-settings-test.js @@ -0,0 +1,72 @@ +import { acceptance, queryAll } from "discourse/tests/helpers/qunit-helpers"; +import { click, currentRouteName, visit } from "@ember/test-helpers"; +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/alternative-group/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/alternative-group/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 }); + + test("enabling SMTP, testing, and saving", async function (assert) { + await visit("/g/alternative-group/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" + ); + }); + } +); From e4305b71bd99698e63d6d2a73dca79896dba85d0 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Mon, 24 May 2021 15:29:00 +1000 Subject: [PATCH 11/21] Acceptance test for group email settings --- .../components/group-imap-email-settings.js | 3 + .../discourse/app/lib/ajax-error.js | 6 + .../components/group-imap-email-settings.hbs | 8 +- .../components/group-manage-save-button.hbs | 2 +- .../components/group-smtp-email-settings.hbs | 6 +- .../group-manage-email-settings-test.js | 163 +++++++++++++++++- .../tests/fixtures/group-fixtures.js | 6 +- config/locales/client.en.yml | 2 +- 8 files changed, 184 insertions(+), 12 deletions(-) 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 index b4d58cf97f9318..5ec38d4d3534f1 100644 --- a/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js @@ -15,6 +15,9 @@ export default Component.extend({ @discourseComputed("group.imap_mailboxes") mailboxes(imapMailboxes) { + if (!imapMailboxes) { + return []; + } return imapMailboxes.map((mailbox) => ({ name: mailbox, value: mailbox })); }, diff --git a/app/assets/javascripts/discourse/app/lib/ajax-error.js b/app/assets/javascripts/discourse/app/lib/ajax-error.js index 0f3feba41caaee..34a2b3604b4e63 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/templates/components/group-imap-email-settings.hbs b/app/assets/javascripts/discourse/app/templates/components/group-imap-email-settings.hbs index c621c89ba18910..25fb8474f64ab1 100644 --- 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 @@ -42,7 +42,7 @@ {{#unless mailboxSelected}} -
+
{{i18n "groups.manage.email.imap_mailbox_not_selected"}}
{{/unless}} @@ -50,7 +50,7 @@
{{d-button disabled=testingSettings - class="btn-primary" + class="btn-primary test-imap-settings" action=(action "testImapSettings") icon="cog" label="groups.manage.email.test_settings" @@ -59,7 +59,9 @@ {{conditional-loading-spinner size="small" condition=testingSettings}} {{#if group.imapSettingsValid}} - {{d-icon "check-circle"}} {{i18n "groups.manage.email.imap_settings_valid"}} + + {{d-icon "check-circle"}} {{i18n "groups.manage.email.imap_settings_valid"}} + {{/if}}
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 6794ddd969f682..e2a16d4c6f6c4a 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,4 +1,4 @@ -
+
{{d-button action=(action "save") disabled=isDisabled class="btn btn-primary group-manage-save" 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 index d3f2dfec306781..ff50a5dc6abbdf 100644 --- 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 @@ -39,7 +39,7 @@
{{d-button disabled=testingSettings - class="btn-primary" + class="btn-primary test-smtp-settings" action=(action "testSmtpSettings") icon="cog" label="groups.manage.email.test_settings" @@ -48,7 +48,9 @@ {{conditional-loading-spinner size="small" condition=testingSettings}} {{#if group.smtpSettingsValid}} - {{d-icon "check-circle"}} {{i18n "groups.manage.email.smtp_settings_valid"}} + + {{d-icon "check-circle"}} {{i18n "groups.manage.email.smtp_settings_valid"}} + {{/if}}
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 c6c26a283adf81..2c7c96dfcfd0eb 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,12 +1,14 @@ import { acceptance, queryAll } from "discourse/tests/helpers/qunit-helpers"; -import { click, currentRouteName, visit } from "@ember/test-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/alternative-group/manage/email"); + await visit("/g/discourse/manage/email"); assert.equal( currentRouteName(), "group.manage.profile", @@ -22,7 +24,7 @@ acceptance( needs.settings({ enable_smtp: true }); test("When SiteSetting.enable_smtp is true but SiteSetting.enable_imap is false", async function (assert) { - await visit("/g/alternative-group/manage/email"); + await visit("/g/discourse/manage/email"); assert.equal( currentRouteName(), "group.manage.email", @@ -42,8 +44,21 @@ acceptance( 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/alternative-group/manage/email"); + await visit("/g/discourse/manage/email"); assert.ok( exists("#enable_imap:disabled"), "IMAP is disabled until SMTP settings are valid" @@ -67,6 +82,146 @@ acceptance( exists("#enable_ssl:checked"), "prefills SMTP ssl settings for gmail" ); + + await click(".test-smtp-settings"); + assert.equal( + queryAll(".modal-body").text(), + I18n.t("groups.manage.email.settings_required"), + "does not allow testing settings if not all fields are filled" + ); + await click(".modal-footer .btn"); + + 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"); + 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 615fe7508645a4..04e15383ace017 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/config/locales/client.en.yml b/config/locales/client.en.yml index c3244f7772fd52..38462ad306c794 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -704,7 +704,7 @@ en: 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 all 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:" From f5c4f428e1ba6d76e4ea7dc4078f3f327a6cb793 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 25 May 2021 12:10:44 +1000 Subject: [PATCH 12/21] Review fixes round 1, minor fixes --- .../components/group-imap-email-settings.js | 51 ++++++++---------- .../components/group-manage-email-settings.js | 1 - .../components/group-manage-save-button.js | 5 -- .../components/group-smtp-email-settings.js | 53 +++++++++---------- .../javascripts/discourse/app/models/group.js | 23 ++------ .../components/group-imap-email-settings.hbs | 23 ++++---- .../components/group-manage-save-button.hbs | 2 +- app/controllers/groups_controller.rb | 2 +- app/services/email_settings_validator.rb | 2 +- config/locales/client.en.yml | 3 +- config/locales/server.en.yml | 2 +- 11 files changed, 66 insertions(+), 101 deletions(-) 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 index 5ec38d4d3534f1..3c70fa0397a7b5 100644 --- a/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js @@ -5,12 +5,11 @@ import bootbox from "bootbox"; import { isEmpty } from "@ember/utils"; import { popupAjaxError } from "discourse/lib/ajax-error"; import discourseComputed, { on } from "discourse-common/utils/decorators"; -import { action } from "@ember/object"; +import EmberObject, { action } from "@ember/object"; import { ajax } from "discourse/lib/ajax"; export default Component.extend({ tagName: "", - group: null, form: null, @discourseComputed("group.imap_mailboxes") @@ -23,11 +22,7 @@ export default Component.extend({ @discourseComputed("group.imap_mailbox_name", "mailboxes.length") mailboxSelected(mailboxName, mailboxesSize) { - if (mailboxesSize === 0) { - return true; - } - - return !isEmpty(mailboxName); + return mailboxesSize === 0 || !isEmpty(mailboxName); }, @action @@ -41,11 +36,14 @@ export default Component.extend({ @on("init") _fillForm() { this.initializing = true; - this.set("form", { - imap_server: this.group.imap_server, - imap_port: this.group.imap_port, - imap_ssl: this.group.imap_ssl, - }); + this.set( + "form", + EmberObject.create({ + imap_server: this.group.imap_server, + imap_port: this.group.imap_port, + imap_ssl: this.group.imap_ssl, + }) + ); later(() => { this.set( @@ -58,26 +56,19 @@ export default Component.extend({ @action prefillSettings(provider) { - let providerDetails = null; switch (provider) { case "gmail": - providerDetails = { - server: "imap.gmail.com", - port: "993", - ssl: true, - }; - } - - if (providerDetails) { - this.set("form.imap_server", providerDetails.server); - this.set("form.imap_port", providerDetails.port); - this.set("form.imap_ssl", providerDetails.ssl); + this.form.setProperties({ + imap_server: "imap.gmail.com", + imap_port: "993", + imap_ssl: true, + }); } }, @action testImapSettings() { - let settings = { + const settings = { host: this.form.imap_server, port: this.form.imap_port, ssl: this.form.imap_ssl, @@ -99,11 +90,11 @@ export default Component.extend({ data: Object.assign(settings, { protocol: "imap" }), }) .then(() => { - this.set("group.imapSettingsValid", true); - this.setProperties({ - "group.imap_server": this.form.imap_server, - "group.imap_port": this.form.imap_port, - "group.imap_ssl": this.form.imap_ssl, + this.group.setProperties({ + imapSettingsValid: true, + imap_server: this.form.imap_server, + imap_port: this.form.imap_port, + imap_ssl: this.form.imap_ssl, }); }) .catch(popupAjaxError) 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 65ce04bef056c6..c96d3a7013c1d1 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 @@ -7,7 +7,6 @@ import { action } from "@ember/object"; export default Component.extend({ tagName: "", - group: null, clearImapEmailSettingsOnSave: false, clearAllEmailSettingsOnSave: false, 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 0c297b1085bfd5..82cc182b7d7879 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 @@ -14,11 +14,6 @@ export default Component.extend({ return saving ? I18n.t("saving") : I18n.t("save"); }, - @discourseComputed("saving", "disabled") - isDisabled(saving, disabled) { - return saving || disabled; - }, - actions: { save() { if (this.beforeSave) { 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 index 14220c6eb59dc2..2fab10fb8859ac 100644 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -5,12 +5,11 @@ import bootbox from "bootbox"; import { isEmpty } from "@ember/utils"; import { popupAjaxError } from "discourse/lib/ajax-error"; import { on } from "discourse-common/utils/decorators"; -import { action } from "@ember/object"; +import EmberObject, { action } from "@ember/object"; import { ajax } from "discourse/lib/ajax"; export default Component.extend({ tagName: "", - group: null, form: null, @action @@ -24,13 +23,16 @@ export default Component.extend({ @on("init") _fillForm() { this.initializing = true; - this.set("form", { - email_username: this.group.email_username, - email_password: this.group.email_password, - smtp_server: this.group.smtp_server, - smtp_port: this.group.smtp_port, - smtp_ssl: this.group.smtp_ssl, - }); + 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, + smtp_ssl: this.group.smtp_ssl, + }) + ); later(() => { this.set( @@ -43,26 +45,19 @@ export default Component.extend({ @action prefillSettings(provider) { - let providerDetails = null; switch (provider) { case "gmail": - providerDetails = { - server: "smtp.gmail.com", - port: "587", - ssl: true, - }; - } - - if (providerDetails) { - this.set("form.smtp_server", providerDetails.server); - this.set("form.smtp_port", providerDetails.port); - this.set("form.smtp_ssl", providerDetails.ssl); + this.form.setProperties({ + smtp_server: "smtp.gmail.com", + smtp_port: "587", + smtp_ssl: true, + }); } }, @action testSmtpSettings() { - let settings = { + const settings = { host: this.form.smtp_server, port: this.form.smtp_port, ssl: this.form.smtp_ssl, @@ -84,13 +79,13 @@ export default Component.extend({ data: Object.assign(settings, { protocol: "smtp" }), }) .then(() => { - this.set("group.smtpSettingsValid", true); - this.setProperties({ - "group.smtp_server": this.form.smtp_server, - "group.smtp_port": this.form.smtp_port, - "group.smtp_ssl": this.form.smtp_ssl, - "group.email_username": this.form.email_username, - "group.email_password": this.form.email_password, + this.group.setProperties({ + smtpSettingsValid: true, + 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) diff --git a/app/assets/javascripts/discourse/app/models/group.js b/app/assets/javascripts/discourse/app/models/group.js index dc679d0218fc44..c5b197c28c6efa 100644 --- a/app/assets/javascripts/discourse/app/models/group.js +++ b/app/assets/javascripts/discourse/app/models/group.js @@ -46,26 +46,9 @@ const Group = RestModel.extend({ smtpEnabled, imapEnabled ) { - let smtpOk = true; - let imapOk = true; - - if (smtpSettingsValid === undefined) { - smtpSettingsValid = false; - } - - if (imapSettingsValid === undefined) { - imapSettingsValid = false; - } - - if (smtpEnabled) { - smtpOk = smtpSettingsValid; - } - - if (imapEnabled) { - imapOk = imapSettingsValid; - } - - return smtpOk && imapOk; + return ( + (!smtpEnabled || smtpSettingsValid) && (!imapEnabled || imapSettingsValid) + ); }, findMembers(params, refresh) { 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 index 25fb8474f64ab1..1d2d559c374163 100644 --- 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 @@ -24,11 +24,12 @@ {{#if mailboxes}} {{combo-box name="imap_mailbox_name" - value=group.imap_mailbox_name - valueProperty="value" - content=mailboxes - none="groups.manage.email.mailboxes.disabled" - onChange=(action (mut group.imap_mailbox_name))}} + value=group.imap_mailbox_name + valueProperty="value" + content=mailboxes + none="groups.manage.email.mailboxes.disabled" + onChange=(action (mut group.imap_mailbox_name)) + }} {{/if}}
@@ -49,11 +50,11 @@
{{d-button - disabled=testingSettings - class="btn-primary test-imap-settings" - action=(action "testImapSettings") - icon="cog" - label="groups.manage.email.test_settings" + disabled=testingSettings + class="btn-primary test-imap-settings" + action=(action "testImapSettings") + icon="cog" + label="groups.manage.email.test_settings" }} {{conditional-loading-spinner size="small" condition=testingSettings}} @@ -66,7 +67,7 @@
-

Additional Settings

+

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

diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 3078d6a461114e..093cae05ceccd6 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -151,7 +151,10 @@ def update 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 @@ -744,4 +747,24 @@ def users_from_params 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 From bb2b4c155bb8db8aa18c32ea8416d433196503c7 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Tue, 25 May 2021 14:45:30 +1000 Subject: [PATCH 17/21] Only show clear fields warning if any values have been entered --- .../components/group-manage-email-settings.js | 28 +++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) 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 d8f073bfbb6597..ffa586b32a31f8 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 @@ -1,4 +1,5 @@ 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"; @@ -48,9 +49,28 @@ export default Component.extend({ ); }, + _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) { + if ( + !event.target.checked && + this.group.smtp_enabled && + this._anySmtpFieldsFilled() + ) { bootbox.confirm( I18n.t("groups.manage.email.smtp_disable_confirm"), (result) => { @@ -68,7 +88,11 @@ export default Component.extend({ @action imapEnabledChange(event) { - if (!event.target.checked && this.group.imap_enabled) { + if ( + !event.target.checked && + this.group.imap_enabled && + this._anyImapFieldsFilled() + ) { bootbox.confirm( I18n.t("groups.manage.email.imap_disable_confirm"), (result) => { From 3201f8c5705b151733bc62d6cf0e1213d0ba27f5 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 26 May 2021 12:26:50 +1000 Subject: [PATCH 18/21] Move gmail related code into provider specific areas --- app/controllers/groups_controller.rb | 21 +-- .../email_settings_exception_handler.rb | 121 ++++++++++++++++++ app/services/email_settings_validator.rb | 79 ++++-------- config/locales/server.en.yml | 2 +- spec/requests/groups_controller_spec.rb | 26 ---- .../email_settings_exception_handler_spec.rb | 100 +++++++++++++++ .../services/email_settings_validator_spec.rb | 85 +----------- 7 files changed, 253 insertions(+), 181 deletions(-) create mode 100644 app/services/email_settings_exception_handler.rb create mode 100644 spec/services/email_settings_exception_handler_spec.rb diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 093cae05ceccd6..cb072e3d319ce2 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -602,6 +602,7 @@ def test_email_settings 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") @@ -614,29 +615,17 @@ def test_email_settings enable_starttls_auto = false settings.delete(:ssl) - # 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 settings[:host] == "smtp.gmail.com" - if settings[:port].to_i == 587 - enable_starttls_auto = true - enable_tls = false - elsif settings[:port].to_i == 465 - enable_starttls_auto = false - enable_tls = true - end - end - - final_settings = settings.merge(enable_tls: enable_tls, enable_starttls_auto: enable_starttls_auto, debug: true) + 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, debug: true) + 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 *EmailSettingsValidator::EXPECTED_EXCEPTIONS => err + rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS => err return render_json_error( - EmailSettingsValidator.friendly_exception_message(err) + EmailSettingsExceptionHandler.friendly_exception_message(err, email_host) ) end render json: success_json diff --git a/app/services/email_settings_exception_handler.rb b/app/services/email_settings_exception_handler.rb new file mode 100644 index 00000000000000..dc9e885e1bc5ec --- /dev/null +++ b/app/services/email_settings_exception_handler.rb @@ -0,0 +1,121 @@ +# frozen_string_literal: true + +require 'net/imap' +require 'net/smtp' +require 'net/pop' + +class EmailSettingsExceptionHandler + EXPECTED_EXCEPTIONS = [ + Net::POPAuthenticationError, + Net::IMAP::NoResponseError, + 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::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_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 f9ec5fdac4219e..238b5869627cd0 100644 --- a/app/services/email_settings_validator.rb +++ b/app/services/email_settings_validator.rb @@ -12,67 +12,16 @@ # # 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.validate_as_user(user, protocol, **kwargs) DistributedMutex.synchronize("validate_#{protocol}_#{user.id}", validity: 10) do self.public_send("validate_#{protocol}", **kwargs) end end - 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 - # 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.smtp_authentication_error_gmail_app_password") - else - I18n.t("email_settings.smtp_authentication_error") - end - 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) - end - end - ## # Attempts to authenticate and disconnect a POP3 session and if that raises # an error then it is assumed the credentials or some other settings are wrong. @@ -86,7 +35,7 @@ def self.validate_pop3( 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) @@ -132,9 +81,13 @@ def self.validate_smtp( 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 @@ -214,4 +167,20 @@ def self.log_and_raise(err, debug) 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/config/locales/server.en.yml b/config/locales/server.en.yml index 606870689da76e..8fc61c3b0da941 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -994,7 +994,7 @@ 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." - smtp_authentication_error_gmail_app_password: "Application-specific password required. Learn more at this Google Help article" + 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}" connection_error: "There was an issue connecting with the server, check the server name and port and try again." diff --git a/spec/requests/groups_controller_spec.rb b/spec/requests/groups_controller_spec.rb index 1ee1f8769f933a..8a3d46868e44d5 100644 --- a/spec/requests/groups_controller_spec.rb +++ b/spec/requests/groups_controller_spec.rb @@ -2024,32 +2024,6 @@ def expect_type_to_return_right_groups(type, expected_group_ids) let(:host) { "smtp.somemailsite.com" } let(:port) { 587 } - context "for port 465 and smtp.gmail.com" do - let(:host) { "smtp.gmail.com" } - let(:port) { 465 } - - it "validates with the correct TLS settings" do - EmailSettingsValidator.expects(:validate_smtp).with( - all_of(has_entry(enable_tls: true), has_entry(enable_starttls_auto: false)) - ) - post "/groups/#{group.id}/test_email_settings.json", params: params - expect(response.status).to eq(200) - end - end - - context "for port 587 and smtp.gmail.com" do - let(:host) { "smtp.gmail.com" } - let(:port) { 587 } - - it "validates with the correct TLS settings" do - EmailSettingsValidator.expects(:validate_smtp).with( - all_of(has_entry(enable_tls: false), has_entry(enable_starttls_auto: true)) - ) - post "/groups/#{group.id}/test_email_settings.json", params: params - expect(response.status).to eq(200) - end - end - context "when an error is raised" do before do EmailSettingsValidator.expects(:validate_smtp).raises(Net::SMTPAuthenticationError, "Invalid credentials") 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 00000000000000..56873a76bb0c45 --- /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 d7eab70e804bf2..1de20827692586 100644 --- a/spec/services/email_settings_validator_spec.rb +++ b/spec/services/email_settings_validator_spec.rb @@ -6,87 +6,6 @@ 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 } @@ -201,12 +120,12 @@ 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 From e18cab57dd476aae9c71830a8e102724d4dec435 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 26 May 2021 13:25:31 +1000 Subject: [PATCH 19/21] Make form usable on mobile and with keyboard --- .../app/components/group-imap-email-settings.js | 2 +- .../app/components/group-smtp-email-settings.js | 2 +- .../components/group-imap-email-settings.hbs | 10 ++++++---- .../components/group-manage-email-settings.hbs | 6 +++--- .../components/group-smtp-email-settings.hbs | 3 ++- app/assets/stylesheets/mobile/group.scss | 15 +++++++++++++++ app/controllers/groups_controller.rb | 2 +- app/services/email_settings_exception_handler.rb | 7 +++++++ config/locales/server.en.yml | 1 + 9 files changed, 37 insertions(+), 11 deletions(-) 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 index 4618bf3fb3ba8d..4a64dc5ff372c4 100644 --- a/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js @@ -36,7 +36,7 @@ export default Component.extend({ "form", EmberObject.create({ imap_server: this.group.imap_server, - imap_port: this.group.imap_port, + imap_port: (this.group.imap_port || "").toString(), imap_ssl: this.group.imap_ssl, }) ); 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 index 50d4b66bd3bea6..45b3732bed8993 100644 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -25,7 +25,7 @@ export default Component.extend({ email_username: this.group.email_username, email_password: this.group.email_password, smtp_server: this.group.smtp_server, - smtp_port: this.group.smtp_port, + smtp_port: (this.group.smtp_port || "").toString(), smtp_ssl: this.group.smtp_ssl, }) ); 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 index 9d4f2e39494392..d9d357b6e31319 100644 --- 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 @@ -3,11 +3,11 @@
- {{input type="text" name="imap_server" value=form.imap_server tabindex="3" onChange=(action "resetSettingsValid")}} + {{input type="text" name="imap_server" value=form.imap_server tabindex="8" onChange=(action "resetSettingsValid")}}
@@ -15,7 +15,7 @@
- {{input type="text" name="imap_port" value=form.imap_port tabindex="4" onChange=(action "resetSettingsValid")}} + {{input type="text" name="imap_port" value=form.imap_port tabindex="9" onChange=(action "resetSettingsValid" form.imap_port)}}
@@ -28,6 +28,7 @@ valueProperty="value" content=mailboxes none="groups.manage.email.mailboxes.disabled" + tabindex="10" onChange=(action (mut group.imap_mailbox_name)) }} {{/if}} @@ -55,6 +56,7 @@ action=(action "testImapSettings") icon="cog" label="groups.manage.email.test_settings" + tabindex="12" }} {{conditional-loading-spinner size="small" condition=testingSettings}} @@ -69,7 +71,7 @@

{{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 index 7980b6221a40c3..97f08c53483235 100644 --- 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 @@ -3,7 +3,7 @@

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

@@ -23,7 +23,7 @@
{{i18n "groups.manage.email.imap_alpha_warning"}}
@@ -34,5 +34,5 @@ {{/if}}
- {{group-manage-save-button model=group disabled=(not emailSettingsValid) beforeSave=beforeSave afterSave=afterSave}} + {{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-smtp-email-settings.hbs b/app/assets/javascripts/discourse/app/templates/components/group-smtp-email-settings.hbs index 05b3c8a3ca0742..50c503419e777d 100644 --- 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 @@ -25,7 +25,7 @@
- {{input type="text" name="smtp_port" value=form.smtp_port tabindex="4" onChange=(action "resetSettingsValid")}} + {{input type="text" name="smtp_port" value=form.smtp_port tabindex="4" onChange=(action "resetSettingsValid" form.smtp_port)}}
@@ -43,6 +43,7 @@ action=(action "testSmtpSettings") icon="cog" label="groups.manage.email.test_settings" + tabindex="6" }} {{conditional-loading-spinner size="small" condition=testingSettings}} diff --git a/app/assets/stylesheets/mobile/group.scss b/app/assets/stylesheets/mobile/group.scss index d54562b81a8084..82cbac7969713b 100644 --- a/app/assets/stylesheets/mobile/group.scss +++ b/app/assets/stylesheets/mobile/group.scss @@ -47,3 +47,18 @@ 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 cb072e3d319ce2..3d6e6262bc3d38 100644 --- a/app/controllers/groups_controller.rb +++ b/app/controllers/groups_controller.rb @@ -623,7 +623,7 @@ def test_email_settings .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 => err + rescue *EmailSettingsExceptionHandler::EXPECTED_EXCEPTIONS, StandardError => err return render_json_error( EmailSettingsExceptionHandler.friendly_exception_message(err, email_host) ) diff --git a/app/services/email_settings_exception_handler.rb b/app/services/email_settings_exception_handler.rb index dc9e885e1bc5ec..ddb3fb908b8194 100644 --- a/app/services/email_settings_exception_handler.rb +++ b/app/services/email_settings_exception_handler.rb @@ -8,6 +8,7 @@ class EmailSettingsExceptionHandler EXPECTED_EXCEPTIONS = [ Net::POPAuthenticationError, Net::IMAP::NoResponseError, + Net::IMAP::Error, Net::SMTPAuthenticationError, Net::SMTPServerBusy, Net::SMTPSyntaxError, @@ -30,6 +31,8 @@ def message 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 @@ -64,6 +67,10 @@ def net_imap_no_response_error 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 diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 8fc61c3b0da941..dbaf87bb0364d3 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -997,6 +997,7 @@ en: 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}" From 8e14aa0e2ac0fef4c7cfc8a183f6afb5de4d7a00 Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 26 May 2021 13:37:59 +1000 Subject: [PATCH 20/21] SCSS lint --- app/assets/stylesheets/mobile/group.scss | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/assets/stylesheets/mobile/group.scss b/app/assets/stylesheets/mobile/group.scss index 82cbac7969713b..7053aa3ee464ba 100644 --- a/app/assets/stylesheets/mobile/group.scss +++ b/app/assets/stylesheets/mobile/group.scss @@ -53,7 +53,9 @@ .groups-form { grid-template-columns: 1fr; - input[type="text"], input[type="password"], .group-imap-mailboxes .combo-box { + input[type="text"], + input[type="password"], + .group-imap-mailboxes .combo-box { width: 100%; } From cfffcef0545bd7adf81dc762d693be4786e4be3f Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Wed, 26 May 2021 15:52:10 +1000 Subject: [PATCH 21/21] Keep testing button disabled until all fields are filled --- .../components/group-imap-email-settings.js | 23 +++++++++++------ .../components/group-smtp-email-settings.js | 25 ++++++++++++------- .../components/group-imap-email-settings.hbs | 3 ++- .../components/group-smtp-email-settings.hbs | 3 ++- .../group-manage-email-settings-test.js | 13 ++++++---- 5 files changed, 43 insertions(+), 24 deletions(-) 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 index 4a64dc5ff372c4..012c1178691992 100644 --- a/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-imap-email-settings.js @@ -1,7 +1,5 @@ import Component from "@ember/component"; import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings"; -import I18n from "I18n"; -import bootbox from "bootbox"; import { isEmpty } from "@ember/utils"; import { popupAjaxError } from "discourse/lib/ajax-error"; import discourseComputed, { on } from "discourse-common/utils/decorators"; @@ -12,6 +10,21 @@ 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) { @@ -57,12 +70,6 @@ export default Component.extend({ password: this.group.email_password, }; - for (const setting in settings) { - if (isEmpty(settings[setting])) { - return bootbox.alert(I18n.t("groups.manage.email.settings_required")); - } - } - this.set("testingSettings", true); this.set("imapSettingsValid", 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 index 45b3732bed8993..d9758d2c4631a6 100644 --- a/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js +++ b/app/assets/javascripts/discourse/app/components/group-smtp-email-settings.js @@ -1,10 +1,8 @@ import Component from "@ember/component"; import emailProviderDefaultSettings from "discourse/lib/email-provider-default-settings"; -import I18n from "I18n"; -import bootbox from "bootbox"; import { isEmpty } from "@ember/utils"; import { popupAjaxError } from "discourse/lib/ajax-error"; -import { on } from "discourse-common/utils/decorators"; +import discourseComputed, { on } from "discourse-common/utils/decorators"; import EmberObject, { action } from "@ember/object"; import { ajax } from "discourse/lib/ajax"; @@ -12,6 +10,21 @@ 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); @@ -46,12 +59,6 @@ export default Component.extend({ password: this.form.email_password, }; - for (const setting in settings) { - if (isEmpty(settings[setting])) { - return bootbox.alert(I18n.t("groups.manage.email.settings_required")); - } - } - this.set("testingSettings", true); this.set("smtpSettingsValid", false); 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 index d9d357b6e31319..2c6025d4ac4cd2 100644 --- 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 @@ -51,12 +51,13 @@
{{d-button - disabled=testingSettings + 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}} 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 index 50c503419e777d..fbe3f88b5cbc5e 100644 --- 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 @@ -38,12 +38,13 @@
{{d-button - disabled=testingSettings + 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}} 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 2c7c96dfcfd0eb..963f51ae171289 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 @@ -83,13 +83,10 @@ acceptance( "prefills SMTP ssl settings for gmail" ); - await click(".test-smtp-settings"); - assert.equal( - queryAll(".modal-body").text(), - I18n.t("groups.manage.email.settings_required"), + assert.ok( + exists(".test-smtp-settings:disabled"), "does not allow testing settings if not all fields are filled" ); - await click(".modal-footer .btn"); await fillIn('input[name="username"]', "myusername@gmail.com"); await fillIn('input[name="password"]', "password@gmail.com"); @@ -135,6 +132,12 @@ acceptance( ); 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(),