From 50a2a54393e378c8cada16a2c815522a4d14aa8e Mon Sep 17 00:00:00 2001 From: Alex Vandiver Date: Wed, 15 Mar 2023 17:13:28 +0000 Subject: [PATCH] test_invite: Rework and expand invitation limit tests. This adds tests for more corner cases, in exchange for dropping the query count tests, which were of dubious utility. It also adds the time-machine library to mock the current time to test that the limits do expire. --- requirements/dev.in | 3 + requirements/dev.txt | 56 ++++++++++ zerver/tests/test_invite.py | 210 +++++++++++++++++++----------------- 3 files changed, 168 insertions(+), 101 deletions(-) diff --git a/requirements/dev.in b/requirements/dev.in index 2044878a66..981944438d 100644 --- a/requirements/dev.in +++ b/requirements/dev.in @@ -72,3 +72,6 @@ natsort # For spell check linter codespell + +# For mocking time +time-machine diff --git a/requirements/dev.txt b/requirements/dev.txt index 63723e028a..2e58996671 100644 --- a/requirements/dev.txt +++ b/requirements/dev.txt @@ -1739,6 +1739,7 @@ python-dateutil==2.8.2 \ # arrow # botocore # moto + # time-machine python-debian==0.1.49 \ --hash=sha256:880f3bc52e31599f2a9b432bd7691844286825087fccdcf2f6ffd5cd79a26f9f \ --hash=sha256:8cf677a30dbcb4be7a99536c17e11308a827a4d22028dc59a67f6c6dd3f0f58c @@ -2252,6 +2253,61 @@ tblib==1.7.0 \ testslide==2.7.0 \ --hash=sha256:23ece0b9f5b704b33b978e9c8797936034516adc3c4743ebdaed664aa700c3cb # via pyre-check +time-machine==2.9.0 \ + --hash=sha256:010a58a8de1120308befae19e6c9de2ef5ca5206635cea33cb264998725cc027 \ + --hash=sha256:0b9c36240876622b7f2f9e11bf72f100857c0a1e1a59af2da3d5067efea62c37 \ + --hash=sha256:1d0ab46ce8a60baf9d86525694bf698fed9efefd22b8cbe1ca3e74abbb3239e1 \ + --hash=sha256:2f080f6f7ca8cfca43bc5639288aebd0a273b4b5bd0acff609c2318728b13a18 \ + --hash=sha256:359c806e5b9a7a3c73dbb808d19dca297f5504a5eefdc5d031db8d918f43e364 \ + --hash=sha256:36dde844d28549929fab171d683c28a8db1c206547bcf6b7aca77319847d2046 \ + --hash=sha256:372a97da01db89533d2f4ce50bbd908e5c56df7b8cfd6a005b177d0b14dc2938 \ + --hash=sha256:3ce445775fcf7cb4040cfdba4b7c4888e7fd98bbcccfe1dc3fa8a798ed1f1d24 \ + --hash=sha256:3ff5148e2e73392db8418a1fe2f0b06f4a0e76772933502fb61e4c3000b5324e \ + --hash=sha256:49df5eea2160068e5b2bf28c22fc4c5aea00862ad88ddc3b62fc0f0683e97538 \ + --hash=sha256:4b55654aaeaba380fcd6c004b8ada2978fdd4ece1e61e6b9717c6d4cc7fbbcd9 \ + --hash=sha256:4f3755d9342ca1f1019418db52072272dfd75eb818fa4726fa8aabe208b38c26 \ + --hash=sha256:5657e0e6077cf15b37f0d8cf78e868113bbb3ecccc60064c40fe52d8166ca8b1 \ + --hash=sha256:60222d43f6e93a926adc36ed37a54bc8e4d0d8d1c4d449096afcfe85086129c2 \ + --hash=sha256:6211beee9f5dace08b1bbbb1fb09e34a69c52d87eea676729f14c8660481dff6 \ + --hash=sha256:6463e302c96eb8c691c4340e281bd54327a213b924fa189aea81accf7e7f78df \ + --hash=sha256:68ec8b83197db32c7a12da5f6b83c91271af3ed7f5dc122d2900a8de01dff9f0 \ + --hash=sha256:69898aed9b2315a90f5855343d9aa34d05fa06032e2e3bb14f2528941ec89dc1 \ + --hash=sha256:6b632d60aa0883dc7292ac3d32050604d26ec2bbd5c4d42fb0de3b4ef17343e2 \ + --hash=sha256:728263611d7940fda34d21573bd2b3f1491bdb52dbf75c5fe6c226dfe4655201 \ + --hash=sha256:748d701228e646c224f2adfa6a11b986cd4aa90f1b8c13ef4534a3919c796bc0 \ + --hash=sha256:8367fd03f2d7349c7fc20f14de186974eaca2502c64b948212de663742c8fd11 \ + --hash=sha256:8670cb5cfda99f483d60de6ce56ceb0ec5d359193e79e4688e1c3c9db3937383 \ + --hash=sha256:8830510adbf0a231184da277db9de1d55ef93ed228a575d217aaee295505abf1 \ + --hash=sha256:8976b7b1f7de13598b655d459f5640f90f3cd587283e1b914a22e45946c5485b \ + --hash=sha256:8bcc86b5a07ea9745f26dfad958dde0a4f56748c2ae0c9a96200a334d1b55055 \ + --hash=sha256:8e2a90b8300812d8d774f2d2fc216fec3c7d94132ac589e062489c395061f16c \ + --hash=sha256:8e797e5a2a99d1b237183e52251abfc1ad85c376278b39d1aca76a451a97861a \ + --hash=sha256:948ca690f9770ad4a93fa183061c11346505598f5f0b721965bc85ec83bb103d \ + --hash=sha256:9ba5fc2655749066d68986de8368984dad4082db2fbeade78f40506dc5b65672 \ + --hash=sha256:9ee553f7732fa51e019e3329a6984593184c4e0410af1e73d91ce38a5d4b34ab \ + --hash=sha256:a2cf80e5deaaa68c6cefb25303a4c870490b4e7591ed8e2435a65728920bc097 \ + --hash=sha256:ae4e3f02ab5dabb35adca606237c7e1a515c86d69c0b7092bbe0e1cfe5cffc61 \ + --hash=sha256:b16a2129f9146faa080bfd1b53447761f7386ec5c72890c827a65f33ab200336 \ + --hash=sha256:b32addbf56639a9a8261fb62f8ea83473447671c83ca2c017ab1eabf4841157f \ + --hash=sha256:b8faff03231ee55d5a216ce3e9171c5205459f866f54d4b5ee8aa1d860e4ce11 \ + --hash=sha256:bb15b2b79b00d3f6cf7d62096f5e782fa740ecedfe0540c09f1d1e4d3d7b81ba \ + --hash=sha256:bdbe785e046d124f73cca603ee37d5fae0b15dc4c13702488ad19de56aae08ba \ + --hash=sha256:bfa82614a98ecee70272bb6038d210b2ad7b2a6b8a678b400c34bdaf776802a7 \ + --hash=sha256:c01dbc3671d0649023daf623e952f9f0b4d904d57ab546d6d35a4aeb14915e8d \ + --hash=sha256:c5dbc8b87cdc7be070a499f2bd1cd405c7f647abeb3447dfd397639df040bc64 \ + --hash=sha256:cb51432652ad663b4cbd631c73c90f9e94f463382b86c0b6b854173700512a70 \ + --hash=sha256:cc6bf01211b5ea40f633d5502c5aa495b415ebaff66e041820997dae70a508e1 \ + --hash=sha256:d329578abe47ce95baa015ef3825acebb1b73b5fa6f818fdf2d4685a00ca457f \ + --hash=sha256:d4380bd6697cc7db3c9e6843f24779ac0550affa9d9a8e5f9e5d5cc139cb6583 \ + --hash=sha256:d79d374e32488c76cdb06fbdd4464083aeaa715ddca3e864bac7c7760eb03729 \ + --hash=sha256:eaf334477bc0a9283d5150a56be8670a07295ef676e5b5a7f086952929d1a56b \ + --hash=sha256:f6e79643368828d4651146a486be5a662846ac223ab5e2c73ddd519acfcc243c \ + --hash=sha256:f92d5d2eb119a6518755c4c9170112094c706d1c604460f50afc1308eeb97f0e \ + --hash=sha256:f97ed8bc5b517844a71030f74e9561de92f4902c306e6ccc8331a5b0c8dd0e00 \ + --hash=sha256:fcdef7687aed5c4331c9808f4a414a41987441c3e7a2ba554e4dccfa4218e788 \ + --hash=sha256:fd72c0b2e7443fff6e4481991742b72c17f73735e5fdd176406ca48df187a5c9 \ + --hash=sha256:fe013942ab7f3241fcbe66ee43222d47f499d1e0cb69e913791c52e638ddd7f0 + # via -r requirements/dev.in tinycss2==1.2.1 \ --hash=sha256:2b80a96d41e7c3914b8cda8bc7f705a4d9c49275616e886103dd839dfc847847 \ --hash=sha256:8cff3a8f066c2ec677c06dbc7b45619804a6938478d9d73c284b29d14ecb0627 diff --git a/zerver/tests/test_invite.py b/zerver/tests/test_invite.py index 0ab2db5d93..0c849eb2a0 100644 --- a/zerver/tests/test_invite.py +++ b/zerver/tests/test_invite.py @@ -6,6 +6,7 @@ from unittest.mock import patch from urllib.parse import urlencode import orjson +import time_machine from django.conf import settings from django.core import mail from django.core.mail.message import EmailMultiAlternatives @@ -28,9 +29,7 @@ from zerver.actions.invites import ( do_invite_users, do_revoke_multi_use_invite, ) -from zerver.actions.realm_settings import ( - do_set_realm_property, -) +from zerver.actions.realm_settings import do_set_realm_property from zerver.actions.user_settings import do_change_full_name from zerver.actions.users import change_user_is_active from zerver.context_processors import common_context @@ -40,11 +39,7 @@ from zerver.lib.send_email import ( send_future_email, ) from zerver.lib.test_classes import ZulipTestCase -from zerver.lib.test_helpers import ( - cache_tries_captured, - find_key_by_email, - queries_captured, -) +from zerver.lib.test_helpers import find_key_by_email from zerver.models import ( Message, MultiuseInvite, @@ -66,7 +61,7 @@ if TYPE_CHECKING: class InviteUserBase(ZulipTestCase): - def check_sent_emails(self, correct_recipients: List[str]) -> None: + def check_sent_emails(self, correct_recipients: List[str], clear: bool = False) -> None: self.assert_length(mail.outbox, len(correct_recipients)) email_recipients = [email.recipients()[0] for email in mail.outbox] self.assertEqual(sorted(email_recipients), sorted(correct_recipients)) @@ -80,7 +75,8 @@ class InviteUserBase(ZulipTestCase): self.email_display_from(mail.outbox[0]), rf" <{self.TOKENIZED_NOREPLY_REGEX}>\Z" ) - self.assertEqual(mail.outbox[0].extra_headers["List-Id"], "Zulip Dev ") + if clear: + mail.outbox = [] def invite( self, @@ -89,6 +85,7 @@ class InviteUserBase(ZulipTestCase): invite_expires_in_minutes: Optional[int] = INVITATION_LINK_VALIDITY_MINUTES, body: str = "", invite_as: int = PreregistrationUser.INVITE_AS["MEMBER"], + realm: Optional[Realm] = None, ) -> "TestHttpResponse": """ Invites the specified users to Zulip with the specified streams. @@ -100,7 +97,7 @@ class InviteUserBase(ZulipTestCase): """ stream_ids = [] for stream_name in stream_names: - stream_ids.append(self.get_stream_id(stream_name)) + stream_ids.append(self.get_stream_id(stream_name, realm=realm)) invite_expires_in: Union[str, Optional[int]] = invite_expires_in_minutes if invite_expires_in is None: @@ -114,6 +111,7 @@ class InviteUserBase(ZulipTestCase): "stream_ids": orjson.dumps(stream_ids).decode(), "invite_as": invite_as, }, + subdomain=realm.string_id if realm else "zulip", ) @@ -153,93 +151,121 @@ class InviteUserTest(InviteUserBase): def test_invite_limits(self) -> None: user_profile = self.example_user("hamlet") realm = user_profile.realm - stream_name = "Denmark" - - # These constants only need to be in descending order - # for this test to trigger an InvitationError based - # on max daily counts. - site_max = 50 - realm_max = 40 - num_invitees = 30 - max_daily_count = 20 - - daily_counts = [(1, max_daily_count)] - - invite_emails = [f"foo-{i:02}@zulip.com" for i in range(num_invitees)] - invitees = ",".join(invite_emails) - self.login_user(user_profile) - realm.max_invites = realm_max + def try_invite( + num_invitees: int, + *, + default_realm_max: int, + new_realm_max: int, + realm_max: int, + open_realm_creation: bool = True, + realm: Optional[Realm] = None, + stream_name: str = "Denmark", + ) -> "TestHttpResponse": + if realm is None: + realm = get_realm("zulip") + invitees = ",".join( + [f"{realm.string_id}-{i:02}@zulip.com" for i in range(num_invitees)] + ) + with self.settings( + OPEN_REALM_CREATION=open_realm_creation, + INVITES_DEFAULT_REALM_DAILY_MAX=default_realm_max, + INVITES_NEW_REALM_LIMIT_DAYS=[(1, new_realm_max)], + ): + realm.max_invites = realm_max + realm.save() + return self.invite(invitees, [stream_name], realm=realm) + + # Trip the "new realm" limits realm.date_created = timezone_now() realm.save() - - def try_invite() -> "TestHttpResponse": - with self.settings( - OPEN_REALM_CREATION=True, - INVITES_DEFAULT_REALM_DAILY_MAX=site_max, - INVITES_NEW_REALM_LIMIT_DAYS=daily_counts, - ): - result = self.invite(invitees, [stream_name]) - return result - - result = try_invite() + result = try_invite(30, default_realm_max=50, new_realm_max=20, realm_max=40) self.assert_json_error_contains(result, "reached the limit") + self.check_sent_emails([]) - # Next show that aggregate limits expire once the realm is old - # enough. + # If some other realm consumes some invites, it affects our realm. Invite 20 users in lear: + lear_realm = get_realm("lear") + lear_realm.date_created = timezone_now() + lear_realm.save() + self.login_user(self.lear_user("king")) + result = try_invite( + 20, + default_realm_max=50, + new_realm_max=20, + realm_max=40, + realm=lear_realm, + stream_name="general", + ) + self.assert_json_success(result) + self.check_sent_emails([f"lear-{i:02}@zulip.com" for i in range(20)], clear=True) + # Which prevents inviting 1 in our realm: + self.login_user(user_profile) + result = try_invite(1, default_realm_max=50, new_realm_max=20, realm_max=40) + self.assert_json_error_contains(result, "reached the limit") + self.check_sent_emails([]) + + # If our realm max is over the default realm's, we're exempt from INVITES_NEW_REALM_LIMIT_DAYS + result = try_invite(10, default_realm_max=15, new_realm_max=5, realm_max=20) + self.assert_json_success(result) + self.check_sent_emails([f"zulip-{i:02}@zulip.com" for i in range(10)], clear=True) + + # We've sent 10 invites. Trying to invite 15 people, even if + # 10 of them are the same, still trips the limit (10 previous + # + 15 in this submission > 20 realm max) + result = try_invite(15, default_realm_max=15, new_realm_max=5, realm_max=20) + self.assert_json_error_contains(result, "reached the limit") + self.check_sent_emails([]) + + # Inviting 10 more people (to the realm max of 20) works, and + # sends emails to the same 10 users again. + result = try_invite(10, default_realm_max=15, new_realm_max=5, realm_max=20) + self.assert_json_success(result) + self.check_sent_emails([f"zulip-{i:02}@zulip.com" for i in range(10)], clear=True) + + # We've sent 20 invites. The 10 we just sent do count against + # us if we send to them again, since we did send mail + result = try_invite(10, default_realm_max=15, new_realm_max=5, realm_max=20) + self.assert_json_error_contains(result, "reached the limit") + self.check_sent_emails([]) + + # We've sent 20 invites. The realm is exempt from the new realm max + # (INVITES_NEW_REALM_LIMIT_DAYS) if it is old enough realm.date_created = timezone_now() - datetime.timedelta(days=8) realm.save() - - with queries_captured() as queries: - with cache_tries_captured() as cache_tries: - result = try_invite() - + result = try_invite(10, default_realm_max=50, new_realm_max=20, realm_max=40) self.assert_json_success(result) + self.check_sent_emails([f"zulip-{i:02}@zulip.com" for i in range(10)], clear=True) - # TODO: Fix large query count here. - # - # TODO: There is some test OTHER than this one - # that is leaking some kind of state change - # that throws off the query count here. It - # is hard to investigate currently (due to - # the large number of queries), so I just - # use an approximate equality check. - actual_count = len(queries) - expected_count = 251 - if abs(actual_count - expected_count) > 1: - raise AssertionError( - f""" - Unexpected number of queries: - - expected query count: {expected_count} - actual: {actual_count} - """ - ) - - # Almost all of these cache hits are to re-fetch each one of the - # invitees. These happen inside our queue processor for sending - # confirmation emails, so they are somewhat difficult to avoid. - # - # TODO: Mock the call to queue_json_publish, so we can measure the - # queue impact separately from the user-perceived impact. - self.assert_length(cache_tries, 32) - - # Next get line coverage on bumping a realm's max_invites. - realm.date_created = timezone_now() - realm.max_invites = site_max + 10 - realm.save() - - result = try_invite() + # We've sent 30 invites. None of the limits matter if open + # realm creation is disabled. + result = try_invite( + 10, default_realm_max=30, new_realm_max=20, realm_max=10, open_realm_creation=False + ) self.assert_json_success(result) + self.check_sent_emails([f"zulip-{i:02}@zulip.com" for i in range(10)], clear=True) - # Finally get coverage on the case that OPEN_REALM_CREATION is False. + # We've sent 40 invites "today". Fast-forward 48 hours + # and ensure that we can invite more people + with time_machine.travel(timezone_now() + datetime.timedelta(hours=48)): + result = try_invite(5, default_realm_max=30, new_realm_max=20, realm_max=10) + self.assert_json_success(result) + self.check_sent_emails([f"zulip-{i:02}@zulip.com" for i in range(5)], clear=True) - with self.settings(OPEN_REALM_CREATION=False): - result = self.invite(invitees, [stream_name]) + # We've sent 5 invites. Ensure we can trip the fresh "today" limit for the realm + result = try_invite(10, default_realm_max=30, new_realm_max=20, realm_max=10) + self.assert_json_error_contains(result, "reached the limit") + self.check_sent_emails([]) - self.assert_json_success(result) + # We've sent 5 invites. Reset the realm to be "recently" + # created, and ensure that we can trip the whole-server + # limit + realm.date_created = timezone_now() - datetime.timedelta(days=3) + realm.save() + result = try_invite(10, default_realm_max=50, new_realm_max=10, realm_max=40) + self.assert_json_error_contains(result, "reached the limit") + self.check_sent_emails([]) def test_invite_user_to_realm_on_manual_license_plan(self) -> None: user = self.example_user("hamlet") @@ -713,24 +739,6 @@ earl-test@zulip.com""", realm.max_invites = settings.INVITES_DEFAULT_REALM_DAILY_MAX realm.save() - def test_invite_too_many_users(self) -> None: - # Only a light test of this pathway; e.g. doesn't test that - # the limit gets reset after 24 hours - self.login("iago") - invitee_emails = "1@zulip.com, 2@zulip.com" - self.invite(invitee_emails, ["Denmark"]) - self.check_sent_emails(["1@zulip.com", "2@zulip.com"]) - - mail.outbox = [] - invitee_emails = ", ".join( - f"more-{i}@zulip.com" for i in range(get_realm("zulip").max_invites - 1) - ) - self.assert_json_error( - self.invite(invitee_emails, ["Denmark"]), - "To protect users, Zulip limits the number of invitations you can send in one day. Because you have reached the limit, no invitations were sent.", - ) - self.check_sent_emails([]) - def test_missing_or_invalid_params(self) -> None: """ Tests inviting with various missing or invalid parameters.