From 80a3651cf362c5dd02e88c4a32e497dd8450069c Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Thu, 19 Jul 2018 03:15:49 +0530 Subject: [PATCH] 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 | 27 +++ .../zerver/social_auth_select_email.html | 33 ++++ zerver/tests/test_auth_backends.py | 177 +++++++++++++++++- zproject/backends.py | 75 ++++++-- 4 files changed, 295 insertions(+), 17 deletions(-) create mode 100644 templates/zerver/social_auth_select_email.html diff --git a/static/styles/portico/portico-signin.scss b/static/styles/portico/portico-signin.scss index 10b5dff3fc..3f2df12713 100644 --- a/static/styles/portico/portico-signin.scss +++ b/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%); + } +} diff --git a/templates/zerver/social_auth_select_email.html b/templates/zerver/social_auth_select_email.html new file mode 100644 index 0000000000..7a3b00d4e5 --- /dev/null +++ b/templates/zerver/social_auth_select_email.html @@ -0,0 +1,33 @@ +{% extends "zerver/portico_signup.html" %} + +{% block portico_content %} +
+ +
+ {% trans %} +

Select email

+ {% endtrans %} +
+ +
+
+
+ + +
+
+ {% for email in verified_non_primary_emails %} +
+
+ + +
+
+ {% endfor %} +
+
+{% endblock %} diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index e95f4e2292..e65527ff1d 100644 --- a/zerver/tests/test_auth_backends.py +++ b/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")) diff --git a/zproject/backends.py b/zproject/backends.py index a4127ec5b8..0ead261af9 100644 --- a/zproject/backends.py +++ b/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. - validated_email = verified_emails[0] + + if verified_emails_length == 1: + 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) - return {'user_profile': user_profile, - 'return_data': return_data} + if type(user_profile) == HttpResponse: + return user_profile + else: + return {'user_profile': user_profile, + '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: - if not email_obj.get("verified"): - continue + for email_obj in self.filter_usable_emails(emails): # 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"""