diff --git a/app/controllers/concerns/draft_management.rb b/app/controllers/concerns/draft_management.rb new file mode 100644 index 000000000..1a7e80f23 --- /dev/null +++ b/app/controllers/concerns/draft_management.rb @@ -0,0 +1,56 @@ +module DraftManagement + extend ActiveSupport::Concern + + DRAFTABLE_FIELDS = [:body, :comment, :excerpt, :license, :saved_at, :tags, :tag_name, :title].freeze + NESTED_DRAFTABLE_FIELDS = [:body, :saved_at].freeze + TOP_LEVEL_DRAFTABLE_FIELDS = [:body, :comment, :excerpt, :license, :tag_name, :tags, :title].freeze + DRAFT_MAX_AGE = 86_400 * 7 + + # saving by-field is kept for backwards compatibility with old drafts + # @param user [User] user to save the draft for + # @param path [String] draft path to save + # @return [String] top level field key + def do_save_draft(user, path) + base_key = "saved_post.#{user.id}.#{path}" + + TOP_LEVEL_DRAFTABLE_FIELDS.each do |key| + next unless params.key?(key) + + key_name = NESTED_DRAFTABLE_FIELDS.include?(key) ? base_key : "#{base_key}.#{key}" + + if key == :tags + valid_tags = params[key]&.select(&:present?) + + RequestContext.redis.del(key_name) + + if valid_tags.present? + RequestContext.redis.sadd(key_name, valid_tags) + end + else + RequestContext.redis.set(key_name, params[key]) + end + + RequestContext.redis.expire(key_name, DRAFT_MAX_AGE) + end + + saved_at_key = "saved_post_at.#{user.id}.#{path}" + RequestContext.redis.set(saved_at_key, DateTime.now.iso8601) + RequestContext.redis.expire(saved_at_key, DRAFT_MAX_AGE) + + base_key + end + + # Attempts to delete a draft for a given path + # @param user [User] user to delete the draft for + # @param path [String] draft path to delete + # @return [Boolean] status of the operation + def do_delete_draft(user, path) + keys = DRAFTABLE_FIELDS.map do |key| + pfx = key == :saved_at ? 'saved_post_at' : 'saved_post' + base = "#{pfx}.#{user.id}.#{path}" + NESTED_DRAFTABLE_FIELDS.include?(key) ? base : "#{base}.#{key}" + end + + RequestContext.redis.del(*keys) + end +end diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 13a05dcba..f9f7481ae 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -1,6 +1,8 @@ # rubocop:disable Metrics/ClassLength # rubocop:disable Metrics/MethodLength class PostsController < ApplicationController + include DraftManagement + before_action :authenticate_user!, except: [:document, :help_center, :show] before_action :set_post, only: [:toggle_comments, :feature, :lock, :unlock] before_action :set_scoped_post, only: [:change_category, :show, :edit, :update, :close, :reopen, :delete, :restore] @@ -111,7 +113,7 @@ def create Rails.cache.delete "community_user/#{current_user.community_user.id}/metric/#{key}" end - do_draft_delete(URI(request.referer || '').path) + do_delete_draft(current_user, URI(request.referer || '').path) redirect_to helpers.generic_show_link(@post) else @@ -261,7 +263,7 @@ def update PostHistory.redact(@post, current_user) end Rails.cache.delete "community_user/#{current_user.community_user.id}/metric/E" - do_draft_delete(URI(request.referer || '').path) + do_delete_draft(current_user, URI(request.referer || '').path) redirect_to post_path(@post) end @@ -300,7 +302,7 @@ def update message += " on '#{@post.parent.title}'" end @post.user.create_notification message, suggested_edit_url(edit, host: @post.community.host) - do_draft_delete(URI(request.referer || '').path) + do_delete_draft(current_user, URI(request.referer || '').path) redirect_to post_path(@post) else @post.errors.copy!(edit.errors) @@ -631,41 +633,13 @@ def feature render json: { status: 'success', success: true } end - # saving by-field is kept for backwards compatibility with old drafts def save_draft - expiration_time = 86_400 * 7 - - base_key = "saved_post.#{current_user.id}.#{params[:path]}" - - [:body, :comment, :excerpt, :license, :tag_name, :tags, :title].each do |key| - next unless params.key?(key) - - key_name = [:body, :saved_at].include?(key) ? base_key : "#{base_key}.#{key}" - - if key == :tags - valid_tags = params[key]&.select(&:present?) - - RequestContext.redis.del(key_name) - - if valid_tags.present? - RequestContext.redis.sadd(key_name, valid_tags) - end - else - RequestContext.redis.set(key_name, params[key]) - end - - RequestContext.redis.expire(key_name, expiration_time) - end - - saved_at_key = "saved_post_at.#{current_user.id}.#{params[:path]}" - RequestContext.redis.set(saved_at_key, DateTime.now.iso8601) - RequestContext.redis.expire(saved_at_key, expiration_time) - + base_key = do_save_draft(current_user, params[:path]) render json: { status: 'success', success: true, key: base_key } end def delete_draft - do_draft_delete(params[:path]) + do_delete_draft(current_user, params[:path]) render json: { status: 'success', success: true } end @@ -726,19 +700,6 @@ def edit_checks def unless_locked check_if_locked(@post) end - - # Attempts to actually delete a post draft - # @param path [String] draft path to delete - # @return [Boolean] status of the operation - def do_draft_delete(path) - keys = [:body, :comment, :excerpt, :license, :saved_at, :tags, :tag_name, :title].map do |key| - pfx = key == :saved_at ? 'saved_post_at' : 'saved_post' - base = "#{pfx}.#{current_user.id}.#{path}" - [:body, :saved_at].include?(key) ? base : "#{base}.#{key}" - end - - RequestContext.redis.del(*keys) - end end # rubocop:enable Metrics/MethodLength # rubocop:enable Metrics/ClassLength diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index df620a1a7..d25cc90a9 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -1,4 +1,6 @@ class TagsController < ApplicationController + include DraftManagement + before_action :authenticate_user!, only: [:new, :create, :edit, :update, :rename, :merge, :select_merge] before_action :set_category, except: [:index] before_action :set_tag, only: [:show, :edit, :update, :children, :rename, :merge, :select_merge, :nuke, :nuke_warning] @@ -92,8 +94,11 @@ def new end def create - @tag = Tag.new(tag_params.merge(tag_set_id: @category.tag_set.id)) + create_params = tag_params.merge(tag_set_id: @category.tag_set.id) + + @tag = Tag.new(create_params) if @tag.save + do_delete_draft(current_user, URI(request.referer || '').path) redirect_to tag_path(id: @category.id, tag_id: @tag.id) else render :new, status: :bad_request @@ -109,7 +114,11 @@ def update return unless check_your_privilege('edit_tags', nil, true) wiki_md = params[:tag][:wiki_markdown] - if @tag.update(tag_params.merge(wiki: wiki_md.present? ? helpers.render_markdown(wiki_md) : nil).except(:name)) + update_params = tag_params.merge(wiki: wiki_md.present? ? helpers.render_markdown(wiki_md) : nil) + .except(:name) + + if @tag.update(update_params) + do_delete_draft(current_user, URI(request.referer || '').path) redirect_to tag_path(id: @category.id, tag_id: @tag.id) else render :edit, status: :bad_request @@ -263,8 +272,7 @@ def exec_sql(sql_array) end def verify_tag_editor - unless user_signed_in? && (current_user.privilege?(:edit_tags) || - current_user.at_least_moderator?) + unless user_signed_in? && current_user.can_edit_tags? respond_to do |format| format.html do render 'errors/not_found', layout: 'without_sidebar', status: :not_found diff --git a/app/models/user.rb b/app/models/user.rb index c387f1546..58302da07 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -138,6 +138,12 @@ def can_delete?(target) privilege?('flag_curate') && !target.deleted? end + # Can the user edit tags? + # @return [Boolean] check result + def can_edit_tags? + privilege?('edit_tags') || false + end + # Can the user handle flags? # @return [Boolean] check result def can_handle_flags? diff --git a/test/controllers/tags_controller_test.rb b/test/controllers/tags_controller_test.rb index 15e3dd5b0..ce44ea0d3 100644 --- a/test/controllers/tags_controller_test.rb +++ b/test/controllers/tags_controller_test.rb @@ -99,6 +99,54 @@ class TagsControllerTest < ActionController::TestCase assert_not_nil assigns(:posts) end + test ':create should require authentication' do + try_create_tag(categories(:main), tag_sets(:main)) + + assert_redirected_to_sign_in + end + + test 'only users who can edit tags should be able to create them' do + category = categories(:main) + tag_set = tag_sets(:main) + + users.each do |user| + sign_in(user) + + draft_path = create_tag_path(id: category.id) + tag_name = "test-tag-#{user.id}" + + @controller.stub('params', { tag_name: tag_name }) do + @controller.do_save_draft(user, draft_path) + end + + try_create_tag(category, tag_set, name: tag_name) + + if !@controller.helpers.user_signed_in? + assert_redirected_to_sign_in + elsif user.can_edit_tags? + assert_response(:found, "Expected user '#{user.name}' to be able to create tags") + assert_draft_deleted(user, draft_path, :tag_name) + else + assert_response(:not_found, "Expected user '#{user.name}' to not be able to create tags") + end + end + end + + test ':create should correctly create tags' do + sign_in users(:tags_editor) + + category = categories(:main) + tag_set = tag_sets(:main) + tag_name = 'test-tag' + + try_create_tag(category, tag_set, name: tag_name) + + @tag = assigns(:tag) + + assert_not_nil @tag + assert_redirected_to(tag_path(id: category.id, tag_id: @tag.id)) + end + test 'should deny edit to anonymous user' do get :edit, params: { id: categories(:main).id, tag_id: tags(:topic).id } @@ -234,6 +282,23 @@ class TagsControllerTest < ActionController::TestCase private + # @param category [Category] category to create the tag in + # @param tag_set [TagSet] tag set the tag belongs to + def try_create_tag(tag_set, category, **opts) + # this is needed for draft deletion until we switch from request.referer + @request.set_header('HTTP_REFERER', create_tag_path(id: category.id)) + + post :create, params: { + id: category.id, + tag: { + excerpt: 'Usage guidance goes here', + name: 'test-tag', + tag_set_id: tag_set.id, + wiki_markdown: 'Extended tag description goes here' + }.merge(opts) + } + end + # @param category [Category] category to rename the tag in # @param tag [Tag] tag to rename # @param name [String] new tag name diff --git a/test/fixtures/community_users.yml b/test/fixtures/community_users.yml index df9e4877e..95cb84669 100644 --- a/test/fixtures/community_users.yml +++ b/test/fixtures/community_users.yml @@ -26,6 +26,13 @@ sample_editor: is_moderator: false reputation: 501 +sample_tags_editor: + user: tags_editor + community: sample + is_admin: false + is_moderator: false + reputation: 42 + sample_deleter: user: deleter community: sample diff --git a/test/fixtures/user_abilities.yml b/test/fixtures/user_abilities.yml index 094a95ec5..07bc981f8 100644 --- a/test/fixtures/user_abilities.yml +++ b/test/fixtures/user_abilities.yml @@ -10,6 +10,10 @@ e_eo: community_user: sample_editor ability: everyone +et_eo: + community_user: sample_tags_editor + ability: everyone + d_eo: community_user: sample_deleter ability: everyone @@ -38,6 +42,10 @@ e_ur: community_user: sample_editor ability: unrestricted +et_ur: + community_user: sample_tags_editor + ability: unrestricted + d_ur: community_user: sample_deleter ability: unrestricted @@ -78,6 +86,10 @@ d_et: community_user: sample_deleter ability: edit_tags +te_et: + community_user: sample_tags_editor + ability: edit_tags + b_eo: community_user: sample_basic_user ability: everyone diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 140caa683..ed8768f65 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -40,6 +40,15 @@ editor: login_token_expires_at: 2000-01-01T00:00:00.000000Z confirmed_at: 2020-01-01T00:00:00.000000Z +tags_editor: + email: editor@example.com + encrypted_password: '$2a$11$roUHXKxecjyQ72Qn7DWs3.9eRCCoRn176kX/UNb/xiue3aGqf7xEW' + sign_in_count: 1337 + username: tags-editor + is_global_admin: false + is_global_moderator: false + confirmed_at: 2020-01-01T00:00:00.000000Z + deleter: email: delete@qpixel-test.net encrypted_password: '$2a$11$roUHXKxecjyQ72Qn7DWs3.9eRCCoRn176kX/UNb/xiue3aGqf7xEW' diff --git a/test/test_helper.rb b/test/test_helper.rb index 8ee379276..44cebba97 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -129,6 +129,16 @@ def copy_abilities(community_id) end end + def assert_draft_deleted(user, path, *fields) + base_key = "saved_post.#{user.id}.#{path}" + + fields.each do |key| + key_name = DraftManagement::NESTED_DRAFTABLE_FIELDS.include?(key) ? base_key : "#{base_key}.#{key}" + assert_not(RequestContext.redis.exists?(key_name), + "Expected '#{key_name}' draft to be deleted") + end + end + def assert_valid_json_response assert_nothing_raised do parsed = JSON.parse(response.body)