use /login?to= instead of next= (bug 583117)

Bonus: login_required plays nice with HTTP if you like that.

apps/amo/decorators.py

@@ -2,6 +2,31 @@ import functools
 import json
 
 from django import http
+from django.contrib.auth import decorators as auth_decorators
+
+
+def login_required(f=None, redirect=True):
+    """
+    Like Django's login_required, but with to= instead of next=.
+
+    If redirect=False then we return 401 instead of redirecting to the
+    login page.  That's nice for ajax views.
+    """
+    if redirect:
+        return auth_decorators.login_required(f, redirect_field_name='to')
+
+    def decorator(func):
+        @functools.wraps(func)
+        def wrapper(request, *args, **kw):
+            if request.user.is_authenticated():
+                return func(request, *args, **kw)
+            else:
+                return http.HttpResponse(status=401)
+        return wrapper
+    if f:
+        return decorator(f)
+    else:
+        return decorator
 
 
 def post_required(f):

apps/amo/tests/test_decorators.py

@@ -4,6 +4,7 @@ import mock
 from nose.tools import eq_
 
 from amo import decorators
+from amo.urlresolvers import reverse
 
 
 def test_post_required():
@@ -42,3 +43,40 @@ def test_json_view_error():
     assert isinstance(response, http.HttpResponseBadRequest)
     eq_(response.content, '{"msg": "error"}')
     eq_(response['Content-Type'], 'application/json')
+
+
+class TestLoginRequired(object):
+
+    def setUp(self):
+        self.f = mock.Mock()
+        self.f.__name__ = 'function'
+        self.request = mock.Mock()
+        self.request.user.is_authenticated.return_value = False
+        self.request.get_full_path.return_value = 'path'
+
+    def test_normal(self):
+        func = decorators.login_required(self.f)
+        response = func(self.request)
+        assert not self.f.called
+        eq_(response.status_code, 302)
+        eq_(response['Location'],
+            '%s?to=%s' % (reverse('users.login'), 'path'))
+
+    def test_no_redirect(self):
+        func = decorators.login_required(self.f, redirect=False)
+        response = func(self.request)
+        assert not self.f.called
+        eq_(response.status_code, 401)
+
+    def test_decorator_syntax(self):
+        # @login_required(redirect=False)
+        func = decorators.login_required(redirect=False)(self.f)
+        response = func(self.request)
+        assert not self.f.called
+        eq_(response.status_code, 401)
+
+    def test_no_redirect_success(self):
+        func = decorators.login_required(redirect=False)(self.f)
+        self.request.user.is_authenticated.return_value = True
+        response = func(self.request)
+        assert self.f.called

apps/amo/tests/test_redirects.py

@@ -54,7 +54,7 @@ class TestRedirects(amo.test_utils.ExtraSetup, test_utils.TestCase):
     def test_parameters(self):
         """Bug 554976. Make sure when we redirect, we preserve our query
         strings."""
-        url = u'/users/login?next=/en-US/firefox/users/edit'
+        url = u'/users/login?to=/en-US/firefox/users/edit'
         r = self.client.get(url, follow=True)
         self.assertRedirects(r, '/en-US/firefox' + url, status_code=301)
 

apps/bandwagon/views.py

@@ -1,11 +1,11 @@
 from django import http
-from django.contrib.auth.decorators import login_required
 from django.shortcuts import get_object_or_404, redirect
 
 import jingo
 from tower import ugettext_lazy as _lazy
 
 import amo.utils
+from amo.decorators import login_required
 from access import acl
 from addons.models import Addon
 from addons.views import BaseFilter

apps/reviews/tests/test_views.py

@@ -29,7 +29,7 @@ class TestFlag(ReviewTest):
     def test_no_login(self):
         self.client.logout()
         response = self.client.post(self.url)
-        assert isinstance(response, http.HttpResponseRedirect)
+        eq_(response.status_code, 401)
 
     def test_new_flag(self):
         response = self.client.post(self.url, {'flag': 'spam'})
@@ -74,7 +74,7 @@ class TestDelete(ReviewTest):
     def test_no_login(self):
         self.client.logout()
         response = self.client.post(self.url)
-        assert isinstance(response, http.HttpResponseRedirect)
+        eq_(response.status_code, 401)
 
     def test_no_perms(self):
         GroupUser.objects.all().delete()

apps/reviews/views.py

@@ -1,5 +1,4 @@
 from django import http
-from django.contrib.auth.decorators import login_required
 from django.shortcuts import get_object_or_404, redirect
 
 import commonware.log
@@ -7,7 +6,7 @@ import jingo
 from tower import ugettext as _
 
 import amo.utils
-from amo.decorators import post_required, json_view
+from amo.decorators import post_required, json_view, login_required
 from access import acl
 from addons.models import Addon
 
@@ -76,7 +75,7 @@ def get_flags(request, reviews):
 
 
 @post_required
-@login_required  # TODO: return a 401?
+@login_required(redirect=False)
 @json_view
 def flag(request, addon_id, review_id):
     d = dict(review=review_id, user=request.user.id)
@@ -96,7 +95,7 @@ def flag(request, addon_id, review_id):
 
 
 @post_required
-@login_required
+@login_required(redirect=False)
 def delete(request, addon_id, review_id):
     if not acl.action_allowed(request, 'Editors', 'DeleteReview'):
         return http.HttpResponseForbidden()
@@ -156,10 +155,9 @@ def add(request, addon_id):
                         dict(addon=addon, form=form))
 
 
+@login_required(redirect=False)
 @post_required
 def edit(request, addon_id, review_id):
-    if not request.user.is_authenticated():
-        return http.HttpResponse(status=401)
     review = get_object_or_404(Review.objects, pk=review_id, addon=addon_id)
     is_admin = acl.action_allowed(request, 'Admin', 'EditAnyAddon')
     if not (request.user.id == review.user.id or is_admin):

apps/users/views.py

@@ -5,7 +5,6 @@ from django.conf import settings
 from django.core.mail import send_mail
 from django.shortcuts import get_object_or_404
 from django.contrib import auth
-from django.contrib.auth.decorators import login_required
 from django.contrib import messages
 from django.template import Context, loader
 
@@ -14,9 +13,10 @@ import jingo
 from tower import ugettext as _
 
 
-from access import acl
 import amo
+from amo.decorators import login_required
 from amo.urlresolvers import reverse
+from access import acl
 from bandwagon.models import Collection
 
 from .models import UserProfile