diff --git a/analytics/views/support.py b/analytics/views/support.py index 3301c9fe4c..5da64c9104 100644 --- a/analytics/views/support.py +++ b/analytics/views/support.py @@ -92,7 +92,9 @@ def get_confirmations( link_status = "" now = timezone_now() - if now < expiry_date: + if expiry_date is None: + expires_in = "Never" + elif now < expiry_date: expires_in = timesince(now, expiry_date) else: expires_in = "Expired" diff --git a/confirmation/migrations/0011_alter_confirmation_expiry_date.py b/confirmation/migrations/0011_alter_confirmation_expiry_date.py new file mode 100644 index 0000000000..4a260c66f8 --- /dev/null +++ b/confirmation/migrations/0011_alter_confirmation_expiry_date.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.9 on 2021-11-30 17:44 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("confirmation", "0010_alter_confirmation_expiry_date"), + ] + + operations = [ + migrations.AlterField( + model_name="confirmation", + name="expiry_date", + field=models.DateTimeField(db_index=True, null=True), + ), + ] diff --git a/confirmation/models.py b/confirmation/models.py index f778bbf0c5..03e6fdce35 100644 --- a/confirmation/models.py +++ b/confirmation/models.py @@ -18,6 +18,7 @@ from django.urls import reverse from django.utils.timezone import now as timezone_now from typing_extensions import Protocol +from zerver.lib.types import UnspecifiedValue from zerver.models import EmailChangeStatus, MultiuseInvite, PreregistrationUser, Realm, UserProfile @@ -70,7 +71,7 @@ def get_object_from_key( except Confirmation.DoesNotExist: raise ConfirmationKeyException(ConfirmationKeyException.DOES_NOT_EXIST) - if timezone_now() > confirmation.expiry_date: + if confirmation.expiry_date is not None and timezone_now() > confirmation.expiry_date: raise ConfirmationKeyException(ConfirmationKeyException.EXPIRED) obj = confirmation.content_object @@ -85,7 +86,7 @@ def create_confirmation_link( obj: Union[Realm, HasRealmObject, OptionalHasRealmObject], confirmation_type: int, *, - validity_in_days: Optional[int] = None, + validity_in_days: Union[Optional[int], UnspecifiedValue] = UnspecifiedValue(), url_args: Mapping[str, str] = {}, ) -> str: # validity_in_days is an override for the default values which are @@ -100,8 +101,12 @@ def create_confirmation_link( current_time = timezone_now() expiry_date = None - if validity_in_days: - expiry_date = current_time + datetime.timedelta(days=validity_in_days) + if not isinstance(validity_in_days, UnspecifiedValue): + if validity_in_days is None: + expiry_date = None + else: + assert validity_in_days is not None + expiry_date = current_time + datetime.timedelta(days=validity_in_days) else: expiry_date = current_time + datetime.timedelta( days=_properties[confirmation_type].validity_in_days @@ -138,7 +143,7 @@ class Confirmation(models.Model): content_object = GenericForeignKey("content_type", "object_id") date_sent: datetime.datetime = models.DateTimeField(db_index=True) confirmation_key: str = models.CharField(max_length=40, db_index=True) - expiry_date: datetime.datetime = models.DateTimeField(db_index=True) + expiry_date: Optional[datetime.datetime] = models.DateTimeField(db_index=True, null=True) realm: Optional[Realm] = models.ForeignKey(Realm, null=True, on_delete=CASCADE) # The following list is the set of valid types diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index a070181454..c417299979 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -21,6 +21,12 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 5.0 +**Feature level 117** + +* `POST /invites`, `POST /invites/multiuse`: Added support for passing + `null` as the `invite_expires_in_days` parameter to request an + invitation that never expires. + **Feature level 116** * [`GET /server_settings`](/api/get-server-settings): Added diff --git a/version.py b/version.py index a0765283f9..44fdd598f8 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md, as well as # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 116 +API_FEATURE_LEVEL = 117 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index ae57eacf81..17d3d9bb3b 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -174,7 +174,7 @@ from zerver.lib.topic import ( update_messages_for_topic_edit, ) from zerver.lib.topic_mutes import add_topic_mute, get_topic_mutes, remove_topic_mute -from zerver.lib.types import ProfileDataElementValue, ProfileFieldData +from zerver.lib.types import ProfileDataElementValue, ProfileFieldData, UnspecifiedValue from zerver.lib.upload import ( claim_attachment, delete_avatar_image, @@ -7526,7 +7526,7 @@ def do_send_confirmation_email( invitee: PreregistrationUser, referrer: UserProfile, email_language: str, - invite_expires_in_days: Optional[int] = None, + invite_expires_in_days: Union[Optional[int], UnspecifiedValue] = UnspecifiedValue(), ) -> str: """ Send the confirmation/welcome e-mail to an invited user. @@ -7625,7 +7625,7 @@ def do_invite_users( invitee_emails: Collection[str], streams: Collection[Stream], *, - invite_expires_in_days: int, + invite_expires_in_days: Optional[int], invite_as: int = PreregistrationUser.INVITE_AS["MEMBER"], ) -> None: num_invites = len(invitee_emails) @@ -7737,6 +7737,13 @@ def do_invite_users( notify_invites_changed(user_profile.realm) +def get_invitation_expiry_date(confirmation_obj: Confirmation) -> Optional[int]: + expiry_date = confirmation_obj.expiry_date + if expiry_date is None: + return expiry_date + return datetime_to_timestamp(expiry_date) + + def do_get_invites_controlled_by_user(user_profile: UserProfile) -> List[Dict[str, Any]]: """ Returns a list of dicts representing invitations that can be controlled by user_profile. @@ -7755,13 +7762,12 @@ def do_get_invites_controlled_by_user(user_profile: UserProfile) -> List[Dict[st invites = [] for invitee in prereg_users: - expiry_date = invitee.confirmation.get().expiry_date invites.append( dict( email=invitee.email, invited_by_user_id=invitee.referred_by.id, invited=datetime_to_timestamp(invitee.invited_at), - expiry_date=datetime_to_timestamp(expiry_date), + expiry_date=get_invitation_expiry_date(invitee.confirmation.get()), id=invitee.id, invited_as=invitee.invited_as, is_multiuse=False, @@ -7773,8 +7779,8 @@ def do_get_invites_controlled_by_user(user_profile: UserProfile) -> List[Dict[st return invites multiuse_confirmation_objs = Confirmation.objects.filter( - realm=user_profile.realm, type=Confirmation.MULTIUSE_INVITE, expiry_date__gte=timezone_now() - ) + realm=user_profile.realm, type=Confirmation.MULTIUSE_INVITE + ).filter(Q(expiry_date__gte=timezone_now()) | Q(expiry_date=None)) for confirmation_obj in multiuse_confirmation_objs: invite = confirmation_obj.content_object assert invite is not None @@ -7782,7 +7788,7 @@ def do_get_invites_controlled_by_user(user_profile: UserProfile) -> List[Dict[st dict( invited_by_user_id=invite.referred_by.id, invited=datetime_to_timestamp(confirmation_obj.date_sent), - expiry_date=datetime_to_timestamp(confirmation_obj.expiry_date), + expiry_date=get_invitation_expiry_date(confirmation_obj), id=invite.id, link_url=confirmation_url( confirmation_obj.confirmation_key, @@ -7812,9 +7818,8 @@ def get_valid_invite_confirmations_generated_by_user( confirmations += list( Confirmation.objects.filter( type=Confirmation.MULTIUSE_INVITE, - expiry_date__gte=timezone_now(), object_id__in=multiuse_invite_ids, - ) + ).filter(Q(expiry_date__gte=timezone_now()) | Q(expiry_date=None)) ) return confirmations @@ -7834,7 +7839,7 @@ def revoke_invites_generated_by_user(user_profile: UserProfile) -> None: def do_create_multiuse_invite_link( referred_by: UserProfile, invited_as: int, - invite_expires_in_days: int, + invite_expires_in_days: Optional[int], streams: Sequence[Stream] = [], ) -> str: realm = referred_by.realm @@ -7883,9 +7888,14 @@ def do_resend_user_invite_email(prereg_user: PreregistrationUser) -> int: prereg_user.invited_at = timezone_now() prereg_user.save() - invite_expires_in_days = ( - prereg_user.confirmation.get().expiry_date - prereg_user.invited_at - ).days + + expiry_date = prereg_user.confirmation.get().expiry_date + if expiry_date is None: + invite_expires_in_days = None + else: + # The resent invitation is reset to expire as long after the + # reminder is sent as it lasted originally. + invite_expires_in_days = (expiry_date - prereg_user.invited_at).days prereg_user.confirmation.clear() do_increment_logging_stat( diff --git a/zerver/lib/types.py b/zerver/lib/types.py index 01b51e9549..15072fd7e9 100644 --- a/zerver/lib/types.py +++ b/zerver/lib/types.py @@ -72,3 +72,20 @@ class SAMLIdPConfigDict(TypedDict, total=False): extra_attrs: List[str] x509cert: str x509cert_path: str + + +class UnspecifiedValue: + """In most API endpoints, we use a default value of `None"` to encode + parameters that the client did not pass, which is nicely Pythonic. + + However, that design does not work for those few endpoints where + we want to allow clients to pass an explicit `null` (which + JSON-decodes to `None`). + + We use this type as an explicit sentinel value for such endpoints. + + TODO: Can this be merged with the _NotSpecified class, which is + currently an internal implementation detail of the REQ class? + """ + + pass diff --git a/zerver/models.py b/zerver/models.py index 088877b59d..cc841f570f 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -86,6 +86,7 @@ from zerver.lib.types import ( ProfileDataElementBase, ProfileDataElementValue, RealmUserValidator, + UnspecifiedValue, UserFieldElement, Validator, ) @@ -2184,17 +2185,30 @@ class PreregistrationUser(models.Model): def filter_to_valid_prereg_users( query: QuerySet, - invite_expires_in_days: Optional[int] = None, + invite_expires_in_days: Union[Optional[int], UnspecifiedValue] = UnspecifiedValue(), ) -> QuerySet: + """ + If invite_expires_in_days is specified, we return only those PreregistrationUser + objects that were created at most that many days in the past. + """ active_value = confirmation_settings.STATUS_ACTIVE revoked_value = confirmation_settings.STATUS_REVOKED query = query.exclude(status__in=[active_value, revoked_value]) - if invite_expires_in_days: + if invite_expires_in_days is None: + # Since invite_expires_in_days is None, we're invitation will never + # expire, we do not need to check anything else and can simply return + # after excluding objects with active and revoked status. + return query + + assert invite_expires_in_days is not None + if not isinstance(invite_expires_in_days, UnspecifiedValue): lowest_datetime = timezone_now() - datetime.timedelta(days=invite_expires_in_days) return query.filter(invited_at__gte=lowest_datetime) else: - return query.filter(confirmation__expiry_date__gte=timezone_now()) + return query.filter( + Q(confirmation__expiry_date=None) | Q(confirmation__expiry_date__gte=timezone_now()) + ) class MultiuseInvite(models.Model): diff --git a/zerver/tests/test_signup.py b/zerver/tests/test_signup.py index f5b572a9f7..b4b7af82df 100644 --- a/zerver/tests/test_signup.py +++ b/zerver/tests/test_signup.py @@ -1045,7 +1045,7 @@ class InviteUserBase(ZulipTestCase): self, invitee_emails: str, stream_names: Sequence[str], - invite_expires_in_days: int = settings.INVITATION_LINK_VALIDITY_DAYS, + invite_expires_in_days: Optional[int] = settings.INVITATION_LINK_VALIDITY_DAYS, body: str = "", invite_as: int = PreregistrationUser.INVITE_AS["MEMBER"], ) -> HttpResponse: @@ -1060,11 +1060,16 @@ class InviteUserBase(ZulipTestCase): stream_ids = [] for stream_name in stream_names: stream_ids.append(self.get_stream_id(stream_name)) + + invite_expires_in: Union[str, Optional[int]] = invite_expires_in_days + if invite_expires_in is None: + invite_expires_in = orjson.dumps(None).decode() + return self.client_post( "/json/invites", { "invitee_emails": invitee_emails, - "invite_expires_in_days": invite_expires_in_days, + "invite_expires_in_days": invite_expires_in, "stream_ids": orjson.dumps(stream_ids).decode(), "invite_as": invite_as, }, @@ -2085,6 +2090,26 @@ so we didn't send them an invitation. We did send invitations to everyone else!" "Whoops. The confirmation link has expired or been deactivated.", result ) + def test_never_expire_confirmation_obejct(self) -> None: + email = self.nonreg_email("alice") + realm = get_realm("zulip") + inviter = self.example_user("iago") + prereg_user = PreregistrationUser.objects.create( + email=email, referred_by=inviter, realm=realm + ) + activation_url = create_confirmation_link( + prereg_user, Confirmation.INVITATION, validity_in_days=None + ) + confirmation = Confirmation.objects.last() + assert confirmation is not None + self.assertEqual(confirmation.expiry_date, None) + activation_key = activation_url.split("/")[-1] + response = self.client_post( + "/accounts/register/", + {"key": activation_key, "from_confirmation": 1, "full_nme": "alice"}, + ) + self.assertEqual(response.status_code, 200) + def test_send_more_than_one_invite_to_same_user(self) -> None: self.user_profile = self.example_user("iago") streams = [] @@ -2349,6 +2374,53 @@ class InvitationsTestCase(InviteUserBase): self.assertTrue(invites[1]["is_multiuse"]) self.assertEqual(invites[1]["invited_by_user_id"], hamlet.id) + def test_get_never_expiring_invitations(self) -> None: + self.login("iago") + user_profile = self.example_user("iago") + + streams = [] + for stream_name in ["Denmark", "Scotland"]: + streams.append(get_stream(stream_name, user_profile.realm)) + + with patch( + "confirmation.models.timezone_now", + return_value=timezone_now() - datetime.timedelta(days=1000), + ): + # Testing the invitation with expiry date set to "None" exists + # after a large amount of days. + do_invite_users( + user_profile, + ["TestOne@zulip.com"], + streams, + invite_expires_in_days=None, + ) + do_invite_users( + user_profile, + ["TestTwo@zulip.com"], + streams, + invite_expires_in_days=100, + ) + do_create_multiuse_invite_link( + user_profile, PreregistrationUser.INVITE_AS["MEMBER"], None + ) + do_create_multiuse_invite_link( + user_profile, PreregistrationUser.INVITE_AS["MEMBER"], 100 + ) + + result = self.client_get("/json/invites") + self.assertEqual(result.status_code, 200) + invites = orjson.loads(result.content)["invites"] + # We only get invitations that will never expire because we have mocked time such + # that the other invitations are created in the deep past. + self.assert_length(invites, 2) + + self.assertFalse(invites[0]["is_multiuse"]) + self.assertEqual(invites[0]["email"], "TestOne@zulip.com") + self.assertEqual(invites[0]["expiry_date"], None) + self.assertTrue(invites[1]["is_multiuse"]) + self.assertEqual(invites[1]["invited_by_user_id"], user_profile.id) + self.assertEqual(invites[1]["expiry_date"], None) + def test_successful_delete_invitation(self) -> None: """ A DELETE call to /json/invites/ should delete the invite and @@ -2635,6 +2707,23 @@ class InvitationsTestCase(InviteUserBase): original_timestamp, scheduledemail_filter.values_list("scheduled_timestamp", flat=True) ) + def test_resend_never_expiring_invitation(self) -> None: + self.login("iago") + invitee = "resend@zulip.com" + + self.assert_json_success(self.invite(invitee, ["Denmark"], None)) + prereg_user = PreregistrationUser.objects.get(email=invitee) + + # Verify and then clear from the outbox the original invite email + self.check_sent_emails([invitee]) + from django.core.mail import outbox + + outbox.pop() + + result = self.client_post("/json/invites/" + str(prereg_user.id) + "/resend") + self.assert_json_success(result) + self.check_sent_emails([invitee]) + def test_accessing_invites_in_another_realm(self) -> None: inviter = UserProfile.objects.exclude(realm=get_realm("zulip")).first() assert inviter is not None diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 5c05c58a9a..85b16aafc9 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -1461,6 +1461,21 @@ class ActivateTest(ZulipTestCase): invite_as=PreregistrationUser.INVITE_AS["REALM_ADMIN"], ) + do_invite_users( + iago, + ["new5@zulip.com"], + [], + invite_expires_in_days=None, + invite_as=PreregistrationUser.INVITE_AS["REALM_ADMIN"], + ) + do_invite_users( + desdemona, + ["new6@zulip.com"], + [], + invite_expires_in_days=None, + invite_as=PreregistrationUser.INVITE_AS["REALM_ADMIN"], + ) + iago_multiuse_key = do_create_multiuse_invite_link( iago, PreregistrationUser.INVITE_AS["MEMBER"], invite_expires_in_days ).split("/")[-2] @@ -1468,17 +1483,24 @@ class ActivateTest(ZulipTestCase): desdemona, PreregistrationUser.INVITE_AS["MEMBER"], invite_expires_in_days ).split("/")[-2] + iago_never_expire_multiuse_key = do_create_multiuse_invite_link( + iago, PreregistrationUser.INVITE_AS["MEMBER"], None + ).split("/")[-2] + desdemona_never_expire_multiuse_key = do_create_multiuse_invite_link( + desdemona, PreregistrationUser.INVITE_AS["MEMBER"], None + ).split("/")[-2] + self.assertEqual( filter_to_valid_prereg_users( PreregistrationUser.objects.filter(referred_by=iago) ).count(), - 2, + 3, ) self.assertEqual( filter_to_valid_prereg_users( PreregistrationUser.objects.filter(referred_by=desdemona) ).count(), - 2, + 3, ) self.assertTrue( Confirmation.objects.get(confirmation_key=iago_multiuse_key).expiry_date @@ -1488,6 +1510,14 @@ class ActivateTest(ZulipTestCase): Confirmation.objects.get(confirmation_key=desdemona_multiuse_key).expiry_date > timezone_now() ) + self.assertIsNone( + Confirmation.objects.get(confirmation_key=iago_never_expire_multiuse_key).expiry_date + ) + self.assertIsNone( + Confirmation.objects.get( + confirmation_key=desdemona_never_expire_multiuse_key + ).expiry_date + ) do_deactivate_user(iago, acting_user=None) @@ -1503,7 +1533,7 @@ class ActivateTest(ZulipTestCase): filter_to_valid_prereg_users( PreregistrationUser.objects.filter(referred_by=desdemona) ).count(), - 2, + 3, ) self.assertTrue( Confirmation.objects.get(confirmation_key=iago_multiuse_key).expiry_date @@ -1513,6 +1543,15 @@ class ActivateTest(ZulipTestCase): Confirmation.objects.get(confirmation_key=desdemona_multiuse_key).expiry_date > timezone_now() ) + self.assertTrue( + Confirmation.objects.get(confirmation_key=iago_never_expire_multiuse_key).expiry_date + <= timezone_now() + ) + self.assertIsNone( + Confirmation.objects.get( + confirmation_key=desdemona_never_expire_multiuse_key + ).expiry_date + ) def test_clear_scheduled_jobs(self) -> None: user = self.example_user("hamlet") diff --git a/zerver/views/invite.py b/zerver/views/invite.py index 6cc522a4d9..55d49069a4 100644 --- a/zerver/views/invite.py +++ b/zerver/views/invite.py @@ -1,5 +1,5 @@ import re -from typing import List, Sequence, Set +from typing import List, Optional, Sequence, Set from django.conf import settings from django.http import HttpRequest, HttpResponse @@ -18,7 +18,7 @@ from zerver.lib.exceptions import JsonableError, OrganizationOwnerRequired from zerver.lib.request import REQ, has_request_variables from zerver.lib.response import json_success from zerver.lib.streams import access_stream_by_id -from zerver.lib.validator import check_int, check_list +from zerver.lib.validator import check_int, check_list, check_none_or from zerver.models import MultiuseInvite, PreregistrationUser, Stream, UserProfile @@ -36,8 +36,8 @@ def invite_users_backend( request: HttpRequest, user_profile: UserProfile, invitee_emails_raw: str = REQ("invitee_emails"), - invite_expires_in_days: int = REQ( - json_validator=check_int, default=settings.INVITATION_LINK_VALIDITY_DAYS + invite_expires_in_days: Optional[int] = REQ( + json_validator=check_none_or(check_int), default=settings.INVITATION_LINK_VALIDITY_DAYS ), invite_as: int = REQ(json_validator=check_int, default=PreregistrationUser.INVITE_AS["MEMBER"]), stream_ids: List[int] = REQ(json_validator=check_list(check_int)), @@ -174,8 +174,8 @@ def resend_user_invite_email( def generate_multiuse_invite_backend( request: HttpRequest, user_profile: UserProfile, - invite_expires_in_days: int = REQ( - json_validator=check_int, default=settings.INVITATION_LINK_VALIDITY_DAYS + invite_expires_in_days: Optional[int] = REQ( + json_validator=check_none_or(check_int), default=settings.INVITATION_LINK_VALIDITY_DAYS ), invite_as: int = REQ(json_validator=check_int, default=PreregistrationUser.INVITE_AS["MEMBER"]), stream_ids: Sequence[int] = REQ(json_validator=check_list(check_int), default=[]), diff --git a/zerver/worker/queue_processors.py b/zerver/worker/queue_processors.py index d617a39ed4..25c8970dd1 100644 --- a/zerver/worker/queue_processors.py +++ b/zerver/worker/queue_processors.py @@ -461,6 +461,12 @@ class ConfirmationEmailWorker(QueueProcessingWorker): activate_url = do_send_confirmation_email( invitee, referrer, email_language, invite_expires_in_days ) + if invite_expires_in_days is None: + # We do not queue reminder email for never expiring + # invitations. This is probably a low importance bug; it + # would likely be more natural to send a reminder after 7 + # days. + return # queue invitation reminder if invite_expires_in_days >= 4: