Opened 7 years ago

Closed 4 years ago

#8159 closed Uncategorized (fixed)

Attempting to delete your own user account in Django admin view is not handled properly

Reported by: erichs@… Owned by: graham_king
Component: contrib.admin Version: master
Severity: Normal Keywords: admin delete
Cc: bas@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX:

Description (last modified by mtredinnick)

When trying to attempt your own user account from the Django admin pages, it firstly doesn't warn you or disallow such an action, and secondly fails badly with an integrity error (see below). As a side effect you can now no longer logout.:

Traceback (most recent call last):

  File "C:\Users\erichs\Documents\eclipse\django\django\core\servers\basehttp.py", line 277, in run
    self.result = application(self.environ, self.start_response)

  File "C:\Users\erichs\Documents\eclipse\django\django\core\servers\basehttp.py", line 634, in __call__
    return self.application(environ, start_response)

  File "C:\Users\erichs\Documents\eclipse\django\django\core\handlers\wsgi.py", line 221, in __call__
    response = middleware_method(request, response)

  File "C:\Users\erichs\Documents\eclipse\django\django\middleware\transaction.py", line 25, in process_response
    transaction.commit()

  File "C:\Users\erichs\Documents\eclipse\django\django\db\transaction.py", line 157, in commit
    connection._commit()

  File "C:\Users\erichs\Documents\eclipse\django\django\db\backends\__init__.py", line 23, in _commit
    return self.connection.commit()

IntegrityError: update or delete on table "auth_user" violates foreign key constraint "auth_message_user_id_fkey" on table "auth_message"
DETAIL:  Key (id)=(1) is still referenced from table "auth_message".

Attachments (3)

8159.diff (3.2 KB) - added by graham_king 7 years ago.
Adds a warning when deleting yourself, and doesn't log the delete and set the message, so the foreignkey doesn't break
8159-warning.diff (630 bytes) - added by graham_king 6 years ago.
Adds a warning when attempting to delete yourself. This patch is the warning only.
8159-2.diff (4.7 KB) - added by graham_king 6 years ago.
Adds a warning when attempting to delete yourself, fixes the foreign key breakage when deleting yourself, and has a test - updated

Download all attachments as: .zip

Change History (17)

comment:1 Changed 7 years ago by mtredinnick

  • Description modified (diff)
  • milestone changed from 1.0 beta to post-1.0
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

Fixed description and milestone.

Moving to post-1.0, since it's a slight usability wart, rather than a real showstopper. "Don't do that" is a reasonable workaround for now.

comment:2 Changed 7 years ago by graham_king

  • Owner changed from nobody to graham_king
  • Status changed from new to assigned

Steps to reproduce:

  • Make sure your db has foreign key support. On MySQL MyISAM and sqlite3 this ticket does not apply. I can reproduce it on MySQL InnoDB.
  • Create a test user
  • Login as that test user and do something (create a group for example)
  • Try and delete test user.

comment:3 Changed 7 years ago by graham_king

  • Has patch set

Changed 7 years ago by graham_king

Adds a warning when deleting yourself, and doesn't log the delete and set the message, so the foreignkey doesn't break

comment:4 Changed 7 years ago by graham_king

As this doesn't occur on sqlite, I'm hoping it will be accepted without a unit test.

comment:5 Changed 7 years ago by graham_king

  • Triage Stage changed from Accepted to Ready for checkin

comment:6 Changed 7 years ago by mtredinnick

  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

Please don't move your own tickets to ready-for-checkin, since that skips the final review process.

I had a quick read and there's a few things that can be potentially be improved in the implementation:

  1. Signal connections are relatively expensive if nothing else is using that signal and post_delete isn't used by anything in Django except file fields. It's a fairly blunt instrument to be using just for this one edge case (since the signal hookup is essentially added every single time). It would be nice if there was a way to do this without using the signal. I can't see immediately how to do that, but some thought might reveal a solution.
  2. If the signal stuff ends up having to stay, the signal handler can be improved. You already know the attribute name of the primary key on the User model -- it's called "pk" (and "id", but "pk" is a nice generic name). So all the getattr() and setattr() stuff can go away.
  3. We use triple-quoted strings for docstrings, not single quoted strings.

I don't understand how the SQLite behaviour is related to unit testing. It looks like something that needs a test.

Changed 6 years ago by graham_king

Adds a warning when attempting to delete yourself. This patch is the warning only.

comment:7 Changed 6 years ago by graham_king

  • Patch needs improvement unset
  1. Apologies for taking so long to get back to this. I was expecting an e-mail from Trac.
  1. Point taken about changing to 'Ready for checkin'
  1. The new patch 8159-2.diff doesn't use signals. Instead in UserAdmin it overrides delete_view, and nulls the request.user.id in there. The patch now only changes one Python file instead of three.

comment:8 Changed 6 years ago by graham_king

  • Needs tests unset

Dear reviewer,

  • Ignore 8159.diff - if you have the right permissions please delete it.
  • 8159-2.diff is the patch of interest. It contains:
    • a template to add a warning in the admin interface if you attempt to delete yourself.
    • a fix in contrib/auth/admin.py to prevent the foreign key blowing up if you do delete yourself
    • a test for that foreign key explosion
  • To reproduce, make sure your database enforces foreign keys (I used MySQL InnoDB), then either:
    • Apply the patch, revert django/contrib/auth/admin.py, and run the 'admin_views.DeleteSelfTest' test.
    • OR create a test user with staff and superuser, login as that test user, delete that test user (i.e. yourself).
  • If this patch is not acceptable, I'd like you to consider 8159-warning.diff. This is the same as 8159-2.diff, except without the fix and the test, so _just_ the template warning which displays 'You are attempting to delete yourself'.
  • Cheers!

