Only persist a collection item's approval URI once it is verified (#38255)

app/models/collection_item.rb

@@ -29,7 +29,7 @@ class CollectionItem < ApplicationRecord
 
   validates :position, numericality: { only_integer: true, greater_than: 0 }
   validates :activity_uri, presence: true, if: :local_item_with_remote_account?
-  validates :approval_uri, presence: true, unless: -> { local? || account&.local? }
+  validates :approval_uri, presence: true, unless: -> { local? || account&.local? || !accepted? }
   validates :account, presence: true, if: :accepted?
   validates :object_uri, presence: true, if: -> { account.nil? }
   validates :uri, presence: true, if: :remote_item_with_remote_account?

app/services/activitypub/process_featured_item_service.rb

@@ -21,9 +21,9 @@ class ActivityPub::ProcessFeaturedItemService
       else
         @collection_item = collection.collection_items.create!(
           uri: item_json['id'],
-          object_uri: item_json['featuredObject'],
-          approval_uri: item_json['featureAuthorization']
+          object_uri: item_json['featuredObject']
         )
+        @approval_uri = item_json['featureAuthorization']
 
         verify_authorization!
       end
@@ -35,8 +35,8 @@ class ActivityPub::ProcessFeaturedItemService
   private
 
   def verify_authorization!
-    ActivityPub::VerifyFeaturedItemService.new.call(@collection_item)
+    ActivityPub::VerifyFeaturedItemService.new.call(@collection_item, @approval_uri)
   rescue Mastodon::RecursionLimitExceededError, Mastodon::UnexpectedResponseError, *Mastodon::HTTP_CONNECTION_ERRORS
-    ActivityPub::VerifyFeaturedItemWorker.perform_in(rand(30..600).seconds, @collection_item.id)
+    ActivityPub::VerifyFeaturedItemWorker.perform_in(rand(30..600).seconds, @collection_item.id, @approval_uri)
   end
 end

app/services/activitypub/verify_featured_item_service.rb

@@ -3,23 +3,23 @@
 class ActivityPub::VerifyFeaturedItemService
   include JsonLdHelper
 
-  def call(collection_item)
+  def call(collection_item, approval_uri)
     @collection_item = collection_item
-    @authorization = fetch_resource(@collection_item.approval_uri, true, raise_on_error: :temporary)
+    @authorization = fetch_resource(approval_uri, true, raise_on_error: :temporary)
 
     if @authorization.nil?
       @collection_item.update!(state: :rejected)
       return
     end
 
-    return if non_matching_uri_hosts?(@collection_item.approval_uri, @authorization['interactionTarget'])
+    return if non_matching_uri_hosts?(approval_uri, @authorization['interactionTarget'])
     return unless matching_type? && matching_collection_uri?
 
     account = Account.where(uri: @collection_item.object_uri).first
     account ||= ActivityPub::FetchRemoteAccountService.new.call(@collection_item.object_uri)
     return if account.blank?
 
-    @collection_item.update!(account:, state: :accepted)
+    @collection_item.update!(account:, approval_uri:, state: :accepted)
   end
 
   private

app/workers/activitypub/verify_featured_item_worker.rb

@@ -7,10 +7,10 @@ class ActivityPub::VerifyFeaturedItemWorker
 
   sidekiq_options queue: 'pull', retry: 5
 
-  def perform(collection_item_id)
+  def perform(collection_item_id, approval_uri)
     collection_item = CollectionItem.find(collection_item_id)
 
-    ActivityPub::VerifyFeaturedItemService.new.call(collection_item)
+    ActivityPub::VerifyFeaturedItemService.new.call(collection_item, approval_uri)
   rescue ActiveRecord::RecordNotFound
     # Do nothing
     nil

spec/fabricators/collection_item_fabricator.rb

@@ -11,6 +11,5 @@ Fabricator(:unverified_remote_collection_item, from: :collection_item) do
   account      nil
   state        :pending
   object_uri   { Fabricate.build(:remote_account).uri }
