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

WIP: feat: a way to grant permissions to webxdcs #4008

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

WofWca
Copy link
Collaborator

@WofWca WofWca commented Jul 4, 2024

Comment on lines 271 to 272
// Or, we should instead dynamically add items here as the app
// makes permission requests (see `permission_handler`).
Copy link
Member

Choose a reason for hiding this comment

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

would rather make the app specify it in its manifest.toml file

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Huh? How would that be useful?

Copy link
Member

Choose a reason for hiding this comment

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

That would allow xdc app stores to show permissions.
And we could also say you need to specify a reason string there that we can show in the permission request dialog.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmmmm idk idk. I think a "website" model is more suitable. Where the website (webxdc in this case) itself would explain why it's going to request a permission, in its UI.

This "manifest" model is more suitable for when there is no way to request permissions dynamically, where you have to assess them prior to installation, but with all permissions defaulting to "denied" I think it's not needed.

@Simon-Laux
Copy link
Member

we could show an electron/native dialog in webxdcWindow.webContents.session.setPermissionRequestHandler when the permission is requested the first time.
Also would make sense to persist webxdc permissions, maybe for now in ui.* config key. like ui.webxdc_permissions.<messageId>. later it would be nice to have a core api for storing permissions per webxdc instance, so that core can automatically delete the permission config when the webxdc is deleted.

@WofWca WofWca force-pushed the wofwca/webxdc-permissions branch from fb0b4be to 557ffc0 Compare July 5, 2024 09:30
@hpk42
Copy link
Contributor

hpk42 commented Jul 5, 2024 via email

@WofWca WofWca force-pushed the wofwca/webxdc-permissions branch 5 times, most recently from e8c06b1 to f8af0b3 Compare July 5, 2024 14:04
@WofWca
Copy link
Collaborator Author

WofWca commented Jul 5, 2024

Pushed a few changes:

  • hid this behind an experimental toggle
  • more possible permissions to grant
  • don't show permissions that were never checked or requested
  • security: check requestingUrl origin just in case
  • adjust log text
  • a refactor of makeMenu

@WofWca WofWca marked this pull request as ready for review September 10, 2024 17:57
@WofWca WofWca changed the title WIP: feat: a way to grant permissions to webxdcs feat: a way to grant permissions to webxdcs Sep 10, 2024
https://www.electronjs.org/docs/latest/api/menu#menugetapplicationmenu :
> Note: The returned Menu instance doesn't support dynamic addition
> or removal of menu items. Instance properties
> can still be dynamically modified.

Also https://stackoverflow.com/questions/47756822/change-electrons-menu-items-status-dynamically/47761652#47761652

I checked and it works
- more possible permissions to grant
- don't show permissions that were never checked or requested
- security: check `requestingUrl` origin just in case
- adjust log text
// TODO some (poorly written?) apps might require a refresh
// after a permission has been granted,
// but we don't support it because
// 1. There is no way to refresh a webxdc
Copy link
Member

Choose a reason for hiding this comment

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

app can call window.location.reload() in it's iframe, you can only not reload the parent page.

@Simon-Laux
Copy link
Member

Simon-Laux commented Sep 10, 2024

What I would like to see in this pr:

  • clipboard read access as toggle-able permission, also loosely related to Make sure WebXDC cannot access the clipboard #3416 (when we find a way to disable the execCommand("paste") method)
  • persist the permissions you have selected in ui config key (probably sth like key=webxdc.permissions.<webxdc_id/msg_id>)
  • ask user automatically when app requests permission in an electron native dialog (because electron native dialog is the least effort) -> only ask one time for each permission, if the user has denied it don't ask again.

permission storage format could be

enum PermissionState {
 GRANTED,
 USER_DENIED, // denied, don't ask again
 UNDEFINED // denied, but ask on first request, not really in that enum, use the normal undefined instead
}

type PermissionStore = {[permission:string]: PermissionState | undefined}

Edit: these 3 states are already common in other web apis, see https://developer.mozilla.org/en-US/docs/Web/API/Notification/permission_static

@Simon-Laux
Copy link
Member

Simon-Laux commented Sep 10, 2024

For other platforms:

I don't think waiting for integrations makes much sense, they come with their own questions and challenges.

The ui does not need to be complicated, we can just start with the following:

  • Experimental setting to turn on the permission system
  • For now only implement the web standard permissions such as camera/microphone/media and clipboard read.
  • UI:
    • in the 3 dot menu on iOS and Android webxdc add a new entry "Permissions" that opens a native dialog with checkboxes that is the ui to change the preferences (on iOS it could also be an action sheet, like how we did the experimental settings there in the past)
    • additionally when the app requests permission for something the first time show an confirmation dialog asking the user to allow / deny it -> only show the dialog if the user has not denied that permission for that app before.
  • Store settings in ui config for now as I described in my last post in this thread.

code implementation:

on both android and electron there is a permission handler function/callback that is called when the website requests a web permission, the handler can then accept or deny the request:

Later we can improve it:

  • define what web permissions the app can request, by manifest, like there needs to exist a key with a reason string, like in iOS.
  • store the permissions somewhere else, like with a new core api?
  • if we do crypto signing we could think about sharing permission settings for a signed app version instead of having it separate for each instance.

@Simon-Laux
Copy link
Member

I rebased it to main (after merging monorepo, I offered to rebase all prs that were made before that)

@r10s
Copy link
Member

r10s commented Sep 12, 2024

sorry to be a killjoy and to repeat things:

this PR and this discussion here is tricky timing-wise.

there is no agreement about the way to proceed with permissions. and the PR does not help on any open, much more important issue - but it adds issues and work on all platforms. distracting development resources from far more important things.

(also, the original idea of a "share location" webxdc seems to be a bit lost on the way - how useful are desktop permissions to make a “share location” thing where most desktop do not even know their locations. this underlines, that it is not just this single PR, but will keep lots of devs busy)

i am not saying that it does not make sense, but i doubt it makes sense now.

i suggest to move this PR to resurrection and reconsider later.

@WofWca
Copy link
Collaborator Author

WofWca commented Sep 12, 2024

Do you think it would be okay to merge if we were to remove the toggle from the UI completely? so that it's only possible to enable for developers.

@r10s
Copy link
Member

r10s commented Sep 13, 2024

i would postpone the pr fully, freeing development resources.

otherwise there will still be discussion, pressure on other os etc., removing focus from other things - let alone bugs :)

developler can still use this PR for experiments

@WofWca WofWca changed the title feat: a way to grant permissions to webxdcs WIP: feat: a way to grant permissions to webxdcs Sep 15, 2024
@WofWca WofWca marked this pull request as draft September 17, 2024 11:04
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.

4 participants