From d7016a0bd497abe33722b85e8b98540d80965231 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 22 Mar 2019 22:52:29 +0100 Subject: [PATCH 1/3] Add validations to admin settings - Validate correct HTML markup - Validate presence of contact username & e-mail - Validate that all usernames are valid - Validate that enums have expected values --- app/controllers/admin/settings_controller.rb | 73 ++-------- app/models/form/admin_settings.rb | 131 ++++++++++++------ app/validators/existing_username_validator.rb | 20 +++ app/validators/html_validator.rb | 14 ++ app/views/admin/settings/edit.html.haml | 1 + config/locales/en.yml | 5 + config/navigation.rb | 2 +- 7 files changed, 136 insertions(+), 110 deletions(-) create mode 100644 app/validators/existing_username_validator.rb create mode 100644 app/validators/html_validator.rb diff --git a/app/controllers/admin/settings_controller.rb b/app/controllers/admin/settings_controller.rb index a763597f20ede0..dc1c79b7fd7747 100644 --- a/app/controllers/admin/settings_controller.rb +++ b/app/controllers/admin/settings_controller.rb @@ -2,84 +2,29 @@ module Admin class SettingsController < BaseController - ADMIN_SETTINGS = %w( - site_contact_username - site_contact_email - site_title - site_short_description - site_description - site_extended_description - site_terms - registrations_mode - closed_registrations_message - open_deletion - timeline_preview - show_staff_badge - bootstrap_timeline_accounts - theme - thumbnail - hero - mascot - min_invite_role - activity_api_enabled - peers_api_enabled - show_known_fediverse_at_about_page - preview_sensitive_media - custom_css - profile_directory - ).freeze - - BOOLEAN_SETTINGS = %w( - open_deletion - timeline_preview - show_staff_badge - activity_api_enabled - peers_api_enabled - show_known_fediverse_at_about_page - preview_sensitive_media - profile_directory - ).freeze - - UPLOAD_SETTINGS = %w( - thumbnail - hero - mascot - ).freeze - def edit authorize :settings, :show? + @admin_settings = Form::AdminSettings.new end def update authorize :settings, :update? - settings_params.each do |key, value| - if UPLOAD_SETTINGS.include?(key) - upload = SiteUpload.where(var: key).first_or_initialize(var: key) - upload.update(file: value) - else - setting = Setting.where(var: key).first_or_initialize(var: key) - setting.update(value: value_for_update(key, value)) - end - end + @admin_settings = Form::AdminSettings.new(settings_params) - flash[:notice] = I18n.t('generic.changes_saved_msg') - redirect_to edit_admin_settings_path + if @admin_settings.save + flash[:notice] = I18n.t('generic.changes_saved_msg') + redirect_to edit_admin_settings_path + else + render :edit + end end private def settings_params - params.require(:form_admin_settings).permit(ADMIN_SETTINGS) - end - - def value_for_update(key, value) - if BOOLEAN_SETTINGS.include?(key) - value == '1' - else - value - end + params.require(:form_admin_settings).permit(*Form::AdminSettings::KEYS) end end end diff --git a/app/models/form/admin_settings.rb b/app/models/form/admin_settings.rb index a21394a52a81a7..154d572ddf85ed 100644 --- a/app/models/form/admin_settings.rb +++ b/app/models/form/admin_settings.rb @@ -3,49 +3,90 @@ class Form::AdminSettings include ActiveModel::Model - delegate( - :site_contact_username, - :site_contact_username=, - :site_contact_email, - :site_contact_email=, - :site_title, - :site_title=, - :site_short_description, - :site_short_description=, - :site_description, - :site_description=, - :site_extended_description, - :site_extended_description=, - :site_terms, - :site_terms=, - :registrations_mode, - :registrations_mode=, - :closed_registrations_message, - :closed_registrations_message=, - :open_deletion, - :open_deletion=, - :timeline_preview, - :timeline_preview=, - :show_staff_badge, - :show_staff_badge=, - :bootstrap_timeline_accounts, - :bootstrap_timeline_accounts=, - :theme, - :theme=, - :min_invite_role, - :min_invite_role=, - :activity_api_enabled, - :activity_api_enabled=, - :peers_api_enabled, - :peers_api_enabled=, - :show_known_fediverse_at_about_page, - :show_known_fediverse_at_about_page=, - :preview_sensitive_media, - :preview_sensitive_media=, - :custom_css, - :custom_css=, - :profile_directory, - :profile_directory=, - to: Setting - ) + KEYS = %i( + site_contact_username + site_contact_email + site_title + site_short_description + site_description + site_extended_description + site_terms + registrations_mode + closed_registrations_message + open_deletion + timeline_preview + show_staff_badge + bootstrap_timeline_accounts + theme + min_invite_role + activity_api_enabled + peers_api_enabled + show_known_fediverse_at_about_page + preview_sensitive_media + custom_css + profile_directory + ).freeze + + BOOLEAN_KEYS = %i( + open_deletion + timeline_preview + show_staff_badge + activity_api_enabled + peers_api_enabled + show_known_fediverse_at_about_page + preview_sensitive_media + profile_directory + ).freeze + + UPLOAD_KEYS = %i( + thumbnail + hero + mascot + ).freeze + + attr_accessor *KEYS + + validates :site_short_description, :site_description, :site_extended_description, :site_terms, :closed_registrations_message, html: true + validates :registrations_mode, inclusion: { in: %w(open approved none) } + validates :min_invite_role, inclusion: { in: %w(disabled user moderator admin) } + validates :site_contact_email, :site_contact_username, presence: true + validates :site_contact_username, existing_username: true + validates :bootstrap_timeline_accounts, existing_username: { multiple: true } + + def initialize(_attributes = {}) + super + initialize_attributes + end + + def save + return false unless valid? + + KEYS.each do |key| + value = instance_variable_get("@#{key}") + + if UPLOAD_KEYS.include?(key) + upload = SiteUpload.where(var: key).first_or_initialize(var: key) + upload.update(file: value) + else + setting = Setting.where(var: key).first_or_initialize(var: key) + setting.update(value: typecast_value(key, value)) + end + end + end + + private + + def initialize_attributes + KEYS.each do |key| + instance_variable_set("@#{key}", Setting.public_send(key)) if instance_variable_get("@#{key}").nil? + end + end + + def typecast_value(key, value) + if BOOLEAN_KEYS.include?(key) + value == '1' + else + value + end + end end diff --git a/app/validators/existing_username_validator.rb b/app/validators/existing_username_validator.rb new file mode 100644 index 00000000000000..4388a0c98365f5 --- /dev/null +++ b/app/validators/existing_username_validator.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +class ExistingUsernameValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return if value.blank? + + if options[:multiple] + missing_usernames = value.split(',').map { |username| username unless Account.find_local(username) }.compact + record.errors.add(attribute, I18n.t('existing_username_validator.not_found_multiple', usernames: missing_usernames.join(', '))) if missing_usernames.any? + else + record.errors.add(attribute, I18n.t('existing_username_validator.not_found')) unless Account.find_local(value) + end + end + + private + + def valid_html?(str) + Nokogiri::HTML.fragment(str).to_s == str + end +end diff --git a/app/validators/html_validator.rb b/app/validators/html_validator.rb new file mode 100644 index 00000000000000..882c35d4136209 --- /dev/null +++ b/app/validators/html_validator.rb @@ -0,0 +1,14 @@ +# frozen_string_literal: true + +class HtmlValidator < ActiveModel::EachValidator + def validate_each(record, attribute, value) + return if value.blank? + record.errors.add(attribute, I18n.t('html_validator.invalid_markup')) unless valid_html?(value) + end + + private + + def valid_html?(str) + Nokogiri::HTML.fragment(str).to_s == str + end +end diff --git a/app/views/admin/settings/edit.html.haml b/app/views/admin/settings/edit.html.haml index d9b4bf01b85f02..1c2c00f109c578 100644 --- a/app/views/admin/settings/edit.html.haml +++ b/app/views/admin/settings/edit.html.haml @@ -2,6 +2,7 @@ = t('admin.settings.title') = simple_form_for @admin_settings, url: admin_settings_path, html: { method: :patch } do |f| + = render 'shared/error_messages', object: @admin_settings .fields-group = f.input :site_title, wrapper: :with_label, label: t('admin.settings.site_title') diff --git a/config/locales/en.yml b/config/locales/en.yml index ba42e7ce1b9b5e..d5ed2062387672 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -586,6 +586,9 @@ en: content: We're sorry, but something went wrong on our end. title: This page is not correct noscript_html: To use the Mastodon web application, please enable JavaScript. Alternatively, try one of the native apps for Mastodon for your platform. + existing_username_validator: + not_found: could not find a local user with that username + not_found_multiple: could not find %{usernames} exports: archive_takeout: date: Date @@ -633,6 +636,8 @@ en: validation_errors: one: Something isn't quite right yet! Please review the error below other: Something isn't quite right yet! Please review %{count} errors below + html_validator: + invalid_markup: contains invalid HTML markup identity_proofs: active: Active authorize: Yes, authorize diff --git a/config/navigation.rb b/config/navigation.rb index 07aec4b9dec703..f136141b3a59fa 100644 --- a/config/navigation.rb +++ b/config/navigation.rb @@ -37,7 +37,7 @@ primary.item :admin, safe_join([fa_icon('cogs fw'), t('admin.title')]), admin_dashboard_url, if: proc { current_user.staff? } do |admin| admin.item :dashboard, safe_join([fa_icon('tachometer fw'), t('admin.dashboard.title')]), admin_dashboard_url - admin.item :settings, safe_join([fa_icon('cogs fw'), t('admin.settings.title')]), edit_admin_settings_url, if: -> { current_user.admin? } + admin.item :settings, safe_join([fa_icon('cogs fw'), t('admin.settings.title')]), edit_admin_settings_url, if: -> { current_user.admin? }, highlights_on: %r{/admin/settings} admin.item :custom_emojis, safe_join([fa_icon('smile-o fw'), t('admin.custom_emojis.title')]), admin_custom_emojis_url, highlights_on: %r{/admin/custom_emojis} admin.item :relays, safe_join([fa_icon('exchange fw'), t('admin.relays.title')]), admin_relays_url, if: -> { current_user.admin? }, highlights_on: %r{/admin/relays} admin.item :subscriptions, safe_join([fa_icon('paper-plane-o fw'), t('admin.subscriptions.title')]), admin_subscriptions_url, if: -> { current_user.admin? } From 72e95b3e6c74997ba3bfd858a23b6f9d0fe199d0 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 22 Mar 2019 23:07:58 +0100 Subject: [PATCH 2/3] Fix code style issue --- app/models/form/admin_settings.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/form/admin_settings.rb b/app/models/form/admin_settings.rb index 154d572ddf85ed..2d3aa726d67169 100644 --- a/app/models/form/admin_settings.rb +++ b/app/models/form/admin_settings.rb @@ -44,7 +44,7 @@ class Form::AdminSettings mascot ).freeze - attr_accessor *KEYS + attr_accessor(*KEYS) validates :site_short_description, :site_description, :site_extended_description, :site_terms, :closed_registrations_message, html: true validates :registrations_mode, inclusion: { in: %w(open approved none) } From 8e243d11b357b6ae011ee53e42ade3a9c2f4a0a6 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Fri, 22 Mar 2019 23:13:32 +0100 Subject: [PATCH 3/3] Fix tests --- spec/controllers/admin/settings_controller_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/controllers/admin/settings_controller_spec.rb b/spec/controllers/admin/settings_controller_spec.rb index 34f6bbdae0e830..6cf0ee20a61d8d 100644 --- a/spec/controllers/admin/settings_controller_spec.rb +++ b/spec/controllers/admin/settings_controller_spec.rb @@ -19,6 +19,10 @@ end describe 'PUT #update' do + before do + allow_any_instance_of(Form::AdminSettings).to receive(:valid?).and_return(true) + end + describe 'for a record that doesnt exist' do around do |example| before = Setting.site_extended_description