xsf / xeps

Hosts the markup for all XMPP Protocol Extensions.
https://xmpp.org/extensions/
Other
125 stars 119 forks source link

XEP-0444: Add restrictions to reactions #1249

Closed truenicoco closed 1 year ago

truenicoco commented 1 year ago

This is my first attempt at working on a XEP.

It probably is wrong in many ways, and I think it may need to be a little more verbose overall. However, I would be happy to have feedback about:

It solves a problem I have with gateways, but I think this "emoji restrictions" might actually be useful for using reactions for polls. I think it could also make the client "reaction UIs" better, if instead of offering the whole list of emoji, only a few are possible, because to me emoji reactions are mostly useful when they require a single click.

For my gateways, I "solved" the issue by using XEP-0356 (Privileged Entity) to impersonate the reaction-sending user and "clear" the emojis not allowed on another network. This works fine with movim, but is broken in dino due to https://github.com/dino/dino/issues/1296.

CLAassistant commented 1 year ago

CLA assistant check
All committers have signed the CLA.

mar-v-in commented 1 year ago

I don't like the idea of having to fetch the available reactions before doing a reaction. This wouldn't work in case the user is temporarily offline or could cause the user interface to need to wait for network, if available reactions are not prefetched for every user you receive a message from. This would also open a can of worms of caching and invalidating cache after certain time etc.

Not saying the feature is not useful, just thinking there should be a better way to do this, as this is very much only useful for gateways.

The variant where the possible reactions to a message are part of that message is certainly better in this regard, but you wouldn't want to send this with every message and if it's only for specific message, it might be for usecases where I would wonder if the gateway should rather implement XEP-0439 actions instead.

I totally agree though, that sending clients should support the recipient (or any intermediary node) to send an error message to reject/undo the reaction message. The XEP-0356 "solution" IMO is too dirty to keep it the intended way of doing this. The error message could even include some machine-readable error message to instruct which / how many reactions are allowed.

PS: We don't need a new xml namespaces for any of this, it works perfectly well in the already existing namespace.

truenicoco commented 1 year ago

I did not know about XEP-0439, but it looks better suited to the "poll" use case, agreed.

Having to fetch the available reactions before reacting is probably not a good idea, agreed too. However, maybe this could make sense for a MUC though? Possibly through a MUC configuration option, something that is fetched on joining the group, and modifiable by admins/owners? I believe telegram has this sort of room-level restrictions. Even outside of a gateway context, letting admins decide which emojis are available can help build some sense of "community", a "personality" for a specific MUC. Let me know what you think and if I should rework that in the muc#config fashion, or if it's also garbage. :)

I like the idea of the human-readable error message in the error stanza, but maybe it could also contain machine-parsable instructions for the client about how many per message and the subset of authorised emojis? Not all clients would need to implement it, but it would certainly improve user experience if clients can hide unavailable emoji somehow? Also, when only one emoji per message is possible, it changes the "meaning" of the UI, ie, clicking an emoji means "changing my current reaction" instead of "adding a new reaction".

Thanks a lot for your feedback, I'll submit a new version soon(tm).

mar-v-in commented 1 year ago

In general the idea of restricting the possible reactions in a room sounds sensible to me and in the room case, it would also be the room server to enforce this and reply with an error, so the room must be aware of restrictions already and can easily pass it to the client via muc#roomconfig or muc#roominfo. However, I'm unsure if this is something that belongs in the reactions XEP or rather in a more general "room restrictions" XEP, that could also be used for example for:

I'm just throwing in some ideas here, but generally allowing room admins to restrict what can be done in the room sounds like something that makes sense to me (and would probably also come handy for gateways that have to restrict features due to them being unavailable on the other network).

Regarding machine-readable errors: Of course the standard human-readable message should still be there even if we add additional machine-readable information. Maybe that's also not so relevant for now as it adds additional complexity. Specifying what a client should do when receiving an error message for a reactions message (= rollback to the previous reactions state) is definitely something that does belong in this XEP (independent of you discovering a real-world usecase for it), so maybe let's start with adding only that for now, so we're not blocking this while thinking about other features.

horazont commented 1 year ago

@truenicoco Hi & welcome to the XEP process and thanks a lot for your contribution.

Generally we try to avoid to have extensive technical discussions in this tracker, in order to avoid splitting discussions between GitHub and the mailing list.

As a next step, @truenicoco, please post to the mailing list and ask for feedback there, linking to this PR and noting that discussion has already happened there, and @mar-v-in please provide updated feedback (I see that @truenicoco added commits).

I'll lock this thread to avoid more people accidentally piling on technical feedback here.

Thanks!

Kev commented 1 year ago

@mar-v-in I think this needs your ok.

Kev commented 1 year ago

(Ok out of band)