[sso] require per-organization validation (#4432)

* [sso] require per-organization validation

This expands on SSO security for accounts which span multiple organizations or existed before SSO. It changes the requirements so that to access an SSO-only organization, you now **must** authenticate against that organization.

For superusers this is still special cased, in that if you're part of an organization and it requires it, you must also do it. If you're not a member of the org you can bypass it.

* Automatically handle 2fa behavior for follow-on auth

* Update test for new behavior

* bad api call

CHANGES

@@ -10,6 +10,7 @@ Version 8.10 (Unreleased)
 - ``SENTRY_FILESTORE_OPTIONS`` deprecated and replaced with ``filestore.options``
 - Add watchOS support for cocoa interface.
 - Fix support for internationalized Origins and raven-js.
+- SSO is now enforced to access data within any org that has it set as a requirement.
 
 API Changes
 ~~~~~~~~~~~

src/sentry/auth/access.py

@@ -9,9 +9,41 @@ from django.conf import settings
 from sentry.models import AuthIdentity, AuthProvider, OrganizationMember
 
 
+def _sso_params(member):
+    """
+    Return a tuple of (requires_sso, sso_is_valid) for a given member.
+    """
+    # TODO(dcramer): we want to optimize this access pattern as its several
+    # network hops and needed in a lot of places
+    try:
+        auth_provider = AuthProvider.objects.get(
+            organization=member.organization_id,
+        )
+    except AuthProvider.DoesNotExist:
+        sso_is_valid = True
+        requires_sso = False
+    else:
+        if auth_provider.flags.allow_unlinked:
+            requires_sso = False
+            sso_is_valid = True
+        else:
+            requires_sso = True
+            try:
+                auth_identity = AuthIdentity.objects.get(
+                    auth_provider=auth_provider,
+                    user=member.user_id,
+                )
+            except AuthIdentity.DoesNotExist:
+                sso_is_valid = False
+            else:
+                sso_is_valid = auth_identity.is_valid(member)
+    return requires_sso, sso_is_valid
+
+
 class BaseAccess(object):
     is_active = False
     sso_is_valid = False
+    requires_sso = False
     # teams with valid access
     teams = ()
     # teams with valid membership
@@ -52,13 +84,15 @@ class Access(BaseAccess):
     # TODO(dcramer): this is still a little gross, and ideally backend access
     # would be based on the same scopes as API access so theres clarity in
     # what things mean
-    def __init__(self, scopes, is_active, teams, memberships, sso_is_valid):
+    def __init__(self, scopes, is_active, teams, memberships, sso_is_valid,
+                 requires_sso):
         self.teams = teams
         self.memberships = memberships
         self.scopes = scopes
 
         self.is_active = is_active
         self.sso_is_valid = sso_is_valid
+        self.requires_sso = requires_sso
 
 
 def from_request(request, organization, scopes=None):
@@ -66,13 +100,26 @@ def from_request(request, organization, scopes=None):
         return DEFAULT
 
     if request.is_superuser():
+        # we special case superuser so that if they're a member of the org
+        # they must still follow SSO checks, but they gain global access
+        try:
+            member = OrganizationMember.objects.get(
+                user=request.user,
+                organization=organization,
+            )
+        except OrganizationMember.DoesNotExist:
+            requires_sso, sso_is_valid = False, True
+        else:
+            requires_sso, sso_is_valid = _sso_params(member)
+
         team_list = list(organization.team_set.all())
         return Access(
             scopes=scopes if scopes is not None else settings.SENTRY_SCOPES,
             is_active=True,
             teams=team_list,
             memberships=team_list,
-            sso_is_valid=True,
+            sso_is_valid=sso_is_valid,
+            requires_sso=requires_sso,
         )
     return from_user(request.user, organization, scopes=scopes)
 
@@ -101,25 +148,7 @@ def from_user(user, organization, scopes=None):
 def from_member(member, scopes=None):
     # TODO(dcramer): we want to optimize this access pattern as its several
     # network hops and needed in a lot of places
-    try:
-        auth_provider = AuthProvider.objects.get(
-            organization=member.organization_id,
-        )
-    except AuthProvider.DoesNotExist:
-        sso_is_valid = True
-    else:
-        if auth_provider.flags.allow_unlinked:
-            sso_is_valid = True
-        else:
-            try:
-                auth_identity = AuthIdentity.objects.get(
-                    auth_provider=auth_provider,
-                    user=member.user_id,
-                )
-            except AuthIdentity.DoesNotExist:
-                sso_is_valid = False
-            else:
-                sso_is_valid = auth_identity.is_valid(member)
+    requires_sso, sso_is_valid = _sso_params(member)
 
     team_memberships = member.get_teams()
     if member.organization.flags.allow_joinleave:
@@ -134,6 +163,7 @@ def from_member(member, scopes=None):
 
     return Access(
         is_active=True,
+        requires_sso=requires_sso,
         sso_is_valid=sso_is_valid,
         scopes=scopes,
         memberships=team_memberships,
@@ -142,24 +172,11 @@ def from_member(member, scopes=None):
 
 
 class NoAccess(BaseAccess):
-    @property
-    def sso_is_valid(self):
-        return True
-
-    @property
-    def is_active(self):
-        return False
-
-    @property
-    def teams(self):
-        return ()
-
-    @property
-    def memberships(self):
-        return ()
-
-    @property
-    def scopes(self):
-        return frozenset()
+    requires_sso = False
+    sso_is_valid = True
+    is_active = False
+    teams = ()
+    memberships = ()
+    scopes = frozenset()
 
 DEFAULT = NoAccess()

src/sentry/auth/helper.py

@@ -430,7 +430,8 @@ class AuthHelper(object):
                 ).exists()
                 if has_membership:
                     if not auth.login(request, existing_user,
-                                      after_2fa=request.build_absolute_uri()):
+                                      after_2fa=request.build_absolute_uri(),
+                                      organization_id=self.organization.id):
                         return HttpResponseRedirect(auth.get_login_redirect(
                             self.request))
                     # assume they've confirmed they want to attach the identity

