Adding support for transcripts, recording, AI summarization and meeting subscription to channels#377
Conversation
| var response *http.Response | ||
| for retries > 0 { | ||
| var err error | ||
| response, err = http.DefaultClient.Do(request) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in network request
There was a problem hiding this comment.
This is because we are handling the incoming webhook from zoom as something secure, it should be signed if you configured it properly
There was a problem hiding this comment.
Can these CodeQL errors be suppressed? We don't want this on every PR going forward after merge.
| return | ||
| } | ||
| request.Header.Set("Authorization", "Bearer "+webhook.DownloadToken) | ||
| response, err := http.DefaultClient.Do(request) |
Check failure
Code scanning / CodeQL
Uncontrolled data used in network request
There was a problem hiding this comment.
This is because we are handling the incoming webhook from zoom as something secure, it should be signed if you configured it properly
There was a problem hiding this comment.
CodeQL code tracing is 👌
There was a problem hiding this comment.
Oh yes, I check the "show paths" and I was really impressed, It is pure gold.
mickmister
left a comment
There was a problem hiding this comment.
Excellent feature here 🚀
I gave the PR a review and added some comments for discussion. Let me know what you think 👍
| return | ||
| } | ||
| request.Header.Set("Authorization", "Bearer "+webhook.DownloadToken) | ||
| response, err := http.DefaultClient.Do(request) |
There was a problem hiding this comment.
CodeQL code tracing is 👌
| case actionConnect: | ||
| return p.runConnectCommand(user, args) | ||
| case actionSubscribe: | ||
| return p.runSubscribeCommand(user, args, meetingID) |
There was a problem hiding this comment.
I like this a lot 👍 Many Zoom meetings do not start in Mattermost, but now they kind of "do" because we can listen for that event in an intentionally subscribed channel. We do need to take access control into account here though. The person running this command should potentially not have access to that meeting
We'll also need to do some exploratory testing in general with this. e.g. Some conflict of meeting ids between starting a meeting in MM with a personal meeting id and a matching subscribed meeting id
There was a problem hiding this comment.
Yes, probably the ideal solution is to not allow to subscribe personal meetings, and also ensure that you have access to the meeting before you subscribe it to the channel.
There was a problem hiding this comment.
This is done. Now, you can only subscribe to meetings that are not personal meetings, so the only meetings that you know the ID upfront are not ad-hoc meetings, so any meeting created within Mattermost should be a PMI or Ad-hoc created on the fly.
There was a problem hiding this comment.
so the only meetings that you know the ID upfront are not ad-hoc meetings
Sure but what if someone knows the meeting id and not e.g. a password, can they start funneling in recordings of the meetings in their own channel? Is "knowing the meeting id ahead of time" enough of a security measure?
There was a problem hiding this comment.
@wiggin77 This has been on my TODO list for a while but other priorities have taken precedence. I even have a PR draft up from when I had started reviewing the plugin as a whole for better understanding #405.
Maybe with the introduction of Security Partners I can get this wrapped up in the next month or two
|
@mickmister PTAL |
…ng-and-ai-summarization
wiggin77
left a comment
There was a problem hiding this comment.
Leaving this request for change to get this off my review queue until it is ready.
|
@jespino Can you sync this PR with master, this fix some of the failing ci |
…ng-and-ai-summarization
|
@wiggin77 It looks like @enzowritescode is going to start with the security review soon, so feel free to start reviewing the code. |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Removing the security review. Please re-add Product Security if the PR is picked up again. |
This PR include 3 important changes:
Testing
For testing this, you should test the 3 features independently (there is some degree of overlap, but I going to propose independent test cases). Another important thing is that before you test it you need to configure the zoom application in the zoom marketplace with the right webhook events (it has change, so check out the documentation).
I expect all the test proposed here are executed after properly configuring zoom plugin and AI copilot plugin
Test case 1: Subscription
/zoom subscribe [meeting id], where[meeting id]is the ID of the created meeting in the pervious step (without spaces)/zoom unsubscribe [meeting id], with the same meeting id.Test case 2: Recordings, Chats, and Transcriptions (Without AI Copilot enabled)
Test case 3: AI Summarization (With AI Copilot enabled)
Documentation PR: mattermost/docs#7276