fix(auth): Fix authentication flow when SSO is required (#20061)

src/sentry/api/endpoints/accept_organization_invite.py

@@ -45,6 +45,7 @@ class AcceptOrganizationInvite(Endpoint):
             "needsAuthentication": not helper.user_authenticated,
             "needs2fa": helper.needs_2fa,
             "needsSso": auth_provider is not None,
+            "requireSso": auth_provider is not None and not auth_provider.flags.allow_unlinked,
             # 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.

src/sentry/api/invite_helper.py

@@ -6,7 +6,13 @@ from django.core.urlresolvers import reverse
 
 from sentry.utils import metrics
 from sentry.utils.audit import create_audit_entry
-from sentry.models import AuditLogEntryEvent, Authenticator, OrganizationMember
+from sentry.models import (
+    AuditLogEntryEvent,
+    Authenticator,
+    AuthIdentity,
+    AuthProvider,
+    OrganizationMember,
+)
 from sentry.signals import member_joined
 
 INVITE_COOKIE = "pending-invite"
@@ -108,6 +114,13 @@ class ApiInviteHelper(object):
                 extra={"organization_id": self.om.organization.id, "user_id": self.request.user.id},
             )
 
+    def handle_member_has_no_sso(self):
+        if self.logger:
+            self.logger.info(
+                "Pending org invite not accepted - User did not have SSO",
+                extra={"organization_id": self.om.organization.id, "user_id": self.request.user.id},
+            )
+
     def handle_invite_not_approved(self):
         if not self.invite_approved:
             self.om.delete()
@@ -170,19 +183,32 @@ class ApiInviteHelper(object):
         if self.member_already_exists:
             self.handle_member_already_exists()
             om.delete()
-        else:
-            om.set_user(user)
-            om.save()
-
-            create_audit_entry(
-                self.request,
-                actor=user,
-                organization=om.organization,
-                target_object=om.id,
-                target_user=user,
-                event=AuditLogEntryEvent.MEMBER_ACCEPT,
-                data=om.get_audit_log_data(),
-            )
+            return
+
+        try:
+            provider = AuthProvider.objects.get(organization=om.organization)
+        except AuthProvider.DoesNotExist:
+            provider = None
+
+        # If SSO is required, check for valid AuthIdentity
+        if provider and not provider.flags.allow_unlinked:
+            # AuthIdentity has a unique constraint on provider and user
+            if not AuthIdentity.objects.filter(auth_provider=provider, user=user).exists():
+                self.handle_member_has_no_sso()
+                return
+
+        om.set_user(user)
+        om.save()
+
+        create_audit_entry(
+            self.request,
+            actor=user,
+            organization=om.organization,
+            target_object=om.id,
+            target_user=user,
+            event=AuditLogEntryEvent.MEMBER_ACCEPT,
+            data=om.get_audit_log_data(),
+        )
 
-            self.handle_success()
-            metrics.incr("organization.invite-accepted", sample_rate=1.0)
+        self.handle_success()
+        metrics.incr("organization.invite-accepted", sample_rate=1.0)

src/sentry/static/sentry/app/views/acceptOrganizationInvite.tsx

@@ -21,6 +21,7 @@ type InviteDetails = {
   needsAuthentication: boolean;
   needs2fa: boolean;
   needsSso: boolean;
+  requireSso: boolean;
   existingMember: boolean;
   ssoProvider?: string;
 };
