mattermost / mattermost-plugin-zoom

Zoom plugin for Mattermost :electric_plug:
Apache License 2.0
106 stars 68 forks source link

Implement webhook validation #279

Closed mickmister closed 1 year ago

mickmister commented 2 years ago

Summary

Viewing the second commit in isolation will make reviewing this PR much more straightforward, since I moved around some code in the first commit.

Description copied from https://github.com/mattermost/mattermost-plugin-zoom/issues/278:

Zoom has changed how their webhook setup works, and requires some logic on our end to validate the webhook is setup properly. This involves:

The webhook must be validated before Zoom's interface will allow the admin to save the webhook configuration in Zoom's app setup UI. This does not affect existing setups of the plugin, but it will affect any new setup, and any instance of an admin wanting to update the webhook secret or change the events associated with the webhook configuration.

Setting up the webhook for validation requires us to store a new Secret Token from Zoom's webhook configuration UI. If this value is not set, we simply skip the "signature verification" step when processing webhook events. We technically don't need to do this step, because Zoom has no way of verifying this is occurring on subsequent webhook events, and because we are already using our own secret for the webhook. Either way, it's best to comply with what Zoom is asking for, and it adds another layer of security provided by Zoom.

Ticket Link

Fixes https://github.com/mattermost/mattermost-plugin-zoom/issues/278

codecov[bot] commented 2 years ago

Codecov Report

Patch coverage: 58.38% and project coverage change: +23.51 :tada:

Comparison is base (cb15d74) 0.00% compared to head (47712f6) 23.51%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #279 +/- ## =========================================== + Coverage 0.00% 23.51% +23.51% =========================================== Files 8 9 +1 Lines 1143 1212 +69 =========================================== + Hits 0 285 +285 + Misses 1143 874 -269 - Partials 0 53 +53 ``` | [Impacted Files](https://codecov.io/gh/mattermost/mattermost-plugin-zoom/pull/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost) | Coverage Δ | | |---|---|---| | [server/configuration.go](https://codecov.io/gh/mattermost/mattermost-plugin-zoom/pull/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL2NvbmZpZ3VyYXRpb24uZ28=) | `25.24% <ø> (+25.24%)` | :arrow_up: | | [server/http.go](https://codecov.io/gh/mattermost/mattermost-plugin-zoom/pull/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL2h0dHAuZ28=) | `20.00% <0.00%> (+20.00%)` | :arrow_up: | | [server/webhook.go](https://codecov.io/gh/mattermost/mattermost-plugin-zoom/pull/279?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost#diff-c2VydmVyL3dlYmhvb2suZ28=) | `59.11% <59.11%> (ø)` | | ... and [4 files with indirect coverage changes](https://codecov.io/gh/mattermost/mattermost-plugin-zoom/pull/279/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost) Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=mattermost)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

DHaussermann commented 2 years ago

This seems functional I can see the hook working when It's deployed. However the validation functionality seems to now be removed from the Zoom so I can't test the API change.

Also it's a bit odd that we're keeping the webhook secret in the query string. We would need to be very clear with the documentation about this but it is working 👍

DHaussermann commented 2 years ago

cc @mickmister ☝️

mickmister commented 2 years ago

@DHaussermann I kept the webhook secret in the query string because it is something we have control over and is battle tested in our system. We can treat it as the primary secret check, while adhering to Zoom's new requirements when they take effect. This also was the simplest route for backwards compatibility.

mickmister commented 1 year ago

@DHaussermann Can you please test this for backwards compatibility? Thank you!

catalintomai commented 1 year ago

cc: @DHaussermann , when you have the bandwidth (or have Malik looking into it).

DHaussermann commented 1 year ago

/update-branch

DHaussermann commented 1 year ago

@mickmister I do not see a change in behavior when running this build. Webhook end point validation is still failing in the Zoom Setup wizard.

image

image

I double checked the URL and secret. I'm a bit confused....

  1. I assume the path where webhook is exposed is not changing?
  2. So what path is the Zoom wizard checking and is there a way I can check it manually with the plugin running? If I could confirm the change with CURL or Postman it will help isolate what's happening here.
  3. Probably not important but I get an older version stamp when I build this ☝️
mickmister commented 1 year ago

@DHaussermann I tested this out again and it's working for me. Can we meet to discuss this?