auth: Let user choose emails in GitHub auth.

Previously, our Github authentication backend just used the user's
primary email address associated with GitHub, which was a reasonable
default, but quite annoying for users who have several email addresses
associated with their GitHub account.

We fix this, by adding a new screen where users can select which of
their (verified) GitHub email addresses to use for authentication.

This is implemented using the "partial" feature of the
python-social-auth pipeline system.

Each email is displayed as a button. Clicking on that button chooses
the email. The email value is stored in a hidden input above the
button. The `primary_email` is displayed on top followed by
`verified_non_primary_emails`. Backend name is also passed as
`backend` to the template, which in our case is GitHub.

Fixes #9876.

static/styles/portico/portico-signin.scss

@@ -1076,3 +1076,30 @@ button.login-google-button {
         color: hsl(165, 100.0%, 14.7%);
     }
 }
+
+#choose_email {
+    flex-direction: column;
+
+    form {
+        padding: 0;
+        margin: 0;
+    }
+
+    button {
+        color: hsl(0, 0%, 0%);
+        background-color: hsl(0, 0%, 100%);
+        width: 100%;
+        margin-top: 15px;
+        border: 1px solid hsl(0, 0%, 80%);
+        border-radius: 4px;
+    }
+
+    button:hover {
+        border-color: hsl(0, 0%, 0%);
+    }
+
+    button:active {
+        border-color: hsl(0, 0%, 60%);
+        background-color: hsl(0, 0%, 95%);
+    }
+}

templates/zerver/social_auth_select_email.html

@@ -0,0 +1,33 @@
+{% extends "zerver/portico_signup.html" %}
+
+{% block portico_content %}
+<div class="register-account flex full-page new-style" id="choose_email">
+
+    <div class="pitch">
+        {% trans %}
+        <h1>Select email</h1>
+        {% endtrans %}
+    </div>
+
+    <div>
+        <form method="post" class="form-horizontal" action="/complete/{{ backend }}/">
+            <div class='choose-email-box'>
+                <input type="hidden" name="email" value="{{ primary_email }}" />
+                <button type="submit" >
+                    {{ primary_email }}
+                </button>
+            </div>
+        </form>
+        {% for email in verified_non_primary_emails %}
+        <form method="post" class="form-horizontal" action="/complete/{{ backend }}/">
+            <div class='choose-email-box'>
+                <input type="hidden" name="email" value="{{ email }}" />
+                <button type="submit" >
+                    {{ email }}
+                </button>
+            </div>
+        </form>
+        {% endfor %}
+    </div>
+</div>
+{% endblock %}

zerver/tests/test_auth_backends.py

@@ -384,7 +384,7 @@ class AuthBackendTest(ZulipTestCase):
             dict(email=user.email,
                  verified=True,
                  primary=True),
-            dict(email="nonprimary@example.com",
+            dict(email="nonprimary@zulip.com",
                  verified=True),
             dict(email="ignored@example.com",
                  verified=False),
@@ -416,9 +416,22 @@ class AuthBackendTest(ZulipTestCase):
         }   # type: Dict[str, Any]
 
         def patched_authenticate(**kwargs: Any) -> Any:
+            # This is how we pass the subdomain to the authentication
+            # backend in production code, so we need to do this setup
+            # here.
             if 'subdomain' in kwargs:
                 backend.strategy.session_set("subdomain", kwargs["subdomain"])
                 del kwargs['subdomain']
+
+            # Because we're not simulating the full python-social-auth
+            # pipeline here, we need to provide the user's choice of
+            # which email to select in the partial phase of the
+            # pipeline when we display an email picker for the GitHub
+            # authentication backend.  We do that here.
+            def return_email() -> Dict[str, str]:
+                return {'email': user.email}
+            backend.strategy.request_data = return_email
+
             result = orig_authenticate(backend, **kwargs)
             return result
 
@@ -502,6 +515,7 @@ class SocialAuthBase(ZulipTestCase):
                          is_signup: Optional[str]=None,
                          next: str='',
                          multiuse_object_key: str='',
+                         expect_choose_email_screen: bool=False,
                          **extra_data: Any) -> HttpResponse:
         url = self.LOGIN_URL
         params = {}
@@ -563,6 +577,46 @@ class SocialAuthBase(ZulipTestCase):
         csrf_state = urllib.parse.parse_qs(parsed_url.query)['state']
         result = self.client_get(self.AUTH_FINISH_URL,
                                  dict(state=csrf_state), **headers)
