initial_snapshot: Centralize group-permission defaults; fix admin permission for legacy servers #1853

chrisbobbe · 2025-09-16T20:39:08Z

Also does the refactor Greg suggested here: #1842 (comment)

chrisbobbe · 2025-09-17T22:12:31Z

lib/api/model/initial_snapshot.dart

+/// A known special string
+/// that [PermissionSettingsItem.defaultGroupName] might be.
+///
+/// See server implementation, e.g.
+/// `can_administer_channel_group` in zerver/models/streams.py.
+@JsonEnum(valueField: 'apiValue', alwaysCreate: true)
+enum PseudoSystemGroupName implements SettableAsDefaultGroupName {
+  streamCreatorOrNobody(apiValue: 'stream_creator_or_nobody'),


On CZO I proposed documenting this: #api design > stream_creator_or_nobody @ 💬

From that discussion, it looks like it might also get renamed.

Cool. Let's include a link to that thread in a comment, too.

rajveermalviya

Thanks for working on this @chrisbobbe! LGTM, moving over to Greg's review.

gnprice

Thanks for building this! Comments below.

gnprice · 2025-09-18T18:53:38Z

lib/api/model/initial_snapshot.dart

+enum SystemGroupName implements SettableAsDefaultGroupName {
+  everyoneOnInternet(apiValue: 'role:internet'),
+  everyone(apiValue: 'role:everyone'),


This feels like it's not specifically about the initial snapshot — it'll eventually be relevant to other parts of the API too. It could go in model.dart… but perhaps better:

Let's have a prep commit move SupportedPermissionSettings and PermissionSettingsItem to their own file, say permission.dart. Then these new types can go there too.

gnprice · 2025-09-18T19:04:42Z

lib/api/model/initial_snapshot.dart

+sealed class SettableAsDefaultGroupName {
+  String toJson();
+}
+
+class DefaultGroupNameConverter extends JsonConverter<SettableAsDefaultGroupName, String> {
+  const DefaultGroupNameConverter();
+
+  @override
+  SettableAsDefaultGroupName fromJson(String json) {


Can this converter be merged into the class? Seems like any time we'd want this class, we'd want this same conversion, right?

(The fromJson method then becomes a factory constructor on that class. See other non-generated fromJson implementations, like on GroupSettingValue.)

gnprice · 2025-09-18T19:05:41Z

lib/api/model/initial_snapshot.dart

+/// A value of [PermissionSettingsItem.defaultGroupName].
+///
+/// Can be any of these:
+/// - a known system group [SystemGroupName]
+/// - a known special string [PseudoSystemGroupName]
+/// - an unknown system group or special string [DefaultGroupNameUnknown]
+sealed class SettableAsDefaultGroupName {


Is there some information that the "settable as" part of the name is meant to convey here? How does the expressed meaning differ from if we called it DefaultGroupName?

Ah nope 🙂 I think I'd had the idea that it should be considered an interface rather than a base class, but I don't really remember why, and I don't see anything wrong with DefaultGroupName when I look at it again :)

gnprice · 2025-09-18T19:15:36Z

lib/api/model/initial_snapshot.dart

+/// A known special string
+/// that [PermissionSettingsItem.defaultGroupName] might be.
+///
+/// See server implementation, e.g.
+/// `can_administer_channel_group` in zerver/models/streams.py.
+@JsonEnum(valueField: 'apiValue', alwaysCreate: true)
+enum PseudoSystemGroupName implements SettableAsDefaultGroupName {
+  streamCreatorOrNobody(apiValue: 'stream_creator_or_nobody'),


Cool. Let's include a link to that thread in a comment, too.

gnprice · 2025-09-18T19:23:21Z

lib/model/realm.dart

+    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:


nit: let's move all this logic to its own method, even if it's a private method located in this same place.

That way selfHasPermissionForGroupSetting gets to stay focused on the main logic it's about, rather than these details which are for a fallback case. As a bonus, this case can go into an early return, so that the method ends with the call to selfInGroupSetting which is the thrust of the method's main narrative.

gnprice · 2025-09-18T19:30:28Z

lib/model/realm.dart

+      case PseudoSystemGroupName.streamCreatorOrNobody:
+        // TODO implement
+        return true;


Hmm — this is a concerning-looking type of TODO comment, because it feels like it's not something we'd necessarily see when the time comes that we need to carry it out.

Is there an issue for a feature where we'd expect to start hitting this case?

Oh also: these defaults come from our code and not the server, so it would take a change to our code for us to start hitting this. Let's add an assert — that should catch any such change while in dev, to remind us this isn't yet implemented.

Good points, yep—

Is there an issue for a feature where we'd expect to start hitting this case?

—#1102

gnprice · 2025-09-18T19:33:13Z

lib/model/realm.dart

+      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);


Hmm. That's the hasPassedWaitingPeriod method, right?

We can just move that to the RealmStore mixin instead of UserStore — it's not using anything from the user store itself, just a User object. Can include a comment justifying that placement by saying it's needed for permissions.

Sure, I can do that. RealmStore doesn't have access to the self-user User object, and it would feel odd to ask selfHasPermissionForGroupSetting's callers to provide it. But the passed-waiting-period method just uses .dateJoined from the user, and we can store that in RealmStoreImpl as ._selfUserDateJoined, the way we already have ._selfUserRole there, if that makes sense.

Hmm true. But yeah, that solution works. The implementation will then live on RealmStoreImpl rather than on the mixin, which is fine.

gnprice · 2025-09-18T19:37:40Z

lib/model/realm.dart

+      case SystemGroupName.everyoneOnInternet:
+      case SystemGroupName.everyone:
+        return true;


Do we have an issue for spectator view? If not, probably good to file one for tracking. (Can be terse and dashed off — it'll be MXB.) Then the everyoneOnInternet line here can get a TODO line pointing to that… or perhaps better, have such a comment at the definition of that enum value, saying to audit all its references.

gnprice · 2025-09-18T19:42:31Z

test/model/realm_test.dart

+          case SystemGroupName.everyone:
+            test('everyone', () {
+              prepare();
+              doCheck(GroupSettingType.realm, 'can_access_all_users_group', true);


nit: strengthen the test

Suggested change

case SystemGroupName.everyone:

test('everyone', () {

prepare();

doCheck(GroupSettingType.realm, 'can_access_all_users_group', true);

case SystemGroupName.everyone:

test('everyone', () {

prepare(selfUserRole: UserRole.guest);

doCheck(GroupSettingType.realm, 'can_access_all_users_group', true);

and similarly .owner for the .nobody case at the end

And we'll use the new selfUserRole param for testing zulip#1850, coming up.

Fixes zulip#1850.

The "role:nobody" group is the default value for both of these group settings (see defaultGroupName in upcoming commits), so it's helpful to be explicit here.

chrisbobbe · 2025-09-19T00:13:50Z

Thanks for the review! Revision pushed.

chrisbobbe · 2025-09-19T00:18:38Z

lib/widgets/action_sheet.dart

  required int channelId,
  bool showTopicListButton = true,
 }) {
+  final now = DateTime.now();


Ah I think I should change this to ZulipBinding.instance.utcNow(); will do that in my next revision.

gnprice

Thanks for the revision! Just one cluster of comments below.

gnprice · 2025-09-19T23:06:40Z

lib/widgets/all_channels.dart

    final channel = this.channel;
    final Subscription? subscription = channel is Subscription ? channel : null;
-    final hasContentAccess = store.selfHasContentAccess(channel);
+    final hasContentAccess = store.selfHasContentAccess(channel, atDate: DateTime.now());


There aren't currently any defaults with the "role:fullmembers" value, so this won't control any behavior now. But some might be added in the future, and we might as well include it for completeness.

Hmm — there probably won't ever be any of these, though. So it's unfortunate to be making this API more awkward for the code across the app to use, when the extra complication only gets used (a) in a fallback for old servers (b) and only after a hypothetical future change we don't expect to happen.

gnprice · 2025-09-19T23:09:34Z

lib/model/realm.dart

+      case SystemGroupName.fullMembers:
+        if (!_selfUserRole.isAtLeast(UserRole.member)) return false;
+        if (_selfUserRole == UserRole.member) {
+          return selfHasPassedWaitingPeriod(byDate: atDate);
+        }
+        return true;


How about we instead just make this unimplemented, like .streamCreatorOrNobody above?

The difference being that in this case, we don't expect to ever add code that triggers this case. (I think we may have never had a permission that defaulted to "full members". If we ever add one in the future, we'd probably make the permission configurable and group-based from the beginning, so there would still never be a fallback to such a default for it.)

gnprice · 2025-09-19T23:10:57Z

lib/model/realm.dart

+  /// callers must also check that the user's role is at least [UserRole.member].
+  // This used to be a method on [UserStore], but we needed it for permissions
+  // here in [RealmStore].
+  bool selfHasPassedWaitingPeriod({required DateTime byDate});


I think the refactor to move this here is still fine, though — logically its purpose is about permissions (that's the only reason to care about having passed the waiting period), so it makes sense for it to live where the permission checks are.

This doesn't change our parsing of server data because we don't currently look at server_supported_permission_settings in the initial snapshot, as noted in the added TODO. It just adds data to the fixture, modeled on current server code.

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.

chrisbobbe · 2025-09-19T23:27:17Z

Thanks for the review! Revision pushed.

gnprice · 2025-09-19T23:42:29Z

Thanks! Looks good; merging.

chrisbobbe requested a review from rajveermalviya September 16, 2025 20:39

chrisbobbe assigned rajveermalviya Sep 16, 2025

chrisbobbe added the maintainer review PR ready for review by Zulip maintainers label Sep 16, 2025

chrisbobbe force-pushed the pr-centralize-group-permission-defaults branch from b6fa709 to 2f0956d Compare September 17, 2025 01:31

chrisbobbe commented Sep 17, 2025

View reviewed changes

rajveermalviya approved these changes Sep 18, 2025

View reviewed changes

rajveermalviya added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Sep 18, 2025

rajveermalviya assigned gnprice and unassigned rajveermalviya Sep 18, 2025

rajveermalviya requested a review from gnprice September 18, 2025 13:47

gnprice reviewed Sep 18, 2025

View reviewed changes

chrisbobbe added 3 commits September 18, 2025 14:50

message test: Vary user role in pre-291 selfCanDeleteMessage tests

a55ee82

And we'll use the new selfUserRole param for testing zulip#1850, coming up.

message: Fix permissions for admins on pre-291 servers

35f4dda

Fixes zulip#1850.

message test [nfc]: Reword comment on pre-407 selfCanDeleteMessage tests

4495100

The "role:nobody" group is the default value for both of these group settings (see defaultGroupName in upcoming commits), so it's helpful to be explicit here.

chrisbobbe force-pushed the pr-centralize-group-permission-defaults branch from 2f0956d to 8e1932d Compare September 19, 2025 00:13

chrisbobbe commented Sep 19, 2025

View reviewed changes

gnprice reviewed Sep 19, 2025

View reviewed changes

chrisbobbe added 4 commits September 19, 2025 16:26

model [nfc]: Move has-passed-waiting-period method to RealmStore

391b56a

permission [nfc]: Move some permissions code out to new file

b584db5

chrisbobbe force-pushed the pr-centralize-group-permission-defaults branch from 8e1932d to 3730c90 Compare September 19, 2025 23:27

gnprice merged commit 3730c90 into zulip:main Sep 19, 2025
1 check passed

initial_snapshot: Centralize group-permission defaults; fix admin permission for legacy servers #1853

initial_snapshot: Centralize group-permission defaults; fix admin permission for legacy servers #1853

Conversation

chrisbobbe commented Sep 16, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya left a comment

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!

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!

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

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!

chrisbobbe commented Sep 19, 2025

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!

chrisbobbe commented Sep 19, 2025

Uh oh!

gnprice commented Sep 19, 2025

Uh oh!

Uh oh!

Uh oh!

chrisbobbe Sep 18, 2025 •

edited

Loading