Bug 229590 - Add prompt for Permissions API
Summary: Add prompt for Permissions API
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on: 229339
Blocks: 229504
  Show dependency treegraph
 
Reported: 2021-08-26 15:14 PDT by Sihui Liu
Modified: 2021-09-08 00:36 PDT (History)
14 users (show)

See Also:


Attachments
Patch (15.15 KB, patch)
2021-09-02 11:59 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (16.00 KB, patch)
2021-09-03 11:10 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (16.88 KB, patch)
2021-09-03 12:35 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (17.95 KB, patch)
2021-09-05 14:28 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2021-09-05 20:38 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (19.29 KB, patch)
2021-09-05 23:07 PDT, Sihui Liu
sihui_liu: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sihui Liu 2021-08-26 15:14:53 PDT
...
Comment 1 Sihui Liu 2021-09-02 11:59:49 PDT
Created attachment 437178 [details]
Patch
Comment 2 Radar WebKit Bug Importer 2021-09-02 12:00:53 PDT
<rdar://problem/82686948>
Comment 3 youenn fablet 2021-09-03 10:15:29 PDT
Comment on attachment 437178 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437178&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:163
> +- (void)_webView:(WKWebView *)webView requestPermissionForName:(NSString *)permissionName origin:(WKSecurityOrigin *)origin decisionHandler:(void (^)(BOOL granted))decisionHandler WK_API_AVAILABLE(macos(12.0), ios(15.0));

Should we use similar approach as capture delegates:
Something like:
requestPermissionForName:(NSString *)name origin:(WKSecurityOrigin *)origin initiatedByFrame:(WKFrameInfo *)frame decisionHandler:(void (^)(WKPermissionDecision decision))decisionHandler
Comment 4 Sihui Liu 2021-09-03 11:10:18 PDT
Created attachment 437286 [details]
Patch
Comment 5 Sihui Liu 2021-09-03 11:16:43 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 437178 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437178&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:163
> > +- (void)_webView:(WKWebView *)webView requestPermissionForName:(NSString *)permissionName origin:(WKSecurityOrigin *)origin decisionHandler:(void (^)(BOOL granted))decisionHandler WK_API_AVAILABLE(macos(12.0), ios(15.0));
> 
> Should we use similar approach as capture delegates:
> Something like:
> requestPermissionForName:(NSString *)name origin:(WKSecurityOrigin *)origin
> initiatedByFrame:(WKFrameInfo *)frame decisionHandler:(void
> (^)(WKPermissionDecision decision))decisionHandler

Updated!
Comment 6 Chris Dumez 2021-09-03 11:24:36 PDT
Comment on attachment 437286 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437286&action=review

> Source/WebCore/Modules/permissions/PermissionName.h:49
> +inline ASCIILiteral permissionNameToString(PermissionName name)

What are these strings used for? Are they meant to be displayed? If so, I don't think things like "BackgroundFetch" or "DisplayCapture" is displayable.

If they are no meant to be displayed, then we are we using strings and not an enum?

> Source/WebCore/Modules/permissions/PermissionName.h:73
> +        return "Nfc"_s;

Should probably be "NFC".

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:163
> +- (void)_webView:(WKWebView *)webView requestPermissionForName:(NSString *)permissionName origin:(WKSecurityOrigin *)origin topOrigin:(WKSecurityOrigin *)topOrigin decisionHandler:(void (^)(WKPermissionDecision decision))decisionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

Why are we using a NSString* instead of an enum type for the permission name?

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1702
> +    WTF::String nameString = permissionNameToString(name);

