streams: Remove "email_address" field from Subscription objects.

This commit removes "email_address" field from Subscription objects
and we would instead a new endpoint in next commit to get email
address for stream with proper access check.

This change also fixes the bug where we would include email address
for the unsubscribed private stream as well when user did not have
permission to send message to the stream, and having email allowed
the unsubscribed user to send message to the stream.

Note that the unsubscribed user can still send message to the stream
if the user had noted down the email before being unsubscribed
and the stream token is not changed after unsubscribing the user.
This commit is contained in:
Sahil Batra
2023-09-29 23:08:07 +05:30
committed by Alex Vandiver
parent 9636362cbd
commit 0a3800332f
9 changed files with 44 additions and 78 deletions

View File

@@ -26,6 +26,11 @@ format used by the Zulip server that they are interacting with.
now included web-public streams as well. This change was backported from now included web-public streams as well. This change was backported from
Zulip 8.0, where it was introduced in feature level 205. Zulip 8.0, where it was introduced in feature level 205.
* [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events),
[`GET /users/me/subscriptions`](/api/get-subscriptions): Removed
`email_address` field from subscription objects. This change was backported
from Zulip 8.0, where it was introduced in feature level 226.
## Changes in Zulip 7.0 ## Changes in Zulip 7.0
**Feature level 185** **Feature level 185**

View File

