API: Access unpublished listings only if you are the owner (#6458) [deploy]

* Access only unpublished listings if they belong to the user when authenticated

* Make sure we never send unpublished listings in the index

* Update API docs

* oops

* Remove unused preloading as this loads a single item

app/controllers/api/v0/api_controller.rb

@@ -40,7 +40,8 @@ class Api::V0::ApiController < ApplicationController
       @user = User.find(doorkeeper_token.resource_owner_id)
       return error_unauthorized unless @user
     elsif request.headers["api-key"]
-      authenticate_with_api_key!
+      @user = authenticate_with_api_key
+      return error_unauthorized unless @user
     elsif current_user
       @user = current_user
     else
@@ -48,29 +49,30 @@ class Api::V0::ApiController < ApplicationController
     end
   end
 
+  # Checks if the user is authenticated, sets @user to nil otherwise
+  def authenticate_with_api_key_or_current_user
+    @user = authenticate_with_api_key || current_user
+  end
+
+  # Checks if the user is authenticated, if so sets the variable @user
+  # Returns HTTP 401 Unauthorized otherwise
   def authenticate_with_api_key_or_current_user!
-    if request.headers["api-key"]
-      authenticate_with_api_key!
-    elsif current_user
-      @user = current_user
-    else
-      error_unauthorized
-    end
+    @user = authenticate_with_api_key || current_user
+    error_unauthorized unless @user
   end
 
-  def authenticate_with_api_key!
+  private
+
+  def authenticate_with_api_key
     api_key = request.headers["api-key"]
-    return error_unauthorized unless api_key
+    return nil unless api_key
 
     api_secret = ApiSecret.includes(:user).find_by(secret: api_key)
-    return error_unauthorized unless api_secret
+    return nil unless api_secret
 
     # guard against timing attacks
     # see <https://www.slideshare.net/NickMalcolm/timing-attacks-and-ruby-on-rails>
-    if ActiveSupport::SecurityUtils.secure_compare(api_secret.secret, api_key)
-      @user = api_secret.user
-    else
-      error_unauthorized
-    end
+    secure_secret = ActiveSupport::SecurityUtils.secure_compare(api_secret.secret, api_key)
+    return api_secret.user if secure_secret
   end
 end

app/controllers/api/v0/classified_listings_controller.rb

@@ -5,6 +5,7 @@ module Api
       include ClassifiedListingsToolkit
 
       before_action :authenticate_with_api_key_or_current_user!, only: %i[create update]
+      before_action :authenticate_with_api_key_or_current_user, only: %i[show]
 
       before_action :set_classified_listing, only: %i[update]
 
@@ -30,10 +31,13 @@ module Api
       end
 
       def show
-        @classified_listing = ClassifiedListing.
-          select(ATTRIBUTES_FOR_SERIALIZATION).
-          includes(:user, :organization).
-          find(params[:id])
+        relation = ClassifiedListing.published
+
+        # if the user is authenticated we allow them to access
+        # their own unpublished listings as well
+        relation = relation.union(@user.classified_listings) if @user
+
+        @classified_listing = relation.select(ATTRIBUTES_FOR_SERIALIZATION).find(params[:id])
 
         set_surrogate_key_header @classified_listing.record_key
       end

docs/api_v0.yml

@@ -8,7 +8,7 @@ info:
 
     Dates and date times, unless otherwise specified, must be in
     the [RFC 3339](https://tools.ietf.org/html/rfc3339) format.
-  version: "0.6.1"
+  version: "0.6.2"
   termsOfService: https://dev.to/terms
   contact:
     name: DEV Team
@@ -1819,6 +1819,9 @@ paths:
       description: |
         This endpoint allows the client to retrieve
         a single listing given its `id`.
+
+        An unpublished listing is only accessible if authentication is supplied
+        and it belongs to the authenticated user.
       tags:
         - listings
       parameters:
@@ -1849,11 +1852,18 @@ paths:
               examples:
                 article-not-found:
                   $ref: "#/components/examples/ErrorNotFound"
+      security:
+        - {} # this indicates that authentication is optional
+        - api_key: []
       x-code-samples: # https://github.com/Redocly/redoc/blob/master/docs/redoc-vendor-extensions.md#x-code-samples
         - lang: Shell
           label: curl
           source: |
             curl https://dev.to/api/listings/1184
+        - lang: Shell
+          label: curl (with authentication)
+          source: |
+            curl -H "api-key: API_KEY" https://dev.to/api/listings/1185
     put:
       operationId: updateListing
       summary: Update a listing

spec/requests/api/v0/classified_listings_spec.rb

@@ -41,6 +41,12 @@ RSpec.describe "Api::V0::Listings" do
     end
   end
 
+  def user_admin_organization(user)
+    org = create(:organization)
+    create(:organization_membership, user_id: user.id, organization_id: org.id, type_of_user: "admin")
+    org
+  end
+
   describe "GET /api/listings" do
     include_context "with 7 listings and 2 user"
 
@@ -83,6 +89,14 @@ RSpec.describe "Api::V0::Listings" do
       ).to_set
       expect(response.headers["surrogate-key"].split.to_set).to eq(expected_key)
     end
+
+    it "does not return unpublished listings" do
+      listing = user1.classified_listings.last
+      listing.update(published: false)
+
+      get api_classified_listings_path
+      expect(response.parsed_body.detect { |l| l["published"] == false }).to be_nil
+    end
   end
 
   describe "GET /api/listings/category/:category" do
@@ -93,13 +107,91 @@ RSpec.describe "Api::V0::Listings" do
       expect(response).to have_http_status(:ok)
       expect(response.parsed_body.size).to eq(3)
     end
+
+    it "does not return unpublished listings" do
+      category = "cfp"
+      listing = user1.classified_listings.where(category: category)
+      listing.update(published: false)
+
+      get api_classified_listings_category_path(category)
+      expect(response.parsed_body.detect { |l| l["published"] == false }).to be_nil
+    end
   end
 
   describe "GET /api/listings/:id" do
     include_context "with 7 listings and 2 user"
     let(:listing) { ClassifiedListing.where(category: "cfp").last }
 
-    it "displays a unique listing" do
+    context "when unauthenticated" do
+      it "returns a published listing" do
+        listing.update(published: true)
+
+        get api_classified_listing_path(listing.id)
+        expect(response).to have_http_status(:ok)
+      end
+
+      it "returns a published listing on behalf of an organization" do
+        org = user_admin_organization(listing.user)
+        listing.update(published: true, organization: org)
+
+        get api_classified_listing_path(listing.id)
+        expect(response).to have_http_status(:ok)
+      end
+
+      it "does not return an unpublished listing" do
+        listing.update(published: false)
+
+        get api_classified_listing_path(listing.id)
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "when unauthorized" do
+      let_it_be_readonly(:headers) { { "api-key" => "invalid api key" } }
+
+      it "returns a published listing" do
+        listing.update(published: true)
+
+        get api_classified_listing_path(listing.id), headers: headers
+        expect(response).to have_http_status(:ok)
+      end
+
+      it "does not return an unpublished listing" do
+        listing.update(published: false)
+
+        get api_classified_listing_path(listing.id), headers: headers
+        expect(response).to have_http_status(:not_found)
+      end
+    end
+
+    context "when authorized" do
+      include_context "when user is authorized"
+
+      let(:headers) { { "api-key" => api_secret.secret } }
+
+      it "returns a published listing" do
+        listing.update(published: true)
+
+        get api_classified_listing_path(listing.id), headers: headers
+        expect(response).to have_http_status(:ok)
+      end
+
+      it "does not return an unpublished listing belonging to another user" do
+        listing.update(published: false, user: user1)
+
+        get api_classified_listing_path(listing.id), headers: headers
+        expect(response).to have_http_status(:not_found)
+      end
+
+      it "returns an unpublished listing belonging to the authenticated user" do
+        listing.update(published: false, user: api_secret.user)
+
+        get api_classified_listing_path(listing.id), headers: headers
+        expect(response).to have_http_status(:ok)
+      end
+    end
+
+    it "returns the correct listing format" do
       get api_classified_listing_path(listing.id)
 
       expect(response).to have_http_status(:ok)
@@ -131,12 +223,6 @@ RSpec.describe "Api::V0::Listings" do
       post api_classified_listings_path, params: { classified_listing: params }.to_json, headers: headers
     end
 
-    def user_admin_organization(user)
-      org = create(:organization)
-      create(:organization_membership, user_id: user.id, organization_id: org.id, type_of_user: "admin")
-      org
-    end
-
     describe "user cannot proceed if not properly unauthorized" do
       let(:api_secret) { create(:api_secret) }