Skip to content

Commit 8e1932d

Browse files
chrisbobbechrisbobbe
committed
initial_snapshot: Centralize (most of) the group-permission defaults
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.
1 parent e63cf6f commit 8e1932d

File tree

4 files changed

+162
-37
lines changed

4 files changed

+162
-37
lines changed

‎lib/model/channel.dart‎

Lines changed: 5 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -177,25 +177,14 @@ mixin ChannelStore on UserStore {
177177
required DateTime atDate,
178178
}) {
179179
// Compare web's stream_data.has_content_access_via_group_permissions.
180-
// TODO(#814) try to clean up this logic; perhaps record more explicitly
181-
// what default/fallback value to use for a given group-based permission
182-
// on older servers.
183-
184-
if (channel.canAddSubscribersGroup != null
185-
&& selfHasPermissionForGroupSetting(channel.canAddSubscribersGroup!,
186-
GroupSettingType.stream, 'can_add_subscribers_group', atDate: atDate)) {
187-
// The behavior before this permission was introduced was equivalent to
188-
// the "nobody" group.
189-
// TODO(server-10): simplify
180+
181+
if (selfHasPermissionForGroupSetting(channel.canAddSubscribersGroup,
182+
GroupSettingType.stream, 'can_add_subscribers_group', atDate: atDate)) {
190183
return true;
191184
}
192185

193-
if (channel.canSubscribeGroup != null
194-
&& selfHasPermissionForGroupSetting(channel.canSubscribeGroup!,
195-
GroupSettingType.stream, 'can_subscribe_group', atDate: atDate)) {
196-
// The behavior before this permission was introduced was equivalent to
197-
// the "nobody" group.
198-
// TODO(server-10): simplify
186+
if (selfHasPermissionForGroupSetting(channel.canSubscribeGroup,
187+
GroupSettingType.stream, 'can_subscribe_group', atDate: atDate)) {
199188
return true;
200189
}
201190

‎lib/model/message.dart‎

Lines changed: 9 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -107,19 +107,14 @@ mixin MessageStore on ChannelStore {
107107
return false;
108108
}
109109

110-
if (realmCanDeleteAnyMessageGroup != null) {
111-
if (selfHasPermissionForGroupSetting(realmCanDeleteAnyMessageGroup!,
112-
GroupSettingType.realm, 'can_delete_any_message_group', atDate: atDate)) {
113-
return true;
114-
}
115-
} else if (selfUser.role.isAtLeast(UserRole.administrator)) {
110+
if (selfHasPermissionForGroupSetting(realmCanDeleteAnyMessageGroup,
111+
GroupSettingType.realm, 'can_delete_any_message_group', atDate: atDate)) {
116112
return true;
117113
}
118114

119115
if (channel != null) {
120-
if (channel.canDeleteAnyMessageGroup != null
121-
&& selfHasPermissionForGroupSetting(channel.canDeleteAnyMessageGroup!,
122-
GroupSettingType.stream, 'can_delete_any_message_group', atDate: atDate)) {
116+
if (selfHasPermissionForGroupSetting(channel.canDeleteAnyMessageGroup,
117+
GroupSettingType.stream, 'can_delete_any_message_group', atDate: atDate)) {
123118
return true;
124119
}
125120
}
@@ -138,6 +133,9 @@ mixin MessageStore on ChannelStore {
138133
// that's impossible here because `message` can't be an [OutboxMessage]
139134
// (it's a [Message] from [MessageStore.messages]).
140135

136+
// (selfHasPermissionForGroupSetting isn't equipped to handle the old-server
137+
// fallback logic for this specific permission; it's dynamic and depends on
138+
// realmDeleteOwnMessagePolicy, so we do our own null check here.)
141139
if (realmCanDeleteOwnMessageGroup != null) {
142140
if (!selfHasPermissionForGroupSetting(realmCanDeleteOwnMessageGroup!,
143141
GroupSettingType.realm, 'can_delete_own_message_group', atDate: atDate)) {
@@ -146,11 +144,8 @@ mixin MessageStore on ChannelStore {
146144
return false;
147145
}
148146

149-
if (
150-
channel.canDeleteOwnMessageGroup == null
151-
|| !selfHasPermissionForGroupSetting(channel.canDeleteOwnMessageGroup!,
152-
GroupSettingType.stream, 'can_delete_own_message_group', atDate: atDate)
153-
) {
147+
if (!selfHasPermissionForGroupSetting(channel.canDeleteOwnMessageGroup,
148+
GroupSettingType.stream, 'can_delete_own_message_group', atDate: atDate)) {
154149
return false;
155150
}
156151
}

‎lib/model/realm.dart‎

Lines changed: 58 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -142,8 +142,11 @@ mixin RealmStore on PerAccountStoreBase, UserGroupStore {
142142
bool selfHasPassedWaitingPeriod({required DateTime byDate});
143143

144144
/// Whether the self-user has the given (group-based) permission.
145-
bool selfHasPermissionForGroupSetting(GroupSettingValue value,
146-
GroupSettingType type, String name, {required DateTime atDate});
145+
///
146+
/// If the server doesn't know about the permission,
147+
/// pass null for [value] and a reasonable default will be chosen.
148+
bool selfHasPermissionForGroupSetting(GroupSettingValue? value,
149+
GroupSettingType type, String name, {required DateTime atDate});
147150
}
148151

149152
enum GroupSettingType { realm, stream, group }
@@ -196,7 +199,7 @@ mixin ProxyRealmStore on RealmStore {
196199
bool selfHasPassedWaitingPeriod({required DateTime byDate}) =>
197200
realmStore.selfHasPassedWaitingPeriod(byDate: byDate);
198201
@override
199-
bool selfHasPermissionForGroupSetting(GroupSettingValue value, GroupSettingType type, String name, {required DateTime atDate}) =>
202+
bool selfHasPermissionForGroupSetting(GroupSettingValue? value, GroupSettingType type, String name, {required DateTime atDate}) =>
200203
realmStore.selfHasPermissionForGroupSetting(value, type, name, atDate: atDate);
201204
}
202205

@@ -258,7 +261,7 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
258261
}
259262

260263
@override
261-
bool selfHasPermissionForGroupSetting(GroupSettingValue value,
264+
bool selfHasPermissionForGroupSetting(GroupSettingValue? value,
262265
GroupSettingType type, String name, {required DateTime atDate}) {
263266
// Compare web's settings_data.user_has_permission_for_group_setting.
264267
//
@@ -269,13 +272,61 @@ class RealmStoreImpl extends HasUserGroupStore with RealmStore {
269272
// That exists for deciding whether to offer the "Generate email address"
270273
// button, and if so then which users to offer in the dropdown;
271274
// it's predicting whether /api/get-stream-email-address would succeed.
272-
if (_selfUserRole == UserRole.guest) {
273-
final config = _groupSettingConfig(type, name);
274-
if (!config.allowEveryoneGroup) return false;
275+
276+
final config = _groupSettingConfig(type, name);
277+
278+
if (_selfUserRole == UserRole.guest && !config.allowEveryoneGroup) {
279+
return false;
280+
}
281+
282+
if (value == null) {
283+
// The server doesn't know about the permission. *We* know about it
284+
// (or presumably we wouldn't have called this method),
285+
// and we know a reasonable default; use that.
286+
return _hasPermissionByDefault(config, atDate: atDate);
275287
}
288+
276289
return selfInGroupSetting(value);
277290
}
278291

292+
bool _hasPermissionByDefault(
293+
PermissionSettingsItem config, {
294+
required DateTime atDate,
295+
}) {
296+
switch (config.defaultGroupName) {
297+
case DefaultGroupNameUnknown():
298+
// When we know about a permission, we should also know about the group
299+
// we've said is the default value for it.
300+
assert(false);
301+
return true;
302+
case PseudoSystemGroupName.streamCreatorOrNobody:
303+
// TODO(#1102) implement
304+
assert(() {
305+
throw UnimplementedError();
306+
}());
307+
return true;
308+
case SystemGroupName.everyoneOnInternet:
309+
case SystemGroupName.everyone:
310+
return true;
311+
case SystemGroupName.members:
312+
return _selfUserRole.isAtLeast(UserRole.member);
313+
case SystemGroupName.fullMembers:
314+
if (!_selfUserRole.isAtLeast(UserRole.member)) return false;
315+
if (_selfUserRole == UserRole.member) {
316+
return selfHasPassedWaitingPeriod(byDate: atDate);
317+
}
318+
return true;
319+
case SystemGroupName.moderators:
320+
return _selfUserRole.isAtLeast(UserRole.moderator);
321+
case SystemGroupName.administrators:
322+
return _selfUserRole.isAtLeast(UserRole.administrator);
323+
case SystemGroupName.owners:
324+
return _selfUserRole.isAtLeast(UserRole.owner);
325+
case SystemGroupName.nobody:
326+
return false;
327+
}
328+
}
329+
279330
/// The metadata for how to interpret the given group-based permission setting.
280331
PermissionSettingsItem _groupSettingConfig(GroupSettingType type, String name) {
281332
final supportedSettings = SupportedPermissionSettings.fixture;

‎test/model/realm_test.dart‎

Lines changed: 90 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@ import 'package:checks/checks.dart';
22
import 'package:test/scaffolding.dart';
33
import 'package:zulip/api/model/events.dart';
44
import 'package:zulip/api/model/model.dart';
5+
import 'package:zulip/api/model/permission.dart';
56
import 'package:zulip/model/realm.dart';
7+
import 'package:zulip/model/store.dart';
68

79
import '../example_data.dart' as eg;
810
import 'binding.dart';
@@ -112,6 +114,94 @@ void main() {
112114
check(hasPermission(selfUser, group, 'can_send_message_group'))
113115
.isFalse();
114116
});
117+
118+
group('fallbacks for permissions not known to the server', () {
119+
late PerAccountStore store;
120+
121+
void prepare({UserRole? selfUserRole}) {
122+
final selfUser = eg.user(role: selfUserRole);
123+
store = eg.store(selfUser: selfUser,
124+
initialSnapshot: eg.initialSnapshot(realmUsers: [selfUser]));
125+
}
126+
127+
void doCheck(GroupSettingType type, String name, bool expected) {
128+
check(store.selfHasPermissionForGroupSetting(null, type, name, atDate: now))
129+
.equals(expected);
130+
}
131+
132+
for (final pseudoSystemGroupName in PseudoSystemGroupName.values) {
133+
switch (pseudoSystemGroupName) {
134+
case PseudoSystemGroupName.streamCreatorOrNobody:
135+
// TODO implement and test
136+
}
137+
}
138+
139+
for (final systemGroupName in SystemGroupName.values) {
140+
switch (systemGroupName) {
141+
case SystemGroupName.everyoneOnInternet:
142+
// (No permissions where we use this default value; continue.)
143+
break;
144+
case SystemGroupName.everyone:
145+
test('everyone', () {
146+
prepare(selfUserRole: UserRole.guest);
147+
doCheck(GroupSettingType.realm, 'can_access_all_users_group', true);
148+
});
149+
case SystemGroupName.members:
150+
test('members, is guest', () {
151+
prepare(selfUserRole: UserRole.guest);
152+
doCheck(GroupSettingType.realm, 'can_add_custom_emoji_group', false);
153+
});
154+
test('members, is member', () {
155+
prepare(selfUserRole: UserRole.member);
156+
doCheck(GroupSettingType.realm, 'can_add_custom_emoji_group', true);
157+
});
158+
case SystemGroupName.fullMembers:
159+
// (No permissions where we use this default value; continue.)
160+
break;
161+
case SystemGroupName.moderators:
162+
test('moderators, is member', () {
163+
prepare(selfUserRole: UserRole.member);
164+
doCheck(GroupSettingType.realm, 'can_set_delete_message_policy_group', false);
165+
});
166+
test('moderators, is moderator', () {
167+
prepare(selfUserRole: UserRole.moderator);
168+
doCheck(GroupSettingType.realm, 'can_set_delete_message_policy_group', true);
169+
});
170+
case SystemGroupName.administrators:
171+
test('administrators, is moderator', () {
172+
prepare(selfUserRole: UserRole.moderator);
173+
doCheck(GroupSettingType.stream, 'can_remove_subscribers_group', false);
174+
});
175+
test('administrators, is administrator', () {
176+
prepare(selfUserRole: UserRole.administrator);
177+
doCheck(GroupSettingType.stream, 'can_remove_subscribers_group', true);
178+
});
179+
case SystemGroupName.owners:
180+
test('owners, is administrator', () {
181+
prepare(selfUserRole: UserRole.administrator);
182+
doCheck(GroupSettingType.realm, 'can_create_web_public_channel_group', false);
183+
});
184+
test('owners, is owner', () {
185+
prepare(selfUserRole: UserRole.owner);
186+
doCheck(GroupSettingType.realm, 'can_create_web_public_channel_group', true);
187+
});
188+
case SystemGroupName.nobody:
189+
test('nobody', () {
190+
prepare(selfUserRole: UserRole.owner);
191+
doCheck(GroupSettingType.stream, 'can_delete_own_message_group', false);
192+
});
193+
}
194+
}
195+
196+
test('throw on unknown name', () {
197+
// We should know about all the permissions we're trying to implement,
198+
// even the ones old servers don't know about.
199+
prepare(selfUserRole: UserRole.member);
200+
check(() => store.selfHasPermissionForGroupSetting(null,
201+
GroupSettingType.realm, 'example_future_permission_name', atDate: now),
202+
).throws<Error>();
203+
});
204+
});
115205
});
116206

117207
group('customProfileFields', () {

0 commit comments

Comments
 (0)