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

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.

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.

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

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

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.

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

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.

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!

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?

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

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.

Milestone post-1.0 deleted