From 19bda2056c3d485f2dc52f0591cdee3c989a53ec Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 23 Dec 2017 13:07:23 -0600 Subject: [PATCH 1/3] tags: update category cache whenever category changes. Do `update_category_cache_for_all` in a callback instead of calling it manually everywhere. --- app/controllers/tags_controller.rb | 1 - app/logical/alias_and_implication_importer.rb | 1 - app/models/tag.rb | 5 +++-- app/models/tag_alias.rb | 1 - test/unit/tag_test.rb | 2 -- 5 files changed, 3 insertions(+), 7 deletions(-) diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index db81aa7b5..ee2136d93 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -36,7 +36,6 @@ class TagsController < ApplicationController @tag = Tag.find(params[:id]) check_privilege(@tag) @tag.update_attributes(params[:tag], :as => CurrentUser.role) - @tag.update_category_cache_for_all respond_with(@tag) end diff --git a/app/logical/alias_and_implication_importer.rb b/app/logical/alias_and_implication_importer.rb index 144210668..4d704d53c 100644 --- a/app/logical/alias_and_implication_importer.rb +++ b/app/logical/alias_and_implication_importer.rb @@ -117,7 +117,6 @@ private tag = Tag.find_by_name(token[1]) tag.category = Tag.categories.value_for(token[2]) tag.save - tag.update_category_cache_for_all else raise "Unknown token: #{token[0]}" diff --git a/app/models/tag.rb b/app/models/tag.rb index 86e4a4278..4a0ef2e28 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -15,6 +15,8 @@ class Tag < ApplicationRecord validates :name, uniqueness: true, tag_name: true, on: :create validates_inclusion_of :category, in: TagCategory.category_ids + after_save :update_category_cache_for_all, if: :category_changed? + module ApiMethods def to_legacy_json return { @@ -221,8 +223,7 @@ class Tag < ApplicationRecord tag.update_category_cache if category_id != tag.category && !tag.is_locked? && ((CurrentUser.is_builder? && tag.post_count < 10_000) || tag.post_count <= 50) - tag.update_column(:category, category_id) - tag.update_category_cache_for_all + tag.update_attribute(:category, category_id) end end diff --git a/app/models/tag_alias.rb b/app/models/tag_alias.rb index b98a0eb55..84dcd4f1f 100644 --- a/app/models/tag_alias.rb +++ b/app/models/tag_alias.rb @@ -173,7 +173,6 @@ class TagAlias < TagRelationship def ensure_category_consistency if antecedent_tag.category != consequent_tag.category && antecedent_tag.category != Tag.categories.general consequent_tag.update_attribute(:category, antecedent_tag.category) - consequent_tag.update_category_cache_for_all end true diff --git a/test/unit/tag_test.rb b/test/unit/tag_test.rb index 21e798b2a..0fd39ab03 100644 --- a/test/unit/tag_test.rb +++ b/test/unit/tag_test.rb @@ -112,11 +112,9 @@ class TagTest < ActiveSupport::TestCase should "reset its category after updating" do tag = FactoryGirl.create(:artist_tag) - tag.update_category_cache_for_all assert_equal(Tag.categories.artist, Cache.get("tc:#{Cache.hash(tag.name)}")) tag.update_attribute(:category, Tag.categories.copyright) - tag.update_category_cache_for_all assert_equal(Tag.categories.copyright, Cache.get("tc:#{Cache.hash(tag.name)}")) end From 2385933e564cf6d2da85fe5627d5faf6de162420 Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 23 Dec 2017 13:16:31 -0600 Subject: [PATCH 2/3] tags: fix /tags/1234/edit not enforcing correct category change restrictions. --- app/models/tag.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/app/models/tag.rb b/app/models/tag.rb index 4a0ef2e28..f7d03c3db 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -202,7 +202,7 @@ class Tag < ApplicationRecord names.map {|x| find_or_create_by_name(x).name} end - def find_or_create_by_name(name, options = {}) + def find_or_create_by_name(name, creator: CurrentUser.user) name = normalize_name(name) category = nil @@ -222,8 +222,8 @@ class Tag < ApplicationRecord # next few lines if the category is changed. tag.update_category_cache - if category_id != tag.category && !tag.is_locked? && ((CurrentUser.is_builder? && tag.post_count < 10_000) || tag.post_count <= 50) - tag.update_attribute(:category, category_id) + if tag.editable_by?(creator) + tag.update(category: category_id) end end @@ -949,7 +949,9 @@ class Tag < ApplicationRecord end def editable_by?(user) - user.is_builder? || (user.is_member? && post_count <= 50) + return true if !is_locked? && user.is_builder? && post_count < 10_000 + return true if !is_locked? && user.is_member? && post_count < 50 + return false end include ApiMethods From 91592b2f18bf40df748f867ae489c2faf67f7e7a Mon Sep 17 00:00:00 2001 From: evazion Date: Sat, 23 Dec 2017 13:19:36 -0600 Subject: [PATCH 3/3] Fix #3448: Lower the limit for tag category changes. Also allow admins to bypass all restrictions. --- app/models/tag.rb | 3 ++- test/functional/tags_controller_test.rb | 8 ++++++++ test/unit/tag_test.rb | 18 ++++++++++++++++-- 3 files changed, 26 insertions(+), 3 deletions(-) diff --git a/app/models/tag.rb b/app/models/tag.rb index f7d03c3db..40d169dcb 100644 --- a/app/models/tag.rb +++ b/app/models/tag.rb @@ -949,7 +949,8 @@ class Tag < ApplicationRecord end def editable_by?(user) - return true if !is_locked? && user.is_builder? && post_count < 10_000 + return true if user.is_admin? + return true if !is_locked? && user.is_builder? && post_count < 1_000 return true if !is_locked? && user.is_member? && post_count < 50 return false end diff --git a/test/functional/tags_controller_test.rb b/test/functional/tags_controller_test.rb index b6cebd20e..7bfc3f624 100644 --- a/test/functional/tags_controller_test.rb +++ b/test/functional/tags_controller_test.rb @@ -73,6 +73,14 @@ class TagsControllerTest < ActionController::TestCase @tag.reload assert_equal(1, @tag.category) end + + should "not change category when the tag is too large to be changed by a builder" do + @tag.update_columns(post_count: 1001) + post :update, {:id => @tag.id, :tag => {:category => "1"}}, {:user_id => @user.id} + + assert_response :forbidden + assert_equal(0, @tag.reload.category) + end end end end diff --git a/test/unit/tag_test.rb b/test/unit/tag_test.rb index 0fd39ab03..0a1b32807 100644 --- a/test/unit/tag_test.rb +++ b/test/unit/tag_test.rb @@ -2,8 +2,8 @@ require 'test_helper' class TagTest < ActiveSupport::TestCase setup do - user = FactoryGirl.create(:builder_user) - CurrentUser.user = user + @builder = FactoryGirl.create(:builder_user) + CurrentUser.user = @builder CurrentUser.ip_addr = "127.0.0.1" end @@ -206,6 +206,20 @@ class TagTest < ActiveSupport::TestCase assert_equal(0, tag.category) end + should "not change category when the tag is too large to be changed by a builder" do + tag = FactoryGirl.create(:tag, post_count: 1001) + Tag.find_or_create_by_name("artist:#{tag.name}", creator: @builder) + + assert_equal(0, tag.reload.category) + end + + should "not change category when the tag is too large to be changed by a member" do + tag = FactoryGirl.create(:tag, post_count: 51) + Tag.find_or_create_by_name("artist:#{tag.name}", creator: FactoryGirl.create(:member_user)) + + assert_equal(0, tag.reload.category) + end + should "be created when one doesn't exist" do assert_difference("Tag.count", 1) do tag = Tag.find_or_create_by_name("hoge")