Create "support" stream type with topic-specific permissions #19434 #29520

sanchi-t · 2024-03-30T06:22:33Z

Fixes: #19434

This PR introduces support for creating private support-type streams, where topics are accessible only to specific user groups. Members of these designated groups can initiate new topics and participate in discussions within topics they create. Conversely, users outside these restricted groups can view all messages and topics within the stream.

To implement this functionality, a new stream setting named "who can access all topics?" is introduced. This setting is defined by a new stream property called is_stream_access_group. By default, the is_stream_access_group property will be assigned the value "role:everyone," indicating that all users will have access to all topics and messages within the stream.

Screenshots and screen captures:

Self-review checklist

Self-reviewed the changes for clarity and maintainability
(variable names, code reuse, readability, etc.).

Communicate decisions, questions, and potential concerns.

Explains differences from previous plans (e.g., issue description).
Highlights technical choices and bugs encountered.
Calls out remaining decisions and concerns.
Automated tests verify logic where appropriate.

Individual commits are ready for review (see commit discipline).

Each commit is a coherent idea.
Commit message(s) explain reasoning and motivation for changes.

Completed manual review and testing of the following:

Visual appearance of the changes.
Responsiveness and internationalization.
Strings and tooltips.
End-to-end functionality of buttons, interactions and flows.
Corner cases, error conditions, and easily imagined bugs.

zulipbot · 2024-03-30T06:22:37Z

Hello @sanchi-t, it seems like you have referenced #19434 in your pull request description, but you have not referenced them in your commit message description(s). Referencing an issue in a commit message automatically closes the corresponding issue when the commit is merged, which makes the issue tracker easier to manage.

Please run git commit --amend in your command line client to amend your commit message description with Fixes #19434..

An example of a correctly-formatted commit:

commit fabd5e450374c8dde65ec35f02140383940fe146
Author: zulipbot
Date:   Sat Mar 18 13:42:40 2017 -0700

    pull requests: Check PR commits reference when issue is referenced.

    Fixes #51.

To learn how to write a great commit message, please refer to our guide.

timabbott · 2024-04-20T20:03:07Z

web/src/stream_popover.js

@@ -173,6 +173,7 @@ function build_stream_popover(opts) {
                $(this).closest(".popover").fadeOut(500).delay(500).remove();

                const sub = stream_popover_sub(e);
+                hide_stream_popover();


Is this an existing bug we should fix via a preparatory PR?

No no I accidentally added an unrelated commit while rebasing.

timabbott · 2024-04-20T20:03:39Z

zerver/migrations/0498_stream_stream_topic_view_policy.py

+        migrations.AddField(
+            model_name="stream",
+            name="stream_topic_view_policy",
+            field=models.PositiveSmallIntegerField(default=STREAM_POST_POLICY_EVERYONE),


This should probably be a UserGroup foreign key.

timabbott · 2024-04-20T20:04:16Z

zerver/openapi/zulip.yaml

+
+                                    Whether 
+
+                                    **Changes**: New in Zulip 5.0 (feature level 71).


How'd you get that version number? Be careful when copy-pasting :P

Yeah, I was just creating a template for myself to work on later. This PR wasn't ready for review back then.

timabbott · 2024-04-20T20:05:55Z

web/src/settings_config.ts

+                "Must be invited by a subscriber; Topics created by others will be hidden from non-administrator users",
+        }),
+    },
+


This should likely be an independent setting, rather than a private type, in the UI.

I also generally recommend putting "add UI for creating a thing" commits at the end of a branch, after all the commits that make the feature work as designed... since it's generally much easier to merge commits that implement a stream type without actually having any exist, than it is to have theme exist but have incomplete/unfinished behavior.

timabbott · 2024-04-20T20:07:22Z

zerver/actions/message_send.py

-            "enable_offline_push_notifications",
-            "is_bot",
-            "bot_type",
-            "long_term_idle",
        )


Removing this optimization is likely not a good idea.

timabbott · 2024-04-20T20:07:38Z

zerver/actions/message_send.py

    )

+    if stream and stream.is_support_stream and not message_sender.is_bot:
+        realm = get_realm_by_id(realm_id)


This is available as stream.realm; no need to fetch it separately.

timabbott · 2024-04-20T20:08:34Z

zerver/lib/topic.py

+
+    return first_message_in_topic.sender_id
+
+def check_access_based_on_stream_topic_view_policy(sender: UserProfile, stream: Stream) -> bool:


This should become a group membership check with the new groups-based permissions systems, which should be much cheaper and also a lot less semi-duplicated code to write.

timabbott · 2024-04-20T20:09:42Z

zerver/actions/message_send.py

+            authorized_user_ids 
+            + ([topic_creator_user_id] if topic_creator_user_id is not None else []) 
+            + [sender_id]
+        )


This might be cleaner with just a set.add(sender_id) type line after initializing the starting set.

timabbott · 2024-04-20T20:11:25Z

zerver/actions/message_send.py

+        if stream.is_support_stream and not check_access_based_on_stream_topic_view_policy(sender,stream):
+            topic_creator_user_id = get_topic_creator_user_id(realm.id,stream.recipient_id,topic_name)
+            if topic_creator_user_id and not (sender.id == topic_creator_user_id):
+                raise UnauthorizedTopicAccessError


So as written, you're calling topic_creator_user_id twice when sending a single message; you'll want to avoid that by just calling it once here, and passing it forward if get_recipient_info also needs the value.

Yeah fixed that.

timabbott · 2024-04-20T20:13:17Z

I posted an initial review on this, but I think it probably makes sense for you to prioritize on other projects -- I'm not entirely sure after reading this that we have the right product design for this feature.

timabbott · 2024-04-22T18:43:12Z

I talked to @alya a bit and I think we're closer to having the right product design for this than I'd thought, so feel free to work on this more -- the main reworking required is just using groups to avoid having to redo everything as part of #19525.

The `stream_topic_access_group` field have been added to the stream model and across various types. The new fields causes the query length to be increased by 34 in `test_subs`

`namedusergroup` is also included in the `ID_MAP`.

Using `stream_topic_access_group.name` in `is_support_stream` function is causing the increase in database query count.

sanchi-t · 2024-05-14T20:51:18Z

zerver/migrations/0521_stream_stream_topic_access_group.py