src/sentry/testutils/cases.py

@@ -42,6 +42,7 @@ from sentry.models import GroupMeta, ProjectOption
 from sentry.plugins import plugins
 from sentry.rules import EventState
 from sentry.utils import json
+from sentry.utils.auth import SSO_SESSION_KEY
 
 from .fixtures import Fixtures
 from .helpers import AuthProvider, Feature, get_auth_header, TaskRunner, override_options
@@ -103,7 +104,7 @@ class BaseTestCase(Fixtures, Exam):
         self.client.cookies[session_cookie] = self.session.session_key
         self.client.cookies[session_cookie].update(cookie_data)
 
-    def login_as(self, user):
+    def login_as(self, user, organization_id=None):
         user.backend = settings.AUTHENTICATION_BACKENDS[0]
 
         request = HttpRequest()
@@ -111,6 +112,8 @@ class BaseTestCase(Fixtures, Exam):
 
         login(request, user)
         request.user = user
+        if organization_id:
+            request.session[SSO_SESSION_KEY] = six.text_type(organization_id)
 
         # Save the session values.
         self.save_session()

src/sentry/utils/auth.py

@@ -22,6 +22,10 @@ logger = logging.getLogger('sentry.auth')
 
 _LOGIN_URL = None
 
+SSO_SESSION_KEY = 'sso'
+
+MFA_SESSION_KEY = 'mfa'
+
 
 class AuthUserPasswordExpired(Exception):
 
@@ -140,6 +144,17 @@ def is_valid_redirect(url):
     return True
 
 
