From 64c7ff7c11dd7bb8e8d282c0737a85c60c349422 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Sat, 18 May 2024 13:12:33 +0200 Subject: [PATCH 01/15] Add integration test for when remote actor username changes --- .../activitypub/inboxes_controller_spec.rb | 147 ++++++++++++++++++ 1 file changed, 147 insertions(+) create mode 100644 spec/requests/activitypub/inboxes_controller_spec.rb diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb new file mode 100644 index 00000000000..cd49f26211d --- /dev/null +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -0,0 +1,147 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe ActivityPub::InboxesController do + let!(:remote_actor_keypair) do + OpenSSL::PKey.read(<<~PEM_TEXT) + -----BEGIN RSA PRIVATE KEY----- + MIIEowIBAAKCAQEAqIAYvNFGbZ5g4iiK6feSdXD4bDStFM58A7tHycYXaYtzZQpI + eHXAmaXuZzXIwtrP4N0gIk8JNwZvXj2UPS+S07t0V9wNK94he01LV5EMz/GN4eNn + FmDL64HIEuKLvV8TvgjbUPRD6Y5X0UpKi2ZIFLSb96Q5w0Z/k7ntpVKV52y8kz5F + jr/O/0JuHryZe0yItzJh8kzFfeMf0EXzfSnaKvT7P9jhgC6uTre+jXyvVZjiHDrn + qvvucdI3I7DRfXo1OqARBrLjy+TdseUAjNYJ+OuPRI1URIWQI01DCHqcohVu9+Ar + +BiCjFp3ua+XMuJvrvbD61d1Fvig/9nbBRR+8QIDAQABAoIBAAgySHnFWI6gItR3 + fkfiqIm80cHCN3Xk1C6iiVu+3oBOZbHpW9R7vl9e/WOA/9O+LPjiSsQOegtWnVvd + RRjrl7Hj20VDlZKv5Mssm6zOGAxksrcVbqwdj+fUJaNJCL0AyyseH0x/IE9T8rDC + I1GH+3tB3JkhkIN/qjipdX5ab8MswEPu8IC4ViTpdBgWYY/xBcAHPw4xuL0tcwzh + FBlf4DqoEVQo8GdK5GAJ2Ny0S4xbXHUURzx/R4y4CCts7niAiLGqd9jmLU1kUTMk + QcXfQYK6l+unLc7wDYAz7sFEHh04M48VjWwiIZJnlCqmQbLda7uhhu8zkF1DqZTu + ulWDGQECgYEA0TIAc8BQBVab979DHEEmMdgqBwxLY3OIAk0b+r50h7VBGWCDPRsC + STD73fQY3lNet/7/jgSGwwAlAJ5PpMXxXiZAE3bUwPmHzgF7pvIOOLhA8O07tHSO + L2mvQe6NPzjZ+6iAO2U9PkClxcvGvPx2OBvisfHqZLmxC9PIVxzruQECgYEAzjM6 + BTUXa6T/qHvLFbN699BXsUOGmHBGaLRapFDBfVvgZrwqYQcZpBBhesLdGTGSqwE7 + gWsITPIJ+Ldo+38oGYyVys+w/V67q6ud7hgSDTW3hSvm+GboCjk6gzxlt9hQ0t9X + 8vfDOYhEXvVUJNv3mYO60ENqQhILO4bQ0zi+VfECgYBb/nUccfG+pzunU0Cb6Dp3 + qOuydcGhVmj1OhuXxLFSDG84Tazo7juvHA9mp7VX76mzmDuhpHPuxN2AzB2SBEoE + cSW0aYld413JRfWukLuYTc6hJHIhBTCRwRQFFnae2s1hUdQySm8INT2xIc+fxBXo + zrp+Ljg5Wz90SAnN5TX0AQKBgDaatDOq0o/r+tPYLHiLtfWoE4Dau+rkWJDjqdk3 + lXWn/e3WyHY3Vh/vQpEqxzgju45TXjmwaVtPATr+/usSykCxzP0PMPR3wMT+Rm1F + rIoY/odij+CaB7qlWwxj0x/zRbwB7x1lZSp4HnrzBpxYL+JUUwVRxPLIKndSBTza + GvVRAoGBAIVBcNcRQYF4fvZjDKAb4fdBsEuHmycqtRCsnkGOz6ebbEQznSaZ0tZE + +JuouZaGjyp8uPjNGD5D7mIGbyoZ3KyG4mTXNxDAGBso1hrNDKGBOrGaPhZx8LgO + 4VXJ+ybXrATf4jr8ccZYsZdFpOphPzz+j55Mqg5vac5P1XjmsGTb + -----END RSA PRIVATE KEY----- + PEM_TEXT + end + let(:remote_actor) do + Fabricate(:account, + domain: 'remote.domain', + uri: 'https://remote.domain/users/bob', + private_key: nil, + public_key: remote_actor_keypair.public_key.to_pem, + protocol: 1) # activitypub + end + let(:local_actor) { Fabricate(:account) } + let(:base_headers) do + { + 'Host' => 'www.remote.domain', + 'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT', + } + end + let(:note_content) { 'Note from remote actor' } + let(:object_json) do + { + id: 'https://remote.domain/activities/objects/1', + type: 'Note', + content: note_content, + to: ActivityPub::TagManager.instance.uri_for(local_actor), + } + end + let(:json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://remote.domain/activities/create/1', + type: 'Create', + actor: remote_actor_json[:id], + object: object_json, + }.with_indifferent_access + end + let(:digest_header) { digest_value(json.to_json) } + let(:signature_header) do + build_signature_string( + remote_actor_keypair, + 'https://remote.domain/users/bob#main-key', + "post /users/#{local_actor.username}/inbox", + base_headers.merge( + 'Digest' => digest_header + ) + ) + end + let(:headers) do + base_headers.merge( + 'Digest' => digest_header, + 'Signature' => signature_header + ) + end + + before do + travel_to '2023-12-20T10:00:00Z' + end + + context 'when remote actor username has changed' do + let(:remote_actor_json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: remote_actor.uri, + type: 'Person', + preferredUsername: 'new_username', + inbox: "#{remote_actor.uri}#inbox", + publicKey: { + id: "#{remote_actor.uri}#main-key", + owner: remote_actor.uri, + publicKeyPem: remote_actor.public_key, + }, + }.with_indifferent_access + end + + before do + stub_request(:get, 'https://remote.domain/users/bob#main-key') + .to_return( + body: remote_actor_json.to_json, + headers: { + 'Content-Type' => 'application/activity+json', + }, + status: 200 + ) + stub_request(:get, 'https://remote.domain/users/bob') + .to_return( + body: remote_actor_json.to_json, + headers: { + 'Content-Type' => 'application/activity+json', + }, + status: 200 + ) + end + + it 'successfuly processes note' do + Sidekiq::Testing.inline! + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect(response).to have_http_status(202) + expect(Status.exists?(uri: object_json[:id])).to be(true) + end + end + + def build_signature_string(keypair, key_id, request_target, headers) + algorithm = 'rsa-sha256' + signed_headers = headers.merge({ '(request-target)' => request_target }) + signed_string = signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") + signature = Base64.strict_encode64(keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string)) + + "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\"" + end + + def digest_value(body) + "SHA-256=#{Digest::SHA256.base64digest(body)}" + end +end From 9ea6ccd3227ffdc56fac613e259d7f6fad6109fd Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Sat, 18 May 2024 13:31:08 +0200 Subject: [PATCH 02/15] Make username state explicit --- spec/requests/activitypub/inboxes_controller_spec.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb index cd49f26211d..1fc5b889deb 100644 --- a/spec/requests/activitypub/inboxes_controller_spec.rb +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -34,12 +34,14 @@ RSpec.describe ActivityPub::InboxesController do -----END RSA PRIVATE KEY----- PEM_TEXT end + let(:remote_actor_original_username) { 'original_username' } let(:remote_actor) do Fabricate(:account, domain: 'remote.domain', uri: 'https://remote.domain/users/bob', private_key: nil, public_key: remote_actor_keypair.public_key.to_pem, + username: remote_actor_original_username, protocol: 1) # activitypub end let(:local_actor) { Fabricate(:account) } @@ -90,12 +92,13 @@ RSpec.describe ActivityPub::InboxesController do end context 'when remote actor username has changed' do + let(:remote_actor_new_username) { 'new_username' } let(:remote_actor_json) do { '@context': 'https://www.w3.org/ns/activitystreams', id: remote_actor.uri, type: 'Person', - preferredUsername: 'new_username', + preferredUsername: remote_actor_new_username, inbox: "#{remote_actor.uri}#inbox", publicKey: { id: "#{remote_actor.uri}#main-key", @@ -129,6 +132,8 @@ RSpec.describe ActivityPub::InboxesController do post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers expect(response).to have_http_status(202) expect(Status.exists?(uri: object_json[:id])).to be(true) + # we don't expect the remote actor username to change + expect(remote_actor.reload.username).to eq(remote_actor_original_username) end end From acba3f11213c27dc08666e94e83e45be4cf3a647 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Sat, 18 May 2024 14:59:55 +0200 Subject: [PATCH 03/15] Add remote username changed search integration test --- spec/requests/api/v2/search_spec.rb | 76 +++++++++++++++++++++++++++++ 1 file changed, 76 insertions(+) diff --git a/spec/requests/api/v2/search_spec.rb b/spec/requests/api/v2/search_spec.rb index 13bcf17984b..c906af276bc 100644 --- a/spec/requests/api/v2/search_spec.rb +++ b/spec/requests/api/v2/search_spec.rb @@ -83,6 +83,82 @@ describe 'Search API' do expect(body_as_json[:accounts].pluck(:id)).to contain_exactly(ana.id.to_s) end end + + context 'when a remote actor username has changed' do + let(:remote_actor_original_username) { 'original_username' } + let(:remote_actor) do + Fabricate(:account, + domain: 'remote.domain', + uri: 'https://remote.domain/users/bob', + private_key: nil, + username: remote_actor_original_username, + protocol: 1) # activitypub + end + let(:remote_actor_new_username) { 'new_username' } + let(:remote_actor_json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: remote_actor.uri, + type: 'Person', + preferredUsername: remote_actor_new_username, + inbox: "#{remote_actor.uri}#inbox", + }.with_indifferent_access + end + let(:remote_actor_new_handle) { "#{remote_actor_new_username}@remote.domain" } + let(:params) { { q: remote_actor_new_handle, resolve: '1' } } + let(:webfinger_response) do + { + subject: "acct:#{remote_actor_new_handle}", + links: [ + { + rel: 'self', + type: 'application/activity+json', + href: remote_actor.uri, + }, + ], + } + end + + before do + sign_in(user) + stub_request(:get, "https://remote.domain/.well-known/webfinger?resource=acct:#{remote_actor_new_handle}") + .to_return( + body: webfinger_response.to_json, + headers: { + 'Content-Type' => 'application/json', + }, + status: 200 + ) + stub_request(:get, remote_actor.uri) + .to_return( + body: remote_actor_json.to_json, + headers: { + 'Content-Type' => 'application/activity+json', + }, + status: 200 + ) + end + + it 'does not increase the number of accounts' do + Sidekiq::Testing.inline! + expect do + get '/api/v2/search', headers: headers, params: params + end.to(not_change { Account.count }) + end + + it 'merges the old account with the new account' do + Sidekiq::Testing.inline! + tom.follow!(remote_actor) + get '/api/v2/search', headers: headers, params: params + expect(Account.exists?(id: remote_actor.id)).to be(false) + new_remote_actor = Account.find_by( + uri: remote_actor.uri, + username: remote_actor_new_username + ) + expect(new_remote_actor.present?).to be(true) + expect(tom.following?(new_remote_actor)).to be(true) + end + end end context 'when search raises syntax error' do From f6307d2c3d1e25eadbcd3d2f04858a8f0de95023 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Thu, 23 May 2024 16:18:45 +0200 Subject: [PATCH 04/15] Improve and add Update and Follow tests --- .../activitypub/inboxes_controller_spec.rb | 265 ++++++++++++++---- spec/requests/signature_verification_spec.rb | 13 - spec/support/signed_request_helpers.rb | 13 + 3 files changed, 219 insertions(+), 72 deletions(-) diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb index 1fc5b889deb..adb75561c0a 100644 --- a/spec/requests/activitypub/inboxes_controller_spec.rb +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -3,6 +3,7 @@ require 'rails_helper' RSpec.describe ActivityPub::InboxesController do + let!(:current_datetime) { 'Wed, 20 Dec 2023 10:00:00 GMT' } let!(:remote_actor_keypair) do OpenSSL::PKey.read(<<~PEM_TEXT) -----BEGIN RSA PRIVATE KEY----- @@ -34,25 +35,27 @@ RSpec.describe ActivityPub::InboxesController do -----END RSA PRIVATE KEY----- PEM_TEXT end - let(:remote_actor_original_username) { 'original_username' } - let(:remote_actor) do + let!(:remote_actor_inbox_url) { 'https://remote.domain/users/bob/inbox' } + let!(:remote_actor_original_username) { 'original_username' } + let!(:remote_actor) do Fabricate(:account, domain: 'remote.domain', uri: 'https://remote.domain/users/bob', private_key: nil, public_key: remote_actor_keypair.public_key.to_pem, username: remote_actor_original_username, - protocol: 1) # activitypub + protocol: 1, # activitypub + inbox_url: remote_actor_inbox_url) end - let(:local_actor) { Fabricate(:account) } - let(:base_headers) do + let!(:local_actor) { Fabricate(:account) } + let!(:base_headers) do { 'Host' => 'www.remote.domain', - 'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT', + 'Date' => current_datetime, } end - let(:note_content) { 'Note from remote actor' } - let(:object_json) do + let!(:note_content) { 'note from remote actor' } + let!(:object_json) do { id: 'https://remote.domain/activities/objects/1', type: 'Note', @@ -60,46 +63,21 @@ RSpec.describe ActivityPub::InboxesController do to: ActivityPub::TagManager.instance.uri_for(local_actor), } end - let(:json) do - { - '@context': 'https://www.w3.org/ns/activitystreams', - id: 'https://remote.domain/activities/create/1', - type: 'Create', - actor: remote_actor_json[:id], - object: object_json, - }.with_indifferent_access - end - let(:digest_header) { digest_value(json.to_json) } - let(:signature_header) do - build_signature_string( - remote_actor_keypair, - 'https://remote.domain/users/bob#main-key', - "post /users/#{local_actor.username}/inbox", - base_headers.merge( - 'Digest' => digest_header - ) - ) - end - let(:headers) do - base_headers.merge( - 'Digest' => digest_header, - 'Signature' => signature_header - ) - end before do - travel_to '2023-12-20T10:00:00Z' + Sidekiq::Testing.inline! + travel_to current_datetime end context 'when remote actor username has changed' do let(:remote_actor_new_username) { 'new_username' } - let(:remote_actor_json) do + let(:updated_remote_actor_json) do { '@context': 'https://www.w3.org/ns/activitystreams', id: remote_actor.uri, type: 'Person', preferredUsername: remote_actor_new_username, - inbox: "#{remote_actor.uri}#inbox", + inbox: remote_actor.inbox_url, publicKey: { id: "#{remote_actor.uri}#main-key", owner: remote_actor.uri, @@ -111,7 +89,7 @@ RSpec.describe ActivityPub::InboxesController do before do stub_request(:get, 'https://remote.domain/users/bob#main-key') .to_return( - body: remote_actor_json.to_json, + body: updated_remote_actor_json.to_json, headers: { 'Content-Type' => 'application/activity+json', }, @@ -119,7 +97,7 @@ RSpec.describe ActivityPub::InboxesController do ) stub_request(:get, 'https://remote.domain/users/bob') .to_return( - body: remote_actor_json.to_json, + body: updated_remote_actor_json.to_json, headers: { 'Content-Type' => 'application/activity+json', }, @@ -127,26 +105,195 @@ RSpec.describe ActivityPub::InboxesController do ) end - it 'successfuly processes note' do - Sidekiq::Testing.inline! - post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers - expect(response).to have_http_status(202) - expect(Status.exists?(uri: object_json[:id])).to be(true) - # we don't expect the remote actor username to change - expect(remote_actor.reload.username).to eq(remote_actor_original_username) + context 'with a create note' do + let(:json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://remote.domain/activities/create/1', + type: 'Create', + actor: remote_actor.uri, + object: object_json, + }.with_indifferent_access + end + let(:digest_header) { digest_value(json.to_json) } + let(:signature_header) do + build_signature_string( + remote_actor_keypair, + 'https://remote.domain/users/bob#main-key', + "post /users/#{local_actor.username}/inbox", + base_headers.merge( + 'Digest' => digest_header + ) + ) + end + let(:headers) do + base_headers.merge( + 'Digest' => digest_header, + 'Signature' => signature_header + ) + end + + it 'creates the note' do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect(response).to have_http_status(202) + expect(Status.exists?(uri: object_json[:id])).to be(true) + end + + it 'does not change the local record of the remote actor' do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect(remote_actor.reload.username).to eq(remote_actor_original_username) + end + end + + context 'with an update note' do + let!(:status) do + Fabricate(:status, + uri: object_json[:id], + text: note_content, + account: remote_actor) + end + let(:updated_content) { 'updated note from remote actor' } + let(:json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://remote.domain/activities/update/1', + type: 'Update', + actor: remote_actor.uri, + object: object_json.merge( + updated: current_datetime, + content: updated_content + ), + }.with_indifferent_access + end + let(:digest_header) { digest_value(json.to_json) } + let(:signature_header) do + build_signature_string( + remote_actor_keypair, + 'https://remote.domain/users/bob#main-key', + "post /users/#{local_actor.username}/inbox", + base_headers.merge( + 'Digest' => digest_header + ) + ) + end + let(:headers) do + base_headers.merge( + 'Digest' => digest_header, + 'Signature' => signature_header + ) + end + + it 'updates the Note' do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect(response).to have_http_status(202) + expect(Status.exists?(id: status.id, text: updated_content)).to be(true) + end + + it 'does not change the local record of the remote actor' do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect(remote_actor.reload.username).to eq(remote_actor_original_username) + end + end + + context 'with an update actor' do + let(:json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://remote.domain/activities/update/1', + type: 'Update', + actor: remote_actor.uri, + object: updated_remote_actor_json, + }.with_indifferent_access + end + let(:digest_header) { digest_value(json.to_json) } + let(:signature_header) do + build_signature_string( + remote_actor_keypair, + 'https://remote.domain/users/bob#main-key', + "post /users/#{local_actor.username}/inbox", + base_headers.merge( + 'Digest' => digest_header + ) + ) + end + let(:headers) do + base_headers.merge( + 'Digest' => digest_header, + 'Signature' => signature_header + ) + end + + it 'does not increase the number of accounts' do + expect do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + end.to(not_change { Account.count }) + end + + it 'does not update the remote actors username' do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect(response).to have_http_status(202) + expect(remote_actor.reload.username).to eq(remote_actor_original_username) + end + end + + context 'with a follow' do + context 'when the remote actor is already following the actor' do + before do + remote_actor.follow!(local_actor) + stub_request(:post, remote_actor_inbox_url) + end + + let(:json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'https://remote.domain/activities/update/1', + type: 'Follow', + actor: remote_actor.uri, + object: ActivityPub::TagManager.instance.uri_for(local_actor), + }.with_indifferent_access + end + let(:digest_header) { digest_value(json.to_json) } + let(:signature_header) do + build_signature_string( + remote_actor_keypair, + 'https://remote.domain/users/bob#main-key', + "post /users/#{local_actor.username}/inbox", + base_headers.merge( + 'Digest' => digest_header + ) + ) + end + let(:headers) do + base_headers.merge( + 'Digest' => digest_header, + 'Signature' => signature_header + ) + end + + it 'does not increase the number of accounts' do + expect do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + end.to(not_change { Account.count }) + end + + it 'does not increase the number of follows' do + expect do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + end.to(not_change { Follow.count }) + end + + it 'posts an acceptance to the remote actors inbox' do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect( + a_request(:post, remote_actor_inbox_url) + ).to have_been_made.once + end + + it 'does not change the local record of the remote actor' do + post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers + expect(remote_actor.reload.username).to eq(remote_actor_original_username) + end + end end end - - def build_signature_string(keypair, key_id, request_target, headers) - algorithm = 'rsa-sha256' - signed_headers = headers.merge({ '(request-target)' => request_target }) - signed_string = signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") - signature = Base64.strict_encode64(keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string)) - - "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\"" - end - - def digest_value(body) - "SHA-256=#{Digest::SHA256.base64digest(body)}" - end end diff --git a/spec/requests/signature_verification_spec.rb b/spec/requests/signature_verification_spec.rb index 401828c4a3c..1df027e431f 100644 --- a/spec/requests/signature_verification_spec.rb +++ b/spec/requests/signature_verification_spec.rb @@ -382,17 +382,4 @@ describe 'signature verification concern' do alias_method :signature_required, :success end end - - def digest_value(body) - "SHA-256=#{Digest::SHA256.base64digest(body)}" - end - - def build_signature_string(keypair, key_id, request_target, headers) - algorithm = 'rsa-sha256' - signed_headers = headers.merge({ '(request-target)' => request_target }) - signed_string = signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") - signature = Base64.strict_encode64(keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string)) - - "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\"" - end end diff --git a/spec/support/signed_request_helpers.rb b/spec/support/signed_request_helpers.rb index eba4095e438..d3a20dac43d 100644 --- a/spec/support/signed_request_helpers.rb +++ b/spec/support/signed_request_helpers.rb @@ -1,6 +1,19 @@ # frozen_string_literal: true module SignedRequestHelpers + def digest_value(body) + "SHA-256=#{Digest::SHA256.base64digest(body)}" + end + + def build_signature_string(keypair, key_id, request_target, headers) + algorithm = 'rsa-sha256' + signed_headers = headers.merge({ '(request-target)' => request_target }) + signed_string = signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") + signature = Base64.strict_encode64(keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string)) + + "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\"" + end + def get(path, headers: nil, sign_with: nil, **args) return super(path, headers: headers, **args) if sign_with.nil? From ff873ebbf25361b8f1c9a63d6f132427642e55e3 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Thu, 23 May 2024 18:14:56 +0200 Subject: [PATCH 05/15] Update spec/requests/activitypub/inboxes_controller_spec.rb Co-authored-by: Claire --- spec/requests/activitypub/inboxes_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb index adb75561c0a..a9931659248 100644 --- a/spec/requests/activitypub/inboxes_controller_spec.rb +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -44,7 +44,7 @@ RSpec.describe ActivityPub::InboxesController do private_key: nil, public_key: remote_actor_keypair.public_key.to_pem, username: remote_actor_original_username, - protocol: 1, # activitypub + protocol: :activitypub, inbox_url: remote_actor_inbox_url) end let!(:local_actor) { Fabricate(:account) } From a3d78540bd331bdc542f120716609c4b8e275adb Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Thu, 23 May 2024 18:21:52 +0200 Subject: [PATCH 06/15] Add more cases to search spec --- spec/requests/api/v2/search_spec.rb | 169 ++++++++++++++++++++++++---- 1 file changed, 146 insertions(+), 23 deletions(-) diff --git a/spec/requests/api/v2/search_spec.rb b/spec/requests/api/v2/search_spec.rb index c906af276bc..62ec0a7d882 100644 --- a/spec/requests/api/v2/search_spec.rb +++ b/spec/requests/api/v2/search_spec.rb @@ -85,27 +85,66 @@ describe 'Search API' do end context 'when a remote actor username has changed' do - let(:remote_actor_original_username) { 'original_username' } - let(:remote_actor) do + let!(:remote_actor_keypair) do + OpenSSL::PKey.read(<<~PEM_TEXT) + -----BEGIN RSA PRIVATE KEY----- + MIIEowIBAAKCAQEAqIAYvNFGbZ5g4iiK6feSdXD4bDStFM58A7tHycYXaYtzZQpI + eHXAmaXuZzXIwtrP4N0gIk8JNwZvXj2UPS+S07t0V9wNK94he01LV5EMz/GN4eNn + FmDL64HIEuKLvV8TvgjbUPRD6Y5X0UpKi2ZIFLSb96Q5w0Z/k7ntpVKV52y8kz5F + jr/O/0JuHryZe0yItzJh8kzFfeMf0EXzfSnaKvT7P9jhgC6uTre+jXyvVZjiHDrn + qvvucdI3I7DRfXo1OqARBrLjy+TdseUAjNYJ+OuPRI1URIWQI01DCHqcohVu9+Ar + +BiCjFp3ua+XMuJvrvbD61d1Fvig/9nbBRR+8QIDAQABAoIBAAgySHnFWI6gItR3 + fkfiqIm80cHCN3Xk1C6iiVu+3oBOZbHpW9R7vl9e/WOA/9O+LPjiSsQOegtWnVvd + RRjrl7Hj20VDlZKv5Mssm6zOGAxksrcVbqwdj+fUJaNJCL0AyyseH0x/IE9T8rDC + I1GH+3tB3JkhkIN/qjipdX5ab8MswEPu8IC4ViTpdBgWYY/xBcAHPw4xuL0tcwzh + FBlf4DqoEVQo8GdK5GAJ2Ny0S4xbXHUURzx/R4y4CCts7niAiLGqd9jmLU1kUTMk + QcXfQYK6l+unLc7wDYAz7sFEHh04M48VjWwiIZJnlCqmQbLda7uhhu8zkF1DqZTu + ulWDGQECgYEA0TIAc8BQBVab979DHEEmMdgqBwxLY3OIAk0b+r50h7VBGWCDPRsC + STD73fQY3lNet/7/jgSGwwAlAJ5PpMXxXiZAE3bUwPmHzgF7pvIOOLhA8O07tHSO + L2mvQe6NPzjZ+6iAO2U9PkClxcvGvPx2OBvisfHqZLmxC9PIVxzruQECgYEAzjM6 + BTUXa6T/qHvLFbN699BXsUOGmHBGaLRapFDBfVvgZrwqYQcZpBBhesLdGTGSqwE7 + gWsITPIJ+Ldo+38oGYyVys+w/V67q6ud7hgSDTW3hSvm+GboCjk6gzxlt9hQ0t9X + 8vfDOYhEXvVUJNv3mYO60ENqQhILO4bQ0zi+VfECgYBb/nUccfG+pzunU0Cb6Dp3 + qOuydcGhVmj1OhuXxLFSDG84Tazo7juvHA9mp7VX76mzmDuhpHPuxN2AzB2SBEoE + cSW0aYld413JRfWukLuYTc6hJHIhBTCRwRQFFnae2s1hUdQySm8INT2xIc+fxBXo + zrp+Ljg5Wz90SAnN5TX0AQKBgDaatDOq0o/r+tPYLHiLtfWoE4Dau+rkWJDjqdk3 + lXWn/e3WyHY3Vh/vQpEqxzgju45TXjmwaVtPATr+/usSykCxzP0PMPR3wMT+Rm1F + rIoY/odij+CaB7qlWwxj0x/zRbwB7x1lZSp4HnrzBpxYL+JUUwVRxPLIKndSBTza + GvVRAoGBAIVBcNcRQYF4fvZjDKAb4fdBsEuHmycqtRCsnkGOz6ebbEQznSaZ0tZE + +JuouZaGjyp8uPjNGD5D7mIGbyoZ3KyG4mTXNxDAGBso1hrNDKGBOrGaPhZx8LgO + 4VXJ+ybXrATf4jr8ccZYsZdFpOphPzz+j55Mqg5vac5P1XjmsGTb + -----END RSA PRIVATE KEY----- + PEM_TEXT + end + let!(:remote_actor_inbox_url) { 'https://remote.domain/users/bob/inbox' } + let!(:remote_actor_original_username) { 'original_username' } + let!(:remote_actor) do Fabricate(:account, domain: 'remote.domain', uri: 'https://remote.domain/users/bob', private_key: nil, + public_key: remote_actor_keypair.public_key.to_pem, username: remote_actor_original_username, - protocol: 1) # activitypub + protocol: 1, # activitypub + inbox_url: remote_actor_inbox_url) end - let(:remote_actor_new_username) { 'new_username' } - let(:remote_actor_json) do + let!(:remote_actor_old_handle) { "#{remote_actor_original_username}@remote.domain" } + let!(:remote_actor_new_username) { 'new_username' } + let!(:remote_actor_json) do { '@context': 'https://www.w3.org/ns/activitystreams', id: remote_actor.uri, type: 'Person', preferredUsername: remote_actor_new_username, - inbox: "#{remote_actor.uri}#inbox", + inbox: remote_actor.inbox_url, + publicKey: { + id: "#{remote_actor.uri}#main-key", + owner: remote_actor.uri, + publicKeyPem: remote_actor.public_key, + }, }.with_indifferent_access end - let(:remote_actor_new_handle) { "#{remote_actor_new_username}@remote.domain" } - let(:params) { { q: remote_actor_new_handle, resolve: '1' } } + let!(:remote_actor_new_handle) { "#{remote_actor_new_username}@remote.domain" } let(:webfinger_response) do { subject: "acct:#{remote_actor_new_handle}", @@ -121,6 +160,7 @@ describe 'Search API' do before do sign_in(user) + tom.follow!(remote_actor) stub_request(:get, "https://remote.domain/.well-known/webfinger?resource=acct:#{remote_actor_new_handle}") .to_return( body: webfinger_response.to_json, @@ -137,26 +177,109 @@ describe 'Search API' do }, status: 200 ) + Sidekiq::Testing.inline! end - it 'does not increase the number of accounts' do - Sidekiq::Testing.inline! - expect do + context 'when requesting the old handle' do + let!(:params) { { q: remote_actor_old_handle, resolve: '1' } } + + it 'does not increase the number of accounts' do + expect do + get '/api/v2/search', headers: headers, params: params + end.to(not_change { Account.count }) + end + + it 'does not change the remote actor account' do get '/api/v2/search', headers: headers, params: params - end.to(not_change { Account.count }) + expect(remote_actor.reload.username).to eq(remote_actor_original_username) + end + + it 'returns the remote actor account' do + get '/api/v2/search', headers: headers, params: params + expect(body_as_json[:accounts].pluck(:id)).to contain_exactly(remote_actor.id.to_s) + end end - it 'merges the old account with the new account' do - Sidekiq::Testing.inline! - tom.follow!(remote_actor) - get '/api/v2/search', headers: headers, params: params - expect(Account.exists?(id: remote_actor.id)).to be(false) - new_remote_actor = Account.find_by( - uri: remote_actor.uri, - username: remote_actor_new_username - ) - expect(new_remote_actor.present?).to be(true) - expect(tom.following?(new_remote_actor)).to be(true) + context 'when requesting the old handle of a stale account' do + let!(:params) { { q: remote_actor_old_handle, resolve: '1' } } + + before do + stub_request(:get, 'https://remote.domain/.well-known/host-meta').to_return(status: 404) + remote_actor.update(last_webfingered_at: 2.days.ago) + end + + it 'makes a webfinger request with the old handle' do + stub_request(:get, "https://remote.domain/.well-known/webfinger?resource=acct:#{remote_actor_old_handle}") + get '/api/v2/search', headers: headers, params: params + expect( + a_request( + :get, + "https://remote.domain/.well-known/webfinger?resource=acct:#{remote_actor_old_handle}" + ) + ).to have_been_made.once + end + + it 'does nothing if the webfinger request returns not found' do + stub_request(:get, "https://remote.domain/.well-known/webfinger?resource=acct:#{remote_actor_old_handle}") + .to_return( + status: 404 + ) + get '/api/v2/search', headers: headers, params: params + expect(body_as_json[:accounts].empty?).to be(true) + expect(remote_actor.reload.username).to eq(remote_actor_original_username) + end + + it 'merges the old account with the new account if the webfinger request succeeds' do + stub_request(:get, "https://remote.domain/.well-known/webfinger?resource=acct:#{remote_actor_old_handle}") + .to_return( + body: { + subject: "acct:#{remote_actor_old_handle}", + links: [ + { + rel: 'self', + type: 'application/activity+json', + href: remote_actor.uri, + }, + ], + }.to_json, + headers: { + 'Content-Type' => 'application/json', + }, + status: 200 + ) + expect do + get '/api/v2/search', headers: headers, params: params + end.to(not_change { Account.count }) + + expect(Account.exists?(id: remote_actor.id)).to be(false) + new_remote_actor = Account.find_by( + uri: remote_actor.uri, + username: remote_actor_new_username + ) + expect(new_remote_actor.present?).to be(true) + expect(tom.following?(new_remote_actor)).to be(true) + end + end + + context 'when requesting the new handle' do + let(:params) { { q: remote_actor_new_handle, resolve: '1' } } + + it 'does not increase the number of accounts' do + expect do + get '/api/v2/search', headers: headers, params: params + end.to(not_change { Account.count }) + end + + it 'merges the old account with the new account' do + get '/api/v2/search', headers: headers, params: params + expect(Account.exists?(id: remote_actor.id)).to be(false) + new_remote_actor = Account.find_by( + uri: remote_actor.uri, + username: remote_actor_new_username + ) + expect(new_remote_actor.present?).to be(true) + expect(tom.following?(new_remote_actor)).to be(true) + end end end end From 91a76ad1626a6ebfe67b22f5bff8a9acefb372a9 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Thu, 23 May 2024 18:31:23 +0200 Subject: [PATCH 07/15] Remove Update and Follow from inbox integration test --- .../activitypub/inboxes_controller_spec.rb | 110 ------------------ 1 file changed, 110 deletions(-) diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb index a9931659248..c432911d525 100644 --- a/spec/requests/activitypub/inboxes_controller_spec.rb +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -145,56 +145,6 @@ RSpec.describe ActivityPub::InboxesController do end end - context 'with an update note' do - let!(:status) do - Fabricate(:status, - uri: object_json[:id], - text: note_content, - account: remote_actor) - end - let(:updated_content) { 'updated note from remote actor' } - let(:json) do - { - '@context': 'https://www.w3.org/ns/activitystreams', - id: 'https://remote.domain/activities/update/1', - type: 'Update', - actor: remote_actor.uri, - object: object_json.merge( - updated: current_datetime, - content: updated_content - ), - }.with_indifferent_access - end - let(:digest_header) { digest_value(json.to_json) } - let(:signature_header) do - build_signature_string( - remote_actor_keypair, - 'https://remote.domain/users/bob#main-key', - "post /users/#{local_actor.username}/inbox", - base_headers.merge( - 'Digest' => digest_header - ) - ) - end - let(:headers) do - base_headers.merge( - 'Digest' => digest_header, - 'Signature' => signature_header - ) - end - - it 'updates the Note' do - post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers - expect(response).to have_http_status(202) - expect(Status.exists?(id: status.id, text: updated_content)).to be(true) - end - - it 'does not change the local record of the remote actor' do - post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers - expect(remote_actor.reload.username).to eq(remote_actor_original_username) - end - end - context 'with an update actor' do let(:json) do { @@ -235,65 +185,5 @@ RSpec.describe ActivityPub::InboxesController do expect(remote_actor.reload.username).to eq(remote_actor_original_username) end end - - context 'with a follow' do - context 'when the remote actor is already following the actor' do - before do - remote_actor.follow!(local_actor) - stub_request(:post, remote_actor_inbox_url) - end - - let(:json) do - { - '@context': 'https://www.w3.org/ns/activitystreams', - id: 'https://remote.domain/activities/update/1', - type: 'Follow', - actor: remote_actor.uri, - object: ActivityPub::TagManager.instance.uri_for(local_actor), - }.with_indifferent_access - end - let(:digest_header) { digest_value(json.to_json) } - let(:signature_header) do - build_signature_string( - remote_actor_keypair, - 'https://remote.domain/users/bob#main-key', - "post /users/#{local_actor.username}/inbox", - base_headers.merge( - 'Digest' => digest_header - ) - ) - end - let(:headers) do - base_headers.merge( - 'Digest' => digest_header, - 'Signature' => signature_header - ) - end - - it 'does not increase the number of accounts' do - expect do - post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers - end.to(not_change { Account.count }) - end - - it 'does not increase the number of follows' do - expect do - post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers - end.to(not_change { Follow.count }) - end - - it 'posts an acceptance to the remote actors inbox' do - post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers - expect( - a_request(:post, remote_actor_inbox_url) - ).to have_been_made.once - end - - it 'does not change the local record of the remote actor' do - post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers - expect(remote_actor.reload.username).to eq(remote_actor_original_username) - end - end - end end end From dbe2282153856aadb7bcf520714421703bd79857 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Thu, 23 May 2024 18:34:31 +0200 Subject: [PATCH 08/15] Change sidekiq inline invocation --- spec/requests/activitypub/inboxes_controller_spec.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb index c432911d525..4dbdea36c8b 100644 --- a/spec/requests/activitypub/inboxes_controller_spec.rb +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -RSpec.describe ActivityPub::InboxesController do +RSpec.describe ActivityPub::InboxesController, :sidekiq_inline do let!(:current_datetime) { 'Wed, 20 Dec 2023 10:00:00 GMT' } let!(:remote_actor_keypair) do OpenSSL::PKey.read(<<~PEM_TEXT) @@ -65,7 +65,6 @@ RSpec.describe ActivityPub::InboxesController do end before do - Sidekiq::Testing.inline! travel_to current_datetime end From 0c75781cfea402043a832365bc3126863097a413 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Thu, 23 May 2024 18:44:30 +0200 Subject: [PATCH 09/15] Allow username updates for Update Actor --- app/lib/activitypub/activity/update.rb | 2 +- app/services/activitypub/process_account_service.rb | 7 ++++++- spec/requests/activitypub/inboxes_controller_spec.rb | 4 ++-- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/app/lib/activitypub/activity/update.rb b/app/lib/activitypub/activity/update.rb index 973706f5957..71b6ba5ca41 100644 --- a/app/lib/activitypub/activity/update.rb +++ b/app/lib/activitypub/activity/update.rb @@ -20,7 +20,7 @@ class ActivityPub::Activity::Update < ActivityPub::Activity def update_account return reject_payload! if @account.uri != object_uri - ActivityPub::ProcessAccountService.new.call(@account.username, @account.domain, @object, signed_with_known_key: true, request_id: @options[:request_id]) + ActivityPub::ProcessAccountService.new.call(@account.username, @account.domain, @object, signed_with_known_key: true, request_id: @options[:request_id], allow_username_update: true) end def update_status diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index b667e97f4d8..47902024dea 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/app/services/activitypub/process_account_service.rb @@ -25,7 +25,7 @@ class ActivityPub::ProcessAccountService < BaseService @options[:request_id] ||= "#{Time.now.utc.to_i}-#{username}@#{domain}" with_redis_lock("process_account:#{@uri}") do - @account = Account.remote.find_by(uri: @uri) if @options[:only_key] + @account = Account.remote.find_by(uri: @uri) if find_remote_account_by_uri? @account ||= Account.find_remote(@username, @domain) @old_public_key = @account&.public_key @old_protocol = @account&.protocol @@ -67,6 +67,10 @@ class ActivityPub::ProcessAccountService < BaseService private + def find_remote_account_by_uri? + @options[:only_key] || @options[:allow_username_update] + end + def create_account @account = Account.new @account.protocol = :activitypub @@ -117,6 +121,7 @@ class ActivityPub::ProcessAccountService < BaseService @account.discoverable = @json['discoverable'] || false @account.indexable = @json['indexable'] || false @account.memorial = @json['memorial'] || false + @account.username = @json['preferredUsername'] if @options[:allow_username_update] end def set_fetchable_key! diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb index 4dbdea36c8b..94a0280c7d1 100644 --- a/spec/requests/activitypub/inboxes_controller_spec.rb +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -178,10 +178,10 @@ RSpec.describe ActivityPub::InboxesController, :sidekiq_inline do end.to(not_change { Account.count }) end - it 'does not update the remote actors username' do + it 'updates the remote actors username' do post "/users/#{local_actor.username}/inbox", params: json.to_json, headers: headers expect(response).to have_http_status(202) - expect(remote_actor.reload.username).to eq(remote_actor_original_username) + expect(remote_actor.reload.username).to eq(remote_actor_new_username) end end end From 138fee197cda1058b484b5ee9fc371b9c44e5479 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Fri, 24 May 2024 11:44:15 +0200 Subject: [PATCH 10/15] Don't update non unique usernames on remote domains --- app/lib/activitypub/activity/update.rb | 14 +++++- spec/lib/activitypub/activity/update_spec.rb | 48 +++++++++++++++++++- 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/app/lib/activitypub/activity/update.rb b/app/lib/activitypub/activity/update.rb index 71b6ba5ca41..5e190c227c4 100644 --- a/app/lib/activitypub/activity/update.rb +++ b/app/lib/activitypub/activity/update.rb @@ -20,7 +20,19 @@ class ActivityPub::Activity::Update < ActivityPub::Activity def update_account return reject_payload! if @account.uri != object_uri - ActivityPub::ProcessAccountService.new.call(@account.username, @account.domain, @object, signed_with_known_key: true, request_id: @options[:request_id], allow_username_update: true) + opts = { + signed_with_known_key: true, + request_id: @options[:request_id], + } + + if @account.username != @object['preferredUsername'] + account_proxy = @account.dup + account_proxy.username = @object['preferredUsername'] + UniqueUsernameValidator.new.validate(account_proxy) + opts[:allow_username_update] = true if account_proxy.errors.blank? + end + + ActivityPub::ProcessAccountService.new.call(@account.username, @account.domain, @object, opts) end def update_status diff --git a/spec/lib/activitypub/activity/update_spec.rb b/spec/lib/activitypub/activity/update_spec.rb index 87e96d2d1b1..5393e69bd9b 100644 --- a/spec/lib/activitypub/activity/update_spec.rb +++ b/spec/lib/activitypub/activity/update_spec.rb @@ -55,13 +55,57 @@ RSpec.describe ActivityPub::Activity::Update do stub_request(:get, actor_json[:following]).to_return(status: 404) stub_request(:get, actor_json[:featured]).to_return(status: 404) stub_request(:get, actor_json[:featuredTags]).to_return(status: 404) - - subject.perform end it 'updates profile' do + subject.perform expect(sender.reload.display_name).to eq 'Totally modified now' end + + context 'when Actor username changes' do + let!(:original_username) { sender.username } + let!(:updated_username) { 'updated_username' } + let(:updated_username_json) { actor_json.merge(preferredUsername: updated_username) } + let(:json) do + { + '@context': 'https://www.w3.org/ns/activitystreams', + id: 'foo', + type: 'Update', + actor: sender.uri, + object: updated_username_json, + }.with_indifferent_access + end + + it 'updates profile' do + subject.perform + expect(sender.reload.display_name).to eq 'Totally modified now' + end + + it 'updates username' do + subject.perform + expect(sender.reload.username).to eq updated_username + end + + context 'when updated username is not unique for domain' do + before do + Fabricate(:account, + username: updated_username, + domain: 'example.com', + inbox_url: "https://example.com/#{updated_username}/inbox", + outbox_url: "https://example.com/#{updated_username}/outbox") + end + + it 'updates profile' do + subject.perform + expect(sender.reload.display_name).to eq 'Totally modified now' + end + + it 'does not update username' do + subject.perform + expect(sender.reload.username).to eq original_username + end + end + end end context 'with a Question object' do From aed85da24ba08de9ccb56057d7955886908bd7b2 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Fri, 24 May 2024 11:50:42 +0200 Subject: [PATCH 11/15] Simplify ops assignment --- app/lib/activitypub/activity/update.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/lib/activitypub/activity/update.rb b/app/lib/activitypub/activity/update.rb index 5e190c227c4..1793b4137ba 100644 --- a/app/lib/activitypub/activity/update.rb +++ b/app/lib/activitypub/activity/update.rb @@ -29,7 +29,7 @@ class ActivityPub::Activity::Update < ActivityPub::Activity account_proxy = @account.dup account_proxy.username = @object['preferredUsername'] UniqueUsernameValidator.new.validate(account_proxy) - opts[:allow_username_update] = true if account_proxy.errors.blank? + opts[:allow_username_update] = account_proxy.errors.blank? end ActivityPub::ProcessAccountService.new.call(@account.username, @account.domain, @object, opts) From 60ba6d1c826d5d0ae514393788e0466bcb335e0b Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Wed, 12 Jun 2024 11:44:11 +0200 Subject: [PATCH 12/15] Add webfinger confirmation to username update scenario --- app/lib/activitypub/activity/update.rb | 24 +++++++--- spec/lib/activitypub/activity/update_spec.rb | 50 ++++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/app/lib/activitypub/activity/update.rb b/app/lib/activitypub/activity/update.rb index 1793b4137ba..b132258aec3 100644 --- a/app/lib/activitypub/activity/update.rb +++ b/app/lib/activitypub/activity/update.rb @@ -25,12 +25,7 @@ class ActivityPub::Activity::Update < ActivityPub::Activity request_id: @options[:request_id], } - if @account.username != @object['preferredUsername'] - account_proxy = @account.dup - account_proxy.username = @object['preferredUsername'] - UniqueUsernameValidator.new.validate(account_proxy) - opts[:allow_username_update] = account_proxy.errors.blank? - end + opts[:allow_username_update] = allow_username_update? if @account.username != @object['preferredUsername'] ActivityPub::ProcessAccountService.new.call(@account.username, @account.domain, @object, opts) end @@ -44,4 +39,21 @@ class ActivityPub::Activity::Update < ActivityPub::Activity ActivityPub::ProcessStatusUpdateService.new.call(@status, @json, @object, request_id: @options[:request_id]) end + + def allow_username_update? + updated_username_unique? && updated_username_confirmed? + end + + def updated_username_unique? + account_proxy = @account.dup + account_proxy.username = @object['preferredUsername'] + UniqueUsernameValidator.new.validate(account_proxy) + account_proxy.errors.blank? + end + + def updated_username_confirmed? + webfinger = Webfinger.new("acct:#{@object['preferredUsername']}@#{@account.domain}").perform + confirmed_username, confirmed_domain = webfinger.subject.delete_prefix('acct:').split('@') + confirmed_username == @object['preferredUsername'] && confirmed_domain == @account.domain + end end diff --git a/spec/lib/activitypub/activity/update_spec.rb b/spec/lib/activitypub/activity/update_spec.rb index 5393e69bd9b..716c4d7363f 100644 --- a/spec/lib/activitypub/activity/update_spec.rb +++ b/spec/lib/activitypub/activity/update_spec.rb @@ -64,7 +64,9 @@ RSpec.describe ActivityPub::Activity::Update do context 'when Actor username changes' do let!(:original_username) { sender.username } + let!(:original_handle) { "#{original_username}@#{sender.domain}" } let!(:updated_username) { 'updated_username' } + let!(:updated_handle) { "#{updated_username}@#{sender.domain}" } let(:updated_username_json) { actor_json.merge(preferredUsername: updated_username) } let(:json) do { @@ -75,6 +77,29 @@ RSpec.describe ActivityPub::Activity::Update do object: updated_username_json, }.with_indifferent_access end + let(:webfinger_response) do + { + subject: "acct:#{updated_handle}", + links: [ + { + rel: 'self', + type: 'application/activity+json', + href: sender.uri, + }, + ], + } + end + + before do + stub_request(:get, "https://example.com/.well-known/webfinger?resource=acct:#{updated_handle}") + .to_return( + body: webfinger_response.to_json, + headers: { + 'Content-Type' => 'application/json', + }, + status: 200 + ) + end it 'updates profile' do subject.perform @@ -105,6 +130,31 @@ RSpec.describe ActivityPub::Activity::Update do expect(sender.reload.username).to eq original_username end end + + context 'when updated username is not confirmed via webfinger' do + let(:webfinger_response) do + { + subject: "acct:#{original_handle}", + links: [ + { + rel: 'self', + type: 'application/activity+json', + href: sender.uri, + }, + ], + } + end + + it 'updates profile' do + subject.perform + expect(sender.reload.display_name).to eq 'Totally modified now' + end + + it 'does not update username' do + subject.perform + expect(sender.reload.username).to eq original_username + end + end end end From 26499e5096aed3560040b1cfcb878cdb53d4fe5e Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Wed, 12 Jun 2024 12:11:28 +0200 Subject: [PATCH 13/15] Fix failing specs --- .../activitypub/inboxes_controller_spec.rb | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/spec/requests/activitypub/inboxes_controller_spec.rb b/spec/requests/activitypub/inboxes_controller_spec.rb index 94a0280c7d1..f1c744a05f8 100644 --- a/spec/requests/activitypub/inboxes_controller_spec.rb +++ b/spec/requests/activitypub/inboxes_controller_spec.rb @@ -70,6 +70,7 @@ RSpec.describe ActivityPub::InboxesController, :sidekiq_inline do context 'when remote actor username has changed' do let(:remote_actor_new_username) { 'new_username' } + let(:remote_actor_new_handle) { "#{remote_actor_new_username}@#{remote_actor.domain}" } let(:updated_remote_actor_json) do { '@context': 'https://www.w3.org/ns/activitystreams', @@ -84,6 +85,18 @@ RSpec.describe ActivityPub::InboxesController, :sidekiq_inline do }, }.with_indifferent_access end + let(:remote_actor_webfinger_response) do + { + subject: "acct:#{remote_actor_new_handle}", + links: [ + { + rel: 'self', + type: 'application/activity+json', + href: remote_actor.uri, + }, + ], + } + end before do stub_request(:get, 'https://remote.domain/users/bob#main-key') @@ -102,6 +115,14 @@ RSpec.describe ActivityPub::InboxesController, :sidekiq_inline do }, status: 200 ) + stub_request(:get, "https://remote.domain/.well-known/webfinger?resource=acct:#{remote_actor_new_handle}") + .to_return( + body: remote_actor_webfinger_response.to_json, + headers: { + 'Content-Type' => 'application/json', + }, + status: 200 + ) end context 'with a create note' do From ceb9c8ff84682b3dde4ca9f4aa0bbd9ddcb6dd41 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Wed, 12 Jun 2024 13:17:53 +0200 Subject: [PATCH 14/15] Add more checks and tests --- app/lib/activitypub/activity/update.rb | 7 +- spec/lib/activitypub/activity/update_spec.rb | 103 ++++++++++++------- 2 files changed, 71 insertions(+), 39 deletions(-) diff --git a/app/lib/activitypub/activity/update.rb b/app/lib/activitypub/activity/update.rb index b132258aec3..1dfa6b73121 100644 --- a/app/lib/activitypub/activity/update.rb +++ b/app/lib/activitypub/activity/update.rb @@ -52,7 +52,12 @@ class ActivityPub::Activity::Update < ActivityPub::Activity end def updated_username_confirmed? - webfinger = Webfinger.new("acct:#{@object['preferredUsername']}@#{@account.domain}").perform + begin + webfinger = Webfinger.new("acct:#{@object['preferredUsername']}@#{@account.domain}").perform + rescue Webfinger::Error + return false + end + confirmed_username, confirmed_domain = webfinger.subject.delete_prefix('acct:').split('@') confirmed_username == @object['preferredUsername'] && confirmed_domain == @account.domain end diff --git a/spec/lib/activitypub/activity/update_spec.rb b/spec/lib/activitypub/activity/update_spec.rb index 716c4d7363f..7490855a873 100644 --- a/spec/lib/activitypub/activity/update_spec.rb +++ b/spec/lib/activitypub/activity/update_spec.rb @@ -77,38 +77,41 @@ RSpec.describe ActivityPub::Activity::Update do object: updated_username_json, }.with_indifferent_access end - let(:webfinger_response) do - { - subject: "acct:#{updated_handle}", - links: [ - { - rel: 'self', - type: 'application/activity+json', - href: sender.uri, - }, - ], - } - end before do - stub_request(:get, "https://example.com/.well-known/webfinger?resource=acct:#{updated_handle}") - .to_return( - body: webfinger_response.to_json, - headers: { - 'Content-Type' => 'application/json', - }, - status: 200 - ) + stub_request(:get, 'https://example.com/.well-known/host-meta').to_return(status: 404) end - it 'updates profile' do - subject.perform - expect(sender.reload.display_name).to eq 'Totally modified now' - end + context 'when updated username is unique and confirmed' do + before do + stub_request(:get, "https://example.com/.well-known/webfinger?resource=acct:#{updated_handle}") + .to_return( + body: { + subject: "acct:#{updated_handle}", + links: [ + { + rel: 'self', + type: 'application/activity+json', + href: sender.uri, + }, + ], + }.to_json, + headers: { + 'Content-Type' => 'application/json', + }, + status: 200 + ) + end - it 'updates username' do - subject.perform - expect(sender.reload.username).to eq updated_username + it 'updates profile' do + subject.perform + expect(sender.reload.display_name).to eq 'Totally modified now' + end + + it 'updates username' do + subject.perform + expect(sender.reload.username).to eq updated_username + end end context 'when updated username is not unique for domain' do @@ -131,18 +134,42 @@ RSpec.describe ActivityPub::Activity::Update do end end - context 'when updated username is not confirmed via webfinger' do - let(:webfinger_response) do - { - subject: "acct:#{original_handle}", - links: [ - { - rel: 'self', - type: 'application/activity+json', - href: sender.uri, + context 'when webfinger of updated username does not contain updated username' do + before do + stub_request(:get, "https://example.com/.well-known/webfinger?resource=acct:#{updated_handle}") + .to_return( + body: { + subject: "acct:#{original_handle}", + links: [ + { + rel: 'self', + type: 'application/activity+json', + href: sender.uri, + }, + ], + }.to_json, + headers: { + 'Content-Type' => 'application/json', }, - ], - } + status: 200 + ) + end + + it 'updates profile' do + subject.perform + expect(sender.reload.display_name).to eq 'Totally modified now' + end + + it 'does not update username' do + subject.perform + expect(sender.reload.username).to eq original_username + end + end + + context 'when webfinger request of updated username fails' do + before do + stub_request(:get, "https://example.com/.well-known/webfinger?resource=acct:#{updated_handle}") + .to_return(status: 404) end it 'updates profile' do From d7cd60dfdd29057a1356ada92d0c5c6fa4bc3c07 Mon Sep 17 00:00:00 2001 From: Angus McLeod Date: Wed, 12 Jun 2024 13:19:57 +0200 Subject: [PATCH 15/15] Use shared_example for does not update username scenarios --- spec/lib/activitypub/activity/update_spec.rb | 42 +++++++------------- 1 file changed, 15 insertions(+), 27 deletions(-) diff --git a/spec/lib/activitypub/activity/update_spec.rb b/spec/lib/activitypub/activity/update_spec.rb index 7490855a873..3fbb799516e 100644 --- a/spec/lib/activitypub/activity/update_spec.rb +++ b/spec/lib/activitypub/activity/update_spec.rb @@ -114,15 +114,7 @@ RSpec.describe ActivityPub::Activity::Update do end end - context 'when updated username is not unique for domain' do - before do - Fabricate(:account, - username: updated_username, - domain: 'example.com', - inbox_url: "https://example.com/#{updated_username}/inbox", - outbox_url: "https://example.com/#{updated_username}/outbox") - end - + shared_examples 'does not update username' do it 'updates profile' do subject.perform expect(sender.reload.display_name).to eq 'Totally modified now' @@ -134,6 +126,18 @@ RSpec.describe ActivityPub::Activity::Update do end end + context 'when updated username is not unique for domain' do + before do + Fabricate(:account, + username: updated_username, + domain: 'example.com', + inbox_url: "https://example.com/#{updated_username}/inbox", + outbox_url: "https://example.com/#{updated_username}/outbox") + end + + include_examples 'does not update username' + end + context 'when webfinger of updated username does not contain updated username' do before do stub_request(:get, "https://example.com/.well-known/webfinger?resource=acct:#{updated_handle}") @@ -155,15 +159,7 @@ RSpec.describe ActivityPub::Activity::Update do ) end - it 'updates profile' do - subject.perform - expect(sender.reload.display_name).to eq 'Totally modified now' - end - - it 'does not update username' do - subject.perform - expect(sender.reload.username).to eq original_username - end + include_examples 'does not update username' end context 'when webfinger request of updated username fails' do @@ -172,15 +168,7 @@ RSpec.describe ActivityPub::Activity::Update do .to_return(status: 404) end - it 'updates profile' do - subject.perform - expect(sender.reload.display_name).to eq 'Totally modified now' - end - - it 'does not update username' do - subject.perform - expect(sender.reload.username).to eq original_username - end + include_examples 'does not update username' end end end