Microsoft Teams Notification

[kingzacko1]

Notes for Reviewers

this is a new PR from the existing PR #962. The latest commits have code cleanup and address PR feedback from the original PR.

• Created Microsoft Teams Notification Plugin.

• Moved HttpClient request to util package which is a common file

• The commit history must be preserved - please use the rebase-merge or standard merge option instead of squash-merge

• Sync up with the author before merging

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
2 out of 3 committers have signed the CLA.

✅ danotorrey
✅ kingzacko1
❌ S srinidhi

---

S srinidhi seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

[mikedklein]

LGTM. I can't test the functionality as I am not on AD. But the front-end linting fixes all look correct.

[ryan-carroll-graylog]

Looks good other than some commented out code in a couple unit tests. Tested the functionality and everything seems to be working.

[roberto-graylog]

Looks good over all. It seem I missed a class field that could be a local variable (I think) in class TeamsEventNotification. If this class is to be instantiated for every execution, then it may be not be an issue.

[danotorrey]

The webhook URL and custom message defaults do not appear to be shown in the UI for new notifications. Do you know why this seems to be happening? The color appears to show.

[kingzacko1]

The default values for the UI are set in the frontend code. See TeamsNotificationForm.tsx

[danotorrey]

This looks like a simple POJO that maps the Java object to JSON. In most of the Graylog codebase, that is done with AutoValue classes (with annotated fields and create or builder methods) and Jackson and not with constructor-driven POJOs that have built-in serialization logic (see the getJsonString() method).

For consistency with the most of the pre-existing codebase, I suggest that we should use an AutoValue object instead, and post that directly with the OkHttp client and let the conversion to JSON happen implicitly, or manually convert it using Jackson.

Here is an example:
https://github.com/Graylog2/graylog-plugin-enterprise/blob/72b5428c61cffe7c483e2363322222abc1701105/enterprise/src/main/java/org/graylog/plugins/license/LicenseReportPeriodical.java#L122-L143

[danotorrey]

Now I am seeing that this was previously established in the Slack notification.

graylog-plugin-integrations/src/main/java/org/graylog/integrations/notifications/types/SlackMessage.java

Line 33 in ac5718a

public class SlackMessage {

I am fine with merging this PR as-is, but I think this should be refactored in a subsequent PR for consistency if possible. I think it would be best to avoid replicating this pattern, unless there is a reason for it I am not seeing here.

[kingzacko1]

I agree we can merge as is but then work a minor task to refactor this and the SlackMessage POJO to an AutoValue based class instead of the current implementation.

[danotorrey]

@kingzacko1 and I discussed that this fits well in a separate PR after this is merged. It will be easier to review that way.

[ryan-carroll-graylog]

Adding this here as because we're discussing a separate PR involving Slack: Webhook validation for Slack doesn't appear to working. I was able to (accidently) use a Teams webhook url in a Slack Graylog Notification, and receive a message in Microsoft Teams.

Dismissing because I might be out tomorrow.