From 3e6463804e06fb7b769b8e0e5a5d401c38fa1421 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Mon, 30 May 2022 17:23:52 +0530 Subject: [PATCH] streams: Allow changing history access without is_private parameter. We now allow changing access to history of the stream by only passing "history_public_to_subscribers" parameter. Previously, "is_private" parameter was also required to change history_public_to_subscribers otherwise the request was silently ignored. We also raise error when only history_public_to_subscribers parameter is passed with value False without "is_private: True" for a public or web-public stream since we do not allow public streams with protected history. --- templates/zerver/api/changelog.md | 10 ++++ version.py | 2 +- zerver/actions/streams.py | 6 ++- zerver/openapi/zulip.yaml | 53 +++++++++++++++----- zerver/tests/test_subs.py | 81 +++++++++++++++++++++++++++++++ zerver/views/streams.py | 10 +++- 6 files changed, 148 insertions(+), 14 deletions(-) diff --git a/templates/zerver/api/changelog.md b/templates/zerver/api/changelog.md index 399aa396d1..c8387e7bf3 100644 --- a/templates/zerver/api/changelog.md +++ b/templates/zerver/api/changelog.md @@ -20,6 +20,16 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 6.0 +**Feature level 136** + +* [`PATCH /streams/{stream_id}`](/api/update-stream): The endpoint + now returns an error for a request to make a public stream with + protected history which was previously ignored silently. +* [`PATCH /streams/{stream_id}`](/api/update-stream): Added support + to change access to history of the stream by only passing + `history_public_to_subscribers` parameter without `is_private` + and `is_web_public` parameters. + **Feature level 135** * [`DELETE /user/{user_id}`](/api/deactivate-user): Added diff --git a/version.py b/version.py index 21965485ce..a0d5f9a06d 100644 --- a/version.py +++ b/version.py @@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.4.3" # Changes should be accompanied by documentation explaining what the # new level means in templates/zerver/api/changelog.md, as well as # "**Changes**" entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 135 +API_FEATURE_LEVEL = 136 # 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/streams.py b/zerver/actions/streams.py index 6599385de5..7d1c1b4b34 100644 --- a/zerver/actions/streams.py +++ b/zerver/actions/streams.py @@ -855,8 +855,12 @@ def do_change_stream_permission( stream.invite_only = False stream.history_public_to_subscribers = True else: - assert invite_only is not None # is_web_public is falsey + if invite_only is None: + # This is necessary to get correct default value for + # history_public_to_subscribers when invite_only is + # None. + invite_only = stream.invite_only history_public_to_subscribers = get_default_value_for_history_public_to_subscribers( stream.realm, invite_only, diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 8cb56a890a..6b67205fd0 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -13751,8 +13751,27 @@ paths: type: boolean example: true required: false + - name: history_public_to_subscribers + in: query + description: | + Whether the stream's message history should be available to + newly subscribed members, or users can only access messages + they actually received while subscribed to the stream. + + Corresponds to the [shared history](/help/stream-permissions) + option in documentation. + + It's an error for this parameter to be false for a public or + web-public stream and when is_private is false. + + **Changes**: Before Zulip 6.0 (feature level 136), `history_public_to_subscribers` + was silently ignored unless the request also contained either `is_private` or + `is_web_public`. + schema: + type: boolean + example: false + required: false - $ref: "#/components/parameters/StreamPostPolicy" - - $ref: "#/components/parameters/HistoryPublicToSubscribers" - $ref: "#/components/parameters/MessageRetentionDays" responses: "200": @@ -13762,16 +13781,28 @@ paths: content: application/json: schema: - allOf: - - $ref: "#/components/schemas/JsonError" - - example: - { - "code": "BAD_REQUEST", - "msg": "Invalid stream ID", - "result": "error", - } - description: | - An example JSON response for when the supplied stream does not exist: + oneOf: + - allOf: + - $ref: "#/components/schemas/JsonError" + - example: + { + "code": "BAD_REQUEST", + "msg": "Invalid stream ID", + "result": "error", + } + description: | + An example JSON response for when the supplied stream does not exist: + - allOf: + - $ref: "#/components/schemas/JsonError" + - example: + { + "code": "BAD_REQUEST", + "msg": "Invalid parameters", + "result": "error", + } + description: | + An example JSON response for when invalid combination of stream permission + parameters are passed. /streams/{stream_id}/delete_topic: post: operationId: delete-topic diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index 6477c1313c..9640e54607 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -879,6 +879,77 @@ class StreamAdminTest(ZulipTestCase): ).decode() self.assertEqual(realm_audit_log.extra_data, expected_extra_data) + def test_change_history_access_for_private_streams(self) -> None: + user_profile = self.example_user("iago") + self.login_user(user_profile) + realm = user_profile.realm + self.make_stream("private_stream", realm=realm, invite_only=True) + stream_id = self.subscribe(user_profile, "private_stream").id + + params = { + "history_public_to_subscribers": orjson.dumps(True).decode(), + } + result = self.client_patch(f"/json/streams/{stream_id}", params) + self.assert_json_success(result) + + stream = get_stream("private_stream", realm) + self.assertTrue(stream.invite_only) + self.assertTrue(stream.history_public_to_subscribers) + + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 1) + expected_notification = ( + f"@_**Iago|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Private, protected history** to **Private, shared history**." + ) + self.assertEqual(messages[0].content, expected_notification) + + realm_audit_log = RealmAuditLog.objects.filter( + event_type=RealmAuditLog.STREAM_PROPERTY_CHANGED, + modified_stream=stream, + ).last() + assert realm_audit_log is not None + expected_extra_data = orjson.dumps( + { + RealmAuditLog.OLD_VALUE: False, + RealmAuditLog.NEW_VALUE: True, + "property": "history_public_to_subscribers", + } + ).decode() + self.assertEqual(realm_audit_log.extra_data, expected_extra_data) + + params = { + "history_public_to_subscribers": orjson.dumps(False).decode(), + } + result = self.client_patch(f"/json/streams/{stream_id}", params) + self.assert_json_success(result) + + stream = get_stream("private_stream", realm) + self.assertTrue(stream.invite_only) + self.assertFalse(stream.history_public_to_subscribers) + + messages = get_topic_messages(user_profile, stream, "stream events") + self.assert_length(messages, 2) + expected_notification = ( + f"@_**Iago|{user_profile.id}** changed the [access permissions](/help/stream-permissions) " + "for this stream from **Private, shared history** to **Private, protected history**." + ) + self.assertEqual(messages[1].content, expected_notification) + + realm_audit_log = RealmAuditLog.objects.filter( + event_type=RealmAuditLog.STREAM_PROPERTY_CHANGED, + modified_stream=stream, + ).last() + assert realm_audit_log is not None + expected_extra_data = orjson.dumps( + { + RealmAuditLog.OLD_VALUE: True, + RealmAuditLog.NEW_VALUE: False, + "property": "history_public_to_subscribers", + } + ).decode() + self.assertEqual(realm_audit_log.extra_data, expected_extra_data) + def test_stream_permission_changes_updates_updates_attachments(self) -> None: self.login("desdemona") fp = StringIO("zulip!") @@ -1037,6 +1108,16 @@ class StreamAdminTest(ZulipTestCase): result = self.client_patch(f"/json/streams/{stream_id}", params) self.assert_json_error(result, "Invalid parameters") + params = { + "history_public_to_subscribers": orjson.dumps(False).decode(), + } + result = self.client_patch(f"/json/streams/{stream_id}", params) + self.assert_json_error(result, "Invalid parameters") + + web_public_stream = self.make_stream("web_public_stream", realm=realm, is_web_public=True) + result = self.client_patch(f"/json/streams/{web_public_stream.id}", params) + self.assert_json_error(result, "Invalid parameters") + def test_subscriber_ids_with_stream_history_access(self) -> None: hamlet = self.example_user("hamlet") polonius = self.example_user("polonius") diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 3a3c931870..0bdceedf13 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -335,7 +335,15 @@ def update_stream_backend( if is_private or history_public_to_subscribers is False: raise JsonableError(_("Invalid parameters")) - if is_private is not None or is_web_public is not None: + if history_public_to_subscribers is False and not stream.realm.is_zephyr_mirror_realm: + if is_private is None and not stream.invite_only: + raise JsonableError(_("Invalid parameters")) + + if ( + is_private is not None + or is_web_public is not None + or history_public_to_subscribers is not None + ): do_change_stream_permission( stream, invite_only=is_private,