signup: Mirror dummy user should be registered with role from invite.

Aside of what's generally explained in the code comment, this is
motivated by the specific situation of import of Slack Connect channels.
These channels contain users who are "external collaborators" and
limited to a single channel in Slack. We don't have more sophisticated
handling of their import, which would map this concept 1-to-1 in Zulip -
but we create them as inactive dummy users, meaning they have to go
through signup before their account is usable.

The issue is that their imported UserProfile.role is set to Member and
when they register, the UserProfile gets reactivated with that role
unchanged. However, if e.g. the user is signing up after they received
an invitation from the admin, they should get the role that was
configured on the invite. In particular important if the user is meant
to still be "limited" and thus the admin invites them as a guest - they
definitely don't want the user to get a full Member account because of
this weird interaction between import and registration.

zerver/tests/test_signup.py

@@ -22,6 +22,7 @@ from confirmation.models import Confirmation, one_click_unsubscribe_link
 from zerver.actions.create_realm import do_change_realm_subdomain, do_create_realm
 from zerver.actions.create_user import add_new_user_history, do_create_user
 from zerver.actions.default_streams import do_add_default_stream, do_create_default_stream_group
+from zerver.actions.invites import do_invite_users
 from zerver.actions.realm_settings import (
     do_deactivate_realm,
     do_set_realm_authentication_methods,
@@ -4071,6 +4072,42 @@ class UserSignUpTest(ZulipTestCase):
                 result,
             )
 
+    def test_registration_of_mirror_dummy_user_role_comes_from_invite(self) -> None:
+        """
+        Verify that when a mirror dummy user does their registration, their role is set
+        based on the value from the invite rather than the original role set on the UserProfile.
+        """
+
+        admin = self.example_user("iago")
+        realm = get_realm("zulip")
+        mirror_dummy = self.example_user("hamlet")
+        do_deactivate_user(mirror_dummy, acting_user=admin)
+        mirror_dummy.is_mirror_dummy = True
+        mirror_dummy.role = UserProfile.ROLE_MEMBER
+        mirror_dummy.save()
+
+        # Invite the user as a guest.
+        with self.captureOnCommitCallbacks(execute=True):
+            do_invite_users(
+                admin,
+                [mirror_dummy.delivery_email],
+                [],
+                invite_expires_in_minutes=None,
+                include_realm_default_subscriptions=True,
+                invite_as=PreregistrationUser.INVITE_AS["GUEST_USER"],
+            )
+
+        result = self.submit_reg_form_for_user(
+            mirror_dummy.delivery_email, "testpassword", full_name="New Hamlet"
+        )
+
+        # Verify that we successfully registered and that the role is set correctly.
+        self.assertEqual(result.status_code, 302)
+        self.assertEqual(result["Location"], f"{realm.url}/")
+        self.assert_logged_in_user_id(mirror_dummy.id)
+        mirror_dummy.refresh_from_db()
+        self.assertEqual(mirror_dummy.role, UserProfile.ROLE_GUEST)
+
     @patch(
         "DNS.dnslookup",
         return_value=[["sipbtest:*:20922:101:Fred Sipb,,,:/mit/sipbtest:/bin/athena/tcsh"]],

zerver/views/registration.py

@@ -10,6 +10,7 @@ from django.contrib.auth import REDIRECT_FIELD_NAME, authenticate, get_backends
 from django.contrib.sessions.backends.base import SessionBase
 from django.core import validators
 from django.core.exceptions import ValidationError
+from django.db import transaction
 from django.db.models import Q
 from django.db.utils import IntegrityError
 from django.http import HttpRequest, HttpResponse, HttpResponseRedirect
@@ -38,6 +39,7 @@ from zerver.actions.user_settings import (
     do_change_password,
     do_change_user_setting,
 )
+from zerver.actions.users import do_change_user_role
 from zerver.context_processors import (
     get_realm_create_form_context,
     get_realm_from_request,
@@ -626,7 +628,19 @@ def registration_helper(
 
         if existing_user_profile is not None and existing_user_profile.is_mirror_dummy:
             user_profile = existing_user_profile
-            do_activate_mirror_dummy_user(user_profile, acting_user=user_profile)
+            with transaction.atomic(durable=True):
+                # We reactivate the mirror dummy user, but we need to respect the role
+                # that was passed in via the PreregistrationUser. Mirror dummy users can
+                # come from data import from 3rd party tools, with their role not necessarily
+                # set to what the realm admins may want. Such users have to explicitly go through
+                # the sign up flow, and thus their final role should match what the administrators
+                # may have set in the invitation, to avoid surprises.
+                #
+                # It's also important to reactivate the account and adjust the role in a single
+                # transaction, to avoid security issues if the process is interrupted halfway,
+                # e.g. leaving the user reactivated but with the wrong role.
+                do_activate_mirror_dummy_user(user_profile, acting_user=user_profile)
+                do_change_user_role(user_profile, role, acting_user=user_profile)
             do_change_password(user_profile, password)
             do_change_full_name(user_profile, full_name, user_profile)
             do_change_user_setting(user_profile, "timezone", timezone, acting_user=user_profile)
@@ -636,8 +650,6 @@ def registration_helper(
                 get_default_language_for_new_user(realm, request=request),
                 acting_user=None,
             )
-            # TODO: When we clean up the `do_activate_mirror_dummy_user` code path,
-            # make it respect invited_as_admin / is_realm_admin.
 
         if user_profile is None:
             try: