Add more tests for ActivityPub controllers (#13585)

app/controllers/accounts_controller.rb

@@ -27,7 +27,7 @@ class AccountsController < ApplicationController
         end
 
         @pinned_statuses = cache_collection(@account.pinned_statuses, Status) if show_pinned_statuses?
-        @statuses        = filtered_status_page(params)
+        @statuses        = filtered_status_page
         @statuses        = cache_collection(@statuses, Status)
         @rss_url         = rss_url
 
@@ -140,12 +140,12 @@ class AccountsController < ApplicationController
     request.path.split('.').first.ends_with?(Addressable::URI.parse("/tagged/#{params[:tag]}").normalize)
   end
 
-  def filtered_status_page(params)
-    if params[:min_id].present?
-      filtered_statuses.paginate_by_min_id(PAGE_SIZE, params[:min_id]).reverse
-    else
-      filtered_statuses.paginate_by_max_id(PAGE_SIZE, params[:max_id], params[:since_id]).to_a
-    end
+  def filtered_status_page
+    filtered_statuses.paginate_by_id(PAGE_SIZE, params_slice(:max_id, :min_id, :since_id))
+  end
+
+  def params_slice(*keys)
+    params.slice(*keys).permit(*keys)
   end
 
   def restrict_fields_to

app/controllers/activitypub/collections_controller.rb

@@ -24,20 +24,23 @@ class ActivityPub::CollectionsController < ActivityPub::BaseController
   def set_size
     case params[:id]
     when 'featured'
-      @account.pinned_statuses.count
+      @size = @account.pinned_statuses.count
     else
-      raise ActiveRecord::RecordNotFound
+      not_found
     end
   end
 
   def scope_for_collection
     case params[:id]
     when 'featured'
-      return Status.none if @account.blocking?(signed_request_account)
-
-      @account.pinned_statuses
-    else
-      raise ActiveRecord::RecordNotFound
+      # Because in public fetch mode we cache the response, there would be no
+      # benefit from performing the check below, since a blocked account or domain
+      # would likely be served the cache from the reverse proxy anyway
+      if authorized_fetch_mode? && !signed_request_account.nil? && (@account.blocking?(signed_request_account) || (!signed_request_account.domain.nil? && @account.domain_blocking?(signed_request_account.domain)))
+        Status.none
+      else
+        @account.pinned_statuses
+      end
     end
   end
 

app/controllers/activitypub/outboxes_controller.rb

@@ -11,7 +11,7 @@ class ActivityPub::OutboxesController < ActivityPub::BaseController
   before_action :set_cache_headers
 
   def show
-    expires_in(page_requested? ? 0 : 3.minutes, public: public_fetch_mode?)
+    expires_in(page_requested? ? 0 : 3.minutes, public: public_fetch_mode? && !(signed_request_account.present? && page_requested?))
     render json: outbox_presenter, serializer: ActivityPub::OutboxSerializer, adapter: ActivityPub::Adapter, content_type: 'application/activity+json'
   end
 
@@ -50,12 +50,12 @@ class ActivityPub::OutboxesController < ActivityPub::BaseController
     return unless page_requested?
 
     @statuses = @account.statuses.permitted_for(@account, signed_request_account)
-    @statuses = params[:min_id].present? ? @statuses.paginate_by_min_id(LIMIT, params[:min_id]).reverse : @statuses.paginate_by_max_id(LIMIT, params[:max_id])
+    @statuses = @statuses.paginate_by_id(LIMIT, params_slice(:max_id, :min_id, :since_id))
     @statuses = cache_collection(@statuses, Status)
   end
 
   def page_requested?
-    params[:page] == 'true'
+    truthy_param?(:page)
   end
 
   def page_params

app/controllers/activitypub/replies_controller.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class ActivityPub::RepliesController < ActivityPub::BaseController
-  include SignatureAuthentication
+  include SignatureVerification
   include Authorization
   include AccountOwnedConcern
 
@@ -19,15 +19,19 @@ class ActivityPub::RepliesController < ActivityPub::BaseController
 
   private
 
+  def pundit_user
+    signed_request_account
+  end
+
   def set_status
     @status = @account.statuses.find(params[:status_id])
     authorize @status, :show?
   rescue Mastodon::NotPermittedError
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def set_replies
-    @replies = page_params[:only_other_accounts] ? Status.where.not(account_id: @account.id) : @account.statuses
+    @replies = only_other_accounts? ? Status.where.not(account_id: @account.id) : @account.statuses
     @replies = @replies.where(in_reply_to_id: @status.id, visibility: [:public, :unlisted])
     @replies = @replies.paginate_by_min_id(DESCENDANTS_LIMIT, params[:min_id])
   end
@@ -38,7 +42,7 @@ class ActivityPub::RepliesController < ActivityPub::BaseController
       type: :unordered,
       part_of: account_status_replies_url(@account, @status),
       next: next_page,
-      items: @replies.map { |status| status.local ? status : status.uri }
+      items: @replies.map { |status| status.local? ? status : status.uri }
     )
 
     return page if page_requested?
