launchpad-reviewers team mailing list archive

["Jeroen T. Vermeulen" <jtv@xxxxxxxxxxxxx>]

Jeroen T. Vermeulen has proposed merging lp:~jtv/maas/bug-971687 into lp:maas.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #971687 in MAAS: "Deleting the same ssh key on two different tabs returns a 404"
  https://bugs.launchpad.net/maas/+bug/971687

For more details, see:
https://code.launchpad.net/~jtv/maas/bug-971687/+merge/102669

As discussed with Julian.

Django's DeleteView doesn't do quite what you'd want: when you try to delete something that's no longer there (presumably it was there at one time, or where would you have gotten the link) it 404s.  That's nice for single-user applications, but a bit fragile in real-world situations.

This branch extends DeleteView with some extra features, such as built-in feedback messages and skipping of the confirmation screen if the object's already gone.  I didn't use it for the user-deletion view because I'm not quite sure it's safe to get the username for a profile if the profile and its User have already been deleted.

Along the way I also removed a shortcut that made things more convenient but now gets in the way, and at any rate didn't seem quite right: when the user tried to delete a key they weren't the owner of, they'd get a Not Found error instead of the Forbidden you'd expect.  Even though SSH keys are involved, I don't see any security implications for revealing the existence of a key (by numeric id) to somebody who doesn't own it.  On the other hand, debugging can be easier if you know _why_ something fails.
-- 
https://code.launchpad.net/~jtv/maas/bug-971687/+merge/102669
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~jtv/maas/bug-971687 into lp:maas.

=== modified file 'src/maasserver/tests/test_views.py'
--- src/maasserver/tests/test_views.py	2012-04-17 09:20:48 +0000
+++ src/maasserver/tests/test_views.py	2012-04-19 11:02:41 +0000
@@ -482,22 +482,46 @@
             [elem.get('action').strip() for elem in doc.cssselect(
                 '#content form')])
 
-    def test_delete_key_GET_cannot_access_someoneelses_key(self):
+    def test_delete_key_GET_cannot_access_someone_elses_key(self):
         key = factory.make_sshkey(factory.make_user())
         del_link = reverse('prefs-delete-sshkey', args=[key.id])
         response = self.client.get(del_link)
 
-        self.assertEqual(httplib.NOT_FOUND, response.status_code)
+        self.assertEqual(httplib.FORBIDDEN, response.status_code)
+
+    def test_delete_key_GET_nonexistent_key_redirects_to_prefs(self):
+        # Deleting a nonexistent key requires no confirmation.  It just
+        # "succeeds" instantaneously.
+        key = factory.make_sshkey(self.logged_in_user)
+        del_link = reverse('prefs-delete-sshkey', args=[key.id])
+        key.delete()
+        response = self.client.get(del_link)
+        self.assertEqual(
+            (httplib.FOUND, '/account/prefs/'),
+            (response.status_code, urlparse(response['Location']).path))
 
     def test_delete_key_POST(self):
-        # A POST request deletes the key.
+        # A POST request deletes the key, and redirects to the prefs.
         key = factory.make_sshkey(self.logged_in_user)
         del_link = reverse('prefs-delete-sshkey', args=[key.id])
         response = self.client.post(del_link, {'post': 'yes'})
 
-        self.assertEqual(httplib.FOUND, response.status_code)
+        self.assertEqual(
+            (httplib.FOUND, '/account/prefs/'),
+            (response.status_code, urlparse(response['Location']).path))
         self.assertFalse(SSHKey.objects.filter(id=key.id).exists())
 
+    def test_delete_key_POST_ignores_nonexistent_key(self):
+        # Deleting a key that's already been deleted?  Basically that's
+        # success.
+        key = factory.make_sshkey(self.logged_in_user)
+        del_link = reverse('prefs-delete-sshkey', args=[key.id])
+        key.delete()
+        response = self.client.post(del_link, {'post': 'yes'})
+        self.assertEqual(
+            (httplib.FOUND, '/account/prefs/'),
+            (response.status_code, urlparse(response['Location']).path))
+
 
 class AdminLoggedInTestCase(LoggedInTestCase):
 

=== modified file 'src/maasserver/views.py'
--- src/maasserver/views.py	2012-04-17 09:20:48 +0000
+++ src/maasserver/views.py	2012-04-19 11:02:41 +0000
@@ -51,8 +51,10 @@
 from django.core.exceptions import PermissionDenied
 from django.core.urlresolvers import reverse
 from django.http import (
+    Http404,
     HttpResponse,
     HttpResponseBadRequest,
+    HttpResponseForbidden,
     HttpResponseNotFound,
     HttpResponseRedirect,
     )
@@ -112,6 +114,79 @@
     return dj_logout(request, next_page=reverse('login'))
 
 
