From 1dc845f07b7dafa6396dc079fced2b7c1cade3bd Mon Sep 17 00:00:00 2001 From: Aman Agrawal Date: Thu, 15 May 2025 18:50:49 +0530 Subject: [PATCH] users: Allow spectators to access `/users` API endpoint. We need this to support faster initial loading time for spectators. --- api_docs/changelog.md | 6 +++++ version.py | 2 +- zerver/openapi/zulip.yaml | 5 ++++ zerver/tests/test_decorators.py | 4 ++-- zerver/tests/test_users.py | 41 +++++++++++++++++++++++++++++++-- zerver/views/users.py | 21 ++++++++++++++--- zproject/urls.py | 4 +++- 7 files changed, 74 insertions(+), 9 deletions(-) diff --git a/api_docs/changelog.md b/api_docs/changelog.md index 9f9a9a4705..a664deceae 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 11.0 +**Feature level 387** + +* [`GET /users`](/api/get-users): This endpoint no longer requires + authentication for organizations using the [public access + option](/help/public-access-option). + **Feature level 386** * [`PATCH /user_groups/{user_group_id}`](/api/update-user-group): diff --git a/version.py b/version.py index 07546d958c..c218381ed5 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 = 386 +API_FEATURE_LEVEL = 387 # 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 24065da2bf..6679c3e4c9 100644 --- a/zerver/openapi/zulip.yaml +++ b/zerver/openapi/zulip.yaml @@ -9526,6 +9526,11 @@ paths: Optionally includes values of [custom profile fields](/help/custom-profile-fields). You can also [fetch details on a single user](/api/get-user). + + **Changes**: This endpoint did not support unauthenticated + access in organizations using the [public access + option](/help/public-access-option) prior to Zulip 11.0 + (feature level 387). x-curl-examples-parameters: oneOf: - type: include diff --git a/zerver/tests/test_decorators.py b/zerver/tests/test_decorators.py index a86ad7264b..f95fc49f31 100644 --- a/zerver/tests/test_decorators.py +++ b/zerver/tests/test_decorators.py @@ -1440,9 +1440,9 @@ class RestAPITest(ZulipTestCase): self.assertEqual(str(result["Allow"]), "DELETE, GET, HEAD, PATCH") def test_http_accept_redirect(self) -> None: - result = self.client_get("/json/users", HTTP_ACCEPT="text/html") + result = self.client_get("/json/attachments", HTTP_ACCEPT="text/html") self.assertEqual(result.status_code, 302) - self.assertTrue(result["Location"].endswith("/login/?next=%2Fjson%2Fusers")) + self.assertTrue(result["Location"].endswith("/login/?next=%2Fjson%2Fattachments")) class TestUserAgentParsing(ZulipTestCase): diff --git a/zerver/tests/test_users.py b/zerver/tests/test_users.py index 63b7afc252..f52d59b80e 100644 --- a/zerver/tests/test_users.py +++ b/zerver/tests/test_users.py @@ -2164,7 +2164,7 @@ class ActivateTest(ZulipTestCase): session_key = self.client.session.session_key self.assertTrue(session_key) - result = self.client_get("/json/users") + result = self.client_get("/json/attachments") self.assert_json_success(result) self.assertEqual(Session.objects.filter(pk=session_key).count(), 1) @@ -2172,7 +2172,7 @@ class ActivateTest(ZulipTestCase): do_deactivate_user(user, acting_user=None) self.assertEqual(Session.objects.filter(pk=session_key).count(), 0) - result = self.client_get("/json/users") + result = self.client_get("/json/attachments") self.assert_json_error( result, "Not logged in: API authentication or user session required", 401 ) @@ -3195,6 +3195,43 @@ class GetProfileTest(ZulipTestCase): ) self.assertEqual(inaccessible_user_ids, {othello.id}) + def test_get_users_for_spectators(self) -> None: + # Checks that spectators can fetch users data. + hamlet = self.example_user("hamlet") + othello = self.example_user("othello") + + # Try with a realm with no web-public channels. + with self.assert_database_query_count(2): + result = self.client_get("/json/users", subdomain="lear") + self.assert_json_error( + result, + "Not logged in: API authentication or user session required", + status_code=401, + ) + + with self.assert_database_query_count(4): + result = self.client_get("/json/users") + self.assert_json_success(result) + result_dict = orjson.loads(result.content) + + all_fetched_users = result_dict["members"] + self.assertEqual( + len(all_fetched_users), UserProfile.objects.filter(realm=hamlet.realm).count() + ) + + user_ids_to_fetch = [hamlet.id, othello.id] + with self.assert_database_query_count(4): + result_dict = orjson.loads( + self.client_get( + "/json/users", {"user_ids": orjson.dumps(user_ids_to_fetch).decode()} + ).content + ) + all_fetched_users = result_dict["members"] + self.assertCountEqual( + [user["user_id"] for user in all_fetched_users], + user_ids_to_fetch, + ) + class DeleteUserTest(ZulipTestCase): def test_do_delete_user(self) -> None: diff --git a/zerver/views/users.py b/zerver/views/users.py index d35b2ff055..f02d908597 100644 --- a/zerver/views/users.py +++ b/zerver/views/users.py @@ -91,6 +91,7 @@ from zerver.models.realms import ( DomainNotAllowedForRealmError, EmailContainsPlusError, InvalidFakeEmailDomainError, + Realm, ) from zerver.models.users import ( get_user_by_delivery_email, @@ -722,12 +723,13 @@ def get_bots_backend(request: HttpRequest, user_profile: UserProfile) -> HttpRes def get_user_data( - user_profile: UserProfile, + user_profile: UserProfile | None, include_custom_profile_fields: bool, client_gravatar: bool, *, target_user: UserProfile | None = None, user_ids: list[int] | None = None, + realm: Realm | None = None, ) -> dict[str, Any]: """ The client_gravatar field here is set to True by default assuming that clients @@ -735,7 +737,9 @@ def get_user_data( an optimization than it might seem because gravatar URLs contain MD5 hashes that compress very poorly compared to other data. """ - realm = user_profile.realm + if realm is None: + assert user_profile is not None + realm = user_profile.realm members = get_users_for_api( realm, @@ -780,17 +784,28 @@ def get_member_backend( @typed_endpoint def get_members_backend( request: HttpRequest, - user_profile: UserProfile, + maybe_user_profile: UserProfile | AnonymousUser, *, user_ids: Json[list[int]] | None = None, include_custom_profile_fields: Json[bool] = False, client_gravatar: Json[bool] = True, ) -> HttpResponse: + if isinstance(maybe_user_profile, UserProfile): + user_profile = maybe_user_profile + realm = user_profile.realm + else: + realm = get_valid_realm_from_request(request) + if not realm.allow_web_public_streams_access(): + raise MissingAuthenticationError + + user_profile = None + data = get_user_data( user_profile, include_custom_profile_fields, client_gravatar, user_ids=user_ids, + realm=realm, ) return json_success(request, data) diff --git a/zproject/urls.py b/zproject/urls.py index 91d58e6159..921e2b899d 100644 --- a/zproject/urls.py +++ b/zproject/urls.py @@ -322,7 +322,9 @@ v1_api_and_json_patterns = [ # realm/deactivate -> zerver.views.deactivate_realm rest_path("realm/deactivate", POST=deactivate_realm), # users -> zerver.views.users - rest_path("users", GET=get_members_backend, POST=create_user_backend), + rest_path( + "users", GET=(get_members_backend, {"allow_anonymous_user_web"}), POST=create_user_backend + ), rest_path("users/me", GET=get_profile_backend, DELETE=deactivate_user_own_backend), rest_path("users//reactivate", POST=reactivate_user_backend), rest_path(