From 30b97325b158a501e844e52c40e501737b5c2569 Mon Sep 17 00:00:00 2001
From: SamuelMartini <samueljmartini@gmail.com>
Date: Fri, 7 Feb 2020 11:01:07 +0100
Subject: [PATCH] Scope image by product_id or variant_id in ImagesController

The image route is under the product and variant resources:
`/api/products/:product_id/images/:id`
`/api/variants/:variant_id/images/:id`

When reading or perfoming any change on the product/variant image, the controller
should scope the product or the variant when finding the image id.
The risk in only find by image id is to delete or read an image which belongs to
another product.
The route path give an expectation that the controller must meet:
a request to a product or variant image.
---
 .../spree/api/images_controller.rb            |  6 ++--
 .../spree/api/images_controller_spec.rb       | 30 +++++++++++++++++--
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/api/app/controllers/spree/api/images_controller.rb b/api/app/controllers/spree/api/images_controller.rb
index 6c7f736bd7f..c18a9b757d1 100644
--- a/api/app/controllers/spree/api/images_controller.rb
+++ b/api/app/controllers/spree/api/images_controller.rb
@@ -9,7 +9,7 @@ def index
       end
 
       def show
-        @image = Spree::Image.accessible_by(current_ability, :read).find(params[:id])
+        @image = scope.images.accessible_by(current_ability, :read).find(params[:id])
         respond_with(@image)
       end
 
@@ -20,13 +20,13 @@ def create
       end
 
       def update
-        @image = Spree::Image.accessible_by(current_ability, :update).find(params[:id])
+        @image = scope.images.accessible_by(current_ability, :update).find(params[:id])
         @image.update(image_params)
         respond_with(@image, default_template: :show)
       end
 
       def destroy
-        @image = Spree::Image.accessible_by(current_ability, :destroy).find(params[:id])
+        @image = scope.images.accessible_by(current_ability, :destroy).find(params[:id])
         @image.destroy
         respond_with(@image, status: 204)
       end
diff --git a/api/spec/requests/spree/api/images_controller_spec.rb b/api/spec/requests/spree/api/images_controller_spec.rb
index 38f0a538c35..6465005058d 100644
--- a/api/spec/requests/spree/api/images_controller_spec.rb
+++ b/api/spec/requests/spree/api/images_controller_spec.rb
@@ -32,7 +32,7 @@ module Spree
         end.to change(Image, :count).by(1)
       end
 
-      context "working with an existing image" do
+      context "working with an existing product image" do
         let!(:product_image) { product.master.images.create!(attachment: image('thinking-cat.jpg')) }
 
         it "can get a single product image" do
@@ -63,18 +63,42 @@ module Spree
 
         it "can update image data" do
           expect(product_image.position).to eq(1)
-          put spree.api_variant_image_path(product.id, product_image), params: { image: { position: 2 } }
+          put spree.api_variant_image_path(product.master.id, product_image), params: { image: { position: 2 } }
           expect(response.status).to eq(200)
           expect(json_response).to have_attributes(attributes)
           expect(product_image.reload.position).to eq(2)
         end
 
         it "can delete an image" do
-          delete spree.api_variant_image_path(product.id, product_image)
+          delete spree.api_variant_image_path(product.master.id, product_image)
           expect(response.status).to eq(204)
           expect { product_image.reload }.to raise_error(ActiveRecord::RecordNotFound)
         end
       end
+
+      context 'when image belongs to another product' do
+        let!(:product_image) { another_product.master.images.create!(attachment: image('thinking-cat.jpg')) }
+        let(:another_product) { create(:product) }
+
+        it "cannot get an image of another product" do
+          get spree.api_product_image_path(product.id, product_image)
+          expect(response.status).to eq(404)
+          expect(json_response['error']).to eq(I18n.t(:resource_not_found, scope: "spree.api"))
+        end
+
+        it "cannot get an image of another variant" do
+          get spree.api_variant_image_path(product.master.id, product_image)
+          expect(response.status).to eq(404)
+          expect(json_response['error']).to eq(I18n.t(:resource_not_found, scope: "spree.api"))
+        end
+
+        it "cannot update image of another product" do
+          expect(product_image.position).to eq(1)
+          put spree.api_variant_image_path(product.master.id, product_image), params: { image: { position: 2 } }
+          expect(response.status).to eq(404)
+          expect(json_response['error']).to eq(I18n.t(:resource_not_found, scope: "spree.api"))
+        end
+      end
     end
 
     context "as a non-admin" do