From 721a07e073e0958874f4dcfa41cad99bcae051d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nicol=C3=A1s=20Tamargo?= Date: Fri, 25 Oct 2024 15:36:05 +0200 Subject: [PATCH] MBS-13770: Allow admins to auto-approve and auto-reject any edit There's plenty of cases where a removal of a spam release for example sits open for days, and closing it earlier requires asking for more eyes on it which does seem like the opposite of what you want to do with spam. This allows account admins to use the same accept/reject mechanism we already used in test servers to immediately accept or reject any open edit (including their own). When doing so, a vote is added to the edit (of the newly added types "Admin approval" or "Admin reject", added in a previous commit so they can be visible in production), so it is as clear as possible why the edit applied or failed. I kept the mechanisms separate, still keeping a /test/ endpoint for the existing testing mechanism while adding a new /admin/ one for the new feature. This is mostly for testing purposes (it's very hard to test for the feature otherwise since the test setup has the buttons on by default because of its DB_STAGING_TESTING_FEATURES setting) but also to avoid setting the new votes every time we use the feature in testing, since that might not be desirable. --- lib/MusicBrainz/Server/Controller/Admin.pm | 65 +++++- lib/MusicBrainz/Server/Data/Vote.pm | 13 +- root/edit/EditIndex.js | 62 ++++-- root/edit/components/EditSummary.js | 26 ++- root/user/PrivilegedUsers.js | 3 +- .../Controller/Admin/AcceptRejectEdits.pm | 198 ++++++++++++++++++ .../Server/Controller/Edit/Show.pm | 107 +++++++++- .../Controller/UnconfirmedEmailAddresses.pm | 2 + t/sql/vote.sql | 4 +- 9 files changed, 451 insertions(+), 29 deletions(-) create mode 100644 t/lib/t/MusicBrainz/Server/Controller/Admin/AcceptRejectEdits.pm diff --git a/lib/MusicBrainz/Server/Controller/Admin.pm b/lib/MusicBrainz/Server/Controller/Admin.pm index 087e78e0247..0bcbd4ea141 100644 --- a/lib/MusicBrainz/Server/Controller/Admin.pm +++ b/lib/MusicBrainz/Server/Controller/Admin.pm @@ -7,7 +7,13 @@ use Try::Tiny; extends 'MusicBrainz::Server::Controller'; -use MusicBrainz::Server::Constants qw( :privileges ); +use DBDefs; + +use MusicBrainz::Server::Constants qw( + :privileges + $VOTE_ADMIN_APPROVE + $VOTE_ADMIN_REJECT +); use MusicBrainz::Server::ControllerUtils::JSON qw( serialize_pager ); use MusicBrainz::Server::Data::Utils qw( boolean_to_json ); @@ -340,6 +346,63 @@ sub unlock_username : Path('/admin/locked-usernames/unlock') Args(1) RequireAuth ); } +sub accept_edit : Path('/admin/accept-edit') Args(1) RequireAuth(account_admin) +{ + my ($self, $c, $edit_id) = @_; + + my $edit = $c->model('Edit')->get_by_id($edit_id) + or $c->detach('/error_404'); + + _accept_edit($c, $edit) if $edit->is_open; + $c->response->redirect($c->uri_for_action('/edit/show', [ $edit->id ])); +} + +sub reject_edit : Path('/admin/reject-edit') Args(1) RequireAuth(account_admin) +{ + my ($self, $c, $edit_id) = @_; + + my $edit = $c->model('Edit')->get_by_id($edit_id) + or $c->detach('/error_404'); + + _reject_edit($c, $edit) if $edit->is_open; + $c->response->redirect($c->uri_for_action('/edit/show', [ $edit->id ])); +} + +sub _accept_edit +{ + my ($c, $edit) = @_; + + my $sql = $c->model('MB')->context->sql; + + Sql::run_in_transaction( sub { + $c->model('Vote')->enter_votes( + $c->user, + [{ + vote => $VOTE_ADMIN_APPROVE, + edit_id => $edit->id, + }], + ); + $c->model('Edit')->accept($edit); + }, $sql ); +} + +sub _reject_edit +{ + my ($c, $edit) = @_; + + my $sql = $c->model('MB')->context->sql; + Sql::run_in_transaction( sub { + $c->model('Vote')->enter_votes( + $c->user, + [{ + vote => $VOTE_ADMIN_REJECT, + edit_id => $edit->id, + }], + ); + $c->model('Edit')->reject($edit); + }, $sql ); +} + 1; =head1 COPYRIGHT AND LICENSE diff --git a/lib/MusicBrainz/Server/Data/Vote.pm b/lib/MusicBrainz/Server/Data/Vote.pm index b600f81ee90..0018f73bc28 100644 --- a/lib/MusicBrainz/Server/Data/Vote.pm +++ b/lib/MusicBrainz/Server/Data/Vote.pm @@ -73,9 +73,20 @@ sub enter_votes # not sufficient to filter the vote because the actual approval is happening elsewhere confess 'Unauthorized editor ' . $editor->id . ' tried to approve edit #' . $_->{edit_id}; } + if (any { $_->{vote} == $VOTE_ADMIN_APPROVE && !$editor->is_account_admin } @votes) { + # not sufficient to filter the vote because the actual approval is happening elsewhere + confess 'Unauthorized editor ' . $editor->id . ' tried to admin-approve edit #' . $_->{edit_id}; + } + if (any { $_->{vote} == $VOTE_ADMIN_REJECT && !$editor->is_account_admin } @votes) { + # not sufficient to filter the vote because the actual rejection is happening elsewhere + confess 'Unauthorized editor ' . $editor->id . ' tried to admin-reject edit #' . $_->{edit_id}; + } unless ($opts{override_privs}) { @votes = grep { - $_->{vote} == $VOTE_APPROVE || $edits->{ $_->{edit_id} }->editor_may_vote_on_edit($editor) + $_->{vote} == $VOTE_APPROVE || + $_->{vote} == $VOTE_ADMIN_APPROVE || + $_->{vote} == $VOTE_ADMIN_REJECT || + $edits->{ $_->{edit_id} }->editor_may_vote_on_edit($editor) } @votes; } diff --git a/root/edit/EditIndex.js b/root/edit/EditIndex.js index 9d3cc25d787..a911b8655ae 100644 --- a/root/edit/EditIndex.js +++ b/root/edit/EditIndex.js @@ -17,6 +17,8 @@ import EditorLink from '../static/scripts/common/components/EditorLink.js'; import {DB_STAGING_TESTING_FEATURES} from '../static/scripts/common/DBDefs.mjs'; import linkedEntities from '../static/scripts/common/linkedEntities.mjs'; +import {isAccountAdmin} + from '../static/scripts/common/utility/privileges.js'; import FormSubmit from '../static/scripts/edit/components/FormSubmit.js'; import getVoteName from '../static/scripts/edit/utility/getVoteName.js'; import {editorMayAddNote, editorMayVoteOnEdit} @@ -36,10 +38,12 @@ component EditIndex( fullWidth: boolean = false, ) { const $c = React.useContext(CatalystContext); + const isAdmin = isAccountAdmin($c.user); const canAddNote = Boolean($c.user && editorMayAddNote(edit, $c.user)); const isOwnEdit = Boolean($c.user && $c.user.id === edit.editor_id); const canVoteHere = Boolean($c.user && editorMayVoteOnEdit(edit, $c.user)); const detailsElement = getEditDetailsElement(edit); + const showAcceptReject = DB_STAGING_TESTING_FEATURES || isAdmin; return ( @@ -123,26 +127,44 @@ component EditIndex( ) : null} {$c.user ? ( - edit.is_open && DB_STAGING_TESTING_FEATURES ? ( - <> -

