diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 97b36002c0..c11ad6d6da 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,14 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 10.0 +**Feature level 296**: + +* [`POST /register`](/api/register-queue), [`GET /events`](/api/get-events), + [`POST /realm/profile_fields`](/api/create-custom-profile-field), + [`GET /realm/profile_fields`](/api/get-custom-profile-fields): Added a new + parameter `editable_by_user` to custom profile field objects, which indicates whether + regular users can edit the value of the profile field on their own account. + **Feature level 295** * [`GET /export/realm/consents`](/api/get-realm-export-consents): Added diff --git a/version.py b/version.py index 6e1d7940dd..bbc05dbd0a 100644 --- a/version.py +++ b/version.py @@ -34,7 +34,7 @@ DESKTOP_WARNING_VERSION = "5.9.3" # new level means in api_docs/changelog.md, as well as "**Changes**" # entries in the endpoint's documentation in `zulip.yaml`. -API_FEATURE_LEVEL = 295 # Last bumped for `/export/realm/consents` endpoint. +API_FEATURE_LEVEL = 296 # Last bumped for `editable_by_user` custom profile field setting. # 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/web/tests/lib/events.js b/web/tests/lib/events.js index 1b209ef98b..fe785bcd93 100644 --- a/web/tests/lib/events.js +++ b/web/tests/lib/events.js @@ -141,6 +141,7 @@ exports.fixtures = { order: 1, display_in_profile_summary: false, required: false, + editable_by_user: true, }, { id: 2, @@ -151,6 +152,7 @@ exports.fixtures = { order: 2, display_in_profile_summary: false, required: false, + editable_by_user: false, }, ], }, diff --git a/zerver/actions/custom_profile_fields.py b/zerver/actions/custom_profile_fields.py index ddd38ca1d5..34db9d869a 100644 --- a/zerver/actions/custom_profile_fields.py +++ b/zerver/actions/custom_profile_fields.py @@ -27,6 +27,7 @@ def try_add_realm_default_custom_profile_field( field_subtype: str, display_in_profile_summary: bool = False, required: bool = False, + editable_by_user: bool = True, ) -> CustomProfileField: field_data = DEFAULT_EXTERNAL_ACCOUNTS[field_subtype] custom_profile_field = CustomProfileField( @@ -37,6 +38,7 @@ def try_add_realm_default_custom_profile_field( field_data=orjson.dumps(dict(subtype=field_subtype)).decode(), display_in_profile_summary=display_in_profile_summary, required=required, + editable_by_user=editable_by_user, ) custom_profile_field.save() custom_profile_field.order = custom_profile_field.id @@ -54,6 +56,7 @@ def try_add_realm_custom_profile_field( field_data: ProfileFieldData | None = None, display_in_profile_summary: bool = False, required: bool = False, + editable_by_user: bool = True, ) -> CustomProfileField: custom_profile_field = CustomProfileField( realm=realm, @@ -61,6 +64,7 @@ def try_add_realm_custom_profile_field( field_type=field_type, display_in_profile_summary=display_in_profile_summary, required=required, + editable_by_user=editable_by_user, ) custom_profile_field.hint = hint if custom_profile_field.field_type in ( @@ -110,6 +114,7 @@ def try_update_realm_custom_profile_field( field_data: ProfileFieldData | None = None, display_in_profile_summary: bool | None = None, required: bool | None = None, + editable_by_user: bool | None = None, ) -> None: if name is not None: field.name = name @@ -117,6 +122,8 @@ def try_update_realm_custom_profile_field( field.hint = hint if required is not None: field.required = required + if editable_by_user is not None: + field.editable_by_user = editable_by_user if display_in_profile_summary is not None: field.display_in_profile_summary = display_in_profile_summary @@ -204,9 +211,18 @@ def do_update_user_custom_profile_data_if_changed( @transaction.atomic(durable=True) -def check_remove_custom_profile_field_value(user_profile: UserProfile, field_id: int) -> None: +def check_remove_custom_profile_field_value( + user_profile: UserProfile, field_id: int, acting_user: UserProfile +) -> None: try: custom_profile_field = CustomProfileField.objects.get(realm=user_profile.realm, id=field_id) + if not acting_user.is_realm_admin and not custom_profile_field.editable_by_user: + raise JsonableError( + _( + "You are not allowed to change this field. Contact an administrator to update it." + ) + ) + field_value = CustomProfileFieldValue.objects.get( field=custom_profile_field, user_profile=user_profile ) diff --git a/zerver/lib/event_schema.py b/zerver/lib/event_schema.py index 4f73918c85..13b8ecae62 100644 --- a/zerver/lib/event_schema.py +++ b/zerver/lib/event_schema.py @@ -171,6 +171,7 @@ custom_profile_field_type = DictType( ("field_data", str), ("order", int), ("required", bool), + ("editable_by_user", bool), ], optional_keys=[ ("display_in_profile_summary", bool), diff --git a/zerver/lib/types.py b/zerver/lib/types.py index 1c42e86abc..6937efeee9 100644 --- a/zerver/lib/types.py +++ b/zerver/lib/types.py @@ -25,6 +25,7 @@ class ProfileDataElementBase(TypedDict, total=False): hint: str display_in_profile_summary: bool required: bool + editable_by_user: bool field_data: str order: int diff --git a/zerver/lib/users.py b/zerver/lib/users.py index 343437403c..34062c80ec 100644 --- a/zerver/lib/users.py +++ b/zerver/lib/users.py @@ -479,7 +479,7 @@ def validate_user_custom_profile_field( def validate_user_custom_profile_data( - realm_id: int, profile_data: list[ProfileDataElementUpdateDict] + realm_id: int, profile_data: list[ProfileDataElementUpdateDict], acting_user: UserProfile ) -> None: # This function validate all custom field values according to their field type. for item in profile_data: @@ -489,6 +489,13 @@ def validate_user_custom_profile_data( except CustomProfileField.DoesNotExist: raise JsonableError(_("Field id {id} not found.").format(id=field_id)) + if not acting_user.is_realm_admin and not field.editable_by_user: + raise JsonableError( + _( + "You are not allowed to change this field. Contact an administrator to update it." + ) + ) + try: validate_user_custom_profile_field(realm_id, field, item["value"]) except ValidationError as error: diff --git a/zerver/migrations/0586_customprofilefield_editable_by_user.py b/zerver/migrations/0586_customprofilefield_editable_by_user.py new file mode 100644 index 0000000000..f7f98612ed --- /dev/null +++ b/zerver/migrations/0586_customprofilefield_editable_by_user.py @@ -0,0 +1,17 @@ +# Generated by Django 5.0.6 on 2024-06-29 20:06 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("zerver", "0585_userprofile_allow_private_data_export_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="customprofilefield", + name="editable_by_user", + field=models.BooleanField(db_default=True, default=True), + ), + ] diff --git a/zerver/models/custom_profile_fields.py b/zerver/models/custom_profile_fields.py index 5c39ada3ff..4e8044de95 100644 --- a/zerver/models/custom_profile_fields.py +++ b/zerver/models/custom_profile_fields.py @@ -82,6 +82,9 @@ class CustomProfileField(models.Model): display_in_profile_summary = models.BooleanField(default=False) required = models.BooleanField(default=False) + # Whether regular users can edit this field on their own account. + editable_by_user = models.BooleanField(default=True, db_default=True) + SHORT_TEXT = 1 LONG_TEXT = 2 SELECT = 3 @@ -170,6 +173,7 @@ class CustomProfileField(models.Model): "field_data": self.field_data, "order": self.order, "required": self.required, + "editable_by_user": self.editable_by_user, } if self.display_in_profile_summary: data_as_dict["display_in_profile_summary"] = True diff --git a/zerver/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 7ae782a936..524dc385f5 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -1866,6 +1866,7 @@ paths: "field_data": "", "order": 1, "required": true, + "editable_by_user": true, }, { "id": 2, @@ -1875,6 +1876,7 @@ paths: "field_data": "", "order": 2, "required": true, + "editable_by_user": true, }, { "id": 3, @@ -1884,6 +1886,7 @@ paths: "field_data": "", "order": 3, "required": false, + "editable_by_user": true, }, { "id": 4, @@ -1894,6 +1897,7 @@ paths: "order": 4, "display_in_profile_summary": true, "required": true, + "editable_by_user": true, }, { "id": 5, @@ -1903,6 +1907,7 @@ paths: "field_data": "", "order": 5, "required": false, + "editable_by_user": false, }, { "id": 6, @@ -1913,6 +1918,7 @@ paths: "order": 6, "display_in_profile_summary": true, "required": false, + "editable_by_user": true, }, { "id": 7, @@ -1922,6 +1928,7 @@ paths: "field_data": "", "order": 7, "required": true, + "editable_by_user": false, }, { "id": 8, @@ -1931,6 +1938,7 @@ paths: "field_data": '{"subtype":"github"}', "order": 8, "required": true, + "editable_by_user": true, }, ], "id": 0, @@ -11060,6 +11068,7 @@ paths: "field_data": "", "order": 1, "required": true, + "editable_by_user": false, }, { "id": 2, @@ -11069,6 +11078,7 @@ paths: "field_data": "", "order": 2, "required": true, + "editable_by_user": true, }, { "id": 3, @@ -11078,6 +11088,7 @@ paths: "field_data": "", "order": 3, "required": false, + "editable_by_user": true, }, { "id": 4, @@ -11088,6 +11099,7 @@ paths: "order": 4, "display_in_profile_summary": true, "required": true, + "editable_by_user": true, }, { "id": 5, @@ -11097,6 +11109,7 @@ paths: "field_data": "", "order": 5, "required": false, + "editable_by_user": false, }, { "id": 6, @@ -11107,6 +11120,7 @@ paths: "order": 6, "display_in_profile_summary": true, "required": false, + "editable_by_user": true, }, { "id": 7, @@ -11116,6 +11130,7 @@ paths: "field_data": "", "order": 7, "required": true, + "editable_by_user": false, }, { "id": 8, @@ -11125,6 +11140,7 @@ paths: "field_data": '{"subtype":"github"}', "order": 8, "required": true, + "editable_by_user": true, }, { "id": 9, @@ -11133,6 +11149,7 @@ paths: "hint": "What pronouns should people use to refer to you?", "order": 9, "required": false, + "editable_by_user": true, }, ], } @@ -11262,6 +11279,16 @@ paths: **Changes**: New in Zulip 9.0 (feature level 244). type: boolean example: true + editable_by_user: + description: | + Whether regular users can edit this profile field on their own account. + + Note that organization administrators can edit custom profile fields for any user + regardless of this setting. + + **Changes**: New in Zulip 10.0 (feature level 296). + type: boolean + example: true required: - field_type encoding: @@ -11273,6 +11300,8 @@ paths: contentType: application/json required: contentType: application/json + editable_by_user: + contentType: application/json responses: "200": description: Success. @@ -21180,6 +21209,16 @@ components: banner to any user who has not set a value for a required field. **Changes**: New in Zulip 9.0 (feature level 244). + editable_by_user: + type: boolean + description: | + Whether regular users can edit this profile field on their own account. + + Note that organization administrators can edit custom profile fields for any user + regardless of this setting. + + **Changes**: New in Zulip 10.0 (feature level 296). + default: true required: - id - type @@ -21187,6 +21226,7 @@ components: - name - hint - required + - editable_by_user OnboardingStep: type: object additionalProperties: false diff --git a/zerver/tests/test_custom_profile_data.py b/zerver/tests/test_custom_profile_data.py index bdda4bfb9c..65f7644ee1 100644 --- a/zerver/tests/test_custom_profile_data.py +++ b/zerver/tests/test_custom_profile_data.py @@ -454,6 +454,82 @@ class DeleteCustomProfileFieldTest(CustomProfileFieldTestCase): self.assertFalse(self.custom_field_exists_in_realm(field.id)) self.assertEqual(user_profile.customprofilefieldvalue_set.count(), self.original_count - 1) + def test_delete_value_with_editable_by_user(self) -> None: + iago = self.example_user("iago") + hamlet = self.example_user("hamlet") + realm = iago.realm + self.login("hamlet") + + biography_custom_field = CustomProfileField.objects.get(name="Biography", realm=realm) + birthday_custom_field = CustomProfileField.objects.get(name="Birthday", realm=realm) + + # Set and assert our initial state. + data = {} + data["editable_by_user"] = "false" + result = self.api_patch( + iago, f"/api/v1/realm/profile_fields/{birthday_custom_field.id}", info=data + ) + self.assert_json_success(result) + + birthday_custom_field.refresh_from_db() + self.assertFalse(birthday_custom_field.editable_by_user) + self.assertTrue(biography_custom_field.editable_by_user) + + self.assertTrue( + CustomProfileFieldValue.objects.filter( + user_profile=iago, field=birthday_custom_field + ).exists() + ) + self.assertTrue( + CustomProfileFieldValue.objects.filter( + user_profile=hamlet, field=birthday_custom_field + ).exists() + ) + self.assertTrue( + CustomProfileFieldValue.objects.filter( + user_profile=hamlet, field=biography_custom_field + ).exists() + ) + + # Users can only delete fields where editable_by_user is true. + result = self.client_delete( + "/json/users/me/profile_data", + {"data": orjson.dumps([biography_custom_field.id]).decode()}, + ) + self.assert_json_success(result) + self.assertFalse( + CustomProfileFieldValue.objects.filter( + user_profile=hamlet, field=biography_custom_field + ).exists() + ) + + result = self.client_delete( + "/json/users/me/profile_data", + {"data": orjson.dumps([birthday_custom_field.id]).decode()}, + ) + self.assert_json_error( + result, + "You are not allowed to change this field. Contact an administrator to update it.", + ) + self.assertTrue( + CustomProfileFieldValue.objects.filter( + user_profile=hamlet, field=birthday_custom_field + ).exists() + ) + + # Admins can always delete field values regardless of editable_by_user. + result = self.api_delete( + iago, + "/api/v1/users/me/profile_data", + {"data": orjson.dumps([birthday_custom_field.id]).decode()}, + ) + self.assert_json_success(result) + self.assertFalse( + CustomProfileFieldValue.objects.filter( + user_profile=iago, field=birthday_custom_field + ).exists() + ) + class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): def test_update(self) -> None: @@ -497,6 +573,15 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): msg = "required is not valid JSON" self.assert_json_error(result, msg) + result = self.client_patch( + f"/json/realm/profile_fields/{field.id}", + info={ + "editable_by_user": "invalid value", + }, + ) + msg = "editable_by_user is not valid JSON" + self.assert_json_error(result, msg) + result = self.client_patch( f"/json/realm/profile_fields/{field.id}", info={ @@ -504,6 +589,7 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): "hint": "New contact number", "display_in_profile_summary": "true", "required": "true", + "editable_by_user": "false", }, ) self.assert_json_success(result) @@ -514,8 +600,9 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): self.assertEqual(field.field_type, CustomProfileField.SHORT_TEXT) self.assertEqual(field.display_in_profile_summary, True) self.assertEqual(field.required, True) + self.assertEqual(field.editable_by_user, False) - # Not sending required should not set it to false. + # Not sending required or editable_by_user should not reset their value to default. result = self.client_patch( f"/json/realm/profile_fields/{field.id}", info={ @@ -526,6 +613,7 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): field.refresh_from_db() self.assertEqual(field.hint, "New hint") self.assertEqual(field.required, True) + self.assertEqual(field.editable_by_user, False) result = self.client_patch( f"/json/realm/profile_fields/{field.id}", @@ -942,6 +1030,78 @@ class UpdateCustomProfileFieldTest(CustomProfileFieldTestCase): ) self.assert_json_error(result, "Default custom field cannot be updated.") + def assert_profile_field_value( + self, user: UserProfile, field_id: int, field_value: str | None + ) -> None: + for field_dict in user.profile_data(): + if field_dict["id"] == field_id: + self.assertEqual(field_dict["value"], field_value) + + def test_update_with_editable_by_user(self) -> None: + iago = self.example_user("iago") + aaron = self.example_user("aaron") + self.login("aaron") + + # Create field with editable_by_user = false + realm_profile_field_data: dict[str, Any] = {} + realm_profile_field_data["name"] = "Dummy field" + realm_profile_field_data["field_type"] = CustomProfileField.SHORT_TEXT + realm_profile_field_data["editable_by_user"] = "false" + result = self.api_post(iago, "/api/v1/realm/profile_fields", info=realm_profile_field_data) + result_json = self.assert_json_success(result) + restricted_field_id = result_json["id"] + + field_data = [ + { + "id": restricted_field_id, + "value": "test", + } + ] + + # Admins can always change their own fields + self.assert_profile_field_value(iago, restricted_field_id, None) + result = self.api_patch( + iago, "/api/v1/users/me/profile_data", {"data": orjson.dumps(field_data).decode()} + ) + self.assert_json_success(result) + self.assert_profile_field_value(iago, restricted_field_id, "test") + + # Admins can always change fields of others + self.assert_profile_field_value(aaron, restricted_field_id, None) + result = self.api_patch( + iago, f"/api/v1/users/{aaron.id}", {"profile_data": orjson.dumps(field_data).decode()} + ) + self.assert_json_success(result) + self.assert_profile_field_value(aaron, restricted_field_id, "test") + + # Users cannot update field value when editable_by_user is false. + self.assert_profile_field_value(aaron, restricted_field_id, "test") + result = self.client_patch( + "/json/users/me/profile_data", {"data": orjson.dumps(field_data).decode()} + ) + self.assert_json_error( + result, + "You are not allowed to change this field. Contact an administrator to update it.", + ) + self.assert_profile_field_value(aaron, restricted_field_id, "test") + + # Change editable_by_user to true. + data = {} + data["editable_by_user"] = "true" + result = self.api_patch( + iago, f"/api/v1/realm/profile_fields/{restricted_field_id}", info=data + ) + self.assert_json_success(result) + + # Users can update field value when editable_by_user is true + self.assert_profile_field_value(aaron, restricted_field_id, "test") + field_data[0]["value"] = "test2" + result = self.client_patch( + "/json/users/me/profile_data", {"data": orjson.dumps(field_data).decode()} + ) + self.assert_json_success(result) + self.assert_profile_field_value(aaron, restricted_field_id, "test2") + class ListCustomProfileFieldTest(CustomProfileFieldTestCase): def test_list(self) -> None: diff --git a/zerver/tests/test_events.py b/zerver/tests/test_events.py index 2b933c6925..1b124dc0c8 100644 --- a/zerver/tests/test_events.py +++ b/zerver/tests/test_events.py @@ -1476,7 +1476,9 @@ class NormalActionsTest(BaseAction): # Test event for removing custom profile data with self.verify_action() as events: - check_remove_custom_profile_field_value(self.user_profile, field_id) + check_remove_custom_profile_field_value( + self.user_profile, field_id, acting_user=self.user_profile + ) check_realm_user_update("events[0]", events[0], "custom_profile_field") self.assertEqual(events[0]["person"]["custom_profile_field"].keys(), {"id", "value"}) diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index b91d1869d2..2371fbce8b 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -752,6 +752,18 @@ class PermissionTest(ZulipTestCase): new_profile_data = [] cordelia = self.example_user("cordelia") + # Setting editable_by_user to false shouldn't affect admin's ability + # to modify the profile field for other users. + biography_field = CustomProfileField.objects.get(name="Biography", realm=realm) + + data = {} + data["editable_by_user"] = "false" + result = self.client_patch(f"/json/realm/profile_fields/{biography_field.id}", info=data) + self.assert_json_success(result) + + biography_field.refresh_from_db() + self.assertFalse(biography_field.editable_by_user) + # Test for all type of data fields = { "Phone number": "short text data", diff --git a/zerver/views/custom_profile_fields.py b/zerver/views/custom_profile_fields.py index 9d8295fe45..68d3c55290 100644 --- a/zerver/views/custom_profile_fields.py +++ b/zerver/views/custom_profile_fields.py @@ -177,6 +177,7 @@ def create_realm_custom_profile_field( field_type: Json[int], display_in_profile_summary: Json[bool] = False, required: Json[bool] = False, + editable_by_user: Json[bool] = True, ) -> HttpResponse: if field_data is None: field_data = {} @@ -195,6 +196,7 @@ def create_realm_custom_profile_field( field_subtype=field_subtype, display_in_profile_summary=display_in_profile_summary, required=required, + editable_by_user=editable_by_user, ) return json_success(request, data={"id": field.id}) else: @@ -206,6 +208,7 @@ def create_realm_custom_profile_field( hint=hint, display_in_profile_summary=display_in_profile_summary, required=required, + editable_by_user=editable_by_user, ) return json_success(request, data={"id": field.id}) except IntegrityError: @@ -237,6 +240,7 @@ def update_realm_custom_profile_field( field_data: Json[ProfileFieldData] | None = None, required: Json[bool] | None = None, display_in_profile_summary: Json[bool] | None = None, + editable_by_user: Json[bool] | None = None, ) -> HttpResponse: realm = user_profile.realm try: @@ -276,6 +280,7 @@ def update_realm_custom_profile_field( field_data=field_data, display_in_profile_summary=display_in_profile_summary, required=required, + editable_by_user=editable_by_user, ) except IntegrityError: raise JsonableError(_("A field with that label already exists.")) @@ -303,7 +308,7 @@ def remove_user_custom_profile_data( data: Json[list[int]], ) -> HttpResponse: for field_id in data: - check_remove_custom_profile_field_value(user_profile, field_id) + check_remove_custom_profile_field_value(user_profile, field_id, acting_user=user_profile) return json_success(request) @@ -315,7 +320,7 @@ def update_user_custom_profile_data( *, data: Json[list[ProfileDataElementUpdateDict]], ) -> HttpResponse: - validate_user_custom_profile_data(user_profile.realm.id, data) + validate_user_custom_profile_data(user_profile.realm.id, data, acting_user=user_profile) do_update_user_custom_profile_data_if_changed(user_profile, data) # We need to call this explicitly otherwise constraints are not check return json_success(request) diff --git a/zerver/views/users.py b/zerver/views/users.py index ba8593525f..79696aa933 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -246,7 +246,7 @@ def update_user_backend( assert not isinstance(entry.value, int) if entry.value is None or not entry.value: field_id = entry.id - check_remove_custom_profile_field_value(target, field_id) + check_remove_custom_profile_field_value(target, field_id, acting_user=user_profile) else: clean_profile_data.append( { @@ -254,7 +254,9 @@ def update_user_backend( "value": entry.value, } ) - validate_user_custom_profile_data(target.realm.id, clean_profile_data) + validate_user_custom_profile_data( + target.realm.id, clean_profile_data, acting_user=user_profile + ) do_update_user_custom_profile_data_if_changed(target, clean_profile_data) return json_success(request)