Implement permissions check for can-delete-message #1842

chrisbobbe · 2025-09-05T01:37:40Z

The permissions logic for whether you can delete a message is pretty complicated, it turns out. Here's a PR with an implementation and tests—which are pretty long, despite trying to simplify where possible. Suggestions would be much appreciated. :)

Oh—and see this comment in the implementation:

  // There might be a better place for this to live…but it uses
  // message, channel, self-user, and user-group data, so seemed simplest to
  // put it here where all that is available.

Very happy to move the code somewhere else, just wasn't sure where would be best :)

Related: #1548

gnprice · 2025-09-05T04:05:28Z

Neat!

I think the right home for this logic is on MessageStore. That class will just need to gain a reference to ChannelStore, in the same way the latter already has UserStore; see Git history for how that was added.

gnprice · 2025-09-05T18:47:41Z

lib/model/store.dart

+    if (realmCanDeleteOwnMessageGroup != null) {
+      // realmCanDeleteOwnMessageGroup always present as of FL 291.
+      // TODO(server-10) make required, simplify, delete this FL-291 comment


This is already covered by the TODO(server-10) on the realmCanDeleteOwnMessageGroup field itself, right? Once that's made non-nullable, the analyzer will naturally point out that this condition can be simplified.

So I think this can just apply the condition without comment.

(Similarly for the other TODO-server comments in this method.)

chrisbobbe · 2025-09-05T21:42:57Z

Thanks! Revision pushed.

gnprice

Thanks! One more comment on the substore code organization — otherwise I'll leave this for @rajveermalviya to do the initial round of review 🙂

gnprice · 2025-09-05T22:30:46Z

lib/model/message.dart

@@ -375,6 +385,113 @@ class MessageStoreImpl extends HasRealmStore with MessageStore, _OutboxMessageSt
    );
  }

