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 ofdd495a0e19
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 admin012aaf0ba3/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.
This commit is contained in:
parent
6a9e143654
commit
366ff0e76b
|
@ -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
|
||||
|
|
|
@ -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
|
||||
|
|
|
@ -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?
|
||||
|
|
|
@ -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
|
||||
|
|
Loading…
Reference in New Issue