From 41f65edb213dc34e723e5bcc7d3ea920752941ef Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 13 Jul 2023 15:53:03 +0200 Subject: [PATCH] Fix embed dropdown menu item for unauthenticated users (#25964) --- app/controllers/api/web/embeds_controller.rb | 37 ++-- .../mastodon/components/status_action_bar.jsx | 2 +- .../mastodon/containers/status_container.jsx | 2 +- .../features/status/components/action_bar.jsx | 2 +- .../containers/detailed_status_container.js | 2 +- .../mastodon/features/status/index.jsx | 2 +- .../features/ui/components/embed_modal.jsx | 6 +- config/routes/api.rb | 2 +- .../api/web/embeds_controller_spec.rb | 54 ------ spec/requests/api/web/embeds_spec.rb | 161 ++++++++++++++++++ 10 files changed, 194 insertions(+), 76 deletions(-) delete mode 100644 spec/controllers/api/web/embeds_controller_spec.rb create mode 100644 spec/requests/api/web/embeds_spec.rb diff --git a/app/controllers/api/web/embeds_controller.rb b/app/controllers/api/web/embeds_controller.rb index 58f6345e6..63c3f2d90 100644 --- a/app/controllers/api/web/embeds_controller.rb +++ b/app/controllers/api/web/embeds_controller.rb @@ -1,25 +1,36 @@ # frozen_string_literal: true class Api::Web::EmbedsController < Api::Web::BaseController - before_action :require_user! + include Authorization - def create - status = StatusFinder.new(params[:url]).status + before_action :set_status - return not_found if status.hidden? + def show + return not_found if @status.hidden? - render json: status, serializer: OEmbedSerializer, width: 400 - rescue ActiveRecord::RecordNotFound - oembed = FetchOEmbedService.new.call(params[:url]) + if @status.local? + render json: @status, serializer: OEmbedSerializer, width: 400 + else + return not_found unless user_signed_in? - return not_found if oembed.nil? + url = ActivityPub::TagManager.instance.url_for(@status) + oembed = FetchOEmbedService.new.call(url) + return not_found if oembed.nil? - begin - oembed[:html] = Sanitize.fragment(oembed[:html], Sanitize::Config::MASTODON_OEMBED) - rescue ArgumentError - return not_found + begin + oembed[:html] = Sanitize.fragment(oembed[:html], Sanitize::Config::MASTODON_OEMBED) + rescue ArgumentError + return not_found + end + + render json: oembed end + end - render json: oembed + def set_status + @status = Status.find(params[:id]) + authorize @status, :show? + rescue Mastodon::NotPermittedError + not_found end end diff --git a/app/javascript/mastodon/components/status_action_bar.jsx b/app/javascript/mastodon/components/status_action_bar.jsx index ab9dac27e..b713c98c6 100644 --- a/app/javascript/mastodon/components/status_action_bar.jsx +++ b/app/javascript/mastodon/components/status_action_bar.jsx @@ -258,7 +258,7 @@ class StatusActionBar extends ImmutablePureComponent { menu.push({ text: intl.formatMessage(messages.share), action: this.handleShareClick }); } - if (publicStatus) { + if (publicStatus && (signedIn || !isRemote)) { menu.push({ text: intl.formatMessage(messages.embed), action: this.handleEmbed }); } diff --git a/app/javascript/mastodon/containers/status_container.jsx b/app/javascript/mastodon/containers/status_container.jsx index 6167b404f..8b3d8b46b 100644 --- a/app/javascript/mastodon/containers/status_container.jsx +++ b/app/javascript/mastodon/containers/status_container.jsx @@ -139,7 +139,7 @@ const mapDispatchToProps = (dispatch, { intl, contextType }) => ({ dispatch(openModal({ modalType: 'EMBED', modalProps: { - url: status.get('url'), + id: status.get('id'), onError: error => dispatch(showAlertForError(error)), }, })); diff --git a/app/javascript/mastodon/features/status/components/action_bar.jsx b/app/javascript/mastodon/features/status/components/action_bar.jsx index d3295913c..0bacf2965 100644 --- a/app/javascript/mastodon/features/status/components/action_bar.jsx +++ b/app/javascript/mastodon/features/status/components/action_bar.jsx @@ -205,7 +205,7 @@ class ActionBar extends PureComponent { menu.push({ text: intl.formatMessage(messages.share), action: this.handleShare }); } - if (publicStatus) { + if (publicStatus && (signedIn || !isRemote)) { menu.push({ text: intl.formatMessage(messages.embed), action: this.handleEmbed }); } diff --git a/app/javascript/mastodon/features/status/containers/detailed_status_container.js b/app/javascript/mastodon/features/status/containers/detailed_status_container.js index e76790a9f..57d5f61c8 100644 --- a/app/javascript/mastodon/features/status/containers/detailed_status_container.js +++ b/app/javascript/mastodon/features/status/containers/detailed_status_container.js @@ -110,7 +110,7 @@ const mapDispatchToProps = (dispatch, { intl }) => ({ dispatch(openModal({ modalType: 'EMBED', modalProps: { - url: status.get('url'), + id: status.get('id'), onError: error => dispatch(showAlertForError(error)), }, })); diff --git a/app/javascript/mastodon/features/status/index.jsx b/app/javascript/mastodon/features/status/index.jsx index e5faba083..e5f91071e 100644 --- a/app/javascript/mastodon/features/status/index.jsx +++ b/app/javascript/mastodon/features/status/index.jsx @@ -449,7 +449,7 @@ class Status extends ImmutablePureComponent { handleEmbed = (status) => { this.props.dispatch(openModal({ modalType: 'EMBED', - modalProps: { url: status.get('url') }, + modalProps: { id: status.get('id') }, })); }; diff --git a/app/javascript/mastodon/features/ui/components/embed_modal.jsx b/app/javascript/mastodon/features/ui/components/embed_modal.jsx index b08d53ab1..bacbad347 100644 --- a/app/javascript/mastodon/features/ui/components/embed_modal.jsx +++ b/app/javascript/mastodon/features/ui/components/embed_modal.jsx @@ -14,7 +14,7 @@ const messages = defineMessages({ class EmbedModal extends ImmutablePureComponent { static propTypes = { - url: PropTypes.string.isRequired, + id: PropTypes.string.isRequired, onClose: PropTypes.func.isRequired, onError: PropTypes.func.isRequired, intl: PropTypes.object.isRequired, @@ -26,11 +26,11 @@ class EmbedModal extends ImmutablePureComponent { }; componentDidMount () { - const { url } = this.props; + const { id } = this.props; this.setState({ loading: true }); - api().post('/api/web/embed', { url }).then(res => { + api().get(`/api/web/embeds/${id}`).then(res => { this.setState({ loading: false, oembed: res.data }); const iframeDocument = this.iframe.contentWindow.document; diff --git a/config/routes/api.rb b/config/routes/api.rb index a10e8058a..2fc84ace2 100644 --- a/config/routes/api.rb +++ b/config/routes/api.rb @@ -296,7 +296,7 @@ namespace :api, format: false do namespace :web do resource :settings, only: [:update] - resource :embed, only: [:create] + resources :embeds, only: [:show] resources :push_subscriptions, only: [:create] do member do put :update diff --git a/spec/controllers/api/web/embeds_controller_spec.rb b/spec/controllers/api/web/embeds_controller_spec.rb deleted file mode 100644 index 8c4e1a8f2..000000000 --- a/spec/controllers/api/web/embeds_controller_spec.rb +++ /dev/null @@ -1,54 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe Api::Web::EmbedsController do - render_views - - let(:user) { Fabricate(:user) } - - before { sign_in user } - - describe 'POST #create' do - subject(:body) { JSON.parse(response.body, symbolize_names: true) } - - let(:response) { post :create, params: { url: url } } - - context 'when successfully finds status' do - let(:status) { Fabricate(:status) } - let(:url) { "http://#{Rails.configuration.x.web_domain}/@#{status.account.username}/#{status.id}" } - - it 'returns a right response' do - expect(response).to have_http_status 200 - expect(body[:author_name]).to eq status.account.username - end - end - - context 'when fails to find status' do - let(:url) { 'https://host.test/oembed.html' } - let(:service_instance) { instance_double(FetchOEmbedService) } - - before do - allow(FetchOEmbedService).to receive(:new) { service_instance } - allow(service_instance).to receive(:call) { call_result } - end - - context 'when successfully fetching oembed' do - let(:call_result) { { result: :ok } } - - it 'returns a right response' do - expect(response).to have_http_status 200 - expect(body[:result]).to eq 'ok' - end - end - - context 'when fails to fetch oembed' do - let(:call_result) { nil } - - it 'returns a right response' do - expect(response).to have_http_status 404 - end - end - end - end -end diff --git a/spec/requests/api/web/embeds_spec.rb b/spec/requests/api/web/embeds_spec.rb new file mode 100644 index 000000000..6314f43aa --- /dev/null +++ b/spec/requests/api/web/embeds_spec.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe '/api/web/embed' do + subject { get "/api/web/embeds/#{id}", headers: headers } + + context 'when accessed anonymously' do + let(:headers) { {} } + + context 'when the requested status is local' do + let(:id) { status.id } + + context 'when the requested status is public' do + let(:status) { Fabricate(:status, visibility: :public) } + + it 'returns JSON with an html attribute' do + subject + + expect(response).to have_http_status(200) + expect(body_as_json[:html]).to be_present + end + end + + context 'when the requested status is private' do + let(:status) { Fabricate(:status, visibility: :private) } + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + end + + context 'when the requested status is remote' do + let(:remote_account) { Fabricate(:account, domain: 'example.com') } + let(:status) { Fabricate(:status, visibility: :public, account: remote_account, url: 'https://example.com/statuses/1') } + let(:id) { status.id } + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + + context 'when the requested status does not exist' do + let(:id) { -1 } + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + end + + context 'with an API token' do + let(:user) { Fabricate(:user) } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: 'read') } + let(:headers) { { 'Authorization' => "Bearer #{token.token}" } } + + context 'when the requested status is local' do + let(:id) { status.id } + + context 'when the requested status is public' do + let(:status) { Fabricate(:status, visibility: :public) } + + it 'returns JSON with an html attribute' do + subject + + expect(response).to have_http_status(200) + expect(body_as_json[:html]).to be_present + end + + context 'when the requesting user is blocked' do + before do + status.account.block!(user.account) + end + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + end + + context 'when the requested status is private' do + let(:status) { Fabricate(:status, visibility: :private) } + + before do + user.account.follow!(status.account) + end + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + end + + context 'when the requested status is remote' do + let(:remote_account) { Fabricate(:account, domain: 'example.com') } + let(:status) { Fabricate(:status, visibility: :public, account: remote_account, url: 'https://example.com/statuses/1') } + let(:id) { status.id } + + let(:service_instance) { instance_double(FetchOEmbedService) } + + before do + allow(FetchOEmbedService).to receive(:new) { service_instance } + allow(service_instance).to receive(:call) { call_result } + end + + context 'when the requesting user is blocked' do + before do + status.account.block!(user.account) + end + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + + context 'when successfully fetching OEmbed' do + let(:call_result) { { html: 'ok' } } + + it 'returns JSON with an html attribute' do + subject + + expect(response).to have_http_status(200) + expect(body_as_json[:html]).to be_present + end + end + + context 'when failing to fetch OEmbed' do + let(:call_result) { nil } + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + end + + context 'when the requested status does not exist' do + let(:id) { -1 } + + it 'returns http not found' do + subject + + expect(response).to have_http_status(404) + end + end + end +end