Bug 210422 – [GTK][WPE] Add API to expose UIClient::requestStorageAccessConfirm

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

.

[Michael Catanzaro]

Naive proposal: WebKitWebView::storage-access-request (parallels WebKitWebView::permission-request). Default impl in WebKit could use a GtkDialog, but Epiphany would reimplement using GtkInfoBar.

[Carlos Garcia Campos]

I think we can just use permission-request.

[Michael Catanzaro]

You're right :)

[Carlos Garcia Campos]

Created attachment 401725 [details]
Patch

I have no idea how to test this.

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Michael Catanzaro]

Comment on attachment 401725 [details]
Patch

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

I think it would be pretty hard to create an API test for this. Manual testing should suffice. (Bonus points for Epiphany implementation.) Do we know of any existing websites that make use of the storage access API with either Safari or Firefox?

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:33
> + * WebKitUserMediaPermissionRequest represents a request for

WebKitWebsiteDataPermissionRequest

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:34
> + * permission to allow a third-party domain access cookies and website data.

access to cookies and website data

Although, I'm not actually certain it *does* allow a third-party domain to access website data other than cookies. Are you sure about this? Firefox allows this according to https://developer.mozilla.org/en-US/docs/Web/API/Storage_Access_API, but its documentation implies that WebKit does not. John would know exactly how this works.

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:81
> +    // Default behaviour when no decision has been made is denying the request.
> +    webkitWebsiteDataAccessPermissionRequestDeny(WEBKIT_PERMISSION_REQUEST(object));

So I see this is copied from our various other permission request classes, but all those classes have a bool madeDecision member variable to track whether a permission request has previously been completed or not and only call the CompletionHandler if so. You don't have that here, so it's going to call the CompletionHandler again unconditionally. If you try this patch in a debug build, I think it's going to assert, because CompletionHandler is designed to assert when called twice.

> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:125
> +/**
> + * webkit_website_data_access_permission_request_get_current_domain:
> + * @request: a #WebKitWebsiteDataAccessPermissionRequest
> + *
> + * Get the domain from which the requesting domain wants to access cookies and website data while browsing.
> + *
> + * Returns: the current domain name
> + *
> + * Since: 2.30

Hm, I assumed that the domain requesting storage access only gets access to its *own* cookies (and maybe also other website data), not access to the first-party domain's data. Does it really get access to the data of the first-party domain? John?

Please investigate before landing; we need to be sure about this before we land this API.

> Source/WebKit/UIProcess/API/gtk/WebKitWebsiteDataAccessPermissionRequest.h:56
> +struct _WebKitWebsiteDataAccessPermissionRequestClass {
> +    GObjectClass parent_class;
> +
> +    void (*_webkit_reserved0) (void);
> +    void (*_webkit_reserved1) (void);
> +    void (*_webkit_reserved2) (void);
> +    void (*_webkit_reserved3) (void);
> +};

Idle brainstorming: now that GTK 4 is upon us, it might be a good idea to figure out if it's possible to hide these class structs from the GTK 4 API. There's no reason for this type to be derivable, and no value in exposing class structs for non-derivable types. Problem is we can't use #if USE(GTK4) in the header files, so we'd need a third copy of all the public header files... possibly not worth it.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #6)
> > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:33
> > + * WebKitUserMediaPermissionRequest represents a request for
> 
> WebKitWebsiteDataPermissionRequest

Even better: WebKitWebsiteDataAccessPermissionRequest :P

[Carlos Garcia Campos]

Comment on attachment 401725 [details]
Patch

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

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:33
>> + * WebKitUserMediaPermissionRequest represents a request for
> 
> WebKitWebsiteDataPermissionRequest

Oops.

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:34
>> + * permission to allow a third-party domain access cookies and website data.
> 
> access to cookies and website data
> 
> Although, I'm not actually certain it *does* allow a third-party domain to access website data other than cookies. Are you sure about this? Firefox allows this according to https://developer.mozilla.org/en-US/docs/Web/API/Storage_Access_API, but its documentation implies that WebKit does not. John would know exactly how this works.

I used the safari dialog message as a reference, see https://webkit.org/wp-content/uploads/storage-access-prompt.png

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:81
>> +    webkitWebsiteDataAccessPermissionRequestDeny(WEBKIT_PERMISSION_REQUEST(object));
> 
> So I see this is copied from our various other permission request classes, but all those classes have a bool madeDecision member variable to track whether a permission request has previously been completed or not and only call the CompletionHandler if so. You don't have that here, so it's going to call the CompletionHandler again unconditionally. If you try this patch in a debug build, I think it's going to assert, because CompletionHandler is designed to assert when called twice.

We don't need an extra variable for that in this case because completion handler is false once it has been called, and I'm checking it's true before calling it in both Allow and Deny.

>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:125
>> + * Since: 2.30
> 
> Hm, I assumed that the domain requesting storage access only gets access to its *own* cookies (and maybe also other website data), not access to the first-party domain's data. Does it really get access to the data of the first-party domain? John?
> 
> Please investigate before landing; we need to be sure about this before we land this API.

Again I used the safari dialog as reference.

>> Source/WebKit/UIProcess/API/gtk/WebKitWebsiteDataAccessPermissionRequest.h:56
>> +};
> 
> Idle brainstorming: now that GTK 4 is upon us, it might be a good idea to figure out if it's possible to hide these class structs from the GTK 4 API. There's no reason for this type to be derivable, and no value in exposing class structs for non-derivable types. Problem is we can't use #if USE(GTK4) in the header files, so we'd need a third copy of all the public header files... possibly not worth it.

We are already adding another header for the cases where gtk4 needs a different API. I don't think it's worth using a different header for all classes in gtk4 just to me them final.

[Michael Catanzaro]

Comment on attachment 401725 [details]
Patch

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

>>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:34
>>> + * permission to allow a third-party domain access cookies and website data.
>> 
>> access to cookies and website data
>> 
>> Although, I'm not actually certain it *does* allow a third-party domain to access website data other than cookies. Are you sure about this? Firefox allows this according to https://developer.mozilla.org/en-US/docs/Web/API/Storage_Access_API, but its documentation implies that WebKit does not. John would know exactly how this works.
> 
> I used the safari dialog message as a reference, see https://webkit.org/wp-content/uploads/storage-access-prompt.png

OK.

>>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:81
>>> +    webkitWebsiteDataAccessPermissionRequestDeny(WEBKIT_PERMISSION_REQUEST(object));
>> 
>> So I see this is copied from our various other permission request classes, but all those classes have a bool madeDecision member variable to track whether a permission request has previously been completed or not and only call the CompletionHandler if so. You don't have that here, so it's going to call the CompletionHandler again unconditionally. If you try this patch in a debug build, I think it's going to assert, because CompletionHandler is designed to assert when called twice.
> 
> We don't need an extra variable for that in this case because completion handler is false once it has been called, and I'm checking it's true before calling it in both Allow and Deny.

OK.

>>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:125
>>> + * Since: 2.30
>> 
>> Hm, I assumed that the domain requesting storage access only gets access to its *own* cookies (and maybe also other website data), not access to the first-party domain's data. Does it really get access to the data of the first-party domain? John?
>> 
>> Please investigate before landing; we need to be sure about this before we land this API.
> 
> Again I used the safari dialog as reference.

In this case, I think you misread the Safari dialog (although I might be wrong). I think the third-party domain is requesting access to its own cookies and website data, not the first-party domain's cookies and website data.

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #8)
> We are already adding another header for the cases where gtk4 needs a
> different API. I don't think it's worth using a different header for all
> classes in gtk4 just to me them final.

At the risk of getting a bit off-topic for this bug, we've probably reached the point where it would be useful to have a "control header" to allow us to use conditional compilation in the public headers. Then, we only need to duplicate the control header for GTK4, rather than every such header. In fact, if we were able to use preprocessor guards we should be able to eliminate duplication between WPE and GTK headers as well and just have one header for WPE, GTK3, and GTK4. Is there any reason we cannot do that? (I guess there is some reason I cannot think of, or you would have done this already?)

Then we can guard all the GObject boilerplate in every header behind #if !USE(GTK4) and use either G_DECLARE_FINAL_TYPE or G_DECLARE_DERIVABLE_TYPE for the USE(GTK4) case, so we have a path to eventually removing the boilerplate 5-10 years from now when dropping GTK 3 support. We can also more aggressively remove all deprecated APIs from the USE(GTK4) case. And we can add more class struct padding to the types that remain derivable, most urgently to WebKitWebView. (It would be a shame to release the new GTK 4 ABI with only a single byte of padding in the WebKitWebView class struct.)

I think the main challenge in supporting different APIs is gtk-doc, but gtk-doc looks at the source files, not the header files.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #10)
> (In reply to Carlos Garcia Campos from comment #8)
> > We are already adding another header for the cases where gtk4 needs a
> > different API. I don't think it's worth using a different header for all
> > classes in gtk4 just to me them final.
> 
> At the risk of getting a bit off-topic for this bug, we've probably reached
> the point where it would be useful to have a "control header" to allow us to
> use conditional compilation in the public headers.

Headers include gtk so I guess we can use #if GTK_CHECK_VERSION. I still don't like condition compilation in public headers, though. What's exactly a control header?

> Then, we only need to
> duplicate the control header for GTK4, rather than every such header. In
> fact, if we were able to use preprocessor guards we should be able to
> eliminate duplication between WPE and GTK headers as well and just have one
> header for WPE, GTK3, and GTK4. Is there any reason we cannot do that? (I
> guess there is some reason I cannot think of, or you would have done this
> already?)

I don't understand the proposal, TBH.

> Then we can guard all the GObject boilerplate in every header behind #if
> !USE(GTK4) and use either G_DECLARE_FINAL_TYPE or G_DECLARE_DERIVABLE_TYPE
> for the USE(GTK4) case, so we have a path to eventually removing the
> boilerplate 5-10 years from now when dropping GTK 3 support. We can also
> more aggressively remove all deprecated APIs from the USE(GTK4) case.

The plan is to remove all deprecated APIs for GTK4 in any case.

> And we
> can add more class struct padding to the types that remain derivable, most
> urgently to WebKitWebView. (It would be a shame to release the new GTK 4 ABI
> with only a single byte of padding in the WebKitWebView class struct.)

That's also the case, no matter how we do it, we won't reuse the same header in those cases.

> I think the main challenge in supporting different APIs is gtk-doc, but
> gtk-doc looks at the source files, not the header files.

It looks at headers too.

[Carlos Garcia Campos]

(In reply to Michael Catanzaro from comment #9)
> Comment on attachment 401725 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401725&action=review
> 
> >>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:34
> >>> + * permission to allow a third-party domain access cookies and website data.
> >> 
> >> access to cookies and website data
> >> 
> >> Although, I'm not actually certain it *does* allow a third-party domain to access website data other than cookies. Are you sure about this? Firefox allows this according to https://developer.mozilla.org/en-US/docs/Web/API/Storage_Access_API, but its documentation implies that WebKit does not. John would know exactly how this works.
> > 
> > I used the safari dialog message as a reference, see https://webkit.org/wp-content/uploads/storage-access-prompt.png
> 
> OK.
> 
> >>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:81
> >>> +    webkitWebsiteDataAccessPermissionRequestDeny(WEBKIT_PERMISSION_REQUEST(object));
> >> 
> >> So I see this is copied from our various other permission request classes, but all those classes have a bool madeDecision member variable to track whether a permission request has previously been completed or not and only call the CompletionHandler if so. You don't have that here, so it's going to call the CompletionHandler again unconditionally. If you try this patch in a debug build, I think it's going to assert, because CompletionHandler is designed to assert when called twice.
> > 
> > We don't need an extra variable for that in this case because completion handler is false once it has been called, and I'm checking it's true before calling it in both Allow and Deny.
> 
> OK.
> 
> >>> Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataAccessPermissionRequest.cpp:125
> >>> + * Since: 2.30
> >> 
> >> Hm, I assumed that the domain requesting storage access only gets access to its *own* cookies (and maybe also other website data), not access to the first-party domain's data. Does it really get access to the data of the first-party domain? John?
> >> 
> >> Please investigate before landing; we need to be sure about this before we land this API.
> > 
> > Again I used the safari dialog as reference.
> 
> In this case, I think you misread the Safari dialog (although I might be
> wrong). I think the third-party domain is requesting access to its own
> cookies and website data, not the first-party domain's cookies and website
> data.

You are indeed right.

[Carlos Garcia Campos]

So, in https://webkit.org/blog/8124/introducing-storage-access-api/ it says:

"WebKit’s implementation of the API only covers cookies for now. It does not affect the partitioning of other storage forms such as IndexedDB or LocalStorage." 

So, yes, for now it's only about cookies. It's a good idea to use WebsiteDataAccess in the permission request name, but maybe we can refer to cookies only in the docs (and message dialog) for now.

[Carlos Garcia Campos]

Created attachment 401888 [details]
Patch for landing

[Carlos Garcia Campos]

Committed r263028: <https://trac.webkit.org/changeset/263028>

[Michael Catanzaro]

OK, tangent:

(In reply to Carlos Garcia Campos from comment #11)
> Headers include gtk so I guess we can use #if GTK_CHECK_VERSION. I still
> don't like condition compilation in public headers, though. What's exactly a
> control header?

Just some header that would define USE_GTK4 for us. Using GTK_CHECK_VERSION is a good idea, but then we still have to have separate headers for GTK and WPE.

I don't mind conditional compilation in public headers if it makes the headers more maintainable. Having two and now potentially three copies of every header that have to be updated in tandem is a shame.

> > Then, we only need to
> > duplicate the control header for GTK4, rather than every such header. In
> > fact, if we were able to use preprocessor guards we should be able to
> > eliminate duplication between WPE and GTK headers as well and just have one
> > header for WPE, GTK3, and GTK4. Is there any reason we cannot do that? (I
> > guess there is some reason I cannot think of, or you would have done this
> > already?)
> 
> I don't understand the proposal, TBH.

For instance:

#define WEBKIT_TYPE_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST            (webkit_website_data_access_permission_request_get_type())

#if GTK_CHECK_VERSION(3, 90, 0)

G_DECLARE_FINAL_TYPE(WebKitWebsiteDataAccessPermissionRequest, webkit_website_data_access_permission_request, WEBKIT, WEBSITE_DATA_ACCESS_PERMISSION_REQUEST, WebKitPermissionRequest)

#else

#define WEBKIT_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST(obj)            (G_TYPE_CHECK_INSTANCE_CAST((obj), WEBKIT_TYPE_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST, WebKitWebsiteDataAccessPermissionRequest))
#define WEBKIT_IS_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST(obj)         (G_TYPE_CHECK_INSTANCE_TYPE((obj), WEBKIT_TYPE_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST))
#define WEBKIT_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST_CLASS(klass)    (G_TYPE_CHECK_CLASS_CAST((klass),  WEBKIT_TYPE_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST, WebKitWebsiteDataAccessPermissionRequestClass))
#define WEBKIT_IS_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST_CLASS(klass) (G_TYPE_CHECK_CLASS_TYPE((klass),  WEBKIT_TYPE_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST))
#define WEBKIT_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST_GET_CLASS(obj)  (G_TYPE_INSTANCE_GET_CLASS((obj),  WEBKIT_TYPE_WEBSITE_DATA_ACCESS_PERMISSION_REQUEST, WebKitWebsiteDataAccessPermissionRequestClass))

typedef struct _WebKitWebsiteDataAccessPermissionRequest        WebKitWebsiteDataAccessPermissionRequest;
typedef struct _WebKitWebsiteDataAccessPermissionRequestClass   WebKitWebsiteDataAccessPermissionRequestClass;
typedef struct _WebKitWebsiteDataAccessPermissionRequestPrivate WebKitWebsiteDataAccessPermissionRequestPrivate;

struct _WebKitWebsiteDataAccessPermissionRequest {
    GObject parent;

    /*< private >*/
    WebKitWebsiteDataAccessPermissionRequestPrivate *priv;
};

