From 894956e20cfa7ea36bb124fb3561fde5694ac955 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 28 Mar 2022 23:57:38 +0200 Subject: [PATCH] Fix /api/v1/admin/accounts (#17887) * Fix /api/v1/admin/accounts Compatibility was broken since #17009 which changed the underlying filter class without changing the controller. This commits restore support for the old parameters. * Add /api/v2/admin/accounts with the new parameters * Add tests * Add missing filter for `silenced` status Co-authored-by: Eugen Rochko Co-authored-by: Eugen Rochko --- .../api/v1/admin/accounts_controller.rb | 16 +++- .../api/v2/admin/accounts_controller.rb | 31 ++++++++ app/models/account_filter.rb | 4 + config/routes.rb | 4 + .../api/v1/admin/accounts_controller_spec.rb | 35 ++++++++- .../api/v2/admin/accounts_controller_spec.rb | 73 +++++++++++++++++++ 6 files changed, 159 insertions(+), 4 deletions(-) create mode 100644 app/controllers/api/v2/admin/accounts_controller.rb create mode 100644 spec/controllers/api/v2/admin/accounts_controller_spec.rb diff --git a/app/controllers/api/v1/admin/accounts_controller.rb b/app/controllers/api/v1/admin/accounts_controller.rb index 65330b8c8..4b6dab208 100644 --- a/app/controllers/api/v1/admin/accounts_controller.rb +++ b/app/controllers/api/v1/admin/accounts_controller.rb @@ -104,13 +104,27 @@ class Api::V1::Admin::AccountsController < Api::BaseController end def filtered_accounts - AccountFilter.new(filter_params).results + AccountFilter.new(translated_filter_params).results end def filter_params params.permit(*FILTER_PARAMS) end + def translated_filter_params + translated_params = { origin: 'local', status: 'active' }.merge(filter_params.slice(*AccountFilter::KEYS)) + + translated_params[:origin] = 'remote' if params[:remote].present? + + %i(active pending disabled silenced suspended).each do |status| + translated_params[:status] = status.to_s if params[status].present? + end + + translated_params[:permissions] = 'staff' if params[:staff].present? + + translated_params + end + def insert_pagination_headers set_pagination_headers(next_path, prev_path) end diff --git a/app/controllers/api/v2/admin/accounts_controller.rb b/app/controllers/api/v2/admin/accounts_controller.rb new file mode 100644 index 000000000..a89e6835e --- /dev/null +++ b/app/controllers/api/v2/admin/accounts_controller.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +class Api::V2::Admin::AccountsController < Api::V1::Admin::AccountsController + FILTER_PARAMS = %i( + origin + status + permissions + username + by_domain + display_name + email + ip + invited_by + ).freeze + + PAGINATION_PARAMS = (%i(limit) + FILTER_PARAMS).freeze + + private + + def filtered_accounts + AccountFilter.new(filter_params).results + end + + def filter_params + params.permit(*FILTER_PARAMS) + end + + def pagination_params(core_params) + params.slice(*PAGINATION_PARAMS).permit(*PAGINATION_PARAMS).merge(core_params) + end +end diff --git a/app/models/account_filter.rb b/app/models/account_filter.rb index 9da1522dd..ec309ce09 100644 --- a/app/models/account_filter.rb +++ b/app/models/account_filter.rb @@ -80,6 +80,10 @@ class AccountFilter accounts_with_users.merge(User.pending) when 'suspended' Account.suspended + when 'disabled' + accounts_with_users.merge(User.disabled) + when 'silenced' + Account.silenced else raise "Unknown status: #{value}" end diff --git a/config/routes.rb b/config/routes.rb index c108e2ec4..7c9a13dc4 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -576,6 +576,10 @@ Rails.application.routes.draw do resources :media, only: [:create] get '/search', to: 'search#index', as: :search resources :suggestions, only: [:index] + + namespace :admin do + resources :accounts, only: [:index] + end end namespace :web do diff --git a/spec/controllers/api/v1/admin/accounts_controller_spec.rb b/spec/controllers/api/v1/admin/accounts_controller_spec.rb index 3f61bbc0b..b69595f7e 100644 --- a/spec/controllers/api/v1/admin/accounts_controller_spec.rb +++ b/spec/controllers/api/v1/admin/accounts_controller_spec.rb @@ -30,15 +30,44 @@ RSpec.describe Api::V1::Admin::AccountsController, type: :controller do end describe 'GET #index' do + let!(:remote_account) { Fabricate(:account, domain: 'example.org') } + let!(:other_remote_account) { Fabricate(:account, domain: 'foo.bar') } + let!(:suspended_account) { Fabricate(:account, suspended: true) } + let!(:suspended_remote) { Fabricate(:account, domain: 'foo.bar', suspended: true) } + let!(:disabled_account) { Fabricate(:user, disabled: true).account } + let!(:pending_account) { Fabricate(:user, approved: false).account } + let!(:admin_account) { user.account } + + let(:params) { {} } + before do - get :index + pending_account.user.update(approved: false) + get :index, params: params end it_behaves_like 'forbidden for wrong scope', 'write:statuses' it_behaves_like 'forbidden for wrong role', 'user' - it 'returns http success' do - expect(response).to have_http_status(200) + [ + [{ active: 'true', local: 'true', staff: 'true' }, [:admin_account]], + [{ by_domain: 'example.org', remote: 'true' }, [:remote_account]], + [{ suspended: 'true' }, [:suspended_account]], + [{ disabled: 'true' }, [:disabled_account]], + [{ pending: 'true' }, [:pending_account]], + ].each do |params, expected_results| + context "when called with #{params.inspect}" do + let(:params) { params } + + it 'returns http success' do + expect(response).to have_http_status(200) + end + + it "returns the correct accounts (#{expected_results.inspect})" do + json = body_as_json + + expect(json.map { |a| a[:id].to_i }).to eq (expected_results.map { |symbol| send(symbol).id }) + end + end end end diff --git a/spec/controllers/api/v2/admin/accounts_controller_spec.rb b/spec/controllers/api/v2/admin/accounts_controller_spec.rb new file mode 100644 index 000000000..3212ddb84 --- /dev/null +++ b/spec/controllers/api/v2/admin/accounts_controller_spec.rb @@ -0,0 +1,73 @@ +require 'rails_helper' + +RSpec.describe Api::V2::Admin::AccountsController, type: :controller do + render_views + + let(:role) { 'moderator' } + let(:user) { Fabricate(:user, role: role) } + let(:scopes) { 'admin:read admin:write' } + let(:token) { Fabricate(:accessible_access_token, resource_owner_id: user.id, scopes: scopes) } + let(:account) { Fabricate(:account) } + + before do + allow(controller).to receive(:doorkeeper_token) { token } + end + + shared_examples 'forbidden for wrong scope' do |wrong_scope| + let(:scopes) { wrong_scope } + + it 'returns http forbidden' do + expect(response).to have_http_status(403) + end + end + + shared_examples 'forbidden for wrong role' do |wrong_role| + let(:role) { wrong_role } + + it 'returns http forbidden' do + expect(response).to have_http_status(403) + end + end + + describe 'GET #index' do + let!(:remote_account) { Fabricate(:account, domain: 'example.org') } + let!(:other_remote_account) { Fabricate(:account, domain: 'foo.bar') } + let!(:suspended_account) { Fabricate(:account, suspended: true) } + let!(:suspended_remote) { Fabricate(:account, domain: 'foo.bar', suspended: true) } + let!(:disabled_account) { Fabricate(:user, disabled: true).account } + let!(:pending_account) { Fabricate(:user, approved: false).account } + let!(:admin_account) { user.account } + + let(:params) { {} } + + before do + pending_account.user.update(approved: false) + get :index, params: params + end + + it_behaves_like 'forbidden for wrong scope', 'write:statuses' + it_behaves_like 'forbidden for wrong role', 'user' + + [ + [{ status: 'active', origin: 'local', permissions: 'staff' }, [:admin_account]], + [{ by_domain: 'example.org', origin: 'remote' }, [:remote_account]], + [{ status: 'suspended' }, [:suspended_remote, :suspended_account]], + [{ status: 'disabled' }, [:disabled_account]], + [{ status: 'pending' }, [:pending_account]], + ].each do |params, expected_results| + context "when called with #{params.inspect}" do + let(:params) { params } + + it 'returns http success' do + expect(response).to have_http_status(200) + end + + it "returns the correct accounts (#{expected_results.inspect})" do + json = body_as_json + + expect(json.map { |a| a[:id].to_i }).to eq (expected_results.map { |symbol| send(symbol).id }) + end + end + end + end +end