Fetch MUC participant avatars, and display avatars in the ChatView

[lissine0]

Add support for fetching MUC participant avatars, using vcard-temp (XEP-0153)
And display the avatars in the ChatView, and in ChannelMemberList.

Fixes #699

[lissine0]

Here are some points for discussion:

• When first joining a room, mam queries don't work until one of the following two things happens:
  • the avatars finish being fetched
  • 60 seconds have passed. At which point the following error is shown: Failed to query new messages for Group/ Channel (stanzaid) room@conference.domain.tld: remote-server-timeout (No response in 60 seconds).
    Avatar fetching also stops at this point, or slows down significantly.
• Currently, if some participants are present in multiple rooms, their avatars are fetched once for every room.
  It would be nice to optimize this case: before sending the iq, we could check if the avatar hash is known in other rooms. If so, we simply copy the file. otherwise we proceed with sending the iq.
• The avatar file storage on disk does not distinguish between different accounts.
  i.e. if two accounts are in the same room, they share the same avatar files.
  (the avatars are stored in documents directory/muc_participant_avatars/room/nick)
  I think this can be extended to the database as well, by dropping the account_id column from the muc_participants table.

• Minor: there's a comment that says //hashes should never be nil, should we enforce that on the DB side? (by making the column NOT NULL, and setting an empty string as a default value)
• ejabberd has a custom muc configuration form field that lets you disable iqs in the room:

<field var="allow_query_users" type="boolean" label="Allow users to query other users">
    <value>1</value>
</field>

Should we skip fetching avatars if we detect that this config option is set to false? If we don't, then we'll be sending a pointless iq every time we get a presence with a hash in it, which will return an error every time.
But this configuration is rarely used I think.

[tmolitor-stud-tu]

See my answers inline.

• When first joining a room, mam queries don't work until one of the following two things happens:

  • the avatars finish being fetched
  • 60 seconds have passed. At which point the following error is shown: Failed to query new messages for Group/ Channel (stanzaid) room@conference.domain.tld: remote-server-timeout (No response in 60 seconds).
    Avatar fetching also stops at this point, or slows down significantly.

Why do avatar fetches block mam? I don't think this is something we want.

• Currently, if some participants are present in multiple rooms, their avatars are fetched once for every room.
  It would be nice to optimize this case: before sending the iq, we could check if the avatar hash is known in other rooms. If so, we simply copy the file. otherwise we proceed with sending the iq.

Well, since different mucs could have different policies for direct communication, I think that optimization would cause more confusion than do good (its hard to understand for ordinary users, why a muc shows only avatars of some users and not others).

• The avatar file storage on disk does not distinguish between different accounts.
  i.e. if two accounts are in the same room, they share the same avatar files.
  (the avatars are stored in documents directory/muc_participant_avatars/room/nick)
  I think this can be extended to the database as well, by dropping the account_id column from the muc_participants table.

No, it should even be the other way round: we need to differentiate by account. My own affiliation might influence, if I'm able to retrieve the avatar or even see the jid of participants. Since the avatar hash of every participant is stored in the db, using something like this: documents directory/muc_participant_avatars/accountId/roomJid/avatarHash

• Minor: there's a comment that says //hashes should never be nil, should we enforce that on the DB side? (by making the column NOT NULL, and setting an empty string as a default value)

No, no. That simply means that we should return an empty string rather than nil to indicate a missing hash. Having NULL in the database it still beneficial, I think. Only the getAvatarHashForContact:andAccount: should always return a string to ease the code using that return value (e.g. calling isEqualToString: on it etc.).

• ejabberd has a custom muc configuration form field that lets you disable iqs in the room:

<field var="allow_query_users" type="boolean" label="Allow users to query other users">
    <value>1</value>
</field>

Should we skip fetching avatars if we detect that this config option is set to false? If we don't, then we'll be sending a pointless iq every time we get a presence with a hash in it, which will return an error every time. But this configuration is rarely used I think.

Yes, I think that would be beneficial, even if that is rarely used.

[tmolitor-stud-tu]

I did only a quick scroll-through

[tmolitor-stud-tu]

I think we should never use the nick to identify channel (anon muc) users, but always use the occupant-id. And use the jid for groups (non-anon mucs).

[lissine0]

And use the jid for groups (non-anon mucs).

If we're going to use occupant-id as identifier, it can work for both groups and channels.

I think we should never use the nick to identify channel (anon muc) users, but always use the occupant-id.

The problem is, some MUC servers don't support occupant-ids. And this will always remain a possibility, because on Prosody occupant-id support depends on loading the muc_mam module.

So, if we use the occupant-id as identifier, we won't be displaying avatars in rooms without occupant-id support.

Furthermore, in those cases, the cache key for the avatar placeholders can't be based on the occupantID. So it'll have to be based on the nick.