+
+        if expect_choose_email_screen and result.status_code == 200:
+            # For authentication backends such as GitHub that
+            # successfully authenticate multiple email addresses,
+            # we'll have an additional screen where the user selects
+            # which email address to login using (this screen is a
+            # "partial" state of the python-social-auth pipeline).
+            #
+            # TODO: Generalize this testing code for use with other
+            # authentication backends; for now, we just assert that
+            # it's definitely the GitHub authentication backend.
+            self.assert_in_success_response(["Select email"], result)
+            assert self.AUTH_FINISH_URL == "/complete/github/"
+
+            # Testing hack: When the pipeline goes to the partial
+            # step, the below given URL is called again in the same
+            # test. If the below URL is not registered again as done
+            # below, the URL returns emails from previous tests. e.g
+            # email = 'invalid' may be one of the emails in the list
+            # in a test function followed by it. This is probably a
+            # bug in httpretty.
+            httpretty.disable()
+            httpretty.enable()
+            httpretty.register_uri(
+                httpretty.GET,
+                "https://api.github.com/user/emails",
+                status=200,
+                body=json.dumps(self.email_data)
+            )
+            result = self.client_get(self.AUTH_FINISH_URL,
+                                     dict(state=csrf_state, email=account_data_dict['email']), **headers)
+        elif self.AUTH_FINISH_URL == "/complete/github/":
+            # We want to be explicit about when we expect a test to
+            # use the "choose email" screen, but of course we should
+            # only check for that screen with the GitHub backend,
+            # because this test code is shared with other
+            # authentication backends that structurally will never use
+            # that screen.
+            assert not expect_choose_email_screen
+
         httpretty.disable()
         return result
 
@@ -577,6 +631,7 @@ class SocialAuthBase(ZulipTestCase):
     def test_social_auth_success(self) -> None:
         account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip', next='/user_uploads/image')
         data = load_subdomain_token(result)
         self.assertEqual(data['email'], self.example_email("hamlet"))
@@ -589,11 +644,64 @@ class SocialAuthBase(ZulipTestCase):
                                  parsed_url.path)
         self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/'))
 
+    def test_github_oauth2_success_non_primary(self) -> None:
+        account_data_dict = dict(email='nonprimary@zulip.com', name="Non Primary")
+        email_data = [
+            dict(email=account_data_dict["email"],
+                 verified=True),
+            dict(email='hamlet@zulip.com',
+                 verified=True,
+                 primary=True),
+            dict(email="ignored@example.com",
+                 verified=False),
+        ]
+        result = self.social_auth_test(account_data_dict,
+                                       subdomain='zulip', email_data=email_data,
+                                       expect_choose_email_screen=True,
+                                       next='/user_uploads/image')
+        data = load_subdomain_token(result)
+        self.assertEqual(data['email'], 'nonprimary@zulip.com')
+        self.assertEqual(data['name'], 'Non Primary')
+        self.assertEqual(data['subdomain'], 'zulip')
+        self.assertEqual(data['next'], '/user_uploads/image')
+        self.assertEqual(result.status_code, 302)
+        parsed_url = urllib.parse.urlparse(result.url)
+        uri = "{}://{}{}".format(parsed_url.scheme, parsed_url.netloc,
+                                 parsed_url.path)
+        self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/'))
+
+    def test_github_oauth2_success_single_email(self) -> None:
+        # If the user has a single email associated with its GitHub account,
+        # the choose email screen should not be shown and the first email
+        # should be used for user's signup/login.
+        account_data_dict = dict(email='not-hamlet@zulip.com', name=self.name)
+        email_data = [
+            dict(email='hamlet@zulip.com',
+                 verified=True,
+                 primary=True),
+        ]
+        result = self.social_auth_test(account_data_dict,
+                                       subdomain='zulip',
+                                       email_data=email_data,
+                                       expect_choose_email_screen=False,
+                                       next='/user_uploads/image')
+        data = load_subdomain_token(result)
+        self.assertEqual(data['email'], self.example_email("hamlet"))
+        self.assertEqual(data['name'], 'Hamlet')
+        self.assertEqual(data['subdomain'], 'zulip')
+        self.assertEqual(data['next'], '/user_uploads/image')
+        self.assertEqual(result.status_code, 302)
+        parsed_url = urllib.parse.urlparse(result.url)
+        uri = "{}://{}{}".format(parsed_url.scheme, parsed_url.netloc,
+                                 parsed_url.path)
+        self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/'))
+
     @override_settings(SOCIAL_AUTH_SUBDOMAIN=None)
     def test_when_social_auth_subdomain_is_not_set(self) -> None:
         account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
         result = self.social_auth_test(account_data_dict,
                                        subdomain='zulip',
+                                       expect_choose_email_screen=True,
                                        next='/user_uploads/image')
         data = load_subdomain_token(result)
         self.assertEqual(data['email'], self.example_email("hamlet"))