+            model_name="stream",
+            name="stream_topic_access_group",
+            field=models.ForeignKey(
+                null=True, on_delete=django.db.models.deletion.RESTRICT, to="zerver.namedusergroup"


The only reason I am writing NamedUserGroup as foreign key instead of UserGroup is that I want to use to is_support_stream function defined here.

With UserGroup there will be no name property on stream_topic_access_group and I will have to do something like this which I think will be a heavier database operation:

NamedUserGroup.objects.get( id=stream_topic_access_group_id, realm=realm, is_system_group=True )

sanchi-t · 2024-05-14T20:53:23Z

zerver/models/streams.py

+    def is_support_stream(self) -> bool:
+        return self.stream_topic_access_group.name != SystemGroups.EVERYONE
+


Accessing stream_topic_access_group.name is leading to an inflated number of database queries. Unfortunately, I can't find a way around.

The `check_access_based_on_stream_topic_view_policy` function, similar to `check_stream_access_based_on_stream_post_policy`, determines restricted users based on stream_topic_view_policy. In `do_send_messages`, `user_ids` are derived from `um_eligible_user_ids` rather than `active_user_ids` for support streams. This ensures that server events for new messages aren't sent to users who lack access to those messages.

We need to check that the user is not restricted in order to allow them to view messages in support stream.

`is_support_stream` and `stream_topic_view_policy` have been added to various types to align with the stream model.

We need to check if a restricted user can access the topic before echoing their message, which happens on the server side.

sanchi-t · 2024-05-14T21:07:15Z

zerver/lib/user_groups.py

 def is_user_in_group(
-    user_group: UserGroup, user: UserProfile, *, direct_member_only: bool = False
+    user_group: UserGroup, user_id: int, *, direct_member_only: bool = False
 ) -> bool:
    if direct_member_only:
-        return get_user_group_direct_members(user_group=user_group).filter(id=user.id).exists()
+        return get_user_group_direct_members(user_group=user_group).filter(id=user_id).exists()

-    return get_recursive_group_members(user_group=user_group).filter(id=user.id).exists()
+    return get_recursive_group_members(user_group=user_group).filter(id=user_id).exists()


The changes were made to maintain the optimization outlined in function get_recipient_info in message_send.py, as shown below:

query: ValuesQuerySet[UserProfile, ActiveUserDict] = UserProfile.objects.filter( is_active=True ).values( "id", "enable_online_push_notifications", "enable_offline_email_notifications", "enable_offline_push_notifications", "is_bot", "bot_type", "long_term_idle", )

As later in the function get_recipient_info we need to call check_access_based_on_stream_topic_access_group which would then call is_user_in_group.

zulipbot added the size: XL label Mar 30, 2024

zulipbot added the has conflicts label Apr 2, 2024

timabbott reviewed Apr 20, 2024

View reviewed changes

sanchi-t force-pushed the issue-19434 branch from e4cf1e1 to 75e526e Compare April 28, 2024 14:12

zulipbot added has conflicts and removed has conflicts labels Apr 28, 2024

sanchi-t force-pushed the issue-19434 branch from 75e526e to 38cd080 Compare May 14, 2024 12:51

sanchi-t changed the title ~~Create "support" stream type with topic-specific permissions~~ Create "support" stream type with topic-specific permissions #19434 May 14, 2024

sanchi-t added 7 commits May 15, 2024 01:34

is_user_in_group: Use user_id parameter of UserProfile.

99c2ee9

migrations: Add stream_topic_access_group field to stream model.

918cc6e

migrations: Set default value for stream_topic_access_group.

59fdd91

streams: Add new field to stream model.

00eabd2

The `stream_topic_access_group` field have been added to the stream model and across various types. The new fields causes the query length to be increased by 34 in `test_subs`

do_import_realm: Update function to support stream_topic_access_group.

cc0e5ce

`namedusergroup` is also included in the `ID_MAP`.

streams: Add a function to check whether a stream is a support stream.

c7a8b3c

Using `stream_topic_access_group.name` in `is_support_stream` function is causing the increase in database query count.

streams: Enable the creation of support streams.

167886c

sanchi-t force-pushed the issue-19434 branch from 38cd080 to 93b2043 Compare May 14, 2024 20:41

sanchi-t marked this pull request as ready for review May 14, 2024 20:42

sanchi-t commented May 14, 2024

View reviewed changes

sanchi-t added 9 commits May 15, 2024 02:26

can_access_stream_history: Deny stream history access.

17a9c94

We need to check that the user is not restricted in order to allow them to view messages in support stream.

IncludeHistoryTest: Update test to include support streams.

333459e

update_stream_backend: Include updating stream_topic_access_group_id.

9067e7e

stream: Add new fields to the stream and types.

90ca579

`is_support_stream` and `stream_topic_view_policy` have been added to various types to align with the stream model.

stream_create: Enable the creation of support streams.

4f3519e

echo: Avoid locally echoing a message.

7f09a49

We need to check if a restricted user can access the topic before echoing their message, which happens on the server side.

stream_edit: Allow changing stream settings for support streams.

9e66805

stream_creation_form: Add UI for creating support type streams.

84e21f0

sanchi-t force-pushed the issue-19434 branch from 93b2043 to 84e21f0 Compare May 14, 2024 20:58

zulipbot removed the has conflicts label May 14, 2024

sanchi-t commented May 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create "support" stream type with topic-specific permissions #19434 #29520

Create "support" stream type with topic-specific permissions #19434 #29520

sanchi-t commented Mar 30, 2024 •

edited

zulipbot commented Mar 30, 2024

timabbott Apr 20, 2024

sanchi-t May 14, 2024

timabbott Apr 20, 2024

timabbott Apr 20, 2024

sanchi-t May 14, 2024

timabbott Apr 20, 2024 •

edited

sanchi-t May 14, 2024

timabbott Apr 20, 2024

timabbott Apr 20, 2024

timabbott Apr 20, 2024

timabbott Apr 20, 2024

sanchi-t May 14, 2024

timabbott Apr 20, 2024

sanchi-t May 14, 2024

timabbott commented Apr 20, 2024

timabbott commented Apr 22, 2024

sanchi-t May 14, 2024

sanchi-t May 14, 2024

sanchi-t May 14, 2024


		return first_message_in_topic.sender_id

		def check_access_based_on_stream_topic_view_policy(sender: UserProfile, stream: Stream) -> bool:

		def is_support_stream(self) -> bool:
		return self.stream_topic_access_group.name != SystemGroups.EVERYONE

Create "support" stream type with topic-specific permissions #19434 #29520

Are you sure you want to change the base?

Create "support" stream type with topic-specific permissions #19434 #29520

Conversation

sanchi-t commented Mar 30, 2024 • edited

zulipbot commented Mar 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott Apr 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Apr 20, 2024

timabbott commented Apr 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanchi-t commented Mar 30, 2024 •

edited

timabbott Apr 20, 2024 •

edited