auto

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1704
> +    [delegate _webView:m_uiDelegate->m_webView.get().get() requestPermissionForName:nameString origin:wrapper(apiOrigin) topOrigin:wrapper(apiTopOrigin) decisionHandler:makeBlockPtr([completionHandler = std::exchange(completionHandler, { }), checker = WTFMove(checker), page = makeWeakPtr(page), name, originData = origin.data()] (WKPermissionDecision decision) mutable {

std::exchange(completionHandler) -> WTFMove(completionHandler)

> Source/WebKit/UIProcess/WebPageProxy.cpp:8511
> +        auto permissionsState = granted ? PermissionState::Granted : PermissionState::Denied;

This local variable is not necessary.
Comment 7 Sihui Liu 2021-09-03 11:34:22 PDT
(In reply to Chris Dumez from comment #6)
> Comment on attachment 437286 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437286&action=review
> 
> > Source/WebCore/Modules/permissions/PermissionName.h:49
> > +inline ASCIILiteral permissionNameToString(PermissionName name)
> 
> What are these strings used for? Are they meant to be displayed? If so, I
> don't think things like "BackgroundFetch" or "DisplayCapture" is displayable.
> 
> If they are no meant to be displayed, then we are we using strings and not
> an enum?

I am not sure how API client will want to handle this, so I just simply convert to string.

> 
> > Source/WebCore/Modules/permissions/PermissionName.h:73
> > +        return "Nfc"_s;
> 
> Should probably be "NFC".

Will change.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:163
> > +- (void)_webView:(WKWebView *)webView requestPermissionForName:(NSString *)permissionName origin:(WKSecurityOrigin *)origin topOrigin:(WKSecurityOrigin *)topOrigin decisionHandler:(void (^)(WKPermissionDecision decision))decisionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));
> 
> Why are we using a NSString* instead of an enum type for the permission name?

I am not sure how API client will want to handle this, so I just simply convert to string. We can convert to enum I guess.

> 
> > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1702
> > +    WTF::String nameString = permissionNameToString(name);
> 
> auto

This returns ASCIILiteral, still need to convert to WTF::String for below.

> 
> > Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1704
> > +    [delegate _webView:m_uiDelegate->m_webView.get().get() requestPermissionForName:nameString origin:wrapper(apiOrigin) topOrigin:wrapper(apiTopOrigin) decisionHandler:makeBlockPtr([completionHandler = std::exchange(completionHandler, { }), checker = WTFMove(checker), page = makeWeakPtr(page), name, originData = origin.data()] (WKPermissionDecision decision) mutable {
> 
> std::exchange(completionHandler) -> WTFMove(completionHandler)

Will change.

> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:8511
> > +        auto permissionsState = granted ? PermissionState::Granted : PermissionState::Denied;
> 
> This local variable is not necessary.

Will remove.
Comment 8 Sihui Liu 2021-09-03 12:35:56 PDT
Created attachment 437295 [details]
Patch
Comment 9 youenn fablet 2021-09-03 14:46:19 PDT
Comment on attachment 437295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437295&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:104
> +    _WKPermissionNameSpeakerSelection

I am not sure we should enumerate permissions that we do not plan to implement (BT or midi for instance).
We should probably start with the core set we actually want to support and that have existing delegates (capture, device orientation, geolocation).
In the future, we might need to extend this list as new APIs are created and added to the spec.

> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:179
> +- (void)_webView:(WKWebView *)webView requestPermissionForName:(_WKPermissionName)name origin:(WKSecurityOrigin *)origin topOrigin:(WKSecurityOrigin *)topOrigin decisionHandler:(void (^)(WKPermissionDecision decision))decisionHandler WK_API_AVAILABLE(macos(WK_MAC_TBA), ios(WK_IOS_TBA));

I would rename it to queryPermissionForName.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.h:178
> +        void requestPermission(WebPageProxy&, const WebCore::SecurityOrigin&, const WebCore::SecurityOrigin&, WebCore::PermissionName, CompletionHandler<void(bool)>&&) final;

I would have completion handler take a permission state as input instead of a bool.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1720
> +void UIDelegate::UIClient::requestPermission(WebKit::WebPageProxy& page, const WebCore::SecurityOrigin& origin, const WebCore::SecurityOrigin& topOrigin, WebCore::PermissionName name, CompletionHandler<void(bool granted)>&& completionHandler)

I would rename requestPermission to queryPermission or queryPermissionState.
This method is not about actually asking the user for permission.
It is to know the existing permission value shown in Safari Preferences -> Websites tab.

> Source/WebKit/UIProcess/Cocoa/UIDelegate.mm:1744
> +            return page->requestPermissionByDefaultAction(originData, name, WTFMove(completionHandler));

By updating completion handler to take prompt/grant/deny as input, we could simplify this code.
We should just return Prompt here.

> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:760
> +    alertForPermission(*this, *reason, origin, WTFMove(completionHandler));

Ah I see, so maybe I am misunderstanding this patch.
My understanding is that we are trying to implement Permissions.query.
I do not think Permissions.query should trigger any prompt to the user.
But maybe we are trying to achieve something else here.
Comment 10 Sihui Liu 2021-09-05 12:13:50 PDT
Comment on attachment 437295 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437295&action=review

>> Source/WebKit/UIProcess/API/Cocoa/WKUIDelegatePrivate.h:104
>> +    _WKPermissionNameSpeakerSelection
> 
> I am not sure we should enumerate permissions that we do not plan to implement (BT or midi for instance).
> We should probably start with the core set we actually want to support and that have existing delegates (capture, device orientation, geolocation).
> In the future, we might need to extend this list as new APIs are created and added to the spec.

Okay, I will keep what we have now.

>> Source/WebKit/UIProcess/Cocoa/WebPageProxyCocoa.mm:760
>> +    alertForPermission(*this, *reason, origin, WTFMove(completionHandler));
> 
> Ah I see, so maybe I am misunderstanding this patch.
> My understanding is that we are trying to implement Permissions.query.
> I do not think Permissions.query should trigger any prompt to the user.
> But maybe we are trying to achieve something else here.

For current implementation (https://trac.webkit.org/changeset/281771/webkit), web process does not send message to UI process for querying permission (if permission state is not in cache it will return "prompt"). It only sends message about requesting permission, and the reply from UI process must be either "granted" or "denied". Web process will cache the decision.

This patch is about showing a prompt to user when permission is requested. We can add a message for querying permission (not showing prompt).
Comment 11 Sihui Liu 2021-09-05 14:28:47 PDT
Created attachment 437366 [details]
Patch
Comment 12 Sihui Liu 2021-09-05 20:38:18 PDT
Created attachment 437377 [details]
Patch
Comment 13 Sihui Liu 2021-09-05 23:07:56 PDT
Created attachment 437386 [details]
Patch
Comment 14 youenn fablet 2021-09-07 00:55:49 PDT
> This patch is about showing a prompt to user when permission is requested.
> We can add a message for querying permission (not showing prompt).

We already have specific delegates to ask for permission (getUserMedia, device orientation, geolocation). 
The generic delegate would be somehow redundant with these.
If we add this generic delegate, what is the plan for the existing delegates?

The alternative to a generic prompt permission would be to add new permission delegates for the APIs we do care. Specific delegates have some benefits over a catch-them-all generic one:
- default behavior if delegate is not implemented
- passing some input/receiving some parameters in addition to grant/deny.
Comment 15 Sihui Liu 2021-09-07 09:41:30 PDT
(In reply to youenn fablet from comment #14)
> > This patch is about showing a prompt to user when permission is requested.
> > We can add a message for querying permission (not showing prompt).
> 
> We already have specific delegates to ask for permission (getUserMedia,
> device orientation, geolocation). 
> The generic delegate would be somehow redundant with these.
> If we add this generic delegate, what is the plan for the existing delegates?

We may just remove existing delegates. Before we remove them, if existing delegate is set, I think we can prefer existing delegate over generic delegate when making a decision.

For new features (or old features with new permissions), we may just use the generic delegate.

> 
> The alternative to a generic prompt permission would be to add new
> permission delegates for the APIs we do care. Specific delegates have some
> benefits over a catch-them-all generic one:
> - default behavior if delegate is not implemented
> - passing some input/receiving some parameters in addition to grant/deny.

There are many features (and permissions) according to the PermissionName enum in spec, which means we could add many new delegates, and clients need to make corresponding change when we add a new delegate (e.g. in the case where clients have a general policy to grant/deny all cross-origin requests). It may also be confusing since delegates don't have the same naming pattern: e.g. we have shouldAllowDeviceOrientationAndMotionAccess and decidePolicyForNotificationPermissionRequest.

As for passing some input/receiving some parameters other than grant and deny, can you give an example? Do you mean we may want to give more information to client other than origins?
Comment 16 youenn fablet 2021-09-07 10:17:25 PDT
(In reply to Sihui Liu from comment #15)
> (In reply to youenn fablet from comment #14)
> > > This patch is about showing a prompt to user when permission is requested.
> > > We can add a message for querying permission (not showing prompt).
> > 
> > We already have specific delegates to ask for permission (getUserMedia,
> > device orientation, geolocation). 
> > The generic delegate would be somehow redundant with these.
> > If we add this generic delegate, what is the plan for the existing delegates?
> 
> We may just remove existing delegates. Before we remove them, if existing
> delegate is set, I think we can prefer existing delegate over generic
> delegate when making a decision.

Some delegates (getUserMedia, device orientations) are WKUIDelegate API.

> For new features (or old features with new permissions), we may just use the
> generic delegate.
> 
> > 
> > The alternative to a generic prompt permission would be to add new
> > permission delegates for the APIs we do care. Specific delegates have some
> > benefits over a catch-them-all generic one:
> > - default behavior if delegate is not implemented
> > - passing some input/receiving some parameters in addition to grant/deny.
> 
> There are many features (and permissions) according to the PermissionName
> enum in spec, which means we could add many new delegates, and clients need
> to make corresponding change when we add a new delegate (e.g. in the case

We want to provide good defaults, which is easy with specific delegates.
A developer only has to implement a specific delegate if it does not like the default behavior for that particular permission.
With a generic delegate, developer has to make a choice for all permissions when implementing the delegate so it is harder.
If we add a new permission enum value with the generic delegate, it is unclear whether the application delegate existing logic will handle the new permission well or not, the developer has to think about this.

> where clients have a general policy to grant/deny all cross-origin
> requests).

Cross-origin requests should only get to the delegate if the top level origin agreed to let the cross-origin iframe to call the API in its name.
In practice, top-level origin is what should be used, I do not think most developers will actually check the cross-origin iframe.

> It may also be confusing since delegates don't have the same
> naming pattern: e.g. we have shouldAllowDeviceOrientationAndMotionAccess and
> decidePolicyForNotificationPermissionRequest.

The API delegates are named requestMediaCapturePermissionForOrigin and requestDeviceOrientationAndMotionPermissionForOrigin.

> As for passing some input/receiving some parameters other than grant and
> deny, can you give an example? Do you mean we may want to give more
> information to client other than origins?

For getUserMedia, we are asking for either camera, microphone or camera+microphone in a single delegate call.
For geolocation, we may want to have a return parameter that allows the application to set the desired accuracy (needs to decide whether we go there or not).
For getDisplayMedia, we may want to allow an application to do the prompt, in which case, the application might need to give us a display ID as an additional return parameter (API plan remains to be done here, maybe we will consider grant=prompt).
For speaker selection, we may also want an audio route ID as additional return parameter (API plan remains to be done here).
Comment 17 Sihui Liu 2021-09-07 18:36:52 PDT
(In reply to youenn fablet from comment #16)
> (In reply to Sihui Liu from comment #15)
> > (In reply to youenn fablet from comment #14)
> > > > This patch is about showing a prompt to user when permission is requested.
> > > > We can add a message for querying permission (not showing prompt).
> > > 
> > > We already have specific delegates to ask for permission (getUserMedia,
> > > device orientation, geolocation). 
> > > The generic delegate would be somehow redundant with these.
> > > If we add this generic delegate, what is the plan for the existing delegates?
> > 
> > We may just remove existing delegates. Before we remove them, if existing
> > delegate is set, I think we can prefer existing delegate over generic
> > delegate when making a decision.
> 
> Some delegates (getUserMedia, device orientations) are WKUIDelegate API.

What's the issue? The new delegate is a WKUIDelegatePrivate so we may check the old delegate in UIDelegate.mm?

> 
> > For new features (or old features with new permissions), we may just use the
> > generic delegate.
> > 
> > > 
> > > The alternative to a generic prompt permission would be to add new
> > > permission delegates for the APIs we do care. Specific delegates have some
> > > benefits over a catch-them-all generic one:
> > > - default behavior if delegate is not implemented
> > > - passing some input/receiving some parameters in addition to grant/deny.
> > 
> > There are many features (and permissions) according to the PermissionName
> > enum in spec, which means we could add many new delegates, and clients need
> > to make corresponding change when we add a new delegate (e.g. in the case
> 
> We want to provide good defaults, which is easy with specific delegates.
> A developer only has to implement a specific delegate if it does not like
> the default behavior for that particular permission.
> With a generic delegate, developer has to make a choice for all permissions
> when implementing the delegate so it is harder.
> If we add a new permission enum value with the generic delegate, it is
> unclear whether the application delegate existing logic will handle the new
> permission well or not, the developer has to think about this.

ah, we can make the callback take a WKPermissionDecision just like requestMediaCapturePermissionForOrigin. Then it should be easy for the developer to make the transition. They can pick "Prompt" for all permissions they don't care and it will give them the same default behavior.

> 
> > where clients have a general policy to grant/deny all cross-origin
> > requests).
> 
> Cross-origin requests should only get to the delegate if the top level
> origin agreed to let the cross-origin iframe to call the API in its name.
> In practice, top-level origin is what should be used, I do not think most
> developers will actually check the cross-origin iframe.

oh I thought we give frame info to the delegate because client may want to check if it's cross-origin. My point is we can provide a way to allow and deny many permissions at a time.

> 
> > It may also be confusing since delegates don't have the same
> > naming pattern: e.g. we have shouldAllowDeviceOrientationAndMotionAccess and
> > decidePolicyForNotificationPermissionRequest.
> 
> The API delegates are named requestMediaCapturePermissionForOrigin and
> requestDeviceOrientationAndMotionPermissionForOrigin.

hmmm I was looking at WKPageUIClient... it seems we still have some confusing permission delegates in WKUIDelegate and WKUIDelegatePrivate, e.g. requestMediaCapturePermissionForOrigin, checkUserMediaPermissionForURL, requestGeolocationPermissionForFrame, requestGeolocationPermissionForOrigin...

> 
> > As for passing some input/receiving some parameters other than grant and
> > deny, can you give an example? Do you mean we may want to give more
> > information to client other than origins?
> 
> For getUserMedia, we are asking for either camera, microphone or
> camera+microphone in a single delegate call.
> For geolocation, we may want to have a return parameter that allows the
> application to set the desired accuracy (needs to decide whether we go there
> or not).
> For getDisplayMedia, we may want to allow an application to do the prompt,
> in which case, the application might need to give us a display ID as an
> additional return parameter (API plan remains to be done here, maybe we will
> consider grant=prompt).
> For speaker selection, we may also want an audio route ID as additional
> return parameter (API plan remains to be done here).

Permissions spec has some proposal for adding more description about the permission, like https://w3c.github.io/permissions/#dfn-extra-permission-data-type and https://w3c.github.io/permissions/#aspects. For the generic delegate, we may provide a dictionary of extra permission data in the future, which will be align with the permissions spec. 

The completion handler of the delegate may also take a dictionary of extra permission constraints (We may convert it to a new PermissionDescriptor and compare it with input descriptor using partial order https://w3c.github.io/permissions/#dfn-permission-descriptor-type). 

I think the generic delegate will be similar to permissions.query(), which is designed for use of different features. If some feature is not applicable (like camera+microphone case), we can use a special delegate for that (or turn it into a special permission).

We can gradually integrate the existing features with Permissions API and the new delegate. If we think some existing delegate is better, we can remove the feature from _WKPermissionName, so requestPermissionForName will only be used by certain features.
Comment 18 youenn fablet 2021-09-08 00:35:08 PDT
> What's the issue? The new delegate is a WKUIDelegatePrivate so we may check
> the old delegate in UIDelegate.mm?

I think the idea is at some point to promote this delegate to API, in which case we then have two APIs for roughly the same thing.
I think it would be good to get some discussions with others on whether we would be able to ship this form of delegates as API.

> ah, we can make the callback take a WKPermissionDecision just like
> requestMediaCapturePermissionForOrigin. Then it should be easy for the
> developer to make the transition. They can pick "Prompt" for all permissions
> they don't care and it will give them the same default behavior.

That is true if the default behavior is 'Prompt'.
For the cases I know of, this is probably fine, but I am unsure whether that it will be the case for all permissions in the future.

> > > It may also be confusing since delegates don't have the same
> > > naming pattern: e.g. we have shouldAllowDeviceOrientationAndMotionAccess and
> > > decidePolicyForNotificationPermissionRequest.
> > 
> > The API delegates are named requestMediaCapturePermissionForOrigin and
> > requestDeviceOrientationAndMotionPermissionForOrigin.
> 
> hmmm I was looking at WKPageUIClient... it seems we still have some
> confusing permission delegates in WKUIDelegate and WKUIDelegatePrivate, e.g.
> requestMediaCapturePermissionForOrigin, checkUserMediaPermissionForURL,
> requestGeolocationPermissionForFrame,
> requestGeolocationPermissionForOrigin...

Right, we need to remove them once we are able to do so.

> Permissions spec has some proposal for adding more description about the
> permission, like
> https://w3c.github.io/permissions/#dfn-extra-permission-data-type and
> https://w3c.github.io/permissions/#aspects. For the generic delegate, we may
> provide a dictionary of extra permission data in the future, which will be
> align with the permissions spec. 

Right, I am not sure whether we want the delegate API to be modelled around permissions spec especially since we already have API delegates that have been exposed and given the uncertainty with regards to handling additional input/output parameters.
I believe this permission model is used in Chrome/Android so this might be feasible but it seems we should discuss this further.

If the short term goal is to implement permission API, the permission API does not need a prompt since it is only about querying, not requesting a permission.
If the short term goal is to add support for a particular prompt (storage persistency maybe?), this can be done with an ad-hoc delegate for now.

Related to the patch, I would also note that we do not want to always have a straight WebPage -> WebPageProxy -> Delegate code path.
In the case of device orientation and getUserMedia for instance, we first want to check for cached decisions, as well as TCC checks, before actually calling the delegate. This is currently permission-specific code.
In the case of getUserMedia, we are doing other stuff in UIProcess at the same time as treating the getUserMedia permission request.