Stop re-adding displayname/avatar fields to leave/ban state events

[gingershaped]

This PR removes a snippet of code that quietly re-added displayname and avatar_url fields to the cached version of leave and ban m.room.member state events stored in RoomMember objects. This code caused the cached member event to be inconsistent with the actual member event returned by other functions, and also caused the (potentially abusive) avatars and displaynames of banned users to appear in clients which call functions that access the cached data.

Checklist

• Tests written for new code (and old code if feasible).
• New or updated public/exported symbols have accurate TSDoc documentation.
• Linter and other CI checks pass.
• Sign-off given on the changes (see CONTRIBUTING.md).

[dbkr]

The comment fairly strongly suggests that it does it for a reason though… will this cause, eg. all messages from people who've left to appear with no display name / avatar? Or is this fine because that's kept in the sentinel events? Do all clients actually use them reliably?

Also, I know you've not written any new code here but some tests could go a long way to help set out what we think should happen, eg: "member event for left user should not contain display name", "member event on historical message for left user should contain display name" etc.

[gingershaped]

The comment fairly strongly suggests that it does it for a reason though… will this cause, eg. all messages from people who've left to appear with no display name / avatar? Or is this fine because that's kept in the sentinel events? Do all clients actually use them reliably?

I suspect that this code was originally added for the convenience of client developers, so they wouldn't have to add extra logic when looking up the display names and avatars of users which are no longer in the room. However, it also removed the client's ability to choose if those users should have their display names and avatars visible. I believe that deciding if and when to show that data should be the responsibility of the client, not the SDK, because having the SDK silently add the fields back in can cause surprising behavior. In fact, I opened this PR after discovering that Cinny showed the abusive avatar of a banned troll even though the troll's join event was redacted.

[gingershaped]

To be more specific, here's the scenario I observed:

1. troll joins and sends spam messages
2. room admin redacts the messages, bans the troll, and redacts the troll's join event
3. Cinny shows the redacted messages with the troll's abusive avatar, which it reads from the prev_content field of the m.room.member state event that banned the troll

matrix-js-sdk's getMxcAvatarUrl function appears to be intended to return the latest avatar a user has in a room. (As far as I can tell, the SDK does not implement the algorithm suggested in 10.2.2.4 for displaying historical display names and avatars.) The current behavior returns the last avatar the user had set in the room, which is different from 10.2.2.4's suggested algorithm because it ignores redactions to older state events. When a user leaves or is banned from a room, they no longer have a latest avatar, so this function should return undefined, which is the behavior this PR implements and I believe to be closer to what the spec recommends.

It might be desirable in the future for matrix-js-sdk to include a way to return historical avatars and display names in a way that respects redactions, as described in 10.2.2.4.

[dbkr]

The js-sdk's current behaviour does seem a bit dumb here. While returning no avatar / display name for left users might be argued to be technically correct, if it's going to cause clients to suddenly start displaying left users with no display name or avatar, I don't think this is a thing we can just hit merge on. It would at least be a breaking change. If possible, fixing getMxcAvatarUrl's current behaviour to respect redactions might be preferable?

[gingershaped]

The js-sdk's current behaviour does seem a bit dumb here. While returning no avatar / display name for left users might be argued to be technically correct, if it's going to cause clients to suddenly start displaying left users with no display name or avatar, I don't think this is a thing we can just hit merge on. It would at least be a breaking change. If possible, fixing getMxcAvatarUrl's current behaviour to respect redactions might be preferable?

As written this is currently a breaking change, yeah. I'll investigate fixing getMxcAvatarUrl.