Skip to content

Commit

Permalink
streams: Send stream deletion events on unsubscribing users.
Browse files Browse the repository at this point in the history
This commit adds code to send stream deletion events when
unsubscribing non-admin users from private streams and
when unsubscribing guests from public streams since
non-admins cannot access unsubscribed private streams
and guests cannot access unsubscribed public streams.
  • Loading branch information
sahil839 authored and alexmv committed Nov 16, 2023
1 parent 6336322 commit ee6ab3d
Show file tree
Hide file tree
Showing 5 changed files with 77 additions and 3 deletions.
15 changes: 15 additions & 0 deletions zerver/actions/streams.py
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@
from zerver.lib.stream_traffic import get_average_weekly_stream_traffic, get_streams_traffic
from zerver.lib.streams import (
can_access_stream_user_ids,
check_basic_stream_access,
get_occupied_streams,
get_stream_permission_policy_name,
render_stream_description,
Expand Down Expand Up @@ -741,6 +742,20 @@ def send_subscription_remove_events(
}
queue_json_publish("deferred_work", event)

if not user_profile.is_realm_admin:
inaccessible_streams = [
stream
for stream in streams_by_user[user_profile.id]
if not check_basic_stream_access(
user_profile, stream, sub=None, allow_realm_admin=True
)
]

if inaccessible_streams:
payload = [stream.to_dict() for stream in inaccessible_streams]
event = dict(type="stream", op="delete", streams=payload)
send_event_on_commit(realm, event, [user_profile.id])

send_peer_remove_events(
realm=realm,
streams=streams,
Expand Down
7 changes: 7 additions & 0 deletions zerver/lib/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -1056,6 +1056,13 @@ def _draft_update_action(i: int) -> None:
state["streams"] = [
s for s in state["streams"] if s["stream_id"] not in deleted_stream_ids
]

state["unsubscribed"] = [
stream
for stream in state["unsubscribed"]
if stream["stream_id"] not in deleted_stream_ids
]

state["never_subscribed"] = [
stream
for stream in state["never_subscribed"]
Expand Down
4 changes: 4 additions & 0 deletions zerver/openapi/zulip.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1185,6 +1185,10 @@ paths:
- type: object
description: |
Event sent to all users who can see a stream when it is deactivated.

This event is also sent when a user loses access to a stream they
previously [could access](/help/stream-permissions) because they
are unsubscribed from a private stream.
properties:
id:
$ref: "#/components/schemas/EventIdSchema"
Expand Down
34 changes: 34 additions & 0 deletions zerver/tests/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -3271,6 +3271,40 @@ def do_test_subscribe_events(self, include_subscribers: bool) -> None:
10,
)

# Add this user to make sure the stream is not deleted on unsubscribing hamlet.
self.subscribe(self.example_user("iago"), stream.name)

# Unsubscribe from invite-only stream.
action = lambda: bulk_remove_subscriptions(realm, [hamlet], [stream], acting_user=None)
events = self.verify_action(action, include_subscribers=include_subscribers, num_events=2)
check_subscription_remove("events[0]", events[0])
check_stream_delete("events[1]", events[1])

stream.invite_only = False
stream.save()

# Test events for guest user.
self.user_profile = self.example_user("polonius")

self.subscribe(self.user_profile, stream.name)
# Unsubscribe guest from public stream.
action = lambda: bulk_remove_subscriptions(
realm, [self.user_profile], [stream], acting_user=None
)
events = self.verify_action(action, include_subscribers=include_subscribers, num_events=2)
check_subscription_remove("events[0]", events[0])
check_stream_delete("events[1]", events[1])

stream = self.make_stream("web-public-stream", is_web_public=True)
self.subscribe(self.user_profile, stream.name)
# Unsubscribe as a guest to web-public stream. Guest does not receive stream deletion
# event for web-public stream.
action = lambda: bulk_remove_subscriptions(
user_profile.realm, [self.user_profile], [stream], acting_user=None
)
events = self.verify_action(action, include_subscribers=include_subscribers, num_events=1)
check_subscription_remove("events[0]", events[0])


class DraftActionTest(BaseAction):
def do_enable_drafts_synchronization(self, user_profile: UserProfile) -> None:
Expand Down
20 changes: 17 additions & 3 deletions zerver/tests/test_subs.py
Original file line number Diff line number Diff line change
Expand Up @@ -4777,8 +4777,9 @@ def test_users_getting_remove_peer_event(self) -> None:
self.subscribe(user2, "private_stream")
self.subscribe(user3, "private_stream")

# Sends 3 peer-remove events and 2 unsubscribe events.
with self.capture_send_event_calls(expected_num_events=5) as events:
# Sends 3 peer-remove events and 2 unsubscribe events
# and 2 stream delete events for private streams.
with self.capture_send_event_calls(expected_num_events=7) as events:
with self.assert_database_query_count(16):
with cache_tries_captured() as cache_count:
bulk_remove_subscriptions(
Expand All @@ -4791,6 +4792,11 @@ def test_users_getting_remove_peer_event(self) -> None:
self.assert_length(cache_count, 3)

peer_events = [e for e in events if e["event"].get("op") == "peer_remove"]
stream_delete_events = [
e
for e in events
if e["event"].get("type") == "stream" and e["event"].get("op") == "delete"
]

# We only care about a subset of users when we inspect
# peer_remove events.
Expand Down Expand Up @@ -4822,6 +4828,14 @@ def test_users_getting_remove_peer_event(self) -> None:
],
)

self.assert_length(stream_delete_events, 2)
self.assertEqual(stream_delete_events[0]["users"], [user1.id])
self.assertEqual(stream_delete_events[1]["users"], [user2.id])
for event in stream_delete_events:
event_streams = event["event"]["streams"]
self.assert_length(event_streams, 1)
self.assertEqual(event_streams[0]["name"], "private_stream")

def test_bulk_subscribe_MIT(self) -> None:
mit_user = self.mit_user("starnine")
num_streams = 15
Expand Down Expand Up @@ -4853,7 +4867,7 @@ def test_bulk_subscribe_MIT(self) -> None:
# Followed by one subscription event:
self.assertEqual(events[num_streams]["event"]["type"], "subscription")

with self.capture_send_event_calls(expected_num_events=1):
with self.capture_send_event_calls(expected_num_events=2):
bulk_remove_subscriptions(
realm,
users=[mit_user],
Expand Down

0 comments on commit ee6ab3d

Please sign in to comment.