Add ability to run validation on site settings. notification_email and other email address settings are now validated.

app/assets/javascripts/admin/models/site_setting.js

@@ -8,6 +8,8 @@
 **/
 Discourse.SiteSetting = Discourse.Model.extend({
 
+  validationMessage: null,
+
   /**
     Is the boolean setting true?
 
@@ -67,6 +69,7 @@ Discourse.SiteSetting = Discourse.Model.extend({
   **/
   resetValue: function() {
     this.set('value', this.get('originalValue'));
+    this.set('validationMessage', null);
   },
 
   /**
@@ -76,13 +79,20 @@ Discourse.SiteSetting = Discourse.Model.extend({
   **/
   save: function() {
     // Update the setting
-    var setting = this, data = {};
+    var self = this, data = {};
     data[this.get('setting')] = this.get('value');
     return Discourse.ajax("/admin/site_settings/" + this.get('setting'), {
       data: data,
       type: 'PUT'
     }).then(function() {
-      setting.set('originalValue', setting.get('value'));
+      self.set('originalValue', self.get('value'));
+      self.set('validationMessage', null);
+    }, function(e) {
+      if (e.responseJSON && e.responseJSON.errors) {
+        self.set('validationMessage', e.responseJSON.errors[0]);
+      } else {
+        self.set('validationMessage', I18n.t('generic_error'));
+      }
     });
   },
 

app/assets/javascripts/admin/templates/site_settings/setting_string.js.handlebars

@@ -3,6 +3,7 @@
 </div>
 <div class="setting-value">
   {{textField value=value classNames="input-setting-string"}}
+  <div {{bind-attr class=":validation-error validationMessage::hidden"}}><i class='fa fa-times'></i> {{validationMessage}}</div>
   <div class='desc'>{{unbound description}}</div>
 </div>
 {{#if dirty}}

app/assets/stylesheets/common/admin/admin_base.scss

@@ -229,13 +229,20 @@
 
     }
 
-    .desc {
+    .desc, .validation-error {
       padding-top: 3px;
-      color: scale-color($primary, $lightness: 50%);
       font-size: 80%;
       line-height: 1.4em;
     }
 
+    .validation-error {
+      color: $danger;
+    }
+
+    .desc {
+      color: scale-color($primary, $lightness: 50%);
+    }
+
     h3 {
       font-size: 13px;
       font-weight: normal;

app/controllers/admin/site_settings_controller.rb

@@ -11,9 +11,14 @@ class Admin::SiteSettingsController < Admin::AdminController
     id = params[:id]
     value = params[id]
     value.strip! if value.is_a?(String)
-    StaffActionLogger.new(current_user).log_site_setting_change(id, SiteSetting.send(id), value) if SiteSetting.has_setting?(id)
+    begin
+      prev_value = SiteSetting.send(id)
       SiteSetting.set(id, value)
+      StaffActionLogger.new(current_user).log_site_setting_change(id, prev_value, value) if SiteSetting.has_setting?(id)
       render nothing: true
+    rescue Discourse::InvalidParameters => e
+      render json: {errors: [e.message]}, status: 422
+    end
   end
 
 end

config/locales/server.en.yml

@@ -909,6 +909,9 @@ en:
     enable_cdn_js_debugging: "Allow /logs to display proper errors by adding crossorigin permissions on all js includes"
     show_create_topics_notice: "If the site has fewer than 5 public topics, show a notice asking admins to create some topics."
 
+    errors:
+      invalid_email: "Invalid email address."
+
   notification_types:
     mentioned: "%{display_username} mentioned you in %{link}"
     liked: "%{display_username} liked your post in %{link}"

config/site_settings.yml

@@ -3,8 +3,12 @@ required:
     client: true
     default: 'Discourse'
   site_description: ''
-  contact_email: ''
-  notification_email: 'info@discourse.org'
+  contact_email:
+    default: ''
+    validator: 'EmailSettingValidator'
+  notification_email:
+    default: 'info@discourse.org'
+    validator: 'EmailSettingValidator'
   site_contact_username: ''
   logo_url:
     client: true
@@ -307,7 +311,9 @@ email:
   email_in:
     default: false
     client: true
-  email_in_address: ''
+  email_in_address:
+    default: ''
+    validator: 'EmailSettingValidator'
   email_in_min_trust:
     default: 3
     enum: 'MinTrustToCreateTopicSetting'

lib/site_setting_extension.rb

@@ -54,6 +54,10 @@ module SiteSettingExtension
     @refresh_settings ||= []
   end
 
+  def validators
+    @validators ||= {}
+  end
+
   def setting(name_arg, default = nil, opts = {})
     name = name_arg.to_sym
     mutex.synchronize do
@@ -78,6 +82,9 @@ module SiteSettingExtension
       if opts[:refresh]
         refresh_settings << name
       end
+      if v = opts[:validator]
+        validators[name] = v.is_a?(String) ? v.constantize : v
+      end
 
       current[name] = current_value
       setup_methods(name, current_value)
@@ -232,6 +239,10 @@ module SiteSettingExtension
       raise Discourse::InvalidParameters.new(:value) unless enum_class(name).valid_value?(val)
     end
 
+    if v = validators[name] and !v.valid_value?(val)
+      raise Discourse::InvalidParameters.new(v.error_message(val))
+    end
+
     provider.save(name, val, type)
     current[name] = convert(val, type)
     clear_cache!

lib/validators/email_setting_validator.rb

@@ -0,0 +1,9 @@
+class EmailSettingValidator
+  def self.valid_value?(val)
+    val == '' || EmailValidator.email_regex =~ val
+  end
+
+  def self.error_message(val)
+    I18n.t('site_settings.errors.invalid_email')
+  end
+end

lib/validators/email_validator.rb

@@ -21,4 +21,8 @@ class EmailValidator < ActiveModel::EachValidator
     value =~ regexp
   end
 
+  def self.email_regex
+    /^[a-zA-Z0-9!#$%&'*+\/=?\^_`{|}~\-]+(?:\.[a-zA-Z0-9!#$%&'\*+\/=?\^_`{|}~\-]+)*@(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?\.)+[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?$/
+  end
+
 end

spec/components/site_setting_extension_spec.rb

@@ -238,6 +238,10 @@ describe SiteSettingExtension do
     end
 
     context 'when overridden' do
+      after :each do
+        settings.remove_override!(:validated_setting)
+      end
+
       it 'stores valid values' do
         test_enum_class.expects(:valid_value?).with('fr').returns(true)
         settings.test_enum = 'fr'
@@ -278,6 +282,36 @@ describe SiteSettingExtension do
     end
   end
 
+  describe "setting with a validator" do
+    before do
+      settings.setting(:validated_setting, "info@example.com", {validator: 'EmailSettingValidator'})
+      settings.refresh!
+    end
+
+    after :each do
+      settings.remove_override!(:validated_setting)
+    end
+
+    it "stores valid values" do
+      EmailSettingValidator.expects(:valid_value?).returns(true)
+      settings.validated_setting = 'success@example.com'
+      settings.validated_setting.should == 'success@example.com'
+    end
+
+    it "rejects invalid values" do
+      expect {
+        EmailSettingValidator.expects(:valid_value?).returns(false)
+        settings.validated_setting = 'nope'
+      }.to raise_error(Discourse::InvalidParameters)
+      settings.validated_setting.should == "info@example.com"
+    end
+
+    it "allows blank values" do
+      settings.validated_setting = ''
+      settings.validated_setting.should == ''
+    end
+  end
+
   describe "set for an invalid setting name" do
     it "raises an error" do
       settings.setting(:test_setting, 77)