+  @override
+  bool selfCanDeleteMessage(int messageId, {required DateTime byDate}) {


Can this (and its private helper) move onto the MessageStore mixin? I think it can — it's just using the interface provided by MessageStore itself (and its ancestors), not any of the details of the data structures internal to MessageStoreImpl.

Compare the selfHasContentAccess and hasPostingPermission methods which live on the ChannelStore mixin.

chrisbobbe · 2025-09-05T22:39:31Z

Thanks, done! Revision pushed.

gnprice · 2025-09-05T23:59:26Z

lib/model/message.dart

+  @override
+  bool selfCanDeleteMessage(int messageId, {required DateTime byDate}) =>
+    messageStore.selfCanDeleteMessage(messageId, byDate: byDate);


nit: now redundant

rajveermalviya

Thanks for working on this @chrisbobbe! One comment otherwise LGTM.

rajveermalviya · 2025-09-08T15:30:26Z

lib/api/model/initial_snapshot.dart

+  /// on supported servers below version 10.
+  ///
+  /// Removed in FL 291.
+  final RealmDeleteOwnMessagePolicy? realmDeleteOwnMessagePolicy; // TODO(server-10)


Should this be delete_own_message_policy and not realm_delete_own_message_policy?
(Docs only has mention of the former.)

Ah I should add a note that this relies on information that isn't in the current doc; I had to look at zulip/zulip@0cd51f2fe, which removed the entry for realm_delete_own_message_policy in the register-response doc.

In general there's a pattern for realm attributes where the field in the register response is prefixed with realm_ but the field in the realm/update_dict event isn't.

nit: the added note (with the reference to that zulip/zulip commit) appears in a later commit

chrisbobbe · 2025-09-08T18:52:58Z

Thanks both 🙂, made those tweaks and pushed a revision. I'll mark this for Greg's review since Rajesh's comment was small and resolved simply.

gnprice

Thanks @chrisbobbe, and thanks @rajveermalviya for the previous review!

Generally this all looks great. Comments below.

gnprice · 2025-09-08T20:11:42Z

lib/api/model/initial_snapshot.dart

+  /// on supported servers below version 10.
+  ///
+  /// Removed in FL 291.
+  final RealmDeleteOwnMessagePolicy? realmDeleteOwnMessagePolicy; // TODO(server-10)


nit: the added note (with the reference to that zulip/zulip commit) appears in a later commit

gnprice · 2025-09-08T20:16:27Z

lib/api/model/initial_snapshot.dart

+
+  const RealmDeleteOwnMessagePolicy({required this.apiValue});
+
+  final int? apiValue;


or just int?

gnprice · 2025-09-08T20:18:06Z

test/example_data.dart

+    // no default; allow `null` to simulate servers without this
+    realmDeleteOwnMessagePolicy: realmDeleteOwnMessagePolicy,


Suggested change

// no default; allow `null` to simulate servers without this

realmDeleteOwnMessagePolicy: realmDeleteOwnMessagePolicy,

realmDeleteOwnMessagePolicy: realmDeleteOwnMessagePolicy,

For this one, null is the natural default (since it's what modern servers always have), so no need to further justify it.

gnprice · 2025-09-08T20:26:46Z

lib/api/model/model.dart

@@ -627,6 +627,7 @@ class ZulipStream {

  final int streamId;
  String name;
+  bool? isArchived; // TODO(server-10)


For servers that don't send this property, I believe they'll only send us non-archived channels. So we can make this non-nullable, and default to false. (And then a TODO-server comment just for removing the default.)

gnprice · 2025-09-08T20:32:40Z

lib/model/message.dart

+  /// Whether the user has permission to delete a message, as of [byDate].
+  ///
+  /// For a value of [byDate], use [ZulipBinding.instance.utcNow].
+  bool selfCanDeleteMessage(int messageId, {required DateTime byDate}) {


nit: "by date" doesn't seem like the right description for this parameter — how about atDate?

gnprice · 2025-09-08T21:04:28Z

lib/model/message.dart

+    if (
+      realmDeleteOwnMessagePolicy != null


It ought to be true that either realmCanDeleteOwnMessageGroup or realmDeleteOwnMessagePolicy is non-null, right?

Probably helpful to make that explicit: have an if/else-if chain for those, and then in the else where both were null, have a TODO(log) and thereby clarify that that case isn't expected to happen.

gnprice · 2025-09-08T21:09:40Z

lib/model/message.dart

+    if (realmMessageContentDeleteLimitSeconds == null) {
+      // i.e., no limit
+      return true;
+    }
+
+    return byDate.millisecondsSinceEpoch ~/ 1000 - message.timestamp
+      <= realmMessageContentDeleteLimitSeconds!;


nit: join as one stanza, because closely related (interpreting the same one setting)

gnprice · 2025-09-08T21:14:08Z

lib/model/message.dart

+    switch (realmDeleteOwnMessagePolicy!) {
+      case RealmDeleteOwnMessagePolicy.members:
+        return true;
+      case RealmDeleteOwnMessagePolicy.admins:
+        return role.isAtLeast(UserRole.administrator);
+      case RealmDeleteOwnMessagePolicy.fullMembers: {
+        if (!role.isAtLeast(UserRole.member)) return false;
+        if (role == UserRole.member) {
+          return hasPassedWaitingPeriod(selfUser, byDate: byDate);
+        }
+        return true;
+      }
+      case RealmDeleteOwnMessagePolicy.moderators:
+        return role.isAtLeast(UserRole.moderator);
+      case RealmDeleteOwnMessagePolicy.everyone:
+        return true;


nit: a bit clearer to understand and verify if the cases written in logical order (broad to narrow, or vice versa) rather than API-value order

The analyzer will check we've covered all the cases, so there's no worry about missing one by making it harder to compare with the enum's definition.

gnprice · 2025-09-08T21:20:55Z

test/model/message_test.dart

+    }
+
+    void doTest(bool expected, CanDeleteMessageParams params) {
+      test('params: ${params.describe()}', () async {


Let's avoid newlines in the test names. It's unconventional for a name; and when I run flutter test on this file, it seems to defeat the normal way that the output stays compact — there ends up being a separate line for each of these test cases, like this:

$ flutter test test/model/message_test.dart … 51 packages have newer versions incompatible with dependency constraints. Try `flutter pub outdated` for more information. 00:02 +39: selfCanDeleteMessage ... truedeleted while request in progress but we get success response 00:02 +40: selfCanDeleteMessage ... true 00:02 +40: selfCanDeleteMessage ... false 00:02 +41: selfCanDeleteMessage ... false 00:02 +41: selfCanDeleteMessage ... true 00:02 +42: selfCanDeleteMessage ... true 00:02 +42: selfCanDeleteMessage ... true 00:02 +43: selfCanDeleteMessage ... true 00:02 +43: selfCanDeleteMessage ... true 00:02 +44: selfCanDeleteMessage ... true 00:02 +44: selfCanDeleteMessage ... true 00:02 +45: selfCanDeleteMessage ... true 00:02 +45: selfCanDeleteMessage ... false 00:02 +46: selfCanDeleteMessage ... false 00:02 +46: selfCanDeleteMessage ... true 00:02 +47: selfCanDeleteMessage ... true 00:02 +47: selfCanDeleteMessage ... true 00:02 +48: selfCanDeleteMessage ... true 00:02 +48: selfCanDeleteMessage ... false 00:02 +49: selfCanDeleteMessage ... false 00:02 +49: selfCanDeleteMessage ... false 00:02 +50: selfCanDeleteMessage ... false 00:02 +50: selfCanDeleteMessage ... false 00:02 +51: selfCanDeleteMessage ... false 00:02 +51: selfCanDeleteMessage ... false 00:02 +52: selfCanDeleteMessage ... false 00:02 +52: selfCanDeleteMessage ... true 00:02 +53: selfCanDeleteMessage ... true 00:02 +53: selfCanDeleteMessage ... true 00:02 +54: selfCanDeleteMessage ... true 00:02 +54: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +55: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +55: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +56: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +56: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +57: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +57: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +58: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +58: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +59: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +59: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +60: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +60: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +61: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +61: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +62: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +62: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +63: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +63: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +64: selfCanDeleteMessage ... channel.canDeleteOwnMessageGroup?: N/A 00:02 +117: All tests passed!

gnprice · 2025-09-08T21:32:34Z

test/model/message_test.dart

+  unknown,
+  self,
+  otherHuman,
+  botOwnedBySelf,


This value doesn't appear to get exercised.

Ah yeah, we should exercise this.

When you're not in the "can delete any message" group, there are 8 distinct paths to approval:

self or self-owned bot

own-message permission by realm or by channel

time not limited or inside time limit

I have a hunch that we don't want to exercise all of them 🙂—

final permissiveSenderConfigs = [ CanDeleteMessageSenderConfig.self, CanDeleteMessageSenderConfig.botOwnedBySelf, ]; final ownMessageReasons = [ (realm: false, channel: true ), (realm: true, channel: false), ]; final permissiveTimeLimitConfigs = [ CanDeleteMessageTimeLimitConfig.notLimited, CanDeleteMessageTimeLimitConfig.insideLimit, ]; for (final senderConfig in permissiveSenderConfigs) { for (final ownMessageReason in ownMessageReasons) { for (final timeLimitConfig in permissiveTimeLimitConfigs) { doTest(true, CanDeleteMessageParams.restrictiveForChannelMessageExcept( isChannelArchived: false, senderConfig: senderConfig, inRealmCanDeleteOwnMessageGroup: ownMessageReason.realm, inChannelCanDeleteOwnMessageGroup: ownMessageReason.channel, timeLimitConfig: timeLimitConfig)); } } }

and if that's right, maybe I can do just these two?:

self, realm, time-not-limited

self-owned bot, channel, inside time limit

(or some other subset)

we don't want to exercise all of them

(because of the code-complexity and computation costs, and we'd be unlikely to catch more bugs by doing so)

Yeah, I agree.

…t doc And leave a comment so we remember to keep this order when adding more fields.

Like HasUserStore for data about users (added in 623bcb4), and HasRealmStore for data about the realm, this will help make it convenient for other substores to refer to data about channels.

This way, this substore becomes a valid home for methods that need to refer to data about both messages and channels. See also fab85ca, where we provided UserStore to ChannelStore.

chrisbobbe · 2025-09-10T23:16:23Z

Thanks for the review! Revision pushed.

gnprice

Thanks for the revision! Just a few comments below.

gnprice · 2025-09-10T23:31:07Z

lib/model/message.dart

+    if (realmCanDeleteAnyMessageGroup != null
+        && selfHasPermissionForGroupSetting(realmCanDeleteAnyMessageGroup!,


Let's file an issue to track that follow-up. Then also this TODO comment can refer to the issue.

gnprice · 2025-09-10T23:33:25Z

lib/model/message.dart

+      case RealmDeleteOwnMessagePolicy.everyone:
+      case RealmDeleteOwnMessagePolicy.members:
+        return true;


Should these cases really have the same behavior? The self-user could be a guest, right?

Ah yeah, I think I was confusing guests with the public-access option (which mobile doesn't offer yet)

gnprice · 2025-09-10T23:38:01Z

lib/model/message.dart

+    final message = messages[messageId];
+    if (message == null) return false; // TODO(log)
+
+    final ZulipStream? channel;
+    if (message is StreamMessage) {
+      channel = streams[message.streamId];
+      if (channel == null) {
+        assert(false); // TODO(log)
+        return true;


Is it intentional that the "unknown message" and "unknown channel" cases end up returning opposite fallback answers? Seems at first thought like they should turn out the same.

gnprice · 2025-09-10T23:40:32Z

test/model/message_test.dart

+  unknown,
+  self,
+  otherHuman,
+  botOwnedBySelf,


Yeah, I agree.

Related: zulip#1548

chrisbobbe · 2025-09-11T00:49:53Z

Thanks! Revision pushed.

gnprice · 2025-09-11T01:08:13Z

Thanks! Looks good; merging.

chrisbobbe requested a review from rajveermalviya September 5, 2025 01:37

chrisbobbe assigned rajveermalviya Sep 5, 2025

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

gnprice reviewed Sep 5, 2025

View reviewed changes

chrisbobbe force-pushed the pr-self-can-delete-message branch from 7aaff57 to 41ba775 Compare September 5, 2025 21:42

gnprice reviewed Sep 5, 2025

View reviewed changes

chrisbobbe force-pushed the pr-self-can-delete-message branch from 41ba775 to 4e8b5c6 Compare September 5, 2025 22:39

gnprice reviewed Sep 5, 2025

View reviewed changes

rajveermalviya reviewed Sep 8, 2025

View reviewed changes

chrisbobbe force-pushed the pr-self-can-delete-message branch from 4e8b5c6 to 263e304 Compare September 8, 2025 18:52

chrisbobbe requested a review from gnprice September 8, 2025 18:53

chrisbobbe assigned gnprice and unassigned rajveermalviya Sep 8, 2025

chrisbobbe 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 8, 2025

gnprice reviewed Sep 8, 2025

View reviewed changes

chrisbobbe added 8 commits September 10, 2025 15:31

api: Add realm-level restrict-message-deleting fields to InitialSnapshot

1c10f9a

channel: Add per-channel restrict-message-deleting fields

1225c09

realm [nfc]: Order updatable realm-setting fields by realm/update_dic…

772b71d

…t doc And leave a comment so we remember to keep this order when adding more fields.

realm: Add realm-level restrict-message-deleting fields to RealmStore

dd70226

channel: Add isArchived field

db5f9bd

initial_snapshot: Fill in SupportedPermissionSettings.fixture.realm

9c37ed4

channel [nfc]: Add HasChannelStore for other substores to use

f978e51

Like HasUserStore for data about users (added in 623bcb4), and HasRealmStore for data about the realm, this will help make it convenient for other substores to refer to data about channels.

messages [nfc]: Provide ChannelStore to MessageStore

e5b43df

This way, this substore becomes a valid home for methods that need to refer to data about both messages and channels. See also fab85ca, where we provided UserStore to ChannelStore.

chrisbobbe force-pushed the pr-self-can-delete-message branch from 263e304 to b25b8b1 Compare September 10, 2025 23:16

gnprice reviewed Sep 10, 2025

View reviewed changes

chrisbobbe mentioned this pull request Sep 11, 2025

Make role:administrators the default for realm_can_delete_any_message_group #1850

Open

message: Implement selfCanDeleteMessage

536cbb7

Related: zulip#1548

chrisbobbe force-pushed the pr-self-can-delete-message branch from b25b8b1 to 536cbb7 Compare September 11, 2025 00:49

gnprice merged commit 536cbb7 into zulip:main Sep 11, 2025
1 check passed


		const RealmDeleteOwnMessagePolicy({required this.apiValue});

		final int? apiValue;

		// no default; allow `null` to simulate servers without this
		realmDeleteOwnMessagePolicy: realmDeleteOwnMessagePolicy,

		if (realmCanDeleteAnyMessageGroup != null
		&& selfHasPermissionForGroupSetting(realmCanDeleteAnyMessageGroup!,

Implement permissions check for can-delete-message #1842

Implement permissions check for can-delete-message #1842

Conversation

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

Uh oh!

Uh oh!

gnprice commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 5, 2025

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Sep 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrisbobbe Sep 8, 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!

chrisbobbe commented Sep 8, 2025

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!

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 10, 2025

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!

chrisbobbe commented Sep 11, 2025

Uh oh!

chrisbobbe commented Sep 5, 2025 •

edited

Loading

chrisbobbe Sep 8, 2025 •

edited

Loading