oauth: overhaul authorization flow

This fixes some behavior if you're not logged in, and ensures you're always directed back to the oauth page. It also avoids an extra redirect from OAuth for login, to avoid conflicting with the standard flow.

examples/README.md

@@ -0,0 +1,10 @@
+## Webserver
+
+1. Setup an API application
+2. Add ``http://127.0.0.1:5000/authorized`` as an Authorized Redirect URI
+3. Launch the service:
+
+    BASE_URL=http://dev.getsentry.net:8000 \
+    CLIENT_ID=XXX \
+    CLIENT_SECRET=XXX \
+    python app.py

examples/oauth2_consumer_webserver/app.py

@@ -7,8 +7,7 @@ import six
 from flask import Flask, redirect, url_for, request, session
 from flask_oauth import OAuth
 
-
-BASE_URL = os.environ.get('BASE_URL', 'http://localhost:8000')
+BASE_URL = os.environ.get('BASE_URL', 'http://dev.getsentry.net:8000')
 CLIENT_ID = os.environ.get('CLIENT_ID')
 CLIENT_SECRET = os.environ.get('CLIENT_SECRET')
 REDIRECT_URI = '/authorized'
@@ -44,17 +43,14 @@ sentry = oauth.remote_app(
 def index():
     access_token = session.get('access_token')
     if access_token is None:
-        return (
-            '<h1>Who are you?</h1>'
-            '<p><a href="{}">Login with Sentry</a></p>'
-        ).format(
-            url_for('login'),
-        )
+        return ('<h1>Who are you?</h1>'
+                '<p><a href="{}">Login with Sentry</a></p>').format(
+                    url_for('login'),
+                )
 
     from urllib2 import Request, urlopen, URLError
     headers = {'Authorization': 'Bearer {}'.format(access_token)}
-    req = Request('{}/api/0/organizations/'.format(BASE_URL),
-                  None, headers)
+    req = Request('{}/api/0/organizations/'.format(BASE_URL), None, headers)
     try:
         res = urlopen(req)
     except URLError, e:
@@ -64,13 +60,11 @@ def index():
             return redirect(url_for('login'))
         return '{}\n{}'.format(six.text_type(e), e.read())
 
-    return (
-        '<h1>Hi, {}!</h1>'
-        '<pre>{}</pre>'
-    ).format(
-        json.loads(session['user'])['email'],
-        json.dumps(json.loads(res.read()), indent=2),
-    )
+    return ('<h1>Hi, {}!</h1>'
+            '<pre>{}</pre>').format(
+                json.loads(session['user'])['email'],
+                json.dumps(json.loads(res.read()), indent=2),
+            )
 
 
 @app.route('/login')
@@ -83,14 +77,12 @@ def login():
 @sentry.authorized_handler
 def authorized(resp):
     if 'error' in request.args:
-        return (
-            '<h1>Error</h1>'
-            '<p>{}</p>'
-            '<p><a href="{}">Try again</a></p>'
-        ).format(
-            request.args['error'],
-            url_for('login'),
-        )
+        return ('<h1>Error</h1>'
+                '<p>{}</p>'
+                '<p><a href="{}">Try again</a></p>').format(
+                    request.args['error'],
+                    url_for('login'),
+                )
     access_token = resp['access_token']
     session['access_token'] = access_token
     session['user'] = json.dumps(resp['user'])

setup.cfg

@@ -9,7 +9,7 @@ phantomjs_path = node_modules/phantomjs-prebuilt/bin/phantomjs
 [flake8]
 ignore = F999,E501,E128,E124,E402,W503,E731,C901
 max-line-length = 100
-exclude = .git,*/south_migrations/*,node_modules/*,src/sentry/static/sentry/vendor/*,docs/*,src/south/*
+exclude = .git,*/south_migrations/*,node_modules/*,src/sentry/static/sentry/vendor/*,docs/*,src/south/*,examples/*
 
 [bdist_wheel]
 python-tag = py27

src/sentry/templates/sentry/bases/modal.html

@@ -15,7 +15,7 @@
         {% block modal_header_signout %}
           {% if request.user.is_authenticated %}
             <div class="pull-right">
-              <a href="{% url 'sentry-logout' %}">{% trans "Sign out" %}</a>
+              <a href="{% url 'sentry-logout' %}?next={{ request.get_full_path|urlencode }}">{% trans "Sign out" %}</a>
             </div>
           {% endif %}
         {% endblock %}

src/sentry/templates/sentry/login.html

@@ -7,6 +7,10 @@
 {% block title %}{% trans "Login" %} | {{ block.super }}{% endblock %}
 
 {% block auth_main %}
+  {% if banner %}
+    <h4>{{ banner }}</h4>
+  {% endif %}
+
   <ul class="nav nav-tabs auth-toggle border-bottom">
     <li{% if op == "login" %} class="active"{% endif %}>
       <a href="#login" data-toggle="tab">{% trans "Login" %}</a>
@@ -35,7 +39,7 @@
         {% endfor %}
 
         <fieldset class="form-actions">
-          <button type="submit" class="btn btn-primary">{% trans "Login" %}</button> <a class="pull-right" style="margin-top: 7px" href="{% url 'sentry-account-recover' %}">{% trans "Lost your password?" %}</a>
+          <button type="submit" class="btn btn-primary">{% trans "Continue" %}</button> <a class="pull-right" style="margin-top: 7px" href="{% url 'sentry-account-recover' %}">{% trans "Lost your password?" %}</a>
         </fieldset>
       </form>
     </div>
@@ -52,7 +56,7 @@
         {% endfor %}
 
         <fieldset class="form-actions">
-          <button type="submit" class="btn btn-primary">{% trans "Register" %}</button>
+          <button type="submit" class="btn btn-primary">{% trans "Continue" %}</button>
         </fieldset>
       </form>
     </div>
@@ -64,13 +68,14 @@
 
         <div class="control-group">
           <div class="controls">
-            <label style="display: block">SSO URL</label>
-            <span style="padding: 7px 0; font-size: 15px">{{ server_hostname }}/</span> <input style="width: 240px; margin: 0 0 4px; padding: 5px 8px; display: inline" type="text" class="form-control" name="organization" placeholder="acme">
+            <label style="display: block">{% trans "Organization ID" %}*</label>
+            <input type="text" class="form-control" name="organization" placeholder="acme" required>
             <p class="help-block">Enter your organization's ID and we'll get things started.</p>
+            <p class="help-block">Your ID is the reference used when Sentry generates URLs.<br />e.g. <code>{{ server_hostname }}/<strong>acme</strong>/</code></p>
           </div>
         </div>
         <div class="form-actions" style="margin-top: 25px">
-          <button class="btn btn-primary">Sign in</button>
+          <button class="btn btn-primary">{% trans "Continue" %}</button>
         </div>
       </form>
     </div>

src/sentry/utils/auth.py

@@ -91,24 +91,15 @@ def get_login_url(reset=False):
 
 
 def initiate_login(request, next_url=None):
-    try:
-        del request.session['_after_2fa']
-    except KeyError:
-        pass
-
-    try:
-        del request.session['_pending_2fa']
-    except KeyError:
-        pass
-
-    if next_url:
-        request.session['_next'] = next_url
-    else:
+    for key in ('_next', '_after_2fa', '_pending_2fa'):
         try:
-            del request.session['_next']
+            del request.session[key]
         except KeyError:
             pass
 
+    if next_url:
+        request.session['_next'] = next_url
+
 
 def get_login_redirect(request, default=None):
     if default is None:

src/sentry/web/frontend/auth_login.py

@@ -52,8 +52,17 @@ class AuthLoginView(BaseView):
             initial=initial,
         )
 
-    def handle_basic_auth(self, request):
-        can_register = auth.has_user_registration() or request.session.get('can_register')
+    def can_register(self, request, *args, **kwargs):
+        return auth.has_user_registration() or request.session.get('can_register')
+
+    def get_next_uri(self, request, *args, **kwargs):
+        return request.GET.get(REDIRECT_FIELD_NAME, None)
+
+    def respond_login(self, request, context, *args, **kwargs):
+        return self.respond('sentry/login.html', context)
+
+    def handle_basic_auth(self, request, *args, **kwargs):
+        can_register = self.can_register(request, *args, **kwargs)
 
         op = request.POST.get('op')
 
@@ -118,9 +127,9 @@ class AuthLoginView(BaseView):
             'register_form': register_form,
             'CAN_REGISTER': can_register,
         }
-        return self.respond('sentry/login.html', context)
+        return self.respond_login(request, context, *args, **kwargs)
 
-    def handle_sso(self, request):
+    def handle_sso(self, request, *args, **kwargs):
         org = request.POST.get('organization')
         if not org:
             return HttpResponseRedirect(request.path)
@@ -134,19 +143,27 @@ class AuthLoginView(BaseView):
 
         return HttpResponseRedirect(next_uri)
 
+    def handle_authenticated(self, request, *args, **kwargs):
+        next_uri = self.get_next_uri(request, *args, **kwargs)
+        if auth.is_valid_redirect(next_uri, host=request.get_host()):
+            return self.redirect(next_uri)
+        return self.redirect_to_org(request)
+
     @never_cache
     @transaction.atomic
-    def handle(self, request):
-        next_uri = request.GET.get(REDIRECT_FIELD_NAME, None)
+    def handle(self, request, *args, **kwargs):
+        return super(AuthLoginView, self).handle(request, *args, **kwargs)
+
+    # XXX(dcramer): OAuth provider hooks this view
+    def get(self, request, *args, **kwargs):
+        next_uri = self.get_next_uri(request, *args, **kwargs)
         if request.user.is_authenticated():
-            if auth.is_valid_redirect(next_uri, host=request.get_host()):
-                return self.redirect(next_uri)
-            return self.redirect_to_org(request)
+            return self.handle_authenticated(request, *args, **kwargs)
 
         request.session.set_test_cookie()
 
-        if next_uri:
-            auth.initiate_login(request, next_uri)
+        # we always reset the state on GET so you dont end up at an odd location
+        auth.initiate_login(request, next_uri)
 
         # Single org mode -- send them to the org-specific handler
         if settings.SENTRY_SINGLE_ORGANIZATION:
@@ -154,24 +171,28 @@ class AuthLoginView(BaseView):
             next_uri = reverse('sentry-auth-organization', args=[org.slug])
             return HttpResponseRedirect(next_uri)
 
+        session_expired = 'session_expired' in request.COOKIES
+        if session_expired:
+            messages.add_message(request, messages.WARNING, WARN_SESSION_EXPIRED)
+
+        response = self.handle_basic_auth(request, *args, **kwargs)
+
+        if session_expired:
+            response.delete_cookie('session_expired')
+
+        return response
+
+    # XXX(dcramer): OAuth provider hooks this view
+    def post(self, request, *args, **kwargs):
         op = request.POST.get('op')
         if op == 'sso' and request.POST.get('organization'):
             auth_provider = self.get_auth_provider(request.POST['organization'])
             if auth_provider:
                 next_uri = reverse('sentry-auth-organization', args=[request.POST['organization']])
             else:
-                next_uri = request.path
+                next_uri = request.get_full_path()
                 messages.add_message(request, messages.ERROR, ERR_NO_SSO)
 
             return HttpResponseRedirect(next_uri)
 
-        session_expired = 'session_expired' in request.COOKIES
-        if session_expired:
-            messages.add_message(request, messages.WARNING, WARN_SESSION_EXPIRED)
-
-        response = self.handle_basic_auth(request)
-
-        if session_expired:
-            response.delete_cookie('session_expired')
-
-        return response
+        return self.handle_basic_auth(request, *args, **kwargs)

src/sentry/web/frontend/oauth_authorize.py

@@ -6,20 +6,19 @@ from django.conf import settings
 from django.db import IntegrityError, transaction
 from django.utils import timezone
 from django.utils.safestring import mark_safe
-from django.views.decorators.cache import never_cache
 from six.moves.urllib.parse import parse_qsl, urlencode, urlparse, urlunparse
 
 from sentry.models import (
     ApiApplication, ApiApplicationStatus, ApiAuthorization, ApiGrant, ApiToken
 )
-from sentry.web.frontend.base import BaseView
+from sentry.web.frontend.auth_login import AuthLoginView
 
 
-class OAuthAuthorizeView(BaseView):
-    @never_cache
-    def dispatch(self, request, *args, **kwargs):
-        with transaction.atomic():
-            return super(OAuthAuthorizeView, self).dispatch(request, *args, **kwargs)
+class OAuthAuthorizeView(AuthLoginView):
+    auth_required = False
+
+    def get_next_uri(self, request, *args, **kwargs):
+        return request.get_full_path()
 
     def redirect_response(self, response_type, redirect_uri, params):
         if response_type == 'token':
@@ -46,6 +45,10 @@ class OAuthAuthorizeView(BaseView):
             }
         )
 
+    def respond_login(self, request, context, application):
+        context['banner'] = 'Connect Sentry to {}'.format(application.name)
+        return self.respond('sentry/login.html', context)
+
     def get(self, request):
         response_type = request.GET.get('response_type')
         client_id = request.GET.get('client_id')
@@ -103,6 +106,19 @@ class OAuthAuthorizeView(BaseView):
         else:
             scopes = []
 
+        payload = {
+            'rt': response_type,
+            'cid': client_id,
+            'ru': redirect_uri,
+            'sc': scopes,
+            'st': state,
+            'uid': request.user.id if request.user.is_authenticated() else '',
+        }
+        request.session['oa2'] = payload
+
+        if not request.user.is_authenticated():
+            return super(OAuthAuthorizeView, self).get(request, application)
+
         if not force_prompt:
             try:
                 existing_auth = ApiAuthorization.objects.get(
@@ -171,14 +187,6 @@ class OAuthAuthorizeView(BaseView):
                 }
             )
 
-        if payload['uid'] != request.user.id:
-            return self.respond(
-                'sentry/oauth-error.html', {
-                    'error':
-                    'We were unable to complete your request. Please re-initiate the authorization flow.',
-                }
-            )
-
         try:
             application = ApiApplication.objects.get(
                 client_id=payload['cid'],
@@ -191,6 +199,22 @@ class OAuthAuthorizeView(BaseView):
                 }
             )
 
+        if not request.user.is_authenticated():
+            response = super(OAuthAuthorizeView, self).post(request, application)
+            # once they login, bind their user ID
+            if request.user.is_authenticated():
+                request.session['oa2']['uid'] = request.user.id
+                request.session.modified = True
+            return response
+
+        if payload['uid'] != request.user.id:
+            return self.respond(
+                'sentry/oauth-error.html', {
+                    'error':
+                    'We were unable to complete your request. Please re-initiate the authorization flow.',
+                }
+            )
+
         response_type = payload['rt']
         redirect_uri = payload['ru']
         scopes = payload['sc']

tests/acceptance/test_auth.py

@@ -8,7 +8,7 @@ class AuthTest(AcceptanceTestCase):
         self.browser.get('/auth/login/')
         self.browser.find_element_by_id('id_username').send_keys(username)
         self.browser.find_element_by_id('id_password').send_keys(password)
-        self.browser.find_element_by_xpath("//button[contains(text(), 'Login')]").click()
+        self.browser.find_element_by_xpath("//button[contains(text(), 'Continue')]").click()
 
     def test_renders(self):
         self.browser.get('/auth/login/')

tests/sentry/web/frontend/test_oauth_authorize.py

@@ -270,6 +270,53 @@ class OAuthAuthorizeCodeTest(TestCase):
             'Read, write, and admin access to organization members.'
         ]
 
+    def test_unauthenticated_basic_auth(self):
+        full_path = '{}?response_type=code&client_id={}'.format(
+            self.path,
+            self.application.client_id,
+        )
+
+        resp = self.client.get(full_path)
+
+        assert resp.status_code == 200
+        self.assertTemplateUsed('sentry/login.html')
+        assert resp.context['banner'] == 'Connect Sentry to {}'.format(self.application.name)
+
+        resp = self.client.post(
+            full_path,
+            {
+                'username': self.user.username,
+                'password': 'admin',
+                'op': 'login',
+            },
+        )
+        assert resp.status_code == 302
+        assert resp.get('Location') == 'http://testserver{}'.format(full_path)
+
+        resp = self.client.get(full_path)
+        self.assertTemplateUsed('sentry/oauth-authorize.html')
+        assert resp.context['application'] == self.application
+
+        resp = self.client.post(full_path, {
+            'op': 'approve',
+        })
+
+        grant = ApiGrant.objects.get(user=self.user)
+        assert grant.redirect_uri == self.application.get_default_redirect_uri()
+        assert grant.application == self.application
+        assert not grant.get_scopes()
+
+        assert resp.status_code == 302
+        assert resp['Location'] == 'https://example.com?code={}'.format(
+            grant.code,
+        )
+
+        authorization = ApiAuthorization.objects.get(
+            user=self.user,
+            application=self.application,
+        )
+        assert authorization.get_scopes() == grant.get_scopes()
+
 
 class OAuthAuthorizeTokenTest(TestCase):
     @fixture