@@ -95,47 +96,69 @@ class AcceptOrganizationInvite extends AsyncView<Props, State> {
 
     return (
       <React.Fragment>
-        <p>
-          {t(
-            `To continue, you must either login to an existing Sentry account,
-             or create a new account.`
-          )}
-        </p>
+        {!inviteDetails.requireSso && (
+          <p data-test-id="action-info-general">
+            {t(
+              `To continue, you must either create a new account, or login to an
+              existing Sentry account.`
+            )}
+          </p>
+        )}
+
         {inviteDetails.needsSso && (
-          <p data-test-id="suggests-sso">
-            {tct(
-              `Note that [orgSlug] has enabled Single-Sign-On (SSO) using
+          <p data-test-id="action-info-sso">
+            {inviteDetails.requireSso
+              ? tct(
+                  `Note that [orgSlug] has required Single Sign-On (SSO) using
                [authProvider]. You may create an account by authenticating with
-               the organizations SSO provider.`,
-              {
-                orgSlug: inviteDetails.orgSlug,
-                authProvider: inviteDetails.ssoProvider,
-              }
-            )}
+               the organization's SSO provider.`,
+                  {
+                    orgSlug: <strong>{inviteDetails.orgSlug}</strong>,
+                    authProvider: inviteDetails.ssoProvider,
+                  }
+                )
+              : tct(
+                  `Note that [orgSlug] has enabled Single Sign-On (SSO) using
+               [authProvider]. You may create an account by authenticating with
+               the organization's SSO provider.`,
+                  {
+                    orgSlug: <strong>{inviteDetails.orgSlug}</strong>,
+                    authProvider: inviteDetails.ssoProvider,
+                  }
+                )}
           </p>
         )}
 
         <Actions>
-          {inviteDetails.needsSso ? (
-            <Button
-              label="sso-login"
-              priority="primary"
-              href={this.makeNextUrl(`/auth/login/${inviteDetails.orgSlug}/`)}
-            >
-              {t('Join with %s', inviteDetails.ssoProvider)}
-            </Button>
-          ) : (
-            <Button
-              label="create-account"
-              priority="primary"
-              href={this.makeNextUrl('/auth/register/')}
+          <ActionsLeft>
+            {inviteDetails.needsSso && (
+              <Button
+                label="sso-login"
+                priority="primary"
+                href={this.makeNextUrl(`/auth/login/${inviteDetails.orgSlug}/`)}
+              >
+                {t('Join with %s', inviteDetails.ssoProvider)}
+              </Button>
+            )}
+            {!inviteDetails.requireSso && (
+              <Button
+                label="create-account"
+                priority="primary"
+                href={this.makeNextUrl('/auth/register/')}
+              >
+                {t('Create a new account')}
+              </Button>
+            )}
+          </ActionsLeft>
+          {!inviteDetails.requireSso && (
+            <ExternalLink
+              href={this.makeNextUrl('/auth/login/')}
+              openInNewTab={false}
+              data-test-id="link-with-existing"
             >
-              {t('Create a new account')}
-            </Button>
+              {t('Login using an existing account')}
+            </ExternalLink>
           )}
-          <ExternalLink href={this.makeNextUrl('/auth/login/')} openInNewTab={false}>
-            {t('Login using an existing account')}
-          </ExternalLink>
         </Actions>
       </React.Fragment>
     );
@@ -222,6 +245,11 @@ const Actions = styled('div')`
   justify-content: space-between;
   margin-bottom: ${space(3)};
 `;
+const ActionsLeft = styled('span')`
+  > a {
+    margin-right: ${space(1)};
+  }
+`;
 
 const InviteDescription = styled('p')`
   font-size: 1.2em;

tests/acceptance/test_accept_organization_invite.py

@@ -48,5 +48,5 @@ class AcceptOrganizationInviteTest(AcceptanceTestCase):
         AuthProvider.objects.create(organization=self.org, provider="google")
         self.browser.get(self.member.get_invite_link().split("/", 3)[-1])
         self.browser.wait_until('[data-test-id="accept-invite"]')
-        assert self.browser.element_exists_by_test_id("suggests-sso")
+        assert self.browser.element_exists_by_test_id("action-info-sso")
         assert self.browser.element_exists('[aria-label="sso-login"]')

tests/js/spec/views/acceptOrganizationInvite.spec.jsx

@@ -22,6 +22,7 @@ describe('AcceptOrganizationInvite', function() {
       needsAuthentication: false,
       needs2fa: false,
       needsSso: false,
+      requireSso: false,
       existingMember: false,
     });
 
@@ -54,6 +55,7 @@ describe('AcceptOrganizationInvite', function() {
       needsAuthentication: true,
       needs2fa: false,
       needsSso: false,
+      requireSso: false,
       existingMember: false,
     });
 
@@ -65,7 +67,12 @@ describe('AcceptOrganizationInvite', function() {
     const joinButton = wrapper.find('Button[label="join-organization"]');
     expect(joinButton.exists()).toBe(false);
 
+    expect(wrapper.find('[data-test-id="action-info-general"]').exists()).toBe(true);
+    expect(wrapper.find('[data-test-id="action-info-sso"]').exists()).toBe(false);
+
+    expect(wrapper.find('Button[label="sso-login"]').exists()).toBe(false);
     expect(wrapper.find('Button[label="create-account"]').exists()).toBe(true);
+    expect(wrapper.find('[data-test-id="link-with-existing"]').exists()).toBe(true);
   });
 
   it('suggests sso authentication to login', function() {
@@ -74,6 +81,7 @@ describe('AcceptOrganizationInvite', function() {
       needsAuthentication: true,
       needs2fa: false,
       needsSso: true,
+      requireSso: false,
       existingMember: false,
     });
 
@@ -85,10 +93,38 @@ describe('AcceptOrganizationInvite', function() {
     const joinButton = wrapper.find('Button[label="join-organization"]');
     expect(joinButton.exists()).toBe(false);
 
-    expect(wrapper.find('Button[label="create-account"]').exists()).toBe(false);
+    expect(wrapper.find('[data-test-id="action-info-general"]').exists()).toBe(true);
+    expect(wrapper.find('[data-test-id="action-info-sso"]').exists()).toBe(true);
+
+    expect(wrapper.find('Button[label="sso-login"]').exists()).toBe(true);
+    expect(wrapper.find('Button[label="create-account"]').exists()).toBe(true);
+    expect(wrapper.find('[data-test-id="link-with-existing"]').exists()).toBe(true);
+  });
+
+  it('enforce required sso authentication', function() {
+    addMock({
+      orgSlug: 'test-org',
+      needsAuthentication: true,
+      needs2fa: false,
+      needsSso: true,
+      requireSso: true,
+      existingMember: false,
+    });
+
+    const wrapper = mountWithTheme(
+      <AcceptOrganizationInvite params={{memberId: '1', token: 'abc'}} />,
+      TestStubs.routerContext()
+    );
+
+    const joinButton = wrapper.find('Button[label="join-organization"]');
+    expect(joinButton.exists()).toBe(false);
+
+    expect(wrapper.find('[data-test-id="action-info-general"]').exists()).toBe(false);
+    expect(wrapper.find('[data-test-id="action-info-sso"]').exists()).toBe(true);
 
-    expect(wrapper.find('[data-test-id="suggests-sso"]').exists()).toBe(true);
     expect(wrapper.find('Button[label="sso-login"]').exists()).toBe(true);
+    expect(wrapper.find('Button[label="create-account"]').exists()).toBe(false);
+    expect(wrapper.find('[data-test-id="link-with-existing"]').exists()).toBe(false);
   });
 
   it('shows a logout button for existing members', async function() {
@@ -97,6 +133,7 @@ describe('AcceptOrganizationInvite', function() {
       needsAuthentication: false,
       needs2fa: false,
       needsSso: false,
+      requireSso: false,
       existingMember: true,
     });
 
@@ -128,6 +165,7 @@ describe('AcceptOrganizationInvite', function() {
       needsAuthentication: false,
       needs2fa: true,
       needsSso: false,
+      requireSso: false,
       existingMember: false,
     });
 

tests/sentry/api/test_invite_helper.py

@@ -0,0 +1,92 @@
+from __future__ import absolute_import
+
+from django.http import HttpRequest
+
+from sentry.api.invite_helper import ApiInviteHelper
+from sentry.models import AuthProvider, OrganizationMember
+from sentry.testutils import TestCase
+from sentry.utils.compat.mock import patch
+
+
+class ApiInviteHelperTest(TestCase):
+    def setUp(self):
+        super(ApiInviteHelperTest, self).setUp()
+        self.org = self.create_organization(name="Rowdy Tiger", owner=None)
+        self.team = self.create_team(organization=self.org, name="Mariachi Band")
+        self.user = self.create_user("foo@example.com")
+        self.member = self.create_member(
+            user=None, email="bar@example.com", organization=self.org, teams=[self.team],
+        )
+        self.auth_provider = AuthProvider(provider="Friendly IdP", organization=self.organization)
+
+        self.request = HttpRequest()
+        self.request.user = self.user
+
+    @patch("sentry.api.invite_helper.create_audit_entry")
+    @patch("sentry.api.invite_helper.OrganizationMember.get_audit_log_data")
+    def test_accept_invite(self, get_audit, create_audit):
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email == self.member.email
+
+        helper = ApiInviteHelper(self.request, self.member.id, None)
+        helper.accept_invite()
+
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email is None
+        assert om.user.id == self.user.id
+
+    @patch("sentry.api.invite_helper.create_audit_entry")
+    @patch("sentry.api.invite_helper.OrganizationMember.get_audit_log_data")
+    @patch("sentry.api.invite_helper.AuthProvider.objects")
+    def test_accept_invite_with_SSO(self, mock_provider, get_audit, create_audit):
+        self.auth_provider.flags.allow_unlinked = True
+        mock_provider.get.return_value = self.auth_provider
+
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email == self.member.email
+
+        helper = ApiInviteHelper(self.request, self.member.id, None)
+        helper.accept_invite()
+
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email is None
+        assert om.user.id == self.user.id
+
+    @patch("sentry.api.invite_helper.create_audit_entry")
+    @patch("sentry.api.invite_helper.OrganizationMember.get_audit_log_data")
+    @patch("sentry.api.invite_helper.AuthProvider.objects")
+    def test_accept_invite_with_required_SSO(self, mock_provider, get_audit, create_audit):
+        self.auth_provider.flags.allow_unlinked = False
+        mock_provider.get.return_value = self.auth_provider
+
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email == self.member.email
+
+        helper = ApiInviteHelper(self.request, self.member.id, None)
+        helper.accept_invite()
+
+        # Invite cannot be accepted without AuthIdentity if SSO is required
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email is not None
+        assert om.user is None
+
+    @patch("sentry.api.invite_helper.create_audit_entry")
+    @patch("sentry.api.invite_helper.OrganizationMember.get_audit_log_data")
+    @patch("sentry.api.invite_helper.AuthProvider.objects")
+    @patch("sentry.api.invite_helper.AuthIdentity.objects")
+    def test_accept_invite_with_required_SSO_with_identity(
+        self, mock_identity, mock_provider, get_audit, create_audit
+    ):
+        self.auth_provider.flags.allow_unlinked = False
+        mock_provider.get.return_value = self.auth_provider
+        mock_identity.exists.return_value = True
+
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email == self.member.email
+
+        helper = ApiInviteHelper(self.request, self.member.id, None)
+        helper.accept_invite()
+
+        om = OrganizationMember.objects.get(id=self.member.id)
+        assert om.email is None
+        assert om.user.id == self.user.id