comment:9 Changed 6 years ago by kmtracey

  • Patch needs improvement set

I tried this out -- beyond just running the test I tried it on one of my test setups and ran into a problem. Looks like if you try to delete yourself and you are not a superuser, things go south (the test deletes a superuser, so doesn't run afoul of the problem). If you change the new test to login as and delete "adduser" instead of "super", you can see the problem:

test_delete_myself (regressiontests.admin_views.tests.DeleteSelfTest) ... ERROR

======================================================================
ERROR: test_delete_myself (regressiontests.admin_views.tests.DeleteSelfTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "D:\u\kmt\django\trunk\tests\regressiontests\admin_views\tests.py", line 747, in test_delete_myself
    response = self.client.post('/admin/auth/user/'+ str(u.id) +'/delete/', {'post': 'yes'})
  File "d:\u\kmt\django\trunk\django\test\client.py", line 299, in post
    return self.request(**r)
  File "d:\u\kmt\django\trunk\django\core\handlers\base.py", line 86, in get_response
    response = callback(request, *callback_args, **callback_kwargs)
  File "d:\u\kmt\django\trunk\django\contrib\admin\sites.py", line 450, in root
    return self.model_page(request, *url.split('/', 2))
  File "d:\u\kmt\django\trunk\django\views\decorators\cache.py", line 44, in _wrapped_view_func
    response = view_func(request, *args, **kwargs)
  File "d:\u\kmt\django\trunk\django\contrib\admin\sites.py", line 469, in model_page
    return admin_obj(request, rest_of_url)
  File "d:\u\kmt\django\trunk\django\contrib\auth\admin.py", line 42, in __call__
    return super(UserAdmin, self).__call__(request, url)
  File "d:\u\kmt\django\trunk\django\contrib\admin\options.py", line 799, in __call__
    return self.delete_view(request, unquote(url[:-7]))
  File "d:\u\kmt\django\trunk\django\contrib\auth\admin.py", line 147, in delete_view
    return super(UserAdmin, self).delete_view(request, object_id, extra_context)
  File "d:\u\kmt\django\trunk\django\contrib\admin\options.py", line 706, in delete_view
    if not self.has_delete_permission(request, obj):
  File "d:\u\kmt\django\trunk\django\contrib\admin\options.py", line 273, in has_delete_permission
    return request.user.has_perm(opts.app_label + '.' + opts.get_delete_permission())
  File "d:\u\kmt\django\trunk\django\contrib\auth\models.py", line 232, in has_perm
    if backend.has_perm(self, perm):
  File "d:\u\kmt\django\trunk\django\contrib\auth\backends.py", line 65, in has_perm
    return perm in self.get_all_permissions(user_obj)
  File "d:\u\kmt\django\trunk\django\contrib\auth\backends.py", line 60, in get_all_permissions
    user_obj._perm_cache = set([u"%s.%s" % (p.content_type.app_label, p.codename) for p in user_obj.user_permissions.select_related()])
  File "d:\u\kmt\django\trunk\django\db\models\fields\related.py", line 568, in __get__
    target_col_name=qn(self.field.m2m_reverse_name())
  File "d:\u\kmt\django\trunk\django\db\models\fields\related.py", line 380, in __init__
    raise ValueError("%r instance needs to have a primary key value before a many-to-many relationship can be used." % instance.__class__.__name__)
ValueError: 'User' instance needs to have a primary key value before a many-to-many relationship can be used.

----------------------------------------------------------------------
Ran 1 test in 0.631s

FAILED (errors=1)
Destroying test database...

I guess if the logged-in user is not a superuser, permissions have to be checked, but the change to avoid problems when deleting self has made that impossible?

Changed 6 years ago by graham_king

Adds a warning when attempting to delete yourself, fixes the foreign key breakage when deleting yourself, and has a test - updated

comment:10 Changed 6 years ago by graham_king

  • Patch needs improvement unset

Yes, that was the problem. I have added a test for that case.

I've updated the patch to take a different route. It catches the IntegrityError. If the logged in user is gone, it ignores the exception, if not it raises it.

The crux of the problem is that the same user is in memory twice - once as the logged in user and once as the deleted user. The id of the deleted user gets correctly null-ed, but not the id of the logged in user. The single instance proposal would fix this: http://code.djangoproject.com/wiki/DjangoSpecifications/Core/SingleInstance

In the meantime this patch adds a helpful warning and prevents things blowing up.

comment:11 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:12 Changed 5 years ago by parxier

  • Cc bas@… added

comment:13 Changed 4 years ago by patchhammer

  • Easy pickings unset
  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Uncategorized

8159-2.diff fails to apply cleanly on to trunk

comment:14 Changed 4 years ago by graham_king

  • Resolution set to fixed
  • Status changed from assigned to closed

This is no longer a problem (in 1.4 pre-alpha SVN-16223 and probably much before). Tested in MySQL and SQLite. Attempting to delete your own user account does exactly what you'd expect: it deletes it. Closing.

Note: See TracTickets for help on using tickets.
Back to Top