Fix embed dropdown menu item for unauthenticated users (#25964)

app/controllers/api/web/embeds_controller.rb

@@ -1,17 +1,20 @@
 # frozen_string_literal: true
 
 class Api::Web::EmbedsController < Api::Web::BaseController
-  before_action :require_user!
+  include Authorization
 
-  def create
+  before_action :set_status
-    status = StatusFinder.new(params[:url]).status
 
-    return not_found if status.hidden?
+  def show
+    return not_found if @status.hidden?
 
-    render json: status, serializer: OEmbedSerializer, width: 400
+    if @status.local?
-  rescue ActiveRecord::RecordNotFound
+      render json: @status, serializer: OEmbedSerializer, width: 400
-    oembed = FetchOEmbedService.new.call(params[:url])
+    else
+      return not_found unless user_signed_in?
 
+      url = ActivityPub::TagManager.instance.url_for(@status)
+      oembed = FetchOEmbedService.new.call(url)
       return not_found if oembed.nil?
 
       begin
@@ -22,4 +25,12 @@ class Api::Web::EmbedsController < Api::Web::BaseController
 
       render json: oembed
     end
+  end
+
+  def set_status
+    @status = Status.find(params[:id])
+    authorize @status, :show?
+  rescue Mastodon::NotPermittedError
+    not_found
+  end
 end

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 });
     }
 

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)),
       },
     }));

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 });
     }
 

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)),
       },
     }));

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') },
     }));
   };
 

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;

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

spec/controllers/api/web/embeds_controller_spec.rb

@@ -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

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