Fix after-login redirects for users landing in the "Add Slack" page

hc/accounts/models.py

@@ -90,11 +90,14 @@ class Profile(models.Model):
     def check_token(self, token, salt):
         return salt in self.token and check_password(token, self.token)
 
-    def send_instant_login_link(self, inviting_profile=None):
+    def send_instant_login_link(self, inviting_profile=None, redirect_url=None):
         token = self.prepare_token("login")
         path = reverse("hc-check-token", args=[self.user.username, token])
+        if redirect_url:
+            path += "?next=%s" % redirect_url
+
         ctx = {
-            "button_text": "Log In",
+            "button_text": "Sign In",
             "button_url": settings.SITE_ROOT + path,
             "inviting_profile": inviting_profile
         }

hc/accounts/tests/test_check_token.py

@@ -35,3 +35,11 @@ class CheckTokenTestCase(BaseTestCase):
         r = self.client.post(url, follow=True)
         self.assertRedirects(r, "/accounts/login/")
         self.assertContains(r, "incorrect or expired")
+
+    def test_it_handles_next_parameter(self):
+        r = self.client.post("/accounts/check_token/alice/secret-token/?next=/integrations/add_slack/")
+        self.assertRedirects(r, "/integrations/add_slack/")
+
+    def test_it_ignores_bad_next_parameter(self):
+        r = self.client.post("/accounts/check_token/alice/secret-token/?next=/evil/")
+        self.assertRedirects(r, "/checks/")

hc/accounts/tests/test_login.py

@@ -14,7 +14,7 @@ class LoginTestCase(TestCase):
         form = {"identity": "alice@example.org"}
 
         r = self.client.post("/accounts/login/", form)
-        assert r.status_code == 302
+        self.assertRedirects(r, "/accounts/login_link_sent/")
 
         # Alice should be the only existing user
         self.assertEqual(User.objects.count(), 1)
@@ -24,6 +24,20 @@ class LoginTestCase(TestCase):
         subject = "Log in to %s" % settings.SITE_NAME
         self.assertEqual(mail.outbox[0].subject, subject)
 
+    def test_it_sends_link_with_next(self):
+        alice = User(username="alice", email="alice@example.org")
+        alice.save()
+
+        form = {"identity": "alice@example.org"}
+
+        r = self.client.post("/accounts/login/?next=/integrations/add_slack/", form)
+        self.assertRedirects(r, "/accounts/login_link_sent/")
+
+        # The check_token link should have a ?next= query parameter:
+        self.assertEqual(len(mail.outbox), 1)
+        body = mail.outbox[0].body
+        self.assertTrue("/?next=/integrations/add_slack/" in body)
+
     def test_it_pops_bad_link_from_session(self):
         self.client.session["bad_link"] = True
         self.client.get("/accounts/login/")
@@ -36,7 +50,7 @@ class LoginTestCase(TestCase):
         form = {"identity": "ALICE@EXAMPLE.ORG"}
 
         r = self.client.post("/accounts/login/", form)
-        assert r.status_code == 302
+        self.assertRedirects(r, "/accounts/login_link_sent/")
 
         # There should be exactly one user:
         self.assertEqual(User.objects.count(), 1)
@@ -56,7 +70,35 @@ class LoginTestCase(TestCase):
         }
 
         r = self.client.post("/accounts/login/", form)
-        self.assertEqual(r.status_code, 302)
+        self.assertRedirects(r, "/checks/")
+
+    def test_it_handles_password_login_with_redirect(self):
+        alice = User(username="alice", email="alice@example.org")
+        alice.set_password("password")
+        alice.save()
+
+        form = {
+            "action": "login",
+            "email": "alice@example.org",
+            "password": "password"
+        }
+
+        r = self.client.post("/accounts/login/?next=/integrations/add_slack/", form)
+        self.assertRedirects(r, "/integrations/add_slack/")
+
+    def test_it_handles_bad_next_parameter(self):
+        alice = User(username="alice", email="alice@example.org")
+        alice.set_password("password")
+        alice.save()
+
+        form = {
+            "action": "login",
+            "email": "alice@example.org",
+            "password": "password"
+        }
+
+        r = self.client.post("/accounts/login/?next=/evil/", form)
+        self.assertRedirects(r, "/checks/")
 
     def test_it_handles_wrong_password(self):
         alice = User(username="alice", email="alice@example.org")

hc/accounts/views.py

@@ -25,6 +25,9 @@ from hc.api.models import Channel, Check
 from hc.lib.badges import get_badge_url
 from hc.payments.models import Subscription
 
+NEXT_WHITELIST = ("/checks/",
+                  "/integrations/add_slack/")
+
 
 def _make_user(email):
     username = str(uuid.uuid4())[:30]
