Skip to content

Commit

Permalink
register: Add client capability to not receive unknown users data.
Browse files Browse the repository at this point in the history
This commit adds a new client capability to decide whether the
client needs unknown users data or not.
  • Loading branch information
sahil839 authored and timabbott committed Dec 6, 2023
1 parent 3697df1 commit 965869d
Show file tree
Hide file tree
Showing 11 changed files with 144 additions and 10 deletions.
17 changes: 17 additions & 0 deletions api_docs/changelog.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,23 @@ format used by the Zulip server that they are interacting with.

## Changes in Zulip 8.0

**Feature level 232**

* [`POST /register`](/api/register-queue): Added a new
`user_list_incomplete` [client
capability](/api/register-queue#parameter-client_capabilities)
controlling whether `realm_users` contains "Unknown user"
placeholder objects for users that the current user cannot access
due to a `can_access_all_users_group` policy.

* [`GET /events`](/api/get-events): The new `user_list_incomplete`
[client
capability](/api/register-queue#parameter-client_capabilities)
controls whether to send `realm_user` events with `op: "add"`
containing "Unknown user" placeholder objects to clients when a new
user is created that the client does not have access to due to a
`can_access_all_users_group` policy.

**Feature level 231**

* [`POST /register`](/api/register-queue):
Expand Down
2 changes: 1 addition & 1 deletion version.py
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@
# 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 = 231
API_FEATURE_LEVEL = 232

# 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
Expand Down
1 change: 1 addition & 0 deletions zerver/actions/create_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -394,6 +394,7 @@ def notify_created_user(user_profile: UserProfile, notify_user_ids: List[int]) -
type="realm_user",
op="add",
person=get_data_for_inaccessible_user(user_profile.realm, user_profile.id),
inaccessible_user=True,
)
send_event_on_commit(user_profile.realm, event, user_ids_without_access_to_created_user)

Expand Down
21 changes: 17 additions & 4 deletions zerver/lib/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,7 @@ def fetch_initial_state_data(
spectator_requested_language: Optional[str] = None,
pronouns_field_type_supported: bool = True,
linkifier_url_template: bool = False,
user_list_incomplete: bool = False,
) -> Dict[str, Any]:
"""When `event_types` is None, fetches the core data powering the
web app's `page_params` and `/api/v1/register` (for mobile/terminal
Expand Down Expand Up @@ -480,6 +481,7 @@ def fetch_initial_state_data(
user_avatar_url_field_optional=user_avatar_url_field_optional,
# Don't send custom profile field values to spectators.
include_custom_profile_fields=user_profile is not None,
user_list_incomplete=user_list_incomplete,
)
state["cross_realm_bots"] = list(get_cross_realm_dicts())

Expand Down Expand Up @@ -704,6 +706,7 @@ def apply_events(
slim_presence: bool,
include_subscribers: bool,
linkifier_url_template: bool,
user_list_incomplete: bool,
) -> None:
for event in events:
if event["type"] == "restart":
Expand All @@ -726,6 +729,7 @@ def apply_events(
slim_presence=slim_presence,
include_subscribers=include_subscribers,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)


Expand All @@ -738,6 +742,7 @@ def apply_event(
slim_presence: bool,
include_subscribers: bool,
linkifier_url_template: bool,
user_list_incomplete: bool,
) -> None:
if event["type"] == "message":
state["max_message_id"] = max(state["max_message_id"], event["message"]["id"])
Expand Down Expand Up @@ -999,10 +1004,13 @@ def _draft_update_action(i: int) -> None:
]
elif event["op"] == "remove":
if person_user_id in state["raw_users"]:
inaccessible_user_dict = get_data_for_inaccessible_user(
user_profile.realm, person_user_id
)
state["raw_users"][person_user_id] = inaccessible_user_dict
if user_list_incomplete:
del state["raw_users"][person_user_id]
else:
inaccessible_user_dict = get_data_for_inaccessible_user(
user_profile.realm, person_user_id
)
state["raw_users"][person_user_id] = inaccessible_user_dict

if include_subscribers:
for sub in state["subscriptions"]:
Expand Down Expand Up @@ -1547,6 +1555,7 @@ def do_events_register(
stream_typing_notifications = client_capabilities.get("stream_typing_notifications", False)
user_settings_object = client_capabilities.get("user_settings_object", False)
linkifier_url_template = client_capabilities.get("linkifier_url_template", False)
user_list_incomplete = client_capabilities.get("user_list_incomplete", False)

if fetch_event_types is not None:
event_types_set: Optional[Set[str]] = set(fetch_event_types)
Expand All @@ -1570,6 +1579,7 @@ def do_events_register(
linkifier_url_template=linkifier_url_template,
user_avatar_url_field_optional=user_avatar_url_field_optional,
user_settings_object=user_settings_object,
user_list_incomplete=user_list_incomplete,
# slim_presence is a noop, because presence is not included.
slim_presence=True,
# Force include_subscribers=False for security reasons.
Expand Down Expand Up @@ -1605,6 +1615,7 @@ def do_events_register(
user_settings_object=user_settings_object,
pronouns_field_type_supported=pronouns_field_type_supported,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)

if queue_id is None:
Expand All @@ -1622,6 +1633,7 @@ def do_events_register(
include_streams=include_streams,
pronouns_field_type_supported=pronouns_field_type_supported,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)

# Apply events that came in while we were fetching initial data
Expand All @@ -1636,6 +1648,7 @@ def do_events_register(
slim_presence=slim_presence,
include_subscribers=include_subscribers,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)
except RestartEventError:
# This represents a rare race condition, where Tornado
Expand Down
12 changes: 7 additions & 5 deletions zerver/lib/users.py
Original file line number Diff line number Diff line change
Expand Up @@ -897,6 +897,7 @@ def get_users_for_api(
client_gravatar: bool,
user_avatar_url_field_optional: bool,
include_custom_profile_fields: bool = True,
user_list_incomplete: bool = False,
) -> Dict[int, APIUserDict]:
"""Fetches data about the target user(s) appropriate for sending to
acting_user via the standard format for the Zulip API. If
Expand Down Expand Up @@ -939,11 +940,12 @@ def get_users_for_api(
custom_profile_field_data=custom_profile_field_data,
)

for inaccessible_user_row in inaccessible_user_dicts:
# We already have the required data for inaccessible users
# in row object, so we can just add it to result directly.
user_id = inaccessible_user_row["user_id"]
result[user_id] = inaccessible_user_row
if not user_list_incomplete:
for inaccessible_user_row in inaccessible_user_dicts:
# We already have the required data for inaccessible users
# in row object, so we can just add it to result directly.
user_id = inaccessible_user_row["user_id"]
result[user_id] = inaccessible_user_row

return result

Expand Down
38 changes: 38 additions & 0 deletions zerver/openapi/zulip.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,22 @@ paths:
Processing this event is important to being able to display
basic details on other users given only their ID.

If the current user is a guest whose access to a newly created user
is limited by a `can_access_all_users_group` policy, and the event
queue was registered with the `user_list_incomplete` client
capability, then the event queue will not receive an event for such
a new user. If a newly created user is inaccessible to the current
user via such a policy, but the client lacks `user_list_incomplete`
client capability, then this event will be delivered to the queue,
with an "Unknown user" object with the usual format but placeholder
data whose only variable content is the user ID.

**Changes**: Before Zulip 8.0 (feature level 232), the
`user_list_incomplete` client capability did not exist, and so all
clients whose access to a new user was prevented by
`can_access_all_users_group` policy would receive a fake "Unknown
user" event for such a user.

**Changes**: Starting with Zulip 8.0 (feature level 228),
this event is also sent when a guest user gains access to
a user.
Expand Down Expand Up @@ -11621,6 +11637,13 @@ paths:
**Changes**: New in Zulip 7.0 (feature level 176). This capability
is for backwards-compatibility.

- `user_list_incomplete`: Boolean for whether the client supports not having an
incomplete user database. If true, then the `realm_users` array in the `register`
response will not include data for inaccessible users and clients of guest users will
not receive `realm_user op:add` events for newly created users that are not accessible
to the current user. **Changes**: New in Zulip 8.0 (feature level 232). This
capability is for backwards-compatibility.

[help-linkifiers]: /help/add-a-custom-linkifier
[rfc6570]: https://www.rfc-editor.org/rfc/rfc6570.html
[events-linkifiers]: /api/get-events#realm_linkifiers
Expand Down Expand Up @@ -15205,7 +15228,22 @@ paths:
the usual User dictionary, this does not contain the `is_active`
key, as all the users present in this array have active accounts.

If the current user is a guest whose access to users is limited by a
`can_access_all_users_group` policy, and the event queue was registered
with the `user_list_incomplete` client capability, then users that the
current user cannot access will not be included in this array. If the
current user's access to a user is restricted but the client lacks this
capability, then that inaccessible user will appear in the users array as
an "Unknown user" object with the usual format but placeholder data whose
only variable content is the user ID.

See also `cross_realm_bots` and `realm_non_active_users`.

**Changes**: Before Zulip 8.0 (feature level 232), the
`user_list_incomplete` client capability did not exist, and so all
clients whose access to a new user was prevented by
`can_access_all_users_group` policy would receive a fake "Unknown
user" event for such users.
items:
$ref: "#/components/schemas/User"
realm_non_active_users:
Expand Down
39 changes: 39 additions & 0 deletions zerver/tests/test_events.py
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,7 @@ def verify_action(
user_settings_object: bool = False,
pronouns_field_type_supported: bool = True,
linkifier_url_template: bool = True,
user_list_incomplete: bool = False,
) -> List[Dict[str, Any]]:
"""
Make sure we have a clean slate of client descriptors for these tests.
Expand Down Expand Up @@ -316,6 +317,7 @@ def verify_action(
user_settings_object=user_settings_object,
pronouns_field_type_supported=pronouns_field_type_supported,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)
)

Expand All @@ -331,6 +333,7 @@ def verify_action(
include_streams=include_streams,
pronouns_field_type_supported=pronouns_field_type_supported,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)

# We want even those `send_event` calls which have been hooked to
Expand Down Expand Up @@ -362,6 +365,7 @@ def verify_action(
slim_presence=slim_presence,
include_subscribers=include_subscribers,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)
post_process_state(self.user_profile, hybrid_state, notification_settings_null)
after = orjson.dumps(hybrid_state)
Expand Down Expand Up @@ -389,6 +393,7 @@ def verify_action(
include_streams=include_streams,
pronouns_field_type_supported=pronouns_field_type_supported,
linkifier_url_template=linkifier_url_template,
user_list_incomplete=user_list_incomplete,
)
post_process_state(self.user_profile, normal_state, notification_settings_null)
self.match_states(hybrid_state, normal_state, events)
Expand Down Expand Up @@ -1562,6 +1567,15 @@ def test_register_events_for_restricted_users(self) -> None:
check_user_group_add_members("events[1]", events[1])
check_user_group_add_members("events[2]", events[2])

events = self.verify_action(
lambda: self.register("alice@zulip.com", "alice"),
num_events=2,
user_list_incomplete=True,
)

check_user_group_add_members("events[0]", events[0])
check_user_group_add_members("events[1]", events[1])

def test_alert_words_events(self) -> None:
events = self.verify_action(lambda: do_add_alert_words(self.user_profile, ["alert_word"]))
check_alert_words("events[0]", events[0])
Expand Down Expand Up @@ -4229,6 +4243,18 @@ def test_user_access_events_on_changing_subscriptions(self) -> None:
check_realm_user_remove("events[1]", events[1])
self.assertEqual(events[1]["person"]["user_id"], othello.id)

# Check the state change works correctly when user_list_complete
# is set to True.
self.subscribe(othello, "test_stream1")
unsubscribe_action = lambda: bulk_remove_subscriptions(
realm, [othello], [stream], acting_user=None
)
events = self.verify_action(unsubscribe_action, num_events=2, user_list_incomplete=True)
check_subscription_peer_remove("events[0]", events[0])
self.assertEqual(set(events[0]["user_ids"]), {othello.id})
check_realm_user_remove("events[1]", events[1])
self.assertEqual(events[1]["person"]["user_id"], othello.id)

def test_user_access_events_on_changing_subscriptions_for_guests(self) -> None:
self.set_up_db_for_testing_user_access()
polonius = self.example_user("polonius")
Expand All @@ -4254,6 +4280,19 @@ def test_user_access_events_on_changing_subscriptions_for_guests(self) -> None:
check_realm_user_remove("events[2]", events[2])
self.assertEqual(events[2]["person"]["user_id"], othello.id)

# Check the state change works correctly when user_list_complete
# is set to True.
stream = self.subscribe(self.example_user("othello"), "new_stream")
self.subscribe(polonius, "new_stream")
unsubscribe_action = lambda: bulk_remove_subscriptions(
realm, [polonius], [stream], acting_user=None
)
events = self.verify_action(unsubscribe_action, num_events=3, user_list_incomplete=True)
check_subscription_remove("events[0]", events[0])
check_stream_delete("events[1]", events[1])
check_realm_user_remove("events[2]", events[2])
self.assertEqual(events[2]["person"]["user_id"], othello.id)


class DraftActionTest(BaseAction):
def do_enable_drafts_synchronization(self, user_profile: UserProfile) -> None:
Expand Down
2 changes: 2 additions & 0 deletions zerver/tornado/django_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ def request_event_queue(
user_settings_object: bool = False,
pronouns_field_type_supported: bool = True,
linkifier_url_template: bool = False,
user_list_incomplete: bool = False,
) -> Optional[str]:
if not settings.USING_TORNADO:
return None
Expand All @@ -110,6 +111,7 @@ def request_event_queue(
"user_settings_object": orjson.dumps(user_settings_object),
"pronouns_field_type_supported": orjson.dumps(pronouns_field_type_supported),
"linkifier_url_template": orjson.dumps(linkifier_url_template),
"user_list_incomplete": orjson.dumps(user_list_incomplete),
}

if event_types is not None:
Expand Down
17 changes: 17 additions & 0 deletions zerver/tornado/event_queue.py
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,7 @@ def __init__(
user_settings_object: bool = False,
pronouns_field_type_supported: bool = True,
linkifier_url_template: bool = False,
user_list_incomplete: bool = False,
) -> None:
# TODO: We eventually want to upstream this code to the caller, but
# serialization concerns make it a bit difficult.
Expand Down Expand Up @@ -127,6 +128,7 @@ def __init__(
self.user_settings_object = user_settings_object
self.pronouns_field_type_supported = pronouns_field_type_supported
self.linkifier_url_template = linkifier_url_template
self.user_list_incomplete = user_list_incomplete

# Default for lifespan_secs is DEFAULT_EVENT_QUEUE_TIMEOUT_SECS;
# but users can set it as high as MAX_QUEUE_TIMEOUT_SECS.
Expand Down Expand Up @@ -156,6 +158,7 @@ def to_dict(self) -> Dict[str, Any]:
user_settings_object=self.user_settings_object,
pronouns_field_type_supported=self.pronouns_field_type_supported,
linkifier_url_template=self.linkifier_url_template,
user_list_incomplete=self.user_list_incomplete,
)

@override
Expand Down Expand Up @@ -191,6 +194,7 @@ def from_dict(cls, d: MutableMapping[str, Any]) -> "ClientDescriptor":
d.get("user_settings_object", False),
d.get("pronouns_field_type_supported", True),
d.get("linkifier_url_template", False),
d.get("user_list_incomplete", False),
)
ret.last_connection_time = d["last_connection_time"]
return ret
Expand Down Expand Up @@ -1394,6 +1398,17 @@ def process_custom_profile_fields_event(event: Mapping[str, Any], users: Iterabl
client.add_event(event)


def process_realm_user_add_event(event: Mapping[str, Any], users: Iterable[int]) -> None:
user_add_event = dict(event)
event_for_inaccessible_user = user_add_event.pop("inaccessible_user", False)
for user_profile_id in users:
for client in get_client_descriptors_for_user(user_profile_id):
if client.accepts_event(user_add_event):
if event_for_inaccessible_user and client.user_list_incomplete:
continue
client.add_event(user_add_event)


def maybe_enqueue_notifications_for_message_update(
user_notifications_data: UserMessageNotificationsData,
message_id: int,
Expand Down Expand Up @@ -1534,6 +1549,8 @@ def process_notification(notice: Mapping[str, Any]) -> None:
process_presence_event(event, cast(List[int], users))
elif event["type"] == "custom_profile_fields":
process_custom_profile_fields_event(event, cast(List[int], users))
elif event["type"] == "realm_user" and event["op"] == "add":
process_realm_user_add_event(event, cast(List[int], users))
elif event["type"] == "cleanup_queue":
# cleanup_event_queue may generate this event to forward cleanup
# requests to the right shard.
Expand Down

0 comments on commit 965869d

Please sign in to comment.