From 23a776c1448da18b906529e5951e24d8d58a7e81 Mon Sep 17 00:00:00 2001 From: Mateusz Mandera Date: Mon, 26 Sep 2022 00:06:13 +0200 Subject: [PATCH] maybe_send_to_registration: Don't reuse pre-existing PreregistraionUser. There was the following bug here: 1. Send an email invite to a user. 2. Have the user sign up via social auth without going through that invite, meaning either going via a multiuse invite link or just straight-up Sign up if the org permissions allow. That resulted in the PreregistrationUser that got generated in step (1) having 2 Confirmations tied to it - because maybe_send_to_registration grabbed the object and created a new confirmation link for it. That is a corrupted state, Confirmation is supposed to be unique. One could try to do fancy things with checking whether a PreregistrationUser already have a Confirmation link, but to avoid races between ConfirmationEmailWorker and maybe_send_to_registration, this would require taking locks and so on - which gets needlessly complicated. It's simpler to not have them compete for the same object. The point of the PreregistrationUser re-use in maybe_send_to_registration is that if an admin invites a user, setting their initial streams and role, it'd be an annoying experience if the user ends up signing up not via the invite and those initial streams streams etc. don't get set up. But to handle this, we can just copy the relevant values from the pre-existing prereg_user, rather than re-using the object itself. --- zerver/tests/test_auth_backends.py | 10 +++++- zerver/views/auth.py | 54 +++++++++++++++++------------- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index ca475b4bc0046..dcdb58d9f54ac 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -6666,7 +6666,10 @@ def is_valid(self) -> bool: email = self.example_email("hamlet") user = PreregistrationUser(email=email, realm=realm) + streams = Stream.objects.filter(realm=realm) user.save() + user.streams.set(streams) + create_confirmation_link(user, Confirmation.USER_REGISTRATION) with mock.patch("zerver.views.auth.HomepageForm", return_value=Form()): @@ -6677,7 +6680,12 @@ def is_valid(self) -> bool: assert confirmation is not None confirmation_key = confirmation.confirmation_key self.assertIn("do_confirm/" + confirmation_key, result["Location"]) - self.assertEqual(PreregistrationUser.objects.all().count(), 1) + prereg_users = list(PreregistrationUser.objects.all()) + self.assert_length(prereg_users, 2) + self.assertEqual( + list(prereg_users[0].streams.all().order_by("id")), + list(prereg_users[1].streams.all().order_by("id")), + ) class TestAdminSetBackends(ZulipTestCase): diff --git a/zerver/views/auth.py b/zerver/views/auth.py index 1eb9cb76ebad6..12f2778464e76 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -225,36 +225,44 @@ def maybe_send_to_registration( # creation or confirm-continue-registration depending on # is_signup. try: - prereg_user = filter_to_valid_prereg_users( + # If there's an existing, valid PreregistrationUser for this + # user, we want to fetch it since some values from it will be used + # as defaults for creating the signed up user. + existing_prereg_user = filter_to_valid_prereg_users( PreregistrationUser.objects.filter(email__iexact=email, realm=realm) ).latest("invited_at") - - # password_required and full_name data passed here as argument should take precedence - # over the defaults with which the existing PreregistrationUser that we've just fetched - # was created. - prereg_user.password_required = password_required - update_fields = ["password_required"] - if full_name: - prereg_user.full_name = full_name - prereg_user.full_name_validated = full_name_validated - update_fields.extend(["full_name", "full_name_validated"]) - prereg_user.save(update_fields=update_fields) except PreregistrationUser.DoesNotExist: - prereg_user = create_preregistration_user( - email, - realm, - password_required=password_required, - full_name=full_name, - full_name_validated=full_name_validated, - multiuse_invite=multiuse_obj, - ) + existing_prereg_user = None + + # password_required and full_name data passed here as argument should take precedence + # over the defaults with which the existing PreregistrationUser that we've just fetched + # was created. + prereg_user = create_preregistration_user( + email, + realm, + password_required=password_required, + full_name=full_name, + full_name_validated=full_name_validated, + multiuse_invite=multiuse_obj, + ) + streams_to_subscribe = None if multiuse_obj is not None: + # If the user came here explicitly via a multiuse invite link, then + # we use the defaults implied by the invite. streams_to_subscribe = list(multiuse_obj.streams.all()) + elif existing_prereg_user: + # Otherwise, the user is doing this signup not via any invite link, + # but we can use the pre-existing PreregistrationUser for these values + # since it tells how they were intended to be, when the user was invited. + streams_to_subscribe = list(existing_prereg_user.streams.all()) + invited_as = existing_prereg_user.invited_as + + if streams_to_subscribe: prereg_user.streams.set(streams_to_subscribe) - prereg_user.invited_as = invited_as - prereg_user.multiuse_invite = multiuse_obj - prereg_user.save() + prereg_user.invited_as = invited_as + prereg_user.multiuse_invite = multiuse_obj + prereg_user.save() confirmation_link = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) if is_signup: