Just add a comment on settings.py with instructions to enable CSRF protection on Django 1.1

Signed-off-by: Guilherme Salgado <guilherme.salgado@linaro.org>
---
 apps/settings.py |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

On Wed, 2011-04-06 at 12:28 -0300, Guilherme Salgado wrote:
> Signed-off-by: Guilherme Salgado <guilherme.salgado@linaro.org>
> ---
>  apps/settings.py |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/apps/settings.py b/apps/settings.py
> index 68837b3..10813d4 100644
> --- a/apps/settings.py
> +++ b/apps/settings.py
> @@ -63,6 +63,9 @@ MIDDLEWARE_CLASSES = (
>      'django.contrib.auth.middleware.AuthenticationMiddleware',
>      'django.middleware.doc.XViewMiddleware',
>      'django.middleware.csrf.CsrfViewMiddleware',
> +    # If using Django 1.1, instead of the line above you'll need:
> +    # 'django.contrib.csrf.CsrfViewMiddleware',
> +    # 'django.contrib.csrf.CsrfResponseMiddleware',

In fact, this should've been 'django.contrib.csrf.middleware.Csrf...',
but although it should be enough to provide CSRF protection on Django
1.1 it doesn't seem to be enough to make Patchwork run on top of Django
1.1 because the templates use the 'csrf_token' tag, which is not
available in 1.1.

Maybe we should just update the docs to state that 1.2 or later is
required?

On Wed, 2011-04-06 at 13:20 -0300, Guilherme Salgado wrote:
> On Wed, 2011-04-06 at 12:28 -0300, Guilherme Salgado wrote:
> > Signed-off-by: Guilherme Salgado <guilherme.salgado@linaro.org>
> > ---
> >  apps/settings.py |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> > 
> > diff --git a/apps/settings.py b/apps/settings.py
> > index 68837b3..10813d4 100644
> > --- a/apps/settings.py
> > +++ b/apps/settings.py
> > @@ -63,6 +63,9 @@ MIDDLEWARE_CLASSES = (
> >      'django.contrib.auth.middleware.AuthenticationMiddleware',
> >      'django.middleware.doc.XViewMiddleware',
> >      'django.middleware.csrf.CsrfViewMiddleware',
> > +    # If using Django 1.1, instead of the line above you'll need:
> > +    # 'django.contrib.csrf.CsrfViewMiddleware',
> > +    # 'django.contrib.csrf.CsrfResponseMiddleware',
> 
> In fact, this should've been 'django.contrib.csrf.middleware.Csrf...',
> but although it should be enough to provide CSRF protection on Django
> 1.1 it doesn't seem to be enough to make Patchwork run on top of Django
> 1.1 because the templates use the 'csrf_token' tag, which is not
> available in 1.1.

1.1.2 seems to have a noop csrf_token tag, so you can either use that or
do something similar to http://code.djangoproject.com/changeset/11674 as
a monkey patch from within your app code.

This seems to be enough to get Patchwork to run on 1.1, but there are
several test failures which are not obvious to me, so they might indeed
represent things that are broken when running on 1.1.  This is one
example:

FAIL: testStateChangeInvalid
(patchwork.tests.updates.MultipleUpdateTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/srv/patchwork/apps/patchwork/tests/updates.py", line 93, in
testStateChangeInvalid
    response = self._testStateChange(state)
  File "/srv/patchwork/apps/patchwork/tests/updates.py", line 80, in
_testStateChange
    status_code = 200)
  File "/usr/lib/pymodules/python2.6/django/test/testcases.py", line
336, in assertContains
    (response.status_code, status_code))
AssertionError: Couldn't retrieve page: Response code was 403 (expected
200)'

Hi Guilherme,

> > --- a/apps/settings.py
> > +++ b/apps/settings.py
> > @@ -63,6 +63,9 @@ MIDDLEWARE_CLASSES = (
> >      'django.contrib.auth.middleware.AuthenticationMiddleware',
> >      'django.middleware.doc.XViewMiddleware',
> >      'django.middleware.csrf.CsrfViewMiddleware',
> > +    # If using Django 1.1, instead of the line above you'll need:
> > +    # 'django.contrib.csrf.CsrfViewMiddleware',
> > +    # 'django.contrib.csrf.CsrfResponseMiddleware',
> 
> In fact, this should've been 'django.contrib.csrf.middleware.Csrf...',
> but although it should be enough to provide CSRF protection on Django
> 1.1 it doesn't seem to be enough to make Patchwork run on top of Django
> 1.1 because the templates use the 'csrf_token' tag, which is not
> available in 1.1.
> 
> Maybe we should just update the docs to state that 1.2 or later is
> required?

I think that would be best. If we're getting test failures, we should
either get everything working with 1.1 again, or document that 1.2 is
required. If we apply this change, it might suggest that patchwork will
work with 1.1.

Cheers,


Jeremy

On Thu, 2011-04-14 at 14:33 +0800, Jeremy Kerr wrote:
> Hi Guilherme,
> 
> > > --- a/apps/settings.py
> > > +++ b/apps/settings.py
> > > @@ -63,6 +63,9 @@ MIDDLEWARE_CLASSES = (
> > >      'django.contrib.auth.middleware.AuthenticationMiddleware',
> > >      'django.middleware.doc.XViewMiddleware',
> > >      'django.middleware.csrf.CsrfViewMiddleware',
> > > +    # If using Django 1.1, instead of the line above you'll need:
> > > +    # 'django.contrib.csrf.CsrfViewMiddleware',
> > > +    # 'django.contrib.csrf.CsrfResponseMiddleware',
> > 
> > In fact, this should've been 'django.contrib.csrf.middleware.Csrf...',
> > but although it should be enough to provide CSRF protection on Django
> > 1.1 it doesn't seem to be enough to make Patchwork run on top of Django
> > 1.1 because the templates use the 'csrf_token' tag, which is not
> > available in 1.1.
> > 
> > Maybe we should just update the docs to state that 1.2 or later is
> > required?
> 
> I think that would be best. If we're getting test failures, we should
> either get everything working with 1.1 again, or document that 1.2 is
> required. If we apply this change, it might suggest that patchwork will
> work with 1.1.

I was hoping I'd be able to change the tests to make them pass using
either 1.1 or 1.2, but I didn't manage to, so I've just submitted
another patch changing docs/INSTALL to state that 1.2 is required.  I
also fixed a link to the deployment chapter of the django book, which
was broken.

Cheers,