-  approval_uri { sequence(:uri) { |i| "https://example.com/authorizations/#{i}" } }
   uri { sequence(:uri) { |i| "https://example.com/collection_items/#{i}" } }
 end

spec/services/activitypub/process_featured_item_service_spec.rb

@@ -52,7 +52,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
 
       new_item = collection.collection_items.last
       expect(new_item.object_uri).to eq 'https://example.com/actor/1'
-      expect(new_item.approval_uri).to eq 'https://example.com/auth/1'
+      expect(new_item.approval_uri).to be_nil
     end
 
     context 'when an item exists for a local featured account' do
@@ -93,7 +93,7 @@ RSpec.describe ActivityPub::ProcessFeaturedItemService do
 
       new_item = collection.collection_items.last
       expect(new_item.object_uri).to eq 'https://example.com/actor/1'
-      expect(new_item.approval_uri).to eq 'https://example.com/auth/1'
+      expect(new_item.approval_uri).to be_nil
     end
   end
 end

spec/services/activitypub/verify_featured_item_service_spec.rb

@@ -12,14 +12,14 @@ RSpec.describe ActivityPub::VerifyFeaturedItemService do
               account: nil,
               state: :pending,
               uri: 'https://other.example.com/items/1',
-              object_uri: 'https://example.com/actor/1',
-              approval_uri: verification_json['id'])
+              object_uri: 'https://example.com/actor/1')
   end
+  let(:approval_uri) { 'https://example.com/auth/1' }
   let(:verification_json) do
     {
       '@context' => 'https://www.w3.org/ns/activitystreams',
       'type' => 'FeatureAuthorization',
-      'id' => 'https://example.com/auth/1',
+      'id' => approval_uri,
       'interactionTarget' => 'https://example.com/actor/1',
       'interactingObject' => collection.uri,
     }
@@ -41,12 +41,13 @@ RSpec.describe ActivityPub::VerifyFeaturedItemService do
       before { featured_account }
 
       it 'verifies and creates the item' do
-        subject.call(collection_item)
+        subject.call(collection_item, approval_uri)
 
         expect(verification_request).to have_been_requested
 
         expect(collection_item.account_id).to eq featured_account.id
         expect(collection_item).to be_accepted
+        expect(collection_item.approval_uri).to eq approval_uri
       end
     end
 
@@ -59,13 +60,14 @@ RSpec.describe ActivityPub::VerifyFeaturedItemService do
       end
 
       it 'fetches the actor and creates the item' do
-        subject.call(collection_item)
+        subject.call(collection_item, approval_uri)
 
         expect(stubbed_service).to have_received(:call)
         expect(verification_request).to have_been_requested
 
         expect(collection_item.account_id).to eq featured_account.id
         expect(collection_item).to be_accepted
+        expect(collection_item.approval_uri).to eq approval_uri
       end
     end
   end
@@ -77,7 +79,7 @@ RSpec.describe ActivityPub::VerifyFeaturedItemService do
     end
 
     it 'creates item without attached account and in proper state' do
-      subject.call(collection_item)
+      subject.call(collection_item, approval_uri)
 
       expect(collection_item.account_id).to be_nil
       expect(collection_item).to be_rejected

spec/workers/activitypub/verify_featured_item_worker_spec.rb

@@ -12,13 +12,13 @@ RSpec.describe ActivityPub::VerifyFeaturedItemWorker do
     before { stub_service }
 
     it 'sends the status to the service' do
-      worker.perform(collection_item.id)
+      worker.perform(collection_item.id, 'https://example.com/authorizations/1')
 
-      expect(service).to have_received(:call).with(collection_item)
+      expect(service).to have_received(:call).with(collection_item, 'https://example.com/authorizations/1')
     end
 
     it 'returns nil for non-existent record' do
-      result = worker.perform(123_123_123)
+      result = worker.perform(123_123_123, 'https://example.com/authorizations/1')
 
       expect(result).to be_nil
     end