From ff1ac57feb84000a27e2d3a6fedd8495c2e9da7e Mon Sep 17 00:00:00 2001 From: Martin Brennan Date: Fri, 12 Feb 2021 09:05:14 +1000 Subject: [PATCH] FIX: Validate duration minutes values for topic timer (#12040) Add server and client side validations to ensure topic timer durations cannot exceed 2 years and cannot be less than or equal to 0. --- .../app/controllers/edit-topic-timer.js | 27 ++++++++++++++----- app/controllers/topics_controller.rb | 22 ++++++++------- app/models/topic_timer.rb | 19 +++++++++++++ config/locales/client.en.yml | 1 + config/locales/server.en.yml | 3 +++ spec/models/topic_timer_spec.rb | 14 ++++++++++ 6 files changed, 70 insertions(+), 16 deletions(-) diff --git a/app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js b/app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js index 3f154f0b2e6..1bff8157a5c 100644 --- a/app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js +++ b/app/assets/javascripts/discourse/app/controllers/edit-topic-timer.js @@ -121,6 +121,9 @@ export default Controller.extend(ModalFunctionality, { actions: { onChangeStatusType(value) { + if (value !== CLOSE_STATUS_TYPE) { + this.set("topicTimer.based_on_last_post", false); + } this.set("topicTimer.status_type", value); }, @@ -145,14 +148,24 @@ export default Controller.extend(ModalFunctionality, { if ( this.get("topicTimer.duration_minutes") && - !this.get("topicTimer.updateTime") && - this.get("topicTimer.duration_minutes") <= 0 + !this.get("topicTimer.updateTime") ) { - this.flash( - I18n.t("topic.topic_status_update.min_duration"), - "alert-error" - ); - return; + if (this.get("topicTimer.duration_minutes") <= 0) { + this.flash( + I18n.t("topic.topic_status_update.min_duration"), + "alert-error" + ); + return; + } + + // cannot be more than 2 years + if (this.get("topicTimer.duration_minutes") > 2 * 365 * 1440) { + this.flash( + I18n.t("topic.topic_status_update.max_duration"), + "alert-error" + ); + return; + } } this._setTimer( diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 54f29248b65..eaf6789ea2e 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -475,19 +475,23 @@ class TopicsController < ApplicationController options.merge!(duration_minutes: params[:duration_minutes].to_i) if params[:duration_minutes].present? options.merge!(duration: params[:duration].to_i) if params[:duration].present? - topic_status_update = topic.set_or_create_timer( - status_type, - params[:time], - **options - ) + begin + topic_timer = topic.set_or_create_timer( + status_type, + params[:time], + **options + ) + rescue ActiveRecord::RecordInvalid => e + return render_json_error(e.message) + end if topic.save render json: success_json.merge!( - execute_at: topic_status_update&.execute_at, - duration_minutes: topic_status_update&.duration_minutes, - based_on_last_post: topic_status_update&.based_on_last_post, + execute_at: topic_timer&.execute_at, + duration_minutes: topic_timer&.duration_minutes, + based_on_last_post: topic_timer&.based_on_last_post, closed: topic.closed, - category_id: topic_status_update&.category_id + category_id: topic_timer&.category_id ) else render_json_error(topic) diff --git a/app/models/topic_timer.rb b/app/models/topic_timer.rb index 08f3e62e490..d80e8012788 100644 --- a/app/models/topic_timer.rb +++ b/app/models/topic_timer.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class TopicTimer < ActiveRecord::Base + MAX_DURATION_MINUTES = 2.years.to_i / 60 + self.ignored_columns = [ "duration" # TODO(2021-06-01): remove ] @@ -20,6 +22,7 @@ class TopicTimer < ActiveRecord::Base validates :category_id, presence: true, if: :publishing_to_category? validate :executed_at_in_future? + validate :duration_in_range? scope :scheduled_bump_topics, -> { where(status_type: TopicTimer.types[:bump], deleted_at: nil).pluck(:topic_id) } scope :pending_timers, ->(before_time = Time.now.utc) do @@ -134,6 +137,22 @@ class TopicTimer < ActiveRecord::Base private + def duration_in_range? + return if duration_minutes.blank? + + if duration_minutes.to_i <= 0 + errors.add(:duration_minutes, I18n.t( + 'activerecord.errors.models.topic_timer.attributes.duration_minutes.cannot_be_zero' + )) + end + + if duration_minutes.to_i > MAX_DURATION_MINUTES + errors.add(:duration_minutes, I18n.t( + 'activerecord.errors.models.topic_timer.attributes.duration_minutes.exceeds_maximum' + )) + end + end + def executed_at_in_future? return if created_at.blank? || (execute_at > created_at) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index f42b0e029c0..8656d6fa6e8 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2431,6 +2431,7 @@ en: when: "When:" time_frame_required: "Please select a time frame" min_duration: "Duration must be greater than 0" + max_duration: "Duration must be less than 2 years" auto_update_input: none: "Select a timeframe" now: "Now" diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 01ac4bf0f83..0b2af2358b6 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -616,6 +616,9 @@ en: attributes: execute_at: in_the_past: "must be in the future." + duration_minutes: + cannot_be_zero: "must be greater than 0." + exceeds_maximum: "cannot be more than 2 years." translation_overrides: attributes: value: diff --git a/spec/models/topic_timer_spec.rb b/spec/models/topic_timer_spec.rb index a9ad8b39bf7..463b2bda76b 100644 --- a/spec/models/topic_timer_spec.rb +++ b/spec/models/topic_timer_spec.rb @@ -23,6 +23,20 @@ RSpec.describe TopicTimer, type: :model do topic_timer.update!(execute_at: 1.minute.ago, created_at: 10.minutes.ago) expect(TopicTimer.pending_timers.pluck(:id)).to include(topic_timer.id) end + + describe "duration values" do + it "does not allow durations <= 0" do + topic_timer.duration_minutes = -1 + topic_timer.save + expect(topic_timer.errors.full_messages.first).to include("Duration minutes must be greater than 0.") + end + + it "does not allow crazy big durations (2 years in minutes)" do + topic_timer.duration_minutes = 3.years.to_i / 60 + topic_timer.save + expect(topic_timer.errors.full_messages.first).to include("Duration minutes cannot be more than 2 years.") + end + end end describe '#status_type' do it 'should ensure that only one active public topic status update exists' do