From ebfe9637c864cd32b69c54149b7ae5eb76e6f112 Mon Sep 17 00:00:00 2001 From: Lauryn Menard Date: Tue, 29 Aug 2023 16:53:47 +0200 Subject: [PATCH] subscribe-unsubscribe: Improve error response for unexpected users. Updates the API error response when there is an unknown or deactivated user in the `principals` parameter for either the `/api/subscribe` or `/api/unsubscribe` endpoints. We now use the `access_user_by_email` and `access_user_by_id` code paths, which return an HTTP response of 400 and a "BAD_REQUEST" code. Previously, an HTTP response of 403 was returned with a special "UNAUTHORIZED_PRINCIPAL" code in the error response. This code was not documented in the API documentation and is removed as a potential JsonableError code with these changes. Fixes #26593. --- api_docs/changelog.md | 10 +++++++++ version.py | 2 +- zerver/openapi/zulip.yaml | 21 ++++++++++++++++++- zerver/tests/test_subs.py | 28 +++++-------------------- zerver/views/streams.py | 44 +++++++++------------------------------ 5 files changed, 46 insertions(+), 59 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index b862cc5fb1..ea54b848af 100644 --- a/api_docs/changelog.md +++ b/api_docs/changelog.md @@ -20,6 +20,16 @@ format used by the Zulip server that they are interacting with. ## Changes in Zulip 8.0 +**Feature level 208** + +* [`POST /users/me/subscriptions`](/api/subscribe), + [`DELETE /users/me/subscriptions`](/api/unsubscribe): These endpoints + now return an HTTP status code of 400 with `code: "BAD_REQUEST"` in + the error response when a user specified in the `principals` parameter + is deactivated or does not exist. Previously, these endpoints returned + an HTTP status code of 403 with `code: "UNAUTHORIZED_PRINCIPAL"` in the + error response for these cases. + **Feature level 207** * [`POST /register`](/api/register-queue): Added `display_name` and diff --git a/version.py b/version.py index b52a17a104..f57744c92b 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 = 207 +API_FEATURE_LEVEL = 208 # 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/openapi/zulip.yaml b/zerver/openapi/zulip.yaml index 705d537cae..d85f24c4ab 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -8531,6 +8531,16 @@ paths: Note that the ability to subscribe oneself and/or other users to a specified stream depends on the [stream's privacy settings](/help/stream-permissions). + + **Changes**: Before Zulip 8.0 (feature level 208), if a user + specified by the [`principals`][principals-param] parameter was + a deactivated user, or did not exist, then an HTTP status code + of 403 was returned with `code: "UNAUTHORIZED_PRINCIPAL"` in the + error response. As of this feature level, an HTTP status code of + 400 is returned with `code: "BAD_REQUEST"` in the error response + for these cases. + + [principals-param]: /api/subscribe#parameter-principals x-curl-examples-parameters: oneOf: - type: include @@ -8836,7 +8846,15 @@ paths: by the [`can_remove_subscribers_group`][can-remove-parameter] for the stream. - **Changes**: Before Zulip 8.0 (feature level 197), + **Changes**: Before Zulip 8.0 (feature level 208), if a user + specified by the [`principals`][principals-param] parameter was + a deactivated user, or did not exist, then an HTTP status code + of 403 was returned with `code: "UNAUTHORIZED_PRINCIPAL"` in the + error response. As of this feature level, an HTTP status code of + 400 is returned with `code: "BAD_REQUEST"` in the error response + for these cases. + + Before Zulip 8.0 (feature level 197), the `can_remove_subscribers_group` setting was named `can_remove_subscribers_group_id`. @@ -8847,6 +8865,7 @@ paths: Before Zulip 6.0 (feature level 145), users had no special privileges for managing bots that they own. + [principals-param]: /api/unsubscribe#parameter-principals [can-remove-parameter]: /api/subscribe#parameter-can_remove_subscribers_group x-curl-examples-parameters: oneOf: diff --git a/zerver/tests/test_subs.py b/zerver/tests/test_subs.py index b6afee0d87..f144c53280 100644 --- a/zerver/tests/test_subs.py +++ b/zerver/tests/test_subs.py @@ -2834,9 +2834,7 @@ class StreamAdminTest(ZulipTestCase): "principals": orjson.dumps([99]).decode(), }, ) - self.assert_json_error( - result, "User not authorized to execute queries on behalf of '99'", status_code=403 - ) + self.assert_json_error(result, "No such user", status_code=400) class DefaultStreamTest(ZulipTestCase): @@ -5091,11 +5089,7 @@ class SubscriptionAPITest(ZulipTestCase): result = self.common_subscribe_to_streams( self.test_user, "Denmark", post_data, allow_fail=True ) - self.assert_json_error( - result, - f"User not authorized to execute queries on behalf of '{target_profile.id}'", - status_code=403, - ) + self.assert_json_error(result, "User is deactivated", status_code=400) def test_subscriptions_add_for_principal_invite_only(self) -> None: """ @@ -5138,11 +5132,7 @@ class SubscriptionAPITest(ZulipTestCase): {"principals": orjson.dumps([invalid_principal]).decode()}, allow_fail=True, ) - self.assert_json_error( - result, - f"User not authorized to execute queries on behalf of '{invalid_principal}'", - status_code=403, - ) + self.assert_json_error(result, "No such user", status_code=400) def test_subscription_add_invalid_principal(self) -> None: invalid_principal = 999 @@ -5155,11 +5145,7 @@ class SubscriptionAPITest(ZulipTestCase): {"principals": orjson.dumps([invalid_principal]).decode()}, allow_fail=True, ) - self.assert_json_error( - result, - f"User not authorized to execute queries on behalf of '{invalid_principal}'", - status_code=403, - ) + self.assert_json_error(result, "No such user", status_code=400) def test_subscription_add_principal_other_realm(self) -> None: """ @@ -5176,11 +5162,7 @@ class SubscriptionAPITest(ZulipTestCase): {"principals": orjson.dumps([principal]).decode()}, allow_fail=True, ) - self.assert_json_error( - result, - f"User not authorized to execute queries on behalf of '{principal}'", - status_code=403, - ) + self.assert_json_error(result, "No such user", status_code=400) def helper_check_subs_before_and_after_remove( self, diff --git a/zerver/views/streams.py b/zerver/views/streams.py index 840057d940..b3a5856070 100644 --- a/zerver/views/streams.py +++ b/zerver/views/streams.py @@ -83,6 +83,7 @@ from zerver.lib.topic import ( ) from zerver.lib.types import Validator from zerver.lib.user_groups import access_user_group_for_setting +from zerver.lib.users import access_user_by_email, access_user_by_id from zerver.lib.utils import assert_is_not_none from zerver.lib.validator import ( check_bool, @@ -98,43 +99,18 @@ from zerver.lib.validator import ( check_union, to_non_negative_int, ) -from zerver.models import ( - Realm, - Stream, - UserGroup, - UserMessage, - UserProfile, - get_active_user, - get_active_user_profile_by_id_in_realm, - get_system_bot, -) - - -class PrincipalError(JsonableError): - code = ErrorCode.UNAUTHORIZED_PRINCIPAL - data_fields = ["principal"] - http_status_code = 403 - - def __init__(self, principal: Union[int, str]) -> None: - self.principal: Union[int, str] = principal - - @staticmethod - def msg_format() -> str: - return _("User not authorized to execute queries on behalf of '{principal}'") +from zerver.models import Realm, Stream, UserGroup, UserMessage, UserProfile, get_system_bot def principal_to_user_profile(agent: UserProfile, principal: Union[str, int]) -> UserProfile: - try: - if isinstance(principal, str): - return get_active_user(principal, agent.realm) - else: - return get_active_user_profile_by_id_in_realm(principal, agent.realm) - except UserProfile.DoesNotExist: - # We have to make sure we don't leak information about which users - # are registered for Zulip in a different realm. We could do - # something a little more clever and check the domain part of the - # principal to maybe give a better error message - raise PrincipalError(principal) + if isinstance(principal, str): + return access_user_by_email( + agent, principal, allow_deactivated=False, allow_bots=True, for_admin=False + ) + else: + return access_user_by_id( + agent, principal, allow_deactivated=False, allow_bots=True, for_admin=False + ) def user_directly_controls_user(user_profile: UserProfile, target: UserProfile) -> bool: