Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 e6102af351
commit 432001656e
9 changed files with 51 additions and 92 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
api_docs/changelog.md
View File

@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with.
## Changes in Zulip 8.0
**Feature level 226**
* [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events),
[`GET /users/me/subscriptions`](/api/get-subscriptions): Removed
`email_address` field from subscription objects.
**Feature level 225**
* `PATCH /realm`, [`POST /register`](/api/register-queue),

1
web/tests/lib/events.js
View File

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

50
zerver/actions/streams.py
View File

@ -1,6 +1,5 @@
import hashlib
from collections import defaultdict
from dataclasses import dataclass
from typing import Any, Collection, Dict, Iterable, List, Mapping, Optional, Set, Tuple
from django.conf import settings
@ -23,7 +22,6 @@ from zerver.lib.cache import (
display_recipient_cache_key,
to_dict_cache_key_id,
)
from zerver.lib.email_mirror_helpers import encode_email_address
from zerver.lib.exceptions import JsonableError
from zerver.lib.mention import silent_mention_syntax_for_user
from zerver.lib.message import get_last_message_id
@ -353,12 +351,6 @@ def get_subscriber_ids(
return subscriptions_query.values_list("user_profile_id", flat=True)
@dataclass
class StreamInfo:
email_address: str
subscribers: List[int]
def send_subscription_add_events(
realm: Realm,
sub_info_list: List[SubInfo],
@ -371,27 +363,23 @@ def send_subscription_add_events(
stream_ids = {sub_info.stream.id for sub_info in sub_info_list}
recent_traffic = get_streams_traffic(stream_ids=stream_ids, realm=realm)
# We generally only have a few streams, so we compute stream
# We generally only have a few streams, so we compute subscriber
# data in its own loop.
stream_info_dict: Dict[int, StreamInfo] = {}
stream_subscribers_dict: Dict[int, List[int]] = {}
for sub_info in sub_info_list:
stream = sub_info.stream
if stream.id not in stream_info_dict:
email_address = encode_email_address(stream, show_sender=True)
if stream.id not in stream_subscribers_dict:
if stream.is_in_zephyr_realm and not stream.invite_only:
subscribers = []
else:
subscribers = list(subscriber_dict[stream.id])
stream_info_dict[stream.id] = StreamInfo(
email_address=email_address,
subscribers=subscribers,
)
stream_subscribers_dict[stream.id] = subscribers
for user_id, sub_infos in info_by_user.items():
sub_dicts: List[APISubscriptionDict] = []
for sub_info in sub_infos:
stream = sub_info.stream
stream_info = stream_info_dict[stream.id]
stream_subscribers = stream_subscribers_dict[stream.id]
subscription = sub_info.sub
stream_dict = stream_to_dict(stream, recent_traffic)
# This is verbose as we cannot unpack existing TypedDict
@ -408,10 +396,9 @@ def send_subscription_add_events(
push_notifications=subscription.push_notifications,
wildcard_mentions_notify=subscription.wildcard_mentions_notify,
# Computed fields not present in Subscription.API_FIELDS
email_address=stream_info.email_address,
in_home_view=not subscription.is_muted,
stream_weekly_traffic=stream_dict["stream_weekly_traffic"],
subscribers=stream_info.subscribers,
subscribers=stream_subscribers,
# Fields from Stream.API_FIELDS
can_remove_subscribers_group=stream_dict["can_remove_subscribers_group"],
date_created=stream_dict["date_created"],
@ -1232,7 +1219,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
stream.name = new_name
stream.save(update_fields=["name"])
@ -1263,26 +1250,14 @@ def do_rename_stream(stream: Stream, new_name: str, user_profile: UserProfile) -
# clearer than trying to set them. display_recipient is the out of
# date field in all cases.
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
# update stream.name = new_name where name = old_name
# and update stream.email = new_email where name = old_name.
# We could optimize this by trying to send one message, but the
# client code really wants one property update at a time, and
# updating stream names is a pretty infrequent operation.
# More importantly, we want to key these updates by id, not name,
# since id is the immutable primary key, and obviously name is not.
data_updates = [
["email_address", new_email],
["name", new_name],
]
for property, value in data_updates:
# We want to key these updates by id, not name, since id is
# the immutable primary key, and obviously name is not.
event = dict(
op="update",
type="stream",
property=property,
value=value,
property="name",
value=new_name,
stream_id=stream.id,
name=old_name,
)
@ -1299,9 +1274,6 @@ def do_rename_stream(stream: Stream, new_name: str, user_profile: UserProfile) -
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(

4
zerver/lib/event_schema.py
View File

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

11
zerver/lib/subscription_info.py
View File

@ -8,7 +8,6 @@ from django.db.models import QuerySet
from django.utils.translation import gettext as _
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.stream_color import STREAM_ASSIGNMENT_COLORS
from zerver.lib.stream_subscription import (
@ -59,7 +58,6 @@ def get_web_public_subs(realm: Realm) -> SubscriptionInfo:
audible_notifications = True
color = get_next_color()
desktop_notifications = True
email_address = ""
email_notifications = True
in_home_view = True
is_muted = False
@ -77,7 +75,6 @@ def get_web_public_subs(realm: Realm) -> SubscriptionInfo:
date_created=date_created,
description=description,
desktop_notifications=desktop_notifications,
email_address=email_address,
email_notifications=email_notifications,
first_message_id=first_message_id,
history_public_to_subscribers=history_public_to_subscribers,
@ -152,10 +149,6 @@ def build_stream_dict_for_sub(
else:
stream_weekly_traffic = None
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.
return SubscriptionStreamDict(
audible_notifications=audible_notifications,
@ -164,7 +157,6 @@ def build_stream_dict_for_sub(
date_created=date_created,
description=description,
desktop_notifications=desktop_notifications,
email_address=email_address,
email_notifications=email_notifications,
first_message_id=first_message_id,
history_public_to_subscribers=history_public_to_subscribers,
@ -410,9 +402,6 @@ def gather_subscriptions_helper(
# The realm_id and recipient_id are generally not needed in the API.
"realm_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] = {
stream["recipient_id"]: stream["id"] for stream in all_streams

3
zerver/lib/types.py
View File

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

20
zerver/openapi/zulip.yaml
View File

@ -650,6 +650,9 @@ paths:
A list of dictionaries where each dictionary contains
information about one of the subscribed streams.
**Changes**: Removed `email_address` field from the dictionary
in Zulip 8.0 (feature level 226).
**Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133).
items:
@ -682,7 +685,6 @@ paths:
"push_notifications": null,
"wildcard_mentions_notify": null,
"in_home_view": true,
"email_address": "test_stream.af64447e9e39374841063747ade8e6b0.show-sender@testserver",
"stream_weekly_traffic": null,
"can_remove_subscribers_group": 2,
"subscribers": [10],
@ -8785,6 +8787,9 @@ paths:
A list of dictionaries where each dictionary contains
information about one of the subscribed streams.
**Changes**: Removed `email_address` field from the dictionary
in Zulip 8.0 (feature level 226).
**Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133).
items:
@ -8800,7 +8805,6 @@ paths:
"color": "#e79ab5",
"description": "A Scandinavian country",
"desktop_notifications": true,
"email_address": "Denmark+187b4125ed36d6af8b5d03ef4f65c0cf@zulipdev.com:9981",
"is_muted": false,
"invite_only": false,
"name": "Denmark",
@ -8814,7 +8818,6 @@ paths:
"color": "#e79ab5",
"description": "Located in the United Kingdom",
"desktop_notifications": true,
"email_address": "Scotland+f5786390183e60a1ccb18374f9d05649@zulipdev.com:9981",
"is_muted": false,
"invite_only": false,
"name": "Scotland",
@ -12109,6 +12112,9 @@ paths:
of a stream the user is subscribed to (as well as that user's
personal per-stream settings).
**Changes**: Removed `email_address` field from the dictionary
in Zulip 8.0 (feature level 226).
**Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133).
unsubscribed:
@ -12125,6 +12131,9 @@ paths:
Unlike `never_subscribed`, the user might have messages in their personal
message history that were sent to these streams.
**Changes**: Removed `email_address` field from the dictionary
in Zulip 8.0 (feature level 226).
**Changes**: Removed `role` field from the dictionary
in Zulip 6.0 (feature level 133).
never_subscribed:
@ -18630,11 +18639,6 @@ components:
description: |
A boolean specifying whether the given stream has been pinned
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:
type: boolean
description: |

9
zerver/tests/test_events.py
View File

@ -2827,15 +2827,12 @@ class NormalActionsTest(BaseAction):
stream = self.make_stream(old_name)
self.subscribe(self.user_profile, stream.name)
action = partial(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])
self.assertEqual(events[0]["name"], old_name)
check_stream_update("events[1]", events[1])
self.assertEqual(events[1]["name"], old_name)
check_message("events[2]", events[2])
check_message("events[1]", events[1])
fields = dict(
sender_email="notification-bot@zulip.com",
@ -2848,7 +2845,7 @@ class NormalActionsTest(BaseAction):
fields[TOPIC_NAME] = "stream events"
msg = events[2]["message"]
msg = events[1]["message"]
for k, v in fields.items():
self.assertEqual(msg[k], v)

25
zerver/tests/test_subs.py
View File

@ -1569,8 +1569,8 @@ class StreamAdminTest(ZulipTestCase):
self.assertIn(cordelia.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
with self.capture_send_event_calls(expected_num_events=3) as events:
# Two events should be sent: a name event and a notification event
with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = get_stream("private_stream", user_profile.realm).id
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "whatever"})
self.assert_json_success(result)
@ -1606,12 +1606,12 @@ class StreamAdminTest(ZulipTestCase):
result = self.client_patch(f"/json/streams/{stream.id}", {"new_name": "sTREAm_name1"})
self.assert_json_success(result)
# Three events should be sent: stream_email update, stream_name update and notification message.
with self.capture_send_event_calls(expected_num_events=3) as events:
# Two events should be sent: stream_name update and notification message.
with self.capture_send_event_calls(expected_num_events=2) as events:
stream_id = get_stream("stream_name1", user_profile.realm).id
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "stream_name2"})
self.assert_json_success(result)
event = events[1]["event"]
event = events[0]["event"]
self.assertEqual(
event,
dict(
@ -1623,7 +1623,7 @@ class StreamAdminTest(ZulipTestCase):
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)
@ -1637,7 +1637,7 @@ class StreamAdminTest(ZulipTestCase):
# Test case to handle Unicode stream name change
# *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
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "नया नाम"})
self.assert_json_success(result)
@ -1648,7 +1648,7 @@ class StreamAdminTest(ZulipTestCase):
# Test case to handle changing of Unicode stream name to newer name
# NOTE: Unicode string being part of URL is handled cleanly
# 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
result = self.client_patch(
f"/json/streams/{stream_id}",
@ -1662,7 +1662,7 @@ class StreamAdminTest(ZulipTestCase):
self.assertTrue(stream_name_new_uni_exists)
# 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
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français"})
self.assert_json_success(result)
@ -1670,7 +1670,7 @@ class StreamAdminTest(ZulipTestCase):
self.assertTrue(stream_name_fr_exists)
# 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
result = self.client_patch(f"/json/streams/{stream_id}", {"new_name": "français name"})
self.assert_json_success(result)
@ -1682,14 +1682,14 @@ class StreamAdminTest(ZulipTestCase):
"stream_private_name1", realm=user_profile.realm, invite_only=True
)
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
result = self.client_patch(
f"/json/streams/{stream_id}",
{"new_name": "stream_private_name2"},
)
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.assertIn(self.example_user("cordelia").id, notified_user_ids)
# An important corner case is that all organization admins are notified.
@ -5844,7 +5844,6 @@ class GetSubscribersTest(ZulipTestCase):
def verify_sub_fields(self, sub_data: SubscriptionInfo) -> None:
other_fields = {
"email_address",
"is_announcement_only",
"in_home_view",
"stream_id",