@@ -610,12 +718,59 @@ class SocialAuthBase(ZulipTestCase):
         user_profile = self.example_user("hamlet")
         do_deactivate_user(user_profile)
         account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
+        # We expect to go through the "choose email" screen here,
+        # because there won't be an existing user account we can
+        # auto-select for the user.
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip')
         self.assertEqual(result.status_code, 302)
         self.assertEqual(result.url, "/login/?is_deactivated=true")
         # TODO: verify whether we provide a clear error message
 
+    def test_github_oauth2_email_no_reply_dot_github_dot_com(self) -> None:
+        # As emails ending with `noreply.github.com` are excluded from
+        # verified_emails, choosing it as an email should raise a `email
+        # not associated` warning.
+        account_data_dict = dict(email="hamlet@noreply.github.com", name=self.name)
+        email_data = [
+            dict(email="notprimary@zulip.com",
+                 verified=True),
+            dict(email="hamlet@zulip.com",
+                 verified=True,
+                 primary=True),
+            dict(email=account_data_dict["email"],
+                 verified=True),
+        ]
+        with mock.patch('logging.warning') as mock_warning:
+            result = self.social_auth_test(account_data_dict,
+                                           subdomain='zulip',
+                                           expect_choose_email_screen=True,
+                                           email_data=email_data)
+            self.assertEqual(result.status_code, 302)
+            self.assertEqual(result.url, "/login/")
+            mock_warning.assert_called_once_with("Social auth (GitHub) failed because user has no verified"
+                                                 " emails associated with the account")
+
+    def test_github_oauth2_email_not_associated(self) -> None:
+        account_data_dict = dict(email='not-associated@zulip.com', name=self.name)
+        email_data = [
+            dict(email='nonprimary@zulip.com',
+                 verified=True,),
+            dict(email='hamlet@zulip.com',
+                 verified=True,
+                 primary=True),
+        ]
+        with mock.patch('logging.warning') as mock_warning:
+            result = self.social_auth_test(account_data_dict,
+                                           subdomain='zulip',
+                                           expect_choose_email_screen=True,
+                                           email_data=email_data)
+            self.assertEqual(result.status_code, 302)
+            self.assertEqual(result.url, "/login/")
+            mock_warning.assert_called_once_with("Social auth (GitHub) failed because user has no verified"
+                                                 " emails associated with the account")
+
     def test_social_auth_invalid_realm(self) -> None:
         account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
         with mock.patch('zerver.middleware.get_realm', return_value=get_realm("zulip")):
@@ -629,6 +784,7 @@ class SocialAuthBase(ZulipTestCase):
     def test_social_auth_invalid_email(self) -> None:
         account_data_dict = self.get_account_data_dict(email="invalid", name=self.name)
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip', next='/user_uploads/image')
         self.assertEqual(result.status_code, 302)
         self.assertEqual(result.url, "/login/?next=/user_uploads/image")
@@ -644,6 +800,7 @@ class SocialAuthBase(ZulipTestCase):
     def test_user_cannot_log_into_wrong_subdomain(self) -> None:
         account_data_dict = self.get_account_data_dict(email=self.email, name=self.name)
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zephyr')
         self.assertTrue(result.url.startswith("http://zephyr.testserver/accounts/login/subdomain/"))
         result = self.client_get(result.url.replace('http://zephyr.testserver', ''),
@@ -669,6 +826,7 @@ class SocialAuthBase(ZulipTestCase):
 
             # Now do it correctly
             result = self.social_auth_test(account_data_dict, subdomain='zulip',
+                                           expect_choose_email_screen=True,
                                            mobile_flow_otp=mobile_flow_otp)
         self.assertEqual(result.status_code, 302)
         redirect_url = result['Location']
@@ -689,6 +847,7 @@ class SocialAuthBase(ZulipTestCase):
         name = 'Full Name'
         account_data_dict = self.get_account_data_dict(email=email, name=name)
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip', is_signup='1')
         data = load_subdomain_token(result)
         self.assertEqual(data['email'], self.example_email("hamlet"))
@@ -710,6 +869,7 @@ class SocialAuthBase(ZulipTestCase):
         realm = get_realm("zulip")
         account_data_dict = self.get_account_data_dict(email=email, name=name)
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip', is_signup='1')
 
         data = load_subdomain_token(result)
@@ -775,6 +935,7 @@ class SocialAuthBase(ZulipTestCase):
 
         # First, try to signup for closed realm without using an invitation
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip', is_signup='1')
         result = self.client_get(result.url)
         # Verify that we're unable to signup, since this is a closed realm
@@ -782,6 +943,7 @@ class SocialAuthBase(ZulipTestCase):
         self.assert_in_success_response(["Sign up"], result)
 
         result = self.social_auth_test(account_data_dict, subdomain='zulip', is_signup='1',
+                                       expect_choose_email_screen=True,
                                        multiuse_object_key=multiuse_object_key)
 
         data = load_subdomain_token(result)
@@ -830,6 +992,7 @@ class SocialAuthBase(ZulipTestCase):
         name = 'Full Name'
         account_data_dict = self.get_account_data_dict(email=email, name=name)
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip')
         self.assertEqual(result.status_code, 302)
         data = load_subdomain_token(result)
@@ -852,6 +1015,7 @@ class SocialAuthBase(ZulipTestCase):
         name = 'Full Name'
         account_data_dict = self.get_account_data_dict(email=email, name=name)
         result = self.social_auth_test(account_data_dict,
+                                       expect_choose_email_screen=True,
                                        subdomain='zulip')
         self.assertEqual(result.status_code, 302)
         data = load_subdomain_token(result)
@@ -924,6 +1088,15 @@ class GitHubAuthBackendTest(SocialAuthBase):
             body=json.dumps(email_data)
         )
 