+class HelpfulDeleteView(DeleteView):
+    """Extension to Django's :class:`django.views.generic.DeleteView`.
+
+    This modifies `DeleteView` in a few ways:
+     - Deleting a nonexistent object is considered successful.
+     - There's a callback that lets you describe the object to the user.
+     - User feedback is built in.
+     - get_success_url defaults to returning the "next" URL.
+     - Confirmation screen also deals nicely with already-deleted object.
+
+    :ivar type_description: A name for the type of object that's being
+        deleted.  This gets included in user feedback text like
+        "<Object> deleted" and "<Object> not found."
+    """
+
+    # Override this for your concrete view type.
+    type_description = "Object"
+
+    def delete(self, *args, **kwargs):
+        """Delete result of self.get_object(), if any."""
+        try:
+            self.object = self.get_object()
+        except Http404:
+            self.object = None
+
+        if self.object is not None:
+            self.object.delete()
+        self.set_feedback_message(self.object)
+        return HttpResponseRedirect(self.get_success_url())
+
+    def get(self, *args, **kwargs):
+        """Prompt for confirmation of deletion request in the UI.
+
+        If the object has been deleted in the meantime, don't bother: just
+        redirect to the success URL and show a notice that the object is no
+        longer there.
+        """
+        try:
+            return super(HelpfulDeleteView, self).get(*args, **kwargs)
+        except Http404:
+            # Object is already gone.  Skip out of the whole deletion.
+            self.set_feedback_message(None)
+            return HttpResponseRedirect(self.get_success_url())
+
+    def set_feedback_message(self, obj=None):
+        """Set the deletion confirmation message to be shown to the user.
+
+        :param obj: The object that's being (or has been) deleted from the
+            database, if any.
+        """
+        if obj is None:
+            notice = "Not deleting: %s not found." % self.type_description
+        else:
+            notice = "%s deleted." % self.name_object(obj)
+        messages.info(self.request, notice)
+
+    def name_object(self, obj):
+        """Overridable: describe object being deleted to the user.
+
+        The result text will be included in a user notice along the lines of
+        "<Object> deleted."
+
+        :param obj: Object that's been deleted from the database.
+        :return: Description of the object, along the lines of
+            "User <obj.username>".
+        """
+        return self.type_description
+
+    def get_success_url(self):
+        """Forwards to `get_next_url`, if that's all the view has."""
+        return self.get_next_url()
+
+
 # Info message displayed on the node page for COMMISSIONING
 # or READY nodes.
 NODE_BOOT_INFO = mark_safe("""
@@ -179,11 +254,11 @@
         return reverse('node-view', args=[self.get_object().system_id])
 
 
-class NodeDelete(DeleteView):
+class NodeDelete(HelpfulDeleteView):
 
     template_name = 'maasserver/node_confirm_delete.html'
-
     context_object_name = 'node_to_delete'
+    type_description = "Node"
 
     def get_object(self):
         system_id = self.kwargs.get('system_id', None)
@@ -197,11 +272,9 @@
     def get_next_url(self):
         return reverse('node-list')
 
-    def delete(self, request, *args, **kwargs):
-        node = self.get_object()
-        node.delete()
-        messages.info(request, "Node %s deleted." % node.system_id)
-        return HttpResponseRedirect(self.get_next_url())
+    def name_object(self, obj):
+        """See `HelpfulDeleteView`."""
+        return "Node %s" % obj.system_id
 
 
 def get_longpoll_context():
@@ -261,22 +334,34 @@
         return reverse('prefs')
 
 
-class SSHKeyDeleteView(DeleteView):
+class NotYourKey(Exception):
+    """Marker exception: Can't delete this SSH key, since it's not yours."""
+
+
+class SSHKeyDeleteView(HelpfulDeleteView):
 
     template_name = 'maasserver/prefs_confirm_delete_sshkey.html'
     context_object_name = 'key'
+    type_description = "SSH key"
 
     def get_object(self):
         keyid = self.kwargs.get('keyid', None)
-        return get_object_or_404(SSHKey, user=self.request.user, id=keyid)
-
-    def form_valid(self, form):
-        messages.info(self.request, "SSH key deleted.")
-        return super(SSHKeyDeleteView, self).form_valid(form)
-
-    def get_success_url(self):
+        key = get_object_or_404(SSHKey, id=keyid)
+        if key.user != self.request.user:
+            raise NotYourKey()
+        return key
+
+    def get_next_url(self):
         return reverse('prefs')
 
+    def get(self, *args, **kwargs):
+        """Prompt user for confirmation of deletion request."""
+        try:
+            return super(SSHKeyDeleteView, self).get(*args, **kwargs)
+        except NotYourKey:
+            return HttpResponseForbidden(
+                "You cannot delete this key; it does not belong to you.")
+
 
 def process_form(request, form_class, redirect_url, prefix,
                  success_message=None, form_kwargs=None):