feat: add session.setPermissionCheckHandler #13925
Conversation
|
Some permission checks aren't a simple
This API probably needs to handle those three types not just a boolean yes/no. |
| base::DictionaryValue details; | ||
| details.SetString("securityOrigin", security_origin.spec()); | ||
| // The permission type doesn't matter here, AUDIO_CAPTURE/VIDEO_CAPTURE | ||
| // are presented as same type in content_converter.h. |
There was a problem hiding this comment.
Perhaps we should fix that? It seems relevant to know whether the webcontents is requesting audio or video.
There was a problem hiding this comment.
Currently they both get converted to media, I agree that the user should be able to tell the different between all the content::MediaStreamType's. We could just set the media stream type on the details object 🤔
I did it this way to be consistent with the existing media permission stuff in setPermissionRequestHandler
There was a problem hiding this comment.
ah right, well we should fix that, but perhaps in a separate PR :)
docs/api/session.md
Outdated
|
|
||
| Sets the handler which can be used to respond to permission checks for the `session`. | ||
| Returning `true` will allow the permission and `false` will reject it. | ||
| To clear the handler, call `setPermissionCheckHandler(null)`. |
There was a problem hiding this comment.
Perhaps it would be good to add a note that this blocks the main thread & so should return quickly?
| @@ -23,6 +23,7 @@ class WebContentsPermissionHelper | |||
| OPEN_EXTERNAL, | |||
| }; | |||
|
|
|||
| // Asyncronous Requests | |||
| @@ -102,4 +113,26 @@ void WebContentsPermissionHelper::RequestOpenExternalPermission( | |||
| callback, user_gesture, &details); | |||
| } | |||
|
|
|||
| std::string MediaStreamTypeToString(content::MediaStreamType type) { | |||
There was a problem hiding this comment.
This function should be in an anonymous namespace
| bool CheckPermissionWithDetails(content::PermissionType permission, | ||
| content::RenderFrameHost* render_frame_host, | ||
| const GURL& requesting_origin, | ||
| const base::DictionaryValue* details); |
| @@ -43,6 +48,9 @@ class WebContentsPermissionHelper | |||
| bool user_gesture = false, | |||
| const base::DictionaryValue* details = nullptr); | |||
|
|
|||
| bool CheckPermission(content::PermissionType permission, | |||
| const base::DictionaryValue* details); | |||
| @@ -34,6 +35,10 @@ class WebContentsPermissionHelper | |||
| bool user_gesture, | |||
| const GURL& url); | |||
|
|
|||
| // Syncronous Checks | |||
| @@ -34,6 +35,10 @@ class WebContentsPermissionHelper | |||
| bool user_gesture, | |||
| const GURL& url); | |||
|
|
|||
| // Syncronous Checks | |||
| bool CheckMediaAccessPermission(const GURL& security_origin, | |||
| content::MediaStreamType type); | |||
| * `webContents` [WebContents](web-contents.md) - WebContents checking the permission. | ||
| * `permission` String - Enum of 'media'. | ||
| * `requestingOrigin` String - The origin URL of the permission check | ||
| * `details` Object - Some properties are only available on certain permission types. |
There was a problem hiding this comment.
the code seems to unconditionally set both properties. Which ones aren't available on certain permission types?
There was a problem hiding this comment.
@ckerr This is future documentation so that when we add new permission types (currently this method only has one) it doesn't catch people by surprise 👍
|
Release Notes Persisted
|
This allows us to call out to JS land for synchronous permission checks (for example the media permission check)
Example usage:
Notes: Added session.setPermissionCheckHandler()