From ada2991f1c3330c59347b1bdc5ec60b13a71dc24 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 6 Jul 2023 12:08:12 +0530 Subject: [PATCH] users: Send stream creation/deletion events on role change. We now send stream creation and stream deletion events on changing a user's role because a user can gain or lose access to some streams on changing their role. --- api_docs/changelog.md | 3 ++ version.py | 2 +- zerver/actions/users.py | 65 +++++++++++++++++++++++++++++++++++++ zerver/lib/events.py | 20 ------------ zerver/openapi/zulip.yaml | 15 +++++++-- zerver/tests/test_events.py | 40 +++++++++++++++++++++-- zerver/tests/test_users.py | 43 +++++++++++++++++++++--- 7 files changed, 157 insertions(+), 31 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 9bb6bfedb9..3d86e506b3 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -24,6 +24,9 @@ format used by the Zulip server that they are interacting with. * [`POST /register`](/api/register-queue): `streams` field in the response now included web-public streams as well. +* [`GET /events`](/api/get-events): Events for stream creation and deletion + are now sent to the user if they gain or lose access to some streams due + to change in their role. **Feature level 204** diff --git a/version.py b/version.py index aa818789af..c273b93491 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # Changes should be accompanied by documentation explaining what the # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 204 +API_FEATURE_LEVEL = 205 # Bump the minor PROVISION_VERSION to indicate that folks should provision # only when going from an old version of the code to a newer version. Bump diff --git a/zerver/actions/users.py b/zerver/actions/users.py index 276a01df60..750f16dafe 100644 --- a/zerver/actions/users.py +++ b/zerver/actions/users.py @@ -19,6 +19,9 @@ from zerver.lib.cache import bot_dict_fields from zerver.lib.create_user import create_user from zerver.lib.send_email import clear_scheduled_emails from zerver.lib.sessions import delete_user_sessions +from zerver.lib.stream_subscription import bulk_get_subscriber_peer_info +from zerver.lib.stream_traffic import get_streams_traffic +from zerver.lib.streams import get_streams_for_user, stream_to_dict from zerver.lib.user_counts import realm_user_count_by_role from zerver.lib.user_groups import get_system_user_group_for_user from zerver.lib.users import get_active_bots_owned_by_user @@ -28,6 +31,7 @@ from zerver.models import ( RealmAuditLog, Recipient, Service, + Stream, Subscription, UserGroupMembership, UserProfile, @@ -309,6 +313,59 @@ def do_deactivate_user( ) +def send_stream_events_for_role_update( + user_profile: UserProfile, old_accessible_streams: List[Stream] +) -> None: + current_accessible_streams = get_streams_for_user( + user_profile, + include_all_active=user_profile.is_realm_admin, + include_web_public=True, + ) + + old_accessible_stream_ids = {stream.id for stream in old_accessible_streams} + current_accessible_stream_ids = {stream.id for stream in current_accessible_streams} + + now_accessible_stream_ids = current_accessible_stream_ids - old_accessible_stream_ids + if now_accessible_stream_ids: + recent_traffic = get_streams_traffic(now_accessible_stream_ids, user_profile.realm) + + now_accessible_streams = [ + stream + for stream in current_accessible_streams + if stream.id in now_accessible_stream_ids + ] + event = dict( + type="stream", + op="create", + streams=[stream_to_dict(stream, recent_traffic) for stream in now_accessible_streams], + ) + send_event_on_commit(user_profile.realm, event, [user_profile.id]) + + subscriber_peer_info = bulk_get_subscriber_peer_info( + user_profile.realm, now_accessible_streams + ) + for stream_id, stream_subscriber_set in subscriber_peer_info.subscribed_ids.items(): + peer_add_event = dict( + type="subscription", + op="peer_add", + stream_ids=[stream_id], + user_ids=sorted(stream_subscriber_set), + ) + send_event_on_commit(user_profile.realm, peer_add_event, [user_profile.id]) + + now_inaccessible_stream_ids = old_accessible_stream_ids - current_accessible_stream_ids + if now_inaccessible_stream_ids: + now_inaccessible_streams = [ + stream for stream in old_accessible_streams if stream.id in now_inaccessible_stream_ids + ] + event = dict( + type="stream", + op="delete", + streams=[stream_to_dict(stream) for stream in now_inaccessible_streams], + ) + send_event_on_commit(user_profile.realm, event, [user_profile.id]) + + @transaction.atomic(durable=True) def do_change_user_role( user_profile: UserProfile, value: int, *, acting_user: Optional[UserProfile] @@ -316,6 +373,12 @@ def do_change_user_role( old_value = user_profile.role old_system_group = get_system_user_group_for_user(user_profile) + previously_accessible_streams = get_streams_for_user( + user_profile, + include_web_public=True, + include_all_active=user_profile.is_realm_admin, + ) + user_profile.role = value user_profile.save(update_fields=["role"]) RealmAuditLog.objects.create( @@ -372,6 +435,8 @@ def do_change_user_role( user_profile.realm, [user_profile.id], acting_user=acting_user ) + send_stream_events_for_role_update(user_profile, previously_accessible_streams) + def do_make_user_billing_admin(user_profile: UserProfile) -> None: user_profile.is_billing_admin = True diff --git a/zerver/lib/events.py b/zerver/lib/events.py index c213d03e48..139ef7376f 100644 --- a/zerver/lib/events.py +++ b/zerver/lib/events.py @@ -882,26 +882,6 @@ def apply_event( state["can_subscribe_other_users"] = user_profile.can_subscribe_other_users() state["can_invite_others_to_realm"] = user_profile.can_invite_others_to_realm() - # TODO: Probably rather than writing the perfect - # live-update code for the case of racing with the - # current user changing roles, we should just do a - # full refetch. - if "never_subscribed" in state: - sub_info = gather_subscriptions_helper( - user_profile, - include_subscribers=include_subscribers, - ) - state["subscriptions"] = sub_info.subscriptions - state["unsubscribed"] = sub_info.unsubscribed - state["never_subscribed"] = sub_info.never_subscribed - - if "streams" in state: - state["streams"] = do_get_streams( - user_profile, - include_web_public=True, - include_all_active=user_profile.is_realm_admin, - ) - if state["is_guest"]: state["realm_default_streams"] = [] else: diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index d478218a79..d1d6d6f3c6 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1165,12 +1165,16 @@ paths: This event is also sent when a user gains access to a stream they previously [could not access](/help/stream-permissions), such as - when a private stream is made public, or when a [guest user](/help/roles-and-permissions) - is subscribed to a public (or private) stream. + when a private stream is made public, their role changes or when + a [guest user](/help/roles-and-permissions) is subscribed to a + public (or private) stream. Note that organization administrators who are not subscribed will not be able to see content on the stream; just that it exists. + **Changes**: Prior to Zulip 8.0 (feature level 205), this event was + not sent when a user gained access to a stream due to role change. + **Changes**: Prior to Zulip 8.0 (feature level 192), this event was not sent when guest users gained access to a public stream by being subscribed. @@ -1222,6 +1226,13 @@ 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) when their roles + changes. + + **Changes**: Prior to Zulip 8.0 (feature level 205), this event was + not sent when a user lost access to a stream due to role change. properties: id: $ref: "#/components/schemas/EventIdSchema" diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index bad9fd2f53..00b63d6cb8 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1859,10 +1859,19 @@ class NormalActionsTest(BaseAction): self.user_profile.refresh_from_db() do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None) + + self.make_stream("Test private stream", invite_only=True) + self.subscribe(self.example_user("othello"), "Test private stream") + for role in [UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_MEMBER]: + if role == UserProfile.ROLE_REALM_ADMINISTRATOR: + num_events = 6 + else: + num_events = 5 + events = self.verify_action( lambda: do_change_user_role(self.user_profile, role, acting_user=None), - num_events=4, + num_events=num_events, ) check_realm_user_update("events[0]", events[0], "role") self.assertEqual(events[0]["person"]["role"], role) @@ -1872,8 +1881,11 @@ class NormalActionsTest(BaseAction): if role == UserProfile.ROLE_REALM_ADMINISTRATOR: check_user_group_remove_members("events[3]", events[3]) + check_stream_create("events[4]", events[4]) + check_subscription_peer_add("events[5]", events[5]) else: check_user_group_add_members("events[3]", events[3]) + check_stream_delete("events[4]", events[4]) def test_change_is_billing_admin(self) -> None: reset_email_visibility_to_everyone_in_zulip_realm() @@ -1896,10 +1908,18 @@ class NormalActionsTest(BaseAction): self.user_profile.refresh_from_db() do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None) + + self.make_stream("Test private stream", invite_only=True) + self.subscribe(self.example_user("othello"), "Test private stream") + for role in [UserProfile.ROLE_REALM_OWNER, UserProfile.ROLE_MEMBER]: + if role == UserProfile.ROLE_REALM_OWNER: + num_events = 6 + else: + num_events = 5 events = self.verify_action( lambda: do_change_user_role(self.user_profile, role, acting_user=None), - num_events=4, + num_events=num_events, ) check_realm_user_update("events[0]", events[0], "role") self.assertEqual(events[0]["person"]["role"], role) @@ -1909,8 +1929,11 @@ class NormalActionsTest(BaseAction): if role == UserProfile.ROLE_REALM_OWNER: check_user_group_remove_members("events[3]", events[3]) + check_stream_create("events[4]", events[4]) + check_subscription_peer_add("events[5]", events[5]) else: check_user_group_add_members("events[3]", events[3]) + check_stream_delete("events[4]", events[4]) def test_change_is_moderator(self) -> None: reset_email_visibility_to_everyone_in_zulip_realm() @@ -1950,9 +1973,16 @@ class NormalActionsTest(BaseAction): do_change_user_role(self.user_profile, UserProfile.ROLE_MEMBER, acting_user=None) for role in [UserProfile.ROLE_GUEST, UserProfile.ROLE_MEMBER]: + if role == UserProfile.ROLE_MEMBER: + # When changing role from guest to member, peer_add events are also sent + # to make sure the subscribers info is provided to the clients for the + # streams added by stream creation event. + num_events = 7 + else: + num_events = 5 events = self.verify_action( lambda: do_change_user_role(self.user_profile, role, acting_user=None), - num_events=4, + num_events=num_events, ) check_realm_user_update("events[0]", events[0], "role") self.assertEqual(events[0]["person"]["role"], role) @@ -1962,8 +1992,12 @@ class NormalActionsTest(BaseAction): if role == UserProfile.ROLE_GUEST: check_user_group_remove_members("events[3]", events[3]) + check_stream_delete("events[4]", events[4]) else: check_user_group_add_members("events[3]", events[3]) + check_stream_create("events[4]", events[4]) + check_subscription_peer_add("events[5]", events[5]) + check_subscription_peer_add("events[6]", events[6]) def test_change_notification_settings(self) -> None: for notification_setting in self.user_profile.notification_setting_types: diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index d4fc551574..d015ab52c9 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -220,7 +220,7 @@ class PermissionTest(ZulipTestCase): self.assertFalse(othello_dict["is_owner"]) req = dict(role=UserProfile.ROLE_REALM_OWNER) - with self.capture_send_event_calls(expected_num_events=4) as events: + with self.capture_send_event_calls(expected_num_events=6) as events: result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) owner_users = realm.get_human_owner_users() @@ -230,7 +230,7 @@ class PermissionTest(ZulipTestCase): self.assertEqual(person["role"], UserProfile.ROLE_REALM_OWNER) req = dict(role=UserProfile.ROLE_MEMBER) - with self.capture_send_event_calls(expected_num_events=4) as events: + with self.capture_send_event_calls(expected_num_events=5) as events: result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) owner_users = realm.get_human_owner_users() @@ -281,7 +281,7 @@ class PermissionTest(ZulipTestCase): # Giveth req = dict(role=orjson.dumps(UserProfile.ROLE_REALM_ADMINISTRATOR).decode()) - with self.capture_send_event_calls(expected_num_events=4) as events: + with self.capture_send_event_calls(expected_num_events=6) as events: result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) admin_users = realm.get_human_admin_users() @@ -292,7 +292,7 @@ class PermissionTest(ZulipTestCase): # Taketh away req = dict(role=orjson.dumps(UserProfile.ROLE_MEMBER).decode()) - with self.capture_send_event_calls(expected_num_events=4) as events: + with self.capture_send_event_calls(expected_num_events=5) as events: result = self.client_patch(f"/json/users/{othello.id}", req) self.assert_json_success(result) admin_users = realm.get_human_admin_users() @@ -518,9 +518,42 @@ class PermissionTest(ZulipTestCase): ) req = dict(role=orjson.dumps(new_role).decode()) + + # The basic events sent in all cases on changing role are - one event + # for changing role and one event each for adding and removing user + # from system user group. num_events = 3 + if UserProfile.ROLE_MEMBER in [old_role, new_role]: - num_events = 4 + # There is one additional event for adding/removing user from + # the "Full members" group as well. + num_events += 1 + + if new_role == UserProfile.ROLE_GUEST: + # There is one additional event deleting the unsubscribed public + # streams that the user will not able to access after becoming guest. + num_events += 1 + + if old_role == UserProfile.ROLE_GUEST: + # User will receive one event for creation of unsubscribed public + # (and private, if the new role is owner or admin) streams that + # they did not have access to previously and 3 more peer_add + # events for each of the public stream. + if new_role in [UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_REALM_OWNER]: + # If the new role is owner or admin, the peer_add event will be + # sent for one private stream as well. + num_events += 5 + else: + num_events += 4 + elif new_role in [ + UserProfile.ROLE_REALM_ADMINISTRATOR, + UserProfile.ROLE_REALM_OWNER, + ] and old_role not in [UserProfile.ROLE_REALM_ADMINISTRATOR, UserProfile.ROLE_REALM_OWNER]: + # If old_role is not guest and user's role is changed to admin or owner from moderator + # or member, then the user gains access to unsubscribed private streams and thus + # receives one event for stream creation and one peer_add event for it. + num_events += 2 + with self.capture_send_event_calls(expected_num_events=num_events) as events: result = self.client_patch(f"/json/users/{user_profile.id}", req) self.assert_json_success(result)