From 859bde482dec7a36ec076a851059c4e8d0197a41 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Thu, 23 Jan 2020 14:22:28 +0100 Subject: [PATCH] auth: Implement server side of desktop_flow_otp. --- zerver/tests/test_auth_backends.py | 52 ++++++++++++++++++++++++++++-- zerver/views/auth.py | 44 ++++++++++++++++++++++--- zproject/backends.py | 24 ++++++++++---- zproject/settings.py | 3 +- 4 files changed, 109 insertions(+), 14 deletions(-) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 96cdfd9179..3546eb7ba4 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -512,6 +512,7 @@ class SocialAuthBase(ZulipTestCase): def prepare_login_url_and_headers(self, subdomain: Optional[str]=None, mobile_flow_otp: Optional[str]=None, + desktop_flow_otp: Optional[str]=None, is_signup: Optional[str]=None, next: str='', multiuse_object_key: str='', @@ -528,6 +529,8 @@ class SocialAuthBase(ZulipTestCase): if mobile_flow_otp is not None: params['mobile_flow_otp'] = mobile_flow_otp headers['HTTP_USER_AGENT'] = "ZulipAndroid" + if desktop_flow_otp is not None: + params['desktop_flow_otp'] = desktop_flow_otp if is_signup is not None: url = self.SIGNUP_URL params['next'] = next @@ -540,6 +543,7 @@ class SocialAuthBase(ZulipTestCase): def social_auth_test(self, account_data_dict: Dict[str, str], *, subdomain: Optional[str]=None, mobile_flow_otp: Optional[str]=None, + desktop_flow_otp: Optional[str]=None, is_signup: Optional[str]=None, next: str='', multiuse_object_key: str='', @@ -547,7 +551,8 @@ class SocialAuthBase(ZulipTestCase): alternative_start_url: Optional[str]=None, **extra_data: Any) -> HttpResponse: url, headers = self.prepare_login_url_and_headers( - subdomain, mobile_flow_otp, is_signup, next, multiuse_object_key, alternative_start_url + subdomain, mobile_flow_otp, desktop_flow_otp, is_signup, next, + multiuse_object_key, alternative_start_url ) result = self.client_get(url, **headers) @@ -748,6 +753,48 @@ class SocialAuthBase(ZulipTestCase): self.assertEqual(len(mail.outbox), 1) self.assertIn('Zulip on Android', mail.outbox[0].body) + def test_social_auth_desktop_success(self) -> None: + desktop_flow_otp = '1234abcd' * 8 + account_data_dict = self.get_account_data_dict(email=self.email, name='Full Name') + + # Verify that the right thing happens with an invalid-format OTP + result = self.social_auth_test(account_data_dict, subdomain='zulip', + desktop_flow_otp="1234") + self.assert_json_error(result, "Invalid OTP") + result = self.social_auth_test(account_data_dict, subdomain='zulip', + desktop_flow_otp="invalido" * 8) + self.assert_json_error(result, "Invalid OTP") + + # Now do it correctly + result = self.social_auth_test(account_data_dict, subdomain='zulip', + expect_choose_email_screen=True, + desktop_flow_otp=desktop_flow_otp) + self.assertEqual(result.status_code, 302) + + redirect_url = result['Location'] + parsed_url = urllib.parse.urlparse(redirect_url) + query_params = urllib.parse.parse_qs(parsed_url.query) + self.assertEqual(parsed_url.scheme, 'zulip') + self.assertEqual(query_params["realm"], ['http://zulip.testserver']) + self.assertEqual(query_params["email"], [self.example_email("hamlet")]) + + encrypted_key = query_params["otp_encrypted_login_key"][0] + decrypted_key = otp_decrypt_api_key(encrypted_key, desktop_flow_otp) + auth_url = 'http://zulip.testserver/accounts/login/subdomain/{}'.format(decrypted_key) + + result = self.client_get(auth_url) + self.assertEqual(result.status_code, 302) + self.assert_logged_in_user_id(self.user_profile.id) + + def test_social_auth_mobile_and_desktop_flow_in_one_request_error(self) -> None: + otp = '1234abcd' * 8 + account_data_dict = self.get_account_data_dict(email=self.email, name='Full Name') + + result = self.social_auth_test(account_data_dict, subdomain='zulip', + expect_choose_email_screen=True, + desktop_flow_otp=otp, mobile_flow_otp=otp) + self.assert_json_error(result, "Can't use both mobile_flow_otp and desktop_flow_otp together.") + def test_social_auth_registration_existing_account(self) -> None: """If the user already exists, signup flow just logs them in""" email = "hamlet@zulip.com" @@ -1039,12 +1086,13 @@ class SAMLAuthBackendTest(SocialAuthBase): def social_auth_test(self, account_data_dict: Dict[str, str], *, subdomain: Optional[str]=None, mobile_flow_otp: Optional[str]=None, + desktop_flow_otp: Optional[str]=None, is_signup: Optional[str]=None, next: str='', multiuse_object_key: str='', **extra_data: Any) -> HttpResponse: url, headers = self.prepare_login_url_and_headers( - subdomain, mobile_flow_otp, is_signup, next, multiuse_object_key + subdomain, mobile_flow_otp, desktop_flow_otp, is_signup, next, multiuse_object_key ) result = self.client_get(url, **headers) diff --git a/zerver/views/auth.py b/zerver/views/auth.py index b99fb017bc..df5c2fff43 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -198,6 +198,8 @@ def register_remote_user(request: HttpRequest, remote_username: str, def login_or_register_remote_user(request: HttpRequest, remote_username: str, user_profile: Optional[UserProfile], full_name: str='', mobile_flow_otp: Optional[str]=None, + desktop_flow_otp: Optional[str]=None, + realm: Optional[Realm]=None, is_signup: bool=False, redirect_to: str='', multiuse_object_key: str='', full_name_validated: bool=False) -> HttpResponse: @@ -216,8 +218,8 @@ def login_or_register_remote_user(request: HttpRequest, remote_username: str, Zulip account but is_signup is False (i.e. the user tried to login and then did social authentication selecting an email address that does not have a Zulip account in this organization). - * A zulip:// URL to send control back to the mobile apps if they - are doing authentication using the mobile_flow_otp flow. + * A zulip:// URL to send control back to the mobile or desktop apps if they + are doing authentication using the mobile_flow_otp or desktop_flow_otp flow. """ if user_profile is None or user_profile.is_mirror_dummy: return register_remote_user(request, remote_username, full_name, @@ -229,17 +231,38 @@ def login_or_register_remote_user(request: HttpRequest, remote_username: str, # or not they're using the mobile OTP flow or want a browser session. if mobile_flow_otp is not None: return finish_mobile_flow(request, user_profile, mobile_flow_otp) + elif desktop_flow_otp is not None: + assert realm is not None + return finish_desktop_flow(request, user_profile, realm, desktop_flow_otp) do_login(request, user_profile) redirect_to = get_safe_redirect_to(redirect_to, user_profile.realm.uri) return HttpResponseRedirect(redirect_to) +def finish_desktop_flow(request: HttpRequest, user_profile: UserProfile, + realm: Realm, otp: str) -> HttpResponse: + """ + The desktop otp flow returns to the app (through a zulip:// redirect) + a token that allows obtaining (through log_into_subdomain) a logged in session + for the user account we authenticated in this flow. + The token can only be used once and within LOGIN_KEY_EXPIRATION_SECONDS + of being created, as nothing more powerful is needed for the desktop flow + and this ensures the key can only be used for completing this authentication attempt. + """ + data = {'email': user_profile.delivery_email, + 'subdomain': realm.subdomain} + token = store_login_data(data) + + return create_response_for_otp_flow(token, otp, user_profile, + encrypted_key_field_name='otp_encrypted_login_key') + def finish_mobile_flow(request: HttpRequest, user_profile: UserProfile, otp: str) -> HttpResponse: # For the mobile Oauth flow, we send the API key and other # necessary details in a redirect to a zulip:// URI scheme. api_key = get_api_key(user_profile) - response = create_response_for_otp_flow(api_key, otp, user_profile) + response = create_response_for_otp_flow(api_key, otp, user_profile, + encrypted_key_field_name='otp_encrypted_api_key') # Since we are returning an API key instead of going through # the Django login() function (which creates a browser @@ -257,9 +280,10 @@ def finish_mobile_flow(request: HttpRequest, user_profile: UserProfile, otp: str return response -def create_response_for_otp_flow(key: str, otp: str, user_profile: UserProfile) -> HttpResponse: +def create_response_for_otp_flow(key: str, otp: str, user_profile: UserProfile, + encrypted_key_field_name: str) -> HttpResponse: params = { - 'otp_encrypted_api_key': otp_encrypt_api_key(key, otp), + encrypted_key_field_name: otp_encrypt_api_key(key, otp), 'email': user_profile.delivery_email, 'realm': user_profile.realm.uri, } @@ -372,6 +396,12 @@ def oauth_redirect_to_root(request: HttpRequest, url: str, raise JsonableError(_("Invalid OTP")) params['mobile_flow_otp'] = mobile_flow_otp + desktop_flow_otp = request.GET.get('desktop_flow_otp') + if desktop_flow_otp is not None: + if not is_valid_otp(desktop_flow_otp): + raise JsonableError(_("Invalid OTP")) + params['desktop_flow_otp'] = desktop_flow_otp + next = request.GET.get('next') if next: params['next'] = next @@ -451,6 +481,10 @@ def log_into_subdomain(request: HttpRequest, token: str) -> HttpResponse: redirect_and_log_into_subdomain called on auth.zulip.example.com), call login_or_register_remote_user, passing all the authentication result data that has been stored in redis, associated with this token. + Obligatory fields for the data are 'subdomain' and 'email', because this endpoint + needs to know which user and realm to log into. Others are optional and only used + if the user account still needs to be made and they're passed as argument to the + register_remote_user function. """ if not has_api_key_format(token): # The tokens are intended to have the same format as API keys. logging.warning("log_into_subdomain: Malformed token given: %s" % (token,)) diff --git a/zproject/backends.py b/zproject/backends.py index 667b7994a5..ec947df14a 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -31,6 +31,7 @@ from django.dispatch import receiver, Signal from django.http import HttpResponse, HttpResponseRedirect from django.shortcuts import render from django.urls import reverse +from django.utils.translation import ugettext as _ from requests import HTTPError from onelogin.saml2.errors import OneLogin_Saml2_Error from social_core.backends.github import GithubOAuth2, GithubOrganizationOAuth2, \ @@ -1076,6 +1077,10 @@ def social_auth_finish(backend: Any, realm = Realm.objects.get(id=return_data["realm_id"]) multiuse_object_key = strategy.session_get('multiuse_object_key', '') mobile_flow_otp = strategy.session_get('mobile_flow_otp') + desktop_flow_otp = strategy.session_get('desktop_flow_otp') + if mobile_flow_otp and desktop_flow_otp: + raise JsonableError(_("Can't use both mobile_flow_otp and desktop_flow_otp together.")) + if user_profile is None or user_profile.is_mirror_dummy: is_signup = strategy.session_get('is_signup') == '1' else: @@ -1086,10 +1091,17 @@ def social_auth_finish(backend: Any, # # The next step is to call login_or_register_remote_user, but # there are two code paths here because of an optimization to save - # a redirect on mobile. + # a redirect on mobile and desktop. - if mobile_flow_otp is not None: - # For mobile app authentication, login_or_register_remote_user + if mobile_flow_otp or desktop_flow_otp: + extra_kwargs = {} + if mobile_flow_otp: + extra_kwargs["mobile_flow_otp"] = mobile_flow_otp + elif desktop_flow_otp: + extra_kwargs["desktop_flow_otp"] = desktop_flow_otp + extra_kwargs["realm"] = realm + + # For mobile and desktop app authentication, login_or_register_remote_user # will redirect to a special zulip:// URL that is handled by # the app after a successful authentication; so we can # redirect directly from here, saving a round trip over what @@ -1098,10 +1110,10 @@ def social_auth_finish(backend: Any, return login_or_register_remote_user( strategy.request, email_address, user_profile, full_name, - mobile_flow_otp=mobile_flow_otp, is_signup=is_signup, redirect_to=redirect_to, - full_name_validated=full_name_validated + full_name_validated=full_name_validated, + **extra_kwargs ) # If this authentication code were executing on @@ -1263,7 +1275,7 @@ class GoogleAuthBackend(SocialAuthMixin, GoogleOAuth2): @external_auth_method class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): auth_backend_name = "SAML" - standard_relay_params = ["subdomain", "multiuse_object_key", "mobile_flow_otp", + standard_relay_params = ["subdomain", "multiuse_object_key", "mobile_flow_otp", "desktop_flow_otp", "next", "is_signup"] REDIS_EXPIRATION_SECONDS = 60 * 15 name = "saml" diff --git a/zproject/settings.py b/zproject/settings.py index 406bf9104a..438fd1dccd 100644 --- a/zproject/settings.py +++ b/zproject/settings.py @@ -964,7 +964,8 @@ if REGISTER_LINK_DISABLED is None: # SOCIAL AUTHENTICATION SETTINGS ######################################################################## -SOCIAL_AUTH_FIELDS_STORED_IN_SESSION = ['subdomain', 'is_signup', 'mobile_flow_otp', 'multiuse_object_key'] +SOCIAL_AUTH_FIELDS_STORED_IN_SESSION = ['subdomain', 'is_signup', 'mobile_flow_otp', 'desktop_flow_otp', + 'multiuse_object_key'] SOCIAL_AUTH_LOGIN_ERROR_URL = '/login/' SOCIAL_AUTH_GITHUB_SECRET = get_secret('social_auth_github_secret')