Trigger outgoing webhooks from incoming webhooks when enabled. Add co…

[REABMAX]

Summary

Added a config attribute "IncomingWebhooksTriggerOutgoingWebhooks" with default value false. If it is enabled outgoing webhooks can be triggered by incoming webhooks.
Also added a test.

Ticket Link

Fixes #16238
JIRA-Ticket: https://mattermost.atlassian.net/browse/MM-31208

Release Note

It's now possible for outgoing webhooks to be triggered by incoming webhooks with the setting IncomingWebhooksTriggerOutgoingWebhooks which is set to false by default

[mm-cloud-bot]

@REABMAX: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

I understand the commands that are listed here

[mattermod]

Hello @REABMAX,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

[cpoile]

Nice, thanks @REABMAX !

@mickmister Can I double check to make sure we want to do this? It seems like it might lead to an infinite recursion (but that might be a danger with any auto-responding plugin, of course).

[mickmister]

LGTM, thanks for the improvement @REABMAX!! Great job on the implementation and tests 👍

@mickmister Can I double check to make sure we want to do this? It seems like it might lead to an infinite recursion (but that might be a danger with any auto-responding plugin, of course).

@cpoile There is indeed no way to avoid the infinite recursion, but I think that an admin that has this use case would be aware of this, and would not want to set the outgoing webhook to communicate to something that would possibly post back to MM. Having the setting default to false covers any accidental usage of this feature.

[mickmister]

@REABMAX Can we convey that the default value is false in the release note?

[mattermod]

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

[emilyacook]

@DHaussermann Would you mind reviewing this whenever you have time, or assigning the review to someone else?

[DHaussermann]

/update-branch

[mm-cloud-bot]

Creating a new SpinWick test server using Mattermost Cloud.

[mm-cloud-bot]

Enterprise Edition Image not available in the 30 minutes timeframe, checking the Team Edition Image and if available will use that.

[DHaussermann]

@REABMAX sorry for the delay in testing this.
I've taken a look and don't see this working as expected. Maybe you see something I'm doing wrong here?

• Deployed MM-31208_incoming_webhooks_trigger_outgoing_webhooks branch locally
• Created and outgoing webhook pointing to an app which replies when hit by the outgoing webhook
• Posted the trigger word 'pickles' in the channel to ensure outgoing webhook works as expected
• Created an incoming webhook that post to the target channel
• Sent a CURL to the webhook where the post text contains the trigger word
  Expected: Post delivered via incoming webhook with trigger word fires outgoing webhook
  Observed: Post appears in channel with trigger word but outgoing webhook did not fire
  

cc @mickmister maybe I have misunderstood something about this PR?

[mickmister]

@DHaussermann you'll need to enable the feature by setting IncomingWebhooksTriggerOutgoingWebhooks to true. Note that the PR's description says that the default value is true, but this is not the case. @REABMAX do you mind updating the description to say the default is false?

[REABMAX]

Updated the description

[DHaussermann]

Thanks, I had not set the flag in the cofig. I do see this working now.

@aaronrothschild should this flag be configurable in the system console as well?

[mattermod]

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

/cc @jasonblais @jfrerich @emilyacook

[REABMAX]

I will push this thread since there wasn't activity for 27 days now :)

[mickmister]

@aaronrothschild There's a pending question here of "should this flag be configurable in the system console?"

[aaronrothschild]

@aaronrothschild There's a pending question here of "should this flag be configurable in the system console?"

Sorry, can someone explain to me what the purpose of this change is? Why would an admin want to trigger an outgoing webhook after receiving an incoming webhook? I understand yet why we would make this a feature/function for someone to enable.

[mickmister]

Sorry, can someone explain to me what the purpose of this change is? Why would an admin want to trigger an outgoing webhook after receiving an incoming webhook? I understand yet why we would make this a feature/function for someone to enable.

@aaronrothschild I would ask the author of #16238. I assume it has to do with some decoupling of the service that is causing the incoming webhook, and the service that is receiving the outgoing webhook.

@REABMAX It looks like there is a conflict that needs to be solved with the master branch

[mm-cloud-bot]

Test server creation failed. See the logs for more information.

[mickmister]

@aaronrothschild Please take another look at my reply above when you have the chance

[aaronrothschild]

Gotcha, it seems like an esoteric use case, but as long as it's a default off and can be configured, it's OK with me. @mickmister

[mickmister]

@aaronrothschild There is still a pending question here of "should the config setting exist in the system console" or should it just exist in the config file without appearing in the system console? I'm not sure what the usual protocol/convention is for deciding this.

[mickmister]

@aaronrothschild Please see the question above when you have the chance

[aaronrothschild]

@mickmister It's somewhat esoteric IMO, and it's not changing default behavior so it's more an exception setting. I think it should not go into the system console and should be set via the config.file or config.db setting.

[isacikgoz]

Hi @REABMAX, big thanks for your contribution! There hasn't been recent activity on this PR so I've marked it as inactive. Following our inactive contribution process, the PR is now closed due to 30 days or more of inactivity.

However, if you're still interested working on the changes, let us know! Note that we merged multiple repositories into one monorepo, which may require you to resubmit the changes.

If you have any questions, don't hesitate to let us know, we're happy to help.