Now, since it's based on the nick, we'll need to pass the nick to the methods reading and writing avatar files (MLImageManager getAvatarForOccupant and MLImageManager setAvatarForOccupant and their callers). At which point, why not use the nick as identifier instead of the occupant-id?

(Moreover, the nick is already needed inMLImageManager getAvatarForOccupant and its callers, in order to generate the eventual placeholder avatar from the first character of the nick)

I have an draft of this PR using occupantIDs. I can push it somewhere if you want to take a look

[lissine0]

We could avoid caching when a room doesn't support occupant-ids, but I'm not very fond of that idea.

[tmolitor-stud-tu]

Yes, we SHOULD avoid caching if the room doesn't support occupant-ids because its a security issue otherwise: someone could take the same nick while you are not joined and still get your avatar image displayed for all Monal users. That will confuse them at best and allow for easy impersonation attacks at worst.

In the long term we should display a status message saying something along the line A new user took the nick 'xyz' to fully prevent such attacks.

As for occupant-ids in general: we already use them to authenticate last message correction and message retraction (using the nick would be insecure as per the impersonation attack described above).

[lissine0]

Yes, we SHOULD avoid caching if the room doesn't support occupant-ids because its a security issue otherwise: someone could take the same nick while you are not joined and still get your avatar image displayed for all Monal users. That will confuse them at best and allow for easy impersonation attacks at worst.

Note that this security issue is not possible, unless the impersonator has the same avatar hash in their presence.
(in MLMucProcessor processStanza:, if we detect that the hash changed, we fetch new avatar; or we just delete the old one if there's no hash in the presence)

[lissine0]

And by the way, when I said:

We could avoid caching when a room doesn't support occupant-ids

I meant the avatar placeholders saved in MLImageManager's NSCache property.
Because if we're going to use occupant-ids as identifiers, avatars won't be displayed anyways in rooms without occupant-id support.

[tmolitor-stud-tu]

Hmmm, well, I get the avatar hash in presence argument. But it still doesn't feel right to use the unstable/spoofable nickname as identifier, especially since occupant-ids are widely supported now.

(Btw: you'd have to sanitize the nickname to not contain characters that allow a directory traversal attack)

[tmolitor-stud-tu]

And yes, I know that we'll show the placeholder avatars for mucs without occupant-ids, but imho that's fine.

[lissine0]

See my answers inline.

• When first joining a room, mam queries don't work until one of the following two things happens:

  • the avatars finish being fetched
  • 60 seconds have passed. At which point the following error is shown: Failed to query new messages for Group/ Channel (stanzaid) room@conference.domain.tld: remote-server-timeout (No response in 60 seconds).
    Avatar fetching also stops at this point, or slows down significantly.

Why do avatar fetches block mam? I don't think this is something we want.

This is a bug, that I was hoping you'd have some insights about. my guess is it's somehow related to the sendQueue having too many iqs, because the mam query seems to be added to the sendQueue after the avatar queries. I'll investigate the issue more.

• Currently, if some participants are present in multiple rooms, their avatars are fetched once for every room.
  It would be nice to optimize this case: before sending the iq, we could check if the avatar hash is known in other rooms. If so, we simply copy the file. otherwise we proceed with sending the iq.

Well, since different mucs could have different policies for direct communication, I think that optimization would cause more confusion than do good (its hard to understand for ordinary users, why a muc shows only avatars of some users and not others).

If by policies for direct communication you mean blocking iqs, my last suggestion already covers this point. we can detect that policy and avoid copying the avatar files for MUCs that block iqs

• The avatar file storage on disk does not distinguish between different accounts.
  i.e. if two accounts are in the same room, they share the same avatar files.
  (the avatars are stored in documents directory/muc_participant_avatars/room/nick)
  I think this can be extended to the database as well, by dropping the account_id column from the muc_participants table.

No, it should even be the other way round: we need to differentiate by account. My own affiliation might influence, if I'm able to retrieve the avatar or even see the jid of participants. Since the avatar hash of every participant is stored in the db, using something like this: documents directory/muc_participant_avatars/accountId/roomJid/avatarHash

I get that my affiliation influences if I can see the jid of participants. But I don't get how it influences if I'm able to retrieve avatars.

[tmolitor-stud-tu]

If by policies for direct communication you mean blocking iqs, my last suggestion already covers this point. we can detect that policy and avoid copying the avatar files for MUCs that block iqs

yes and no. allowing iqs between participants isn't mandated by the spec and that ejabberd exposes the fact that it blocks them is entirely an implementation detail. other servers might block iqs but not expose it as such. especially bridges to other chat networks like biboumi.

--> you can't rely on this field to detect this.

[tmolitor-stud-tu]

I get that my affiliation influences if I can see the jid of participants. But I don't get how it influences if I'm able to retrieve avatars.

Because servers could choose to allow moderators/admins to send iqs to participants, but not ordinary users. Or allow it for members, but not for participants having no affiliation.