From 1a3441dbf5ec38a305105cdf9120f302b74d5e52 Mon Sep 17 00:00:00 2001 From: Anders Kaseorg Date: Sat, 13 Jun 2020 16:36:12 -0700 Subject: [PATCH] confirmation: Pass realm rather than host to confirmation_url. Signed-off-by: Anders Kaseorg --- analytics/tests/test_views.py | 2 +- analytics/views.py | 9 +------ confirmation/models.py | 23 ++++++++++-------- zerver/lib/actions.py | 10 ++++---- .../commands/generate_invite_links.py | 2 +- zerver/tests/test_auth_backends.py | 4 ++-- zerver/tests/test_email_change.py | 10 ++++---- zerver/tests/test_realm.py | 4 ++-- zerver/tests/test_signup.py | 24 +++++++++---------- zerver/views/auth.py | 17 +------------ zerver/views/development/email_log.py | 2 +- zerver/views/development/registration.py | 4 ++-- zerver/views/registration.py | 2 +- 13 files changed, 47 insertions(+), 66 deletions(-) diff --git a/analytics/tests/test_views.py b/analytics/tests/test_views.py index d7ad08dd9e..eb80ebedf0 100644 --- a/analytics/tests/test_views.py +++ b/analytics/tests/test_views.py @@ -461,7 +461,7 @@ class TestSupportEndpoint(ZulipTestCase): def check_realm_creation_query_result(result: HttpResponse, email: str) -> None: self.assert_in_success_response(['preregistration user\n', 'realm creation\n', - 'Link: http://zulip.testserver/accounts/do_confirm/', + 'Link: http://testserver/accounts/do_confirm/', 'Expires in: 1\xa0day
\n', ], result) diff --git a/analytics/views.py b/analytics/views.py index 0e158cc3e3..a73df62abd 100644 --- a/analytics/views.py +++ b/analytics/views.py @@ -1081,13 +1081,6 @@ def get_confirmations(types: List[int], object_ids: List[int], realm = confirmation.realm content_object = confirmation.content_object - if realm is not None: - realm_host = realm.host - elif isinstance(content_object, Realm): - realm_host = content_object.host - else: - realm_host = hostname - type = confirmation.type days_to_activate = _properties[type].validity_in_days expiry_date = confirmation.date_sent + timedelta(days=days_to_activate) @@ -1105,7 +1098,7 @@ def get_confirmations(types: List[int], object_ids: List[int], else: expires_in = "Expired" - url = confirmation_url(confirmation.confirmation_key, realm_host, type) + url = confirmation_url(confirmation.confirmation_key, realm, type) confirmation_dicts.append({"object": confirmation.content_object, "url": url, "type": type, "link_status": link_status, "expires_in": expires_in}) diff --git a/confirmation/models.py b/confirmation/models.py index 27d7d53a07..e6a5da9f9f 100644 --- a/confirmation/models.py +++ b/confirmation/models.py @@ -5,6 +5,7 @@ import datetime import string from random import SystemRandom from typing import Mapping, Optional, Union +from urllib.parse import urljoin from django.conf import settings from django.contrib.contenttypes.fields import GenericForeignKey @@ -63,7 +64,7 @@ def get_object_from_key(confirmation_key: str, obj.save(update_fields=['status']) return obj -def create_confirmation_link(obj: ContentType, host: str, +def create_confirmation_link(obj: ContentType, confirmation_type: int, url_args: Mapping[str, str] = {}) -> str: key = generate_key() @@ -75,15 +76,17 @@ def create_confirmation_link(obj: ContentType, host: str, Confirmation.objects.create(content_object=obj, date_sent=timezone_now(), confirmation_key=key, realm=realm, type=confirmation_type) - return confirmation_url(key, host, confirmation_type, url_args) + return confirmation_url(key, realm, confirmation_type, url_args) -def confirmation_url(confirmation_key: str, host: str, +def confirmation_url(confirmation_key: str, realm: Optional[Realm], confirmation_type: int, url_args: Mapping[str, str] = {}) -> str: url_args = dict(url_args) url_args['confirmation_key'] = confirmation_key - return '%s%s%s' % (settings.EXTERNAL_URI_SCHEME, host, - reverse(_properties[confirmation_type].url_name, kwargs=url_args)) + return urljoin( + settings.ROOT_DOMAIN_URI if realm is None else realm.uri, + reverse(_properties[confirmation_type].url_name, kwargs=url_args), + ) class Confirmation(models.Model): content_type = models.ForeignKey(ContentType, on_delete=CASCADE) @@ -135,7 +138,7 @@ def one_click_unsubscribe_link(user_profile: UserProfile, email_type: str) -> st Generate a unique link that a logged-out user can visit to unsubscribe from Zulip e-mails without having to first log in. """ - return create_confirmation_link(user_profile, user_profile.realm.host, + return create_confirmation_link(user_profile, Confirmation.UNSUBSCRIBE, url_args = {'email_type': email_type}) @@ -165,10 +168,10 @@ def generate_realm_creation_url(by_admin: bool=False) -> str: RealmCreationKey.objects.create(creation_key=key, date_created=timezone_now(), presume_email_valid=by_admin) - return '%s%s%s' % (settings.EXTERNAL_URI_SCHEME, - settings.EXTERNAL_HOST, - reverse('zerver.views.create_realm', - kwargs={'creation_key': key})) + return urljoin( + settings.ROOT_DOMAIN_URI, + reverse('zerver.views.create_realm', kwargs={'creation_key': key}), + ) class RealmCreationKey(models.Model): creation_key = models.CharField('activation key', db_index=True, max_length=40) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 681541df6f..e06ca39475 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -974,7 +974,7 @@ def do_start_email_change_process(user_profile: UserProfile, new_email: str) -> obj = EmailChangeStatus.objects.create(new_email=new_email, old_email=old_email, user_profile=user_profile, realm=user_profile.realm) - activation_url = create_confirmation_link(obj, user_profile.realm.host, Confirmation.EMAIL_CHANGE) + activation_url = create_confirmation_link(obj, Confirmation.EMAIL_CHANGE) from zerver.context_processors import common_context context = common_context(user_profile) context.update({ @@ -4956,7 +4956,7 @@ def do_send_confirmation_email(invitee: PreregistrationUser, """ Send the confirmation/welcome e-mail to an invited user. """ - activation_url = create_confirmation_link(invitee, referrer.realm.host, Confirmation.INVITATION) + activation_url = create_confirmation_link(invitee, Confirmation.INVITATION) context = {'referrer_full_name': referrer.full_name, 'referrer_email': referrer.delivery_email, 'activate_url': activation_url, 'referrer_realm_name': referrer.realm.name} from_name = f"{referrer.full_name} (via Zulip)" @@ -5149,7 +5149,7 @@ def do_get_user_invites(user_profile: UserProfile) -> List[Dict[str, Any]]: invited=datetime_to_timestamp(confirmation_obj.date_sent), id=invite.id, link_url=confirmation_url(confirmation_obj.confirmation_key, - user_profile.realm.host, + user_profile.realm, Confirmation.MULTIUSE_INVITE), invited_as=invite.invited_as, is_multiuse=True)) @@ -5164,7 +5164,7 @@ def do_create_multiuse_invite_link(referred_by: UserProfile, invited_as: int, invite.invited_as = invited_as invite.save() notify_invites_changed(referred_by) - return create_confirmation_link(invite, realm.host, Confirmation.MULTIUSE_INVITE) + return create_confirmation_link(invite, Confirmation.MULTIUSE_INVITE) def do_revoke_user_invite(prereg_user: PreregistrationUser) -> None: email = prereg_user.email @@ -5793,7 +5793,7 @@ def check_delete_user_group(user_group_id: int, user_profile: UserProfile) -> No do_send_delete_user_group_event(user_profile.realm, user_group_id, user_profile.realm.id) def do_send_realm_reactivation_email(realm: Realm) -> None: - url = create_confirmation_link(realm, realm.host, Confirmation.REALM_REACTIVATION) + url = create_confirmation_link(realm, Confirmation.REALM_REACTIVATION) context = {'confirmation_url': url, 'realm_uri': realm.uri, 'realm_name': realm.name} diff --git a/zerver/management/commands/generate_invite_links.py b/zerver/management/commands/generate_invite_links.py index cfb832469d..08f975ba14 100644 --- a/zerver/management/commands/generate_invite_links.py +++ b/zerver/management/commands/generate_invite_links.py @@ -54,5 +54,5 @@ class Command(ZulipBaseCommand): prereg_user = PreregistrationUser(email=email, realm=realm) prereg_user.save() - print(email + ": " + create_confirmation_link(prereg_user, realm.host, + print(email + ": " + create_confirmation_link(prereg_user, Confirmation.INVITATION)) diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 99b3950aa2..90c1228717 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -1256,7 +1256,7 @@ class SocialAuthBase(DesktopFlowTestingLib, ZulipTestCase): referrer = self.example_user("hamlet") multiuse_obj = MultiuseInvite.objects.create(realm=realm, referred_by=referrer) multiuse_obj.streams.set(streams) - create_confirmation_link(multiuse_obj, realm.host, Confirmation.MULTIUSE_INVITE) + create_confirmation_link(multiuse_obj, Confirmation.MULTIUSE_INVITE) multiuse_confirmation = Confirmation.objects.all().last() multiuse_object_key = multiuse_confirmation.confirmation_key account_data_dict = self.get_account_data_dict(email=email, name=name) @@ -2910,7 +2910,7 @@ class GoogleAuthBackendTest(SocialAuthBase): referrer = self.example_user("hamlet") multiuse_obj = MultiuseInvite.objects.create(realm=realm, referred_by=referrer) multiuse_obj.streams.set(streams) - create_confirmation_link(multiuse_obj, realm.host, Confirmation.MULTIUSE_INVITE) + create_confirmation_link(multiuse_obj, Confirmation.MULTIUSE_INVITE) multiuse_confirmation = Confirmation.objects.all().last() multiuse_object_key = multiuse_confirmation.confirmation_key diff --git a/zerver/tests/test_email_change.py b/zerver/tests/test_email_change.py index fc4a335d59..08dead6ffb 100644 --- a/zerver/tests/test_email_change.py +++ b/zerver/tests/test_email_change.py @@ -21,14 +21,14 @@ class EmailChangeTestCase(ZulipTestCase): def test_confirm_email_change_with_non_existent_key(self) -> None: self.login('hamlet') key = generate_key() - url = confirmation_url(key, 'testserver', Confirmation.EMAIL_CHANGE) + url = confirmation_url(key, None, Confirmation.EMAIL_CHANGE) response = self.client_get(url) self.assert_in_success_response(["Whoops. We couldn't find your confirmation link in the system."], response) def test_confirm_email_change_with_invalid_key(self) -> None: self.login('hamlet') key = 'invalid_key' - url = confirmation_url(key, 'testserver', Confirmation.EMAIL_CHANGE) + url = confirmation_url(key, None, Confirmation.EMAIL_CHANGE) response = self.client_get(url) self.assert_in_success_response(["Whoops. The confirmation link is malformed."], response) @@ -47,7 +47,7 @@ class EmailChangeTestCase(ZulipTestCase): date_sent=date_sent, confirmation_key=key, type=Confirmation.EMAIL_CHANGE) - url = confirmation_url(key, user_profile.realm.host, Confirmation.EMAIL_CHANGE) + url = confirmation_url(key, user_profile.realm, Confirmation.EMAIL_CHANGE) response = self.client_get(url) self.assert_in_success_response(["The confirmation link has expired or been deactivated."], response) @@ -72,7 +72,7 @@ class EmailChangeTestCase(ZulipTestCase): date_sent=now(), confirmation_key=key, type=Confirmation.EMAIL_CHANGE) - url = confirmation_url(key, user_profile.realm.host, Confirmation.EMAIL_CHANGE) + url = confirmation_url(key, user_profile.realm, Confirmation.EMAIL_CHANGE) response = self.client_get(url) self.assertEqual(response.status_code, 200) @@ -206,7 +206,7 @@ class EmailChangeTestCase(ZulipTestCase): date_sent=now(), confirmation_key=key, type=Confirmation.EMAIL_CHANGE) - url = confirmation_url(key, user_profile.realm.host, Confirmation.EMAIL_CHANGE) + url = confirmation_url(key, user_profile.realm, Confirmation.EMAIL_CHANGE) response = self.client_get(url) self.assertEqual(response.status_code, 200) diff --git a/zerver/tests/test_realm.py b/zerver/tests/test_realm.py index a9d3b6cee8..a4184a6e1d 100644 --- a/zerver/tests/test_realm.py +++ b/zerver/tests/test_realm.py @@ -223,7 +223,7 @@ class RealmTest(ZulipTestCase): realm = get_realm('zulip') do_deactivate_realm(realm) self.assertTrue(realm.deactivated) - confirmation_url = create_confirmation_link(realm, realm.host, Confirmation.REALM_REACTIVATION) + confirmation_url = create_confirmation_link(realm, Confirmation.REALM_REACTIVATION) response = self.client_get(confirmation_url) self.assert_in_success_response(['Your organization has been successfully reactivated'], response) realm = get_realm('zulip') @@ -233,7 +233,7 @@ class RealmTest(ZulipTestCase): realm = get_realm('zulip') do_deactivate_realm(realm) self.assertTrue(realm.deactivated) - create_confirmation_link(realm, realm.host, Confirmation.REALM_REACTIVATION) + create_confirmation_link(realm, Confirmation.REALM_REACTIVATION) confirmation = Confirmation.objects.last() self.assertEqual(confirmation.content_object, realm) self.assertEqual(confirmation.realm, realm) diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index 84f6c796cd..b948123d25 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -1420,7 +1420,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" data = {"email": invitee_email, "referrer_email": current_user.email} invitee = PreregistrationUser.objects.get(email=data["email"]) referrer = self.example_user(referrer_name) - link = create_confirmation_link(invitee, referrer.realm.host, Confirmation.INVITATION) + link = create_confirmation_link(invitee, Confirmation.INVITATION) context = common_context(referrer) context.update({ 'activate_url': link, @@ -1477,7 +1477,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" inviter = self.example_user('iago') prereg_user = PreregistrationUser.objects.create( email=email, referred_by=inviter, realm=realm) - url = create_confirmation_link(prereg_user, 'host', Confirmation.USER_REGISTRATION) + url = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) registration_key = url.split('/')[-1] # Mainly a test of get_object_from_key, rather than of the invitation pathway @@ -1486,7 +1486,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" self.assertEqual(cm.exception.error_type, ConfirmationKeyException.DOES_NOT_EXIST) # Verify that using the wrong type doesn't work in the main confirm code path - email_change_url = create_confirmation_link(prereg_user, 'host', Confirmation.EMAIL_CHANGE) + email_change_url = create_confirmation_link(prereg_user, Confirmation.EMAIL_CHANGE) email_change_key = email_change_url.split('/')[-1] url = '/accounts/do_confirm/' + email_change_key result = self.client_get(url) @@ -1499,7 +1499,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" inviter = self.example_user('iago') prereg_user = PreregistrationUser.objects.create( email=email, referred_by=inviter, realm=realm) - url = create_confirmation_link(prereg_user, 'host', Confirmation.USER_REGISTRATION) + url = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) registration_key = url.split('/')[-1] conf = Confirmation.objects.filter(confirmation_key=registration_key).first() @@ -1557,7 +1557,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" inviter = self.example_user('iago') prereg_user = PreregistrationUser.objects.create( email=email, referred_by=inviter, realm=realm) - confirmation_link = create_confirmation_link(prereg_user, 'host', Confirmation.USER_REGISTRATION) + confirmation_link = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) registration_key = 'invalid_confirmation_key' url = '/accounts/register/' @@ -1579,7 +1579,7 @@ so we didn't send them an invitation. We did send invitations to everyone else!" prereg_user = PreregistrationUser.objects.create( email=email, referred_by=inviter, realm=realm) - confirmation_link = create_confirmation_link(prereg_user, 'host', Confirmation.USER_REGISTRATION) + confirmation_link = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) registration_key = confirmation_link.split('/')[-1] url = "/accounts/register/" @@ -1640,10 +1640,10 @@ class InvitationsTestCase(InviteUserBase): othello = self.example_user('othello') multiuse_invite_one = MultiuseInvite.objects.create(referred_by=hamlet, realm=realm) - create_confirmation_link(multiuse_invite_one, realm.host, Confirmation.MULTIUSE_INVITE) + create_confirmation_link(multiuse_invite_one, Confirmation.MULTIUSE_INVITE) multiuse_invite_two = MultiuseInvite.objects.create(referred_by=othello, realm=realm) - create_confirmation_link(multiuse_invite_two, realm.host, Confirmation.MULTIUSE_INVITE) + create_confirmation_link(multiuse_invite_two, Confirmation.MULTIUSE_INVITE) confirmation = Confirmation.objects.last() confirmation.date_sent = expired_datetime confirmation.save() @@ -1730,7 +1730,7 @@ class InvitationsTestCase(InviteUserBase): zulip_realm = get_realm("zulip") multiuse_invite = MultiuseInvite.objects.create(referred_by=self.example_user("hamlet"), realm=zulip_realm) - create_confirmation_link(multiuse_invite, zulip_realm.host, Confirmation.MULTIUSE_INVITE) + create_confirmation_link(multiuse_invite, Confirmation.MULTIUSE_INVITE) result = self.client_delete('/json/invites/multiuse/' + str(multiuse_invite.id)) self.assertEqual(result.status_code, 200) self.assertIsNone(MultiuseInvite.objects.filter(id=multiuse_invite.id).first()) @@ -1741,7 +1741,7 @@ class InvitationsTestCase(InviteUserBase): # Test deleting multiuse invite from another realm mit_realm = get_realm("zephyr") multiuse_invite_in_mit = MultiuseInvite.objects.create(referred_by=self.mit_user("sipbtest"), realm=mit_realm) - create_confirmation_link(multiuse_invite_in_mit, mit_realm.host, Confirmation.MULTIUSE_INVITE) + create_confirmation_link(multiuse_invite_in_mit, Confirmation.MULTIUSE_INVITE) error_result = self.client_delete('/json/invites/multiuse/' + str(multiuse_invite_in_mit.id)) self.assert_json_error(error_result, "No such invitation") @@ -1852,7 +1852,7 @@ class InvitationsTestCase(InviteUserBase): prereg_user = PreregistrationUser.objects.create( email=email, referred_by=inviter, realm=realm) - confirmation_link = create_confirmation_link(prereg_user, 'host', Confirmation.USER_REGISTRATION) + confirmation_link = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) registration_key = confirmation_link.split('/')[-1] result = self.client_post( @@ -1922,7 +1922,7 @@ class MultiuseInviteTest(ZulipTestCase): Confirmation.objects.create(content_object=invite, date_sent=date_sent, confirmation_key=key, type=Confirmation.MULTIUSE_INVITE) - return confirmation_url(key, self.realm.host, Confirmation.MULTIUSE_INVITE) + return confirmation_url(key, self.realm, Confirmation.MULTIUSE_INVITE) def check_user_able_to_register(self, email: str, invite_link: str) -> None: password = "password" diff --git a/zerver/views/auth.py b/zerver/views/auth.py index 86fc3148bc..3ce3ff6fae 100644 --- a/zerver/views/auth.py +++ b/zerver/views/auth.py @@ -187,22 +187,7 @@ def maybe_send_to_registration(request: HttpRequest, email: str, full_name: str= prereg_user.invited_as = invited_as prereg_user.save() - # We want to create a confirmation link to create an account - # in the current realm, i.e. one with a hostname of - # realm.host. For the Apache REMOTE_USER_SSO auth code path, - # this is preferable over realm.get_host() because the latter - # contains the port number of the Apache instance and we want - # to send the user back to nginx. But if we're in the realm - # creation code path, there might not be a realm yet, so we - # have to use request.get_host(). - if realm is not None: - host = realm.host - else: - host = request.get_host() - # Mark 'host' as safe for use in a redirect. It's pulled from the - # current request or realm, both of which only allow a limited set of - # trusted hosts. - confirmation_link = create_confirmation_link(prereg_user, mark_sanitized(host), + confirmation_link = create_confirmation_link(prereg_user, Confirmation.USER_REGISTRATION) if is_signup: return redirect(confirmation_link) diff --git a/zerver/views/development/email_log.py b/zerver/views/development/email_log.py index bbfcbff344..40f8a9bc73 100755 --- a/zerver/views/development/email_log.py +++ b/zerver/views/development/email_log.py @@ -109,7 +109,7 @@ def generate_all_emails(request: HttpRequest) -> HttpResponse: # Email change successful key = Confirmation.objects.filter(type=Confirmation.EMAIL_CHANGE).latest('id').confirmation_key - url = confirmation_url(key, realm.host, Confirmation.EMAIL_CHANGE) + url = confirmation_url(key, realm, Confirmation.EMAIL_CHANGE) user_profile = get_user_by_delivery_email(registered_email, realm) result = client.get(url) assert result.status_code == 200 diff --git a/zerver/views/development/registration.py b/zerver/views/development/registration.py index 467ea77605..bc0a52ced8 100644 --- a/zerver/views/development/registration.py +++ b/zerver/views/development/registration.py @@ -31,7 +31,7 @@ def register_development_user(request: HttpRequest) -> HttpResponse: email = f'{name}@zulip.com' prereg = create_preregistration_user(email, request, realm_creation=False, password_required=False) - activation_url = create_confirmation_link(prereg, request.get_host(), + activation_url = create_confirmation_link(prereg, Confirmation.USER_REGISTRATION) key = activation_url.split('/')[-1] # Need to add test data to POST request as it doesnt originally contain the required parameters @@ -47,7 +47,7 @@ def register_development_realm(request: HttpRequest) -> HttpResponse: realm_name = 'realm-%d' % (count,) prereg = create_preregistration_user(email, request, realm_creation=True, password_required=False) - activation_url = create_confirmation_link(prereg, request.get_host(), + activation_url = create_confirmation_link(prereg, Confirmation.REALM_CREATION) key = activation_url.split('/')[-1] # Need to add test data to POST request as it doesnt originally contain the required parameters diff --git a/zerver/views/registration.py b/zerver/views/registration.py index 47fdc06e20..b095453db3 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -471,7 +471,7 @@ def prepare_activation_url(email: str, request: HttpRequest, if realm_creation: confirmation_type = Confirmation.REALM_CREATION - activation_url = create_confirmation_link(prereg_user, request.get_host(), confirmation_type) + activation_url = create_confirmation_link(prereg_user, confirmation_type) if settings.DEVELOPMENT and realm_creation: request.session['confirmation_key'] = {'confirmation_key': activation_url.split('/')[-1]} return activation_url