diff --git a/lib/validators/post_validator.rb b/lib/validators/post_validator.rb index 0386a4331ef..2977ae5764a 100644 --- a/lib/validators/post_validator.rb +++ b/lib/validators/post_validator.rb @@ -4,27 +4,27 @@ class PostValidator < ActiveModel::Validator def validate(record) presence(record) - return if record.acting_user.try(:staged?) - if record.acting_user.try(:admin?) && Discourse.static_doc_topic_ids.include?(record.topic_id) - return - end + return if record.acting_user&.staged? + return if record.acting_user&.admin? && Discourse.static_doc_topic_ids.include?(record.topic_id) post_body_validator(record) max_posts_validator(record) - max_mention_validator(record) - max_embedded_media_validator(record) - max_attachments_validator(record) - can_post_links_validator(record) unique_post_validator(record) - force_edit_last_validator(record) + + # These validators might cook the post or do SQL queries. + # So we only run them if the post is otherwise valid. + if record.errors.empty? + max_mention_validator(record) + max_embedded_media_validator(record) + max_attachments_validator(record) + max_links_validator(record) + force_edit_last_validator(record) + end end def presence(post) - unless options[:skip_topic] - post.errors.add(:topic_id, :blank, **options) if post.topic_id.blank? - end - - post.errors.add(:user_id, :blank, **options) if post.new_record? && post.user_id.nil? + post.errors.add(:topic_id, :blank, **options) if !options[:skip_topic] && post.topic_id.blank? + post.errors.add(:user_id, :blank, **options) if post.new_record? && post.user_id.blank? end def post_body_validator(post) @@ -37,31 +37,40 @@ class PostValidator < ActiveModel::Validator def stripped_length(post) range = if private_message?(post) - # private message SiteSetting.private_message_post_length elsif post.is_first_post? || (post.topic.present? && post.topic.posts_count == 0) - # creating/editing first post if post.topic&.featured_link&.present? (0..SiteSetting.max_post_length) else SiteSetting.first_post_length end else - # regular post SiteSetting.post_length end StrippedLengthValidator.validate(post, :raw, post.raw, range) end - def raw_quality(post) - sentinel = TextSentinel.body_sentinel(post.raw, private_message: private_message?(post)) - post.errors.add(:raw, I18n.t(:is_invalid)) unless sentinel.valid? + def max_posts_validator(post) + if post.new_record? && post.acting_user&.posted_too_much_in_topic?(post.topic_id) + post.errors.add( + :base, + I18n.t(:too_many_replies, count: SiteSetting.newuser_max_replies_per_topic), + ) + end + end + + def unique_post_validator(post) + return if SiteSetting.unique_posts_mins == 0 + return if post.skip_unique_check + return if post.acting_user&.staff? + return if post.raw.blank? + + post.errors.add(:raw, I18n.t(:just_posted_that)) if post.matches_recent_post? end - # Ensure maximum amount of mentions in a post def max_mention_validator(post) - return if post.acting_user.try(:staff?) + return if post.acting_user&.staff? if acting_user_is_trusted?(post) || private_message?(post) add_error_if_count_exceeded( @@ -82,19 +91,9 @@ class PostValidator < ActiveModel::Validator end end - def max_posts_validator(post) - if post.new_record? && post.acting_user.present? && - post.acting_user.posted_too_much_in_topic?(post.topic_id) - post.errors.add( - :base, - I18n.t(:too_many_replies, count: SiteSetting.newuser_max_replies_per_topic), - ) - end - end - - # Ensure new users can not put too many media embeds (images, video, audio) in a post def max_embedded_media_validator(post) - return if post.acting_user.blank? || post.acting_user&.staff? + return if post.acting_user.nil? + return if post.acting_user.staff? if !post.acting_user.in_any_groups?(SiteSetting.embedded_media_post_allowed_groups_map) add_error_if_count_exceeded( @@ -115,9 +114,9 @@ class PostValidator < ActiveModel::Validator end end - # Ensure new users can not put too many attachments in a post def max_attachments_validator(post) return if acting_user_is_trusted?(post) || private_message?(post) + add_error_if_count_exceeded( post, :no_attachments_allowed, @@ -127,7 +126,7 @@ class PostValidator < ActiveModel::Validator ) end - def can_post_links_validator(post) + def max_links_validator(post) if (post.link_count == 0 && !post.has_oneboxes?) || private_message?(post) return newuser_links_validator(post) end @@ -140,35 +139,11 @@ class PostValidator < ActiveModel::Validator post.errors.add(:base, I18n.t(:links_require_trust)) end - # Ensure new users can not put too many links in a post - def newuser_links_validator(post) - return if acting_user_is_trusted?(post) || private_message?(post) - add_error_if_count_exceeded( - post, - :no_links_allowed, - :too_many_links, - post.link_count, - SiteSetting.newuser_max_links, - ) - end - - # Stop us from posting the same thing too quickly - def unique_post_validator(post) - return if SiteSetting.unique_posts_mins == 0 - return if post.skip_unique_check - return if post.acting_user.try(:staff?) - - # If the post is empty, default to the validates_presence_of - return if post.raw.blank? - - post.errors.add(:raw, I18n.t(:just_posted_that)) if post.matches_recent_post? - end - def force_edit_last_validator(post) - if SiteSetting.max_consecutive_replies == 0 || post.id || post.acting_user&.staff? || - private_message?(post) - return - end + return if post.id + return if private_message?(post) + return if post.acting_user&.staff? + return if SiteSetting.max_consecutive_replies == 0 topic = post.topic return if topic&.ordered_posts&.first&.user == post.user @@ -207,12 +182,29 @@ class PostValidator < ActiveModel::Validator private + def raw_quality(post) + return if TextSentinel.body_sentinel(post.raw, private_message: private_message?(post)).valid? + post.errors.add(:raw, I18n.t(:is_invalid)) + end + def acting_user_is_trusted?(post, level = 1) - post.acting_user.present? && post.acting_user.has_trust_level?(TrustLevel[level]) + post.acting_user&.has_trust_level?(TrustLevel[level]) end def private_message?(post) - post.topic.try(:private_message?) || options[:private_message] + post.topic&.private_message? || options[:private_message] + end + + def newuser_links_validator(post) + return if acting_user_is_trusted?(post) || private_message?(post) + + add_error_if_count_exceeded( + post, + :no_links_allowed, + :too_many_links, + post.link_count, + SiteSetting.newuser_max_links, + ) end def add_error_if_count_exceeded( diff --git a/lib/validators/stripped_length_validator.rb b/lib/validators/stripped_length_validator.rb index 2cef231b18f..984f0e37da7 100644 --- a/lib/validators/stripped_length_validator.rb +++ b/lib/validators/stripped_length_validator.rb @@ -2,7 +2,7 @@ class StrippedLengthValidator < ActiveModel::EachValidator def self.validate(record, attribute, value, range) - if value.nil? + if value.blank? record.errors.add attribute, I18n.t("errors.messages.blank") elsif value.length > range.end record.errors.add attribute, diff --git a/plugins/poll/lib/poll.rb b/plugins/poll/lib/poll.rb index 37c370c2b5b..f51f43cf9c1 100644 --- a/plugins/poll/lib/poll.rb +++ b/plugins/poll/lib/poll.rb @@ -318,6 +318,9 @@ class DiscoursePoll::Poll # creation process. `raw` could be nil here. return [] if raw.blank? + # bail-out early if the post does not contain a poll + return [] if !raw.include?("[/poll]") + # TODO: we should fix the callback mess so that the cooked version is available # in the validators instead of cooking twice raw = raw.sub(%r{\[quote.+/quote\]}m, "") diff --git a/spec/lib/validators/post_validator_spec.rb b/spec/lib/validators/post_validator_spec.rb index 75962203d7a..256d75d9c74 100644 --- a/spec/lib/validators/post_validator_spec.rb +++ b/spec/lib/validators/post_validator_spec.rb @@ -5,7 +5,7 @@ RSpec.describe PostValidator do let(:post) { build(:post, topic: topic) } let(:validator) { PostValidator.new({}) } - describe "#post_body_validator" do + describe "post_body_validator" do it "should not allow a post with an empty raw" do post.raw = "" validator.post_body_validator(post) @@ -105,7 +105,7 @@ RSpec.describe PostValidator do end end - describe "too_many_posts" do + describe "max_posts_validator" do it "should be invalid when the user has posted too much" do post.user.expects(:posted_too_much_in_topic?).returns(true) validator.max_posts_validator(post) @@ -126,115 +126,6 @@ RSpec.describe PostValidator do end end - describe "too_many_mentions" do - before do - SiteSetting.newuser_max_mentions_per_post = 2 - SiteSetting.max_mentions_per_post = 3 - end - - it "should be invalid when new user exceeds max mentions limit" do - post.acting_user = build(:newuser) - post.expects(:raw_mentions).returns(%w[jake finn jake_old]) - validator.max_mention_validator(post) - expect(post.errors.count).to be > 0 - end - - it "should be invalid when leader user exceeds max mentions limit" do - post.acting_user = build(:trust_level_4) - post.expects(:raw_mentions).returns(%w[jake finn jake_old jake_new]) - validator.max_mention_validator(post) - expect(post.errors.count).to be > 0 - end - - it "should be valid when new user does not exceed max mentions limit" do - post.acting_user = build(:newuser) - post.expects(:raw_mentions).returns(%w[jake finn]) - validator.max_mention_validator(post) - expect(post.errors.count).to be(0) - end - - it "should be valid when new user exceeds max mentions limit in PM" do - post.acting_user = build(:newuser) - post.topic.expects(:private_message?).returns(true) - post.expects(:raw_mentions).returns(%w[jake finn jake_old]) - validator.max_mention_validator(post) - expect(post.errors.count).to be(0) - end - - it "should be valid when leader user does not exceed max mentions limit" do - post.acting_user = build(:trust_level_4) - post.expects(:raw_mentions).returns(%w[jake finn jake_old]) - validator.max_mention_validator(post) - expect(post.errors.count).to be(0) - end - - it "should be valid for moderator in all cases" do - post.acting_user = build(:moderator) - post.expects(:raw_mentions).never - validator.max_mention_validator(post) - expect(post.errors.count).to be(0) - end - - it "should be valid for admin in all cases" do - post.acting_user = build(:admin) - post.expects(:raw_mentions).never - validator.max_mention_validator(post) - expect(post.errors.count).to be(0) - end - end - - describe "too_many_embedded_media" do - fab!(:new_user) { Fabricate(:newuser, refresh_auto_groups: true) } - - before do - SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_0] - SiteSetting.newuser_max_embedded_media = 2 - end - - it "should be invalid when new user exceeds max mentions limit" do - post.acting_user = new_user - post.expects(:embedded_media_count).returns(3) - validator.max_embedded_media_validator(post) - expect(post.errors.count).to be > 0 - end - - it "should be valid when new user does not exceed max mentions limit" do - post.acting_user = new_user - post.expects(:embedded_media_count).returns(2) - validator.max_embedded_media_validator(post) - expect(post.errors.count).to be(0) - end - - it "should be invalid when user trust level is not sufficient" do - SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_4] - post.acting_user = Fabricate(:leader, groups: []) - post.expects(:embedded_media_count).returns(2) - validator.max_embedded_media_validator(post) - expect(post.errors.count).to be > 0 - end - - it "should be valid for moderator in all cases" do - post.acting_user = Fabricate(:moderator) - post.expects(:embedded_media_count).never - validator.max_embedded_media_validator(post) - expect(post.errors.count).to be(0) - end - - it "should be valid for admin in all cases" do - post.acting_user = Fabricate(:admin) - post.expects(:embedded_media_count).never - validator.max_embedded_media_validator(post) - expect(post.errors.count).to be(0) - end - end - - describe "invalid post" do - it "should be invalid" do - validator.validate(post) - expect(post.errors.count).to be > 0 - end - end - describe "unique_post_validator" do fab!(:user) fab!(:post) { Fabricate(:post, raw: "Non PM topic body", user: user, topic: topic) } @@ -319,6 +210,162 @@ RSpec.describe PostValidator do end end + describe "max_mention_validator" do + before do + SiteSetting.newuser_max_mentions_per_post = 2 + SiteSetting.max_mentions_per_post = 3 + end + + it "should be invalid when new user exceeds max mentions limit" do + post.acting_user = build(:newuser) + post.expects(:raw_mentions).returns(%w[jake finn jake_old]) + validator.max_mention_validator(post) + expect(post.errors.count).to be > 0 + end + + it "should be invalid when leader user exceeds max mentions limit" do + post.acting_user = build(:trust_level_4) + post.expects(:raw_mentions).returns(%w[jake finn jake_old jake_new]) + validator.max_mention_validator(post) + expect(post.errors.count).to be > 0 + end + + it "should be valid when new user does not exceed max mentions limit" do + post.acting_user = build(:newuser) + post.expects(:raw_mentions).returns(%w[jake finn]) + validator.max_mention_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be valid when new user exceeds max mentions limit in PM" do + post.acting_user = build(:newuser) + post.topic.expects(:private_message?).returns(true) + post.expects(:raw_mentions).returns(%w[jake finn jake_old]) + validator.max_mention_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be valid when leader user does not exceed max mentions limit" do + post.acting_user = build(:trust_level_4) + post.expects(:raw_mentions).returns(%w[jake finn jake_old]) + validator.max_mention_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be valid for moderator in all cases" do + post.acting_user = build(:moderator) + post.expects(:raw_mentions).never + validator.max_mention_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be valid for admin in all cases" do + post.acting_user = build(:admin) + post.expects(:raw_mentions).never + validator.max_mention_validator(post) + expect(post.errors.count).to be(0) + end + end + + describe "max_embedded_media_validator" do + fab!(:new_user) { Fabricate(:newuser, refresh_auto_groups: true) } + + before do + SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_0] + SiteSetting.newuser_max_embedded_media = 2 + end + + it "should be invalid when new user exceeds max mentions limit" do + post.acting_user = new_user + post.expects(:embedded_media_count).returns(3) + validator.max_embedded_media_validator(post) + expect(post.errors.count).to be > 0 + end + + it "should be valid when new user does not exceed max mentions limit" do + post.acting_user = new_user + post.expects(:embedded_media_count).returns(2) + validator.max_embedded_media_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be invalid when user trust level is not sufficient" do + SiteSetting.embedded_media_post_allowed_groups = Group::AUTO_GROUPS[:trust_level_4] + post.acting_user = Fabricate(:leader, groups: []) + post.expects(:embedded_media_count).returns(2) + validator.max_embedded_media_validator(post) + expect(post.errors.count).to be > 0 + end + + it "should be valid for moderator in all cases" do + post.acting_user = Fabricate(:moderator) + post.expects(:embedded_media_count).never + validator.max_embedded_media_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be valid for admin in all cases" do + post.acting_user = Fabricate(:admin) + post.expects(:embedded_media_count).never + validator.max_embedded_media_validator(post) + expect(post.errors.count).to be(0) + end + end + + describe "max_attachments_validator" do + fab!(:new_user) { Fabricate(:newuser) } + + before { SiteSetting.newuser_max_attachments = 2 } + + it "should be invalid when new user exceeds max attachments limit" do + post.acting_user = new_user + post.expects(:attachment_count).returns(3) + validator.max_attachments_validator(post) + expect(post.errors.count).to be > 0 + end + + it "should be valid when new user does not exceed max attachments limit" do + post.acting_user = new_user + post.expects(:attachment_count).returns(2) + validator.max_attachments_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be valid for moderator in all cases" do + post.acting_user = Fabricate(:moderator) + post.expects(:attachment_count).never + validator.max_attachments_validator(post) + expect(post.errors.count).to be(0) + end + + it "should be valid for admin in all cases" do + post.acting_user = Fabricate(:admin) + post.expects(:attachment_count).never + validator.max_attachments_validator(post) + expect(post.errors.count).to be(0) + end + end + + describe "max_links_validator" do + fab!(:new_user) { Fabricate(:newuser) } + + before { SiteSetting.newuser_max_links = 2 } + + it "should be invalid when new user exceeds max links limit" do + post.acting_user = new_user + post.stubs(:link_count).returns(3) + validator.max_links_validator(post) + expect(post.errors.count).to be > 0 + end + + it "should be valid when new user does not exceed max links limit" do + post.acting_user = new_user + post.stubs(:link_count).returns(2) + validator.max_links_validator(post) + expect(post.errors.count).to be(0) + end + end + describe "force_edit_last_validator" do fab!(:user) { Fabricate(:user, refresh_auto_groups: true) } fab!(:other_user) { Fabricate(:user) } @@ -381,14 +428,13 @@ RSpec.describe PostValidator do shared_examples "almost no validations" do it "skips most validations" do - validator.expects(:stripped_length).never - validator.expects(:raw_quality).never + validator.expects(:post_body_validator).never validator.expects(:max_posts_validator).never + validator.expects(:unique_post_validator).never validator.expects(:max_mention_validator).never validator.expects(:max_embedded_media_validator).never validator.expects(:max_attachments_validator).never - validator.expects(:newuser_links_validator).never - validator.expects(:unique_post_validator).never + validator.expects(:max_links_validator).never validator.expects(:force_edit_last_validator).never validator.validate(post) end