+def mark_sso_complete(request, organization_id):
+    sso = request.session.get(SSO_SESSION_KEY, '').split(',')
+    sso.append(six.text_type(organization_id))
+    request.session[SSO_SESSION_KEY] = ','.join(sso)
+
+
+def has_completed_sso(request, organization_id):
+    sso = request.session.get(SSO_SESSION_KEY, '').split(',')
+    return six.text_type(organization_id) in sso
+
+
 def find_users(username, with_valid_password=True, is_active=None):
     """
     Return a list of users that match a username
@@ -165,22 +180,36 @@ def find_users(username, with_valid_password=True, is_active=None):
     return []
 
 
-def login(request, user, passed_2fa=False, after_2fa=None):
-    """This logs a user in for the sesion and current request.  If 2FA is
-    enabled this method will start the 2FA flow and return False, otherwise
-    it will return True.  If `passed_2fa` is set to `True` then the 2FA flow
-    is set to be finalized (user passed the flow).
+def login(request, user, passed_2fa=None, after_2fa=None,
+          organization_id=None):
+    """
+    This logs a user in for the sesion and current request.
+
+    If 2FA is enabled this method will start the MFA flow and return False as
+    required.  If `passed_2fa` is set to `True` then the 2FA flow is set to be
+    finalized (user passed the flow).
+
+    If the session has already resolved MFA in the past, it will automatically
+    detect it from the session.
 
     Optionally `after_2fa` can be set to a URL which will be used to override
     the regular session redirect target directly after the 2fa flow.
+
+    Returns boolean indicating if the user was logged in.
     """
     has_2fa = Authenticator.objects.user_has_2fa(user)
+    if passed_2fa is None:
+        passed_2fa = (
+            request.session.get(MFA_SESSION_KEY, '') == six.text_type(user.id)
+        )
+
     if has_2fa and not passed_2fa:
         request.session['_pending_2fa'] = [user.id, time()]
         if after_2fa is not None:
             request.session['_after_2fa'] = after_2fa
         return False
 
+    request.session[MFA_SESSION_KEY] = six.text_type(user.id)
     request.session.pop('_pending_2fa', None)
 
     # Check for expired passwords here after we cleared the 2fa flow.
@@ -200,6 +229,8 @@ def login(request, user, passed_2fa=False, after_2fa=None):
     if not hasattr(user, 'backend'):
         user.backend = settings.AUTHENTICATION_BACKENDS[0]
     _login(request, user)
+    if organization_id:
+        mark_sso_complete(request, organization_id)
     log_auth_success(request, user.username)
     return True
 

src/sentry/web/frontend/auth_organization_login.py

@@ -57,7 +57,7 @@ class AuthOrganizationLoginView(BaseView):
             # HACK: grab whatever the first backend is and assume it works
             user.backend = settings.AUTHENTICATION_BACKENDS[0]
 
-            auth.login(request, user)
+            auth.login(request, user, organization_id=organization.id)
 
             # can_register should only allow a single registration
             request.session.pop('can_register', None)

src/sentry/web/frontend/base.py

@@ -3,12 +3,10 @@ from __future__ import absolute_import
 import logging
 import six
 
-from django.contrib import messages
 from django.core.context_processors import csrf
 from django.core.urlresolvers import reverse
 from django.http import HttpResponseRedirect
 from django.utils.decorators import method_decorator
-from django.utils.translation import ugettext_lazy as _
 from django.views.decorators.csrf import csrf_protect
 from django.views.generic import View
 from sudo.views import redirect_to_sudo
