23

In a mini blog app, I want to create a delete function, so that the owner of the blog can delete his entries (and only his entries). I guess that the only methods for doing do, is using a form. Though my the deletion code seems clear and correct, it doesn't work. My code:

def delete_new(request,id):
   u = New.objects.get(pk=id).delete()
   if request.method == 'POST':
       form = DeleteNewForm(request.POST)    
       form.u.delete()             
       form.save()   
   return render_to_response('news/deleteNew.html', {
           'form': form,
           }, 
        context_instance=RequestContext(request)) 

and in the template:

<a href='/news/delete_new/{{object.id}}/'> Delete</a> <br /> 

Is this a correct approach? I mean, creating a form for this? also, the only way to take the blog post associated with the deletion link is having an id as a parameter. Is it right? I mean, maybe any user can type another id, in the url, and delete another entry (eventually not one of his)

Siegmeyer
  • 4,312
  • 6
  • 26
  • 43
dana
  • 5,168
  • 20
  • 75
  • 116

2 Answers2

35

You need to use a form, or you're vulnerable to CSRF attacks. You're also deleting the model before you've checked whether the request was a GET or a POST.

Create a simple ModelForm:

from django import forms

from .models import New

class DeleteNewForm(forms.ModelForm):
    class Meta:
        model = New
        fields = []

In your views.py in the same Django app:

from django.shortcuts import render, get_object_or_404

from .forms import DeleteNewForm
from .models import New

def delete_new(request, new_id):
    new_to_delete = get_object_or_404(New, id=new_id)
    #+some code to check if this object belongs to the logged in user

    if request.method == 'POST':
        form = DeleteNewForm(request.POST, instance=new_to_delete)

        if form.is_valid(): # checks CSRF
            new_to_delete.delete()
            return HttpResponseRedirect("/") # wherever to go after deleting

    else:
        form = DeleteNewForm(instance=new_to_delete)

    template_vars = {'form': form}
    return render(request, 'news/deleteNew.html', template_vars)
Wilfred Hughes
  • 29,846
  • 15
  • 139
  • 192
  • 8
    To explain why this is best: in ANY case, submits, updates and deletes MUST use POST (or PUT) requests. GET requests can be masked in otherwise inoffensive-looking links. Say, if a major site had a simple "Delete all my stuff" at example.com/account/delete/, I could hide such a link in a blog post, or in TinyURL-like services. Upon visit, you'd see a nice "Your profile was deleted." page. Hybrid methods such as Wilfred Hughes', display a confirmation page on GET, that contains a form with a CSRF token. POST requests validate for CSRF and then ultimately delete the requested resource. – sleblanc Apr 01 '13 at 04:02
21

In general, for deleting objects you should rather use POST (or DELETE) HTTP methods.

If you really want to use HTTP GET for your example, here is what you need to fix:

If you have url pointing to some url like yours: <a href='/news/delete_new/{{object.id}}/'> Delete</a> then you can simply write view that will check if object belongs to logged in user and delete this entry if yes, like in code you have already written:

def delete_new(request,id):
   #+some code to check if New belongs to logged in user
   u = New.objects.get(pk=id).delete()

To check if New objects belogs to some user you need to create realation between User and New (like created_by = models.ForeignKey(User) in New model).

You can get logged in user this way: request.user

I hope I got your point correctly and my answer helps you somehow.

PS: You can also consider using {% url %} tag instead of writing urls directly in your templates.

dzida
  • 8,854
  • 2
  • 36
  • 57
  • 1
    it rolls smoothly, and in just one line: u = New.objects.filter(created_by = request.user).get(pk=id).delete() thank you! :) – dana Jun 26 '10 at 17:59
  • Exactly, you can add some error handling if there is no such New object for a given user and display nice error message (with your one liner it will fails loudly with 500 error). But basically that is all what you need to do here:) I'm glad that my answer was helpful for you. – dzida Jun 26 '10 at 18:34
  • 24
    I'd strongly recommend using a form and checking POST, because GETs to pages are not supposed to change the state on the server. (Although, in practice what's happening here is relatively 'safe') – Steve Jalim Jun 27 '10 at 11:53
  • 14
    Doing this would open your website up to CSRF, as no token is transmitted/required. – Gelatin Jun 30 '12 at 02:37
  • 4
    This is *not* safe and enables CSRF attacks in which a user deletes content belonging to other users. – Natan Yellin Sep 10 '12 at 18:31