Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Commit

Permalink
Handle missing content keys when calculating presentable names. (#9165)
Browse files Browse the repository at this point in the history
Treat the content as untrusted and do not assume it is of
the proper form.
  • Loading branch information
clokep authored Jan 25, 2021
1 parent a7882f9 commit 6f7417c
Show file tree
Hide file tree
Showing 4 changed files with 242 additions and 16 deletions.
1 change: 1 addition & 0 deletions changelog.d/9165.bugfix
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix a long-standing bug where invalid data could cause errors when calculating the presentable room name for push.
26 changes: 11 additions & 15 deletions synapse/push/presentable_names.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import re
from typing import TYPE_CHECKING, Dict, Iterable, Optional

from synapse.api.constants import EventTypes
from synapse.api.constants import EventTypes, Membership
from synapse.events import EventBase
from synapse.types import StateMap

Expand Down Expand Up @@ -63,7 +63,7 @@ async def calculate_room_name(
m_room_name = await store.get_event(
room_state_ids[(EventTypes.Name, "")], allow_none=True
)
if m_room_name and m_room_name.content and m_room_name.content["name"]:
if m_room_name and m_room_name.content and m_room_name.content.get("name"):
return m_room_name.content["name"]

# does it have a canonical alias?
Expand All @@ -74,15 +74,11 @@ async def calculate_room_name(
if (
canon_alias
and canon_alias.content
and canon_alias.content["alias"]
and canon_alias.content.get("alias")
and _looks_like_an_alias(canon_alias.content["alias"])
):
return canon_alias.content["alias"]

# at this point we're going to need to search the state by all state keys
# for an event type, so rearrange the data structure
room_state_bytype_ids = _state_as_two_level_dict(room_state_ids)

if not fallback_to_members:
return None

Expand All @@ -94,7 +90,7 @@ async def calculate_room_name(

if (
my_member_event is not None
and my_member_event.content["membership"] == "invite"
and my_member_event.content.get("membership") == Membership.INVITE
):
if (EventTypes.Member, my_member_event.sender) in room_state_ids:
inviter_member_event = await store.get_event(
Expand All @@ -111,6 +107,10 @@ async def calculate_room_name(
else:
return "Room Invite"

# at this point we're going to need to search the state by all state keys
# for an event type, so rearrange the data structure
room_state_bytype_ids = _state_as_two_level_dict(room_state_ids)

# we're going to have to generate a name based on who's in the room,
# so find out who is in the room that isn't the user.
if EventTypes.Member in room_state_bytype_ids:
Expand All @@ -120,8 +120,8 @@ async def calculate_room_name(
all_members = [
ev
for ev in member_events.values()
if ev.content["membership"] == "join"
or ev.content["membership"] == "invite"
if ev.content.get("membership") == Membership.JOIN
or ev.content.get("membership") == Membership.INVITE
]
# Sort the member events oldest-first so the we name people in the
# order the joined (it should at least be deterministic rather than
Expand Down Expand Up @@ -194,11 +194,7 @@ def descriptor_from_member_events(member_events: Iterable[EventBase]) -> str:


def name_from_member_event(member_event: EventBase) -> str:
if (
member_event.content
and "displayname" in member_event.content
and member_event.content["displayname"]
):
if member_event.content and member_event.content.get("displayname"):
return member_event.content["displayname"]
return member_event.state_key

Expand Down
229 changes: 229 additions & 0 deletions tests/push/test_presentable_names.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,229 @@
# Copyright 2021 The Matrix.org Foundation C.I.C.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.

from typing import Iterable, Optional, Tuple

from synapse.api.constants import EventTypes, Membership
from synapse.api.room_versions import RoomVersions
from synapse.events import FrozenEvent
from synapse.push.presentable_names import calculate_room_name
from synapse.types import StateKey, StateMap

from tests import unittest


class MockDataStore:
"""
A fake data store which stores a mapping of state key to event content.
(I.e. the state key is used as the event ID.)
"""

def __init__(self, events: Iterable[Tuple[StateKey, dict]]):
"""
Args:
events: A state map to event contents.
"""
self._events = {}

for i, (event_id, content) in enumerate(events):
self._events[event_id] = FrozenEvent(
{
"event_id": "$event_id",
"type": event_id[0],
"sender": "@user:test",
"state_key": event_id[1],
"room_id": "#room:test",
"content": content,
"origin_server_ts": i,
},
RoomVersions.V1,
)

async def get_event(
self, event_id: StateKey, allow_none: bool = False
) -> Optional[FrozenEvent]:
assert allow_none, "Mock not configured for allow_none = False"

return self._events.get(event_id)

async def get_events(self, event_ids: Iterable[StateKey]):
# This is cheating since it just returns all events.
return self._events


class PresentableNamesTestCase(unittest.HomeserverTestCase):
USER_ID = "@test:test"
OTHER_USER_ID = "@user:test"

def _calculate_room_name(
self,
events: StateMap[dict],
user_id: str = "",
fallback_to_members: bool = True,
fallback_to_single_member: bool = True,
):
# This isn't 100% accurate, but works with MockDataStore.
room_state_ids = {k[0]: k[0] for k in events}

return self.get_success(
calculate_room_name(
MockDataStore(events),
room_state_ids,
user_id or self.USER_ID,
fallback_to_members,
fallback_to_single_member,
)
)

def test_name(self):
"""A room name event should be used."""
events = [
((EventTypes.Name, ""), {"name": "test-name"}),
]
self.assertEqual("test-name", self._calculate_room_name(events))

# Check if the event content has garbage.
events = [((EventTypes.Name, ""), {"foo": 1})]
self.assertEqual("Empty Room", self._calculate_room_name(events))

events = [((EventTypes.Name, ""), {"name": 1})]
self.assertEqual(1, self._calculate_room_name(events))

def test_canonical_alias(self):
"""An canonical alias should be used."""
events = [
((EventTypes.CanonicalAlias, ""), {"alias": "#test-name:test"}),
]
self.assertEqual("#test-name:test", self._calculate_room_name(events))

# Check if the event content has garbage.
events = [((EventTypes.CanonicalAlias, ""), {"foo": 1})]
self.assertEqual("Empty Room", self._calculate_room_name(events))

events = [((EventTypes.CanonicalAlias, ""), {"alias": "test-name"})]
self.assertEqual("Empty Room", self._calculate_room_name(events))

def test_invite(self):
"""An invite has special behaviour."""
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.INVITE}),
((EventTypes.Member, self.OTHER_USER_ID), {"displayname": "Other User"}),
]
self.assertEqual("Invite from Other User", self._calculate_room_name(events))
self.assertIsNone(
self._calculate_room_name(events, fallback_to_single_member=False)
)
# Ensure this logic is skipped if we don't fallback to members.
self.assertIsNone(self._calculate_room_name(events, fallback_to_members=False))

# Check if the event content has garbage.
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.INVITE}),
((EventTypes.Member, self.OTHER_USER_ID), {"foo": 1}),
]
self.assertEqual("Invite from @user:test", self._calculate_room_name(events))

# No member event for sender.
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.INVITE}),
]
self.assertEqual("Room Invite", self._calculate_room_name(events))

