Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Admin-level API for actioning Statuses #41

Merged
merged 3 commits into from
Sep 15, 2023
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 49 additions & 0 deletions app/controllers/api/v1/admin/status_actions_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
# frozen_string_literal: true

class Api::V1::Admin::StatusActionsController < Api::BaseController
# modeled on api/v1/admin/account_actions_controller.rb
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left 3 of these modeled on comments in so that it's clear where some of the new code is descended/remixed from. Happy to remove them before merging.


include Authorization

# only support a subset of StatusBatchAction types
ALLOWED_TYPES = %w(
delete
sensitive
).freeze

before_action -> { authorize_if_got_token! :'admin:write', :'admin:write:statuses' }
before_action :set_status

after_action :verify_authorized

def create
authorize [:admin, @status], :update?
raise ActiveRecord::RecordInvalid unless valid_type?

status_batch_action = Admin::StatusBatchAction.new(resource_params)
status_batch_action.status_ids = [@status.id]
status_batch_action.current_account = current_account
status_batch_action.save!

render_empty
end

private

def valid_type?
params[:type] && ALLOWED_TYPES.include?(params[:type])
end

def set_status
@status = Status.find(params[:status_id])
end

def resource_params
params.permit(
:type,
:report_id,
:text,
:send_email_notification
)
end
end
51 changes: 51 additions & 0 deletions app/controllers/api/v1/admin/statuses_controller.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
# frozen_string_literal: true

class Api::V1::Admin::StatusesController < Api::BaseController
# modeled on api/v1/admin/accounts_controller.rb

include Authorization
include AccountableConcern

before_action -> { authorize_if_got_token! :'admin:read', :'admin:read:statuses' }, only: [:show]
before_action -> { authorize_if_got_token! :'admin:write', :'admin:write:statuses' }, except: [:show]
before_action :set_status

after_action :verify_authorized

def show
authorize [:admin, @status], :show?
render json: @status, serializer: REST::StatusSerializer
end

def destroy
# modeled on handle_delete from status_batch_action.rb
authorize [:admin, @status], :destroy?
ApplicationRecord.transaction do
@status.discard_with_reblogs
log_action :destroy, @status
Tombstone.find_or_create_by(uri: @status.uri, account: @status.account)
aaga marked this conversation as resolved.
Show resolved Hide resolved
end
json = render_to_body json: @status, serializer: REST::StatusSerializer, source_requested: true

RemovalWorker.perform_async(@status.id, { 'preserve' => @status.account.local?, 'immediate' => !@status.account.local? })

render json: json
end

def unsensitive
# modeled on undo_mark_statuses_as_sensitive from approve_appeal_service.rb
authorize [:admin, @status], :update?
representative_account = Account.representative
ApplicationRecord.transaction do
UpdateStatusService.new.call(@status, representative_account.id, sensitive: false) if @status.with_media?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm curious about including if @status.with_media?. I see that's how they are doing it in the approve_appeal_service but I don't really get why. Doesn't this mean that UpdateStatusService won't get called if the status doesn't have media?

Copy link
Author

@aaga aaga Sep 15, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's a bit weird but it does mean that UpdateStatusService won't get called if the status doesn't have media. I left it like this to match the equivalent action – mark_as_sensitive – in status_batch_action.rb, which similarly doesn't call UpdateStatusService with sensitive: true unless there is media.

Since this method is intended to undo moderation decisions, I figured it made sense to mirror the behavior.

Zooming out, I guess the intention is that moderators should only be able to mark visual media as sensitive since it can be jarring/triggering/affective in a user's feed. I'll ask the T&S team if they want to change that policy to allow marking text-only posts as sensitive. Then, we can change both the action and undo action together.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perfect, that sounds excellent!

log_action :unsensitive, @status
end
render json: @status, serializer: REST::StatusSerializer
end

private

def set_status
@status = Status.find(params[:id])
end
end
2 changes: 1 addition & 1 deletion app/models/admin/status_batch_action.rb
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ def process_action!
case type
when 'delete'
handle_delete!
when 'mark_as_sensitive'
when 'mark_as_sensitive', 'sensitive'
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing non-API controller uses mark_as_sensitive as the action "type". I wanted the new API to be consistent with the nomenclature of the equivalent Accounts API (which just uses sensitive), so I decided to add another "type" that maps to the same action.

handle_mark_as_sensitive!
when 'report'
handle_report!
Expand Down
2 changes: 1 addition & 1 deletion app/models/status.rb
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def non_sensitive_with_media?
end

def reported?
@reported ||= Report.where(target_account: account).unresolved.where('? = ANY(status_ids)', id).exists?
@reported ||= Report.where(target_account: account).where('? = ANY(status_ids)', id).exists?
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted the definition of status.reported? to be true even if the Report against the Status has already been resolved.

The Admin Status Policy lets the Admin API "show (GET)" Statuses that would otherwise not be visible to them, if the status is reported.

Since we auto-resolve reports in the bridge, we need reported? to persist as true even after the report is resolved.

I don't think this changes the intention too much, so I think upstream will be ok with this too. If not, it's easy to remove for the PR we do there.

end

def emojis
Expand Down
2 changes: 2 additions & 0 deletions config/initializers/doorkeeper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@
:'admin:read',
:'admin:read:accounts',
:'admin:read:reports',
:'admin:read:statuses',
:'admin:read:domain_allows',
:'admin:read:domain_blocks',
:'admin:read:ip_blocks',
Expand All @@ -106,6 +107,7 @@
:'admin:write',
:'admin:write:accounts',
:'admin:write:reports',
:'admin:write:statuses',
:'admin:write:domain_allows',
:'admin:write:domain_blocks',
:'admin:write:ip_blocks',
Expand Down
8 changes: 8 additions & 0 deletions config/routes/api.rb
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,14 @@
resource :action, only: [:create], controller: 'account_actions'
end

