From 8ac7fca5d06b776cc09127f9c6a9018e9acade5b Mon Sep 17 00:00:00 2001 From: Patrick Figel Date: Sat, 29 Apr 2017 00:18:32 +0200 Subject: [PATCH] Set correct attachment type for rejected media (#2599) In #2110, a new attachment type "unknown" was introduced for attachments that were rejected due to a domain being blocked using reject_media. However, the "type" field was never set to "unknown" because a default value of "0" (image) is set for that column, causing the `type.blank?` expression to always equal false. This version uses type_changed? instead, causing the type to be set to "unknown" unless a type has been explicitly set. This introduces a small change in behaviour causing the type to be set to unknown before paperclip calls `before_post_process`. Presumably this behaviour is more appropriate than the current one because the attachment type has not been determined by that point. Included are new tests for `ProcessFeedService` and `UpdateRemoteProfileService` which now check that remote media is downloaded for non-blocked domains and is rejected for others. --- app/models/media_attachment.rb | 2 +- spec/services/process_feed_service_spec.rb | 95 +++++++++++++------ .../update_remote_profile_service_spec.rb | 25 +++++ 3 files changed, 93 insertions(+), 29 deletions(-) diff --git a/app/models/media_attachment.rb b/app/models/media_attachment.rb index aae896d20..2ebddcb69 100644 --- a/app/models/media_attachment.rb +++ b/app/models/media_attachment.rb @@ -96,7 +96,7 @@ class MediaAttachment < ApplicationRecord private def set_shortcode - self.type = :unknown if file.blank? && type.blank? + self.type = :unknown if file.blank? && !type_changed? return unless local? diff --git a/spec/services/process_feed_service_spec.rb b/spec/services/process_feed_service_spec.rb index 7f2b4ce1d..19054ecd4 100644 --- a/spec/services/process_feed_service_spec.rb +++ b/spec/services/process_feed_service_spec.rb @@ -12,43 +12,82 @@ RSpec.describe ProcessFeedService do stub_request(:get, "http://kickass.zone/system/accounts/avatars/000/000/001/large/eris.png").to_return(request_fixture('avatar.txt')) stub_request(:get, "http://kickass.zone/system/media_attachments/files/000/000/002/original/morpheus_linux.jpg?1476059910").to_return(request_fixture('attachment1.txt')) stub_request(:get, "http://kickass.zone/system/media_attachments/files/000/000/003/original/gizmo.jpg?1476060065").to_return(request_fixture('attachment2.txt')) - - subject.call(body, account) end - it 'updates remote user\'s account information' do - account.reload - expect(account.display_name).to eq '::1' - expect(account).to have_attached_file(:avatar) + context 'when domain does not reject media' do + before do + subject.call(body, account) + end + + it 'updates remote user\'s account information' do + account.reload + expect(account.display_name).to eq '::1' + expect(account).to have_attached_file(:avatar) + expect(account.avatar_file_name).not_to be_nil + end + + it 'creates posts' do + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Status')).to_not be_nil + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Status')).to_not be_nil + end + + it 'ignores delete statuses unless they existed before' do + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Status')).to be_nil + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=12:objectType=Status')).to be_nil + end + + it 'does not create statuses for follows' do + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Follow')).to be_nil + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Follow')).to be_nil + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=4:objectType=Follow')).to be_nil + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=7:objectType=Follow')).to be_nil + end + + it 'does not create statuses for favourites' do + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Favourite')).to be_nil + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Favourite')).to be_nil + end + + it 'creates posts with media' do + status = Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=14:objectType=Status') + + expect(status).to_not be_nil + expect(status.media_attachments.first).to have_attached_file(:file) + expect(status.media_attachments.first.image?).to be true + expect(status.media_attachments.first.file_file_name).not_to be_nil + end end - it 'creates posts' do - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Status')).to_not be_nil - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Status')).to_not be_nil - end + context 'when domain is set to reject media' do + let!(:domain_block) { Fabricate(:domain_block, domain: 'kickass.zone', reject_media: true) } - it 'ignores delete statuses unless they existed before' do - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Status')).to be_nil - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=12:objectType=Status')).to be_nil - end + before do + subject.call(body, account) + end - it 'does not create statuses for follows' do - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Follow')).to be_nil - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Follow')).to be_nil - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=4:objectType=Follow')).to be_nil - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=7:objectType=Follow')).to be_nil - end + it 'updates remote user\'s account information' do + account.reload + expect(account.display_name).to eq '::1' + end - it 'does not create statuses for favourites' do - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Favourite')).to be_nil - expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=3:objectType=Favourite')).to be_nil - end + it 'rejects remote user\'s avatar' do + account.reload + expect(account.display_name).to eq '::1' + expect(account.avatar_file_name).to be_nil + end - it 'creates posts with media' do - status = Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=14:objectType=Status') + it 'creates posts' do + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=1:objectType=Status')).to_not be_nil + expect(Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=2:objectType=Status')).to_not be_nil + end - expect(status).to_not be_nil - expect(status.media_attachments.first).to have_attached_file(:file) + it 'creates posts with remote-only media' do + status = Status.find_by(uri: 'tag:kickass.zone,2016-10-10:objectId=14:objectType=Status') + + expect(status).to_not be_nil + expect(status.media_attachments.first.file_file_name).to be_nil + expect(status.media_attachments.first.unknown?).to be true + end end end diff --git a/spec/services/update_remote_profile_service_spec.rb b/spec/services/update_remote_profile_service_spec.rb index f96f2f6ba..64ec2dbbb 100644 --- a/spec/services/update_remote_profile_service_spec.rb +++ b/spec/services/update_remote_profile_service_spec.rb @@ -56,4 +56,29 @@ RSpec.describe UpdateRemoteProfileService do expect(remote_account.reload.note).to eq 'Software engineer, free time musician and ＤＩＧＩＴＡＬ ＳＰＯＲＴＳ enthusiast. Likes cats. Warning: May contain memes' end end + + context 'with updated details from a domain set to reject media' do + let(:remote_account) { Fabricate(:account, username: 'bob', domain: 'example.com') } + let!(:domain_block) { Fabricate(:domain_block, domain: 'example.com', reject_media: true) } + + before do + subject.call(xml, remote_account) + end + + it 'does not the avatar remote url' do + expect(remote_account.reload.avatar_remote_url).to be_nil + end + + it 'sets display name' do + expect(remote_account.reload.display_name).to eq 'ＤＩＧＩＴＡＬ ＣＡＴ' + end + + it 'sets note' do + expect(remote_account.reload.note).to eq 'Software engineer, free time musician and ＤＩＧＩＴＡＬ ＳＰＯＲＴＳ enthusiast. Likes cats. Warning: May contain memes' + end + + it 'does not set store the avatar' do + expect(remote_account.reload.avatar_file_name).to be_nil + end + end end