def test_no_members(self):
"""Behaviour of an empty room."""
events = []
self.assertEqual("Empty Room", self._calculate_room_name(events))

# Note that events with invalid (or missing) membership are ignored.
events = [
((EventTypes.Member, self.OTHER_USER_ID), {"foo": 1}),
((EventTypes.Member, "@foo:test"), {"membership": "foo"}),
]
self.assertEqual("Empty Room", self._calculate_room_name(events))

def test_no_other_members(self):
"""Behaviour of a room with no other members in it."""
events = [
(
(EventTypes.Member, self.USER_ID),
{"membership": Membership.JOIN, "displayname": "Me"},
),
]
self.assertEqual("Me", self._calculate_room_name(events))

# Check if the event content has no displayname.
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.JOIN}),
]
self.assertEqual("@test:test", self._calculate_room_name(events))

# 3pid invite, use the other user (who is set as the sender).
events = [
((EventTypes.Member, self.OTHER_USER_ID), {"membership": Membership.JOIN}),
]
self.assertEqual(
"nobody", self._calculate_room_name(events, user_id=self.OTHER_USER_ID)
)

events = [
((EventTypes.Member, self.OTHER_USER_ID), {"membership": Membership.JOIN}),
((EventTypes.ThirdPartyInvite, self.OTHER_USER_ID), {}),
]
self.assertEqual(
"Inviting email address",
self._calculate_room_name(events, user_id=self.OTHER_USER_ID),
)

def test_one_other_member(self):
"""Behaviour of a room with a single other member."""
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.JOIN}),
(
(EventTypes.Member, self.OTHER_USER_ID),
{"membership": Membership.JOIN, "displayname": "Other User"},
),
]
self.assertEqual("Other User", self._calculate_room_name(events))
self.assertIsNone(
self._calculate_room_name(events, fallback_to_single_member=False)
)

# Check if the event content has no displayname and is an invite.
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.JOIN}),
(
(EventTypes.Member, self.OTHER_USER_ID),
{"membership": Membership.INVITE},
),
]
self.assertEqual("@user:test", self._calculate_room_name(events))

def test_other_members(self):
"""Behaviour of a room with multiple other members."""
# Two other members.
events = [
((EventTypes.Member, self.USER_ID), {"membership": Membership.JOIN}),
(
(EventTypes.Member, self.OTHER_USER_ID),
{"membership": Membership.JOIN, "displayname": "Other User"},
),
((EventTypes.Member, "@foo:test"), {"membership": Membership.JOIN}),
]
self.assertEqual("Other User and @foo:test", self._calculate_room_name(events))

# Three or more other members.
events.append(
((EventTypes.Member, "@fourth:test"), {"membership": Membership.INVITE})
)
self.assertEqual("Other User and 2 others", self._calculate_room_name(events))
2 changes: 1 addition & 1 deletion tests/push/test_push_rule_evaluator.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def _get_evaluator(self, content):
"type": "m.room.history_visibility",
"sender": "@user:test",
"state_key": "",
"room_id": "@room:test",
"room_id": "#room:test",
"content": content,
},
RoomVersions.V1,
Expand Down

0 comments on commit 6f7417c

Please sign in to comment.