From 366ff0e76b37ba489bc5297eddb9b537ebcccba9 Mon Sep 17 00:00:00 2001 From: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com> Date: Mon, 24 Apr 2023 20:22:37 -0500 Subject: [PATCH] FIX: Don't display destroy reviewable button on client (#21226) # Context https://meta.discourse.org/t/missing-translate-in-review-page/262604 ![image](https://user-images.githubusercontent.com/50783505/234089049-72332040-e7d5-4081-824a-b0b36e37187a.png) An additional button was added as a result of https://github.com/discourse/discourse/commit/dd495a0e194928fb2a11a14ff9b3403f61df1259 which was intended to grant access to deleting reviewable from the API. We were being too flexible by only checking if the user was an admin https://github.com/discourse/discourse/blob/012aaf0ba32890a93f913b469681792341198ecc/lib/guardian.rb#L237 where it should instead by scoped to check if the request was an API call. # Fix https://github.com/discourse/discourse/pull/21226/files#diff-0a2548be4b18bd4ef2dffb3ef8e44984d2fef7f037b53e98f67abea52ef75aa2R237 # Additions Added a new guard of `is_api?` https://github.com/discourse/discourse/pull/21226/files#diff-0a2548be4b18bd4ef2dffb3ef8e44984d2fef7f037b53e98f67abea52ef75aa2R657-R660 In `app/models/reviewable.rb` we check if the user has the permissions to the destroy action via the `Guardian`. To do this we were instantiating a new `Guardian` class which then caused us to lose the context of the request. The request is a necessary component in the guard of `is_api?` so we needed to pass the already defined Guardian from the `app/controllers/reviewables_controller.rb` to the `#perform` method to ensure the request is present. --- app/controllers/reviewables_controller.rb | 2 +- app/models/reviewable.rb | 2 +- lib/guardian.rb | 6 ++- spec/lib/guardian_spec.rb | 56 +++++++++++++++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb index f0f9766691a..112e8980929 100644 --- a/app/controllers/reviewables_controller.rb +++ b/app/controllers/reviewables_controller.rb @@ -169,7 +169,7 @@ class ReviewablesController < ApplicationController reviewable = Reviewable.find_by(id: params[:reviewable_id], created_by: user) raise Discourse::NotFound.new if reviewable.blank? - reviewable.perform(current_user, :delete) + reviewable.perform(current_user, :delete, { guardian: @guardian }) render json: success_json end diff --git a/app/models/reviewable.rb b/app/models/reviewable.rb index c159967014f..e1406627a25 100644 --- a/app/models/reviewable.rb +++ b/app/models/reviewable.rb @@ -314,7 +314,7 @@ class Reviewable < ActiveRecord::Base valid = [action_id, aliases.to_a.select { |k, v| v == action_id }.map(&:first)].flatten # Ensure the user has access to the action - actions = actions_for(Guardian.new(performed_by), args) + actions = actions_for(args[:guardian] || Guardian.new(performed_by), args) raise InvalidAction.new(action_id, self.class) unless valid.any? { |a| actions.has?(a) } perform_method = "perform_#{aliases[action_id] || action_id}".to_sym diff --git a/lib/guardian.rb b/lib/guardian.rb index ed862df604d..d4323575951 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -234,7 +234,7 @@ class Guardian def can_delete_reviewable_queued_post?(reviewable) return false if reviewable.blank? return false if !authenticated? - return true if @user.admin? + return true if is_api? && is_admin? reviewable.created_by_id == @user.id end @@ -654,6 +654,10 @@ class Guardian end end + def is_api? + @user && request&.env&.dig(Auth::DefaultCurrentUserProvider::API_KEY_ENV) + end + protected def category_group_moderation_allowed? diff --git a/spec/lib/guardian_spec.rb b/spec/lib/guardian_spec.rb index 6207bce5baa..1d175a34f03 100644 --- a/spec/lib/guardian_spec.rb +++ b/spec/lib/guardian_spec.rb @@ -4194,4 +4194,60 @@ RSpec.describe Guardian do expect(guardian.is_category_group_moderator?(category)).to eq(false) end end + + describe "#can_delete_reviewable_queued_post" do + context "when attempting to destroy a non personal reviewable" do + it "returns true for api requests from admins" do + api_key = Fabricate(:api_key).key + queued_post = Fabricate(:reviewable_queued_post, created_by: user) + opts = { + "HTTP_API_USERNAME" => admin.username, + "HTTP_API_KEY" => api_key, + "REQUEST_METHOD" => "DELETE", + "#{Auth::DefaultCurrentUserProvider::API_KEY_ENV}" => "foo", + } + + env = create_request_env(path: "/review/#{queued_post.id}.json").merge(opts) + guardian = Guardian.new(admin, ActionDispatch::Request.new(env)) + expect(guardian.can_delete_reviewable_queued_post?(queued_post)).to eq(true) + end + + it "returns false for admin requests not via the API" do + queued_post = Fabricate(:reviewable_queued_post, created_by: user) + env = + create_request_env(path: "/review/#{queued_post.id}.json").merge( + { "REQUEST_METHOD" => "DELETE" }, + ) + guardian = Guardian.new(admin, ActionDispatch::Request.new(env)) + expect(guardian.can_delete_reviewable_queued_post?(queued_post)).to eq(false) + end + + it "returns false for api requests from tl4 users" do + api_key = Fabricate(:api_key).key + queued_post = Fabricate(:reviewable_queued_post, created_by: user) + opts = { + "HTTP_API_USERNAME" => trust_level_4.username, + "HTTP_API_KEY" => api_key, + "REQUEST_METHOD" => "DELETE", + "#{Auth::DefaultCurrentUserProvider::API_KEY_ENV}" => "foo", + } + + env = create_request_env(path: "/review/#{queued_post.id}.json").merge(opts) + guardian = Guardian.new(trust_level_4, ActionDispatch::Request.new(env)) + expect(guardian.can_delete_reviewable_queued_post?(queued_post)).to eq(false) + end + end + + context "when attempting to destroy your own reviewable" do + it "returns true" do + queued_post = Fabricate(:reviewable_queued_post, created_by: user) + env = + create_request_env(path: "/review/#{queued_post.id}.json").merge( + { "REQUEST_METHOD" => "DELETE" }, + ) + guardian = Guardian.new(user, ActionDispatch::Request.new(env)) + expect(guardian.can_delete_reviewable_queued_post?(queued_post)).to eq(true) + end + end + end end