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

[erichs@…]

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".

[Malcolm Tredinnick]

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.

[Graham King]

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.

[Graham King]

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

[Graham King]

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

[Malcolm Tredinnick]

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.

[Graham King]

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

[Graham King]

1. Apologies for taking so long to get back to this. I was expecting an e-mail from Trac.

2. Point taken about changing to 'Ready for checkin'

3. 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.

[Graham King]

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!

[Karen Tracey]

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?

[Graham King]

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

[Graham King]

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.

[(none)]

Milestone post-1.0 deleted

[patchhammer]

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

[Graham King]

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.