From 122d59ac41a0c637b19357c2b7422002ffa0381c Mon Sep 17 00:00:00 2001 From: Evan Minto Date: Tue, 25 Apr 2017 06:06:06 -0700 Subject: [PATCH] Change ActivityPub paging to match spec. Clean up ActivityPub outbox changes. (#2410) * Change ActivityPub paging to match spec. Clean up ActivityPub outbox changes. * Fix code style and test failures for OutboxController. * Attempt to fix CI errors. --- app/controllers/accounts_controller.rb | 4 +- .../api/activitypub/activities_controller.rb | 2 - .../api/activitypub/notes_controller.rb | 2 - .../api/activitypub/outbox_controller.rb | 60 +++++-- .../activitystreams2_builder_helper.rb | 2 +- .../types/collection.activitystreams2.rabl | 2 - ...ered_collection_page.activitystreams2.rabl | 1 - .../outbox/show.activitystreams2.rabl | 21 +-- .../outbox/show_page.activitystreams2.rabl | 16 ++ config/application.rb | 2 +- config/locales/en.yml | 4 +- .../activitypub/activities_controller_spec.rb | 14 +- .../api/activitypub/notes_controller_spec.rb | 14 +- .../api/activitypub/outbox_controller_spec.rb | 148 +++++++++++++----- 14 files changed, 182 insertions(+), 110 deletions(-) create mode 100644 app/views/api/activitypub/outbox/show_page.activitystreams2.rabl diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index d79ed142a..8eda96336 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -15,9 +15,7 @@ class AccountsController < ApplicationController render xml: AtomSerializer.render(AtomSerializer.new.feed(@account, @entries.to_a)) end - format.activitystreams2 do - headers['Access-Control-Allow-Origin'] = '*' - end + format.activitystreams2 end end diff --git a/app/controllers/api/activitypub/activities_controller.rb b/app/controllers/api/activitypub/activities_controller.rb index 03f27c7f6..ba03738fc 100644 --- a/app/controllers/api/activitypub/activities_controller.rb +++ b/app/controllers/api/activitypub/activities_controller.rb @@ -8,8 +8,6 @@ class Api::Activitypub::ActivitiesController < ApiController # Show a status in AS2 format, as either an Announce (reblog) or a Create (post) activity. def show_status - headers['Access-Control-Allow-Origin'] = '*' - return forbidden unless @status.permitted? if @status.reblog? diff --git a/app/controllers/api/activitypub/notes_controller.rb b/app/controllers/api/activitypub/notes_controller.rb index 722961ec6..6489243dc 100644 --- a/app/controllers/api/activitypub/notes_controller.rb +++ b/app/controllers/api/activitypub/notes_controller.rb @@ -6,8 +6,6 @@ class Api::Activitypub::NotesController < ApiController respond_to :activitystreams2 def show - headers['Access-Control-Allow-Origin'] = '*' - forbidden unless @status.permitted? end diff --git a/app/controllers/api/activitypub/outbox_controller.rb b/app/controllers/api/activitypub/outbox_controller.rb index 05d779910..7b6cbdd38 100644 --- a/app/controllers/api/activitypub/outbox_controller.rb +++ b/app/controllers/api/activitypub/outbox_controller.rb @@ -6,30 +6,47 @@ class Api::Activitypub::OutboxController < ApiController respond_to :activitystreams2 def show - headers['Access-Control-Allow-Origin'] = '*' + if params[:max_id] || params[:since_id] + show_outbox_page + else + show_base_outbox + end + end - @statuses = Status.as_outbox_timeline(@account).paginate_by_max_id(limit_param(DEFAULT_STATUSES_LIMIT), params[:max_id], params[:since_id]) + private + + def show_base_outbox + @statuses = Status.as_outbox_timeline(@account) @statuses = cache_collection(@statuses) set_maps(@statuses) - # Since the statuses are in reverse chronological order, last is the lowest ID. - @next_path = api_activitypub_outbox_url(max_id: @statuses.last.id) if @statuses.size == limit_param(DEFAULT_STATUSES_LIMIT) + set_first_last_page(@statuses) - unless @statuses.empty? - if @statuses.first.id == 1 - @prev_path = api_activitypub_outbox_url - elsif params[:max_id] - @prev_path = api_activitypub_outbox_url(since_id: @statuses.first.id) - end - end - - @paginated = @next_path || @prev_path - - set_pagination_headers(@next_path, @prev_path) + render :show end - private + def show_outbox_page + all_statuses = Status.as_outbox_timeline(@account) + @statuses = all_statuses.paginate_by_max_id(limit_param(DEFAULT_STATUSES_LIMIT), params[:max_id], params[:since_id]) + + all_statuses = cache_collection(all_statuses) + @statuses = cache_collection(@statuses) + + set_maps(@statuses) + + set_first_last_page(all_statuses) + + @next_page_url = api_activitypub_outbox_url(pagination_params(max_id: @statuses.last.id)) unless @statuses.empty? + @prev_page_url = api_activitypub_outbox_url(pagination_params(since_id: @statuses.first.id)) unless @statuses.empty? + + @paginated = @next_page_url || @prev_page_url + @part_of_url = api_activitypub_outbox_url + + set_pagination_headers(@next_page_url, @prev_page_url) + + render :show_page + end def cache_collection(raw) super(raw, Status) @@ -38,4 +55,15 @@ class Api::Activitypub::OutboxController < ApiController def set_account @account = Account.find(params[:id]) end + + def set_first_last_page(statuses) # rubocop:disable Style/AccessorMethodName + return if statuses.empty? + + @first_page_url = api_activitypub_outbox_url(max_id: statuses.first.id + 1) + @last_page_url = api_activitypub_outbox_url(since_id: statuses.last.id - 1) + end + + def pagination_params(core_params) + params.permit(:local, :limit).merge(core_params) + end end diff --git a/app/helpers/activitystreams2_builder_helper.rb b/app/helpers/activitystreams2_builder_helper.rb index eeada56f2..717b470f0 100644 --- a/app/helpers/activitystreams2_builder_helper.rb +++ b/app/helpers/activitystreams2_builder_helper.rb @@ -3,6 +3,6 @@ module Activitystreams2BuilderHelper # Gets a usable name for an account, using display name or username. def account_name(account) - account.display_name.empty? ? account.username : account.display_name + account.display_name.presence || account.username end end diff --git a/app/views/activitypub/types/collection.activitystreams2.rabl b/app/views/activitypub/types/collection.activitystreams2.rabl index 9e7e14e2b..d3f8ddcac 100644 --- a/app/views/activitypub/types/collection.activitystreams2.rabl +++ b/app/views/activitypub/types/collection.activitystreams2.rabl @@ -1,5 +1,3 @@ extends 'activitypub/intransient.activitystreams2.rabl' node(:type) { 'Collection' } -node(:items) { [] } -node(:totalItems) { 0 } diff --git a/app/views/activitypub/types/ordered_collection_page.activitystreams2.rabl b/app/views/activitypub/types/ordered_collection_page.activitystreams2.rabl index f498fe8e5..c821fa928 100644 --- a/app/views/activitypub/types/ordered_collection_page.activitystreams2.rabl +++ b/app/views/activitypub/types/ordered_collection_page.activitystreams2.rabl @@ -1,4 +1,3 @@ extends 'activitypub/types/ordered_collection.activitystreams2.rabl' node(:type) { 'OrderedCollectionPage' } -node(:current) { request.original_url } diff --git a/app/views/api/activitypub/outbox/show.activitystreams2.rabl b/app/views/api/activitypub/outbox/show.activitystreams2.rabl index a498f74bc..273b15e82 100644 --- a/app/views/api/activitypub/outbox/show.activitystreams2.rabl +++ b/app/views/api/activitypub/outbox/show.activitystreams2.rabl @@ -1,23 +1,12 @@ -if @paginated - extends 'activitypub/types/ordered_collection_page.activitystreams2.rabl' -else - extends 'activitypub/types/ordered_collection.activitystreams2.rabl' -end +extends 'activitypub/types/ordered_collection.activitystreams2.rabl' object @account -node(:items) do - @statuses.map { |status| api_activitypub_status_url(status) } -end - node(:totalItems) { @statuses.count } -node(:next) { @next_path } if @next_path -node(:prev) { @prev_path } if @prev_path +node(:current) { @first_page_url } if @first_page_url +node(:first) { @first_page_url } if @first_page_url +node(:last) { @last_page_url } if @last_page_url node(:name) { |account| t('activitypub.outbox.name', account_name: account_name(account)) } node(:summary) { |account| t('activitypub.outbox.summary', account_name: account_name(account)) } -node(:updated) do |account| - times = @statuses.map { |status| status.updated_at.to_time } - times << account.created_at.to_time - times.max.xmlschema -end +node(:updated) { |account| (@statuses.empty? ? account.created_at.to_time : @statuses.first.updated_at.to_time).xmlschema } diff --git a/app/views/api/activitypub/outbox/show_page.activitystreams2.rabl b/app/views/api/activitypub/outbox/show_page.activitystreams2.rabl new file mode 100644 index 000000000..b6433ccf2 --- /dev/null +++ b/app/views/api/activitypub/outbox/show_page.activitystreams2.rabl @@ -0,0 +1,16 @@ +extends 'activitypub/types/ordered_collection_page.activitystreams2.rabl' + +object @account + +node(:items) do + @statuses.map { |status| api_activitypub_status_url(status) } +end + +node(:next) { @next_page_url } if @next_page_url +node(:prev) { @prev_page_url } if @prev_page_url +node(:current) { @first_page_url } if @first_page_url +node(:first) { @first_page_url } if @first_page_url +node(:last) { @last_page_url } if @last_page_url +node(:partOf) { @part_of_url } if @part_of_url + +node(:updated) { |account| (@statuses.empty? ? account.created_at.to_time : @statuses.first.updated_at.to_time).xmlschema } diff --git a/config/application.rb b/config/application.rb index 7711b96a0..996fd5f84 100644 --- a/config/application.rb +++ b/config/application.rb @@ -64,7 +64,7 @@ module Mastodon config.middleware.insert_before 0, Rack::Cors do allow do origins '*' - + resource '/@:username', headers: :any, methods: [:get], credentials: false resource '/api/*', headers: :any, methods: [:post, :put, :delete, :get, :patch, :options], credentials: false, expose: ['Link', 'X-RateLimit-Reset', 'X-RateLimit-Limit', 'X-RateLimit-Remaining', 'X-Request-Id'] resource '/oauth/token', headers: :any, methods: [:post], credentials: false end diff --git a/config/locales/en.yml b/config/locales/en.yml index c226da3e4..6f54e343e 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -43,12 +43,12 @@ en: activitypub: activity: announce: - name: "%{account_name} announced an activity." + name: "%{account_name} shared an activity." create: name: "%{account_name} created a note." outbox: name: "%{account_name}'s Outbox" - summary: A collection of activities from user %{account_name}. + summary: "A collection of activities from user %{account_name}." admin: accounts: are_you_sure: Are you sure? diff --git a/spec/controllers/api/activitypub/activities_controller_spec.rb b/spec/controllers/api/activitypub/activities_controller_spec.rb index 2de530289..2c966a45a 100644 --- a/spec/controllers/api/activitypub/activities_controller_spec.rb +++ b/spec/controllers/api/activitypub/activities_controller_spec.rb @@ -10,7 +10,7 @@ RSpec.describe Api::Activitypub::ActivitiesController, type: :controller do public_status = nil before do - public_status = Status.create!(account: user.account, text: 'Hello world', visibility: :public) + public_status = Fabricate(:status, account: user.account, text: 'Hello world', visibility: :public) @request.env['HTTP_ACCEPT'] = 'application/activity+json' get :show_status, params: { id: public_status.id } @@ -24,10 +24,6 @@ RSpec.describe Api::Activitypub::ActivitiesController, type: :controller do expect(response.header['Content-Type']).to include 'application/activity+json' end - it 'sets Access-Control-Allow-Origin header to *' do - expect(response.header['Access-Control-Allow-Origin']).to eq '*' - end - it 'returns http success' do json_data = JSON.parse(response.body) expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') @@ -44,8 +40,8 @@ RSpec.describe Api::Activitypub::ActivitiesController, type: :controller do reblog = nil before do - original = Status.create!(account: user.account, text: 'Hello world', visibility: :public) - reblog = Status.create!(account: user.account, reblog_of_id: original.id, visibility: :public) + original = Fabricate(:status, account: user.account, text: 'Hello world', visibility: :public) + reblog = Fabricate(:status, account: user.account, reblog_of_id: original.id, visibility: :public) @request.env['HTTP_ACCEPT'] = 'application/activity+json' get :show_status, params: { id: reblog.id } @@ -59,10 +55,6 @@ RSpec.describe Api::Activitypub::ActivitiesController, type: :controller do expect(response.header['Content-Type']).to include 'application/activity+json' end - it 'sets Access-Control-Allow-Origin header to *' do - expect(response.header['Access-Control-Allow-Origin']).to eq '*' - end - it 'returns http success' do json_data = JSON.parse(response.body) expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') diff --git a/spec/controllers/api/activitypub/notes_controller_spec.rb b/spec/controllers/api/activitypub/notes_controller_spec.rb index 8b8db2fec..39ddec03e 100644 --- a/spec/controllers/api/activitypub/notes_controller_spec.rb +++ b/spec/controllers/api/activitypub/notes_controller_spec.rb @@ -11,7 +11,7 @@ RSpec.describe Api::Activitypub::NotesController, type: :controller do public_status = nil before do - public_status = Status.create!(account: user_alice.account, text: 'Hello world', visibility: :public) + public_status = Fabricate(:status, account: user_alice.account, text: 'Hello world', visibility: :public) @request.env['HTTP_ACCEPT'] = 'application/activity+json' get :show, params: { id: public_status.id } @@ -25,10 +25,6 @@ RSpec.describe Api::Activitypub::NotesController, type: :controller do expect(response.header['Content-Type']).to include 'application/activity+json' end - it 'sets Access-Control-Allow-Origin header to *' do - expect(response.header['Access-Control-Allow-Origin']).to eq '*' - end - it 'returns http success' do json_data = JSON.parse(response.body) expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') @@ -46,8 +42,8 @@ RSpec.describe Api::Activitypub::NotesController, type: :controller do reply = nil before do - original = Status.create!(account: user_alice.account, text: 'Hello world', visibility: :public) - reply = Status.create!(account: user_bob.account, text: 'Hello world', in_reply_to_id: original.id, visibility: :public) + original = Fabricate(:status, account: user_alice.account, text: 'Hello world', visibility: :public) + reply = Fabricate(:status, account: user_bob.account, text: 'Hello world', in_reply_to_id: original.id, visibility: :public) @request.env['HTTP_ACCEPT'] = 'application/activity+json' get :show, params: { id: reply.id } @@ -61,10 +57,6 @@ RSpec.describe Api::Activitypub::NotesController, type: :controller do expect(response.header['Content-Type']).to include 'application/activity+json' end - it 'sets Access-Control-Allow-Origin header to *' do - expect(response.header['Access-Control-Allow-Origin']).to eq '*' - end - it 'returns http success' do json_data = JSON.parse(response.body) expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') diff --git a/spec/controllers/api/activitypub/outbox_controller_spec.rb b/spec/controllers/api/activitypub/outbox_controller_spec.rb index 9a47dde6b..99797c582 100644 --- a/spec/controllers/api/activitypub/outbox_controller_spec.rb +++ b/spec/controllers/api/activitypub/outbox_controller_spec.rb @@ -7,17 +7,17 @@ RSpec.describe Api::Activitypub::OutboxController, type: :controller do describe 'GET #show' do before do - @request.env['HTTP_ACCEPT'] = 'application/activity+json' + @request.headers['ACCEPT'] = 'application/activity+json' end - describe 'small number of statuses' do + describe 'collection with small number of statuses' do public_status = nil before do - public_status = Status.create!(account: user.account, text: 'Hello world', visibility: :public) - Status.create!(account: user.account, text: 'Hello world', visibility: :private) - Status.create!(account: user.account, text: 'Hello world', visibility: :unlisted) - Status.create!(account: user.account, text: 'Hello world', visibility: :direct) + public_status = Fabricate(:status, account: user.account, text: 'Hello world', visibility: :public) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :private) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :unlisted) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :direct) get :show, params: { id: user.account.id } end @@ -30,8 +30,37 @@ RSpec.describe Api::Activitypub::OutboxController, type: :controller do expect(response.header['Content-Type']).to include 'application/activity+json' end - it 'sets Access-Control-Allow-Origin header to *' do - expect(response.header['Access-Control-Allow-Origin']).to eq '*' + it 'returns AS2 JSON body' do + json_data = JSON.parse(response.body) + expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') + expect(json_data).to include('id' => @request.url) + expect(json_data).to include('type' => 'OrderedCollection') + expect(json_data).to include('totalItems' => 1) + expect(json_data).to include('current') + expect(json_data).to include('first') + expect(json_data).to include('last') + end + end + + describe 'collection with large number of statuses' do + before do + 30.times do + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :public) + end + + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :private) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :unlisted) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :direct) + + get :show, params: { id: user.account.id } + end + + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'sets Content-Type header to AS2' do + expect(response.header['Content-Type']).to include 'application/activity+json' end it 'returns AS2 JSON body' do @@ -39,53 +68,88 @@ RSpec.describe Api::Activitypub::OutboxController, type: :controller do expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') expect(json_data).to include('id' => @request.url) expect(json_data).to include('type' => 'OrderedCollection') - expect(json_data).to include('totalItems' => 1) - expect(json_data).to include('items') - expect(json_data['items'].count).to eq(1) - expect(json_data['items']).to include(api_activitypub_status_url(public_status)) + expect(json_data).to include('totalItems' => 30) + expect(json_data).to include('current') + expect(json_data).to include('first') + expect(json_data).to include('last') end end - describe 'large number of statuses' do + describe 'page with small number of statuses' do + statuses = [] + before do - 30.times do - Status.create!(account: user.account, text: 'Hello world', visibility: :public) + 5.times do + statuses << Fabricate(:status, account: user.account, text: 'Hello world', visibility: :public) end - Status.create!(account: user.account, text: 'Hello world', visibility: :private) - Status.create!(account: user.account, text: 'Hello world', visibility: :unlisted) - Status.create!(account: user.account, text: 'Hello world', visibility: :direct) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :private) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :unlisted) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :direct) + + get :show, params: { id: user.account.id, max_id: statuses.last.id + 1 } end - describe 'first page' do - before do - get :show, params: { id: user.account.id } + it 'returns http success' do + expect(response).to have_http_status(:success) + end + + it 'sets Content-Type header to AS2' do + expect(response.header['Content-Type']).to include 'application/activity+json' + end + + it 'returns AS2 JSON body' do + json_data = JSON.parse(response.body) + expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') + expect(json_data).to include('id' => @request.url) + expect(json_data).to include('type' => 'OrderedCollectionPage') + expect(json_data).to include('partOf') + expect(json_data).to include('items') + expect(json_data['items'].length).to eq(5) + expect(json_data).to include('prev') + expect(json_data).to include('next') + expect(json_data).to include('current') + expect(json_data).to include('first') + expect(json_data).to include('last') + end + end + + describe 'page with large number of statuses' do + statuses = [] + + before do + 30.times do + statuses << Fabricate(:status, account: user.account, text: 'Hello world', visibility: :public) end - it 'returns http success' do - expect(response).to have_http_status(:success) - end + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :private) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :unlisted) + Fabricate(:status, account: user.account, text: 'Hello world', visibility: :direct) - it 'sets Content-Type header to AS2' do - expect(response.header['Content-Type']).to include 'application/activity+json' - end + get :show, params: { id: user.account.id, max_id: statuses.last.id + 1 } + end - it 'sets Access-Control-Allow-Origin header to *' do - expect(response.header['Access-Control-Allow-Origin']).to eq '*' - end + it 'returns http success' do + expect(response).to have_http_status(:success) + end - it 'returns AS2 JSON body' do - json_data = JSON.parse(response.body) - expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') - expect(json_data).to include('id' => @request.url) - expect(json_data).to include('type' => 'OrderedCollectionPage') - expect(json_data).to include('totalItems' => 20) - expect(json_data).to include('items') - expect(json_data['items'].count).to eq(20) - expect(json_data).to include('current' => @request.url) - expect(json_data).to include('next') - expect(json_data).to_not include('prev') - end + it 'sets Content-Type header to AS2' do + expect(response.header['Content-Type']).to include 'application/activity+json' + end + + it 'returns AS2 JSON body' do + json_data = JSON.parse(response.body) + expect(json_data).to include('@context' => 'https://www.w3.org/ns/activitystreams') + expect(json_data).to include('id' => @request.url) + expect(json_data).to include('type' => 'OrderedCollectionPage') + expect(json_data).to include('partOf') + expect(json_data).to include('items') + expect(json_data['items'].length).to eq(20) + expect(json_data).to include('prev') + expect(json_data).to include('next') + expect(json_data).to include('current') + expect(json_data).to include('first') + expect(json_data).to include('last') end end end