[MM-60227] Allow plugins to ask for desktop sources for screen sharing #3135
Conversation
streamer45
added
2: Dev Review
| if (event.sender.id !== this.mainView?.webContentsId) { | ||
| // For Calls we make an extra check to ensure the event is coming from the expected window (main view). | ||
| // Otherwise we want to allow for other plugins to ask for screen sharing sources. | ||
| if (this.mainView && event.sender.id !== this.mainView.webContentsId) { | ||
| log.warn('handleGetDesktopSources', 'Blocked on wrong webContentsId'); |
There was a problem hiding this comment.
If we're throwing errors here, these logs should be unnecessary as the CriticalErrorHandler should catch and log these.
| // For Calls we make an extra check to ensure the event is coming from the expected window (main view). | ||
| // Otherwise we want to allow for other plugins to ask for screen sharing sources. | ||
| if (this.mainView && event.sender.id !== this.mainView.webContentsId) { |
There was a problem hiding this comment.
Was this something that already worked? I don't see any changes that allow for other windows to perform this function, unless this is just the first part of a change.
@devinbinnie The above is the logic change. It wasn't working before because we'd require the sender of the event (the requester) to be the calls widget window. Now, we only do that if the widget is actually initialized (e.g., the user has joined a call).
There was a problem hiding this comment.
I'd definitely want security to weigh in on this one - since it makes it even easier for the remote code to abuse the getDesktopSources API.
There was a problem hiding this comment.
Well, I am not sure we can have it both ways if the goal is to allow plugins to ask for these. It's not any less secure than what we did for Calls prior to the global widget, but it's certainly something to consider.
| @@ -408,7 +408,7 @@ export class CallsWidgetWindow { | |||
|
|
|||
| if (!await PermissionsManager.doPermissionRequest(view.webContentsId, 'screenShare', {requestingUrl: view.view.server.url.toString(), isMainFrame: false})) { | |||
| log.warn('screen share permissions disallowed', view.webContentsId, view.view.server.url.toString()); | |||
There was a problem hiding this comment.
Still don't need the extra log item here if we're throwing an error.
devinbinnie
added
4: Reviews Complete
|
@streamer45 - Are dev docs updates needed for this PR from your perspective? |
|
@cwarnermm Not at this time. However, ideally, we should have some documentation on how plugins can integrate with our Desktop App, and this sort of functionality would fit in there. That's a larger effort, though, that likely needs to start from the eng side. |
Summary
PR adds support for more than just Calls to ask for desktop screen-sharing sources. Since
CallsWidgetWindowis a singleton, we can easily reuse most of the existing logic without having to make significant changes.While we are here, I am also making the API call properly throw errors so that plugins can handle any permission issue without requiring them to listen to Calls-specific errors.
Note: we still expect the plugin side to implement a selection modal. I am still considering whether we should simplify this process as well, but if so it would be in another PR.
Example
desktop_sources.mp4
Ticket Link
https://mattermost.atlassian.net/browse/MM-60227
Release Note