@@ -686,7 +686,6 @@ exports.fixtures = {
audible_notifications: true, audible_notifications: true,
color: "blue", color: "blue",
desktop_notifications: false, desktop_notifications: false,
email_address: "whatever",
email_notifications: false, email_notifications: false,
in_home_view: false, in_home_view: false,
is_muted: true, is_muted: true,

View File

@@ -25,7 +25,6 @@ from zerver.lib.cache import (
get_stream_cache_key, get_stream_cache_key,
to_dict_cache_key_id, to_dict_cache_key_id,
) )
from zerver.lib.email_mirror_helpers import encode_email_address
from zerver.lib.exceptions import JsonableError from zerver.lib.exceptions import JsonableError
from zerver.lib.mention import silent_mention_syntax_for_user from zerver.lib.mention import silent_mention_syntax_for_user
from zerver.lib.message import get_last_message_id from zerver.lib.message import get_last_message_id
@@ -317,7 +316,6 @@ def get_subscriber_ids(
@dataclass @dataclass
class StreamInfo: class StreamInfo:
email_address: str
stream_weekly_traffic: Optional[int] stream_weekly_traffic: Optional[int]
subscribers: List[int] subscribers: List[int]
@@ -340,7 +338,6 @@ def send_subscription_add_events(
for sub_info in sub_info_list: for sub_info in sub_info_list:
stream = sub_info.stream stream = sub_info.stream
if stream.id not in stream_info_dict: if stream.id not in stream_info_dict:
email_address = encode_email_address(stream, show_sender=True)
stream_weekly_traffic = get_average_weekly_stream_traffic( stream_weekly_traffic = get_average_weekly_stream_traffic(
stream.id, stream.date_created, recent_traffic stream.id, stream.date_created, recent_traffic
) )
@@ -349,7 +346,6 @@ def send_subscription_add_events(
else: else:
subscribers = list(subscriber_dict[stream.id]) subscribers = list(subscriber_dict[stream.id])
stream_info_dict[stream.id] = StreamInfo( stream_info_dict[stream.id] = StreamInfo(
email_address=email_address,
stream_weekly_traffic=stream_weekly_traffic, stream_weekly_traffic=stream_weekly_traffic,
subscribers=subscribers, subscribers=subscribers,
) )
@@ -375,7 +371,6 @@ def send_subscription_add_events(
push_notifications=subscription.push_notifications, push_notifications=subscription.push_notifications,
wildcard_mentions_notify=subscription.wildcard_mentions_notify, wildcard_mentions_notify=subscription.wildcard_mentions_notify,
# Computed fields not present in Subscription.API_FIELDS # Computed fields not present in Subscription.API_FIELDS
email_address=stream_info.email_address,
in_home_view=not subscription.is_muted, in_home_view=not subscription.is_muted,
stream_weekly_traffic=stream_info.stream_weekly_traffic, stream_weekly_traffic=stream_info.stream_weekly_traffic,
subscribers=stream_info.subscribers, subscribers=stream_info.subscribers,
@@ -1177,7 +1172,7 @@ def do_change_stream_post_policy(
) )
def do_rename_stream(stream: Stream, new_name: str, user_profile: UserProfile) -> Dict[str, str]: def do_rename_stream(stream: Stream, new_name: str, user_profile: UserProfile) -> None:
old_name = stream.name old_name = stream.name
stream.name = new_name stream.name = new_name
stream.save(update_fields=["name"]) stream.save(update_fields=["name"])
@@ -1213,30 +1208,18 @@ def do_rename_stream(stream: Stream, new_name: str, user_profile: UserProfile) -
# clearer than trying to set them. display_recipient is the out of # clearer than trying to set them. display_recipient is the out of
# date field in all cases. # date field in all cases.
cache_delete_many(to_dict_cache_key_id(message.id) for message in messages) cache_delete_many(to_dict_cache_key_id(message.id) for message in messages)
new_email = encode_email_address(stream, show_sender=True)
# We will tell our users to essentially # We want to key these updates by id, not name, since id is
# update stream.name = new_name where name = old_name # the immutable primary key, and obviously name is not.
# and update stream.email = new_email where name = old_name. event = dict(
# We could optimize this by trying to send one message, but the op="update",
# client code really wants one property update at a time, and type="stream",
# updating stream names is a pretty infrequent operation. property="name",
# More importantly, we want to key these updates by id, not name, value=new_name,
# since id is the immutable primary key, and obviously name is not. stream_id=stream.id,
data_updates = [ name=old_name,
["email_address", new_email], )
["name", new_name], send_event(stream.realm, event, can_access_stream_user_ids(stream))
]
for property, value in data_updates:
event = dict(
op="update",
type="stream",
property=property,
value=value,
stream_id=stream.id,
name=old_name,
)
send_event(stream.realm, event, can_access_stream_user_ids(stream))
sender = get_system_bot(settings.NOTIFICATION_BOT, stream.realm_id) sender = get_system_bot(settings.NOTIFICATION_BOT, stream.realm_id)
with override_language(stream.realm.default_language): with override_language(stream.realm.default_language):
internal_send_stream_message( internal_send_stream_message(
@@ -1249,9 +1232,6 @@ def do_rename_stream(stream: Stream, new_name: str, user_profile: UserProfile) -
new_stream_name=f"**{new_name}**", new_stream_name=f"**{new_name}**",
), ),
) )
# Even though the token doesn't change, the web client needs to update the
# email forwarding address to display the correctly-escaped new name.
return {"email_address": new_email}
def send_change_stream_description_notification( def send_change_stream_description_notification(

View File

@@ -70,7 +70,6 @@ subscription_fields: Sequence[Tuple[str, object]] = [
("audible_notifications", OptionalType(bool)), ("audible_notifications", OptionalType(bool)),
("color", str), ("color", str),
("desktop_notifications", OptionalType(bool)), ("desktop_notifications", OptionalType(bool)),
("email_address", str),
("email_notifications", OptionalType(bool)), ("email_notifications", OptionalType(bool)),
("in_home_view", bool), ("in_home_view", bool),
("is_muted", bool), ("is_muted", bool),
@@ -1268,9 +1267,6 @@ def check_stream_update(
if prop == "description": if prop == "description":
assert extra_keys == {"rendered_description"} assert extra_keys == {"rendered_description"}
assert isinstance(value, str) assert isinstance(value, str)
elif prop == "email_address":
assert extra_keys == set()
assert isinstance(value, str)
elif prop == "invite_only": elif prop == "invite_only":
assert extra_keys == {"history_public_to_subscribers", "is_web_public"} assert extra_keys == {"history_public_to_subscribers", "is_web_public"}
assert isinstance(value, bool) assert isinstance(value, bool)

View File

@@ -8,7 +8,6 @@ from django.db.models import QuerySet
from django.utils.translation import gettext as _ from django.utils.translation import gettext as _
from psycopg2.sql import SQL from psycopg2.sql import SQL
from zerver.lib.email_mirror_helpers import encode_email_address_helper
from zerver.lib.exceptions import JsonableError from zerver.lib.exceptions import JsonableError
from zerver.lib.stream_color import STREAM_ASSIGNMENT_COLORS from zerver.lib.stream_color import STREAM_ASSIGNMENT_COLORS
from zerver.lib.stream_subscription import ( from zerver.lib.stream_subscription import (
@@ -59,7 +58,6 @@ def get_web_public_subs(realm: Realm) -> SubscriptionInfo:
audible_notifications = True audible_notifications = True
color = get_next_color() color = get_next_color()
desktop_notifications = True desktop_notifications = True
email_address = ""
email_notifications = True email_notifications = True
in_home_view = True in_home_view = True
is_muted = False is_muted = False
@@ -77,7 +75,6 @@ def get_web_public_subs(realm: Realm) -> SubscriptionInfo:
date_created=date_created, date_created=date_created,
description=description, description=description,
desktop_notifications=desktop_notifications, desktop_notifications=desktop_notifications,
email_address=email_address,
email_notifications=email_notifications, email_notifications=email_notifications,
first_message_id=first_message_id, first_message_id=first_message_id,
history_public_to_subscribers=history_public_to_subscribers, history_public_to_subscribers=history_public_to_subscribers,
@@ -149,10 +146,6 @@ def build_stream_dict_for_sub(
raw_stream_dict["id"], raw_stream_dict["date_created"], recent_traffic raw_stream_dict["id"], raw_stream_dict["date_created"], recent_traffic
) )
email_address = encode_email_address_helper(
raw_stream_dict["name"], raw_stream_dict["email_token"], show_sender=True
)
# Our caller may add a subscribers field. # Our caller may add a subscribers field.
return SubscriptionStreamDict( return SubscriptionStreamDict(
audible_notifications=audible_notifications, audible_notifications=audible_notifications,
@@ -161,7 +154,6 @@ def build_stream_dict_for_sub(
date_created=date_created, date_created=date_created,
description=description, description=description,
desktop_notifications=desktop_notifications, desktop_notifications=desktop_notifications,
email_address=email_address,
email_notifications=email_notifications, email_notifications=email_notifications,
first_message_id=first_message_id, first_message_id=first_message_id,
history_public_to_subscribers=history_public_to_subscribers, history_public_to_subscribers=history_public_to_subscribers,
@@ -400,9 +392,6 @@ def gather_subscriptions_helper(
# The realm_id and recipient_id are generally not needed in the API. # The realm_id and recipient_id are generally not needed in the API.
"realm_id", "realm_id",
"recipient_id", "recipient_id",
# email_token isn't public to some users with access to
# the stream, so doesn't belong in API_FIELDS.
"email_token",
) )
recip_id_to_stream_id: Dict[int, int] = { recip_id_to_stream_id: Dict[int, int] = {
stream["recipient_id"]: stream["id"] for stream in all_streams stream["recipient_id"]: stream["id"] for stream in all_streams

View File

@@ -136,7 +136,6 @@ class RawStreamDict(TypedDict):
can_remove_subscribers_group_id: int can_remove_subscribers_group_id: int
date_created: datetime.datetime date_created: datetime.datetime
description: str description: str
email_token: str
first_message_id: Optional[int] first_message_id: Optional[int]
history_public_to_subscribers: bool history_public_to_subscribers: bool
id: int id: int
@@ -178,7 +177,6 @@ class SubscriptionStreamDict(TypedDict):
date_created: int date_created: int
description: str description: str
desktop_notifications: Optional[bool] desktop_notifications: Optional[bool]
email_address: str
email_notifications: Optional[bool] email_notifications: Optional[bool]
first_message_id: Optional[int] first_message_id: Optional[int]
history_public_to_subscribers: bool history_public_to_subscribers: bool
@@ -254,7 +252,6 @@ class APISubscriptionDict(APIStreamDict):
push_notifications: Optional[bool] push_notifications: Optional[bool]
wildcard_mentions_notify: Optional[bool] wildcard_mentions_notify: Optional[bool]
# Computed fields not specified in `Subscription.API_FIELDS` # Computed fields not specified in `Subscription.API_FIELDS`
email_address: str
in_home_view: bool in_home_view: bool
stream_weekly_traffic: Optional[int] stream_weekly_traffic: Optional[int]
subscribers: List[int] subscribers: List[int]

View File

@@ -626,6 +626,9 @@ paths:
A list of dictionaries where each dictionary contains A list of dictionaries where each dictionary contains
information about one of the subscribed streams. information about one of the subscribed streams.
**Changes**: Removed `email_address` field from the dictionary
in Zulip 7.5 (feature level 186).
**Changes**: Removed `role` field from the dictionary **Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133). in Zulip 6.0 (feature level 133).
items: items:
@@ -658,7 +661,6 @@ paths:
"push_notifications": null, "push_notifications": null,
"wildcard_mentions_notify": null, "wildcard_mentions_notify": null,
"in_home_view": true, "in_home_view": true,
"email_address": "test_stream.af64447e9e39374841063747ade8e6b0.show-sender@testserver",
"stream_weekly_traffic": null, "stream_weekly_traffic": null,
"can_remove_subscribers_group_id": 2, "can_remove_subscribers_group_id": 2,
"subscribers": [10], "subscribers": [10],
@@ -8390,6 +8392,9 @@ paths:
A list of dictionaries where each dictionary contains A list of dictionaries where each dictionary contains
information about one of the subscribed streams. information about one of the subscribed streams.
**Changes**: Removed `email_address` field from the dictionary
in Zulip 7.5 (feature level 186).
**Changes**: Removed `role` field from the dictionary **Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133). in Zulip 6.0 (feature level 133).
items: items:
@@ -8405,7 +8410,6 @@ paths:
"color": "#e79ab5", "color": "#e79ab5",
"description": "A Scandinavian country", "description": "A Scandinavian country",
"desktop_notifications": true, "desktop_notifications": true,
"email_address": "Denmark+187b4125ed36d6af8b5d03ef4f65c0cf@zulipdev.com:9981",
"is_muted": false, "is_muted": false,
"invite_only": false, "invite_only": false,
"name": "Denmark", "name": "Denmark",
@@ -8419,7 +8423,6 @@ paths:
"color": "#e79ab5", "color": "#e79ab5",
"description": "Located in the United Kingdom", "description": "Located in the United Kingdom",
"desktop_notifications": true, "desktop_notifications": true,
"email_address": "Scotland+f5786390183e60a1ccb18374f9d05649@zulipdev.com:9981",
"is_muted": false, "is_muted": false,
"invite_only": false, "invite_only": false,
"name": "Scotland", "name": "Scotland",
@@ -11336,6 +11339,9 @@ paths:
of a stream the user is subscribed to (as well as that user's of a stream the user is subscribed to (as well as that user's
personal per-stream settings). personal per-stream settings).
**Changes**: Removed `email_address` field from the dictionary
in Zulip 7.5 (feature level 186).
**Changes**: Removed `role` field from the dictionary **Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133). in Zulip 6.0 (feature level 133).
unsubscribed: unsubscribed:
@@ -11352,6 +11358,9 @@ paths:
Unlike `never_subscribed`, the user might have messages in their personal Unlike `never_subscribed`, the user might have messages in their personal
message history that were sent to these streams. message history that were sent to these streams.
**Changes**: Removed `email_address` field from the dictionary
in Zulip 7.5 (feature level 186).
**Changes**: Removed `role` field from the dictionary **Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133). in Zulip 6.0 (feature level 133).
never_subscribed: never_subscribed:
@@ -17264,11 +17273,6 @@ components:
description: | description: |
A boolean specifying whether the given stream has been pinned A boolean specifying whether the given stream has been pinned
to the top. to the top.
email_address:
type: string
description: |
Email address of the given stream, used for
[sending emails to the stream](/help/message-a-stream-by-email).
is_muted: is_muted:
type: boolean type: boolean
description: | description: |

View File

@@ -2414,15 +2414,12 @@ class NormalActionsTest(BaseAction):
stream = self.make_stream(old_name) stream = self.make_stream(old_name)
self.subscribe(self.user_profile, stream.name) self.subscribe(self.user_profile, stream.name)
action = lambda: do_rename_stream(stream, new_name, self.user_profile) action = lambda: do_rename_stream(stream, new_name, self.user_profile)
events = self.verify_action(action, num_events=3, include_streams=include_streams) events = self.verify_action(action, num_events=2, include_streams=include_streams)
check_stream_update("events[0]", events[0]) check_stream_update("events[0]", events[0])
self.assertEqual(events[0]["name"], old_name) self.assertEqual(events[0]["name"], old_name)
check_stream_update("events[1]", events[1]) check_message("events[1]", events[1])
self.assertEqual(events[1]["name"], old_name)
check_message("events[2]", events[2])
fields = dict( fields = dict(
sender_email="notification-bot@zulip.com", sender_email="notification-bot@zulip.com",
@@ -2435,7 +2432,7 @@ class NormalActionsTest(BaseAction):
fields[TOPIC_NAME] = "stream events" fields[TOPIC_NAME] = "stream events"
msg = events[2]["message"] msg = events[1]["message"]
for k, v in fields.items(): for k, v in fields.items():
self.assertEqual(msg[k], v) self.assertEqual(msg[k], v)

View File

@@ -1475,8 +1475,8 @@ class StreamAdminTest(ZulipTestCase):
self.assertIn(cordelia.id, notified_user_ids) self.assertIn(cordelia.id, notified_user_ids)
self.assertNotIn(prospero.id, notified_user_ids) self.assertNotIn(prospero.id, notified_user_ids)
# Three events should be sent: a name event, an email address event and a notification event # Two events should be sent: a name event and a notification event
with self.capture_send_event_calls(expected_num_events=3) as events: with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = get_stream("private_stream", user_profile.realm).id stream_id = get_stream("private_stream", user_profile.realm).id
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "whatever"}) result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "whatever"})
self.assert_json_success(result) self.assert_json_success(result)
@@ -1512,12 +1512,12 @@ class StreamAdminTest(ZulipTestCase):
result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "sTREAm_name1"}) result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "sTREAm_name1"})
self.assert_json_success(result) self.assert_json_success(result)
# Three events should be sent: stream_email update, stream_name update and notification message. # Two events should be sent: stream_name update and notification message.
with self.capture_send_event_calls(expected_num_events=3) as events: with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = get_stream("stream_name1", user_profile.realm).id stream_id = get_stream("stream_name1", user_profile.realm).id
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "stream_name2"}) result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "stream_name2"})
self.assert_json_success(result) self.assert_json_success(result)
event = events[1]["event"] event = events[0]["event"]
self.assertEqual( self.assertEqual(
event, event,
dict( dict(
@@ -1529,7 +1529,7 @@ class StreamAdminTest(ZulipTestCase):
name="sTREAm_name1", name="sTREAm_name1",
), ),
) )
notified_user_ids = set(events[1]["users"]) notified_user_ids = set(events[0]["users"])
self.assertRaises(Stream.DoesNotExist, get_stream, "stream_name1", realm) self.assertRaises(Stream.DoesNotExist, get_stream, "stream_name1", realm)
@@ -1543,7 +1543,7 @@ class StreamAdminTest(ZulipTestCase):
# Test case to handle Unicode stream name change # Test case to handle Unicode stream name change
# *NOTE: Here encoding is needed when Unicode string is passed as an argument* # *NOTE: Here encoding is needed when Unicode string is passed as an argument*
with self.capture_send_event_calls(expected_num_events=3) as events: with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = stream_name2_exists.id stream_id = stream_name2_exists.id
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "नया नाम"}) result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "नया नाम"})
self.assert_json_success(result) self.assert_json_success(result)
@@ -1554,7 +1554,7 @@ class StreamAdminTest(ZulipTestCase):
# Test case to handle changing of Unicode stream name to newer name # Test case to handle changing of Unicode stream name to newer name
# NOTE: Unicode string being part of URL is handled cleanly # NOTE: Unicode string being part of URL is handled cleanly
# by client_patch call, encoding of URL is not needed. # by client_patch call, encoding of URL is not needed.
with self.capture_send_event_calls(expected_num_events=3) as events: with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = stream_name_uni_exists.id stream_id = stream_name_uni_exists.id
result = self.client_patch( result = self.client_patch(
f"/json/streams/{stream_id}", f"/json/streams/{stream_id}",
@@ -1568,7 +1568,7 @@ class StreamAdminTest(ZulipTestCase):
self.assertTrue(stream_name_new_uni_exists) self.assertTrue(stream_name_new_uni_exists)
# Test case to change name from one language to other. # Test case to change name from one language to other.
with self.capture_send_event_calls(expected_num_events=3) as events: with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = stream_name_new_uni_exists.id stream_id = stream_name_new_uni_exists.id
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français"}) result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français"})
self.assert_json_success(result) self.assert_json_success(result)
@@ -1576,7 +1576,7 @@ class StreamAdminTest(ZulipTestCase):
self.assertTrue(stream_name_fr_exists) self.assertTrue(stream_name_fr_exists)
# Test case to change name to mixed language name. # Test case to change name to mixed language name.
with self.capture_send_event_calls(expected_num_events=3) as events: with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = stream_name_fr_exists.id stream_id = stream_name_fr_exists.id
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français name"}) result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français name"})
self.assert_json_success(result) self.assert_json_success(result)
@@ -1588,14 +1588,14 @@ class StreamAdminTest(ZulipTestCase):
"stream_private_name1", realm=user_profile.realm, invite_only=True "stream_private_name1", realm=user_profile.realm, invite_only=True
) )
self.subscribe(self.example_user("cordelia"), "stream_private_name1") self.subscribe(self.example_user("cordelia"), "stream_private_name1")
with self.capture_send_event_calls(expected_num_events=3) as events: with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = get_stream("stream_private_name1", realm).id stream_id = get_stream("stream_private_name1", realm).id
result = self.client_patch( result = self.client_patch(
f"/json/streams/{stream_id}", f"/json/streams/{stream_id}",
{"new_name": "stream_private_name2"}, {"new_name": "stream_private_name2"},
) )
self.assert_json_success(result) self.assert_json_success(result)
notified_user_ids = set(events[1]["users"]) notified_user_ids = set(events[0]["users"])
self.assertEqual(notified_user_ids, can_access_stream_user_ids(stream_private)) self.assertEqual(notified_user_ids, can_access_stream_user_ids(stream_private))
self.assertIn(self.example_user("cordelia").id, notified_user_ids) self.assertIn(self.example_user("cordelia").id, notified_user_ids)
# An important corner case is that all organization admins are notified. # An important corner case is that all organization admins are notified.
@@ -5725,7 +5725,6 @@ class GetSubscribersTest(ZulipTestCase):
def verify_sub_fields(self, sub_data: SubscriptionInfo) -> None: def verify_sub_fields(self, sub_data: SubscriptionInfo) -> None:
other_fields = { other_fields = {
"email_address",
"is_announcement_only", "is_announcement_only",
"in_home_view", "in_home_view",
"stream_id", "stream_id",