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

@@ -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

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

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:

zerver/tests/test_subs.py

@@ -2834,9 +2834,7 @@ class StreamAdminTest(ZulipTestCase):
                 "principals": orjson.dumps([99]).decode(),
             },
         )
-        self.assert_json_error(
+        self.assert_json_error(result, "No such user", status_code=400)
-            result, "User not authorized to execute queries on behalf of '99'", status_code=403
-        )
 
 
 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(
+        self.assert_json_error(result, "User is deactivated", status_code=400)
-            result,
-            f"User not authorized to execute queries on behalf of '{target_profile.id}'",
-            status_code=403,
-        )
 
     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(
+        self.assert_json_error(result, "No such user", status_code=400)
-            result,
-            f"User not authorized to execute queries on behalf of '{invalid_principal}'",
-            status_code=403,
-        )
 
     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(
+        self.assert_json_error(result, "No such user", status_code=400)
-            result,
-            f"User not authorized to execute queries on behalf of '{invalid_principal}'",
-            status_code=403,
-        )
 
     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(
+        self.assert_json_error(result, "No such user", status_code=400)
-            result,
-            f"User not authorized to execute queries on behalf of '{principal}'",
-            status_code=403,
-        )
 
     def helper_check_subs_before_and_after_remove(
         self,

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 (
+from zerver.models import Realm, Stream, UserGroup, UserMessage, UserProfile, get_system_bot
-    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}'")
 
 
 def principal_to_user_profile(agent: UserProfile, principal: Union[str, int]) -> UserProfile:
-    try:
+    if isinstance(principal, str):
-        if isinstance(principal, str):
+        return access_user_by_email(
-            return get_active_user(principal, agent.realm)
+            agent, principal, allow_deactivated=False, allow_bots=True, for_admin=False
-        else:
+        )
-            return get_active_user_profile_by_id_in_realm(principal, agent.realm)
+    else:
-    except UserProfile.DoesNotExist:
+        return access_user_by_id(
-        # We have to make sure we don't leak information about which users
+            agent, principal, allow_deactivated=False, allow_bots=True, for_admin=False
-        # 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)
 
 
 def user_directly_controls_user(user_profile: UserProfile, target: UserProfile) -> bool: