WIP: Murmur: forward text message to linked channels #3457
Conversation
davidebeatrici
added
server
feature-request
|
After this patch, text messages will not fail if the user sends a message to a channel and the channel has a linked channel on which the user does not have text message permission. I didn't test it, but, is an infinite loop possible when resolving the tree links, if there is a bidirectional channel link? |
From a quick glance at the code, I think that could happen, as there is no check as to which channels have been visited before... |
davidebeatrici
changed the title
There was a problem hiding this comment.
As I see it, if messages are sent to the tree, the immediate links are being accounted for. However if a channel that gets added to q due to being linked to another channel in the tree the message is originally sent to, then that channel won't be checked for additional links.
That is if we have 3 channels: A, B and C and A is linked to B and B is linked to C. Now we send a message to tree so that A receives it. The code checks A's links and finds B and adds B to q. However as I see it, the message will not reach C.
Now this could of course also be a feature / wanted bhavior. Just wanted to point it out.
As to the potential endless loop: I Don't see a chance for this happening as long as this fuction doesn't call itself somehow (which I don't see happening). We might get some duplicates in q though but as soon as we process q to add the contained users to the set, the duplication is removed by using a St of users instead of a list.
|
|
||
| // Forward text message to linked channels | ||
| if (c->qhLinks.count() > 0) { | ||
| foreach(Channel *l, c->qhLinks.keys()) { |
There was a problem hiding this comment.
| foreach(Channel *l, c->qhLinks.keys()) { | |
| // First check if the sender has the permission to write a text-msg to the linked chanel | |
| if (! ChanACL::hasPermission(uSource, l, ChanACL::TextMessage, &acCache)) { | |
| continue; | |
| } | |
| foreach(Channel *l, c->qhLinks.keys()) { |
| // Forward text message to linked channels | ||
| if (c->qhLinks.count() > 0) { | ||
| foreach(Channel *l, c->qhLinks.keys()) { | ||
| q.enqueue(l); |
There was a problem hiding this comment.
| q.enqueue(l); | |
| // Permission is checked later, so we don't have to worry about it here | |
| q.enqueue(l); |
|
@kripton you said that you wanted to work on this feature. I just found this PR again and wanted to make sure you know about it. I think you can pick up where it was left, if you want to :) |
|
@Krzmbrzl: Thanks, I'm aware. Might take some days to think through on how I'd want this to work exactly. But I'll have a look what's here and what the comments are 👍 |
|
Well, I've started working on this. And can we take the "limitations" of the current UI into account when checking permissions? Because this check already leads to unintended rejections if a message would go to a channel (where the sender has the permission to send a message to) PLUS several users (where just one of them is in a channel where the sender does NOT have the permission to send messages to): mumble/src/murmur/Messages.cpp Line 1480 in 681779b |
I don't think so, but I am also not super familiar with the code parts for text messaging...
Yes indeed. The fact that a single rejected message effectively cancels all other (potentially valid) messages is (imo) a shortcoming of the current implementation |
|
Just noticed that I've continued the discussion in the PR instead of #1719 where it actually belongs :-/ I am fully in line that the server should support all cases supported by the protocol, even if the official client cannot trigger some cases. The question when a message is counted as "failed" is pretty complex since we only have "success" or "failure" (permission denied) as responses. Of course we could introduce a third one ("partially failed") but I feel that it might be a bad idea. Sorry if I'm asking so many question but I would like the design to be same so it can be "easily" explained to the users |
In the end it doesn't really matter :)
yes
I thin the problem that we are facing is that we can't tell the user that it failed for multiple targets. Therefore I think we have to make the best out of it and tell the user about the first direct target that failed. The way I imagine this to work is to process all targets, put the ones that aren't allowed into a list/set and when actually sending the messages, ignore targets that the user may not send messages to.
Absolutely no problem. In fact I think it is important to discuss issues like this in order to arrive at the best possible end result 👍 |
|
Mh, didn't have time for this for too long, sorry. So, here we go :) Terms:
I've thought a bit about expanding the
I've drafted the // Sent by the server to the user who just sent a TextMessage to indicate the
// targets the message has (or not) been delivered to.
// If the actor is not allowed to send a message to a user, channel or tree
// that was directly requested as a target in the TextMessage command
// (= primary target), a PermissionDenied message is sent back instead of the
// TextMessageDeliveryReport
message TextMessageDeliveryReport {
// The message sender, identified by its session.
required uint32 actor = 1;
// The UTF-8 encoded message as sent to the other users
// Might differ from the message the sender sent if it was too long or
// contained HTML and the server doesn't allow HTML messages
required string message = 2;
// The primary users (identified by their session) the message has been
// delivered to.
// Basically the same users that the sender requested but we copy the
// information for the convenience of the sender to not need to keep track
repeated uint32 primary_users_delivered = 3;
// The primary channel IDs the message has been delivered to.
// Basically the same channels that the sender requested but we copy the
// information for the convenience of the sender to not need to keep track
repeated uint32 primary_channels_delivered = 4;
// The primary root channel IDs the message has been delivered to,
// Only the roots. For all channels the message went to, see
// secondary_channels_delivered
// Basically the same root channels that the sender requested but we copy
// the information for the convenience of the sender to not need to keep
// track.
repeated uint32 primary_tree_root_delivered = 5;
// The secondary users (identified by their session) the message has been
// delivered to.
// Secondary users are users in:
// * channels OR
// * channels below a tree root OR
// * channels linked to channels
// the message was being sent to OR
// * users that listened to channels the message was being sent to
// Those are all the users that got the message minus the ones listed
// in primary_users_delivered.
repeated uint32 secondary_users_delivered = 6;
// The secondary channel IDs the message has been delivered to.
// This includes channels that were part of a channel tree or channels
// that were linked to ones the message has been targeted at.
// Those are all the channels the message has been delivered to minus the
// ones listed in primary_channels_delivered and primary_tree_root_delivered
repeated uint32 secondary_channels_delivered = 7;
// The secondary channel IDs the message could not be delivered to.
// Those are the channels that the message should be sent to but the
// actor was lacking the required permissions. In those cases, no
// PermissionDenied-Message is being generated but the client can find
// the information here.
repeated uint32 secondary_channels_failed = 8;
}I've thought about adding a Does that make sense? Shall I go ahead implementing it like that, including defining the new message type in the proto and sending it to the client? |
|
Creating a new message gives us more flexibility but we should also keep in mind that this way for older clients, we'll have to fall-back to the current solution as they don't know about the new message.
I think we only need this message in case the delivery failed at some point. If all went fine, I don't see the point in sending it.
Nah, they're great. This way one does not need any implicit knowledge to understand the message's purpose 👍 The message itself seems to contain way more information than it actually needs to though. 👀
We should probably figure this out first, so that we can then tune the report to contain just the necessary information. No need to send unneeded info back and forth. That only uses up bandwidth ☝️ |
Code by @trustable-code: https://gist.github.com/trustable-code/3030df0d2589c6c800ee
Fixes #1719