struct _WebKitWebsiteDataAccessPermissionRequestClass {
    GObjectClass parent_class;

    void (*_webkit_reserved0) (void);
    void (*_webkit_reserved1) (void);
    void (*_webkit_reserved2) (void);
    void (*_webkit_reserved3) (void);
};

#endif

Now, let's say we remove the GTK 3 API in 2025. Then that's a lot of boilerplate we can remove from the header files five years from now. But if we don't switch to G_DECLARE_[FINAL,DERIVABLE]_TYPE now, we're stuck with it.

Or, for something completely different... we could use it in WebKitWebContext.h to hide functions entirely from the GTK 4 API:

#if !GTK_CHECK_VERSION(3, 90, 0)
WEBKIT_API void
webkit_web_context_set_sandbox_enabled              (WebKitWebContext              *context,
                                                     gboolean                       enabled);

WEBKIT_API gboolean
webkit_web_context_get_sandbox_enabled              (WebKitWebContext              *context);
#endif

Or consider WebKitWebPage.h, it's not meaningful to create your own WebKitWebPage, so why should the class struct be declared in a public header? Ideally every single public header would change for GTK 4, but we don't really want to have three copies of each header.

[Michael Catanzaro]

I forgot the most important example:

struct _WebKitWebViewClass {
    // ...