@@ -22,8 +20,6 @@ from sentry.models import (
 from sentry.utils import auth
 from sentry.web.helpers import render_to_response
 
-ERR_MISSING_SSO_LINK = _('You need to link your account with the SSO provider to continue.')
-
 logger = logging.getLogger(__name__)
 audit_logger = logging.getLogger('sentry.audit.ui')
 
@@ -299,8 +295,11 @@ class OrganizationView(BaseView):
     def has_permission(self, request, organization, *args, **kwargs):
         if organization is None:
             return False
-        if self.valid_sso_required and not request.access.sso_is_valid:
-            return False
+        if self.valid_sso_required:
+            if not request.access.sso_is_valid:
+                return False
+            if self.needs_sso(request, organization):
+                return False
         if self.required_scope and not request.access.has_scope(self.required_scope):
             logger.info('User %s does not have %s permission to access organization %s',
                 request.user, self.required_scope, organization)
@@ -334,24 +333,34 @@ class OrganizationView(BaseView):
         return False
 
     def handle_permission_required(self, request, organization, *args, **kwargs):
-        needs_link = (
-            organization and request.user.is_authenticated()
-            and self.valid_sso_required and not request.access.sso_is_valid
-        )
-
-        auth.initiate_login(request, next_url=request.get_full_path())
-
-        if needs_link:
-            messages.add_message(
-                request, messages.ERROR,
-                ERR_MISSING_SSO_LINK,
-            )
+        if self.needs_sso(request, organization):
+            logger.info('access.must-sso', extra={
+                'organization_id': organization.id,
+                'user_id': request.user.id,
+            })
+            auth.initiate_login(request, next_url=request.get_full_path())
             redirect_uri = reverse('sentry-auth-organization',
                                    args=[organization.slug])
         else:
             redirect_uri = self.get_no_permission_url(request, *args, **kwargs)
         return self.redirect(redirect_uri)
 
+    def needs_sso(self, request, organization):
+        if not organization:
+            return False
+        # XXX(dcramer): this branch should really never hit
+        if not request.user.is_authenticated():
+            return False
+        if not self.valid_sso_required:
+            return False
+        if not request.access.requires_sso:
+            return False
+        if not auth.has_completed_sso(request, organization.id):
+            return True
+        if not request.access.sso_is_valid:
+            return True
+        return False
+
     def convert_args(self, request, organization_slug=None, *args, **kwargs):
         active_organization = self.get_active_organization(
             request=request,

tests/integration/test_sso.py

@@ -0,0 +1,53 @@
+from __future__ import absolute_import
+
+import six
+
+from sentry.models import AuthIdentity, AuthProvider
+from sentry.testutils import AuthProviderTestCase
+from sentry.utils.auth import SSO_SESSION_KEY
+
+
+# TODO(dcramer): this is an integration test
+class OrganizationAuthLoginTest(AuthProviderTestCase):
+    def test_sso_auth_required(self):
+        user = self.create_user('foo@example.com', is_superuser=False)
+        organization = self.create_organization(name='foo')
+        member = self.create_member(user=user, organization=organization)
+        setattr(member.flags, 'sso:linked', True)
+        member.save()
+
+        auth_provider = AuthProvider.objects.create(
+            organization=organization,
+            provider='dummy',
+            flags=0,
+        )
+
+        AuthIdentity.objects.create(
+            auth_provider=auth_provider,
+            user=user,
+        )
+
+        self.login_as(user)
+
+        path = '/{}/'.format(organization.slug)
+        redirect_uri = 'http://testserver/auth/login/{}/'.format(organization.slug)
+
+        # we should be redirecting the user to the authentication form as they
+        # haven't verified this specific organization
+        resp = self.client.get(path)
+        assert resp.status_code == 302
+        assert resp['Location'] == redirect_uri
+
+        # superuser should still require SSO as they're a member of the org
+        user.update(is_superuser=True)
+        resp = self.client.get(path)
+        assert resp.status_code == 302
+        assert resp['Location'] == redirect_uri
+
+        # XXX(dcramer): using internal API as exposing a request object is hard
+        self.session[SSO_SESSION_KEY] = six.text_type(organization.id)
+        self.save_session()
+
+        # now that SSO is marked as complete, we should be able to access dash
+        resp = self.client.get(path)
+        assert resp.status_code == 200

tests/sentry/web/frontend/test_organization_auth_settings.py

@@ -122,7 +122,7 @@ class OrganizationAuthSettingsTest(AuthProviderTestCase):
 
         path = reverse('sentry-organization-auth-settings', args=[organization.slug])
 
-        self.login_as(self.user)
+        self.login_as(self.user, organization_id=organization.id)
 
         with self.feature('organizations:sso'):
             resp = self.client.post(path, {'op': 'disable'})