Merge pull request #258 from sharetribe/fix-incorrect-authorization-c…

…heck

Fix incorrect authorization check

app/controllers/int_api/listing/blocked_dates_controller.rb

@@ -1,13 +1,21 @@
 class IntApi::Listing::BlockedDatesController < ApplicationController
   respond_to :json
 
+  before_action :ensure_current_user_is_listing_author
+
   def index
     respond_with listing.blocked_dates.in_period(params[:start_on], params[:end_on]), location: nil
   end
 
   private
 
   def listing
-    @listing ||= Listing.find(params[:listing_id])
+    @listing ||= @current_community.listings.find(params[:listing_id])
+  end
+
+  def ensure_current_user_is_listing_author
+    return true if !listing.deleted? && (current_user?(listing.author) || @current_user.has_admin_rights?(@current_community))
+
+    head(:forbidden)
   end
 end

app/controllers/int_api/listing/bookings_controller.rb

@@ -1,13 +1,21 @@
 class IntApi::Listing::BookingsController < ApplicationController
   respond_to :json
 
+  before_action :ensure_current_user_is_listing_author
+
   def index
     respond_with listing.booked_dates(params[:start_on].to_date, params[:end_on].to_date).sort, location: nil
   end
 
   private
 
   def listing
-    @listing ||= Listing.find(params[:listing_id])
+    @listing ||= @current_community.listings.find(params[:listing_id])
+  end
+
+  def ensure_current_user_is_listing_author
+    return true if !listing.deleted? && (current_user?(listing.author) || @current_user.has_admin_rights?(@current_community))
+
+    head(:forbidden)
   end
 end

app/controllers/int_api/listings_controller.rb

@@ -17,7 +17,7 @@ def update_blocked_dates
   private
 
   def listing
-    @listing ||= Listing.find(params[:id])
+    @listing ||= @current_community.listings.find(params[:id])
   end
 
   def working_time_slots_params
@@ -33,8 +33,8 @@ def blocked_dates_params
   end
 
   def ensure_current_user_is_listing_author
-    return true if current_user?(listing.author) || @current_user.has_admin_rights?(@current_community)
+    return true if !listing.deleted? && (current_user?(listing.author) || @current_user.has_admin_rights?(@current_community))
 
-    head(403)
+    head(:forbidden)
   end
 end

app/controllers/listings_controller.rb

@@ -57,7 +57,7 @@ def index
 
   def listing_bubble
     if params[:id]
-      @listing = Listing.find(params[:id])
+      @listing = @current_community.listings.find(params[:id])
       if Policy::ListingPolicy.new(@listing, @current_community, @current_user).visible?
         render :partial => "homepage/listing_bubble", :locals => { :listing => @listing }
       else
@@ -277,8 +277,11 @@ def delete
   end
 
   def ensure_current_user_is_listing_author(error_message)
-    @listing = Listing.find(params[:id])
-    return if current_user?(@listing.author) || @current_user.has_admin_rights?(@current_community)
+    @listing = @current_community.listings.find(params[:id])
+
+    raise ListingDeleted if @listing.deleted?
+
+    return if @listing && (current_user?(@listing.author) || @current_user.has_admin_rights?(@current_community))
 
     flash[:error] = error_message
     redirect_to @listing and return

app/presenters/listings_feed_presenter.rb

@@ -42,7 +42,7 @@ def search_listings(community, params)
   end
 
   def build_title(community, params)
-    category = Category.find_by_id(params["category"])
+    category = community.categories.find_by_id(params["category"])
     category_label = (category.present? ? "(" + localized_category_label(category) + ")" : "")
 
     listing_type_label = if ["request","offer"].include? params['share_type']

spec/controllers/int_api/listings_controller_spec.rb

@@ -4,16 +4,16 @@
   render_views
 
   let(:community) { FactoryGirl.create(:community) }
-  let(:listing) { FactoryGirl.create(:listing) }
-
-  before(:each) do
-    @request.host = "#{community.ident}.lvh.me"
-    @request.env[:current_marketplace] = community
-    user = create_admin_for(community)
-    sign_in_for_spec(user)
-  end
+  let(:listing) { FactoryGirl.create(:listing, community_id: community.id) }
 
   describe "#update_working_time_slots" do
+    before(:each) do
+      @request.host = "#{community.ident}.lvh.me"
+      @request.env[:current_marketplace] = community
+      user = create_admin_for(community)
+      sign_in_for_spec(user)
+    end
+
     it 'works' do
       expect(listing.working_time_slots.count).to eq 0
       listing_params = {
@@ -43,8 +43,8 @@
       expect(listing.working_time_slots.count).to eq 0
       working_time_slot = JSON.parse(response.body)["working_time_slots"].first
       expect(working_time_slot["errors"]).to eq({
-        "from" => ["\"Start time\" must be less than \"End time\""], "till"=>["\"Start time\" must be less than \"End time\""]
-      })
+                                                  "from" => ["\"Start time\" must be less than \"End time\""], "till"=>["\"Start time\" must be less than \"End time\""]
+                                                })
     end
   end
 
