From d9740045a5aa61b3031e713eec7ba75a2d7856c0 Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Thu, 14 Jan 2021 15:18:42 +0000 Subject: [PATCH] refactor: Eliminate checks in build_stream_dict_for_sub. We eliminate some redundant checks. We also consistently provide a `subscribers` field in our stream data with `[]`, even if our users can't access subscribers. We therefore bump the API version and tweak the docs. (See further down for a detailed justification of the change.) Even though it is sometimes fine to have redundant code that is defensive in nature, some upcoming changes are gonna move subscriber-related logic out of build_stream_dict_for_sub for certain codepaths as part of our effort to streamline the payload for subscribers within page_params. So we can't rely on the code that I removed here inside of build_stream_dict_for_sub. Anyway, it makes more sense to do these checks explicitly in the validate function. The code in build_stream_dict_for_sub was almost effectively a noop, since the validation function was already preventing us from getting subscriber info. The only difference it made was sometimes converting `[]` to `None`, and then subsequently omitting the subscribers field. Neither ZT nor the webapp make any distinction between `[]` or for the `subscribers` data in `page_params`. The webapp has had this code for a long time (and now equivalent code elsewhere in this PR): if (!Object.prototype.hasOwnProperty.call(sub, "subscribers")) { sub.subscribers = new LazySet([]); } The webapp calculates access based on booleans, anyway: sub.can_access_subscribers = page_params.is_admin || sub.subscribed || (!page_params.is_guest && !sub.invite_only); And ZT would choke if `subscribers` were missing, except that it never gets to the relevant code due to other checks: def get_other_subscribers_in_stream(): assert stream_id is not None or stream_name is not None if stream_id: assert self.is_user_subscribed_to_stream(stream_id) return [sub for sub in self.stream_dict[stream_id]['subscribers'] if sub != self.user_id] else: return [sub for _, stream in self.stream_dict.items() for sub in stream['subscribers'] if stream['name'] == stream_name if sub != self.user_id] You could make a semantic argument that we should prefer to `[]` when subscribers aren't even available, but we have precedent from the way that `bulk_get_subscriber_user_ids` has traditionally populated its result: result: Dict[int, List[int]] = {stream["id"]: [] for stream in stream_dicts} If we changed `stream_dicts` to `target_stream_dicts` we would faciliate a move toward `None`, but it would just cause headaches for other server code as well as the frontends (which, to reiterate, already prefer the empty array for convenience). --- templates/zerver/api/changelog.md | 6 ++++++ version.py | 2 +- zerver/lib/actions.py | 13 +++---------- zerver/openapi/zulip.yaml | 6 ++++++ zerver/tests/test_subs.py | 4 ++-- 5 files changed, 18 insertions(+), 13 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 48f7419f96..ff41e7bd34 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -10,6 +10,12 @@ below features are supported. ## Changes in Zulip 4.0 +**Feature level 37** + +* Consistently provide `subscribers` in stream data when + clients register for subscriptions with `include_subscribers`, + even if the user can't access subscribers. + **Feature level 36** * [`POST /users`](/api/create-user): Restricted access to organization diff --git a/version.py b/version.py index 655d81fefa..f4dd698314 100644 --- a/version.py +++ b/version.py @@ -28,7 +28,7 @@ DESKTOP_WARNING_VERSION = "5.2.0" # # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md. -API_FEATURE_LEVEL = 36 +API_FEATURE_LEVEL = 37 # 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/lib/actions.py b/zerver/lib/actions.py index 31cc0bf5f2..40e214c9e9 100644 --- a/zerver/lib/actions.py +++ b/zerver/lib/actions.py @@ -2666,7 +2666,9 @@ def validate_user_access_to_subscribers_helper( if stream_dict["is_web_public"]: return - # Guest users can access subscribed public stream's subscribers + # With the exception of web public streams, a guest must + # be subscribed to a stream (even a public one) in order + # to see subscribers. if user_profile.is_guest: if check_user_subscribed(user_profile): return @@ -5056,15 +5058,6 @@ def build_stream_dict_for_sub( result["email_address"] = encode_email_address_helper( stream["name"], stream["email_token"], show_sender=True) - # Important: don't show the subscribers if the stream is invite only - # and this user isn't on it anymore (or a realm administrator). - if stream["invite_only"] and not (sub["active"] or user.is_realm_admin): - subscribers = None - - # Guest users lose access to subscribers when they are unsubscribed if the stream - # is not web-public. - if not sub["active"] and user.is_guest and not stream["is_web_public"]: - subscribers = None if subscribers is not None: result["subscribers"] = subscribers diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 4bed3f6a43..07829a6627 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -6226,6 +6226,12 @@ paths: description: | A list of user IDs of users who are subscribed to the stream. Included only if `include_subscribers` is `true`. + + If a user is not allowed to know the subscribers for + a stream, we will send an empty array. API authors + should use other data to determine whether users like + guest users are forbidden to know the subscribers. + description: | Present if `subscription` is present in `fetch_event_types`. diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index d62ca4a7a8..68afa89f25 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -4425,12 +4425,12 @@ class GetSubscribersTest(ZulipTestCase): sub_data = gather_subscriptions_helper(non_admin_user) unsubscribed_streams = sub_data[1] self.assertEqual(len(unsubscribed_streams), 1) - self.assertFalse('subscribers' in unsubscribed_streams[0]) + self.assertEqual(unsubscribed_streams[0]['subscribers'], []) sub_data = gather_subscriptions_helper(guest_user) unsubscribed_streams = sub_data[1] self.assertEqual(len(unsubscribed_streams), 1) - self.assertFalse('subscribers' in unsubscribed_streams[0]) + self.assertEqual(unsubscribed_streams[0]['subscribers'], []) def test_gather_subscriptions_mit(self) -> None: """