From 8bca5652183c66436c9ddeddf28189d1649c6105 Mon Sep 17 00:00:00 2001 From: Sahil Batra Date: Thu, 2 May 2024 19:22:23 +0530 Subject: [PATCH] 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 Co-authored-by: Greg PRice --- api_docs/changelog.md | 6 + api_docs/group-setting-values.md | 83 ++++++++++++ api_docs/roles-and-permissions.md | 5 + api_docs/sidebar_index.md | 1 + version.py | 2 +- zerver/lib/exceptions.py | 13 ++ zerver/openapi/zulip.yaml | 85 +++++++----- zerver/tests/test_user_groups.py | 208 +++++++++++++++++++++++++++--- zerver/views/user_groups.py | 35 ++++- 9 files changed, 384 insertions(+), 54 deletions(-) create mode 100644 api_docs/group-setting-values.md diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 7928d4e50d..a54dd829cf 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -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): diff --git a/api_docs/group-setting-values.md b/api_docs/group-setting-values.md new file mode 100644 index 0000000000..aceaf4f7c2 --- /dev/null +++ b/api_docs/group-setting-values.md @@ -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. diff --git a/api_docs/roles-and-permissions.md b/api_docs/roles-and-permissions.md index 662ae6513d..db3c4acba9 100644 --- a/api_docs/roles-and-permissions.md +++ b/api_docs/roles-and-permissions.md @@ -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 diff --git a/api_docs/sidebar_index.md b/api_docs/sidebar_index.md index 69a65ca321..10540e7946 100644 --- a/api_docs/sidebar_index.md +++ b/api_docs/sidebar_index.md @@ -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) diff --git a/version.py b/version.py index e812d0a121..21259995b8 100644 --- a/version.py +++ b/version.py @@ -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 diff --git a/zerver/lib/exceptions.py b/zerver/lib/exceptions.py index 45d4189522..e970544a60 100644 --- a/zerver/lib/exceptions.py +++ b/zerver/lib/exceptions.py @@ -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.") diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index e2a1e59c9e..f95a10058a 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -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: | diff --git a/zerver/tests/test_user_groups.py b/zerver/tests/test_user_groups.py index f1783b77b4..eefed49ccb 100644 --- a/zerver/tests/test_user_groups.py +++ b/zerver/tests/test_user_groups.py @@ -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") diff --git a/zerver/views/user_groups.py b/zerver/views/user_groups.py index c6abb9f522..8ca83bae70 100644 --- a/zerver/views/user_groups.py +++ b/zerver/views/user_groups.py @@ -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,