Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: route frame based permission checks through our permission check handler #14569

Closed
wants to merge 1 commit into from
Closed

feat: route frame based permission checks through our permission check handler #14569

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 7 additions & 2 deletions atom/browser/atom_permission_manager.cc
View file
Open in desktop
Expand Up @@ -214,6 +214,7 @@ blink::mojom::PermissionStatus AtomPermissionManager::GetPermissionStatus(
content::PermissionType permission,
const GURL& requesting_origin,
const GURL& embedding_origin) {
// TODO(MarshallOfSound): Investigate how we can emit this permission check on a session object
return blink::mojom::PermissionStatus::GRANTED;
}

Expand Down Expand Up @@ -245,9 +246,13 @@ bool AtomPermissionManager::CheckPermissionWithDetails(
blink::mojom::PermissionStatus
AtomPermissionManager::GetPermissionStatusForFrame(
content::PermissionType permission,
content::RenderFrameHost* render_frame_host,
content::RenderFrameHost* rfh,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👎
Abbreviations are evil.

const GURL& requesting_origin) {
return blink::mojom::PermissionStatus::GRANTED;
base::DictionaryValue details;
bool granted = CheckPermissionWithDetails(permission, rfh, requesting_origin,
&details);
return granted ? blink::mojom::PermissionStatus::GRANTED
: blink::mojom::PermissionStatus::DENIED;
}

} // namespace atom
8 changes: 6 additions & 2 deletions docs/api/session.md
View file
Open in desktop
Expand Up @@ -315,17 +315,21 @@ session.fromPartition('some-partition').setPermissionRequestHandler((webContents

* `handler` Function<Boolean> | null
* `webContents` [WebContents](web-contents.md) - WebContents checking the permission.
* `permission` String - Enum of 'media'.
* `permission` String - Enum of 'media', 'midiSysex', 'notifications', 'geolocation', 'mediaKeySystem' or 'midi'.
* `requestingOrigin` String - The origin URL of the permission check
* `details` Object - Some properties are only available on certain permission types.
* `securityOrigin` String - The security orign of the `media` check.
* `mediaType` String - The type of media access being requested, can be `video`,
`audio` or `unknown`
`audio` or `unknown`. This property is only set on `media` checks.

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)`.

Please note not all syncronous permission checks are passed through this handler,
for instance `Notification.granted` is not routed but `new Notification` will cause
the `permissionRequestHandler` to be fired.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is confusing. Is returning false from this function enough to prevent usage of the feature? If not, what else do I have to do?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nornagon In some cases returning false here will block usage of the feature, in other cases you'll have to return false here and block it in the request handler as well

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a developer, that's totally useless information to me unless the docs specifically describe what this will prevent.

I understand if the intention is that ultimately, returning false from this function will block the usage of that feature the same as if the user had clicked 'block' in the browser, but it's not currently true because we haven't finished implementing the feature. If that's the case, then we should mark this function experimental.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nornagon I'm not sure how to phrase this but basically there are 3 types of checks coming through / not coming through this handler now

  1. A check coming through the handler that results in a feature being enabled / disabled (see the media permission)
  2. A check coming through the handler that results in a feature "appearing" to be enabled / disabled (Notification.granted) but require the request handler to actually allow / deny the request. I.e. Denying the check but allowing the request would make the feature active.
  3. Checks not coming through the handler right now (notification checks aren't router here yet)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will block the usage of that feature the same as if the user had clicked 'block' in the browser

That functionality comes from setPermissionRequestHandler, it's asyncronous

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By using "for instance" in docs here, it is not clear which checks will come through the handler and which will not.

I think we should list all checks that will not come through the handler, and mark that type of check as experimental.


```javascript
const {session} = require('electron')
session.fromPartition('some-partition').setPermissionCheckHandler((webContents, permission) => {
Expand Down