From feb2cdf378e4017ebb95c0f281f007c2f6bd2324 Mon Sep 17 00:00:00 2001 From: Shubham Padia Date: Fri, 10 Aug 2018 04:28:44 +0530 Subject: [PATCH] onboarding: Change logic for preventing new login emails for a new user. Issue: When you created a new organization with /new, the "new login" emails were emailed. We previously had a hack of adding the .just_registered property to the user Python object to attempt to prevent the emails, and checking that in zerver/signals.py. This commit gets rid of the .just_registered check. Instead of the .just_registered check, this checks if the user has joined more than a minute before. A test test_dont_send_login_emails_for_new_user_registration_logins already exists. Tweaked by tabbott to introduce the constant JUST_CREATED_THRESHOLD. Fixes #10179. --- zerver/signals.py | 4 +++- zerver/tests/test_auth_backends.py | 22 ++++++++++++++++++---- zerver/tests/test_new_users.py | 8 +++++--- zerver/views/registration.py | 3 --- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/zerver/signals.py b/zerver/signals.py index 3130c2915b..7d3a9aed02 100644 --- a/zerver/signals.py +++ b/zerver/signals.py @@ -15,6 +15,8 @@ from zerver.lib.send_email import FromAddress from zerver.models import UserProfile from zerver.lib.timezone import get_timezone +JUST_CREATED_THRESHOLD = 60 + def get_device_browser(user_agent: str) -> Optional[str]: user_agent = user_agent.lower() if "zulip" in user_agent: @@ -66,7 +68,7 @@ def email_on_new_login(sender: Any, user: UserProfile, request: Any, **kwargs: A if request: # If the user's account was just created, avoid sending an email. - if getattr(user, "just_registered", False): + if (timezone_now() - user.date_joined).total_seconds() <= JUST_CREATED_THRESHOLD: return user_agent = request.META.get('HTTP_USER_AGENT', "").lower() diff --git a/zerver/tests/test_auth_backends.py b/zerver/tests/test_auth_backends.py index 750487eaf0..d4a2390413 100644 --- a/zerver/tests/test_auth_backends.py +++ b/zerver/tests/test_auth_backends.py @@ -6,6 +6,7 @@ from django.test import override_settings from django_auth_ldap.backend import _LDAPUser from django.contrib.auth import authenticate from django.test.client import RequestFactory +from django.utils.timezone import now as timezone_now from typing import Any, Callable, Dict, List, Optional, Tuple from builtins import object from oauth2client.crypt import AppIdentityError @@ -19,6 +20,7 @@ import jwt import mock import re import time +import datetime from zerver.forms import HomepageForm from zerver.lib.actions import ( @@ -43,6 +45,7 @@ from zerver.lib.test_helpers import POSTRequestMock, HostRequestMock from zerver.models import \ get_realm, email_to_username, UserProfile, \ PreregistrationUser, Realm, get_user, MultiuseInvite +from zerver.signals import JUST_CREATED_THRESHOLD from confirmation.models import Confirmation, confirmation_url, create_confirmation_link @@ -657,6 +660,9 @@ class GitHubAuthBackendTest(ZulipTestCase): mobile_flow_otp = '1234abcd' * 8 account_data_dict = dict(email=self.email, name='Full Name') self.assertEqual(len(mail.outbox), 0) + self.user_profile.date_joined = timezone_now() - datetime.timedelta(seconds=JUST_CREATED_THRESHOLD + 1) + self.user_profile.save() + with self.settings(SEND_LOGIN_EMAILS=True): # Verify that the right thing happens with an invalid-format OTP result = self.github_oauth2_test(account_data_dict, subdomain='zulip', @@ -905,13 +911,17 @@ class GoogleSubdomainLoginTest(GoogleOAuthTest): self.assertTrue(uri.startswith('http://zulip.testserver/accounts/login/subdomain/')) def test_google_oauth2_mobile_success(self) -> None: + self.user_profile = self.example_user('hamlet') + self.user_profile.date_joined = timezone_now() - datetime.timedelta(seconds=JUST_CREATED_THRESHOLD + 1) + self.user_profile.save() mobile_flow_otp = '1234abcd' * 8 token_response = ResponseMock(200, {'access_token': "unique_token"}) account_data = dict(name=dict(formatted="Full Name"), emails=[dict(type="account", - value=self.example_email("hamlet"))]) + value=self.user_profile.email)]) account_response = ResponseMock(200, account_data) self.assertEqual(len(mail.outbox), 0) + with self.settings(SEND_LOGIN_EMAILS=True): # Verify that the right thing happens with an invalid-format OTP result = self.google_oauth2_test(token_response, account_response, subdomain='zulip', @@ -930,9 +940,9 @@ class GoogleSubdomainLoginTest(GoogleOAuthTest): 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")]) + self.assertEqual(query_params["email"], [self.user_profile.email]) encrypted_api_key = query_params["otp_encrypted_api_key"][0] - hamlet_api_keys = get_all_api_keys(self.example_user('hamlet')) + hamlet_api_keys = get_all_api_keys(self.user_profile) self.assertIn(otp_decrypt_api_key(encrypted_api_key, mobile_flow_otp), hamlet_api_keys) self.assertEqual(len(mail.outbox), 1) self.assertIn('Zulip on Android', mail.outbox[0].body) @@ -1354,6 +1364,8 @@ class FetchAPIKeyTest(ZulipTestCase): SEND_LOGIN_EMAILS=True) def test_google_oauth2_token_success(self) -> None: self.assertEqual(len(mail.outbox), 0) + self.user_profile.date_joined = timezone_now() - datetime.timedelta(seconds=JUST_CREATED_THRESHOLD + 1) + self.user_profile.save() with mock.patch( 'apiclient.sample_tools.client.verify_id_token', return_value={ @@ -1897,9 +1909,11 @@ class TestZulipRemoteUserBackend(ZulipTestCase): def test_login_mobile_flow_otp_success(self) -> None: user_profile = self.example_user('hamlet') email = user_profile.email + user_profile.date_joined = timezone_now() - datetime.timedelta(seconds=61) + user_profile.save() mobile_flow_otp = '1234abcd' * 8 - # Verify that the right thing happens with an invalid-format OTP + # Verify that the right thing happens with an invalid-format OTP result = self.client_post('/accounts/login/sso/', dict(mobile_flow_otp="1234"), REMOTE_USER=email, diff --git a/zerver/tests/test_new_users.py b/zerver/tests/test_new_users.py index 32b21e2e61..e48f7b53ab 100644 --- a/zerver/tests/test_new_users.py +++ b/zerver/tests/test_new_users.py @@ -3,7 +3,7 @@ from django.conf import settings from django.core import mail from django.contrib.auth.signals import user_logged_in from zerver.lib.test_classes import ZulipTestCase -from zerver.signals import get_device_browser, get_device_os +from zerver.signals import get_device_browser, get_device_os, JUST_CREATED_THRESHOLD from zerver.lib.actions import notify_new_user from zerver.models import Recipient, Stream, Realm from zerver.lib.initial_password import initial_password @@ -25,14 +25,16 @@ class SendLoginEmailTest(ZulipTestCase): with self.settings(SEND_LOGIN_EMAILS=True): self.assertTrue(settings.SEND_LOGIN_EMAILS) # we don't use the self.login method since we spoof the user-agent + utc = get_timezone('utc') + mock_time = datetime.datetime(year=2018, month=1, day=1, tzinfo=utc) + user = self.example_user('hamlet') user.timezone = 'US/Pacific' + user.date_joined = mock_time - datetime.timedelta(seconds=JUST_CREATED_THRESHOLD + 1) user.save() password = initial_password(user.email) firefox_windows = "Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:47.0) Gecko/20100101 Firefox/47.0" - utc = get_timezone('utc') user_tz = get_timezone(user.timezone) - mock_time = datetime.datetime(year=2018, month=1, day=1, tzinfo=utc) utc_offset = mock_time.astimezone(user_tz).strftime('%z') reference_time = mock_time.astimezone(user_tz).strftime('%A, %B %d, %Y at %I:%M%p ') + utc_offset with mock.patch('zerver.signals.timezone_now', return_value=mock_time): diff --git a/zerver/views/registration.py b/zerver/views/registration.py index cd99e19250..b1774804f3 100644 --- a/zerver/views/registration.py +++ b/zerver/views/registration.py @@ -335,9 +335,6 @@ def accounts_register(request: HttpRequest) -> HttpResponse: ) def login_and_go_to_home(request: HttpRequest, user_profile: UserProfile) -> HttpResponse: - - # Mark the user as having been just created, so no "new login" email is sent - user_profile.just_registered = True do_login(request, user_profile) return HttpResponseRedirect(user_profile.realm.uri + reverse('zerver.views.home.home'))