From 5aded51b739f0bf7ca1fee0d906de158f5ae4567 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Fri, 1 Nov 2019 00:00:36 +0100 Subject: [PATCH] register: Pre-populate Name in social backend flow. By adding some additional plumbing (through PreregistrationUser) of the full_name and an additional full_name_validated option, we pre-populate the Full Name field in the registration form when coming through a social backend (google/github/saml/etc.) and potentially skip the registration form (if the user would have nothing to do there other than clicking the Confirm button) and just create the account and log the user in. --- .../0251_prereg_user_add_full_name.py | 25 +++++++++ zerver/models.py | 5 ++ zerver/tests/test_auth_backends.py | 52 +++++++++++-------- zerver/tests/test_signup.py | 15 ++++++ zerver/views/auth.py | 42 ++++++++++----- zerver/views/registration.py | 9 ++++ zproject/backends.py | 43 +++++++++++---- 7 files changed, 147 insertions(+), 44 deletions(-) create mode 100644 zerver/migrations/0251_prereg_user_add_full_name.py diff --git a/zerver/migrations/0251_prereg_user_add_full_name.py b/zerver/migrations/0251_prereg_user_add_full_name.py new file mode 100644 index 0000000000..39e6c5abc3 --- /dev/null +++ b/zerver/migrations/0251_prereg_user_add_full_name.py @@ -0,0 +1,25 @@ +# -*- coding: utf-8 -*- +# Generated by Django 1.11.25 on 2019-10-31 20:31 +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('zerver', '0250_saml_auth'), + ] + + operations = [ + migrations.AddField( + model_name='preregistrationuser', + name='full_name', + field=models.CharField(max_length=100, null=True), + ), + migrations.AddField( + model_name='preregistrationuser', + name='full_name_validated', + field=models.BooleanField(default=False), + ), + ] diff --git a/zerver/models.py b/zerver/models.py index f327ed4f81..92edf4ebcd 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1168,6 +1168,11 @@ class PreregistrationUser(models.Model): # form. email = models.EmailField() # type: str + + # If the pre-registration process provides a suggested full name for this user, + # store it here to use it to prepopulate the Full Name field in the registration form: + full_name = models.CharField(max_length=UserProfile.MAX_NAME_LENGTH, null=True) # type: Optional[str] + full_name_validated = models.BooleanField(default=False) referred_by = models.ForeignKey(UserProfile, null=True, on_delete=CASCADE) # type: Optional[UserProfile] streams = models.ManyToManyField('Stream') # type: Manager invited_at = models.DateTimeField(auto_now=True) # type: datetime.datetime diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index e7a2dda3e5..7861b24dee 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -766,6 +766,7 @@ class SocialAuthBase(ZulipTestCase): # Name wasn't changed at all self.assertEqual(hamlet.full_name, "King Hamlet") + @override_settings(TERMS_OF_SERVICE=None) def test_social_auth_registration(self) -> None: """If the user doesn't exist yet, social auth can be used to register an account""" email = "newuser@zulip.com" @@ -795,26 +796,30 @@ class SocialAuthBase(ZulipTestCase): result = self.client_get(result.url) self.assert_in_response('action="/accounts/register/"', result) data = {"from_confirmation": "1", - "full_name": name, "key": confirmation_key} result = self.client_post('/accounts/register/', data) - self.assert_in_response("We just need you to do one last thing", result) + if not self.BACKEND_CLASS.full_name_validated: + self.assert_in_response("We just need you to do one last thing", result) - # Verify that the user is asked for name but not password - self.assert_not_in_success_response(['id_password'], result) - self.assert_in_success_response(['id_full_name'], result) + # Verify that the user is asked for name but not password + self.assert_not_in_success_response(['id_password'], result) + self.assert_in_success_response(['id_full_name'], result) + # Verify the name field gets correctly pre-populated: + self.assert_in_success_response([name], result) - # Click confirm registration button. - result = self.client_post( - '/accounts/register/', - {'full_name': name, - 'key': confirmation_key, - 'terms': True}) + # Click confirm registration button. + result = self.client_post( + '/accounts/register/', + {'full_name': name, + 'key': confirmation_key, + 'terms': True}) self.assertEqual(result.status_code, 302) user_profile = get_user(email, realm) self.assert_logged_in_user_id(user_profile.id) + self.assertEqual(user_profile.full_name, name) + @override_settings(TERMS_OF_SERVICE=None) def test_social_auth_registration_using_multiuse_invite(self) -> None: """If the user doesn't exist yet, social auth can be used to register an account""" email = "newuser@zulip.com" @@ -870,25 +875,28 @@ class SocialAuthBase(ZulipTestCase): result = self.client_get(result.url) self.assert_in_response('action="/accounts/register/"', result) data = {"from_confirmation": "1", - "full_name": name, "key": confirmation_key} result = self.client_post('/accounts/register/', data) - self.assert_in_response("We just need you to do one last thing", result) + if not self.BACKEND_CLASS.full_name_validated: + self.assert_in_response("We just need you to do one last thing", result) - # Verify that the user is asked for name but not password - self.assert_not_in_success_response(['id_password'], result) - self.assert_in_success_response(['id_full_name'], result) + # Verify that the user is asked for name but not password + self.assert_not_in_success_response(['id_password'], result) + self.assert_in_success_response(['id_full_name'], result) + # Verify the name field gets correctly pre-populated: + self.assert_in_success_response([name], result) - # Click confirm registration button. - result = self.client_post( - '/accounts/register/', - {'full_name': name, - 'key': confirmation_key, - 'terms': True}) + # Click confirm registration button. + result = self.client_post( + '/accounts/register/', + {'full_name': name, + 'key': confirmation_key, + 'terms': True}) self.assertEqual(result.status_code, 302) user_profile = get_user(email, realm) self.assert_logged_in_user_id(user_profile.id) + self.assertEqual(user_profile.full_name, name) def test_social_auth_registration_without_is_signup(self) -> None: """If `is_signup` is not set then a new account isn't created""" diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index bfaf8509f5..5cb08ae722 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -79,6 +79,7 @@ class RedirectAndLogIntoSubdomainTestCase(ZulipTestCase): data = load_subdomain_token(response) self.assertDictEqual(data, {'name': name, 'next': '', 'email': email, + 'full_name_validated': False, 'subdomain': realm.subdomain, 'is_signup': False, 'multiuse_object_key': ''}) @@ -89,6 +90,20 @@ class RedirectAndLogIntoSubdomainTestCase(ZulipTestCase): data = load_subdomain_token(response) self.assertDictEqual(data, {'name': name, 'next': '', 'email': email, + 'full_name_validated': False, + 'subdomain': realm.subdomain, + 'is_signup': True, + 'multiuse_object_key': 'key' + }) + + response = redirect_and_log_into_subdomain(realm, name, email, + is_signup=True, + full_name_validated=True, + multiuse_object_key='key') + data = load_subdomain_token(response) + self.assertDictEqual(data, {'name': name, 'next': '', + 'email': email, + 'full_name_validated': True, 'subdomain': realm.subdomain, 'is_signup': True, 'multiuse_object_key': 'key' diff --git a/zerver/views/auth.py b/zerver/views/auth.py index b6b643be46..9b720d4b91 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -60,21 +60,27 @@ def get_safe_redirect_to(url: str, redirect_host: str) -> str: return redirect_host def create_preregistration_user(email: str, request: HttpRequest, realm_creation: bool=False, - password_required: bool=True) -> HttpResponse: + password_required: bool=True, full_name: Optional[str]=None, + full_name_validated: bool=False) -> HttpResponse: realm = None if not realm_creation: try: realm = get_realm(get_subdomain(request)) except Realm.DoesNotExist: pass - return PreregistrationUser.objects.create(email=email, - realm_creation=realm_creation, - password_required=password_required, - realm=realm) + return PreregistrationUser.objects.create( + email=email, + realm_creation=realm_creation, + password_required=password_required, + realm=realm, + full_name=full_name, + full_name_validated=full_name_validated + ) def maybe_send_to_registration(request: HttpRequest, email: str, full_name: str='', is_signup: bool=False, password_required: bool=True, - multiuse_object_key: str='') -> HttpResponse: + multiuse_object_key: str='', + full_name_validated: bool=False) -> HttpResponse: """Given a successful authentication for an email address (i.e. we've confirmed the user controls the email address) that does not currently have a Zulip account in the target realm, send them to @@ -114,8 +120,12 @@ def maybe_send_to_registration(request: HttpRequest, email: str, full_name: str= prereg_user = create_preregistration_user(email, request, password_required=password_required) else: - prereg_user = create_preregistration_user(email, request, - password_required=password_required) + prereg_user = create_preregistration_user( + email, request, + password_required=password_required, + full_name=full_name, + full_name_validated=full_name_validated + ) if multiuse_object_key: request.session.modified = True @@ -167,7 +177,8 @@ def login_or_register_remote_user(request: HttpRequest, remote_username: str, user_profile: Optional[UserProfile], full_name: str='', mobile_flow_otp: Optional[str]=None, is_signup: bool=False, redirect_to: str='', - multiuse_object_key: str='') -> HttpResponse: + multiuse_object_key: str='', + full_name_validated: bool=False) -> HttpResponse: """Given a successful authentication showing the user controls given email address (remote_username) and potentially a UserProfile object (if the user already has a Zulip account), redirect the @@ -192,7 +203,8 @@ def login_or_register_remote_user(request: HttpRequest, remote_username: str, # there's no associated Zulip user account. Consider sending # the request to registration. return maybe_send_to_registration(request, email, full_name, password_required=False, - is_signup=is_signup, multiuse_object_key=multiuse_object_key) + is_signup=is_signup, multiuse_object_key=multiuse_object_key, + full_name_validated=full_name_validated) # Otherwise, the user has successfully authenticated to an # account, and we need to do the right thing depending whether @@ -435,6 +447,7 @@ def log_into_subdomain(request: HttpRequest, token: str) -> HttpResponse: full_name = data['name'] is_signup = data['is_signup'] redirect_to = data['next'] + full_name_validated = data.get('full_name_validated', False) if 'multiuse_object_key' in data: multiuse_object_key = data['multiuse_object_key'] @@ -473,14 +486,17 @@ def log_into_subdomain(request: HttpRequest, token: str) -> HttpResponse: return login_or_register_remote_user(request, email_address, user_profile, full_name, is_signup=is_signup, redirect_to=redirect_to, - multiuse_object_key=multiuse_object_key) + multiuse_object_key=multiuse_object_key, + full_name_validated=full_name_validated) def redirect_and_log_into_subdomain(realm: Realm, full_name: str, email_address: str, is_signup: bool=False, redirect_to: str='', - multiuse_object_key: str='') -> HttpResponse: + multiuse_object_key: str='', + full_name_validated: bool=False) -> HttpResponse: data = {'name': full_name, 'email': email_address, 'subdomain': realm.subdomain, 'is_signup': is_signup, 'next': redirect_to, - 'multiuse_object_key': multiuse_object_key} + 'multiuse_object_key': multiuse_object_key, + 'full_name_validated': full_name_validated} token = signing.dumps(data, salt=_subdomain_token_salt) subdomain_login_uri = (realm.uri + reverse('zerver.views.auth.log_into_subdomain', args=[token])) diff --git a/zerver/views/registration.py b/zerver/views/registration.py index d16d39bee3..579350adde 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -166,6 +166,15 @@ def accounts_register(request: HttpRequest) -> HttpResponse: except TypeError: # Let the user fill out a name and/or try another backend form = RegistrationForm(realm_creation=realm_creation) + elif prereg_user.full_name: + if prereg_user.full_name_validated: + request.session['authenticated_full_name'] = prereg_user.full_name + name_validated = True + form = RegistrationForm({'full_name': prereg_user.full_name}, + realm_creation=realm_creation) + else: + form = RegistrationForm(initial={'full_name': prereg_user.full_name}, + realm_creation=realm_creation) elif 'full_name' in request.POST: form = RegistrationForm( initial={'full_name': request.POST.get('full_name')}, diff --git a/zproject/backends.py b/zproject/backends.py index 252a09c7f2..608dce6e23 100644 --- a/zproject/backends.py +++ b/zproject/backends.py @@ -948,6 +948,7 @@ def social_auth_finish(backend: Any, assert return_data.get('valid_attestation') is True strategy = backend.strategy + full_name_validated = backend.full_name_validated email_address = return_data['validated_email'] full_name = return_data['full_name'] is_signup = strategy.session_get('is_signup') == '1' @@ -970,11 +971,14 @@ def social_auth_finish(backend: Any, # redirect directly from here, saving a round trip over what # we need to do to create session cookies on the right domain # in the web login flow (below). - 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) + 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 + ) # If this authentication code were executing on # subdomain.zulip.example.com, we would just call @@ -989,10 +993,13 @@ def social_auth_finish(backend: Any, # cryptographically signed token) to a route on # subdomain.zulip.example.com that will verify the signature and # then call login_or_register_remote_user. - return redirect_and_log_into_subdomain(realm, full_name, email_address, - is_signup=is_signup, - redirect_to=redirect_to, - multiuse_object_key=multiuse_object_key) + return redirect_and_log_into_subdomain( + realm, full_name, email_address, + is_signup=is_signup, + redirect_to=redirect_to, + multiuse_object_key=multiuse_object_key, + full_name_validated=full_name_validated + ) class SocialAuthMixin(ZulipAuthMixin): auth_backend_name = "undeclared" @@ -1003,6 +1010,18 @@ class SocialAuthMixin(ZulipAuthMixin): # higher sort order are displayed first. sort_order = 0 + # Whether we expect that the full_name value obtained by the + # social backend is definitely how the user should be referred to + # in Zulip, which in turn determines whether we should always show + # a registration form in the event with a default value of the + # user's name when using this social backend so they can change + # it. For social backends like SAML that are expected to be a + # central database, this should be True; for backends like GitHub + # where the user might not have a name set or have it set to + # something other than the name they will prefer to use in Zulip, + # it should be False. + full_name_validated = False + def auth_complete(self, *args: Any, **kwargs: Any) -> Optional[HttpResponse]: """This is a small wrapper around the core `auth_complete` method of python-social-auth, designed primarily to prevent 500s for @@ -1125,6 +1144,12 @@ class SAMLAuthBackend(SocialAuthMixin, SAMLAuth): # There's no common default logo for SAML authentication. display_icon = "" + # The full_name provided by the IdP is very likely the standard + # employee directory name for the user, and thus what they and + # their organization want to use in Zulip. So don't unnecessarily + # provide a registration flow prompt for them to set their name. + full_name_validated = True + def auth_url(self) -> str: """Get the URL to which we must redirect in order to authenticate the user. Overriding the original SAMLAuth.auth_url.