@@ -51,16 +55,21 @@ class ActivityPub::RepliesController < ActivityPub::BaseController
   end
 
   def page_requested?
-    params[:page] == 'true'
+    truthy_param?(:page)
+  end
+
+  def only_other_accounts?
+    truthy_param?(:only_other_accounts)
   end
 
   def next_page
     only_other_accounts = !(@replies&.last&.account_id == @account.id && @replies.size == DESCENDANTS_LIMIT)
+
     account_status_replies_url(
       @account,
       @status,
       page: true,
-      min_id: only_other_accounts && !page_params[:only_other_accounts] ? nil : @replies&.last&.id,
+      min_id: only_other_accounts && !only_other_accounts? ? nil : @replies&.last&.id,
       only_other_accounts: only_other_accounts
     )
   end

app/controllers/api/v1/polls/votes_controller.rb

@@ -18,7 +18,7 @@ class Api::V1::Polls::VotesController < Api::BaseController
     @poll = Poll.attached.find(params[:poll_id])
     authorize @poll.status, :show?
   rescue Mastodon::NotPermittedError
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def vote_params

app/controllers/api/v1/polls_controller.rb

@@ -17,7 +17,7 @@ class Api::V1::PollsController < Api::BaseController
     @poll = Poll.attached.find(params[:id])
     authorize @poll.status, :show?
   rescue Mastodon::NotPermittedError
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def refresh_poll

app/controllers/api/v1/push/subscriptions_controller.rb

@@ -4,6 +4,7 @@ class Api::V1::Push::SubscriptionsController < Api::BaseController
   before_action -> { doorkeeper_authorize! :push }
   before_action :require_user!
   before_action :set_web_push_subscription
+  before_action :check_web_push_subscription, only: [:show, :update]
 
   def create
     @web_subscription&.destroy!
@@ -21,16 +22,11 @@ class Api::V1::Push::SubscriptionsController < Api::BaseController
   end
 
   def show
-    raise ActiveRecord::RecordNotFound if @web_subscription.nil?
-
     render json: @web_subscription, serializer: REST::WebPushSubscriptionSerializer
   end
 
   def update
-    raise ActiveRecord::RecordNotFound if @web_subscription.nil?
-
     @web_subscription.update!(data: data_params)
-
     render json: @web_subscription, serializer: REST::WebPushSubscriptionSerializer
   end
 
@@ -45,12 +41,17 @@ class Api::V1::Push::SubscriptionsController < Api::BaseController
     @web_subscription = ::Web::PushSubscription.find_by(access_token_id: doorkeeper_token.id)
   end
 
+  def check_web_push_subscription
+    not_found if @web_subscription.nil?
+  end
+
   def subscription_params
     params.require(:subscription).permit(:endpoint, keys: [:auth, :p256dh])
   end
 
   def data_params
     return {} if params[:data].blank?
+
     params.require(:data).permit(alerts: [:follow, :follow_request, :favourite, :reblog, :mention, :poll])
   end
 end

app/controllers/api/v1/statuses/mutes_controller.rb

@@ -28,8 +28,7 @@ class Api::V1::Statuses::MutesController < Api::BaseController
     @status = Status.find(params[:status_id])
     authorize @status, :show?
   rescue Mastodon::NotPermittedError
-    # Reraise in order to get a 404 instead of a 403 error code
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def set_conversation

app/controllers/api/v1/statuses_controller.rb

@@ -67,7 +67,7 @@ class Api::V1::StatusesController < Api::BaseController
     @status = Status.find(params[:id])
     authorize @status, :show?
   rescue Mastodon::NotPermittedError
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def set_thread

app/controllers/media_controller.rb

@@ -33,7 +33,7 @@ class MediaController < ApplicationController
   def verify_permitted_status!
     authorize @media_attachment.status, :show?
   rescue Mastodon::NotPermittedError
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def check_playable

app/controllers/remote_interaction_controller.rb

@@ -41,7 +41,7 @@ class RemoteInteractionController < ApplicationController
     @status = Status.find(params[:id])
     authorize @status, :show?
   rescue Mastodon::NotPermittedError
-    raise ActiveRecord::RecordNotFound
+    not_found
   end
 
   def set_body_classes

app/controllers/statuses_controller.rb

@@ -46,7 +46,7 @@ class StatusesController < ApplicationController
   end
 
   def embed
-    return not_found if @status.hidden?
+    return not_found if @status.hidden? || @status.reblog?
 
     expires_in 180, public: true
     response.headers['X-Frame-Options'] = 'ALLOWALL'