Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add message-forwarding functionality #1732

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

worlword
Copy link
Contributor

Adds a new action button to messages in the chat-history for message-
forwarding. The Button opens a modal dialog where the user can enter
a destination for the message. The destination can be a user in the
roster or a MUC that is currently opened.

So another pull-request XD

The following screenshot shows how forwarded messages are displayed in chat (I just saw that the text is in german but it should be understandable).
message_forwarding

The user can forward any message in the chat-history. During mouse-over a new button will be displayed. When the user clicks on the button the following dialog will be displayed.
message_forwarding_modal

@lgtm-com
Copy link

lgtm-com bot commented Sep 25, 2019

This pull request introduces 2 alerts and fixes 1 when merging 211cdbc into 44e5b4c - view on LGTM.com

new alerts:

  • 1 for Identical operands
  • 1 for Unused variable, import, function or class

fixed alerts:

  • 1 for Unused variable, import, function or class

@worlword worlword force-pushed the pullRequest_MessageForwarding branch from 211cdbc to 6959b2b Compare September 26, 2019 07:04
@lgtm-com
Copy link

lgtm-com bot commented Sep 26, 2019

This pull request fixes 1 alert when merging 6959b2b into 44e5b4c - view on LGTM.com

fixed alerts:

  • 1 for Unused variable, import, function or class

@jcbrand
Copy link
Member

jcbrand commented Sep 26, 2019

This is great, thanks @worlword! Can you please look at the two issues the LGTM bot raised?

I'll look into this ASAP.

@worlword
Copy link
Contributor Author

hey,

i already updated :) Everything should be fixed.

stanza,
'Forwarded messages not part of an encapsulating protocol are not supported'
);
}
Copy link
Member

@jcbrand jcbrand Oct 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is dangerous, because if the forward-message plugin is not loaded, then removing this code causes a security vulnerability which allows malicious users to spoof messages as if they're coming from someone else.

I'm not sure currently what the right way is to go about adding this feature. One option is to make message forwarding a more "core" feature such as message corrections and spoilers, meaning that it doesn't go into its own plugin but instead goes into converse-chatboxes and converse-chatview. However I'm not yet convinced that this is the right way.

I've been thinking about using JXT from @legastero to convert incoming XML stanzas to JSON. I think if we did that, we could let the forward-message plugin register a JXT definition for message forwarding. That way, if the plugin is not loaded, forwarded messages are dropped because they can't be parsed and we avoid this problem. That's at least the theory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. I've been thinking about this. I haven't tested this, but would it be possible to register our own strophe.js-handler (http://strophe.im/strophejs/doc/1.1.3/files/strophe-js.html#Strophe.Connection.addHandler) inside the plugin? That way the handler should not be registered if the plugin is missing and forwarded messages should be dropped again. When the plugin is active, the handler takes the stanza and calls the new functions. If it is not a forwarded message it will be dropped (I don't know if strophe.js allows to register a handler just for XEP-0297-stanzas).
I can't test this right now, but I think this could be a solution without relying on JXT. What do you think?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like you can register a handler only for forwarded messages, however you could register a more broader handler and then inside the handler check if the message is a forwarded one.

@worlword worlword force-pushed the pullRequest_MessageForwarding branch 2 times, most recently from 0ad6151 to bc18be1 Compare November 10, 2019 22:24
@worlword
Copy link
Contributor Author

I just updated the pull-request. Sorry for the delay I was busy the past weeks.
I use my own handler now and moved almost all code into converse-message-forwarding. I copied most of your code to handle the incoming stanzas. I wanted to avaid code duplication but the module breaks very quickly when the code inside converse-muc or converse-chat is changed.

I don't know why I get the error in the emoji-test, I hoped it was because of my local configuration... I shouldn't have changed anything there. But I don't get the XEP-0363 errors when I test it locally. I have to look into that tomorrow

Copy link
Member

@jcbrand jcbrand left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks pretty good, but I'll need to look at it in more detail later.

I've noticed a small thing for which I've made a suggestion.

@@ -17,6 +17,7 @@
['rosterGroupsFetched', 'chatBoxesFetched'], {},
async function (done, _converse) {

_converse.whitelisted_plugins = _converse.whitelisted_plugins.filter(e => e !== 'converse-forward-message');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second parameter of initConverse 3 lines above is a settings map.
So you can pass in {blacklisted_plugins: ['converse-forward-message']} there instead of doing this here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it. Then the test fails again. The problem is, that I check with the following if the plugin is enabled:
if (!_converse.whitelisted_plugins.includes('converse-forward-message'))
So when I add it to the blacklisted plugins it is also a whitelisted plugin. That's why it will not reject the message and the test fails. I'm thinking about also adding || _converse.blacklisted_plugins.includes('converse-forward-message') to the if-statement. I don't really like the solution anyways, but could not find a better way from preventing converse to send an error-message on forwarded-messages, if the plugin is enabled.

I also tried to find out why the tests for xep-0363. For me all tests work and I cannot find anything in the code (by just looking) that jumps out. I updated the pull-Request anyway to the latest commit.

Adds a new action button to messages in the chat-history for message-
forwarding. The Button opens a modal dialog where the user can enter
a destination for the message. The destination can be a user in the
roster or a MUC that is currently opened.
@worlword worlword force-pushed the pullRequest_MessageForwarding branch from bc18be1 to a693fcb Compare November 13, 2019 19:23
@jcbrand jcbrand force-pushed the master branch 2 times, most recently from aa8c887 to d7d810b Compare November 29, 2019 08:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants