initial_snapshot: Centralize (most of) the group-permission defaults

chrisbobbe · chrisbobbe · commit 2f0956d0289a · 2025-09-16T18:23:18.000-07:00
Now these are explicit and it's clear where they come from. Thanks Greg for this suggestion: zulip#1842 (comment) The exception is the pre-291 fallback, which doesn't fit into a static fixture because it depends on the realm setting realmDeleteOwnMessagePolicy.
diff --git a/lib/model/channel.dart b/lib/model/channel.dart
@@ -156,25 +156,14 @@ mixin ChannelStore on UserStore {
 
   bool _selfHasContentAccessViaGroupPermissions(ZulipStream channel) {
     // Compare web's stream_data.has_content_access_via_group_permissions.
-    // TODO(#814) try to clean up this logic; perhaps record more explicitly
-    //   what default/fallback value to use for a given group-based permission
-    //   on older servers.
-
-    if (channel.canAddSubscribersGroup != null
-        && selfHasPermissionForGroupSetting(channel.canAddSubscribersGroup!,
-             GroupSettingType.stream, 'can_add_subscribers_group')) {
-      // The behavior before this permission was introduced was equivalent to
-      // the "nobody" group.
-      // TODO(server-10): simplify
+
+    if (selfHasPermissionForGroupSetting(channel.canAddSubscribersGroup,
+          GroupSettingType.stream, 'can_add_subscribers_group')) {
       return true;
     }
 
-    if (channel.canSubscribeGroup != null
-        && selfHasPermissionForGroupSetting(channel.canSubscribeGroup!,
-             GroupSettingType.stream, 'can_subscribe_group')) {
-      // The behavior before this permission was introduced was equivalent to
-      // the "nobody" group.
-      // TODO(server-10): simplify
+    if (selfHasPermissionForGroupSetting(channel.canSubscribeGroup,
+          GroupSettingType.stream, 'can_subscribe_group')) {
       return true;
     }
 
diff --git a/lib/model/message.dart b/lib/model/message.dart
@@ -107,19 +107,14 @@ mixin MessageStore on ChannelStore {
       return false;
     }
 
-    if (realmCanDeleteAnyMessageGroup != null) {
-      if (selfHasPermissionForGroupSetting(realmCanDeleteAnyMessageGroup!,
-            GroupSettingType.realm, 'can_delete_any_message_group')) {
-        return true;
-      }
-    } else if (selfUser.role.isAtLeast(UserRole.administrator)) {
+    if (selfHasPermissionForGroupSetting(realmCanDeleteAnyMessageGroup,
+          GroupSettingType.realm, 'can_delete_any_message_group')) {
       return true;
     }
 
     if (channel != null) {
-      if (channel.canDeleteAnyMessageGroup != null
-          && selfHasPermissionForGroupSetting(channel.canDeleteAnyMessageGroup!,
-               GroupSettingType.stream, 'can_delete_any_message_group')) {
+      if (selfHasPermissionForGroupSetting(channel.canDeleteAnyMessageGroup,
+            GroupSettingType.stream, 'can_delete_any_message_group')) {
         return true;
       }
     }
@@ -138,6 +133,9 @@ mixin MessageStore on ChannelStore {
     // that's impossible here because `message` can't be an [OutboxMessage]
     // (it's a [Message] from [MessageStore.messages]).
 
+    // (selfHasPermissionForGroupSetting isn't equipped to handle the old-server
+    // fallback logic for this specific permission; it's dynamic and depends on
+    // realmDeleteOwnMessagePolicy, so we do our own null check here.)
     if (realmCanDeleteOwnMessageGroup != null) {
       if (!selfHasPermissionForGroupSetting(realmCanDeleteOwnMessageGroup!,
             GroupSettingType.realm, 'can_delete_own_message_group')) {
@@ -146,11 +144,8 @@ mixin MessageStore on ChannelStore {
           return false;
         }
 
-        if (
-          channel.canDeleteOwnMessageGroup == null
-          || !selfHasPermissionForGroupSetting(channel.canDeleteOwnMessageGroup!,
-               GroupSettingType.stream, 'can_delete_own_message_group')
-        ) {
+        if (!selfHasPermissionForGroupSetting(channel.canDeleteOwnMessageGroup,
+              GroupSettingType.stream, 'can_delete_own_message_group')) {
           return false;
         }
       }
diff --git a/lib/model/realm.dart b/lib/model/realm.dart
@@ -129,7 +129,10 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore {
   }
 
   /// Whether the self-user has the given (group-based) permission.
-  bool selfHasPermissionForGroupSetting(GroupSettingValue value,
+  ///
+  /// If the server doesn't know about the permission,
+  /// pass null for [value] and a reasonable default will be chosen.
+  bool selfHasPermissionForGroupSetting(GroupSettingValue? value,
       GroupSettingType type, String name);
 }
 
@@ -180,7 +183,7 @@ mixin ProxyRealmStore on RealmStore {
   @override
   List<CustomProfileField> get customProfileFields => realmStore.customProfileFields;
   @override
-  bool selfHasPermissionForGroupSetting(GroupSettingValue value, GroupSettingType type, String name) =>
+  bool selfHasPermissionForGroupSetting(GroupSettingValue? value, GroupSettingType type, String name) =>
     realmStore.selfHasPermissionForGroupSetting(value, type, name);
 }
 
@@ -225,7 +228,7 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
     customProfileFields = _sortCustomProfileFields(initialSnapshot.customProfileFields);
 
   @override
-  bool selfHasPermissionForGroupSetting(GroupSettingValue value,
+  bool selfHasPermissionForGroupSetting(GroupSettingValue? value,
       GroupSettingType type, String name) {
     // Compare web's settings_data.user_has_permission_for_group_setting.
     //
@@ -236,11 +239,48 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
     // That exists for deciding whether to offer the "Generate email address"
     // button, and if so then which users to offer in the dropdown;
     // it's predicting whether /api/get-stream-email-address would succeed.
-    if (_selfUserRole == UserRole.guest) {
-      final config = _groupSettingConfig(type, name);
-      if (!config.allowEveryoneGroup) return false;
+
+    final config = _groupSettingConfig(type, name);
+
+    if (_selfUserRole == UserRole.guest && !config.allowEveryoneGroup) {
+      return false;
+    }
+
+    if (value != null) {
+      return selfInGroupSetting(value);
+    }
+
+    // [value] will be null when the server doesn't know about the permission.
+    // *We* know about it (or presumably we wouldn't have called this method),
+    // and we know a reasonable default; use that.
+
+    switch (config.defaultGroupName) {
+      case DefaultGroupNameUnknown():
+        // When we know about a permission, we should also know about the group
+        // we've said is the default value for it.
+        assert(false);
+        return true;
+      case PseudoSystemGroupName.streamCreatorOrNobody:
+        // TODO implement
+        return true;
+      case SystemGroupName.everyoneOnInternet:
+      case SystemGroupName.everyone:
+        return true;
+      case SystemGroupName.members:
+        return _selfUserRole.isAtLeast(UserRole.member);
+      case SystemGroupName.fullMembers:
+        // TODO check waiting period (nontrivial to get access to that logic
+        //   here in RealmStore; it's on UserStore, which depends on RealmStore)
+        return _selfUserRole.isAtLeast(UserRole.member);
+      case SystemGroupName.moderators:
+        return _selfUserRole.isAtLeast(UserRole.moderator);
+      case SystemGroupName.administrators:
+        return _selfUserRole.isAtLeast(UserRole.administrator);
+      case SystemGroupName.owners:
+        return _selfUserRole.isAtLeast(UserRole.owner);
+      case SystemGroupName.nobody:
+        return false;
     }
-    return selfInGroupSetting(value);
   }
 
   /// The metadata for how to interpret the given group-based permission setting.
diff --git a/test/model/realm_test.dart b/test/model/realm_test.dart
@@ -1,8 +1,10 @@
 import 'package:checks/checks.dart';
 import 'package:test/scaffolding.dart';
 import 'package:zulip/api/model/events.dart';
+import 'package:zulip/api/model/initial_snapshot.dart';
 import 'package:zulip/api/model/model.dart';
 import 'package:zulip/model/realm.dart';
+import 'package:zulip/model/store.dart';
 
 import '../example_data.dart' as eg;
 
@@ -82,6 +84,94 @@ void main() {
       check(hasPermission(selfUser, group, 'can_send_message_group'))
         .isFalse();
     });
+
+    group('fallbacks for permissions not known to the server', () {
+      late PerAccountStore store;
+
+      void prepare({UserRole? selfUserRole}) {
+        final selfUser = eg.user(role: selfUserRole);
+        store = eg.store(selfUser: selfUser,
+          initialSnapshot: eg.initialSnapshot(realmUsers: [selfUser]));
+      }
+
+      void doCheck(GroupSettingType type, String name, bool expected) {
+        check(store.selfHasPermissionForGroupSetting(null, type, name))
+          .equals(expected);
+      }
+
+      for (final pseudoSystemGroupName in PseudoSystemGroupName.values) {
+        switch (pseudoSystemGroupName) {
+          case PseudoSystemGroupName.streamCreatorOrNobody:
+            // TODO implement and test
+        }
+      }
+
+      for (final systemGroupName in SystemGroupName.values) {
+        switch (systemGroupName) {
+          case SystemGroupName.everyoneOnInternet:
+            // (No permissions where we use this default value; continue.)
+            break;
+          case SystemGroupName.everyone:
+            test('everyone', () {
+              prepare();
+              doCheck(GroupSettingType.realm, 'can_access_all_users_group', true);
+            });
+          case SystemGroupName.members:
+            test('members, is guest', () {
+              prepare(selfUserRole: UserRole.guest);
+              doCheck(GroupSettingType.realm, 'can_add_custom_emoji_group', false);
+            });
+            test('members, is member', () {
+              prepare(selfUserRole: UserRole.member);
+              doCheck(GroupSettingType.realm, 'can_add_custom_emoji_group', true);
+            });
+          case SystemGroupName.fullMembers:
+            // (No permissions where we use this default value; continue.)
+            break;
+          case SystemGroupName.moderators:
+            test('moderators, is member', () {
+              prepare(selfUserRole: UserRole.member);
+              doCheck(GroupSettingType.realm, 'can_set_delete_message_policy_group', false);
+            });
+            test('moderators, is moderator', () {
+              prepare(selfUserRole: UserRole.moderator);
+              doCheck(GroupSettingType.realm, 'can_set_delete_message_policy_group', true);
+            });
+          case SystemGroupName.administrators:
+            test('administrators, is moderator', () {
+              prepare(selfUserRole: UserRole.moderator);
+              doCheck(GroupSettingType.stream, 'can_remove_subscribers_group', false);
+            });
+            test('administrators, is administrator', () {
+              prepare(selfUserRole: UserRole.administrator);
+              doCheck(GroupSettingType.stream, 'can_remove_subscribers_group', true);
+            });
+          case SystemGroupName.owners:
+            test('owners, is administrator', () {
+              prepare(selfUserRole: UserRole.administrator);
+              doCheck(GroupSettingType.realm, 'can_create_web_public_channel_group', false);
+            });
+            test('owners, is owner', () {
+              prepare(selfUserRole: UserRole.owner);
+              doCheck(GroupSettingType.realm, 'can_create_web_public_channel_group', true);
+            });
+          case SystemGroupName.nobody:
+            test('nobody', () {
+              prepare();
+              doCheck(GroupSettingType.stream, 'can_delete_own_message_group', false);
+            });
+        }
+      }
+
+      test('throw on unknown name', () {
+        // We should know about all the permissions we're trying to implement,
+        // even the ones old servers don't know about.
+        prepare(selfUserRole: UserRole.member);
+        check(() => store.selfHasPermissionForGroupSetting(null,
+          GroupSettingType.realm, 'example_future_permission_name'),
+        ).throws<Error>();
+      });
+    });
   });
 
   group('customProfileFields', () {
