Improve accept invite flow (#4255)

- Show a message if you're an existing member (and allow logout)
- Add next param to login/register urls

src/sentry/templates/sentry/accept-organization-invite.html

@@ -19,6 +19,14 @@
           {% blocktrans with org_name=organization.name %}<strong>{{ org_name }}</strong> is using Sentry to aggregate errors.{% endblocktrans %}
         </p>
 
+        {% if existing_member %}
+          <div class="alert alert-block">
+            <p>Your account (<strong>{{ request.user.username }}</strong>) is already a member of this organization.</p>
+
+            <p>Did you want to <a href="{{ logout_url }}">switch accounts</a>?</p>
+          </div>
+        {% endif %}
+
         <p>{% blocktrans %}You have been invited to join this organization, which manages <strong>{{ project_count }}</strong> project(s), including:{% endblocktrans %}</p>
         <ul>
           {% for project in project_list|slice:"5" %}
@@ -33,9 +41,9 @@
 
           <fieldset class="form-actions">
             <div class="pull-right" style="margin-top: 5px;">
-              <a href="{% url 'sentry-login' %}">{% trans "Login as an existing user" %}</a>
+              <a href="{{ login_url }}">{% trans "Login as an existing user" %}</a>
             </div>
-            <a href="{% url 'sentry-register' %}" class="btn btn-primary">{% trans "Create a new account" %}</a>
+            <a href="{{ register_url }}" class="btn btn-primary">{% trans "Create a new account" %}</a>
           </fieldset>
         {% else %}
           <form method="POST">

src/sentry/utils/auth.py

@@ -120,14 +120,26 @@ def get_login_redirect(request, default=None):
     if after_2fa is not None:
         return after_2fa
 
-    login_url = request.session.pop('_next', None) or default
-    if login_url.startswith(('http://', 'https://')):
-        login_url = default
-    elif login_url.startswith(get_login_url()):
+    login_url = request.session.pop('_next', None)
+    if not login_url:
+        return default
+
+    if not is_valid_redirect(login_url):
         login_url = default
+
     return login_url
 
 
+def is_valid_redirect(url):
+    if not url:
+        return False
+    if url.startswith(('http://', 'https://')):
+        return False
+    if url.startswith(get_login_url()):
+        return False
+    return True
+
+
 def find_users(username, with_valid_password=True, is_active=None):
     """
     Return a list of users that match a username

src/sentry/web/frontend/accept_organization_invite.py

@@ -69,6 +69,18 @@ class AcceptOrganizationInviteView(BaseView):
             'project_list': project_list,
             'project_count': project_count,
             'needs_authentication': not request.user.is_authenticated(),
+            'logout_url': '{}?next={}'.format(
+                reverse('sentry-logout'),
+                request.path,
+            ),
+            'login_url': '{}?next={}'.format(
+                reverse('sentry-login'),
+                request.path,
+            ),
+            'register_url': '{}?next={}'.format(
+                reverse('sentry-register'),
+                request.path,
+            ),
         }
 
         if not request.user.is_authenticated():
@@ -79,6 +91,14 @@ class AcceptOrganizationInviteView(BaseView):
 
             return self.respond('sentry/accept-organization-invite.html', context)
 
+        # if they're already a member of the organization its likely they're
+        # using a shared account and either previewing this invite or
+        # are incorrectly expecting this to create a new account for them
+        context['existing_member'] = OrganizationMember.objects.filter(
+            user=request.user.id,
+            organization=om.organization_id,
+        ).exists()
+
         form = self.get_form(request)
         if form.is_valid():
             if OrganizationMember.objects.filter(organization=organization, user=request.user).exists():

src/sentry/web/frontend/auth_login.py

@@ -96,8 +96,6 @@ class AuthLoginView(BaseView):
 
             return self.redirect(auth.get_login_redirect(request))
 
-        request.session.set_test_cookie()
-
         context = {
             'op': op or 'login',
             'server_hostname': get_server_hostname(),
@@ -125,14 +123,17 @@ class AuthLoginView(BaseView):
     @never_cache
     @transaction.atomic
     def handle(self, request):
-        redirect_next = request.GET.get('next', None)
-        if redirect_next:
-            # TODO: enforce max redirect_next limit? (potential for abuse?)
-            request.session['_next'] = redirect_next
-
+        next_uri = request.GET.get('next', None)
         if request.user.is_authenticated():
+            if auth.is_valid_redirect(next_uri):
+                return self.redirect(next_uri)
             return self.redirect_to_org(request)
 
+        request.session.set_test_cookie()
+
+        if next_uri:
+            auth.initiate_login(request, next_uri)
+
         # Single org mode -- send them to the org-specific handler
         if settings.SENTRY_SINGLE_ORGANIZATION:
             org = Organization.get_default()

src/sentry/web/frontend/auth_logout.py

@@ -4,14 +4,16 @@ from django.contrib.auth import logout
 from django.contrib.auth.models import AnonymousUser
 
 from sentry.web.frontend.base import BaseView
-from sentry.utils.auth import get_login_redirect
+from sentry.utils import auth
 
 
 class AuthLogoutView(BaseView):
     auth_required = False
 
     def handle(self, request):
-        rv = get_login_redirect(request)
+        next = request.GET.get('next', '')
+        if not next.startswith('/'):
+            next = auth.get_login_url()
         logout(request)
         request.user = AnonymousUser()
-        return self.redirect(rv)
+        return self.redirect(next)

tests/acceptance/test_accept_organization_invite.py

@@ -0,0 +1,29 @@
+from __future__ import absolute_import
+
+from sentry.testutils import AcceptanceTestCase
+
+
+class AcceptOrganizationInviteTest(AcceptanceTestCase):
+    def setUp(self):
+        super(AcceptOrganizationInviteTest, self).setUp()
+        self.user = self.create_user('foo@example.com')
+        self.org = self.create_organization(
+            name='Rowdy Tiger',
+            owner=None,
+        )
+        self.team = self.create_team(
+            organization=self.org,
+            name='Mariachi Band'
+        )
+        self.member = self.create_member(
+            user=None,
+            email='bar@example.com',
+            organization=self.org,
+            role='owner',
+            teams=[self.team],
+        )
+        self.login_as(self.user)
+
+    def test_invite(self):
+        self.browser.get(self.member.get_invite_link().split('/', 3)[-1])
+        self.browser.snapshot(name='accept organization invite')

tests/sentry/web/frontend/test_auth_logout.py

@@ -2,6 +2,7 @@ from __future__ import absolute_import
 
 from django.core.urlresolvers import reverse
 from exam import fixture
+from six.moves.urllib.parse import quote
 
 from sentry.testutils import TestCase
 
@@ -22,3 +23,20 @@ class AuthLogoutTest(TestCase):
         resp = self.client.get(self.path)
         assert resp.status_code == 302
         assert list(self.client.session.keys()) == []
+
+    def test_redirects_to_relative_next_url(self):
+        self.login_as(self.user)
+
+        next = '/welcome'
+        resp = self.client.get(self.path + '?next=' + next)
+        assert resp.status_code == 302
+        assert resp.get('Location', '').endswith(next)
+
+    def test_doesnt_redirect_to_external_next_url(self):
+        next = "http://example.com"
+        self.client.get(self.path + '?next=' + quote(next))
+
+        resp = self.client.get(self.path)
+        assert resp.status_code == 302
+        assert next not in resp['Location']
+        assert resp['Location'] == 'http://testserver/auth/login/'