https://www.redmine.org/https://www.redmine.org/favicon.ico?16793021292012-09-19T07:45:29ZRedmineRedmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=410182012-09-19T07:45:29ZEtienne Massip
<ul></ul><p>I don't get why it would be a defect? Just don't delete your admin account if you have only one...</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=981762020-06-13T01:08:44ZGo MAEDA
<ul></ul><p>The web UI does not allow admins to delete your account unconditionally.</p>
<ul>
<li>"Administration > Users" page always disallows users to delete their own account</li>
<li>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</li>
</ul>
<p>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.</p>
<pre><code class="diff syntaxhl"><span class="gh">diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb
index 2fb297874..578ac5a9a 100644
</span><span class="gd">--- a/app/controllers/users_controller.rb
</span><span class="gi">+++ b/app/controllers/users_controller.rb
</span><span class="p">@@ -184,6 +184,8 @@</span> class UsersController < ApplicationController
end
def destroy
<span class="gi">+ raise Unauthorized if @user == User.current && !@user.own_account_deletable?
+
</span> @user.destroy
respond_to do |format|
format.html { redirect_back_or_default(users_path) }
</code></pre> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=983562020-06-25T06:55:42ZMizuki ISHIKAWA
<ul><li><strong>File</strong> <a href="/attachments/25609">fix-11870.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/25609/fix-11870.patch">fix-11870.patch</a> added</li></ul><p>I've attached a patch based on <a class="issue tracker-1 status-5 priority-4 priority-default closed" title="Defect: Users can delete their own accounts unconditionally via REST API (Closed)" href="https://www.redmine.org/issues/11870#note-2">#11870#note-2</a>.<br />It was developed by pair programming with <a href="https://www.redmine.org/users/204091" class="external">@kfischer_okarin</a>.</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=983732020-06-25T23:18:28Zvzvu 3k6k
<ul></ul><p>LGTM.</p>
<p>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 <code>render_api_errors</code> or something, but anyway it looks good to me.</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=983992020-06-28T07:08:58ZGo MAEDA
<ul><li><strong>Target version</strong> set to <i>4.2.0</i></li></ul><p>Setting the target version to 4.2.0.</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=984312020-07-02T04:11:49ZMizuki ISHIKAWA
<ul><li><strong>File</strong> <a href="/attachments/25639">fix-11870-v2.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/25639/fix-11870-v2.patch">fix-11870-v2.patch</a> added</li></ul><p>vzvu 3k6k wrote:</p>
<blockquote>
<p>LGTM.</p>
<p>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 <code>render_api_errors</code> or something, but anyway it looks good to me.</p>
</blockquote>
<p>Thank you for your feedback.<br />I changed to return error message.</p>
<p><code>@user.own_account_deletable?</code> 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</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=985212020-07-13T07:47:06ZGo MAEDA
<ul></ul><p>Mizuki ISHIKAWA wrote:</p>
<blockquote>
<p>vzvu 3k6k wrote:</p>
<blockquote>
<p>LGTM.</p>
<p>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 <code>render_api_errors</code> or something, but anyway it looks good to me.</p>
</blockquote>
<p>Thank you for your feedback.<br />I changed to return error message.</p>
</blockquote>
<p>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.</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=985222020-07-13T08:04:04ZMizuki ISHIKAWA
<ul></ul><p>Go MAEDA wrote:</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>Thanks for reviewing this patch!</p>
<p>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.<br />I don't think the behavior of the API from the user's perspective is that special.</p>
<p>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.</p>
<hr />
<p>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!</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=985252020-07-13T09:13:44ZKevin Fischer
<ul></ul><p>I think for now it might be good to follow Redmine's current practice, i.e. no special error message.</p>
<p>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</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=985442020-07-14T16:39:33Zvzvu 3k6k
<ul><li><strong>File</strong> <a href="/attachments/25691">fix-11870-v3.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/25691/fix-11870-v3.patch">fix-11870-v3.patch</a> added</li></ul><p>Go MAEDA wrote:</p>
<blockquote>
<p>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.</p>
</blockquote>
<p>Thank you for pointing it out.</p>
<p>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.</p>
<p>Maybe this should be discussed on another issue as Kevin Fischer says.</p>
<hr />
<p>Mizuki ISHIKAWA wrote:</p>
<blockquote>
<p><code>@user.own_account_deletable?</code> 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</p>
</blockquote>
<p>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.</p>
<p>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 <code>User.current</code> was the last admin user.</p>
<p>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.</p> Redmine - Defect #11870: Users can delete their own accounts unconditionally via REST APIhttps://www.redmine.org/issues/11870?journal_id=1013102021-03-13T07:24:41ZGo MAEDA
<ul><li><strong>Subject</strong> changed from <i>REST API allows delete Admin user, making Redmine unusable</i> to <i>Users can delete their own accounts unconditionally via REST API</i></li><li><strong>Status</strong> changed from <i>New</i> to <i>Closed</i></li><li><strong>Assignee</strong> set to <i>Go MAEDA</i></li><li><strong>Resolution</strong> set to <i>Fixed</i></li></ul><p>Committed <a class="attachment" href="https://www.redmine.org/attachments/25609">fix-11870.patch</a>. Thank you for your contribution.</p>
<p>The following issues have been fixed in <a class="changeset" title="Fix that users can delete their own accounts unconditionally via REST API (#11870). Patch by Miz..." href="https://www.redmine.org/projects/redmine/repository/svn/revisions/20782">r20782</a>:</p>
<ul>
<li>Users can delete their own account even when the setting "Allow users to delete their own account" is disabled</li>
<li>An admin can delete their own account even if they are the last admin</li>
</ul>