From d5c9feb7b7fc489afbd0a287431fe07b42451ef0 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 17 Jan 2022 00:49:55 +0100 Subject: [PATCH] Add support for private pinned posts (#16954) * Add support for private pinned toots * Allow local user to pin private toots * Change wording to avoid "direct message" --- app/controllers/accounts_controller.rb | 13 +++++- .../activitypub/collections_controller.rb | 1 + .../api/v1/accounts/statuses_controller.rb | 4 +- .../mastodon/components/status_action_bar.js | 3 +- .../features/status/components/action_bar.js | 3 +- app/lib/activitypub/activity/accept.rb | 13 +++++- app/lib/activitypub/activity/add.rb | 3 +- .../fetch_featured_collection_service.rb | 6 ++- app/validators/status_pin_validator.rb | 2 +- app/workers/remote_account_refresh_worker.rb | 24 +++++++++++ config/locales/en.yml | 2 +- spec/controllers/accounts_controller_spec.rb | 1 + .../collections_controller_spec.rb | 23 +++++++++- .../v1/accounts/statuses_controller_spec.rb | 35 +++++++++++++++- spec/lib/activitypub/activity/accept_spec.rb | 5 +++ spec/lib/activitypub/activity/add_spec.rb | 42 ++++++++++++++++--- spec/models/status_pin_spec.rb | 4 +- spec/validators/status_pin_validator_spec.rb | 8 ++-- 18 files changed, 164 insertions(+), 28 deletions(-) create mode 100644 app/workers/remote_account_refresh_worker.rb diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 8210918d86..ddd38cbb01 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -28,7 +28,7 @@ class AccountsController < ApplicationController return end - @pinned_statuses = cache_collection(@account.pinned_statuses, Status) if show_pinned_statuses? + @pinned_statuses = cached_filtered_status_pins if show_pinned_statuses? @statuses = cached_filtered_status_page @rss_url = rss_url @@ -64,6 +64,10 @@ class AccountsController < ApplicationController [replies_requested?, media_requested?, tag_requested?, params[:max_id].present?, params[:min_id].present?].none? end + def filtered_pinned_statuses + @account.pinned_statuses.where(visibility: [:public, :unlisted]) + end + def filtered_statuses default_statuses.tap do |statuses| statuses.merge!(hashtag_scope) if tag_requested? @@ -142,6 +146,13 @@ class AccountsController < ApplicationController request.path.split('.').first.end_with?(Addressable::URI.parse("/tagged/#{params[:tag]}").normalize) end + def cached_filtered_status_pins + cache_collection( + filtered_pinned_statuses, + Status + ) + end + def cached_filtered_status_page cache_collection_paginated_by_id( filtered_statuses, diff --git a/app/controllers/activitypub/collections_controller.rb b/app/controllers/activitypub/collections_controller.rb index c8b6dcc88d..e4e994a985 100644 --- a/app/controllers/activitypub/collections_controller.rb +++ b/app/controllers/activitypub/collections_controller.rb @@ -21,6 +21,7 @@ class ActivityPub::CollectionsController < ActivityPub::BaseController case params[:id] when 'featured' @items = for_signed_account { cache_collection(@account.pinned_statuses, Status) } + @items = @items.map { |item| item.distributable? ? item : ActivityPub::TagManager.instance.uri_for(item) } when 'tags' @items = for_signed_account { @account.featured_tags } when 'devices' diff --git a/app/controllers/api/v1/accounts/statuses_controller.rb b/app/controllers/api/v1/accounts/statuses_controller.rb index 92ccb80615..2c027ea769 100644 --- a/app/controllers/api/v1/accounts/statuses_controller.rb +++ b/app/controllers/api/v1/accounts/statuses_controller.rb @@ -46,9 +46,7 @@ class Api::V1::Accounts::StatusesController < Api::BaseController end def pinned_scope - return Status.none if @account.blocking?(current_account) - - @account.pinned_statuses + @account.pinned_statuses.permitted_for(@account, current_account) end def no_replies_scope diff --git a/app/javascript/mastodon/components/status_action_bar.js b/app/javascript/mastodon/components/status_action_bar.js index 85c76edee4..d125359e9e 100644 --- a/app/javascript/mastodon/components/status_action_bar.js +++ b/app/javascript/mastodon/components/status_action_bar.js @@ -225,6 +225,7 @@ class StatusActionBar extends ImmutablePureComponent { const anonymousAccess = !me; const publicStatus = ['public', 'unlisted'].includes(status.get('visibility')); + const pinnableStatus = ['public', 'unlisted', 'private'].includes(status.get('visibility')); const mutingConversation = status.get('muted'); const account = status.get('account'); const writtenByMe = status.getIn(['account', 'id']) === me; @@ -242,7 +243,7 @@ class StatusActionBar extends ImmutablePureComponent { menu.push({ text: intl.formatMessage(status.get('bookmarked') ? messages.removeBookmark : messages.bookmark), action: this.handleBookmarkClick }); - if (writtenByMe && publicStatus) { + if (writtenByMe && pinnableStatus) { menu.push({ text: intl.formatMessage(status.get('pinned') ? messages.unpin : messages.pin), action: this.handlePinClick }); } diff --git a/app/javascript/mastodon/features/status/components/action_bar.js b/app/javascript/mastodon/features/status/components/action_bar.js index ffa2510c0d..e60119bc47 100644 --- a/app/javascript/mastodon/features/status/components/action_bar.js +++ b/app/javascript/mastodon/features/status/components/action_bar.js @@ -188,6 +188,7 @@ class ActionBar extends React.PureComponent { const { status, relationship, intl } = this.props; const publicStatus = ['public', 'unlisted'].includes(status.get('visibility')); + const pinnableStatus = ['public', 'unlisted', 'private'].includes(status.get('visibility')); const mutingConversation = status.get('muted'); const account = status.get('account'); const writtenByMe = status.getIn(['account', 'id']) === me; @@ -201,7 +202,7 @@ class ActionBar extends React.PureComponent { } if (writtenByMe) { - if (publicStatus) { + if (pinnableStatus) { menu.push({ text: intl.formatMessage(status.get('pinned') ? messages.unpin : messages.pin), action: this.handlePinClick }); menu.push(null); } diff --git a/app/lib/activitypub/activity/accept.rb b/app/lib/activitypub/activity/accept.rb index 7010ff43e8..5126e23c6a 100644 --- a/app/lib/activitypub/activity/accept.rb +++ b/app/lib/activitypub/activity/accept.rb @@ -3,7 +3,7 @@ class ActivityPub::Activity::Accept < ActivityPub::Activity def perform return accept_follow_for_relay if relay_follow? - return follow_request_from_object.authorize! unless follow_request_from_object.nil? + return accept_follow!(follow_request_from_object) unless follow_request_from_object.nil? case @object['type'] when 'Follow' @@ -19,7 +19,16 @@ class ActivityPub::Activity::Accept < ActivityPub::Activity return if target_account.nil? || !target_account.local? follow_request = FollowRequest.find_by(account: target_account, target_account: @account) - follow_request&.authorize! + accept_follow!(follow_request) + end + + def accept_follow!(request) + return if request.nil? + + is_first_follow = !request.target_account.followers.local.exists? + request.authorize! + + RemoteAccountRefreshWorker.perform_async(request.target_account_id) if is_first_follow end def accept_follow_for_relay diff --git a/app/lib/activitypub/activity/add.rb b/app/lib/activitypub/activity/add.rb index 688ab00b33..845eeaef79 100644 --- a/app/lib/activitypub/activity/add.rb +++ b/app/lib/activitypub/activity/add.rb @@ -4,8 +4,7 @@ class ActivityPub::Activity::Add < ActivityPub::Activity def perform return unless @json['target'].present? && value_or_id(@json['target']) == @account.featured_collection_url - status = status_from_uri(object_uri) - status ||= fetch_remote_original_status + status = status_from_object return unless !status.nil? && status.account_id == @account.id && !@account.pinned?(status) diff --git a/app/services/activitypub/fetch_featured_collection_service.rb b/app/services/activitypub/fetch_featured_collection_service.rb index 72352aca6f..9fce478c14 100644 --- a/app/services/activitypub/fetch_featured_collection_service.rb +++ b/app/services/activitypub/fetch_featured_collection_service.rb @@ -23,7 +23,7 @@ class ActivityPub::FetchFeaturedCollectionService < BaseService def process_items(items) status_ids = items.map { |item| value_or_id(item) } - .filter_map { |uri| ActivityPub::FetchRemoteStatusService.new.call(uri) unless ActivityPub::TagManager.instance.local_uri?(uri) } + .filter_map { |uri| ActivityPub::FetchRemoteStatusService.new.call(uri, on_behalf_of: local_follower) unless ActivityPub::TagManager.instance.local_uri?(uri) } .filter_map { |status| status.id if status.account_id == @account.id } to_remove = [] to_add = status_ids @@ -46,4 +46,8 @@ class ActivityPub::FetchFeaturedCollectionService < BaseService def supported_context? super(@json) end + + def local_follower + @local_follower ||= account.followers.local.without_suspended.first + end end diff --git a/app/validators/status_pin_validator.rb b/app/validators/status_pin_validator.rb index 2c7bce674b..2fdd5b34fd 100644 --- a/app/validators/status_pin_validator.rb +++ b/app/validators/status_pin_validator.rb @@ -4,7 +4,7 @@ class StatusPinValidator < ActiveModel::Validator def validate(pin) pin.errors.add(:base, I18n.t('statuses.pin_errors.reblog')) if pin.status.reblog? pin.errors.add(:base, I18n.t('statuses.pin_errors.ownership')) if pin.account_id != pin.status.account_id - pin.errors.add(:base, I18n.t('statuses.pin_errors.private')) unless %w(public unlisted).include?(pin.status.visibility) + pin.errors.add(:base, I18n.t('statuses.pin_errors.direct')) if pin.status.direct_visibility? pin.errors.add(:base, I18n.t('statuses.pin_errors.limit')) if pin.account.status_pins.count > 4 && pin.account.local? end end diff --git a/app/workers/remote_account_refresh_worker.rb b/app/workers/remote_account_refresh_worker.rb new file mode 100644 index 0000000000..9632936b54 --- /dev/null +++ b/app/workers/remote_account_refresh_worker.rb @@ -0,0 +1,24 @@ +# frozen_string_literal: true + +class RemoteAccountRefreshWorker + include Sidekiq::Worker + include ExponentialBackoff + include JsonLdHelper + + sidekiq_options queue: 'pull', retry: 3 + + def perform(id) + account = Account.find_by(id: id) + return if account.nil? || account.local? + + ActivityPub::FetchRemoteAccountService.new.call(account.uri) + rescue Mastodon::UnexpectedResponseError => e + response = e.response + + if response_error_unsalvageable?(response) + # Give up + else + raise e + end + end +end diff --git a/config/locales/en.yml b/config/locales/en.yml index 32b48dbfff..693a7b4008 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1300,9 +1300,9 @@ en: open_in_web: Open in web over_character_limit: character limit of %{max} exceeded pin_errors: + direct: Posts that are only visible to mentioned users cannot be pinned limit: You have already pinned the maximum number of posts ownership: Someone else's post cannot be pinned - private: Non-public posts cannot be pinned reblog: A boost cannot be pinned poll: total_people: diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index ac426b01e6..7c5ba8754d 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -35,6 +35,7 @@ RSpec.describe AccountsController, type: :controller do before do status_media.media_attachments << Fabricate(:media_attachment, account: account, type: :image) account.pinned_statuses << status_pinned + account.pinned_statuses << status_private end shared_examples 'preliminary checks' do diff --git a/spec/controllers/activitypub/collections_controller_spec.rb b/spec/controllers/activitypub/collections_controller_spec.rb index d584136ff5..21a0339458 100644 --- a/spec/controllers/activitypub/collections_controller_spec.rb +++ b/spec/controllers/activitypub/collections_controller_spec.rb @@ -4,6 +4,7 @@ require 'rails_helper' RSpec.describe ActivityPub::CollectionsController, type: :controller do let!(:account) { Fabricate(:account) } + let!(:private_pinned) { Fabricate(:status, account: account, text: 'secret private stuff', visibility: :private) } let(:remote_account) { nil } shared_examples 'cachable response' do @@ -27,6 +28,7 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do Fabricate(:status_pin, account: account) Fabricate(:status_pin, account: account) + Fabricate(:status_pin, account: account, status: private_pinned) Fabricate(:status, account: account, visibility: :private) end @@ -50,7 +52,15 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do it 'returns orderedItems with pinned statuses' do expect(body[:orderedItems]).to be_an Array - expect(body[:orderedItems].size).to eq 2 + expect(body[:orderedItems].size).to eq 3 + end + + it 'includes URI of private pinned status' do + expect(body[:orderedItems]).to include(ActivityPub::TagManager.instance.uri_for(private_pinned)) + end + + it 'does not include contents of private pinned status' do + expect(response.body).not_to include(private_pinned.text) end context 'when account is permanently suspended' do @@ -96,7 +106,16 @@ RSpec.describe ActivityPub::CollectionsController, type: :controller do it 'returns orderedItems with pinned statuses' do json = body_as_json expect(json[:orderedItems]).to be_an Array - expect(json[:orderedItems].size).to eq 2 + expect(json[:orderedItems].size).to eq 3 + end + + it 'includes URI of private pinned status' do + json = body_as_json + expect(json[:orderedItems]).to include(ActivityPub::TagManager.instance.uri_for(private_pinned)) + end + + it 'does not include contents of private pinned status' do + expect(response.body).not_to include(private_pinned.text) end end diff --git a/spec/controllers/api/v1/accounts/statuses_controller_spec.rb b/spec/controllers/api/v1/accounts/statuses_controller_spec.rb index 693cd1ac66..0a18ddcbdd 100644 --- a/spec/controllers/api/v1/accounts/statuses_controller_spec.rb +++ b/spec/controllers/api/v1/accounts/statuses_controller_spec.rb @@ -39,7 +39,7 @@ describe Api::V1::Accounts::StatusesController do end end - context 'with only pinned' do + context 'with only own pinned' do before do Fabricate(:status_pin, account: user.account, status: Fabricate(:status, account: user.account)) end @@ -50,5 +50,38 @@ describe Api::V1::Accounts::StatusesController do expect(response).to have_http_status(200) end end + + context "with someone else's pinned statuses" do + let(:account) { Fabricate(:account, username: 'bob', domain: 'example.com') } + let(:status) { Fabricate(:status, account: account) } + let(:private_status) { Fabricate(:status, account: account, visibility: :private) } + let!(:pin) { Fabricate(:status_pin, account: account, status: status) } + let!(:private_pin) { Fabricate(:status_pin, account: account, status: private_status) } + + it 'returns http success' do + get :index, params: { account_id: account.id, pinned: true } + expect(response).to have_http_status(200) + end + + context 'when user does not follow account' do + it 'lists the public status only' do + get :index, params: { account_id: account.id, pinned: true } + json = body_as_json + expect(json.map { |item| item[:id].to_i }).to eq [status.id] + end + end + + context 'when user follows account' do + before do + user.account.follow!(account) + end + + it 'lists both the public and the private statuses' do + get :index, params: { account_id: account.id, pinned: true } + json = body_as_json + expect(json.map { |item| item[:id].to_i }.sort).to eq [status.id, private_status.id].sort + end + end + end end end diff --git a/spec/lib/activitypub/activity/accept_spec.rb b/spec/lib/activitypub/activity/accept_spec.rb index 883bab6ac9..304cf2208a 100644 --- a/spec/lib/activitypub/activity/accept_spec.rb +++ b/spec/lib/activitypub/activity/accept_spec.rb @@ -23,6 +23,7 @@ RSpec.describe ActivityPub::Activity::Accept do subject { described_class.new(json, sender) } before do + allow(RemoteAccountRefreshWorker).to receive(:perform_async) Fabricate(:follow_request, account: recipient, target_account: sender) subject.perform end @@ -34,6 +35,10 @@ RSpec.describe ActivityPub::Activity::Accept do it 'removes the follow request' do expect(recipient.requested?(sender)).to be false end + + it 'queues a refresh' do + expect(RemoteAccountRefreshWorker).to have_received(:perform_async).with(sender.id) + end end context 'given a relay' do diff --git a/spec/lib/activitypub/activity/add_spec.rb b/spec/lib/activitypub/activity/add_spec.rb index 16db71c880..e6408b610f 100644 --- a/spec/lib/activitypub/activity/add_spec.rb +++ b/spec/lib/activitypub/activity/add_spec.rb @@ -1,8 +1,8 @@ require 'rails_helper' RSpec.describe ActivityPub::Activity::Add do - let(:sender) { Fabricate(:account, featured_collection_url: 'https://example.com/featured') } - let(:status) { Fabricate(:status, account: sender) } + let(:sender) { Fabricate(:account, featured_collection_url: 'https://example.com/featured', domain: 'example.com') } + let(:status) { Fabricate(:status, account: sender, visibility: :private) } let(:json) do { @@ -24,6 +24,8 @@ RSpec.describe ActivityPub::Activity::Add do end context 'when status was not known before' do + let(:service_stub) { double } + let(:json) do { '@context': 'https://www.w3.org/ns/activitystreams', @@ -36,12 +38,40 @@ RSpec.describe ActivityPub::Activity::Add do end before do - stub_request(:get, 'https://example.com/unknown').to_return(status: 410) + allow(ActivityPub::FetchRemoteStatusService).to receive(:new).and_return(service_stub) end - it 'fetches the status' do - subject.perform - expect(a_request(:get, 'https://example.com/unknown')).to have_been_made.at_least_once + context 'when there is a local follower' do + before do + account = Fabricate(:account) + account.follow!(sender) + end + + it 'fetches the status and pins it' do + allow(service_stub).to receive(:call) do |uri, id: true, on_behalf_of: nil| + expect(uri).to eq 'https://example.com/unknown' + expect(id).to eq true + expect(on_behalf_of&.following?(sender)).to eq true + status + end + subject.perform + expect(service_stub).to have_received(:call) + expect(sender.pinned?(status)).to be true + end + end + + context 'when there is no local follower' do + it 'tries to fetch the status' do + allow(service_stub).to receive(:call) do |uri, id: true, on_behalf_of: nil| + expect(uri).to eq 'https://example.com/unknown' + expect(id).to eq true + expect(on_behalf_of).to eq nil + nil + end + subject.perform + expect(service_stub).to have_received(:call) + expect(sender.pinned?(status)).to be false + end end end end diff --git a/spec/models/status_pin_spec.rb b/spec/models/status_pin_spec.rb index 6f0b2feb8b..c18faca782 100644 --- a/spec/models/status_pin_spec.rb +++ b/spec/models/status_pin_spec.rb @@ -24,11 +24,11 @@ RSpec.describe StatusPin, type: :model do expect(StatusPin.new(account: account, status: reblog).save).to be false end - it 'does not allow pins of private statuses' do + it 'does allow pins of direct statuses' do account = Fabricate(:account) status = Fabricate(:status, account: account, visibility: :private) - expect(StatusPin.new(account: account, status: status).save).to be false + expect(StatusPin.new(account: account, status: status).save).to be true end it 'does not allow pins of direct statuses' do diff --git a/spec/validators/status_pin_validator_spec.rb b/spec/validators/status_pin_validator_spec.rb index 06532e5b3f..d5bd0d1b83 100644 --- a/spec/validators/status_pin_validator_spec.rb +++ b/spec/validators/status_pin_validator_spec.rb @@ -9,7 +9,7 @@ RSpec.describe StatusPinValidator, type: :validator do end let(:pin) { double(account: account, errors: errors, status: status, account_id: pin_account_id) } - let(:status) { double(reblog?: reblog, account_id: status_account_id, visibility: visibility) } + let(:status) { double(reblog?: reblog, account_id: status_account_id, visibility: visibility, direct_visibility?: visibility == 'direct') } let(:account) { double(status_pins: status_pins, local?: local) } let(:status_pins) { double(count: count) } let(:errors) { double(add: nil) } @@ -37,11 +37,11 @@ RSpec.describe StatusPinValidator, type: :validator do end end - context 'unless %w(public unlisted).include?(pin.status.visibility)' do - let(:visibility) { '' } + context 'if pin.status.direct_visibility?' do + let(:visibility) { 'direct' } it 'calls errors.add' do - expect(errors).to have_received(:add).with(:base, I18n.t('statuses.pin_errors.private')) + expect(errors).to have_received(:add).with(:base, I18n.t('statuses.pin_errors.direct')) end end