From 6d2301988fdc0118c5583f48ba6da4a3b8247ba4 Mon Sep 17 00:00:00 2001 From: Rakib Hasan Date: Wed, 1 Feb 2017 21:07:38 -0500 Subject: [PATCH 1/5] Fix for issue #462 Modified uploadCompose action to send media ids of attached media when sending a request. Modified create method in MediaController to check if when posting a video, there are no other media attached to the status by looking at the media ids sent from the uploadCompose action. --- app/assets/javascripts/components/actions/compose.jsx | 5 ++++- app/controllers/api/v1/media_controller.rb | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/components/actions/compose.jsx b/app/assets/javascripts/components/actions/compose.jsx index 03aae885e0..84fbc7fc51 100644 --- a/app/assets/javascripts/components/actions/compose.jsx +++ b/app/assets/javascripts/components/actions/compose.jsx @@ -119,7 +119,10 @@ export function uploadCompose(files) { let data = new FormData(); data.append('file', files[0]); - + data.append('media_ids', getState().getIn( + ['compose', 'media_attachments'] + ).map(item => item.get('id'))); + api(getState).post('/api/v1/media', data, { onUploadProgress: function (e) { dispatch(uploadComposeProgress(e.loaded, e.total)); diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index f8139ade77..582d04dafa 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -11,6 +11,10 @@ class Api::V1::MediaController < ApiController def create @media = MediaAttachment.create!(account: current_user.account, file: params[:file]) + if @media.video? and params[:media_ids] != "List []" + @media.destroy + render json: {error: 'Cannot attach a video to a toot that already contains images'}, status: 422 + end rescue Paperclip::Errors::NotIdentifiedByImageMagickError render json: { error: 'File type of uploaded media could not be verified' }, status: 422 rescue Paperclip::Error From 6f9ecd899e9e7cb335940465c23dd53acc37269c Mon Sep 17 00:00:00 2001 From: Rakib Hasan Date: Thu, 2 Feb 2017 23:10:17 -0500 Subject: [PATCH 2/5] revisted fix for #462 Moved validation to services/post_status_service.rb --- .../javascripts/components/actions/compose.jsx | 5 +---- app/controllers/api/v1/media_controller.rb | 4 ---- app/controllers/api/v1/statuses_controller.rb | 16 ++++++++++------ app/services/post_status_service.rb | 8 +++++++- 4 files changed, 18 insertions(+), 15 deletions(-) diff --git a/app/assets/javascripts/components/actions/compose.jsx b/app/assets/javascripts/components/actions/compose.jsx index 84fbc7fc51..03aae885e0 100644 --- a/app/assets/javascripts/components/actions/compose.jsx +++ b/app/assets/javascripts/components/actions/compose.jsx @@ -119,10 +119,7 @@ export function uploadCompose(files) { let data = new FormData(); data.append('file', files[0]); - data.append('media_ids', getState().getIn( - ['compose', 'media_attachments'] - ).map(item => item.get('id'))); - + api(getState).post('/api/v1/media', data, { onUploadProgress: function (e) { dispatch(uploadComposeProgress(e.loaded, e.total)); diff --git a/app/controllers/api/v1/media_controller.rb b/app/controllers/api/v1/media_controller.rb index 582d04dafa..f8139ade77 100644 --- a/app/controllers/api/v1/media_controller.rb +++ b/app/controllers/api/v1/media_controller.rb @@ -11,10 +11,6 @@ class Api::V1::MediaController < ApiController def create @media = MediaAttachment.create!(account: current_user.account, file: params[:file]) - if @media.video? and params[:media_ids] != "List []" - @media.destroy - render json: {error: 'Cannot attach a video to a toot that already contains images'}, status: 422 - end rescue Paperclip::Errors::NotIdentifiedByImageMagickError render json: { error: 'File type of uploaded media could not be verified' }, status: 422 rescue Paperclip::Error diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 69cbdce5d6..036383d1eb 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -62,12 +62,16 @@ class Api::V1::StatusesController < ApiController end def create - @status = PostStatusService.new.call(current_user.account, params[:status], params[:in_reply_to_id].blank? ? nil : Status.find(params[:in_reply_to_id]), media_ids: params[:media_ids], - sensitive: params[:sensitive], - spoiler_text: params[:spoiler_text], - visibility: params[:visibility], - application: doorkeeper_token.application) - + begin + @status = PostStatusService.new.call(current_user.account, params[:status], params[:in_reply_to_id].blank? ? nil : Status.find(params[:in_reply_to_id]), media_ids: params[:media_ids], + sensitive: params[:sensitive], + spoiler_text: params[:spoiler_text], + visibility: params[:visibility], + application: doorkeeper_token.application) + rescue Mastodon::NotPermitted => e + render json: {error: e.message}, status: 422 + return + end render action: :show end diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index 979941c84c..d701035475 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -35,8 +35,14 @@ class PostStatusService < BaseService def attach_media(status, media_ids) return if media_ids.nil? || !media_ids.is_a?(Enumerable) - media = MediaAttachment.where(status_id: nil).where(id: media_ids.take(4).map(&:to_i)) + if media.length > 1 + media.each do |m| + if m.video? + raise Mastodon::NotPermitted, 'Cannot attach a video to a toot that already contains images' + end + end + end media.update(status_id: status.id) end From 87a6bed9e947aaad62466f072d1a27cd0f782a7d Mon Sep 17 00:00:00 2001 From: Rakib Hasan Date: Sat, 4 Feb 2017 22:03:24 -0500 Subject: [PATCH 3/5] previous commit was creating the status regardless of mix of video and images in status, just wasn't rendering the show action. I moved the validation before the status creation --- app/services/post_status_service.rb | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index d701035475..7ead804302 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -13,6 +13,7 @@ class PostStatusService < BaseService # @option [Doorkeeper::Application] :application # @return [Status] def call(account, text, in_reply_to = nil, options = {}) + media = validate_media options[:media_ids] status = account.statuses.create!(text: text, thread: in_reply_to, sensitive: options[:sensitive], @@ -20,7 +21,7 @@ class PostStatusService < BaseService visibility: options[:visibility], application: options[:application]) - attach_media(status, options[:media_ids]) + attach_media(status, media) process_mentions_service.call(status) process_hashtags_service.call(status) @@ -33,7 +34,7 @@ class PostStatusService < BaseService private - def attach_media(status, media_ids) + def validate_media(media_ids) return if media_ids.nil? || !media_ids.is_a?(Enumerable) media = MediaAttachment.where(status_id: nil).where(id: media_ids.take(4).map(&:to_i)) if media.length > 1 @@ -43,6 +44,11 @@ class PostStatusService < BaseService end end end + return media + end + + def attach_media(status, media) + return if media.nil? media.update(status_id: status.id) end From 9433d03705d3aa86a059d82ffc549c699092912d Mon Sep 17 00:00:00 2001 From: Rakib Hasan Date: Fri, 17 Feb 2017 02:58:16 +0000 Subject: [PATCH 4/5] Removed try clause from create action in status controller Using catch statement in api_controller.rb to catch NotPermitted Exception, and render error message --- app/controllers/api/v1/statuses_controller.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 036383d1eb..2ffd4a0181 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -62,16 +62,11 @@ class Api::V1::StatusesController < ApiController end def create - begin @status = PostStatusService.new.call(current_user.account, params[:status], params[:in_reply_to_id].blank? ? nil : Status.find(params[:in_reply_to_id]), media_ids: params[:media_ids], sensitive: params[:sensitive], spoiler_text: params[:spoiler_text], visibility: params[:visibility], application: doorkeeper_token.application) - rescue Mastodon::NotPermitted => e - render json: {error: e.message}, status: 422 - return - end render action: :show end From 5f511324b65f94d800dbbd3850214955d7d9eb73 Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Sun, 26 Feb 2017 23:23:06 +0100 Subject: [PATCH 5/5] Add validation of media attachments, clean up mastodon-own exception classes --- app/controllers/api/v1/statuses_controller.rb | 10 +++++----- app/controllers/api_controller.rb | 4 ++-- .../authorize_follow_controller.rb | 2 +- app/lib/exceptions.rb | 3 ++- app/services/favourite_service.rb | 2 +- app/services/follow_service.rb | 2 +- app/services/post_status_service.rb | 19 +++++++++---------- app/services/reblog_service.rb | 2 +- 8 files changed, 22 insertions(+), 22 deletions(-) diff --git a/app/controllers/api/v1/statuses_controller.rb b/app/controllers/api/v1/statuses_controller.rb index 2ffd4a0181..552f1b1b39 100644 --- a/app/controllers/api/v1/statuses_controller.rb +++ b/app/controllers/api/v1/statuses_controller.rb @@ -62,11 +62,11 @@ class Api::V1::StatusesController < ApiController end def create - @status = PostStatusService.new.call(current_user.account, params[:status], params[:in_reply_to_id].blank? ? nil : Status.find(params[:in_reply_to_id]), media_ids: params[:media_ids], - sensitive: params[:sensitive], - spoiler_text: params[:spoiler_text], - visibility: params[:visibility], - application: doorkeeper_token.application) + @status = PostStatusService.new.call(current_user.account, params[:status], params[:in_reply_to_id].blank? ? nil : Status.find(params[:in_reply_to_id]), media_ids: params[:media_ids], + sensitive: params[:sensitive], + spoiler_text: params[:spoiler_text], + visibility: params[:visibility], + application: doorkeeper_token.application) render action: :show end diff --git a/app/controllers/api_controller.rb b/app/controllers/api_controller.rb index 5d2bd9a225..c2002cb796 100644 --- a/app/controllers/api_controller.rb +++ b/app/controllers/api_controller.rb @@ -10,7 +10,7 @@ class ApiController < ApplicationController before_action :set_rate_limit_headers - rescue_from ActiveRecord::RecordInvalid do |e| + rescue_from ActiveRecord::RecordInvalid, Mastodon::ValidationError do |e| render json: { error: e.to_s }, status: 422 end @@ -30,7 +30,7 @@ class ApiController < ApplicationController render json: { error: 'Remote SSL certificate could not be verified' }, status: 503 end - rescue_from Mastodon::NotPermitted do + rescue_from Mastodon::NotPermittedError do render json: { error: 'This action is not allowed' }, status: 403 end diff --git a/app/controllers/authorize_follow_controller.rb b/app/controllers/authorize_follow_controller.rb index e866b55993..c98a5f45f5 100644 --- a/app/controllers/authorize_follow_controller.rb +++ b/app/controllers/authorize_follow_controller.rb @@ -25,7 +25,7 @@ class AuthorizeFollowController < ApplicationController else redirect_to web_url("accounts/#{@account.id}") end - rescue ActiveRecord::RecordNotFound, Mastodon::NotPermitted + rescue ActiveRecord::RecordNotFound, Mastodon::NotPermittedError render :error end diff --git a/app/lib/exceptions.rb b/app/lib/exceptions.rb index 359228c297..200da9fe16 100644 --- a/app/lib/exceptions.rb +++ b/app/lib/exceptions.rb @@ -2,5 +2,6 @@ module Mastodon class Error < StandardError; end - class NotPermitted < Error; end + class NotPermittedError < Error; end + class ValidationError < Error; end end diff --git a/app/services/favourite_service.rb b/app/services/favourite_service.rb index 8188983028..5cc96403cd 100644 --- a/app/services/favourite_service.rb +++ b/app/services/favourite_service.rb @@ -6,7 +6,7 @@ class FavouriteService < BaseService # @param [Status] status # @return [Favourite] def call(account, status) - raise Mastodon::NotPermitted unless status.permitted?(account) + raise Mastodon::NotPermittedError unless status.permitted?(account) favourite = Favourite.create!(account: account, status: status) diff --git a/app/services/follow_service.rb b/app/services/follow_service.rb index 915f95b4c4..17b3b25423 100644 --- a/app/services/follow_service.rb +++ b/app/services/follow_service.rb @@ -10,7 +10,7 @@ class FollowService < BaseService target_account = FollowRemoteAccountService.new.call(uri) raise ActiveRecord::RecordNotFound if target_account.nil? || target_account.id == source_account.id || target_account.suspended? - raise Mastodon::NotPermitted if target_account.blocking?(source_account) || source_account.blocking?(target_account) + raise Mastodon::NotPermittedError if target_account.blocking?(source_account) || source_account.blocking?(target_account) if target_account.locked? request_follow(source_account, target_account) diff --git a/app/services/post_status_service.rb b/app/services/post_status_service.rb index 7ead804302..b8179f7dcc 100644 --- a/app/services/post_status_service.rb +++ b/app/services/post_status_service.rb @@ -13,7 +13,7 @@ class PostStatusService < BaseService # @option [Doorkeeper::Application] :application # @return [Status] def call(account, text, in_reply_to = nil, options = {}) - media = validate_media options[:media_ids] + media = validate_media!(options[:media_ids]) status = account.statuses.create!(text: text, thread: in_reply_to, sensitive: options[:sensitive], @@ -34,17 +34,16 @@ class PostStatusService < BaseService private - def validate_media(media_ids) + def validate_media!(media_ids) return if media_ids.nil? || !media_ids.is_a?(Enumerable) + + raise Mastodon::ValidationError, 'Cannot attach more than 4 files' if media_ids.size > 4 + media = MediaAttachment.where(status_id: nil).where(id: media_ids.take(4).map(&:to_i)) - if media.length > 1 - media.each do |m| - if m.video? - raise Mastodon::NotPermitted, 'Cannot attach a video to a toot that already contains images' - end - end - end - return media + + raise Mastodon::ValidationError, 'Cannot attach a video to a toot that already contains images' if media.size > 1 && media.find(&:video?) + + media end def attach_media(status, media) diff --git a/app/services/reblog_service.rb b/app/services/reblog_service.rb index 7a52f041fc..c14b2925ac 100644 --- a/app/services/reblog_service.rb +++ b/app/services/reblog_service.rb @@ -10,7 +10,7 @@ class ReblogService < BaseService def call(account, reblogged_status) reblogged_status = reblogged_status.reblog if reblogged_status.reblog? - raise Mastodon::NotPermitted if reblogged_status.private_visibility? || !reblogged_status.permitted?(account) + raise Mastodon::NotPermittedError if reblogged_status.private_visibility? || !reblogged_status.permitted?(account) reblog = account.statuses.create!(reblog: reblogged_status, text: '')