+        httpretty.register_uri(
+            httpretty.GET,
+            "https://api.github.com/teams/zulip-webapp/members/None",
+            status=200,
+            body=json.dumps(email_data)
+        )
+
+        self.email_data = email_data
+
     def get_account_data_dict(self, email: str, name: str) -> Dict[str, Any]:
         return dict(email=email, name=name)
 
@@ -961,6 +1134,7 @@ class GitHubAuthBackendTest(SocialAuthBase):
         with mock.patch('social_core.backends.github.GithubTeamOAuth2.user_data',
                         return_value=account_data_dict):
             result = self.social_auth_test(account_data_dict,
+                                           expect_choose_email_screen=True,
                                            subdomain='zulip')
         data = load_subdomain_token(result)
         self.assertEqual(data['email'], self.example_email("hamlet"))
@@ -985,6 +1159,7 @@ class GitHubAuthBackendTest(SocialAuthBase):
         with mock.patch('social_core.backends.github.GithubOrganizationOAuth2.user_data',
                         return_value=account_data_dict):
             result = self.social_auth_test(account_data_dict,
+                                           expect_choose_email_screen=True,
                                            subdomain='zulip')
         data = load_subdomain_token(result)
         self.assertEqual(data['email'], self.example_email("hamlet"))

zproject/backends.py

@@ -24,6 +24,7 @@ from django.conf import settings
 from django.core.exceptions import ValidationError
 from django.core.validators import validate_email
 from django.http import HttpResponse, HttpResponseRedirect
+from django.shortcuts import render
 from django.urls import reverse
 from requests import HTTPError
 from social_core.backends.github import GithubOAuth2, GithubOrganizationOAuth2, \
@@ -31,6 +32,7 @@ from social_core.backends.github import GithubOAuth2, GithubOrganizationOAuth2,
 from social_core.backends.azuread import AzureADOAuth2
 from social_core.backends.base import BaseAuth
 from social_core.backends.oauth import BaseOAuth2
+from social_core.pipeline.partial import partial
 from social_core.exceptions import AuthFailed, SocialAuthBaseException
 
 from zerver.lib.actions import do_create_user, do_reactivate_user, do_deactivate_user, \