    void (*_webkit_reserved0) (void);

#if GTK_CHECK_VERSION(3, 90, 0)
    void (*_webkit_reserved1) (void);
    void (*_webkit_reserved2) (void);
    void (*_webkit_reserved3) (void);
    void (*_webkit_reserved4) (void);
    void (*_webkit_reserved5) (void);
    void (*_webkit_reserved6) (void);
    void (*_webkit_reserved7) (void);
    void (*_webkit_reserved8) (void);
    void (*_webkit_reserved9) (void);
    void (*_webkit_reserved10) (void);
    void (*_webkit_reserved11) (void);
    void (*_webkit_reserved12) (void);
    void (*_webkit_reserved13) (void);
    void (*_webkit_reserved14) (void);
    void (*_webkit_reserved15) (void);
    void (*_webkit_reserved16) (void);
    void (*_webkit_reserved17) (void);
    void (*_webkit_reserved18) (void);
    void (*_webkit_reserved19) (void);
    void (*_webkit_reserved20) (void);
    void (*_webkit_reserved21) (void);
    void (*_webkit_reserved22) (void);
    void (*_webkit_reserved23) (void);
    void (*_webkit_reserved24) (void);
#endif
}

Probably we won't use that much, but we'll feel really silly if we only add 16 new padding bytes and run out. :)