From 1b6f68bb59dc85125ff21fbd3f98a88f39c0730d Mon Sep 17 00:00:00 2001 From: WookieMonkeys Date: Tue, 23 Mar 2021 02:34:01 -0400 Subject: [PATCH] stream: Add entropy to deactivated streams. Adding an additional `!` to the stream name each time a stream is deactivated, to a maximum of 21 times, effectively limits number of times a stream with a given name can be deactivated. This is unlikely to come up in common usage, but may be confusing when testing. Change what we prepend to deactivated stream names to something with more entropy than just `!`, by instead prepending a substring of hash of the stream's ID. `!`s. Using 128 bits of the hash means that it will require more than 10^18th renames to have a 1% chance of collision. Because too-long stream names are also truncated at 60 characters, having this entropy in the beginning of the name also helps address potential issues from stream names that differed only in, e.g. the 60th character. Fixes #17016. --- zerver/lib/actions.py | 18 +++++++----------- zerver/models.py | 7 ------- zerver/tests/test_subs.py | 7 +++++-- 3 files changed, 12 insertions(+), 20 deletions(-) diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index 1c1dc814ab..7f0a948cc1 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -1,4 +1,5 @@ import datetime +import hashlib import itertools import logging import os @@ -232,7 +233,6 @@ from zerver.models import ( is_cross_realm_bot_email, query_for_ids, realm_filters_for_realm, - stream_name_in_use, validate_attachment_request, ) from zerver.tornado.django_api import send_event @@ -1237,17 +1237,13 @@ def do_deactivate_stream( # special prefix that both indicates that the stream is deactivated and # frees up the original name for reuse. old_name = stream.name - new_name = ("!DEACTIVATED:" + old_name)[: Stream.MAX_NAME_LENGTH] - for i in range(20): - if stream_name_in_use(new_name, stream.realm_id): - # This stream has already been deactivated, keep prepending !s until - # we have a unique stream name or you've hit a rename limit. - new_name = ("!" + new_name)[: Stream.MAX_NAME_LENGTH] - else: - break - # If you don't have a unique name at this point, this will fail later in the - # code path. + # Prepend a substring of the hashed stream ID to the new stream name + streamID = str(stream.id) + stream_id_hash_object = hashlib.sha512(streamID.encode("utf-8")) + hashed_stream_id = stream_id_hash_object.hexdigest()[0:7] + + new_name = (hashed_stream_id + "!DEACTIVATED:" + old_name)[: Stream.MAX_NAME_LENGTH] stream.name = new_name[: Stream.MAX_NAME_LENGTH] stream.save(update_fields=["name", "deactivated", "invite_only"]) diff --git a/zerver/models.py b/zerver/models.py index c47c4c05a8..490c8525d4 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -1863,13 +1863,6 @@ def get_realm_stream(stream_name: str, realm_id: int) -> Stream: return Stream.objects.select_related().get(name__iexact=stream_name.strip(), realm_id=realm_id) -def stream_name_in_use(stream_name: str, realm_id: int) -> bool: - return Stream.objects.filter( - name__iexact=stream_name.strip(), - realm_id=realm_id, - ).exists() - - def get_active_streams(realm: Optional[Realm]) -> QuerySet: # TODO: Change return type to QuerySet[Stream] # NOTE: Return value is used as a QuerySet, so cannot currently be Sequence[QuerySet] diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 340dfaa3a6..20ae882c63 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -1,3 +1,4 @@ +import hashlib import random from datetime import timedelta from typing import Any, Dict, List, Mapping, Optional, Sequence, Set, Union @@ -1330,7 +1331,8 @@ class StreamAdminTest(ZulipTestCase): # Simulate that a stream by the same name has already been # deactivated, just to exercise our renaming logic: - ensure_stream(realm, "!DEACTIVATED:" + active_name) + # Since we do not know the id of these simulated stream we prepend the name with a random hashed_stream_id + ensure_stream(realm, "DB32B77" + "!DEACTIVATED:" + active_name) events: List[Mapping[str, Any]] = [] with tornado_redirected_to_list(events): @@ -1352,7 +1354,8 @@ class StreamAdminTest(ZulipTestCase): # A deleted stream's name is changed, is deactivated, is invite-only, # and has no subscribers. - deactivated_stream_name = "!!DEACTIVATED:" + active_name + hashed_stream_id = hashlib.sha512(str(stream_id).encode("utf-8")).hexdigest()[0:7] + deactivated_stream_name = hashed_stream_id + "!DEACTIVATED:" + active_name deactivated_stream = get_stream(deactivated_stream_name, realm) self.assertTrue(deactivated_stream.deactivated) self.assertTrue(deactivated_stream.invite_only)