Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

github: Refactor email extraction to use the full emails data set. 

It's possible to make GitHub social authentication support letting the
user pick which of their verified email addresses to pick, using the
python-social-auth pipeline feature.  We need to add an additional
screen to let the user pick, so we're not adding support for that now,
but this at least migrates this to use the data set of all emails that
have been verified as associated with the user's GitHub account (and
we just assume the user wants their primary email).

This also fixes the inability for very old GitHub accounts (where the
`email` field in the details might be a string the user wanted on
their GitHub profile page) to using GitHub auth to login.

Fixes #9127.
This commit is contained in:
Tim Abbott 2018-06-06 15:19:06 -07:00
parent d8ba378050
commit c9b0c0add4
2 changed files with 87 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

54
zerver/tests/test_auth_backends.py
View File

@ -336,6 +336,15 @@ class AuthBackendTest(ZulipTestCase):
'token_type': 'bearer'
}
account_data_dict = dict(email=user.email, name=user.full_name)
email_data = [
dict(email=account_data_dict["email"],
verified=True,
primary=True),
dict(email="nonprimary@example.com",
verified=True),
dict(email="ignored@example.com",
verified=False),
]
httpretty.enable()
httpretty.register_uri(
httpretty.POST,
@ -349,6 +358,12 @@ class AuthBackendTest(ZulipTestCase):
status=200,
body=json.dumps(account_data_dict)
)
httpretty.register_uri(
httpretty.GET,
"https://api.github.com/user/emails",
status=200,
body=json.dumps(email_data)
)
backend = GitHubAuthBackend()
backend.strategy = DjangoStrategy(storage=BaseDjangoStorage())
@ -361,8 +376,9 @@ class AuthBackendTest(ZulipTestCase):
result = orig_authenticate(backend, *args, **kwargs)
return result
backend.authenticate = patched_authenticate
good_kwargs = dict(backend=backend, strategy=backend.strategy, storage=backend.strategy.storage,
response=account_data_dict,
good_kwargs = dict(backend=backend, strategy=backend.strategy,
storage=backend.strategy.storage,
response=token_data_dict,
subdomain='zulip')
bad_kwargs = dict(subdomain='acme')
with mock.patch('zerver.views.auth.redirect_and_log_into_subdomain',
@ -410,6 +426,7 @@ class GitHubAuthBackendTest(ZulipTestCase):
*, subdomain: Optional[str]=None,
mobile_flow_otp: Optional[str]=None,
is_signup: Optional[str]=None,
email_not_verified: bool=False,
next: str='') -> HttpResponse:
url = "/accounts/login/social/github"
params = {}
@ -442,6 +459,22 @@ class GitHubAuthBackendTest(ZulipTestCase):
'access_token': 'foobar',
'token_type': 'bearer'
}
if email_not_verified:
email_data = [
dict(email=account_data_dict["email"],
verified=False,
primary=True),
]
else:
email_data = [
dict(email=account_data_dict["email"],
verified=True,
primary=True),
dict(email="ignored@example.com",
verified=False),
dict(email="notprimary@example.com",
verified=True),
]
# We register callbacks for the key URLs on github.com that
# /complete/github will call
httpretty.enable()
@ -457,6 +490,12 @@ class GitHubAuthBackendTest(ZulipTestCase):
status=200,
body=json.dumps(account_data_dict)
)
httpretty.register_uri(
httpretty.GET,
"https://api.github.com/user/emails",
status=200,
body=json.dumps(email_data)
)
parsed_url = urllib.parse.urlparse(result.url)
csrf_state = urllib.parse.parse_qs(parsed_url.query)['state']
@ -488,6 +527,17 @@ class GitHubAuthBackendTest(ZulipTestCase):
parsed_url.path)
self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/'))
def test_github_oauth2_email_not_verified(self) -> None:
account_data_dict = dict(email=self.email, name=self.name)
with mock.patch('logging.warning') as mock_warning:
result = self.github_oauth2_test(account_data_dict,
subdomain='zulip',
email_not_verified=True)
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")
@override_settings(SOCIAL_AUTH_GITHUB_TEAM_ID='zulip-webapp')
def test_github_oauth2_github_team_not_member_failed(self) -> None:
account_data_dict = dict(email=self.email, name=self.name)

40
zproject/backends.py
View File

@ -10,6 +10,7 @@ from django.core.exceptions import ValidationError
from django.core.validators import validate_email
from django.http import HttpResponse
from oauth2client.crypt import AppIdentityError
from requests import HTTPError
from social_core.backends.github import GithubOAuth2, GithubOrganizationOAuth2, \
GithubTeamOAuth2
from social_core.backends.base import BaseAuth
@ -380,10 +381,20 @@ def social_associate_user_helper(backend: BaseAuth, return_data: Dict[str, Any],
if 'auth_failed_reason' in kwargs.get('response', {}):
return_data["social_auth_failed_reason"] = kwargs['response']["auth_failed_reason"]
return None
elif hasattr(backend, 'get_validated_email'):
elif hasattr(backend, 'get_verified_emails'):
# Some social backends, like GitHubAuthBackend, don't guarantee that
# the `details` data is validated.
validated_email = backend.get_validated_email(*args, **kwargs)
verified_emails = backend.get_verified_emails(*args, **kwargs)
if len(verified_emails) == 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]
else: # nocoverage
# This code path isn't used by GitHubAuthBackend
validated_email = kwargs["details"].get("email")
@ -434,6 +445,7 @@ def social_auth_finish(backend: Any,
user_profile = kwargs['user_profile']
return_data = kwargs['return_data']
no_verified_email = return_data.get("email_not_verified")
auth_backend_disabled = return_data.get('auth_backend_disabled')
inactive_user = return_data.get('inactive_user')
inactive_realm = return_data.get('inactive_realm')
@ -445,7 +457,7 @@ def social_auth_finish(backend: Any,
if invalid_realm:
from zerver.views.auth import redirect_to_subdomain_login_url
return redirect_to_subdomain_login_url()
if auth_backend_disabled or inactive_user or inactive_realm:
if auth_backend_disabled or inactive_user or inactive_realm or no_verified_email:
# Redirect to login page. We can't send to registration
# workflow with these errors. We will redirect to login page.
return None
@ -490,8 +502,26 @@ def social_auth_finish(backend: Any,
class GitHubAuthBackend(GithubOAuth2):
auth_backend_name = "GitHub"
def get_validated_email(self, *args: Any, **kwargs: Any) -> Optional[str]:
return kwargs['response']['email']
def get_verified_emails(self, *args: Any, **kwargs: Any) -> List[str]:
access_token = kwargs["response"]["access_token"]
try:
emails = self._user_data(access_token, '/emails')
except (HTTPError, ValueError, TypeError): # nocoverage
# We don't really need an explicit test for this code
# path, since the outcome will be the same as any other
# case without any verified emails
emails = []
verified_emails = []
for email_obj in emails:
if not email_obj.get("verified"):
continue
# TODO: When we add a screen to let the user select an email, remove this line.
if not email_obj.get("primary"):
continue
verified_emails.append(email_obj["email"])
return verified_emails
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