@@ -53,6 +53,13 @@
       FactoryGirl.create(:listing_blocked_date, listing_id: listing.id, blocked_at: '2020-01-01')
     end
 
+    before(:each) do
+      @request.host = "#{community.ident}.lvh.me"
+      @request.env[:current_marketplace] = community
+      user = create_admin_for(community)
+      sign_in_for_spec(user)
+    end
+
     it 'works' do
       expect(listing.blocked_dates.count).to eq 0
       listing_params = {
@@ -82,4 +89,47 @@
       expect(listing.blocked_dates.count).to eq 1
     end
   end
+
+  describe 'authorization' do
+    let(:other_community) { FactoryGirl.create(:community) }
+
+    before(:each) do
+      @request.host = "#{other_community.ident}.lvh.me"
+      @request.env[:current_marketplace] = other_community
+      user = create_admin_for(other_community)
+      sign_in_for_spec(user)
+    end
+
+    it '#update_working_time_slots' do
+      expect(listing.working_time_slots.count).to eq 0
+      listing_params = {
+        listing: {
+          working_time_slots_attributes: {
+            id: nil, from: '09:00', till: '17:00', week_day: 'mon'
+          }
+        }
+      }
+      expect {
+        get :update_working_time_slots, params: {id: listing.id, format: :json}.merge(listing_params)
+      }.to raise_error(ActiveRecord::RecordNotFound)
+      listing.reload
+      expect(listing.working_time_slots.count).to eq 0
+    end
+
+    it '#update_blocked_dates' do
+      expect(listing.blocked_dates.count).to eq 0
+      listing_params = {
+        listing: {
+          blocked_dates_attributes: {
+            id: nil, blocked_at: '2020-01-01'
+          }
+        }
+      }
+      expect {
+        get :update_blocked_dates, params: {id: listing.id, format: :json}.merge(listing_params)
+      }.to raise_error(ActiveRecord::RecordNotFound)
+      listing.reload
+      expect(listing.blocked_dates.count).to eq 0
+    end
+  end
 end

spec/controllers/listings_controller_spec.rb

@@ -514,7 +514,7 @@ def create_shape(community_id, type, process, translations = [], categories = []
     end
   end
 
-  describe "delete" do
+  describe "update and delete" do
     let(:community){ FactoryGirl.create(:community, :settings => {"locales" => ["en", "fi"]}) }
     let(:offer_process) {
       FactoryGirl.create(:transaction_process,
@@ -537,17 +537,64 @@ def create_shape(community_id, type, process, translations = [], categories = []
                          price: Money.new(4567, "USD")
                         )
     }
-
-    before :each do
-      @request.host = "#{community.ident}.lvh.me"
-      @request.env[:current_marketplace] = community
+    let(:other_community) { FactoryGirl.create(:community) }
+
+    context 'allowed' do
+      before :each do
+        @request.host = "#{community.ident}.lvh.me"
+        @request.env[:current_marketplace] = community
+      end
+
+      it 'author updates listing' do
+        sign_in_for_spec(person)
+        patch :update, params: {
+                id: listing.id,
+                listing: {
+                  title: 'not a bike',
+                  listing_shape_id: listing.listing_shape_id,
+                  price: Money.new(4567, "USD"),
+                  unit: "{\"unit_type\":\"unit\",\"kind\":\"quantity\",\"quantity_selector\":\"number\"}"
+                }}
+        listing.reload
+        expect(listing.title).to eq 'not a bike'
+      end
+
+      it 'author deletes listing' do
+        sign_in_for_spec(person)
+        delete :delete, params: {id: listing.id}
+        listing.reload
+        expect(listing.deleted).to eq true
+      end
     end
 
-    it 'author deletes listing' do
-      sign_in_for_spec(person)
-      delete :delete, params: {id: listing.id}
-      listing.reload
-      expect(listing.deleted).to eq true
+    context 'other community' do
+      before :each do
+        @request.host = "#{other_community.ident}.lvh.me"
+        @request.env[:current_marketplace] = other_community
+      end
+
+      it 'does not allow updating listing in another community' do
+        sign_in_for_spec(create_admin_for(other_community))
+        expect {
+        patch :update, params: {
+                id: listing.id,
+                listing: {
+                  title: 'not a bike',
+                  listing_shape_id: listing.listing_shape_id,
+                  price: Money.new(4567, "USD"),
+                  unit: "{\"unit_type\":\"unit\",\"kind\":\"quantity\",\"quantity_selector\":\"number\"}"
+                }}
+        }.to raise_error(ActiveRecord::RecordNotFound)
+        listing.reload
+        expect(listing.title).to eq 'bike'
+      end
+
+      it 'does not allow deleting listing in another community' do
+        sign_in_for_spec(create_admin_for(other_community))
+        expect{delete :delete, params: {id: listing.id}}.to raise_error(ActiveRecord::RecordNotFound)
+        listing.reload
+        expect(listing.deleted).to eq false
+      end
     end
   end
 end