diff --git a/hc/accounts/models.py b/hc/accounts/models.py index cd99fed0..6441f953 100644 --- a/hc/accounts/models.py +++ b/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 } diff --git a/hc/accounts/tests/test_check_token.py b/hc/accounts/tests/test_check_token.py index b814189c..36f9ab80 100644 --- a/hc/accounts/tests/test_check_token.py +++ b/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/") diff --git a/hc/accounts/tests/test_login.py b/hc/accounts/tests/test_login.py index a61f756e..c6b847fa 100644 --- a/hc/accounts/tests/test_login.py +++ b/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") diff --git a/hc/accounts/views.py b/hc/accounts/views.py index cc87c8de..125e07d4 100644 --- a/hc/accounts/views.py +++ b/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") diff --git a/hc/front/tests/test_add_slack_btn.py b/hc/front/tests/test_add_slack_btn.py index a564ab34..1d508a6a 100644 --- a/hc/front/tests/test_add_slack_btn.py +++ b/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 diff --git a/hc/front/views.py b/hc/front/views.py index 04b7a411..826e33c7 100644 --- a/hc/front/views.py +++ b/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) diff --git a/templates/integrations/add_slack.html b/templates/integrations/add_slack.html index 932a8efe..38e79f5a 100644 --- a/templates/integrations/add_slack.html +++ b/templates/integrations/add_slack.html @@ -35,26 +35,8 @@ {% site_name %}:

-
- {% csrf_token %} - -
-
-
@
- -
-
-
- -
-
+ Sign In
{% endif %} @@ -65,7 +47,7 @@
1

- 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