From b4a480edaebfff828da16e45a4f850936da26990 Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Tue, 26 Jan 2021 09:12:24 +0700 Subject: [PATCH] Add validations to SiteConfig model (#12341) * Move straightforward validations * Add color contrast validation to SiteConfig * Move domain and emoji validation to SiteConfig * Add specs * Update service object * Experimentally remove validation * Update spec * Fix comment --- app/models/site_config.rb | 64 +++++++++---- app/services/site_configs/upsert.rb | 73 ++------------- app/validators/color_contrast_validator.rb | 11 +++ app/validators/emoji_only_validator.rb | 9 ++ app/validators/valid_domain_csv_validator.rb | 10 +++ spec/models/site_config_spec.rb | 94 +++++++++++++++++++- 6 files changed, 175 insertions(+), 86 deletions(-) create mode 100644 app/validators/color_contrast_validator.rb create mode 100644 app/validators/emoji_only_validator.rb create mode 100644 app/validators/valid_domain_csv_validator.rb diff --git a/app/models/site_config.rb b/app/models/site_config.rb index 517ea4155..93a8f1eb7 100644 --- a/app/models/site_config.rb +++ b/app/models/site_config.rb @@ -4,14 +4,15 @@ class SiteConfig < RailsSettings::Base self.table_name = "site_configs" - validates :var, presence: true - # the site configuration is cached, change this if you want to force update # the cache, or call SiteConfig.clear_cache cache_prefix { "v1" } + HEX_COLOR_REGEX = /\A#(\h{6}|\h{3})\z/.freeze LIGHTNING_ICON = File.read(Rails.root.join("app/assets/images/lightning.svg")).freeze STACK_ICON = File.read(Rails.root.join("app/assets/images/stack.svg")).freeze + VALID_URL = %r{\A(http|https)://([/|.\w\s-])*.[a-z]{2,5}(:[0-9]{1,5})?(/.*)?\z}.freeze + URL_MESSAGE = "must be a valid URL".freeze # Meta field :admin_action_taken_at, type: :datetime, default: Time.current @@ -27,7 +28,9 @@ class SiteConfig < RailsSettings::Base # Authentication field :allow_email_password_registration, type: :boolean, default: false field :allow_email_password_login, type: :boolean, default: true - field :allowed_registration_email_domains, type: :array, default: %w[] + field :allowed_registration_email_domains, type: :array, default: %w[], validates: { + valid_domain_csv: true + } field :display_email_domain_allow_list_publicly, type: :boolean, default: false field :require_captcha_for_email_password_registration, type: :boolean, default: false field :authentication_providers, type: :array, default: %w[] @@ -48,14 +51,16 @@ class SiteConfig < RailsSettings::Base field :campaign_hero_html_variant_name, type: :string, default: "" field :campaign_featured_tags, type: :array, default: %w[] field :campaign_sidebar_enabled, type: :boolean, default: 0 - field :campaign_sidebar_image, type: :string, default: nil + field :campaign_sidebar_image, type: :string, default: nil, validates: { + format: { with: VALID_URL, message: URL_MESSAGE } + } field :campaign_url, type: :string, default: nil field :campaign_articles_require_approval, type: :boolean, default: 0 field :campaign_articles_expiry_time, type: :integer, default: 4 # Community Content field :community_name, type: :string, default: ApplicationConfig["COMMUNITY_NAME"] || "New Forem" - field :community_emoji, type: :string, default: "🌱" + field :community_emoji, type: :string, default: "🌱", validates: { emoji_only: true } # collective_noun and collective_noun_disabled have been added back temporarily for # a data_update script, but will be removed in a future PR! field :collective_noun, type: :string, default: "Community" @@ -63,9 +68,9 @@ class SiteConfig < RailsSettings::Base field :community_description, type: :string field :community_member_label, type: :string, default: "user" field :tagline, type: :string - field :community_copyright_start_year, type: :integer, - default: ApplicationConfig["COMMUNITY_COPYRIGHT_START_YEAR"] || - Time.zone.today.year + field :community_copyright_start_year, + type: :integer, + default: ApplicationConfig["COMMUNITY_COPYRIGHT_START_YEAR"] || Time.zone.today.year field :staff_user_id, type: :integer, default: 1 field :experience_low, type: :string, default: "Total Newbies" field :experience_high, type: :string, default: "Experienced Users" @@ -95,13 +100,21 @@ class SiteConfig < RailsSettings::Base field :recaptcha_secret_key, type: :string, default: ApplicationConfig["RECAPTCHA_SECRET"] # Images - field :main_social_image, type: :string, default: proc { URL.local_image("social-media-cover.png") } + field :main_social_image, + type: :string, + default: proc { URL.local_image("social-media-cover.png") }, + validates: { format: { with: VALID_URL, message: URL_MESSAGE } } field :favicon_url, type: :string, default: proc { URL.local_image("favicon.ico") } - field :logo_png, type: :string, default: proc { URL.local_image("icon.png") } + field :logo_png, + type: :string, + default: proc { URL.local_image("icon.png") }, + validates: { format: { with: VALID_URL, message: URL_MESSAGE } } field :logo_svg, type: :string - field :secondary_logo_url, type: :string + field :secondary_logo_url, type: :string, validates: { + format: { with: VALID_URL, message: URL_MESSAGE } + } field :left_navbar_svg_icon, type: :string, default: STACK_ICON field :right_navbar_svg_icon, type: :string, default: LIGHTNING_ICON @@ -109,9 +122,14 @@ class SiteConfig < RailsSettings::Base # Mascot field :mascot_user_id, type: :integer, default: nil - field :mascot_image_url, type: :string, default: proc { URL.local_image("mascot.png") } + field :mascot_image_url, + type: :string, + default: proc { URL.local_image("mascot.png") }, + validates: { format: { with: VALID_URL, message: URL_MESSAGE } } field :mascot_image_description, type: :string, default: "The community mascot" - field :mascot_footer_image_url, type: :string + field :mascot_footer_image_url, type: :string, validates: { + format: { with: VALID_URL, message: URL_MESSAGE } + } field :mascot_footer_image_width, type: :integer, default: 52 field :mascot_footer_image_height, type: :integer, default: 120 @@ -140,9 +158,15 @@ class SiteConfig < RailsSettings::Base field :mailchimp_incoming_webhook_secret, type: :string, default: "" # Onboarding - field :onboarding_logo_image, type: :string - field :onboarding_background_image, type: :string - field :onboarding_taskcard_image, type: :string + field :onboarding_logo_image, type: :string, validates: { + format: { with: VALID_URL, message: URL_MESSAGE } + } + field :onboarding_background_image, type: :string, validates: { + format: { with: VALID_URL, message: URL_MESSAGE } + } + field :onboarding_taskcard_image, type: :string, validates: { + format: { with: VALID_URL, message: URL_MESSAGE } + } field :suggested_tags, type: :array, default: %w[] field :suggested_users, type: :array, default: %w[] field :prefer_manual_suggested_users, type: :boolean, default: false @@ -190,7 +214,13 @@ class SiteConfig < RailsSettings::Base field :public, type: :boolean, default: 0 # The default font for all users that have not chosen a custom font yet field :default_font, type: :string, default: "sans_serif" - field :primary_brand_color_hex, type: :string, default: "#3b49df" + field :primary_brand_color_hex, type: :string, default: "#3b49df", validates: { + format: { + with: HEX_COLOR_REGEX, + message: "must be be a 3 or 6 character hex (starting with #)" + }, + color_contrast: true + } field :feed_strategy, type: :string, default: "basic" field :tag_feed_minimum_score, type: :integer, default: 0 field :home_feed_minimum_score, type: :integer, default: 0 diff --git a/app/services/site_configs/upsert.rb b/app/services/site_configs/upsert.rb index 3c620d60b..852beb399 100644 --- a/app/services/site_configs/upsert.rb +++ b/app/services/site_configs/upsert.rb @@ -1,21 +1,6 @@ module SiteConfigs class Upsert EMOJI_ONLY_FIELDS = %w[community_emoji].freeze - IMAGE_FIELDS = - %w[ - main_social_image - logo_png - secondary_logo_url - campaign_sidebar_image - mascot_image_url - mascot_footer_image_url - onboarding_logo_image - onboarding_background_image - onboarding_taskcard_image - ].freeze - - VALID_URL = %r{\A(http|https)://([/|.\w\s-])*.[a-z]{2,5}(:[0-9]{1,5})?(/.*)?\z}.freeze - VALID_DOMAIN = /^[a-zA-Z0-9]{1,61}[a-zA-Z0-9](?:\.[a-zA-Z]{2,})+$/.freeze PARAMS_TO_BE_CLEANED = %i[sidebar_tags suggested_tags suggested_users].freeze @@ -32,16 +17,13 @@ module SiteConfigs end def call - @errors = [] clean_up_params - validate_inputs - validate_emoji - validate_image_urls - return self if @errors.flatten.any? + @errors = [] + upsert_configs + return self if @errors.any? @success = true - upsert_configs after_upsert_tasks self end @@ -61,6 +43,9 @@ module SiteConfigs else SiteConfig.public_send("#{key}=", value.strip) unless value.nil? end + rescue ActiveRecord::RecordInvalid => e + @errors << e.message + next end end @@ -75,27 +60,6 @@ module SiteConfigs @configs[:credit_prices_in_cents]&.transform_values!(&:to_i) end - def validate_inputs - @errors << "Brand color must be darker for accessibility." if brand_contrast_too_low - @errors << "Brand color must be be a 6 character hex (starting with #)." if brand_color_not_hex - @errors << "Allowed emails must be list of domains." if allowed_domains_include_improper_format - end - - def validate_emoji - emoji_params = @configs.slice(*EMOJI_ONLY_FIELDS).to_h - @errors << emoji_params.filter_map do |field, value| - non_emoji_characters = value.downcase.gsub(EmojiRegex::RGIEmoji, "") - "#{field} contains invalid emoji" if non_emoji_characters.present? - end - end - - def validate_image_urls - image_params = @configs.slice(*IMAGE_FIELDS).to_h - @errors << image_params.filter_map do |field, url| - "#{field} must be a valid URL" unless url.blank? || valid_image_url(url) - end - end - def update_enabled_auth_providers(value) enabled_providers = value.split(",").filter_map do |entry| entry unless invalid_provider_entry(entry) @@ -124,30 +88,5 @@ module SiteConfigs Tag.find_or_create_all_with_like_by_name(SiteConfig.suggested_tags + SiteConfig.sidebar_tags) end - - # Validations - def brand_contrast_too_low - hex = @configs[:primary_brand_color_hex] - hex.present? && Color::Accessibility.new(hex).low_contrast? - end - - def brand_color_not_hex - hex = @configs[:primary_brand_color_hex] - hex.present? && !hex.match?(/\A#(\h{6}|\h{3})\z/) - end - - def allowed_domains_include_improper_format - domains = @configs[:allowed_registration_email_domains] - return unless domains - - domains_array = domains.delete(" ").split(",") - valid_domains = domains_array - .select { |d| d.match?(VALID_DOMAIN) } - valid_domains.size != domains_array.size - end - - def valid_image_url(url) - url.match?(VALID_URL) - end end end diff --git a/app/validators/color_contrast_validator.rb b/app/validators/color_contrast_validator.rb new file mode 100644 index 000000000..47e96f2d5 --- /dev/null +++ b/app/validators/color_contrast_validator.rb @@ -0,0 +1,11 @@ +class ColorContrastValidator < ActiveModel::EachValidator + DEFAULT_MESSAGE = "must be darker for accessibility".freeze + + def validate_each(record, attribute, value) + return unless Color::Accessibility.new(value).low_contrast? + + record.errors.add(attribute, (options[:message] || DEFAULT_MESSAGE)) + rescue WCAGColorContrast::InvalidColorError + # nothing to do here, this should be picked up by the format validation + end +end diff --git a/app/validators/emoji_only_validator.rb b/app/validators/emoji_only_validator.rb new file mode 100644 index 000000000..fb4ab4d0d --- /dev/null +++ b/app/validators/emoji_only_validator.rb @@ -0,0 +1,9 @@ +class EmojiOnlyValidator < ActiveModel::EachValidator + DEFAULT_MESSAGE = "contains non-emoji characters or invalid emoji".freeze + + def validate_each(record, attribute, value) + return if value.gsub(EmojiRegex::RGIEmoji, "").blank? + + record.errors.add(attribute, options[:message] || DEFAULT_MESSAGE) + end +end diff --git a/app/validators/valid_domain_csv_validator.rb b/app/validators/valid_domain_csv_validator.rb new file mode 100644 index 000000000..e646eeeb9 --- /dev/null +++ b/app/validators/valid_domain_csv_validator.rb @@ -0,0 +1,10 @@ +class ValidDomainCsvValidator < ActiveModel::EachValidator + DEFAULT_MESSAGE = "must be a comma-separated list of valid domains".freeze + VALID_DOMAIN = /^[a-zA-Z0-9]{1,61}[a-zA-Z0-9](?:\.[a-zA-Z]{2,})+$/.freeze + + def validate_each(record, attribute, value) + return if value.all? { |domain| domain.match?(VALID_DOMAIN) } + + record.errors.add(attribute, options[:message] || DEFAULT_MESSAGE) + end +end diff --git a/spec/models/site_config_spec.rb b/spec/models/site_config_spec.rb index b444ad0da..c08b9d707 100644 --- a/spec/models/site_config_spec.rb +++ b/spec/models/site_config_spec.rb @@ -2,8 +2,98 @@ require "rails_helper" RSpec.describe SiteConfig, type: :model do describe "validations" do - describe "builtin validations" do - it { is_expected.to validate_presence_of(:var) } + describe "validating URLs" do + let(:url_fields) do + %w[ + campaign_sidebar_image main_social_image logo_png secondary_logo_url + mascot_image_url mascot_footer_image_url onboarding_background_image + onboarding_logo_image onboarding_taskcard_image + ] + end + + it "accepts valid URLs" do + url_fields.each do |attribute| + expect do + described_class.public_send("#{attribute}=", "https://example.com") + end.not_to raise_error + end + end + + it "rejects invalid URLs and accepts valid ones", :aggregate_failures do + url_fields.each do |attribute| + expect do + described_class.public_send("#{attribute}=", "example.com") + end.to raise_error(/must be a valid URL/) + end + end + end + + describe "validating domain lists" do + it "allows valid domain lists" do + expect do + described_class.allowed_registration_email_domains = "example.com, example2.com" + end.not_to raise_error + end + + it "rejects invalid domain lists" do + expect do + described_class.allowed_registration_email_domains = "example.com, e.c" + end.to raise_error(/must be a comma-separated list of valid domains/) + end + end + + describe "validating emojis strings" do + it "allows emoji-only strings" do + expect do + described_class.community_emoji = "💯" + end.not_to raise_error + end + + it "rejects non emoji-only strings" do + expect do + described_class.community_emoji = "abc" + end.to raise_error(/contains non-emoji characters or invalid emoji/) + end + end + + describe "validating hex string format" do + it "allows 3 chacter hex strings" do + expect do + described_class.primary_brand_color_hex = "#000" + end.not_to raise_error + end + + it "allows 6 character hex strings" do + expect do + described_class.primary_brand_color_hex = "#000000" + end.not_to raise_error + end + + it "rejects strings without leading #" do + expect do + described_class.primary_brand_color_hex = "000000" + end.to raise_error(/must be be a 3 or 6 character hex \(starting with #\)/) + end + + it "rejects invalid character" do + expect do + described_class.primary_brand_color_hex = "#00000g" + end.to raise_error(/must be be a 3 or 6 character hex \(starting with #\)/) + end + end + + describe "validating color contrast" do + it "allows high enough color contrast" do + expect do + described_class.primary_brand_color_hex = "#000" + end.not_to raise_error + end + + it "rejects too low color contrast" do + expect do + described_class.primary_brand_color_hex = "#fff" + end.to raise_error(/must be darker for accessibility/) + end end end