@@ -667,16 +669,44 @@ def social_associate_user_helper(backend: BaseAuth, return_data: Dict[str, Any],
         # custom per-backend code to properly fetch only verified
         # email addresses from the appropriate third-party API.
         verified_emails = backend.get_verified_emails(*args, **kwargs)
-        if len(verified_emails) == 0:
+        verified_emails_length = len(verified_emails)
+        if verified_emails_length == 0:
             # TODO: Provide a nice error message screen to the user
             # for this case, rather than just logging a warning.
             logging.warning("Social auth (%s) failed because user has no verified emails" %
                             (backend.auth_backend_name,))
             return_data["email_not_verified"] = True
             return None
-        # TODO: ideally, we'd prompt the user for which email they
+
-        # want to use with another pipeline stage here.
+        if verified_emails_length == 1:
-        validated_email = verified_emails[0]
+            chosen_email = verified_emails[0]
+        else:
+            chosen_email = backend.strategy.request_data().get('email')
+
+        if not chosen_email:
+            return render(backend.strategy.request, 'zerver/social_auth_select_email.html', context = {
+                'primary_email': verified_emails[0],
+                'verified_non_primary_emails': verified_emails[1:],
+                'backend': 'github'
+            })
+
+        try:
+            validate_email(chosen_email)
+        except ValidationError:
+            return_data['invalid_email'] = True
+            return None
+
+        if chosen_email not in verified_emails:
+            # If a user edits the submit value for the choose email form, we might
+            # end up with a wrong email associated with the account. The below code
+            # takes care of that.
+            logging.warning("Social auth (%s) failed because user has no verified"
+                            " emails associated with the account" %
+                            (backend.auth_backend_name,))
+            return_data["email_not_associated"] = True
+            return None
+
+        validated_email = chosen_email
     else:  # nocoverage
         # This code path isn't used by GitHubAuthBackend
         validated_email = kwargs["details"].get("email")
@@ -686,11 +716,6 @@ def social_associate_user_helper(backend: BaseAuth, return_data: Dict[str, Any],
         # social auth backends.
         return_data['invalid_email'] = True
         return None
-    try:
-        validate_email(validated_email)
-    except ValidationError:
-        return_data['invalid_email'] = True
-        return None
 
     return_data["valid_attestation"] = True
     return_data['validated_email'] = validated_email
@@ -705,22 +730,29 @@ def social_associate_user_helper(backend: BaseAuth, return_data: Dict[str, Any],
 
     return user_profile
 
+@partial
 def social_auth_associate_user(
         backend: BaseAuth,
         *args: Any,
-        **kwargs: Any) -> Dict[str, Any]:
+        **kwargs: Any) -> Union[HttpResponse, Dict[str, Any]]:
     """A simple wrapper function to reformat the return data from
     social_associate_user_helper as a dictionary.  The
     python-social-auth infrastructure will then pass those values into
     later stages of settings.SOCIAL_AUTH_PIPELINE, such as
     social_auth_finish, as kwargs.
     """
+    partial_token = backend.strategy.request_data().get('partial_token')
     return_data = {}  # type: Dict[str, Any]
     user_profile = social_associate_user_helper(
         backend, return_data, *args, **kwargs)
 
+    if type(user_profile) == HttpResponse:
+        return user_profile
+    else:
         return {'user_profile': user_profile,
-            'return_data': return_data}
+                'return_data': return_data,
+                'partial_token': partial_token,
+                'partial_backend_name': backend}
 
 def social_auth_finish(backend: Any,
                        details: Dict[str, Any],
@@ -747,6 +779,7 @@ def social_auth_finish(backend: Any,
     invalid_realm = return_data.get('invalid_realm')
     invalid_email = return_data.get('invalid_email')
     auth_failed_reason = return_data.get("social_auth_failed_reason")
+    email_not_associated = return_data.get("email_not_associated")
 
     if invalid_realm:
         from zerver.views.auth import redirect_to_subdomain_login_url
@@ -755,7 +788,7 @@ def social_auth_finish(backend: Any,
     if inactive_user:
         return redirect_deactivated_user_to_login()
 
-    if auth_backend_disabled or inactive_realm or no_verified_email:
+    if auth_backend_disabled or inactive_realm or no_verified_email or email_not_associated:
         # Redirect to login page. We can't send to registration
         # workflow with these errors. We will redirect to login page.
         return None
@@ -869,9 +902,7 @@ class GitHubAuthBackend(SocialAuthMixin, GithubOAuth2):
             emails = []
 
         verified_emails = []  # type: List[str]
-        for email_obj in emails:
+        for email_obj in self.filter_usable_emails(emails):
-            if not email_obj.get("verified"):
-                continue
             # social_associate_user_helper assumes that the first email in
             # verified_emails is primary.
             if email_obj.get("primary"):
@@ -881,6 +912,18 @@ class GitHubAuthBackend(SocialAuthMixin, GithubOAuth2):
 
         return verified_emails
 
+    def filter_usable_emails(self, emails: List[Dict[str, Any]]) -> List[Dict[str, Any]]:
+        # We only let users login using email addresses that are verified
+        # by GitHub, because the whole point is for the user to
+        # demonstrate that they control the target email address.  We also
+        # disallow the @noreply.github.com email addresses, because
+        # structurally, we only want to allow email addresses that can
+        # receive emails, and those cannot.
+        return [
+            email for email in emails
+            if email.get('verified') and not email["email"].endswith("@noreply.github.com")
+        ]
+
     def user_data(self, access_token: str, *args: Any, **kwargs: Any) -> Dict[str, str]:
         """This patched user_data function lets us combine together the 3
         social auth backends into a single Zulip backend for GitHub Oauth2"""