diff --git a/zerver/lib/actions.py b/zerver/lib/actions.py index cda5e36b15..ad0c189d45 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -150,6 +150,7 @@ from zerver.lib.streams import ( check_stream_access_based_on_stream_post_policy, create_stream_if_needed, get_default_value_for_history_public_to_subscribers, + get_stream_permission_policy_name, get_web_public_streams_queryset, render_stream_description, send_stream_creation_event, @@ -5007,13 +5008,45 @@ def do_change_can_create_users(user_profile: UserProfile, value: bool) -> None: user_profile.save(update_fields=["can_create_users"]) +def send_change_stream_permission_notification( + stream: Stream, + *, + old_policy_name: str, + new_policy_name: str, + acting_user: UserProfile, +) -> None: + sender = get_system_bot(settings.NOTIFICATION_BOT, acting_user.realm_id) + user_mention = silent_mention_syntax_for_user(acting_user) + + with override_language(stream.realm.default_language): + notification_string = _( + "{user} changed the [access permissions](/help/stream-permissions) " + "for this stream from **{old_policy}** to **{new_policy}**." + ) + notification_string = notification_string.format( + user=user_mention, + old_policy=old_policy_name, + new_policy=new_policy_name, + ) + internal_send_stream_message( + sender, stream, Realm.STREAM_EVENTS_NOTIFICATION_TOPIC, notification_string + ) + + def do_change_stream_permission( stream: Stream, *, invite_only: Optional[bool] = None, history_public_to_subscribers: Optional[bool] = None, is_web_public: Optional[bool] = None, + acting_user: UserProfile, ) -> None: + old_policy_name = get_stream_permission_policy_name( + invite_only=stream.invite_only, + history_public_to_subscribers=stream.history_public_to_subscribers, + is_web_public=stream.is_web_public, + ) + # A note on these assertions: It's possible we'd be better off # making all callers of this function pass the full set of # parameters, rather than having default values. Doing so would @@ -5042,6 +5075,12 @@ def do_change_stream_permission( stream.save(update_fields=["invite_only", "history_public_to_subscribers", "is_web_public"]) + new_policy_name = get_stream_permission_policy_name( + invite_only=stream.invite_only, + history_public_to_subscribers=stream.history_public_to_subscribers, + is_web_public=stream.is_web_public, + ) + event = dict( op="update", type="stream", @@ -5053,6 +5092,12 @@ def do_change_stream_permission( name=stream.name, ) send_event(stream.realm, event, can_access_stream_user_ids(stream)) + send_change_stream_permission_notification( + stream, + old_policy_name=old_policy_name, + new_policy_name=new_policy_name, + acting_user=acting_user, + ) def send_change_stream_post_policy_notification( diff --git a/zerver/lib/streams.py b/zerver/lib/streams.py index 5168c29b11..c271a85184 100644 --- a/zerver/lib/streams.py +++ b/zerver/lib/streams.py @@ -57,6 +57,26 @@ class StreamDict(TypedDict, total=False): message_retention_days: Optional[int] +def get_stream_permission_policy_name( + *, + invite_only: Optional[bool] = None, + history_public_to_subscribers: Optional[bool] = None, + is_web_public: Optional[bool] = None, +) -> str: + policy_name = None + for permission, permission_dict in Stream.PERMISSION_POLICIES.items(): + if ( + permission_dict["invite_only"] == invite_only + and permission_dict["history_public_to_subscribers"] == history_public_to_subscribers + and permission_dict["is_web_public"] == is_web_public + ): + policy_name = permission_dict["policy_name"] + break + + assert policy_name is not None + return policy_name + + def get_default_value_for_history_public_to_subscribers( realm: Realm, invite_only: bool, diff --git a/zerver/models.py b/zerver/models.py index eb76b435ae..8a72a87a61 100644 --- a/zerver/models.py +++ b/zerver/models.py @@ -2239,6 +2239,41 @@ class Stream(models.Model): # Foreign key to the Recipient object for STREAM type messages to this stream. recipient = models.ForeignKey(Recipient, null=True, on_delete=models.SET_NULL) + # Various permission policy configurations + PERMISSION_POLICIES: Dict[str, Dict[str, Any]] = { + "web_public": { + "invite_only": False, + "history_public_to_subscribers": True, + "is_web_public": True, + "policy_name": gettext_lazy("Web public"), + }, + "public": { + "invite_only": False, + "history_public_to_subscribers": True, + "is_web_public": False, + "policy_name": gettext_lazy("Public"), + }, + "private_shared_history": { + "invite_only": True, + "history_public_to_subscribers": True, + "is_web_public": False, + "policy_name": gettext_lazy("Private, shared history"), + }, + "private_protected_history": { + "invite_only": True, + "history_public_to_subscribers": False, + "is_web_public": False, + "policy_name": gettext_lazy("Private, protected history"), + }, + # Public streams with protected history are currently only + # available in Zephyr realms + "public_protected_history": { + "invite_only": False, + "history_public_to_subscribers": False, + "is_web_public": False, + "policy_name": gettext_lazy("Public, protected history"), + }, + } invite_only: Optional[bool] = models.BooleanField(null=True, default=False) history_public_to_subscribers: bool = models.BooleanField(default=False) diff --git a/zerver/openapi/curl_param_value_generators.py b/zerver/openapi/curl_param_value_generators.py index 29b2049a55..d98cab6d96 100644 --- a/zerver/openapi/curl_param_value_generators.py +++ b/zerver/openapi/curl_param_value_generators.py @@ -128,7 +128,7 @@ def add_emoji_to_message() -> Dict[str, object]: # The message ID here is hardcoded based on the corresponding value # for the example message IDs we use in zulip.yaml. - message_id = 44 + message_id = 46 emoji_name = "octopus" emoji_code = "1f419" reaction_type = "unicode_emoji" diff --git a/zerver/tests/test_bots.py b/zerver/tests/test_bots.py index d94a8a2bb7..ac2e3bcd59 100644 --- a/zerver/tests/test_bots.py +++ b/zerver/tests/test_bots.py @@ -420,7 +420,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): user_profile = self.example_user("hamlet") stream = get_stream("Denmark", user_profile.realm) self.subscribe(user_profile, stream.name) - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission(stream, invite_only=True, acting_user=user_profile) self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] @@ -464,7 +464,9 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission( + stream, invite_only=True, acting_user=self.example_user("hamlet") + ) bot_info = { "full_name": "The Bot of Hamlet", @@ -492,7 +494,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") user_profile = self.example_user("hamlet") stream = self.subscribe(user_profile, "Denmark") - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission(stream, invite_only=True, acting_user=user_profile) self.assert_num_bots_equal(0) events: List[Mapping[str, Any]] = [] @@ -536,7 +538,9 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission( + stream, invite_only=True, acting_user=self.example_user("hamlet") + ) self.assert_num_bots_equal(0) bot_info = { @@ -1132,7 +1136,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") user_profile = self.example_user("hamlet") stream = self.subscribe(user_profile, "Denmark") - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission(stream, invite_only=True, acting_user=user_profile) bot_info = { "full_name": "The Bot of Hamlet", @@ -1158,7 +1162,9 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission( + stream, invite_only=True, acting_user=self.example_user("hamlet") + ) bot_info = { "full_name": "The Bot of Hamlet", @@ -1239,7 +1245,7 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): self.login("hamlet") user_profile = self.example_user("hamlet") stream = self.subscribe(user_profile, "Denmark") - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission(stream, invite_only=True, acting_user=user_profile) bot_info = { "full_name": "The Bot of Hamlet", @@ -1264,7 +1270,9 @@ class BotTest(ZulipTestCase, UploadSerializeMixin): realm = self.example_user("hamlet").realm stream = get_stream("Denmark", realm) self.unsubscribe(self.example_user("hamlet"), "Denmark") - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission( + stream, invite_only=True, acting_user=self.example_user("hamlet") + ) bot_info = { "full_name": "The Bot of Hamlet", diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 863707eda6..40b889128a 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -2578,17 +2578,27 @@ class SubscribeActionTest(BaseAction): # Update stream privacy - make stream web public action = lambda: do_change_stream_permission( - stream, invite_only=False, history_public_to_subscribers=True, is_web_public=True + stream, + invite_only=False, + history_public_to_subscribers=True, + is_web_public=True, + acting_user=self.example_user("hamlet"), ) - events = self.verify_action(action, include_subscribers=include_subscribers) + events = self.verify_action(action, include_subscribers=include_subscribers, num_events=2) check_stream_update("events[0]", events[0]) + check_message("events[1]", events[1]) # Update stream privacy - make stream private action = lambda: do_change_stream_permission( - stream, invite_only=True, history_public_to_subscribers=True, is_web_public=False + stream, + invite_only=True, + history_public_to_subscribers=True, + is_web_public=False, + acting_user=self.example_user("hamlet"), ) - events = self.verify_action(action, include_subscribers=include_subscribers) + events = self.verify_action(action, include_subscribers=include_subscribers, num_events=2) check_stream_update("events[0]", events[0]) + check_message("events[1]", events[1]) # Update stream stream_post_policy property action = lambda: do_change_stream_post_policy( diff --git a/zerver/tests/test_message_flags.py b/zerver/tests/test_message_flags.py index a846fd18bb..7170a4e55f 100644 --- a/zerver/tests/test_message_flags.py +++ b/zerver/tests/test_message_flags.py @@ -1264,12 +1264,16 @@ class MessageAccessTests(ZulipTestCase): # Guest user can access messages of subscribed private streams if # history is public to subscribers - do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=True) + do_change_stream_permission( + stream, invite_only=True, history_public_to_subscribers=True, acting_user=guest_user + ) result = self.change_star(message_id) self.assert_json_success(result) # With history not public to subscribers, they can still see new messages - do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=False) + do_change_stream_permission( + stream, invite_only=True, history_public_to_subscribers=False, acting_user=guest_user + ) self.login_user(normal_user) message_id = [ self.send_stream_message(normal_user, stream_name, "test 2"), @@ -1312,7 +1316,12 @@ class MessageAccessTests(ZulipTestCase): self.assert_length(filtered_messages, 1) self.assertEqual(filtered_messages[0].id, message_two_id) - do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=True) + do_change_stream_permission( + stream, + invite_only=True, + history_public_to_subscribers=True, + acting_user=self.example_user("cordelia"), + ) with queries_captured() as queries: filtered_messages = bulk_access_messages(later_subscribed_user, messages, stream=stream) diff --git a/zerver/tests/test_message_topics.py b/zerver/tests/test_message_topics.py index e11273ac79..c834892f89 100644 --- a/zerver/tests/test_message_topics.py +++ b/zerver/tests/test_message_topics.py @@ -132,7 +132,9 @@ class TopicHistoryTest(ZulipTestCase): ) # Now make stream private, but subscribe cordelia - do_change_stream_permission(stream, invite_only=True) + do_change_stream_permission( + stream, invite_only=True, acting_user=self.example_user("cordelia") + ) self.subscribe(self.example_user("cordelia"), stream.name) result = self.client_get(endpoint, {}) @@ -231,10 +233,12 @@ class TopicDeleteTest(ZulipTestCase): }, ) self.assert_json_error(result, "Must be an organization administrator") - self.assertEqual(self.get_last_message().id, last_msg_id) + self.assertTrue(Message.objects.filter(id=last_msg_id).exists()) # Make stream private with limited history - do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=False) + do_change_stream_permission( + stream, invite_only=True, history_public_to_subscribers=False, acting_user=user_profile + ) # ADMIN USER subscribed now user_profile = self.example_user("iago") @@ -252,7 +256,7 @@ class TopicDeleteTest(ZulipTestCase): }, ) self.assert_json_success(result) - self.assertEqual(self.get_last_message().id, last_msg_id) + self.assertTrue(Message.objects.filter(id=last_msg_id).exists()) # Try to delete all messages in the topic again. There are no messages accessible # to the administrator, so this should do nothing. @@ -263,10 +267,12 @@ class TopicDeleteTest(ZulipTestCase): }, ) self.assert_json_success(result) - self.assertEqual(self.get_last_message().id, last_msg_id) + self.assertTrue(Message.objects.filter(id=last_msg_id).exists()) # Make the stream's history public to subscribers - do_change_stream_permission(stream, invite_only=True, history_public_to_subscribers=True) + do_change_stream_permission( + stream, invite_only=True, history_public_to_subscribers=True, acting_user=user_profile + ) # Delete the topic should now remove all messages result = self.client_post( endpoint, @@ -275,7 +281,8 @@ class TopicDeleteTest(ZulipTestCase): }, ) self.assert_json_success(result) - self.assertEqual(self.get_last_message().id, initial_last_msg_id) + self.assertFalse(Message.objects.filter(id=last_msg_id).exists()) + self.assertTrue(Message.objects.filter(id=initial_last_msg_id).exists()) # Delete again, to test the edge case of deleting an empty topic. result = self.client_post( @@ -285,4 +292,5 @@ class TopicDeleteTest(ZulipTestCase): }, ) self.assert_json_success(result) - self.assertEqual(self.get_last_message().id, initial_last_msg_id) + self.assertFalse(Message.objects.filter(id=last_msg_id).exists()) + self.assertTrue(Message.objects.filter(id=initial_last_msg_id).exists()) diff --git a/zerver/tests/test_reactions.py b/zerver/tests/test_reactions.py index 66a3fb1360..582d90c297 100644 --- a/zerver/tests/test_reactions.py +++ b/zerver/tests/test_reactions.py @@ -543,7 +543,9 @@ class ReactionEventTest(ZulipTestCase): self.assert_json_success(remove) # Make stream history public to subscribers - do_change_stream_permission(stream, invite_only=False, history_public_to_subscribers=True) + do_change_stream_permission( + stream, invite_only=False, history_public_to_subscribers=True, acting_user=iago + ) # Since stream history is public to subscribers, reacting to # message_before_id should notify all subscribers: # Iago and Hamlet. @@ -562,7 +564,7 @@ class ReactionEventTest(ZulipTestCase): self.assert_json_success(remove) # Make stream web_public as well. - do_change_stream_permission(stream, is_web_public=True) + do_change_stream_permission(stream, is_web_public=True, acting_user=iago) # For is_web_public streams, events even on old messages # should go to all subscribers, including guests like polonius. with self.tornado_redirected_to_list(events, expected_num_events=1): diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index ae53533415..d37aafd070 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -467,6 +467,14 @@ class StreamAdminTest(ZulipTestCase): self.assertFalse(stream.invite_only) self.assertTrue(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**King Hamlet|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Private, protected history** to **Public**." + ) + self.assertEqual(messages[0].content, expected_notification) + do_change_user_role(user_profile, UserProfile.ROLE_MEMBER, acting_user=None) params = { "stream_name": orjson.dumps("private_stream_2").decode(), @@ -493,6 +501,14 @@ class StreamAdminTest(ZulipTestCase): self.assertFalse(stream.invite_only) self.assertTrue(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**King Hamlet|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Private, protected history** to **Public**." + ) + self.assertEqual(messages[0].content, expected_notification) + def test_make_stream_private(self) -> None: user_profile = self.example_user("hamlet") self.login_user(user_profile) @@ -505,13 +521,21 @@ class StreamAdminTest(ZulipTestCase): "stream_name": orjson.dumps("public_stream_1").decode(), "is_private": orjson.dumps(True).decode(), } - stream_id = get_stream("public_stream_1", realm).id + stream_id = self.subscribe(user_profile, "public_stream_1").id result = self.client_patch(f"/json/streams/{stream_id}", params) self.assert_json_success(result) stream = get_stream("public_stream_1", realm) self.assertTrue(stream.invite_only) self.assertFalse(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**King Hamlet|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Public** to **Private, protected history**." + ) + self.assertEqual(messages[0].content, expected_notification) + default_stream = self.make_stream("default_stream", realm=realm) do_add_default_stream(default_stream) params = { @@ -548,6 +572,14 @@ class StreamAdminTest(ZulipTestCase): self.assertTrue(stream.invite_only) self.assertFalse(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**King Hamlet|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Public** to **Private, protected history**." + ) + self.assertEqual(messages[0].content, expected_notification) + def test_create_web_public_stream(self) -> None: user_profile = self.example_user("hamlet") owner = self.example_user("desdemona") @@ -614,6 +646,14 @@ class StreamAdminTest(ZulipTestCase): self.assertFalse(stream.invite_only) self.assertFalse(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**{user_profile.full_name}|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Private, protected history** to **Public, protected history**." + ) + self.assertEqual(messages[0].content, expected_notification) + def test_make_stream_private_with_public_history(self) -> None: user_profile = self.example_user("hamlet") self.login_user(user_profile) @@ -626,19 +666,27 @@ class StreamAdminTest(ZulipTestCase): "is_private": orjson.dumps(True).decode(), "history_public_to_subscribers": orjson.dumps(True).decode(), } - stream_id = get_stream("public_history_stream", realm).id + stream_id = self.subscribe(user_profile, "public_history_stream").id result = self.client_patch(f"/json/streams/{stream_id}", params) self.assert_json_success(result) stream = get_stream("public_history_stream", realm) self.assertTrue(stream.invite_only) self.assertTrue(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**King Hamlet|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Public** to **Private, shared history**." + ) + self.assertEqual(messages[0].content, expected_notification) + def test_make_stream_web_public(self) -> None: user_profile = self.example_user("hamlet") self.login_user(user_profile) realm = user_profile.realm self.make_stream("test_stream", realm=realm) - stream_id = get_stream("test_stream", realm).id + stream_id = self.subscribe(user_profile, "test_stream").id params = { "stream_name": orjson.dumps("test_stream").decode(), @@ -699,6 +747,14 @@ class StreamAdminTest(ZulipTestCase): self.assertFalse(stream.invite_only) self.assertTrue(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**King Hamlet|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Public** to **Web public**." + ) + self.assertEqual(messages[0].content, expected_notification) + def test_try_make_stream_public_with_private_history(self) -> None: user_profile = self.example_user("hamlet") self.login_user(user_profile) @@ -711,13 +767,33 @@ class StreamAdminTest(ZulipTestCase): "is_private": orjson.dumps(False).decode(), "history_public_to_subscribers": orjson.dumps(False).decode(), } - stream_id = get_stream("public_stream", realm).id + stream_id = self.subscribe(user_profile, "public_stream").id result = self.client_patch(f"/json/streams/{stream_id}", params) self.assert_json_success(result) stream = get_stream("public_stream", realm) self.assertFalse(stream.invite_only) self.assertTrue(stream.history_public_to_subscribers) + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + + # This test verifies the (weird) outcome for a transition that + # is not currently possible. For background, we only support + # public streams with private history if + # is_zephyr_mirror_realm, and don't allow changing stream + # permissions in such realms. So changing the + # history_public_to_subscribers property of a public stream is + # not possible in Zulip today; this test covers that situation + # and will produce the odd/wrong output of "Public to Public". + # + # This test should be corrected if we add support for such a + # stream configuration transition. + expected_notification = ( + f"@_**King Hamlet|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Public** to **Public**." + ) + self.assertEqual(messages[0].content, expected_notification) + def test_subscriber_ids_with_stream_history_access(self) -> None: hamlet = self.example_user("hamlet") polonius = self.example_user("polonius") diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 3c835f8231..f9560529be 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -329,6 +329,7 @@ def update_stream_backend( invite_only=is_private, history_public_to_subscribers=history_public_to_subscribers, is_web_public=is_web_public, + acting_user=user_profile, ) return json_success()