Defect #11870

REST API allows delete Admin user, making Redmine unusable

Added by Enrique Castilla Contreras almost 8 years ago. Updated 6 days ago.

Status:NewStart date:
Priority:NormalDue date:
Assignee:-% Done:

0%

Category:REST API
Target version:4.2.0
Resolution: Affected version:

Description

On Users collection, an administrator may delete its own account on Redmine, making it unusable.

Doing exploratory testing tasks with API REST I've accidentally deleted my own user on http://ecastillac.m.redmine.org, making the server unusable for me.

I've tried this script, provided Admin user had id=2 as shown in a previous execution:

#!/usr/bin/perl -w

use strict;
use warnings;

our ($VERSION) = '0.01'; # q$Revision$ =~ /(\d+)/;

use Test::More;
use Data::Dump;

# --------------------------------------------------

use Redmine::API;

my($API_Key, $BASE_URL) = do 'config';

my $api = Redmine::API->new( auth_key => $API_Key
                           , base_url => $BASE_URL
                           , trace => $ARGV[0] || 0);

my($res, $res1);

# -------------------------------------------------

#$res = $api->users->x->all();
#ddx $res->body;

$res1 = $api->users->user->del( 2 );
ddx $res1->body;

fix-11870.patch Magnifier (1.46 KB) Mizuki ISHIKAWA, 2020-06-25 08:49

fix-11870-v2.patch Magnifier (3.54 KB) Mizuki ISHIKAWA, 2020-07-02 06:05

History

#1 Updated by Etienne Massip almost 8 years ago

I don't get why it would be a defect? Just don't delete your admin account if you have only one...

#2 Updated by Go MAEDA 25 days ago

The web UI does not allow admins to delete your account unconditionally.

  • "Administration > Users" page always disallows users to delete their own account
  • Users can delete their own account on My account page when the setting "Allow users to delete their own account" is enabled, but admins are allowed to delete their own account on the page only when there is another user with an admin privilege

So, I think API should not allow avoiding the limitations. The following code disallows admins to delete their own accounts via API or by sending a crafted request if "Allow users to delete their own account" is not enabled.

diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 2fb297874..578ac5a9a 100644
--- a/app/controllers/users_controller.rb
+++ b/app/controllers/users_controller.rb
@@ -184,6 +184,8 @@ class UsersController < ApplicationController
   end

   def destroy
+    raise Unauthorized if @user == User.current && !@user.own_account_deletable?
+
     @user.destroy
     respond_to do |format|
       format.html { redirect_back_or_default(users_path) }

#3 Updated by Mizuki ISHIKAWA 13 days ago

I've attached a patch based on #11870#note-2.
It was developed by pair programming with @kfischer_okarin.

#4 Updated by vzvu 3k6k 12 days ago

LGTM.

If I have to say something, it would be more user-friendly if the reason of the error is returned (such as "Can't delete your own account") with render_api_errors or something, but anyway it looks good to me.

#5 Updated by Go MAEDA 10 days ago

  • Target version set to 4.2.0

Setting the target version to 4.2.0.

#6 Updated by Mizuki ISHIKAWA 6 days ago

vzvu 3k6k wrote:

LGTM.

If I have to say something, it would be more user-friendly if the reason of the error is returned (such as "Can't delete your own account") with render_api_errors or something, but anyway it looks good to me.

Thank you for your feedback.
I changed to return error message.

@user.own_account_deletable? will be false if User.current was the last admin user. If there is an opinion that it is better to display another error message only in that case, I will improve the patch

Also available in: Atom PDF