From 14cdb0125410e8e5467bc447e14562625a3c076e Mon Sep 17 00:00:00 2001 From: Penar Musaraj <pmusaraj@gmail.com> Date: Wed, 21 Aug 2019 16:33:01 -0400 Subject: [PATCH] FIX: Allow topic edits when using a hidden tag Previously, a regular user could not edit the title or category of a topic if a hidden tag had already been applied. This also stops hidden tag names from leaking in the error message. --- app/controllers/topics_controller.rb | 16 +++++++++- config/locales/server.en.yml | 3 +- spec/requests/topics_controller_spec.rb | 41 ++++++++++++++++++++++++- 3 files changed, 57 insertions(+), 3 deletions(-) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 1c6a905d39a..f3f37bfcbba 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -320,8 +320,22 @@ class TopicsController < ApplicationController ).pluck("tags.name") invalid_tags = topic_tags - allowed_tags + + # Do not raise an error on a topic's hidden tags when not modifying tags + if params[:tags].blank? + invalid_tags.each do |tag_name| + if DiscourseTagging.hidden_tag_names.include?(tag_name) + invalid_tags.delete(tag_name) + end + end + end + if !invalid_tags.empty? - return render_json_error(I18n.t('category.errors.disallowed_topic_tags', tags: invalid_tags.join(", "))) + if (invalid_tags & DiscourseTagging.hidden_tag_names).present? + return render_json_error(I18n.t('category.errors.disallowed_tags_generic')) + else + return render_json_error(I18n.t('category.errors.disallowed_topic_tags', tags: invalid_tags.join(", "))) + end end end end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index da087d53e09..cdfb2bfaf30 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -609,6 +609,7 @@ en: description_incomplete: "The category description post must have at least one paragraph." permission_conflict: "Any group that is allowed to access a subcategory must also be allowed to access the parent category. The following groups have access to one of the subcategories, but no access to parent category: %{group_names}." disallowed_topic_tags: "This topic has tags not allowed by this category: '%{tags}'" + disallowed_tags_generic: "This topic has disallowed tags." cannot_delete: uncategorized: "This category is special. It is intended as a holding area for topics that have no category; it cannot be deleted." has_subcategories: "Can't delete this category because it has sub-categories." @@ -2227,7 +2228,7 @@ en: errors: "%{errors}" not_available: "Not available. Try %{suggestion}?" something_already_taken: "Something went wrong, perhaps the username or email is already registered. Try the forgot password link." - omniauth_error: + omniauth_error: generic: "Sorry, there was an error authorizing your account. Please try again." csrf_detected: "Authorization timed out, or you have switched browsers. Please try again." request_error: "An error occured when starting authorization. Please try again." diff --git a/spec/requests/topics_controller_spec.rb b/spec/requests/topics_controller_spec.rb index 864931ef12e..5ea6b9b6f04 100644 --- a/spec/requests/topics_controller_spec.rb +++ b/spec/requests/topics_controller_spec.rb @@ -1138,16 +1138,55 @@ RSpec.describe TopicsController do restricted_category.allowed_tags = [tag2.name] put "/t/#{topic.slug}/#{topic.id}.json", params: { - tags: [tag2], + tags: [tag2.name], category_id: category.id } result = ::JSON.parse(response.body) expect(response.status).to eq(422) expect(result['errors']).to be_present + expect(result['errors'][0]).to include(tag2.name) expect(topic.reload.category_id).not_to eq(restricted_category.id) end + it 'allows category change when topic has a hidden tag' do + Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag1.name]) + + put "/t/#{topic.slug}/#{topic.id}.json", params: { + category_id: category.id + } + + result = ::JSON.parse(response.body) + expect(response.status).to eq(200) + expect(topic.reload.tags).to include(tag1) + end + + it 'allows category change when topic has a read-only tag' do + Fabricate(:tag_group, permissions: { "staff" => 1, "everyone" => 3 }, tag_names: [tag1.name]) + + put "/t/#{topic.slug}/#{topic.id}.json", params: { + category_id: category.id + } + + result = ::JSON.parse(response.body) + expect(response.status).to eq(200) + expect(topic.reload.tags).to include(tag1) + end + + it 'does not leak tag name when trying to use a staff tag' do + Fabricate(:tag_group, permissions: { "staff" => 1 }, tag_names: [tag2.name]) + + put "/t/#{topic.slug}/#{topic.id}.json", params: { + tags: [tag2.name], + category_id: category.id + } + + result = ::JSON.parse(response.body) + expect(response.status).to eq(422) + expect(result['errors']).to be_present + expect(result['errors'][0]).not_to include(tag2.name) + end + it 'will clean tag params' do restricted_category.allowed_tags = [tag2.name]