resources :statuses, only: [:show, :destroy] do
member do
post :unsensitive
end

resource :action, only: [:create], controller: 'status_actions'
end

resources :reports, only: [:index, :update, :show] do
member do
post :assign_to_self
Expand Down
74 changes: 74 additions & 0 deletions spec/controllers/api/v1/admin/statuses_controller_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe Api::V1::Admin::StatusesController do
render_views

let(:role) { UserRole.find_by(name: 'Moderator') }
let(:user) { Fabricate(:user, role: role) }
let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) }

before do
allow(controller).to receive(:doorkeeper_token) { token }
end

describe 'GET #show' do
let(:scopes) { 'admin:read:statuses' }
let(:status) { Fabricate(:status) }

before do
get :show, params: { id: status.id }
end

it_behaves_like 'forbidden for wrong scope', 'read:statuses' # non-admin scope
it_behaves_like 'forbidden for wrong role', ''

it 'returns http success' do
expect(response).to have_http_status(200)
end
end

describe 'DELETE #destroy' do
let(:scopes) { 'admin:write:statuses' }
let(:status) { Fabricate(:status) }

before do
post :destroy, params: { id: status.id }
end

it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses'
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope
it_behaves_like 'forbidden for wrong role', ''

it 'returns http success' do
expect(response).to have_http_status(200)
end

it 'removes the status' do
expect(Status.find_by(id: status.id)).to be_nil
end
end

describe 'POST #unsensitive' do
let(:scopes) { 'admin:write:statuses' }
let(:media) { Fabricate(:media_attachment) }
let(:status) { Fabricate(:status, media_attachments: [media], sensitive: true) }

before do
post :unsensitive, params: { id: status.id }
end

it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses'
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope
it_behaves_like 'forbidden for wrong role', ''

it 'returns http success' do
expect(response).to have_http_status(200)
end

it 'unmarks status as sensitive' do
expect(status.reload.sensitive?).to be false
end
end
end
122 changes: 122 additions & 0 deletions spec/requests/api/v1/admin/status_actions_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe 'Status actions' do
let(:role) { UserRole.find_by(name: 'Moderator') }
let(:user) { Fabricate(:user, role: role) }
let(:scopes) { 'admin:write admin:write:statuses' }
let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) }
let(:headers) { { 'Authorization' => "Bearer #{token.token}" } }
let(:mailer) { instance_double(ActionMailer::MessageDelivery, deliver_later!: nil) }

before do
allow(UserMailer).to receive(:warning).with(status.account.user, anything).and_return(mailer)
end

shared_examples 'a successful notification delivery' do
it 'notifies the user about the action taken' do
subject

expect(UserMailer).to have_received(:warning).with(status.account.user, anything).once
expect(mailer).to have_received(:deliver_later!).once
end
end

shared_examples 'a successful logged action' do |action_type, target_type|
it 'logs action' do
subject

log_item = Admin::ActionLog.where(action: action_type).last

expect(log_item).to be_present
expect(log_item.account_id).to eq(user.account_id)
expect(log_item.target_id).to eq(target_type == :status ? status.id : report.id)
end
end

describe 'POST /api/v1/admin/statuses/:id/action' do
subject do
post "/api/v1/admin/statuses/#{status.id}/action", headers: headers, params: params
end

let(:account) { Fabricate(:account, domain: nil) } # local account for email notification
let(:media) { Fabricate(:media_attachment) }
let(:status) { Fabricate(:status, media_attachments: [media], account: account) }

context 'with type of delete' do
let(:params) { { type: 'delete' } }

it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses'
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope
it_behaves_like 'forbidden for wrong role', ''
it_behaves_like 'a successful logged action', :destroy, :status

it 'returns http success' do
subject

expect(response).to have_http_status(200)
end

it 'deletes the status' do
expect { subject }.to change { Status.find_by(id: status.id) }.from(status).to(nil)
end
end

context 'with type of sensitive' do
let(:params) { { type: 'sensitive' } }

it_behaves_like 'forbidden for wrong scope', 'admin:read:statuses'
it_behaves_like 'forbidden for wrong scope', 'write:statuses' # non-admin scope
it_behaves_like 'forbidden for wrong role', ''
it_behaves_like 'a successful logged action', :update, :status

it 'returns http success' do
subject

expect(response).to have_http_status(200)
end

it 'marks the status as sensitive' do
expect { subject }.to change { status.reload.sensitive? }.from(false).to(true)
end
end

context 'with no type' do
let(:params) { {} }

it 'returns http unprocessable entity' do
subject

expect(response).to have_http_status(422)
end
end

context 'with invalid type' do
let(:params) { { type: 'invalid' } }

it 'returns http unprocessable entity' do
subject

expect(response).to have_http_status(422)
end
end

context 'with notification delivery' do
let(:params) { { type: 'delete', send_email_notification: true } }

it_behaves_like 'a successful notification delivery'
end

context 'with report' do
let(:report) { Fabricate(:report) }
let(:params) { { type: 'delete', report_id: report.id } }

it_behaves_like 'a successful logged action', :resolve, :report

it 'resolves report' do
expect { subject }.to change { report.reload.unresolved? }.from(true).to(false)
end
end
end
end