Mirror
/
zulip
mirror of https://github.com/zulip/zulip.git
1
0
Fork 0
Code Issues Projects Releases Wiki Activity

groups: Pass old setting value for can_mention_group. 

This commit adds support to pass object containing both old and new
values of the can_mention_group setting, as well as detailed API
documentation for this part of the API system.

Co-authored-by: Tim Abbott <tabbott@zulip.com>
Co-authored-by: Greg PRice <greg@zulip.com>
This commit is contained in:
Sahil Batra 2024-05-02 19:22:23 +05:30 committed by Tim Abbott
parent 0cc2244aac
commit 8bca565218
9 changed files with 384 additions and 54 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

6
api_docs/changelog.md
View File

@ -20,6 +20,12 @@ format used by the Zulip server that they are interacting with.
## Changes in Zulip 9.0
**Feature level 260**:
* [`PATCH /user_groups/{user_group_id}`](/api/update-user-group):
Updating `can_mention_group` now uses a race-resistant format where
the client sends the expected `old` value and desired `new` value.
**Feature level 259**:
* [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events):

83
api_docs/group-setting-values.md Normal file
View File

@ -0,0 +1,83 @@
# Group-setting values
Settings defining permissions in Zulip are increasingly represented
using [user groups](/help/user-groups), which offer much more flexible
configuration than the older [roles](/api/roles-and-permissions) system.
In the API, these settings are represented using a **group-setting
value**, which can take two forms:
- An integer user group ID, which can be either a named user group
visible in the UI or a [role-based system group](#system-groups).
- An object with fields `direct_member_ids` containing a list of
integer user IDs and `direct_subgroup_ids` containing a list of
integer group IDs. The setting's value is the union of the
identified collection of users and groups.
Group-setting values in the object form function very much like a
formal user group object, without requiring the naming and UI clutter
overhead involved with creating a visible user group just to store the
value of a single setting.
The server will canonicalize an object with empty `direct_member_ids`
and with `direct_subgroup_ids` containing just the given group ID to
the integer format.
## System groups
The Zulip server maintains a collection of system groups that
correspond to the users with a given role; this makes it convenient to
store concepts like "all administrators" in a group-setting
value. These use a special naming convention and can be recognized by
the `is_system_group` property on their group object.
The following system groups are maintained by the Zulip server:
- `role:internet`: Everyone on the Internet has this permission; this
is used to configure the [public access
option](/help/public-access-option).
- `role:everyone`: All users, including guests.
- `role:members`: All users, excluding guests.
- `role:fullmembers`: All [full
members](https://zulip.com/api/roles-and-permissions#determining-if-a-user-is-a-full-member)
of the organization.
- `role:moderators`: All users with at least the moderator role.
- `role:administrators`: All users with at least the administrator
role.
- `role:owners`: All users with the owner role.
- `role:nobody`: The formal empty group. Used in the API to represent
disabling a feature.
Client UI for setting a permission is encouraged to display system
groups using their description, rather than using their names, which
are chosen to be unique and clear in the API.
System groups should generally not be displayed in UI for
administering an organization's user groups, since they are not
directly mutable.
## Updating group-setting values
The Zulip API uses a special format for modifying an existing setting
using a group-setting value.
A **group-setting update** is an object with a `new` field and an
optional `old` field, each containing a group-setting value. The
setting's value will be set to the membership expressed by the `new`
field.
The `old` field expresses the client's understanding of the current
value of the setting. If the `old` field is present and does not match
the actual current value of the setting, then the request will fail
with error code `EXPECTATION_MISMATCH` and no changes will be applied.
When a user edits the setting in a UI, the resulting API request
should generally always include the `old` field, giving the value
the list had when the user started editing. This accurately expresses
the user's intent, and if two users edit the same list around the
same time, it prevents a situation where the second change
accidentally reverts the first one without either user noticing.
Omitting `old` is appropriate where the intent really is a new complete
list rather than an edit, for example in an integration that syncs the
list from an external source of truth.

5
api_docs/roles-and-permissions.md
View File

@ -102,6 +102,11 @@ and owners.
Note that specific settings and policies in the Zulip API that use these
permission levels will likely support a subset of those listed above.
## Group-based permissions
Some settings have been migrated to a more flexible system based on
[user groups](/api/group-setting-values).
## Determining if a user is a full member
When a Zulip organization has set up a [waiting period before new members

1
api_docs/sidebar_index.md
View File

@ -21,6 +21,7 @@
* [HTTP headers](/api/http-headers)
* [Error handling](/api/rest-error-handling)
* [Roles and permissions](/api/roles-and-permissions)
* [Group-setting values](/api/group-setting-values)
* [Message formatting](/api/message-formatting)
* [Client libraries](/api/client-libraries)
* [API changelog](/api/changelog)

2
version.py
View File

@ -33,7 +33,7 @@ DESKTOP_WARNING_VERSION = "5.9.3"
# Changes should be accompanied by documentation explaining what the
# new level means in api_docs/changelog.md, as well as "**Changes**"
# entries in the endpoint's documentation in `zulip.yaml`.
API_FEATURE_LEVEL = 259
API_FEATURE_LEVEL = 260
# Bump the minor PROVISION_VERSION to indicate that folks should provision
# only when going from an old version of the code to a newer version. Bump

13
zerver/lib/exceptions.py
View File

@ -52,6 +52,7 @@ class ErrorCode(Enum):
REMOTE_BILLING_UNAUTHENTICATED_USER = auto()
REMOTE_REALM_SERVER_MISMATCH_ERROR = auto()
PUSH_NOTIFICATIONS_DISALLOWED = auto()
EXPECTATION_MISMATCH = auto()
class JsonableError(Exception):
@ -656,3 +657,15 @@ class TopicWildcardMentionNotAllowedError(JsonableError):
@override
def msg_format() -> str:
return _("You do not have permission to use topic wildcard mentions in this topic.")
class PreviousSettingValueMismatchedError(JsonableError):
code: ErrorCode = ErrorCode.EXPECTATION_MISMATCH
def __init__(self) -> None:
pass
@staticmethod
@override
def msg_format() -> str:
return _("'old' value does not match the expected value.")

85
zerver/openapi/zulip.yaml
View File

@ -3167,7 +3167,7 @@ paths:
changed.
can_mention_group:
allOf:
- $ref: "#/components/schemas/CanMentionGroup"
- $ref: "#/components/schemas/GroupSettingValue"
- description: |
Either the ID of a named user group that has permission
to mention the group, or an object describing the set of
@ -18589,9 +18589,8 @@ paths:
can_mention_group:
allOf:
- description: |
Either the ID of a named user group that has permission to
mention the group, or an object describing the set of users
and groups who have permission mention the new group.
A [group-setting value](/api/group-setting-values) defining the set
of users who have permission to mention the new group.
This setting cannot be set to `"role:internet"` and
`"role:owners"` system groups.
@ -18604,7 +18603,7 @@ paths:
New in Zulip 8.0 (feature level 191). Previously, groups
could be mentioned if and only if they were not system groups.
- $ref: "#/components/schemas/CanMentionGroup"
- $ref: "#/components/schemas/GroupSettingValue"
example: 11
required:
- name
@ -18742,25 +18741,47 @@ paths:
type: string
example: The marketing team.
can_mention_group:
allOf:
- description: |
Either the ID of a named user group that has permission to
mention the group, or an object describing the set of users
and groups who have permission mention the group.
description: |
The set of users who have permission to mention
this group, expressed as an [update to a
group-setting value](/api/group-setting-values#updating-group-setting-values).
This setting cannot be set to `"role:internet"` and `"role:owners"`
system groups.
This setting cannot be set to `"role:internet"` and `"role:owners"`
system groups.
**Changes**: Before Zulip 9.0 (feature level 258), the
`can_mention_group` field was always an integer.
**Changes**: In Zulip 9.0 (feature level 260), this was updated
to only accept an object with `old` and `new` fields.
**Changes**: Before Zulip 8.0 (feature level 198),
the `can_mention_group` setting was named `can_mention_group_id`.
**Changes**: Before Zulip 9.0 (feature level 258), the
`can_mention_group` field was always an integer.
New in Zulip 8.0 (feature level 191). Previously, groups
could be mentioned if and only if they were not system groups.
- $ref: "#/components/schemas/CanMentionGroup"
example: 12
**Changes**: Before Zulip 8.0 (feature level 198),
the `can_mention_group` setting was named `can_mention_group_id`.
New in Zulip 8.0 (feature level 191). Previously, groups
could be mentioned if and only if they were not system groups.
type: object
additionalProperties: false
properties:
new:
allOf:
- description: |
The new [group-setting value](/api/group-setting-values) for who would
have the permission to mention the group.
- $ref: "#/components/schemas/GroupSettingValue"
old:
allOf:
- description: |
The expected current [group-setting value](/api/group-setting-values)
for who has the permission to mention the group.
- $ref: "#/components/schemas/GroupSettingValue"
required:
- new
example:
{
"new": {"direct_members": [10], "direct_subgroups": [11]},
"old": 11,
}
encoding:
can_mention_group:
contentType: application/json
@ -18881,11 +18902,10 @@ paths:
**Changes**: New in Zulip 5.0 (feature level 93).
can_mention_group:
allOf:
- $ref: "#/components/schemas/CanMentionGroup"
- $ref: "#/components/schemas/GroupSettingValue"
- description: |
Either the ID of a named user group that has permission to
mention the group, or an object describing the set of users
and groups who have permission to mention the group.
A [group-setting value](/api/group-setting-values) defining the set
of users who have permission to mention the new group.
**Changes**: Before Zulip 9.0 (feature level 258), the
`can_mention_group` field was always an integer.
@ -20065,11 +20085,10 @@ components:
**Changes**: New in Zulip 5.0 (feature level 93).
can_mention_group:
allOf:
- $ref: "#/components/schemas/CanMentionGroup"
- $ref: "#/components/schemas/GroupSettingValue"
- description: |
Either the ID of a named user group that has permission to
mention the group, or an object describing the set of users
and groups who have permission mention the group.
A [group-setting value](/api/group-setting-values) defining the set
of users who have permission to mention the new group.
**Changes**: Before Zulip 9.0 (feature level 258), the
`can_mention_group` field was always an integer.
@ -20079,26 +20098,26 @@ components:
New in Zulip 8.0 (feature level 191). Previously, groups
could be mentioned if and only if they were not system groups.
CanMentionGroup:
GroupSettingValue:
oneOf:
- type: object
additionalProperties: false
properties:
direct_members:
description: |
The list of user IDs that have permission to
mention the group.
The list of IDs of individual users in the collection of users with this permission.
type: array
items:
type: integer
direct_subgroups:
description: |
The list of user group IDs that have permission
to mention the group.
The list of IDs of the groups in the collection of users with this permission.
type: array
items:
type: integer
- type: integer
description: |
The ID of the [user group](/help/user-groups) with this permission.
Invite:
type: object
description: |

208
zerver/tests/test_user_groups.py
View File

@ -645,7 +645,11 @@ class UserGroupAPITestCase(UserGroupTestCase):
self.login("hamlet")
params = {
"can_mention_group": orjson.dumps(moderators_group.id).decode(),
"can_mention_group": orjson.dumps(
{
"new": moderators_group.id,
}
).decode(),
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_success(result)
@ -653,7 +657,11 @@ class UserGroupAPITestCase(UserGroupTestCase):
self.assertEqual(support_group.can_mention_group, moderators_group.usergroup_ptr)
params = {
"can_mention_group": orjson.dumps(marketing_group.id).decode(),
"can_mention_group": orjson.dumps(
{
"new": marketing_group.id,
}
).decode(),
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_success(result)
@ -664,7 +672,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
name="role:nobody", realm=hamlet.realm, is_system_group=True
)
params = {
"can_mention_group": orjson.dumps(nobody_group.id).decode(),
"can_mention_group": orjson.dumps({"new": nobody_group.id}).decode(),
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_success(result)
@ -675,8 +683,10 @@ class UserGroupAPITestCase(UserGroupTestCase):
params = {
"can_mention_group": orjson.dumps(
{
"direct_members": [othello.id],
"direct_subgroups": [moderators_group.id, marketing_group.id],
"new": {
"direct_members": [othello.id],
"direct_subgroups": [moderators_group.id, marketing_group.id],
}
}
).decode()
}
@ -696,8 +706,10 @@ class UserGroupAPITestCase(UserGroupTestCase):
params = {
"can_mention_group": orjson.dumps(
{
"direct_members": [othello.id, prospero.id],
"direct_subgroups": [moderators_group.id, marketing_group.id],
"new": {
"direct_members": [othello.id, prospero.id],
"direct_subgroups": [moderators_group.id, marketing_group.id],
}
}
).decode()
}
@ -717,7 +729,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
[marketing_group, moderators_group],
)
params = {"can_mention_group": orjson.dumps(marketing_group.id).decode()}
params = {"can_mention_group": orjson.dumps({"new": marketing_group.id}).decode()}
previous_can_mention_group_id = support_group.can_mention_group_id
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_success(result)
@ -731,7 +743,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
name="role:owners", realm=hamlet.realm, is_system_group=True
)
params = {
"can_mention_group": orjson.dumps(owners_group.id).decode(),
"can_mention_group": orjson.dumps({"new": owners_group.id}).decode(),
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(
@ -742,7 +754,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
name="role:internet", realm=hamlet.realm, is_system_group=True
)
params = {
"can_mention_group": orjson.dumps(internet_group.id).decode(),
"can_mention_group": orjson.dumps({"new": internet_group.id}).decode(),
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(
@ -750,7 +762,7 @@ class UserGroupAPITestCase(UserGroupTestCase):
)
params = {
"can_mention_group": orjson.dumps(1111).decode(),
"can_mention_group": orjson.dumps({"new": 1111}).decode(),
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "Invalid user group")
@ -758,8 +770,10 @@ class UserGroupAPITestCase(UserGroupTestCase):
params = {
"can_mention_group": orjson.dumps(
{
"direct_members": [1111, othello.id],
"direct_subgroups": [moderators_group.id, marketing_group.id],
"new": {
"direct_members": [1111, othello.id],
"direct_subgroups": [moderators_group.id, marketing_group.id],
}
}
).decode()
}
@ -769,8 +783,10 @@ class UserGroupAPITestCase(UserGroupTestCase):
params = {
"can_mention_group": orjson.dumps(
{
"direct_members": [prospero.id, othello.id],
"direct_subgroups": [1111, marketing_group.id],
"new": {
"direct_members": [prospero.id, othello.id],
"direct_subgroups": [1111, marketing_group.id],
}
}
).decode()
}
@ -790,6 +806,168 @@ class UserGroupAPITestCase(UserGroupTestCase):
result = self.client_patch(f"/json/user_groups/{support_user_group.id}", info=params)
self.assert_json_error(result, f"User group '{marketing_user_group.name}' already exists.")
def test_update_can_mention_group_setting_with_previous_value_passed(self) -> None:
hamlet = self.example_user("hamlet")
support_group = check_add_user_group(hamlet.realm, "support", [hamlet], acting_user=None)
marketing_group = check_add_user_group(
hamlet.realm, "marketing", [hamlet], acting_user=None
)
everyone_group = NamedUserGroup.objects.get(
name="role:everyone", realm=hamlet.realm, is_system_group=True
)
moderators_group = NamedUserGroup.objects.get(
name="role:moderators", realm=hamlet.realm, is_system_group=True
)
self.assertEqual(marketing_group.can_mention_group.id, everyone_group.id)
self.login("hamlet")
params = {
"can_mention_group": orjson.dumps(
{
"new": marketing_group.id,
"old": moderators_group.id,
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "'old' value does not match the expected value.")
othello = self.example_user("othello")
params = {
"can_mention_group": orjson.dumps(
{
"new": marketing_group.id,
"old": {
"direct_members": [othello.id],
"direct_subgroups": [everyone_group.id],
},
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "'old' value does not match the expected value.")
params = {
"can_mention_group": orjson.dumps(
{
"new": marketing_group.id,
"old": everyone_group.id,
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_success(result)
support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm)
self.assertEqual(support_group.can_mention_group, marketing_group.usergroup_ptr)
params = {
"can_mention_group": orjson.dumps(
{
"new": {
"direct_members": [othello.id],
"direct_subgroups": [moderators_group.id],
},
"old": {"direct_members": [], "direct_subgroups": [marketing_group.id]},
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_success(result)
support_group = NamedUserGroup.objects.get(name="support", realm=hamlet.realm)
self.assertCountEqual(
list(support_group.can_mention_group.direct_members.all()),
[othello],
)
self.assertCountEqual(
list(support_group.can_mention_group.direct_subgroups.all()),
[moderators_group],
)
params = {
"can_mention_group": orjson.dumps(
{
"new": {
"direct_members": [hamlet.id],
"direct_subgroups": [marketing_group.id],
},
"old": support_group.can_mention_group_id,
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "'old' value does not match the expected value.")
params = {
"can_mention_group": orjson.dumps(
{
"new": {
"direct_members": [hamlet.id],
"direct_subgroups": [marketing_group.id],
},
"old": moderators_group.id,
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "'old' value does not match the expected value.")
params = {
"can_mention_group": orjson.dumps(
{
"new": {
"direct_members": [hamlet.id],
"direct_subgroups": [marketing_group.id],
},
"old": {
"direct_members": [othello.id],
"direct_subgroups": [moderators_group.id],
},
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_success(result)
self.assertCountEqual(
list(support_group.can_mention_group.direct_members.all()),
[hamlet],
)
self.assertCountEqual(
list(support_group.can_mention_group.direct_subgroups.all()),
[marketing_group],
)
# Test error cases for completeness.
params = {
"can_mention_group": orjson.dumps(
{
"new": {
"direct_members": [othello.id],
"direct_subgroups": [moderators_group.id],
},
"old": {
"direct_members": [hamlet.id],
"direct_subgroups": [1111],
},
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "'old' value does not match the expected value.")
params = {
"can_mention_group": orjson.dumps(
{
"new": 1111,
"old": {
"direct_members": [hamlet.id],
"direct_subgroups": [marketing_group.id],
},
}
).decode()
}
result = self.client_patch(f"/json/user_groups/{support_group.id}", info=params)
self.assert_json_error(result, "Invalid user group")
def test_user_group_delete(self) -> None:
hamlet = self.example_user("hamlet")
self.login("hamlet")

35
zerver/views/user_groups.py
View File

@ -1,3 +1,4 @@
from dataclasses import dataclass
from typing import List, Optional, Sequence, Union
from django.conf import settings
@ -20,7 +21,7 @@ from zerver.actions.user_groups import (
remove_subgroups_from_user_group,
)
from zerver.decorator import require_member_or_admin, require_user_group_edit_permission
from zerver.lib.exceptions import JsonableError
from zerver.lib.exceptions import JsonableError, PreviousSettingValueMismatchedError
from zerver.lib.mention import MentionBackend, silent_mention_syntax_for_user
from zerver.lib.request import REQ, has_request_variables
from zerver.lib.response import json_success
@ -125,15 +126,31 @@ def are_both_setting_values_equal(
return False
def check_setting_value_changed(
def validate_group_setting_value_change(
current_value: UserGroup,
new_setting_value: Union[int, AnonymousSettingGroupDict],
expected_current_setting_value: Optional[Union[int, AnonymousSettingGroupDict]],
) -> bool:
current_setting_api_value = get_group_setting_value_for_api(current_value)
if expected_current_setting_value is not None and not are_both_setting_values_equal(
expected_current_setting_value,
current_setting_api_value,
):
# This check is here to help prevent races, by refusing to
# change a setting where the client (and thus the UI presented
# to user) showed a different existing state.
raise PreviousSettingValueMismatchedError
return not are_both_setting_values_equal(current_setting_api_value, new_setting_value)
@dataclass
class GroupSettingChangeRequest:
new: Union[int, AnonymousSettingGroupDict]
old: Optional[Union[int, AnonymousSettingGroupDict]] = None
@transaction.atomic
@require_user_group_edit_permission
@typed_endpoint
@ -144,7 +161,7 @@ def edit_user_group(
user_group_id: PathOnly[int],
name: Optional[str] = None,
description: Optional[str] = None,
can_mention_group: Optional[Json[Union[int, AnonymousSettingGroupDict]]] = None,
can_mention_group: Optional[Json[GroupSettingChangeRequest]] = None,
) -> HttpResponse:
if name is None and description is None and can_mention_group is None:
raise JsonableError(_("No new data supplied"))
@ -166,9 +183,17 @@ def edit_user_group(
if request_settings_dict[setting_name] is None:
continue
setting_value = request_settings_dict[setting_name]
new_setting_value = parse_group_setting_value(setting_value.new)
expected_current_setting_value = None
if setting_value.old is not None:
expected_current_setting_value = parse_group_setting_value(setting_value.old)
current_value = getattr(user_group, setting_name)
new_setting_value = parse_group_setting_value(request_settings_dict[setting_name])
if check_setting_value_changed(current_value, new_setting_value):
if validate_group_setting_value_change(
current_value, new_setting_value, expected_current_setting_value
):
setting_value_group = access_user_group_for_setting(
new_setting_value,
user_profile,