From dd495a0e194928fb2a11a14ff9b3403f61df1259 Mon Sep 17 00:00:00 2001
From: Isaac Janzen <50783505+janzenisaac@users.noreply.github.com>
Date: Thu, 20 Apr 2023 09:38:41 -0500
Subject: [PATCH] FEATURE: Allow admins to delete reviewables via API (#21174)

This PR adds the ability to destroy reviewables for a passed user via the API. This was not possible before as this action was reserved for reviewables for you created only.

If a user is an admin and calls the `#destroy` action from the API they are able to destroy a reviewable for a passed user. A user can be targeted by passed either their:
- username
- external_id (for SSO)

to the request.

In the case you attempt to destroy a non-personal reviewable and
- You are not an admin
- You do not access the `#destroy` action via the API

you will raise a `Discourse::InvalidAccess` (403) and will not succeed in destroying the reviewable.
---
 app/controllers/reviewables_controller.rb    | 13 +++-
 lib/guardian.rb                              |  2 +-
 spec/requests/reviewables_controller_spec.rb | 73 +++++++++++++++++++-
 3 files changed, 84 insertions(+), 4 deletions(-)

diff --git a/app/controllers/reviewables_controller.rb b/app/controllers/reviewables_controller.rb
index 68a218100b0..f0f9766691a 100644
--- a/app/controllers/reviewables_controller.rb
+++ b/app/controllers/reviewables_controller.rb
@@ -155,7 +155,18 @@ class ReviewablesController < ApplicationController
   end
 
   def destroy
-    reviewable = Reviewable.find_by(id: params[:reviewable_id], created_by: current_user)
+    user =
+      if is_api?
+        if @guardian.is_admin?
+          fetch_user_from_params
+        else
+          raise Discourse::InvalidAccess
+        end
+      else
+        current_user
+      end
+
+    reviewable = Reviewable.find_by(id: params[:reviewable_id], created_by: user)
     raise Discourse::NotFound.new if reviewable.blank?
 
     reviewable.perform(current_user, :delete)
diff --git a/lib/guardian.rb b/lib/guardian.rb
index de7a380c5d8..6960e461c9c 100644
--- a/lib/guardian.rb
+++ b/lib/guardian.rb
@@ -232,7 +232,7 @@ class Guardian
   end
 
   def can_delete_reviewable_queued_post?(reviewable)
-    reviewable.present? && authenticated? && reviewable.created_by_id == @user.id
+    reviewable.present? && authenticated? && (reviewable.created_by_id == @user.id || @user.admin?)
   end
 
   def can_see_group?(group)
diff --git a/spec/requests/reviewables_controller_spec.rb b/spec/requests/reviewables_controller_spec.rb
index d40131b3d72..853750ad2f6 100644
--- a/spec/requests/reviewables_controller_spec.rb
+++ b/spec/requests/reviewables_controller_spec.rb
@@ -790,25 +790,94 @@ RSpec.describe ReviewablesController do
     describe "#destroy" do
       fab!(:user) { Fabricate(:user) }
 
-      before { sign_in(user) }
-
       it "returns 404 if the reviewable doesn't exist" do
+        sign_in(user)
         delete "/review/1234.json"
         expect(response.code).to eq("404")
       end
 
       it "returns 404 if the user can't see the reviewable" do
+        sign_in(user)
         queued_post = Fabricate(:reviewable_queued_post)
         delete "/review/#{queued_post.id}.json"
         expect(response.code).to eq("404")
       end
 
       it "returns 200 if the user can delete the reviewable" do
+        sign_in(user)
         queued_post = Fabricate(:reviewable_queued_post, created_by: user)
         delete "/review/#{queued_post.id}.json"
         expect(response.code).to eq("200")
         expect(queued_post.reload).to be_deleted
       end
+
+      it "denies attempts to destroy unowned reviewables" do
+        sign_in(admin)
+        queued_post = Fabricate(:reviewable_queued_post, created_by: user)
+        delete "/review/#{queued_post.id}.json"
+        expect(response.status).to eq(404)
+        # Reviewable is not deleted because request is not via API
+        expect(queued_post.reload).to be_present
+      end
+
+      shared_examples "for a passed user" do
+        it "deletes reviewable" do
+          api_key = Fabricate(:api_key).key
+          queued_post = Fabricate(:reviewable_queued_post, created_by: recipient)
+          delete "/review/#{queued_post.id}.json",
+                 params: {
+                   username: recipient.username,
+                 },
+                 headers: {
+                   HTTP_API_USERNAME: caller.username,
+                   HTTP_API_KEY: api_key,
+                 }
+
+          expect(response.status).to eq(response_code)
+
+          if reviewable_deleted
+            expect(queued_post.reload).to be_deleted
+          else
+            expect(queued_post.reload).to be_present
+          end
+        end
+      end
+
+      describe "api called by admin" do
+        include_examples "for a passed user" do
+          let(:caller) { Fabricate(:admin) }
+          let(:recipient) { user }
+          let(:response_code) { 200 }
+          let(:reviewable_deleted) { true }
+        end
+      end
+
+      describe "api called by tl4 user" do
+        include_examples "for a passed user" do
+          let(:caller) { Fabricate(:trust_level_4) }
+          let(:recipient) { user }
+          let(:response_code) { 403 }
+          let(:reviewable_deleted) { false }
+        end
+      end
+
+      describe "api called by regular user" do
+        include_examples "for a passed user" do
+          let(:caller) { user }
+          let(:recipient) { Fabricate(:user) }
+          let(:response_code) { 403 }
+          let(:reviewable_deleted) { false }
+        end
+      end
+
+      describe "api called by admin for another admin" do
+        include_examples "for a passed user" do
+          let(:caller) { Fabricate(:admin) }
+          let(:recipient) { Fabricate(:admin) }
+          let(:response_code) { 200 }
+          let(:reviewable_deleted) { true }
+        end
+      end
     end
 
     describe "#count" do