@@ -59,6 +62,16 @@ def _ensure_own_team(request):
         request.profile.save()
 
 
+def _redirect_after_login(request):
+    """ Redirect to the URL indicated in ?next= query parameter. """
+
+    redirect_url = request.GET.get("next")
+    if redirect_url in NEXT_WHITELIST:
+        return redirect(redirect_url)
+
+    return redirect("hc-checks")
+
+
 def login(request):
     form = EmailPasswordForm()
     magic_form = ExistingEmailForm()
@@ -68,13 +81,19 @@ def login(request):
             form = EmailPasswordForm(request.POST)
             if form.is_valid():
                 auth_login(request, form.user)
-                return redirect("hc-checks")
+                return _redirect_after_login(request)
 
         else:
             magic_form = ExistingEmailForm(request.POST)
             if magic_form.is_valid():
                 profile = Profile.objects.for_user(magic_form.user)
-                profile.send_instant_login_link()
+
+                redirect_url = request.GET.get("next")
+                if redirect_url in NEXT_WHITELIST:
+                    profile.send_instant_login_link(redirect_url=redirect_url)
+                else:
+                    profile.send_instant_login_link()
+
                 return redirect("hc-login-link-sent")
 
     bad_link = request.session.pop("bad_link", None)
@@ -122,7 +141,7 @@ def link_sent(request):
 def check_token(request, username, token):
     if request.user.is_authenticated and request.user.username == username:
         # User is already logged in
-        return redirect("hc-checks")
+        return _redirect_after_login(request)
 
     # Some email servers open links in emails to check for malicious content.
     # To work around this, we sign user in if the method is POST.
@@ -137,7 +156,7 @@ def check_token(request, username, token):
             user.profile.save()
             auth_login(request, user)
 
-            return redirect("hc-checks")
+            return _redirect_after_login(request)
 
         request.session["bad_link"] = True
         return redirect("hc-login")

hc/front/tests/test_add_slack_btn.py

@@ -9,13 +9,12 @@ from mock import patch
 class AddSlackBtnTestCase(BaseTestCase):
 
     @override_settings(SLACK_CLIENT_ID="foo")
-    def test_instructions_work(self):
+    def test_it_prepares_login_link(self):
         r = self.client.get("/integrations/add_slack/")
         self.assertContains(r, "Before adding Slack integration",
                             status_code=200)
 
-        # There should now be a key in session
-        self.assertTrue("slack" in self.client.session)
+        self.assertContains(r, "?next=/integrations/add_slack/")
 
     @override_settings(SLACK_CLIENT_ID="foo")
     def test_slack_button(self):
@@ -23,6 +22,9 @@ class AddSlackBtnTestCase(BaseTestCase):
         r = self.client.get("/integrations/add_slack/")
         self.assertContains(r, "slack.com/oauth/authorize", status_code=200)
 
+        # There should now be a key in session
+        self.assertTrue("slack" in self.client.session)
+
     @patch("hc.front.views.requests.post")
     def test_it_handles_oauth_response(self, mock_post):
         session = self.client.session

hc/front/views.py

@@ -692,7 +692,7 @@ def add_slack(request):
         "slack_client_id": settings.SLACK_CLIENT_ID
     }
 
-    if settings.SLACK_CLIENT_ID:
+    if settings.SLACK_CLIENT_ID and request.user.is_authenticated:
         ctx["state"] = _prepare_state(request, "slack")
 
     return render(request, "integrations/add_slack.html", ctx)

templates/integrations/add_slack.html

@@ -35,26 +35,8 @@
         {% site_name %}:</p>
 
         <div class="text-center">
-        <form class="form-inline" action="{% url 'hc-login' %}" method="post">
-            {% csrf_token %}
-
-            <div class="form-group">
-                <div class="input-group input-group-lg">
-                    <div class="input-group-addon">@</div>
-                    <input
-                        type="email"
-                        class="form-control"
-                        name="email"
-                        autocomplete="email"
-                        placeholder="Email">
-                </div>
-            </div>
-            <div class="form-group">
-                <button type="submit" class="btn btn-lg btn-primary pull-right">
-                    Log In
-                </button>
-            </div>
-        </form>
+            <a href="{% url 'hc-login' %}?next={% url 'hc-add-slack' %}"
+               class="btn btn-primary btn-lg">Sign In</a>
         </div>
         {% endif %}
     </div>
@@ -65,7 +47,7 @@
         <div class="col-sm-6">
             <span class="step-no">1</span>
             <p>
-                After {% if request.user.is_authenticated %}{% else %}logging in and{% endif %}
+                After {% if request.user.is_authenticated %}{% else %}signing in and{% endif %}
                 clicking on "Add to Slack", you should
                 be on a page that says "{% site_name %} would like access to
                 your Slack team". Select the team you want to add the