Defect #11870

REST API allows delete Admin user, making Redmine unusable

Added by Enrique Castilla Contreras about 8 years ago. Updated 4 months 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

fix-11870-v3.patch Magnifier (1.54 KB) vzvu 3k6k, 2020-07-14 17:57

History

#1 Updated by Etienne Massip about 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 5 months 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 4 months 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 4 months 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 4 months ago

  • Target version set to 4.2.0

Setting the target version to 4.2.0.

#6 Updated by Mizuki ISHIKAWA 4 months 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

#7 Updated by Go MAEDA 4 months ago

Mizuki ISHIKAWA wrote:

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.

I agree that returning an informative error message is user-friendly, however, I think the behavior is not consistent with other API responses. I prefer the first patch.

#8 Updated by Mizuki ISHIKAWA 4 months ago

Go MAEDA wrote:

I agree that returning an informative error message is user-friendly, however, I think the behavior is not consistent with other API responses. I prefer the first patch.

Thanks for reviewing this patch!

Although there is no precedent that directly uses render_api_errors to return error messages, it is common to return validation error messages with render_validation_errors.
I don't think the behavior of the API from the user's perspective is that special.

And, in the case of exceptions that meet special conditions like this time, it is better to issue an error message even if it is different from other API responses.


I wrote it as above, but if the current specification makes it difficult to commit, I think it is better to prioritize the commit of the first patch!

#9 Updated by Kevin Fischer 4 months ago

I think for now it might be good to follow Redmine's current practice, i.e. no special error message.

But still it would be good to create another ticket (if one doesn't exist yet) to discuss/improve the general Rest API error response format of Redmine and adapt it to best practices for Rest APIs

#10 Updated by vzvu 3k6k 4 months ago

Go MAEDA wrote:

I agree that returning an informative error message is user-friendly, however, I think the behavior is not consistent with other API responses. I prefer the first patch.

Thank you for pointing it out.

Certainly most of (or all of?) Redmine APIs don't return error messages when deletion fails. I think it is because in most cases users can easily guess why their request failed. On the flip side, it might be worth returning a error message when it is difficult for users to guess why.

Maybe this should be discussed on another issue as Kevin Fischer says.


Mizuki ISHIKAWA wrote:

@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

I'm sorry for the late reply. Good catch, and thank you for writing another patch. I didn't think of it until you said.

As you suggest, I feel the message of "This user is your own user and cannot be deleted" can be confusing when the error reason is that User.current was the last admin user.

I've attached another patch to try to solve this confusion by adding another error message, though I'm not sure this API should return an error message for now.

Also available in: Atom PDF