From 96c93ba835d3b2fef7726cf0d8a0a360870f4e78 Mon Sep 17 00:00:00 2001 From: David Roetzel Date: Tue, 17 Mar 2026 16:14:30 +0100 Subject: [PATCH] Only persist a collection item's approval URI once it is verified (#38255) --- app/models/collection_item.rb | 2 +- .../activitypub/process_featured_item_service.rb | 8 ++++---- .../activitypub/verify_featured_item_service.rb | 8 ++++---- .../activitypub/verify_featured_item_worker.rb | 4 ++-- spec/fabricators/collection_item_fabricator.rb | 1 - .../process_featured_item_service_spec.rb | 4 ++-- .../verify_featured_item_service_spec.rb | 14 ++++++++------ .../verify_featured_item_worker_spec.rb | 6 +++--- 8 files changed, 24 insertions(+), 23 deletions(-) diff --git a/app/models/collection_item.rb b/app/models/collection_item.rb index 78186a89fc..018fd3cb17 100644 --- a/app/models/collection_item.rb +++ b/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? diff --git a/app/services/activitypub/process_featured_item_service.rb b/app/services/activitypub/process_featured_item_service.rb index 56edf14c79..b69cfc54e8 100644 --- a/app/services/activitypub/process_featured_item_service.rb +++ b/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 diff --git a/app/services/activitypub/verify_featured_item_service.rb b/app/services/activitypub/verify_featured_item_service.rb index f3dcccf4a0..ecebdf2075 100644 --- a/app/services/activitypub/verify_featured_item_service.rb +++ b/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 diff --git a/app/workers/activitypub/verify_featured_item_worker.rb b/app/workers/activitypub/verify_featured_item_worker.rb index 6eda194717..cd47f5ee21 100644 --- a/app/workers/activitypub/verify_featured_item_worker.rb +++ b/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 diff --git a/spec/fabricators/collection_item_fabricator.rb b/spec/fabricators/collection_item_fabricator.rb index 7c4bcb3d85..c3f62ca15a 100644 --- a/spec/fabricators/collection_item_fabricator.rb +++ b/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 diff --git a/spec/services/activitypub/process_featured_item_service_spec.rb b/spec/services/activitypub/process_featured_item_service_spec.rb index dc04ce5d08..55bf89df91 100644 --- a/spec/services/activitypub/process_featured_item_service_spec.rb +++ b/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 diff --git a/spec/services/activitypub/verify_featured_item_service_spec.rb b/spec/services/activitypub/verify_featured_item_service_spec.rb index 5976ffeffc..5698c23155 100644 --- a/spec/services/activitypub/verify_featured_item_service_spec.rb +++ b/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 diff --git a/spec/workers/activitypub/verify_featured_item_worker_spec.rb b/spec/workers/activitypub/verify_featured_item_worker_spec.rb index f94313ce1d..bbea044c34 100644 --- a/spec/workers/activitypub/verify_featured_item_worker_spec.rb +++ b/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