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.

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

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

Merged
merged 2 commits into from Dec 7, 2020

Conversation

MarshallOfSound
Copy link
Member

@MarshallOfSound MarshallOfSound commented Aug 22, 2019

Bringing back #14544

Fixes #14544

Notes: Additional permission checks are now routed through session.setPermissionCheckHandler. These include Notification.permission, and permission.query. Please note that the webContents parameter to the check handler can now be null.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Aug 22, 2019
@MarshallOfSound MarshallOfSound added the semver/major incompatible API changes label Aug 22, 2019
@MarshallOfSound
Copy link
Member Author

This is tagged as semver/major because webContents can be null in the permission check handler now. This can't be deprecated, but can only go into a major release (8.0.0)

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Aug 23, 2019
@zcbenz zcbenz added the wip ⚒ label Jan 20, 2020
@nornagon
Copy link
Member

Could we perhaps make this not a breaking change by having a default implementation of PermissionCheckHandler which returns whatever the last return value from the PermissionRequestHandler was?

i.e.

session.setPermissionRequestHandler((webContents, permission, callback) => {
  if (can(webContents, permission)) callback(true)
})

// => subsequent permission checks return `true` by default
// installing your own PermissionCheckHandler overrides this behavior

Does Chromium ever call the check before the request..?

Another way to handle this API would be something like:

session.setPermissionRequestHandler((webContents, permission, callback) => {
  webContents.grantPermission(permission)
  callback(true)
})

// => the permission has been set to 'enabled'. At any future time `revokePermission` can be called to revoke permission. `grantPermission` can also be called proactively.

@MarshallOfSound
Copy link
Member Author

@nornagon This isn't a breaking change because of that. The only reason this is marked as semver/major is because the webContents parameter to an existing event can now be null.

@nornagon
Copy link
Member

@nornagon This isn't a breaking change because of that. The only reason this is marked as semver/major is because the webContents parameter to an existing event can now be null.

I'm pretty sure requiring a new permission handler for e.g. web notifications or fullscreen is also a breaking change.

@MarshallOfSound
Copy link
Member Author

@nornagon it doesn't require a new permission handler, the handler defaults to granted so it's not a breaking change

@nikitakot
Copy link
Contributor

@nornagon @MarshallOfSound Hi there, what is the status of this PR? When it can be ready? Also will it be available in electron 8 and 9? Thank you.

@nornagon
Copy link
Member

@nikitakot as you can see from the comments in this PR:

  1. It's old, and hasn't been recently worked on.
  2. It's a breaking change.

We do not merge breaking changes to old branches. The soonest this could possibly be available is Electron 12, as Electron 11 is a quiet release.

@MarshallOfSound MarshallOfSound added semver/major incompatible API changes and removed semver/major incompatible API changes labels Dec 7, 2020
@MarshallOfSound MarshallOfSound changed the title [WIP] feat: route frame based permission checks through our permission check handler feat: route frame based permission checks through our permission check handler Dec 7, 2020
@MarshallOfSound MarshallOfSound added semver/major incompatible API changes and removed semver/major incompatible API changes labels Dec 7, 2020
@MarshallOfSound MarshallOfSound merged commit 771e34a into master Dec 7, 2020
@release-clerk
Copy link

release-clerk bot commented Dec 7, 2020

Release Notes Persisted

Additional permission checks are now routed through session.setPermissionCheckHandler. These include Notification.permission, and permission.query. Please note that the webContents parameter to the check handler can now be null.

@MarshallOfSound MarshallOfSound deleted the expand-check-scope branch December 7, 2020 23:45
@ChALkeR
Copy link
Contributor

ChALkeR commented Aug 12, 2021

@MarshallOfSound This PR (commit 771e34a) introduced an undocumented window-placement event in permission check handler:

To reproduce:

const { app, BrowserWindow } = require('electron')

app.whenReady().then(() => {
  const win = new BrowserWindow({ width: 800, height: 600 })

  const ses = win.webContents.session
  console.log(ses.getUserAgent())
  ses.setPermissionRequestHandler((...args) => console.log('request', args))
  ses.setPermissionCheckHandler((...args) => console.log('check', args))

  win.loadURL('about:blank')
})

To trigger the event do any screen configuration changes: e.g. connect or disconnect an external display, or change screen rotation or resolution, or just turn the monitor off and on again. Even that might not be needed, for some setups this just happens on launch apparently.

This still happens even if --disable-blink-features=WindowPlacement is added to the options.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[C67] Review synchronous permission check implementation
5 participants