Group-based permissions #1822

gnprice · 2025-08-20T00:27:41Z

Fixes #1687.
Fixes #814.

Stacked atop #1814.

This adds all the generic infrastructure needed for implementing any given group-based permission. The last commit (or pair of commits) then adds the permission "has content access (to a channel)" (which we'll want for #188), to demonstrate end-to-end how to add individual permissions using the new infrastructure.

gnprice · 2025-08-20T00:29:00Z

lib/model/user_group.dart

+    // TODO(perf), TODO(#814): memoize which groups the self-user is in,
+    //   to save doing this depth-first search on each permission check
+    return group.members.contains(selfUserId)
+      || group.directSubgroupIds.any(_selfInGroup);


I have a draft branch for this optimization. But it's a bit tricky to get right, so I've separated it out in order to let this PR focus on all the other aspects of the semantics of these permissions.

chrisbobbe

Thanks, this will be great to have! Comments below.

chrisbobbe · 2025-08-20T18:43:50Z

lib/model/user_group.dart

+  void handleRealmUserUpdateEvent(RealmUserUpdateEvent event) {
+    // TODO(#1687): test handling user deactivation
+    for (final group in _groups.values) {
+      group.members.remove(event.userId);
    }
  }


This looks like it'll touch the groups on any user update, not just deactivation: so when a user changes their name, avatar, etc. 🙂

Oof indeed, good catch, thanks.

chrisbobbe · 2025-08-20T18:48:58Z

lib/api/model/model.dart

+    return switch (json) {
+      int() => GroupSettingValueNamed.fromJson(json),
+      Map<String, dynamic>() => GroupSettingValueNameless.fromJson(json),
+      _ => throw TypeError(),


throw FormatException(), maybe? This would be the only result of a git grep 'throw TypeError'

chrisbobbe · 2025-08-20T18:54:58Z

lib/model/user_group.dart

+    return group.members.contains(selfUserId)
+      || group.directSubgroupIds.any(_selfInGroup);


Hmm I hope a group is never a subgroup of itself :)

Indeed, that's an invariant of the system. The subgroup relation is acyclic: no group is a subgroup of itself, nor of any of its transitive subgroups.

I'm not finding that documented, though — looked here:
https://zulip.com/api/group-setting-values
https://zulip.com/api/update-user-group-subgroups
— so it should probably get written down more explicitly in the API docs.

chrisbobbe · 2025-08-20T19:08:47Z

lib/api/model/initial_snapshot.dart

  @JsonKey(readValue: _readUsersIsActiveFallbackTrue)
  final List<User> crossRealmBots;

+  final SupportedPermissionSettings serverSupportedPermissionSettings;


The API doc says

New in Zulip 8.0 (feature level 221)

so we should make this optional.

Oops, good catch, thanks.

chrisbobbe · 2025-08-20T19:17:53Z

lib/api/model/model.dart

+  GroupSettingValue canAddSubscribersGroup;
+  GroupSettingValue canSubscribeGroup;


These are documented as new in Zulip 10.0, so we should make them optional.

Also we should add these fields to ChannelUpdateEvent if they can change with that event, or leave a comment there if they can't.

Indeed, thanks for those catches.

chrisbobbe · 2025-08-20T19:27:27Z

lib/model/channel.dart

+    return _selfHasContentAccessViaGroupPermissions(channel);
+  }
+
+  bool _selfHasContentAccessViaGroupPermissions(ZulipStream channel) {


channel: Add selfHasContentAccess, using canSubscribeGroup et al […] This particular permission is one we don't yet have any logic that should consume […]

I think it does actually :) see

Check group-based permission to subscribe to a channel, as needed #1786

which has corresponding TODO(#1786)s in the code.

Hmm indeed, thanks for the tip.

One of those two is directly covered by the permission logic here, so I revised this commit to take care of that. The other requires a bit of a variation, so I think I'll leave #1786 open for handling that one.

gnprice

Thanks for the review! Seems I was a bit hasty in sending this yesterday; thanks for those catches. Revision pushed — PTAL.

gnprice · 2025-08-20T22:27:41Z

lib/api/model/model.dart

+    return switch (json) {
+      int() => GroupSettingValueNamed.fromJson(json),
+      Map<String, dynamic>() => GroupSettingValueNameless.fromJson(json),
+      _ => throw TypeError(),


gnprice · 2025-08-20T22:41:43Z

lib/api/model/initial_snapshot.dart

  @JsonKey(readValue: _readUsersIsActiveFallbackTrue)
  final List<User> crossRealmBots;

+  final SupportedPermissionSettings serverSupportedPermissionSettings;


Oops, good catch, thanks.

gnprice · 2025-08-20T22:51:53Z

lib/api/model/model.dart

+  GroupSettingValue canAddSubscribersGroup;
+  GroupSettingValue canSubscribeGroup;


Indeed, thanks for those catches.

gnprice · 2025-08-20T23:28:07Z

lib/model/channel.dart

+    return _selfHasContentAccessViaGroupPermissions(channel);
+  }
+
+  bool _selfHasContentAccessViaGroupPermissions(ZulipStream channel) {


Hmm indeed, thanks for the tip.

One of those two is directly covered by the permission logic here, so I revised this commit to take care of that. The other requires a bit of a variation, so I think I'll leave #1786 open for handling that one.

gnprice · 2025-08-20T23:30:17Z

I haven't made any revision yet for the point you raised at #api design > server_supported_permission_settings @ 💬: this server_supported_permission_settings API is described as unstable.

gnprice · 2025-08-21T00:31:38Z

(I plan to leave this PR out of today's release, pending that API question. I hope to merge a version tomorrow before I go on vacation, though.)

chrisbobbe · 2025-08-21T01:29:28Z

Thanks, this looks great! So I guess hopefully that API design thread resolves soon: #api design > server_supported_permission_settings @ 💬

but as you mentioned there

Pending an answer, I guess our alternative would be to ignore that part of the /api/register-queue response, and instead hard-code whichever values we want to use.

Otherwise LGTM.

gnprice · 2025-08-21T21:46:25Z

OK, revised with that change! PTAL.

gnprice · 2025-08-21T21:48:09Z

For the record, if we were to decide the existing API is stable and so just start using that, the change to make would be the diff below, plus a couple more items:

updating docs on SupportedPermissionSettings and its .fixture
optionally, moving SupportedPermissionSettings.fixture into test/example_data.dart (like it was in c475382 , in a previous revision of this PR)
- The main reason we might not do that is if we decide to have the same structure encode default/fallback values, describing how to behave when a server doesn't have a given permission at all (so presumably is older than the introduction of the permission). In that case we'd still probably rename it, and certainly revise its doc.

That diff's diffstat:

 lib/api/model/initial_snapshot.dart   |   4 ++--
 lib/api/model/initial_snapshot.g.dart | Bin 14829 -> 15167 bytes
 lib/model/realm.dart                  |  16 ++++++++++++++--
 test/example_data.dart                |   3 +++
 4 files changed, 19 insertions(+), 4 deletions(-)

and details:

Diff

diff --git lib/api/model/initial_snapshot.dart lib/api/model/initial_snapshot.dart
index 5edc04afd..3e019cb9a 100644
--- lib/api/model/initial_snapshot.dart
+++ lib/api/model/initial_snapshot.dart
@@ -110,6 +110,5 @@ class InitialSnapshot {
   final List<User> crossRealmBots;
 
-  // TODO(server): Get this API stabilized, to replace [SupportedPermissionSettings.fixture].
-  // final SupportedPermissionSettings? serverSupportedPermissionSettings;
+  final SupportedPermissionSettings? serverSupportedPermissionSettings; // TODO(server-8)
 
   // TODO etc., etc.
@@ -173,4 +172,5 @@ class InitialSnapshot {
     required this.realmNonActiveUsers,
     required this.crossRealmBots,
+    required this.serverSupportedPermissionSettings,
   });
 
diff --git lib/api/model/initial_snapshot.g.dart lib/api/model/initial_snapshot.g.dart
index 481b3184b..05e270a85 100644
Binary files lib/api/model/initial_snapshot.g.dart and lib/api/model/initial_snapshot.g.dart differ
diff --git lib/model/realm.dart lib/model/realm.dart
index 02db21c56..b62973e93 100644
--- lib/model/realm.dart
+++ lib/model/realm.dart
@@ -124,4 +124,6 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore {
 
   /// Whether the self-user has the given (group-based) permission.
+  ///
+  /// Throws if the server does not have the concept of the given permission.
   bool selfHasPermissionForGroupSetting(GroupSettingValue value,
       GroupSettingType type, String name);
@@ -190,4 +192,5 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
   }) :
     _selfUserRole = selfUser.role,
+    _serverSupportedPermissionSettings = initialSnapshot.serverSupportedPermissionSettings,
     serverPresencePingIntervalSeconds = initialSnapshot.serverPresencePingIntervalSeconds,
     serverPresenceOfflineThresholdSeconds = initialSnapshot.serverPresenceOfflineThresholdSeconds,
@@ -226,7 +229,14 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
   }
 
-  /// The metadata for how to interpret the given group-based permission setting.
+  /// The config the server supplied for how to interpret the given
+  /// group-based permission setting.
+  ///
+  /// Throws if there is no such config from the server.
+  /// This method should therefore only be called if the server is one that
+  /// actually has the concept of the given permission.
   PermissionSettingsItem _groupSettingConfig(GroupSettingType type, String name) {
-    final supportedSettings = SupportedPermissionSettings.fixture;
+    // If the server has the particular permission being asked about, then it
+    // must have this bit of general infrastructure for group-based permissions.
+    final supportedSettings = _serverSupportedPermissionSettings!;
 
     // Compare web's group_permission_settings.get_group_permission_setting_config.
@@ -247,4 +257,6 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
   UserRole _selfUserRole;
 
+  final SupportedPermissionSettings? _serverSupportedPermissionSettings; // TODO(server-8)
+
   @override
   final int serverPresencePingIntervalSeconds;
diff --git test/example_data.dart test/example_data.dart
index 84d1abf6a..d3518ebbb 100644
--- test/example_data.dart
+++ test/example_data.dart
@@ -1251,4 +1251,5 @@ InitialSnapshot initialSnapshot({
   List<User>? realmNonActiveUsers,
   List<User>? crossRealmBots,
+  SupportedPermissionSettings? serverSupportedPermissionSettings,
 }) {
   return InitialSnapshot(
@@ -1300,4 +1301,6 @@ InitialSnapshot initialSnapshot({
     realmNonActiveUsers: realmNonActiveUsers ?? [],
     crossRealmBots: crossRealmBots ?? [],
+    serverSupportedPermissionSettings: serverSupportedPermissionSettings
+      ?? SupportedPermissionSettings.fixture,
   );
 }

chrisbobbe

Thanks, LGTM! One nit below, then please merge at will.

chrisbobbe · 2025-08-21T23:18:49Z

lib/api/model/initial_snapshot.dart

+/// This is the type that [InitialSnapshot.serverSupportedPermissionSettings],
+/// would have, according to the API as it exists as of 2025-08;
+/// but that API is documented as unstable and subject to change.


nit:

Suggested change

/// This is the type that [InitialSnapshot.serverSupportedPermissionSettings],

/// would have, according to the API as it exists as of 2025-08;

/// but that API is documented as unstable and subject to change.

/// This is the type that [InitialSnapshot.serverSupportedPermissionSettings]

/// would have, according to the API as it exists as of 2025-08;

/// but that API is documented as unstable and subject to change.

Fixes zulip#1687. We'll add tests for this after an upcoming commit provides a way of accessing group membership that's more relevant for the app's needs. That way we can write the tests to that interface.

This is the way we'll be inspecting group membership for checking permissions (zulip#814), which is the main way we'll be using group membership in general. Because this method is how most of the app will consume group membership, adding this method also lets us write natural tests for the logic that maintains group membership on events. At this stage we stick with a very straightforward implementation. We might optimize it later with more data structures.

Fixes zulip#814. Fixes part of zulip#1786. The general implementation of group-based permissions (zulip#814) was added in the commits leading up to this one. This commit demonstrates implementing a given such permission. Then we also use the new permission in one place where we already want to use it, toward zulip#1786. (There's another part of zulip#1786 which calls for a variation: determining whether the user would be able to re-subscribe to a channel if they unsubscribe. We'll leave that for a follow-up.) We'll also be adding a more prominent bit of UI that wants this permission soon: the list of channels, zulip#188. To exercise this method on real data, I added some debugging prints that printed a list of the channels where this returned true, and a list of the channels where it returned false. I also manually tested the change for zulip#1786, by visiting an unsubscribed private channel (via the link in a #-mention), long-pressing the app bar to get the channel action sheet, and then (as another user, on web) adding and removing permission for the test user to subscribe themself.

gnprice · 2025-08-22T00:33:54Z

Thanks! Done.

…cess This tests the change in cf83c9e / zulip#1822.

gnprice requested a review from chrisbobbe August 20, 2025 00:27

gnprice added the maintainer review PR ready for review by Zulip maintainers label Aug 20, 2025

gnprice commented Aug 20, 2025

View reviewed changes

chrisbobbe reviewed Aug 20, 2025

View reviewed changes

gnprice force-pushed the pr-groups branch 3 times, most recently from 898844d to d0b8bf5 Compare August 20, 2025 23:27

gnprice commented Aug 20, 2025

View reviewed changes

gnprice force-pushed the pr-groups branch from d0b8bf5 to eb80091 Compare August 20, 2025 23:46

gnprice force-pushed the pr-groups branch from eb80091 to 0c8a2bd Compare August 21, 2025 21:39

chrisbobbe approved these changes Aug 21, 2025

View reviewed changes

gnprice added 14 commits August 21, 2025 17:32

api: Add user-group memberships, and their events

4333598

group: Maintain user-group membership on events

a0cfecd

Fixes zulip#1687. We'll add tests for this after an upcoming commit provides a way of accessing group membership that's more relevant for the app's needs. That way we can write the tests to that interface.

api: Define type GroupSettingValue

cd47dcc

group test: Test group membership, and handling events for it

11b09ff

group [nfc]: Add HasUserGroupStore

1637dee

realm [nfc]: Give RealmStore a reference to UserGroupStore

65a0977

realm: Denormalize self-user role onto RealmStoreImpl

f080dad

api: Add SupportedPermissionSettings type, and fixture

76aa6b5

realm test [nfc]: Reorder tests to match implementation

1550173

realm: Add selfHasPermissionForGroupSetting

c6206a8

test: Add eg.nobodyGroup

8c3b11f

api: Add channel fields canSubscribeGroup, canAddSubscribersGroup

c3be229

gnprice force-pushed the pr-groups branch from 0c8a2bd to cf83c9e Compare August 22, 2025 00:32

gnprice merged commit cf83c9e into zulip:main Aug 22, 2025
1 check passed

gnprice deleted the pr-groups branch August 22, 2025 00:42

chrisbobbe mentioned this pull request Sep 29, 2025

Check group-based permission to subscribe to a channel, as needed #1786

Closed

chrisbobbe added a commit to chrisbobbe/zulip-flutter that referenced this pull request Oct 1, 2025

action_sheet test: Check "Subscribe" button absent when no content ac…

0a5c691

…cess This tests the change in cf83c9e / zulip#1822.

chrisbobbe mentioned this pull request Oct 1, 2025

action_sheet: Check can_subscribe_group for "Subscribe" confirmation dialog #1887

Merged

gnprice pushed a commit to chrisbobbe/zulip-flutter that referenced this pull request Oct 3, 2025

action_sheet test: Check "Subscribe" button absent when no content ac…

35bbaef

…cess This tests the change in cf83c9e / zulip#1822.

		return group.members.contains(selfUserId)
		\|\| group.directSubgroupIds.any(_selfInGroup);

		GroupSettingValue canAddSubscribersGroup;
		GroupSettingValue canSubscribeGroup;

Group-based permissions #1822

Group-based permissions #1822

Uh oh!

Conversation

gnprice commented Aug 20, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice commented Aug 20, 2025

Uh oh!

gnprice commented Aug 21, 2025

Uh oh!

chrisbobbe commented Aug 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gnprice commented Aug 21, 2025

Uh oh!

gnprice commented Aug 21, 2025

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gnprice commented Aug 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

chrisbobbe commented Aug 21, 2025 •

edited

Loading