{l('Testing features')}

-

- {l(`To aid in testing, the following features - have been made available on testing servers:`)} -

- - + edit.is_open && showAcceptReject ? ( + isAdmin ? ( + <> +

{l_admin('Admin features')}

+ + + ) : ( + <> +

{l('Testing features')}

+

+ {l(`To aid in testing, the following features + have been made available on testing servers:`)} +

+ + + ) ) : null ) : (

diff --git a/root/edit/components/EditSummary.js b/root/edit/components/EditSummary.js index 585659feafa..9e38a2bb65e 100644 --- a/root/edit/components/EditSummary.js +++ b/root/edit/components/EditSummary.js @@ -18,6 +18,8 @@ import { DB_READ_ONLY, DB_STAGING_TESTING_FEATURES, } from '../../static/scripts/common/DBDefs.mjs'; +import {isAccountAdmin} + from '../../static/scripts/common/utility/privileges.js'; import { editorMayAddNote, editorMayApprove, @@ -34,9 +36,11 @@ component EditSummary( ) { const $c = React.useContext(CatalystContext); const user = $c.user; + const isAdmin = isAccountAdmin(user); const mayAddNote = editorMayAddNote(edit, user); const mayApprove = editorMayApprove(edit, user); const mayCancel = editorMayCancel(edit, user); + const showAcceptReject = DB_STAGING_TESTING_FEATURES || isAdmin; return ( <> @@ -77,8 +81,23 @@ component EditSummary( ) : null} - {edit.status === EDIT_STATUS_OPEN && - DB_STAGING_TESTING_FEATURES ? ( + {edit.status === EDIT_STATUS_OPEN && showAcceptReject ? ( + isAdmin ? ( + <> + + {l('Accept edit')} + + + {l('Reject edit')} + + + ) : ( <> - ) : null} + ) + ) : null} ) : null} ); diff --git a/root/user/PrivilegedUsers.js b/root/user/PrivilegedUsers.js index 5ea46a9526d..1abef3b91bd 100644 --- a/root/user/PrivilegedUsers.js +++ b/root/user/PrivilegedUsers.js @@ -110,7 +110,8 @@ component PrivilegedUsers(

{lp('Account administrators', 'header')}

- {l('Account administrators can edit and delete user accounts.')} + {l(`Account administrators can edit and delete user accounts, + and immediately accept and reject edits.`)}

{texp.l( diff --git a/t/lib/t/MusicBrainz/Server/Controller/Admin/AcceptRejectEdits.pm b/t/lib/t/MusicBrainz/Server/Controller/Admin/AcceptRejectEdits.pm new file mode 100644 index 00000000000..50c8dc805aa --- /dev/null +++ b/t/lib/t/MusicBrainz/Server/Controller/Admin/AcceptRejectEdits.pm @@ -0,0 +1,198 @@ +package t::MusicBrainz::Server::Controller::Admin::AcceptRejectEdits; +use strict; +use warnings; + +use HTTP::Status qw( :constants ); +use Test::Routine; +use Test::More; +use MusicBrainz::Server::Constants qw( + :edit_status + :vote + $EDIT_ARTIST_EDIT + $UNTRUSTED_FLAG +); + +with 't::Mechanize', 't::Context'; + +=head2 Test description + +This test checks whether admins can accept and reject any edits, and ensures +non-admins are not able to do so. + +=cut + +test 'Try to accept/reject edit as a regular user' => sub { + my $test = shift; + my $mech = $test->mech; + my $edit = prepare($test); + + $mech->get_ok('/login'); + $mech->submit_form( with_fields => { + username => 'editor1', + password => 'pass', + } ); + + $mech->get('/'); + $mech->content_contains('editor1', 'Non-admin user "editor1" is logged in'); + + $mech->get( + '/admin/accept-edit/' . $edit->id, + q(Try to access the accept edit endpoint as a non-admin), + ); + is($mech->status, HTTP_FORBIDDEN, + 'The attempt to access the endpoint was rejected'); + + $edit = $test->c->model('Edit')->get_by_id($edit->id); + $test->c->model('Vote')->load_for_edits($edit); + is($edit->status, $STATUS_OPEN, 'The edit is still open'); + is(scalar @{ $edit->votes }, 0, 'The vote count is still 0'); + + $mech->get( + '/admin/reject-edit/' . $edit->id, + q(Try to access the reject edit endpoint as a non-admin), + ); + is($mech->status, HTTP_FORBIDDEN, + 'The attempt to access the endpoint was rejected'); + + $edit = $test->c->model('Edit')->get_by_id($edit->id); + $test->c->model('Vote')->load_for_edits($edit); + is($edit->status, $STATUS_OPEN, 'The edit is still open'); + is(scalar @{ $edit->votes }, 0, 'The vote count is still 0'); +}; + +test 'Try to accept an edit as an admin' => sub { + my $test = shift; + my $mech = $test->mech; + my $edit = prepare($test); + + $mech->get_ok('/login'); + $mech->submit_form( with_fields => { + username => 'editor7', + password => 'pass', + } ); + + $mech->get('/'); + $mech->content_contains('editor7', 'Admin user "editor7" is logged in'); + + $mech->get_ok( + '/admin/accept-edit/' . $edit->id, + q(Admin can access the accept edit endpoint for other's edits), + ); + + $edit = $test->c->model('Edit')->get_by_id($edit->id); + is($edit->status, $STATUS_APPLIED, 'applied'); + $test->c->model('Vote')->load_for_edits($edit); + is($edit->votes->[0]->vote, $VOTE_ADMIN_APPROVE, 'Vote is Admin approval'); + is($edit->votes->[0]->editor_id, 7, 'Vote is by editor7'); +}; + +test 'Try to reject an edit as an admin' => sub { + my $test = shift; + my $mech = $test->mech; + my $edit = prepare($test); + + $mech->get_ok('/login'); + $mech->submit_form( with_fields => { + username => 'editor7', + password => 'pass', + } ); + + $mech->get('/'); + $mech->content_contains('editor7', 'Admin user "editor7" is logged in'); + + $mech->get_ok( + '/admin/reject-edit/' . $edit->id, + q(Admin can access the reject edit endpoint for other's edits), + ); + + $edit = $test->c->model('Edit')->get_by_id($edit->id); + is($edit->status, $STATUS_FAILEDVOTE, 'Rejected with Failed vote'); + $test->c->model('Vote')->load_for_edits($edit); + is($edit->votes->[0]->vote, $VOTE_ADMIN_REJECT, 'Vote is Admin rejection'); + is($edit->votes->[0]->editor_id, 7, 'Vote is by editor7'); +}; + +test 'Try to accept own edit as an admin' => sub { + my $test = shift; + my $mech = $test->mech; + my $edit = prepare($test, 7); + + $mech->get_ok('/login'); + $mech->submit_form( with_fields => { + username => 'editor7', + password => 'pass', + } ); + + $mech->get('/'); + $mech->content_contains('editor7', 'Admin user "editor7" is logged in'); + + $mech->get_ok( + '/admin/accept-edit/' . $edit->id, + q(Admin can access the accept edit endpoint for own edits), + ); + + $edit = $test->c->model('Edit')->get_by_id($edit->id); + is($edit->status, $STATUS_APPLIED, 'applied'); + $test->c->model('Vote')->load_for_edits($edit); + is($edit->votes->[0]->vote, $VOTE_ADMIN_APPROVE, 'Vote is Admin approval'); + is($edit->votes->[0]->editor_id, 7, 'Vote is by editor7'); +}; + +test 'Try to reject own edit as an admin' => sub { + my $test = shift; + my $mech = $test->mech; + my $edit = prepare($test, 7); + + $mech->get_ok('/login'); + $mech->submit_form( with_fields => { + username => 'editor7', + password => 'pass', + } ); + + $mech->get('/'); + $mech->content_contains('editor7', 'Admin user "editor7" is logged in'); + + $mech->get_ok( + '/admin/reject-edit/' . $edit->id, + q(Admin can access the reject edit endpoint for own edits), + ); + + $edit = $test->c->model('Edit')->get_by_id($edit->id); + is($edit->status, $STATUS_FAILEDVOTE, 'Rejected with Failed vote'); + $test->c->model('Vote')->load_for_edits($edit); + is($edit->votes->[0]->vote, $VOTE_ADMIN_REJECT, 'Vote is Admin rejection'); + is($edit->votes->[0]->editor_id, 7, 'Vote is by editor7'); +}; + +sub prepare { + my ($test, $editor_id) = @_; + + local *DBDefs::DB_STAGING_TESTING_FEATURES = sub { 0 }; + my $c = $test->c; + + MusicBrainz::Server::Test->prepare_test_database($test->c, '+vote'); + + my $edit = $test->c->model('Edit')->create( + editor_id => $editor_id // 1, + edit_type => $EDIT_ARTIST_EDIT, + to_edit => $c->model('Artist')->get_by_id(1), + comment => 'Changed comment', + ipi_codes => [], + isni_codes => [], + privileges => $UNTRUSTED_FLAG, + ); + + return $edit; +} + +1; + +=head1 COPYRIGHT AND LICENSE + +Copyright (C) 2024 MetaBrainz Foundation + +This file is part of MusicBrainz, the open internet music database, +and is licensed under the GPL version 2, or (at your option) any +later version: http://www.gnu.org/licenses/gpl-2.0.txt + +=cut diff --git a/t/lib/t/MusicBrainz/Server/Controller/Edit/Show.pm b/t/lib/t/MusicBrainz/Server/Controller/Edit/Show.pm index 7fa5d25d949..d34a495fdc0 100644 --- a/t/lib/t/MusicBrainz/Server/Controller/Edit/Show.pm +++ b/t/lib/t/MusicBrainz/Server/Controller/Edit/Show.pm @@ -52,6 +52,14 @@ test 'Check edit page displays basic components' => sub { 'Edit by ', 'The edit page lists the editor who entered the edit', ); + $mech->content_lacks( + '/admin/accept-edit/' . $edit->id, + 'The edit page lacks the button for the admin to accept the edit', + ); + $mech->content_lacks( + '/admin/reject-edit/' . $edit->id, + 'The edit page lacks the button for the admin to reject the edit', + ); }; test 'Check edit page differences for own edit' => sub { @@ -79,6 +87,14 @@ test 'Check edit page differences for own edit' => sub { 'You are not currently able to vote on this edit', 'The message about not being able to vote is not present', ); + $mech->content_lacks( + '/admin/accept-edit/' . $edit->id, + 'The edit page lacks the button for the admin to accept the edit', + ); + $mech->content_lacks( + '/admin/reject-edit/' . $edit->id, + 'The edit page lacks the button for the admin to reject the edit', + ); }; test 'Check edit page differences for editor without confirmed email' => sub { @@ -106,6 +122,14 @@ test 'Check edit page differences for editor without confirmed email' => sub { 'You are not currently able to add notes to this edit', 'The message about not being able to add notes is present', ); + $mech->content_lacks( + '/admin/accept-edit/' . $edit->id, + 'The edit page lacks the button for the admin to accept the edit', + ); + $mech->content_lacks( + '/admin/reject-edit/' . $edit->id, + 'The edit page lacks the button for the admin to reject the edit', + ); }; test 'Check edit page differences for beginner editor' => sub { @@ -133,6 +157,14 @@ test 'Check edit page differences for beginner editor' => sub { 'You are not currently able to vote on this edit', 'The message about not being able to vote is present', ); + $mech->content_lacks( + '/admin/accept-edit/' . $edit->id, + 'The edit page lacks the button for the admin to accept the edit', + ); + $mech->content_lacks( + '/admin/reject-edit/' . $edit->id, + 'The edit page lacks the button for the admin to reject the edit', + ); }; test 'Check edit page differences when logged out' => sub { @@ -166,16 +198,87 @@ test 'Check edit page differences when logged out' => sub { 'Raw edit data for this edit', 'The raw data link is hidden', ); + $mech->content_lacks( + '/admin/accept-edit/' . $edit->id, + 'The edit page lacks the button for the admin to accept the edit', + ); + $mech->content_lacks( + '/admin/reject-edit/' . $edit->id, + 'The edit page lacks the button for the admin to reject the edit', + ); }; -sub prepare { +test 'Check edit page differences for account admin' => sub { my $test = shift; + my $mech = $test->mech; + my $edit = prepare($test); + + $mech->get_ok('/login'); + $mech->submit_form( with_fields => { + username => 'editor7', + password => 'pass', + } ); + + $mech->get_ok( + '/edit/' . $edit->id, + 'Fetched edit page as an account admin other than the edit author', + ); + html_ok($mech->content); + + $mech->content_contains( + 'Submit vote and note', + 'The edit page contains the button to vote and add an edit note', + ); + $mech->content_contains( + '/admin/accept-edit/' . $edit->id, + 'The edit page contains the button for the admin to accept the edit', + ); + $mech->content_contains( + '/admin/reject-edit/' . $edit->id, + 'The edit page contains the button for the admin to reject the edit', + ); +}; + +test 'Check edit page differences for account admin on own edit' => sub { + my $test = shift; + my $mech = $test->mech; + my $edit = prepare($test, 7); + + $mech->get_ok('/login'); + $mech->submit_form( with_fields => { + username => 'editor7', + password => 'pass', + } ); + + $mech->get_ok( + '/edit/' . $edit->id, + 'Fetched edit page as an account admin who is the edit author', + ); + html_ok($mech->content); + + $mech->content_contains( + 'Submit note', + 'The edit page contains the button to add an edit note', + ); + $mech->content_contains( + 'Accept edit', + 'The edit page contains the button for the admin to accept the edit', + ); + $mech->content_contains( + 'Reject edit', + 'The edit page contains the button for the admin to reject the edit', + ); +}; + +sub prepare { + my ($test, $editor_id) = @_; + my $c = $test->c; MusicBrainz::Server::Test->prepare_test_database($test->c, '+vote'); my $edit = $test->c->model('Edit')->create( - editor_id => 1, + editor_id => $editor_id // 1, edit_type => $EDIT_ARTIST_EDIT, to_edit => $c->model('Artist')->get_by_id(1), comment => 'Changed comment', diff --git a/t/lib/t/MusicBrainz/Server/Controller/UnconfirmedEmailAddresses.pm b/t/lib/t/MusicBrainz/Server/Controller/UnconfirmedEmailAddresses.pm index 760a7c8109b..500a2dda7f4 100644 --- a/t/lib/t/MusicBrainz/Server/Controller/UnconfirmedEmailAddresses.pm +++ b/t/lib/t/MusicBrainz/Server/Controller/UnconfirmedEmailAddresses.pm @@ -84,6 +84,7 @@ test 'Paths that allow browsing without a confirmed email address' => sub { 'Controller::Admin::StatisticsEvent::index', 'Controller::Admin::WikiDoc::history', 'Controller::Admin::WikiDoc::index', + 'Controller::Admin::accept_edit', 'Controller::Admin::delete_user', 'Controller::Admin::edit_banner', 'Controller::Admin::edit_user', @@ -91,6 +92,7 @@ test 'Paths that allow browsing without a confirmed email address' => sub { 'Controller::Admin::ip_lookup', 'Controller::Admin::locked_username_search', 'Controller::Admin::privilege_search', + 'Controller::Admin::reject_edit', 'Controller::Admin::unlock_username', 'Controller::Ajax::filter_artist_events_form', 'Controller::Ajax::filter_artist_recordings_form', diff --git a/t/sql/vote.sql b/t/sql/vote.sql index d5ec7f34e2f..029a6aada24 100644 --- a/t/sql/vote.sql +++ b/t/sql/vote.sql @@ -11,4 +11,6 @@ INSERT INTO editor (id, name, password, email, ha1, email_confirm_date, member_s -- Non-verified editor (5, 'editor5', '{CLEARTEXT}pass', null, '01de7bc91330d78a6d0a84033e293f15', null, '2014-12-03', 0), -- Beginner editor - (6, 'editor6', '{CLEARTEXT}pass', 'editor6@example.com', '01de7bc91330d78a6d0a84033e293f11', now(), '2014-12-03', 8192); + (6, 'editor6', '{CLEARTEXT}pass', 'editor6@example.com', '01de7bc91330d78a6d0a84033e293f11', now(), '2014-12-03', 8192), + -- Account admin + (7, 'editor7', '{CLEARTEXT}pass', 'editor7@example.com', '01de7bc91330d78a6d0a84033e293f12', now(), '2014-12-03', 128);