This is the next big patch adding the classes required to use libwebrtc backend support for the GTK and WPE ports.
*** Bug 172269 has been marked as a duplicate of this bug. ***
Created attachment 340841 [details] Patch
Comment on attachment 340841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340841&action=review > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:449 > + static int hashSaltSize = 128; > + static int randomDataSize = hashSaltSize / 16; > + uint64_t randomData[randomDataSize]; > + cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(randomData), sizeof(randomData)); > + > + StringBuilder builder; > + builder.reserveCapacity(128); > + for (int i = 0; i < randomDataSize; i++) > + appendUnsigned64AsHex(randomData[0], builder); > + priv->mediaIdentifierHashSalt = builder.toString(); > + } What's the desired amount of random data you're after? Effectively you're getting 64 * 128 / 16 = 512 bits, but hashSaltSize doesn't help clarify that. Assuming 512 bits, allow me to be nit-picky and recommend using std:array: ``` static constexpr size_t hashSaltSize = 512; std::array<uint64_t, hashSaltSize / sizeof(uint64_t)> randomData; cryptographicallyRandomValues(reinterpret_cast<void*>(randomData.data()), randomData.size() * sizeof(uint64_t)); StringBuilder builder; builder.reserveCapacity(randomData.size() * sizeof(uint64_t)); for (auto val : randomData) appendUnsigned64AsHex(val, builder); priv->mediaIdentifierHashSalt = builder.toString(); ```
Thanks for the review! (In reply to Zan Dobersek from comment #3) > Comment on attachment 340841 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340841&action=review > > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:449 > > + static int hashSaltSize = 128; > > + static int randomDataSize = hashSaltSize / 16; > > + uint64_t randomData[randomDataSize]; > > + cryptographicallyRandomValues(reinterpret_cast<unsigned char*>(randomData), sizeof(randomData)); > > + > > + StringBuilder builder; > > + builder.reserveCapacity(128); > > + for (int i = 0; i < randomDataSize; i++) > > + appendUnsigned64AsHex(randomData[0], builder); > > + priv->mediaIdentifierHashSalt = builder.toString(); > > + } > > What's the desired amount of random data you're after? Effectively you're > getting 64 * 128 / 16 = 512 bits, but hashSaltSize doesn't help clarify that. > Regarding the size, this is basically a default method we expect embedders to override and properly create a storage of hash salts per origin, which should be handled like cookies. My understanding is that the salt should be as big as to avoid to have many collisions per origin. But in this case we are storing one per webview because this is just the default testing method, it is just a matter of not having a fixed one and being able to even fingerprint this testing situation. I guess 128 is more than enough for this, that is why my goal was 128. I think your calculation is not correct, basically the method chooses random hexadecimal values from a fixed array of size 16, that means 4 bits to choose a character per uint64_t number, so 16 hexadecimal characters per uint64_t, that is why we need 128 / 16 unint64_t elements to get 128 hexadecimal values. > Assuming 512 bits, allow me to be nit-picky and recommend using std:array: > ``` > static constexpr size_t hashSaltSize = 512; > std::array<uint64_t, hashSaltSize / sizeof(uint64_t)> randomData; > > cryptographicallyRandomValues(reinterpret_cast<void*>(randomData.data()), > randomData.size() * sizeof(uint64_t)); > > StringBuilder builder; > builder.reserveCapacity(randomData.size() * sizeof(uint64_t)); > for (auto val : randomData) > appendUnsigned64AsHex(val, builder); > priv->mediaIdentifierHashSalt = builder.toString(); > ``` Looks great, I'll use the array thanks, I just need to replace the size part. Something like this: static constexpr size_t hashSaltSize = 128; std::array<uint64_t, hashSaltSize / 16> randomData; cryptographicallyRandomValues(reinterpret_cast<void*>(randomData.data()), randomData.size() * sizeof(uint64_t)); StringBuilder builder; builder.reserveCapacity(hashSaltSize); for (auto val : randomData) appendUnsigned64AsHex(val, builder); priv->mediaIdentifierHashSalt = builder.toString();
Comment on attachment 340841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340841&action=review I did a first pass on the GStreamer bits. > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:17 > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR Hum I don't think we want the Apple version of the header. For every new file please make sure to use the standard BSD header. > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:135 > +void GStreamerAudioCaptureSource::stopProducingData() This should undo what startProducingData() does I think? Namely disconnect the new-sample signal. > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:142 > + if (!m_capabilities) { This could become an early return. > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCapturer.cpp:49 > + // FIXME Handle errors. Please open a bug and mention it for every FIXME :) > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioStreamDescription.h:58 > + m_platformDescription = { PlatformDescription::GStreamerAudioStreamDescription, (AudioStreamBasicDescription*) &m_info }; Please avoid C-style casts everywhere in C++ code. Use static_cast or reinterpret_cast. > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioStreamDescription.h:83 > + double sampleRate() const final { return GST_AUDIO_INFO_RATE (&m_info); } No space before ( ... I wonder why check-webkit-style doesn't complain about that :/ > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 > + ASSERT_NOT_REACHED(); This is enabled only for debug builds I think. You'd need an early return for release builds. > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:172 > + // Only accept raw video for now. > + if (!gst_structure_has_name(str, "video/x-raw")) Will be nice to support compressed formats too :)
Comment on attachment 340841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340841&action=review Wow, huge patch detected. You drew my attention because it adds new GTK/WPE API. ;) I have enough questions and concerns about the new API that I'm going to use r-, even though it's a fairly small portion of the patch. > Source/WebKit/ChangeLog:21 > + webkit_permission_request_resolve_check API, adding the hash salt > + and the decision about allowing access to the devices. Main point > + of this API is to make sure that the user can control the webpages > + and that they don't get smart trying to fingerprint the system. There is a What exactly is the salt used for? What is being salted in the first place? How is the browser supposed to know what it's supposed to salt? >> Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 >> + if (!gst_element_link_pads(m_tee.get(), "src_%u", queue, "sink")) >> + ASSERT_NOT_REACHED(); >> + >> + if (!gst_element_link(queue, newSink)) >> + ASSERT_NOT_REACHED(); > > This is enabled only for debug builds I think. You'd need an early return for release builds. A minor point, but I would say the code is good and an early return should not be added, because the code should assume the assertion is never reached. That's the point of the asserts, after all. Unless GCC is giving -Wreturn-type or -Wswitch warnings, which shouldn't occur in this case. You wouldn't add an early return in GStreamerCapturer::play or GStreamerCapturer::stop, would you? Of course not! This isn't any different. > Source/WebKit/UIProcess/API/glib/WebKitPermissionRequest.cpp:92 > + * We have to call this function to specify if a request is allowed > + * using the @allowed parameter. Also we add the random data that > + * would be used as a hash salt in case the user did not allow the > + * origin for this kind of requests, using the > + * @salt parameter. > + */ Since: 2.22 > Source/WebKit/UIProcess/API/glib/WebKitPermissionRequest.cpp:93 > +void webkit_permission_request_resolve_check(WebKitPermissionRequest *request, const gchar* mediaIdentifierHashSalt, gboolean allowed) The mediaIdentifierHashSalt parameter is specific to WebKitUserMediaPermissionRequest. Surely it shouldn't be in any WebKitPermissionRequest functions? Why does it need to be part of the interface, instead of specific to WebKitUserMediaPermissionRequest? This causes more problems below, that would probably be best avoided here. > Source/WebKit/UIProcess/API/glib/WebKitUserMediaPermissionRequest.cpp:57 > struct _WebKitUserMediaPermissionRequestPrivate { > RefPtr<UserMediaPermissionRequestProxy> request; > + RefPtr<UserMediaPermissionCheckProxy> checkRequest; I think it would be a lot simpler to add a new WebKitUserMediaPermissionCheckRequest subclass instead, that could subclass WebKitUserMediaPermissionRequest, and then embedders would know not to actually prompt the user. But maybe Carlos Garcia will have a better idea. >>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:449 >>> + } >> >> What's the desired amount of random data you're after? Effectively you're getting 64 * 128 / 16 = 512 bits, but hashSaltSize doesn't help clarify that. >> >> Assuming 512 bits, allow me to be nit-picky and recommend using std:array: >> ``` >> static constexpr size_t hashSaltSize = 512; >> std::array<uint64_t, hashSaltSize / sizeof(uint64_t)> randomData; >> cryptographicallyRandomValues(reinterpret_cast<void*>(randomData.data()), randomData.size() * sizeof(uint64_t)); >> >> StringBuilder builder; >> builder.reserveCapacity(randomData.size() * sizeof(uint64_t)); >> for (auto val : randomData) >> appendUnsigned64AsHex(val, builder); >> priv->mediaIdentifierHashSalt = builder.toString(); >> ``` > > Regarding the size, this is basically a default method we expect embedders to override and properly create a storage of hash salts per origin, which should be handled like cookies. My understanding is that the salt should be as big as to avoid to have many collisions per origin. But in this case we are storing one per webview because this is just the default testing method, it is just a matter of not having a fixed one and being able to even fingerprint this testing situation. I guess 128 is more than enough for this, that is why my goal was 128. > > I think your calculation is not correct, basically the method chooses random hexadecimal values from a fixed array of size 16, that means 4 bits to choose a character per uint64_t number, so 16 hexadecimal characters per uint64_t, that is why we need 128 / 16 unint64_t elements to get 128 hexadecimal values. WebKitWebView is surely not an appropriate place for this code? This all comes down to the weird salt parameter in webkit_permission_request_resolve_check(). > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1532 > + * This signal is emitted when WebKit wants to check the > + * permissions already given to an origin by a user, such as > + * allowing the browser to use the microphone or the > + * camera. Usually the embedder does not ask the user to resolve > + * the request but uses what the user answered previously, denying > + * by default if the user did not give permissions specifically to > + * this origin. OK, I see what the motivation for adding this was, but I think it's a misunderstanding of how WebKitPermissionRequest works. The browser doesn't *have* to display a permission request to the user. E.g. for Epiphany we always check whether the user has already made a decision, and just return that if so. The difference is you want the browser to *never* display the permission request. That's simple enough, since browsers need to manually implement support for each new type of permission request: you can just document that your new permission request type is not intended to be displayed to the user. Right? Then we would not need WebKitWebView::permission-check at all? > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1550 > + * Since: 2.22 > Source/WebKit/UIProcess/API/gtk/WebKitPermissionRequest.h:47 > + void (* resolve_check) (WebKitPermissionRequest *request, > + const gchar* salt, > + gboolean allowed); You can't add new vfuncs without removing padding, and for some reason there's no padding. So it requires a soname bump (for both GTK and WPE). Wow. If we've ever had to do this before, it was before I started contributing to WebKit. We'll need to discuss with Carlos Garcia, and take the opportunity to add a bunch of new padding to WebKitWebView as well. But I'm confused whether the salt is really an appropriate parameter to have here. Like I said above, I think it really belongs in WebKitUserMediaPermissionRequest, so we can probably avoid this. > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:235 > + gboolean (* permission_check) (WebKitWebView *web_view, > + WebKitPermissionRequest *permission_request); As was mentioned on Matrix, you'll need to remove padding here too. Same for the WPE header.
(In reply to Alejandro G. Castro from comment #4) > Regarding the size, this is basically a default method we expect embedders > to override and properly create a storage of hash salts per origin, which > should be handled like cookies. I already asked what exactly the purpose of these salts is for, since it affects the API. One more question: is this something that WebKit could be handling instead of the embedder?
(In reply to Michael Catanzaro from comment #6) > What exactly is the salt used for? What is being salted in the first place? > How is the browser supposed to know what it's supposed to salt? Well, maybe I should have looked at the title of the bug. I assume each media device is represented as a hash that gets exposed to the web. How that's constructed, I'm not sure, but you then salt it with some randomness. The randomness used is different for every origin and thus fingerprinting is avoided. Right? "How that's constructed, I'm not sure" is the problem there. Surely that should be done in WebKit, not by browsers. And then storing the result is another problem. Have you tried implementing this for Epiphany yet? It would require a lot of changes in EphyPermissionsManager to store strings instead of non-boolean values, right? Not sure about that. We normally require either a client implementation somewhere before adding new API, usually either MiniBrowser or Epiphany. In this case, we probably should expect both, because we surely want the web API to work properly in MiniBrowser, but MiniBrowser not a good test case for the GTK/WPE API as it doesn't store permissions on disk. Seeing the implementations will help us understand whether we have the API right.
Comment on attachment 340841 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=340841&action=review >> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:235 >> + WebKitPermissionRequest *permission_request); > > As was mentioned on Matrix, you'll need to remove padding here too. Same for the WPE header. Oh, also it needs to go on the bottom, otherwise it'll break applications. (Unless we do a soname bump.)
Thanks for the review! (In reply to Philippe Normand from comment #5) > Comment on attachment 340841 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340841&action=review > > I did a first pass on the GStreamer bits. > > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:17 > > + * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR > > Hum I don't think we want the Apple version of the header. For every new > file please make sure to use the standard BSD header. > Right. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:135 > > +void GStreamerAudioCaptureSource::stopProducingData() > > This should undo what startProducingData() does I think? Namely disconnect > the new-sample signal. > Right, we have missed that one, I have doubts about the setupPipeline but it is true we have to check that situation more carefully. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCaptureSource.cpp:142 > > + if (!m_capabilities) { > > This could become an early return. > Right. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioCapturer.cpp:49 > > + // FIXME Handle errors. > > Please open a bug and mention it for every FIXME :) > Right. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioStreamDescription.h:58 > > + m_platformDescription = { PlatformDescription::GStreamerAudioStreamDescription, (AudioStreamBasicDescription*) &m_info }; > > Please avoid C-style casts everywhere in C++ code. Use static_cast or > reinterpret_cast. > Right. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerAudioStreamDescription.h:83 > > + double sampleRate() const final { return GST_AUDIO_INFO_RATE (&m_info); } > > No space before ( ... I wonder why check-webkit-style doesn't complain about > that :/ > Right. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 > > + ASSERT_NOT_REACHED(); > > This is enabled only for debug builds I think. You'd need an early return > for release builds. > Right, we missed the return. > > Source/WebCore/platform/mediastream/gstreamer/GStreamerVideoCaptureSource.cpp:172 > > + // Only accept raw video for now. > > + if (!gst_structure_has_name(str, "video/x-raw")) > > Will be nice to support compressed formats too :) Yep :-), we will work on that at some point.
Thanks for the review Michael, good points! (In reply to Michael Catanzaro from comment #6) > Comment on attachment 340841 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340841&action=review > > Wow, huge patch detected. > > You drew my attention because it adds new GTK/WPE API. ;) I have enough > questions and concerns about the new API that I'm going to use r-, even > though it's a fairly small portion of the patch. > Yep, that kind of patch ;-). > > Source/WebKit/ChangeLog:21 > > + webkit_permission_request_resolve_check API, adding the hash salt > > + and the decision about allowing access to the devices. Main point > > + of this API is to make sure that the user can control the webpages > > + and that they don't get smart trying to fingerprint the system. There is a > > What exactly is the salt used for? What is being salted in the first place? > How is the browser supposed to know what it's supposed to salt? > Right, we will try to add some more information about it in the documentation. In summary we are trying to avoid fingerprinting using these calls. In case the user authorizes we have to warn them carefully and there are other options we can use but they are something we will address in the getUserMedia implementation. This salt controls the case where the user does not give authotization, in this case we have to avoid sending the same ID to every origin because it could be used to track the user, but they could be persistent to allow webpages to store previous configurations of the multimedia. The salt per origin allows the browser to support controlling the fingerprinting and using a persistent id to allow webpages to store preferences regarding the devices. The salt is used to hash the device persistent ID int he system and generate an ID, this way we can have the same IDs for an origin storing the salt. If you want to read more information check the spec here: https://w3c.github.io/mediacapture-main/#enumerating-devices And mainly the non-normative section about privacy and security considerations: https://w3c.github.io/mediacapture-main/#privacy-and-security-considerations > >> Source/WebCore/platform/mediastream/gstreamer/GStreamerCapturer.cpp:141 > >> + if (!gst_element_link_pads(m_tee.get(), "src_%u", queue, "sink")) > >> + ASSERT_NOT_REACHED(); > >> + > >> + if (!gst_element_link(queue, newSink)) > >> + ASSERT_NOT_REACHED(); > > > > This is enabled only for debug builds I think. You'd need an early return for release builds. > > A minor point, but I would say the code is good and an early return should > not be added, because the code should assume the assertion is never reached. > That's the point of the asserts, after all. Unless GCC is giving > -Wreturn-type or -Wswitch warnings, which shouldn't occur in this case. You > wouldn't add an early return in GStreamerCapturer::play or > GStreamerCapturer::stop, would you? Of course not! This isn't any different. > Right, we missed the return. > > Source/WebKit/UIProcess/API/glib/WebKitPermissionRequest.cpp:92 > > + * We have to call this function to specify if a request is allowed > > + * using the @allowed parameter. Also we add the random data that > > + * would be used as a hash salt in case the user did not allow the > > + * origin for this kind of requests, using the > > + * @salt parameter. > > + */ > > Since: 2.22 > Right. > > Source/WebKit/UIProcess/API/glib/WebKitPermissionRequest.cpp:93 > > +void webkit_permission_request_resolve_check(WebKitPermissionRequest *request, const gchar* mediaIdentifierHashSalt, gboolean allowed) > > The mediaIdentifierHashSalt parameter is specific to > WebKitUserMediaPermissionRequest. Surely it shouldn't be in any > WebKitPermissionRequest functions? Why does it need to be part of the > interface, instead of specific to WebKitUserMediaPermissionRequest? > > This causes more problems below, that would probably be best avoided here. > The browser needs to handle the salt per origin storage, like it does for the cookies, and being able to remove them at some point if the user wants. > > Source/WebKit/UIProcess/API/glib/WebKitUserMediaPermissionRequest.cpp:57 > > struct _WebKitUserMediaPermissionRequestPrivate { > > RefPtr<UserMediaPermissionRequestProxy> request; > > + RefPtr<UserMediaPermissionCheckProxy> checkRequest; > > I think it would be a lot simpler to add a new > WebKitUserMediaPermissionCheckRequest subclass instead, that could subclass > WebKitUserMediaPermissionRequest, and then embedders would know not to > actually prompt the user. But maybe Carlos Garcia will have a better idea. > That was my preference initially but a couple of weeks ago after checking with Carlos he is more interested in reducing the changes to the API, so I decided to implement this option first. I still need to talk to him but he is even more interested in trying to add support for this without even the signal. > >>> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:449 > >>> + } > >> > >> What's the desired amount of random data you're after? Effectively you're getting 64 * 128 / 16 = 512 bits, but hashSaltSize doesn't help clarify that. > >> > >> Assuming 512 bits, allow me to be nit-picky and recommend using std:array: > >> ``` > >> static constexpr size_t hashSaltSize = 512; > >> std::array<uint64_t, hashSaltSize / sizeof(uint64_t)> randomData; > >> cryptographicallyRandomValues(reinterpret_cast<void*>(randomData.data()), randomData.size() * sizeof(uint64_t)); > >> > >> StringBuilder builder; > >> builder.reserveCapacity(randomData.size() * sizeof(uint64_t)); > >> for (auto val : randomData) > >> appendUnsigned64AsHex(val, builder); > >> priv->mediaIdentifierHashSalt = builder.toString(); > >> ``` > > > > Regarding the size, this is basically a default method we expect embedders to override and properly create a storage of hash salts per origin, which should be handled like cookies. My understanding is that the salt should be as big as to avoid to have many collisions per origin. But in this case we are storing one per webview because this is just the default testing method, it is just a matter of not having a fixed one and being able to even fingerprint this testing situation. I guess 128 is more than enough for this, that is why my goal was 128. > > > > I think your calculation is not correct, basically the method chooses random hexadecimal values from a fixed array of size 16, that means 4 bits to choose a character per uint64_t number, so 16 hexadecimal characters per uint64_t, that is why we need 128 / 16 unint64_t elements to get 128 hexadecimal values. > > WebKitWebView is surely not an appropriate place for this code? This all > comes down to the weird salt parameter in > webkit_permission_request_resolve_check(). > Right, Carlos and me have a pending talk about this this week, we could use a parameter to the WebKitUserMediaPermissionRequest instead and try to reduce the modifications to the API. > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1532 > > + * This signal is emitted when WebKit wants to check the > > + * permissions already given to an origin by a user, such as > > + * allowing the browser to use the microphone or the > > + * camera. Usually the embedder does not ask the user to resolve > > + * the request but uses what the user answered previously, denying > > + * by default if the user did not give permissions specifically to > > + * this origin. > > OK, I see what the motivation for adding this was, but I think it's a > misunderstanding of how WebKitPermissionRequest works. The browser doesn't > *have* to display a permission request to the user. E.g. for Epiphany we > always check whether the user has already made a decision, and just return > that if so. The difference is you want the browser to *never* display the > permission request. That's simple enough, since browsers need to manually > implement support for each new type of permission request: you can just > document that your new permission request type is not intended to be > displayed to the user. Right? Then we would not need > WebKitWebView::permission-check at all? > Right, exactly, the user agent can decide about it because does not seem normative but chrome and safari are doing that, in the common case you don't want to ask the user and you want the webapps to try to use this call to get information about the system because of it. I guess you mean adding a new class, that is my preference too, I'll talk to Carlos about it and we will check if that solves all the issues. > > Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:1550 > > + * > > Since: 2.22 > Right. > > Source/WebKit/UIProcess/API/gtk/WebKitPermissionRequest.h:47 > > + void (* resolve_check) (WebKitPermissionRequest *request, > > + const gchar* salt, > > + gboolean allowed); > > You can't add new vfuncs without removing padding, and for some reason > there's no padding. So it requires a soname bump (for both GTK and WPE). > Wow. If we've ever had to do this before, it was before I started > contributing to WebKit. We'll need to discuss with Carlos Garcia, and take > the opportunity to add a bunch of new padding to WebKitWebView as well. > Right, I'm talking to him about this, we did some weeks ago and this was kind of the first option but we have a pending talk this week. > But I'm confused whether the salt is really an appropriate parameter to have > here. Like I said above, I think it really belongs in > WebKitUserMediaPermissionRequest, so we can probably avoid this. > > > Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:235 > > + gboolean (* permission_check) (WebKitWebView *web_view, > > + WebKitPermissionRequest *permission_request); > > As was mentioned on Matrix, you'll need to remove padding here too. Same for > the WPE header. Right. Thanks again for the review.
(In reply to Michael Catanzaro from comment #7) > (In reply to Alejandro G. Castro from comment #4) > > Regarding the size, this is basically a default method we expect embedders > > to override and properly create a storage of hash salts per origin, which > > should be handled like cookies. > > I already asked what exactly the purpose of these salts is for, since it > affects the API. One more question: is this something that WebKit could be > handling instead of the embedder? AFAIK WebKit could be handling them if we provide API to control the storage, this is essentially a persistent cookie, I don't recall what are we currently doing about them. I guess we have the option to provide more code to handle this information, anyway the API could initially give the check permission class letting the embedder the responsibility and later add some new API object to handle its storage.
(In reply to Michael Catanzaro from comment #9) > Comment on attachment 340841 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=340841&action=review > > >> Source/WebKit/UIProcess/API/gtk/WebKitWebView.h:235 > >> + WebKitPermissionRequest *permission_request); > > > > As was mentioned on Matrix, you'll need to remove padding here too. Same for the WPE header. > > Oh, also it needs to go on the bottom, otherwise it'll break applications. > (Unless we do a soname bump.) Right, I forgot about the class functions layout in gobject. Thanks for the review!
(In reply to Michael Catanzaro from comment #8) > (In reply to Michael Catanzaro from comment #6) > > What exactly is the salt used for? What is being salted in the first place? > > How is the browser supposed to know what it's supposed to salt? > > Well, maybe I should have looked at the title of the bug. I assume each > media device is represented as a hash that gets exposed to the web. How > that's constructed, I'm not sure, but you then salt it with some randomness. > The randomness used is different for every origin and thus fingerprinting is > avoided. Right? > > "How that's constructed, I'm not sure" is the problem there. Surely that > should be done in WebKit, not by browsers. And then storing the result is > another problem. Have you tried implementing this for Epiphany yet? It would > require a lot of changes in EphyPermissionsManager to store strings instead > of non-boolean values, right? Not sure about that. > > We normally require either a client implementation somewhere before adding > new API, usually either MiniBrowser or Epiphany. In this case, we probably > should expect both, because we surely want the web API to work properly in > MiniBrowser, but MiniBrowser not a good test case for the GTK/WPE API as it > doesn't store permissions on disk. Seeing the implementations will help us > understand whether we have the API right. Right, we will have to do that implementation, we could even remove all the default implementation (just send a fixed salt) and add it to the minibrowser as an example, now I doubt we care about the testing issues with fingerprinting. Also regarding the information about this situation, I copied from the answer of your first comment: Right, we will try to add some more information about it in the documentation. In summary we are trying to avoid fingerprinting using these calls. In case the user authorizes we have to warn them carefully and there are other options we can use but they are something we will address in the getUserMedia implementation. This salt controls the case where the user does not give authotization, in this case we have to avoid sending the same ID to every origin because it could be used to track the user, but they could be persistent to allow webpages to store previous configurations of the multimedia. The salt per origin allows the browser to support controlling the fingerprinting and using a persistent id to allow webpages to store preferences regarding the devices. The salt is used to hash the device persistent ID int he system and generate an ID, this way we can have the same IDs for an origin storing the salt. If you want to read more information check the spec here: https://w3c.github.io/mediacapture-main/#enumerating-devices And mainly the non-normative section about privacy and security considerations: https://w3c.github.io/mediacapture-main/#privacy-and-security-considerations
(In reply to Alejandro G. Castro from comment #14) > Right, we will have to do that implementation, we could even remove all the > default implementation (just send a fixed salt) and add it to the > minibrowser as an example, now I doubt we care about the testing issues with > fingerprinting. Problem is the default implementation is not going to be used just for testing, it's going to be used for almost every application out there. Probably only Epiphany and a couple other apps like Eolie will take the time to override the default implementation.
I wonder whether there could be a way to split this patch in several pieces, for instance: - Add enough support for mock capture to start running layout tests - Add client API to allow/deny access to getUserMedia - Add GStreamer capture I also wonder whether the salt storage/deletion/management should not be made part of WebKit 2 code base.
(In reply to youenn fablet from comment #16) > I wonder whether there could be a way to split this patch in several pieces, > for instance: > - Add enough support for mock capture to start running layout tests > - Add client API to allow/deny access to getUserMedia > - Add GStreamer capture > It is actually divided, this patch does not include getUserMedia support or API for allowing/denying access to getUserMedia, just the classes required for enumerateDevices. We have two more patches almost ready: one for the getUserMedia support, adding the media source and test structure, etc. And the next one for the PeerConnection support. The getUserMedia API to allow/deny is already upstream, because it was used in the previous webrtc implementation, this is just the check permissions part API used for the enumerate devices call. > I also wonder whether the salt storage/deletion/management should not be > made part of WebKit 2 code base. Yep, that is something we are considering after the comments, it seems we could share part of the code and export the APIs. I'll ping you about that to agree about a solution in we think we should go in that direction. Anyway, we will review the comments sent and have that meeting with Carlos about the API. After that we will check if we can move some parts of the patch to the getUserMedia patch to make it smaller. Thanks for commenting!
After reading the spec, I think it would be easier if we don't delegate the handling of the persistent permissions storage to the browser. It could be done by WebKit, as youenn suggested, similar to other persistent stroage handled by WebKit. We could have a manager that we can configure to set a patch where to store the cache, and a way to delete it (maybe this fits in WebsiteDataStore). Then, when a permission request is made we ask the manager for the permission for the given origin and use it without asking the user at all. If we don't have permissions stored we ask the user and store the result. I think this will be simpler, requires less API, and allows us to share more code among ports, but will also save a lot of code in the browser side.
(In reply to Carlos Garcia Campos from comment #18) > After reading the spec, I think it would be easier if we don't delegate the > handling of the persistent permissions storage to the browser. It could be > done by WebKit, as youenn suggested, similar to other persistent stroage > handled by WebKit. We could have a manager that we can configure to set a > patch where to store the cache, and a way to delete it (maybe this fits in > WebsiteDataStore). Then, when a permission request is made we ask the > manager for the permission for the given origin and use it without asking > the user at all. If we don't have permissions stored we ask the user and > store the result. I think this will be simpler, requires less API, and > allows us to share more code among ports, but will also save a lot of code > in the browser side. I agree that we should store the salt and persistent permissions (if any) in WebsiteDataStore. This will allow us to remove UIClient.checkUserMediaPermissionForOrigin and simplify UserMediaPermissionRequestManagerProxy considerably.
(In reply to Eric Carlson from comment #19) > (In reply to Carlos Garcia Campos from comment #18) > > After reading the spec, I think it would be easier if we don't delegate the > > handling of the persistent permissions storage to the browser. It could be > > done by WebKit, as youenn suggested, similar to other persistent stroage > > handled by WebKit. We could have a manager that we can configure to set a > > patch where to store the cache, and a way to delete it (maybe this fits in > > WebsiteDataStore). Then, when a permission request is made we ask the > > manager for the permission for the given origin and use it without asking > > the user at all. If we don't have permissions stored we ask the user and > > store the result. I think this will be simpler, requires less API, and > > allows us to share more code among ports, but will also save a lot of code > > in the browser side. > > I agree that we should store the salt and persistent permissions (if any) in > WebsiteDataStore. This will allow us to remove > UIClient.checkUserMediaPermissionForOrigin and simplify > UserMediaPermissionRequestManagerProxy considerably. Thanks for the comments! I think we have an agreement about this. We will modify the patch accordingly to add the support WebKit. We have also decided to rework the patchset, we are going to remove the capturers and realtimemediasource center from this patch as suggested by Youenn. That way we are not going to block in the enumerateDevices patch anymore to upload the getUserMedia patch implementation in a different patch. This patch is going to depend on the other one, that way we can add more code in this patch just focused in the support of the salt and persistent permissions code.
Created attachment 346900 [details] WIP This is WIP patch, we have landed the getUserMedia and the PeerConnection support in other patches to simplify this one, now it just includes the enumerateDevices implementation. After spending some time checking the options we have created this patch but we need help from Apple developers because we want to use multiport code and we don't know how Safari was using the API and what is more interesting to move forward with solution that we can share. Main points about the patch: - The main idea for this patch is to create a PermissionManager that can handle the permissions in the engine, the data structures it uses tries to support the Permissions spec, it does not include any API implementation of that spec though, it could in future patches. - It implements the WebsiteDataStore API, the browser can list and delete the permissions using that API, so we are not adding new API to the engine. GTK Minibrowser allows to test that in the about:data. I wonder if this API would be enough if we want to use this at some point to handle other permissions in the engine, maybe we would need a more specific API because we want to handle removing just one of the permissions for one origin. But we can leave this for the future if we think we can use it. - It adds PermissionDescription because the MediaPermissionDescription adds the deviceID and we don't support requesting the permissions per device, not sure if Safari supports this. - It continues using the UIClient.checkUserMediaPermissionForOrigin for other ports (just GTK and WPE implement it) using an ifdef, that way we don't change the behaviour of other ports. - It handles the in-memory granted and denied requests cache in the UserMediaPermissionRequestManagerProxy with the PermissionManager instead of creating 2 vectors, this is for every port. It changes subtly the way it works for COCOA, before the 2 vectors cache was reset more frequently, even when reloading. Now it is more persistent and we don't check the FrameID, so any tab can use the permissions defined previously by the user. Is this ok for Safari? - It does not include disk persistence, should we add it directly in the engine? It has some security related information such as if the webpage can access the media devices, and probably we would have to change the schema in the future, maybe some API to let the browser to do job would be a better option? We could also leave this for future patches and make the change incremental. Any review or opinion is welcome, we want to clarify these points for all the ports to make sure everyone is happy with the implementation or we should just be less ambitious and to go back to a port specific solution for this case.
Adding alex to help with the definition.
(In reply to Alejandro G. Castro from comment #21) > Created attachment 346900 [details] > WIP > > This is WIP patch, we have landed the getUserMedia and the PeerConnection > support in other patches to simplify this one, now it just includes the > enumerateDevices implementation. After spending some time checking the > options we have created this patch but we need help from Apple developers > because we want to use multiport code and we don't know how Safari was using > the API and what is more interesting to move forward with solution that we > can share. > > Main points about the patch: > - The main idea for this patch is to create a PermissionManager that can > handle the permissions in the engine, the data structures it uses tries to > support the Permissions spec, it does not include any API implementation of > that spec though, it could in future patches. > > - It implements the WebsiteDataStore API, the browser can list and delete > the permissions using that API, so we are not adding new API to the engine. > GTK Minibrowser allows to test that in the about:data. I wonder if this API > would be enough if we want to use this at some point to handle other > permissions in the engine, maybe we would need a more specific API because > we want to handle removing just one of the permissions for one origin. But > we can leave this for the future if we think we can use it. > > - It adds PermissionDescription because the MediaPermissionDescription > adds the deviceID and we don't support requesting the permissions per > device, not sure if Safari supports this. > No, we don't store state per device ID. > - It continues using the UIClient.checkUserMediaPermissionForOrigin for > other ports (just GTK and WPE implement it) using an ifdef, that way we > don't change the behaviour of other ports. > Thanks! > - It handles the in-memory granted and denied requests cache in the > UserMediaPermissionRequestManagerProxy with the PermissionManager instead of > creating 2 vectors, this is for every port. It changes subtly the way it > works for COCOA, before the 2 vectors cache was reset more frequently, even > when reloading. Now it is more persistent and we don't check the FrameID, so > any tab can use the permissions defined previously by the user. Is this ok > for Safari? > I think this will be fine, but I will talk to Youenn about this when he is back in the office next week. > - It does not include disk persistence, should we add it directly in the > engine? It has some security related information such as if the webpage can > access the media devices, and probably we would have to change the schema in > the future, maybe some API to let the browser to do job would be a better > option? We could also leave this for future patches and make the change > incremental. > I agree that there are pros and cons, so let's leave it out for now at least. > Any review or opinion is welcome, we want to clarify these points for all > the ports to make sure everyone is happy with the implementation or we > should just be less ambitious and to go back to a port specific solution for > this case. > At a high level I think this is a good approach. I will take a closer look at the patch in the next day or so. Thanks!
(In reply to Eric Carlson from comment #23) > (In reply to Alejandro G. Castro from comment #21) > > Created attachment 346900 [details] > > WIP > > > > - It handles the in-memory granted and denied requests cache in the > > UserMediaPermissionRequestManagerProxy with the PermissionManager instead of > > creating 2 vectors, this is for every port. It changes subtly the way it > > works for COCOA, before the 2 vectors cache was reset more frequently, even > > when reloading. Now it is more persistent and we don't check the FrameID, so > > any tab can use the permissions defined previously by the user. Is this ok > > for Safari? > > > I think this will be fine, but I will talk to Youenn about this when he is > back in the office next week. > Youenn reminded me that we initially reset cached permission on reload but web developers complained about this during initial testing because it was confusing that users had to close the tab to reset state, so we definitely *do* want to reset state on a page reload. I agree that we don't need to check FrameID. > > - It does not include disk persistence, should we add it directly in the > > engine? It has some security related information such as if the webpage can > > access the media devices, and probably we would have to change the schema in > > the future, maybe some API to let the browser to do job would be a better > > option? We could also leave this for future patches and make the change > > incremental. > > > I agree that there are pros and cons, so let's leave it out for now at > least. > After thinking about this, I do think that we want to be able to write user media state to disk when appropriate. I don't *think* it poses more of a security risk to have WebKit do this than to do it in the app. It will be fine to do this in a followup patch. > > Any review or opinion is welcome, we want to clarify these points for all > > the ports to make sure everyone is happy with the implementation or we > > should just be less ambitious and to go back to a port specific solution for > > this case. > > > At a high level I think this is a good approach. I will take a closer look > at the patch in the next day or so. Thanks! > I have a few very minor nits, but I think this is a great improvement overall.
Comment on attachment 346900 [details] WIP View in context: https://bugs.webkit.org/attachment.cgi?id=346900&action=review > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:465 > + if (dataTypes.contains(WebsiteDataType::Permissions) && m_permissionRequestManager) { NIt: I would switch the order of comparisons here, it is much cheaper to null-check than to search the set. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:800 > + if (dataTypes.contains(WebsiteDataType::Permissions) && m_permissionRequestManager) { Ditto. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1094 > + if (dataTypes.contains(WebsiteDataType::Permissions) && m_permissionRequestManager) { Ditto.
Thanks very much for the review! (In reply to Eric Carlson from comment #24) > (In reply to Eric Carlson from comment #23) > > (In reply to Alejandro G. Castro from comment #21) > > > Created attachment 346900 [details] > > > WIP > > > > > > - It handles the in-memory granted and denied requests cache in the > > > UserMediaPermissionRequestManagerProxy with the PermissionManager instead of > > > creating 2 vectors, this is for every port. It changes subtly the way it > > > works for COCOA, before the 2 vectors cache was reset more frequently, even > > > when reloading. Now it is more persistent and we don't check the FrameID, so > > > any tab can use the permissions defined previously by the user. Is this ok > > > for Safari? > > > > > I think this will be fine, but I will talk to Youenn about this when he is > > back in the office next week. > > > Youenn reminded me that we initially reset cached permission on reload but > web developers complained about this during initial testing because it was > confusing that users had to close the tab to reset state, so we definitely > *do* want to reset state on a page reload. > > Right, I understand, with this patch there is another option for developers to remove the permissions, namely using the WebsiteDataStore API, when testing the patch I was using about:data in the GTK Minibrowser which allows you to remove a permission for an origin or clear all of them. Anyway I'll add the reset on reload to avoid modifying the behaviour in that case. Just one doubt, how does that reset when reloading work if the permissions are stored in the disk? Do you remove all the permissions in the disk when reloading or closing the browser? > I agree that we don't need to check FrameID. > > > > - It does not include disk persistence, should we add it directly in the > > > engine? It has some security related information such as if the webpage can > > > access the media devices, and probably we would have to change the schema in > > > the future, maybe some API to let the browser to do job would be a better > > > option? We could also leave this for future patches and make the change > > > incremental. > > > > > I agree that there are pros and cons, so let's leave it out for now at > > least. > > > After thinking about this, I do think that we want to be able to write user > media state to disk when appropriate. I don't *think* it poses more of a > security > risk to have WebKit do this than to do it in the app. It will be fine to do > this > in a followup patch. > Great, thanks for the comment, I agree, I've checked other browsers use the general preferences json file in the home directory to store this. My main doubt here now is if we are stepping on browser's toes regarding how it wants to store the preferences in each system. > > > Any review or opinion is welcome, we want to clarify these points for all > > > the ports to make sure everyone is happy with the implementation or we > > > should just be less ambitious and to go back to a port specific solution for > > > this case. > > > > > At a high level I think this is a good approach. I will take a closer look > > at the patch in the next day or so. Thanks! > > > I have a few very minor nits, but I think this is a great improvement > overall. Thanks very much, I'll check the comments.
(In reply to Eric Carlson from comment #25) > Comment on attachment 346900 [details] > WIP > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346900&action=review > > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:465 > > + if (dataTypes.contains(WebsiteDataType::Permissions) && m_permissionRequestManager) { > > NIt: I would switch the order of comparisons here, it is much cheaper to > null-check than to search the set. > Right. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:800 > > + if (dataTypes.contains(WebsiteDataType::Permissions) && m_permissionRequestManager) { > > Ditto. > Right. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1094 > > + if (dataTypes.contains(WebsiteDataType::Permissions) && m_permissionRequestManager) { > > Ditto. Right. Thanks again for the review!
(In reply to Alejandro G. Castro from comment #26) > Right, I understand, with this patch there is another option for developers > to remove the permissions, namely using the WebsiteDataStore API, when > testing the patch I was using about:data in the GTK Minibrowser which allows > you to remove a permission for an origin or clear all of them. Anyway I'll > add the reset on reload to avoid modifying the behaviour in that case. Jumping into the middle of this discussion without the relevant background knowledge, so sorry if something I say here doesn't make sense: I don't think we would want to expose this as WebsiteDataStore API, since that API is designed for storing website data, not permissions. To see example UI you can look at Epiphany -> Preferences -> Stored Data -> Manage Personal Data. Not a good fit. Yes, it would be possible for clients to use it and construct an alternative UI for permission data types, but it's still just not a great fit. For permissions, we would currently use a WebKitPolicyDecision, and delegate persistent storage of the permission to the browser. E.g. Epiphany will store the permissions in GSettings backed by GKeyfile storage. That doesn't mean it's what we have to do for WebRTC, but there should be a reason if we deviate from that pattern.
> I don't think we would want to expose this as WebsiteDataStore API, since > that API is designed for storing website data, not permissions. To see > example UI you can look at Epiphany -> Preferences -> Stored Data -> Manage > Personal Data. Not a good fit. Yes, it would be possible for clients to use > it and construct an alternative UI for permission data types, but it's still > just not a great fit. WebsiteDataStore is currently the only way to tie user data with the concept of a browsing session, be it regular or ephemeral. It is true that "Epiphany -> Preferences -> Stored Data -> Manage Personal Data" might only want to expose some of the data types in WebsiteDataStore and some other UI would expose other kinds of WebsiteDataStore. That seems ok to me. > For permissions, we would currently use a WebKitPolicyDecision, and delegate > persistent storage of the permission to the browser. E.g. Epiphany will > store the permissions in GSettings backed by GKeyfile storage. That doesn't > mean it's what we have to do for WebRTC, but there should be a reason if we > deviate from that pattern. Note that the plan is to store a per-origin UUID (to be used as a salt) if persistent access is granted. Making the change would simplify apps as they would not need to duplicate code. That might also help simplifying WebKit flow as well.
@Michael - I agree with Youenn. The web API requires that device IDs must be unique per domain, and that they must persist across reloads when the user has persistent permission to use capture devices. If WebKit doesn't store the salt, we will require every application that uses WebKit will have store it and I fear that many will do it incorrectly.
Apologies for the overly-long comment. (In reply to Eric Carlson from comment #30) > @Michael - I agree with Youenn. The web API requires that device IDs must be > unique per domain, and that they must persist across reloads when the user > has persistent permission to use capture devices. > > If WebKit doesn't store the salt, we will require every application that > uses WebKit will have store it and I fear that many will do it incorrectly. OK, that's what I was missing... now I understand the motivation for this. Hmmm. Well, it certainly makes sense to store the device ID in WebsiteDataStore, since it is website data. I'm not sure about storing permission to use it, though. Let's think about the existing API for permission requests. Question 1: If the browser has a user interface for managing permissions (Epiphany does not currently, but we ought to) then that interface should work the same for user media permissions as for other permissions, which seems important. E.g. say the browser sees a WebKitUserMediaPermissionRequest, prompts the user for permission, receives a "yes", stores the result in the browser storage (that WebKit doesn't known about!) the same way it does for other permissions... and then the user opens the permissions dialog and changes the permission to deny. The browser author shouldn't need to know that special logic is required (open the WebsiteDataStore and delete a particular bit of WebsiteData... not very friendly). It should suffice to for the browser to just keep track of the changed permission, and respond with a No next time the permission request is requested. Question 2: If you use WebsiteDataStore to store the permission, how does the application implement the standard choice "yes, grant the permission, but only this once, don't store it"? The status quo when applications make permission decisions is that storage is handled by the application, so the simple use/ignore API of WebKitPermissionRequest is sufficient: WebKit never even knows whether the permission is stored or not, because the application handles that. So WebKit should still always create a permission request even if the device ID is stored, right? That way, browsers can reject the permission request even if it was previously accepted in the past. i.e. be sure that calling still webkit_permission_request_allow() means the request is allowed just once. The semantics of that should not change to allow future permission: that should require some new API. So my first thought was: the presence of a device ID in the WebsiteDataStore should not imply permission to use it without first asking the client. That seems like it would solve all these issues. But then I looked at the patch... I see it adds a generic PermissionManager class to WebKit, and it looks like that was a lot of work to implement. Oh no! Well, having a PermissionManager in WebKit is not necessarily a bad thing, if it could obsolete existing browser permission managers. So I don't want to suggest that we absolutely cannot do this... but if we do, we should be careful. It should really totally obsolete the current browser-level permissions stores (e.g. EphyPermissionsManager), so we don't have separate permissions stores in the browser and in WebKit, and it should do this from the beginning so that we don't end up in a half-finished transition state: (a) It should handle all of WebKitGeolocationPermissionRequest, WebKitInstallMissingMediaPluginsPermissionRequest, and WebKitNotificationPermissionRequest, not just WebKitUserMediaPermissionRequest. (b) What about permissions that WebKit itself does not know about? E.g. Epiphany needs to store whether credentials should be saved. So the API would need to be designed such as to allow applications to store arbitrary types of permissions. (A time-tested solution is for the API to operate on strings, and define some constants for specific strings, while allowing the browser to create its own permissions by just using its own string.) (c) There should be some API for enumerating permissions, so might as well bite the bullet and add WebKitPermissionManager to the public API. The API is good enough if EphyPermissionManager can be replaced with it. (d) The WebKitPermissionRequest API ought to be extended to allow tri-state decisions (yes/no/never) or perhaps separate yes/no and remember/forget. (Current API users should not notice any difference, so nothing should be stored unless the application requests remember.) A simple solution would be to add webkit_permission_request_remember(), which if called would permit persistent storage. I think with all of the above, it could replace browsers' current permission stores. And without all of the above, we'll wind up with permissions in two different places, which would be quite undesirable. We could also just as well throw up our hands and give up on the PermissionsManager, and just store the device IDs as website data while treating the permissions totally separately. Alex, what do you think would be best...?
Thanks for the extensive explanation Michael. (In reply to Michael Catanzaro from comment #31) > Apologies for the overly-long comment. > > (In reply to Eric Carlson from comment #30) > > @Michael - I agree with Youenn. The web API requires that device IDs must be > > unique per domain, and that they must persist across reloads when the user > > has persistent permission to use capture devices. > > > > If WebKit doesn't store the salt, we will require every application that > > uses WebKit will have store it and I fear that many will do it incorrectly. > > OK, that's what I was missing... now I understand the motivation for this. > Hmmm. Well, it certainly makes sense to store the device ID in > WebsiteDataStore, since it is website data. > > [...] > > I think with all of the above, it could replace browsers' current permission > stores. And without all of the above, we'll wind up with permissions in two > different places, which would be quite undesirable. > > We could also just as well throw up our hands and give up on the > PermissionsManager, and just store the device IDs as website data while > treating the permissions totally separately. > > Alex, what do you think would be best...? When I said I was concerned about stepping on browsers toes with this patch I was basically talking about what you are explaining. I agree with your points and personally I would be less ambitious with the patch because it would mean a big refactor task even inside Epiphany, which could be great but we are currently focused in a different topic. I don't think spending time on replacing the whole permissions manager is something very interesting nowadays. So my proposal considering what we have is: replace the PermissionManager with MediaPermissionCacheManager, that would refactor the cache we already have in the UserMediaPermissionRequestManagerProxy, which would make the code cleaner. That class would also handle the salt creation and management but will allow the browser to store it. That cache will be transient like the one we currently have there. Browser could check the PermissionCacheManager using the WebsiteDataStore, although reloading would be enough to remove the permissions/salt. Add the API to the GTK and WPE port for checking the permissions, that way the browser could decide to make it persistent and the engine would call that API if the PermissionCacheManager does not have the permission/salt for an origin, to check if the browser decided to store it in a previous session, if it doesn't it would generate it and deny access. I think Apple is currently doing this, they do not have the call for the enumerateDevices to the permissions cache but I think it is correct for all the ports to have it. The check-permissions API would be a way to allow checking if the browser made the salt and permission persistent for that origin. What do you think? Is this good enough for everyone? -
(In reply to Alejandro G. Castro from comment #32) > Thanks for the extensive explanation Michael. > > (In reply to Michael Catanzaro from comment #31) > > Apologies for the overly-long comment. > > > > (In reply to Eric Carlson from comment #30) > > > @Michael - I agree with Youenn. The web API requires that device IDs must be > > > unique per domain, and that they must persist across reloads when the user > > > has persistent permission to use capture devices. > > > > > > If WebKit doesn't store the salt, we will require every application that > > > uses WebKit will have store it and I fear that many will do it incorrectly. > > > > OK, that's what I was missing... now I understand the motivation for this. > > Hmmm. Well, it certainly makes sense to store the device ID in > > WebsiteDataStore, since it is website data. > > > > [...] > > > > I think with all of the above, it could replace browsers' current permission > > stores. And without all of the above, we'll wind up with permissions in two > > different places, which would be quite undesirable. > > > > We could also just as well throw up our hands and give up on the > > PermissionsManager, and just store the device IDs as website data while > > treating the permissions totally separately. > > > > Alex, what do you think would be best...? > > When I said I was concerned about stepping on browsers toes with this patch > I was basically talking about what you are explaining. I agree with your > points and personally I would be less ambitious with the patch because it > would mean a big refactor task even inside Epiphany, which could be great > but we are currently focused in a different topic. I don't think spending > time on replacing the whole permissions manager is something very > interesting nowadays. > I agree with this, we aren't interested in replacing Safari's permission manager either. > So my proposal considering what we have is: replace the PermissionManager > with MediaPermissionCacheManager, that would refactor the cache we already > have in the UserMediaPermissionRequestManagerProxy, which would make the > code cleaner. That class would also handle the salt creation and management > but will allow the browser to store it. That cache will be transient like > the one we currently have there. > I agree with this as well. > Browser could check the > PermissionCacheManager using the WebsiteDataStore, although reloading would > be enough to remove the permissions/salt. Add the API to the GTK and WPE > port for checking the permissions, that way the browser could decide to make > it persistent and the engine would call that API if the > PermissionCacheManager does not have the permission/salt for an origin, to > check if the browser decided to store it in a previous session, if it > doesn't it would generate it and deny access. > I was picturing something slightly different: - when a page calls getUserMedia, WebKit *always* checks with the host application to see if it allows persistent access for the domain - if it does allow persistent access, WebKit looks for a stored salt in a cache file for that domain (in e.g. WebsiteDataStore::resolvedMediaPermissionDirectory()). If there is no entry for that domain, it creates a salt and writes it to the cache file - if the host does not allow persistent access, it creates a new salt and deletes the cache file (if one exists) > I think Apple is currently > doing this, they do not have the call for the enumerateDevices to the > permissions cache but I think it is correct for all the ports to have it. > We currently call Safari to check for persistent access and to get the salt. This requires Safari to always generate the salt, and to store the permission bit and the salt when the user grants/denies persistent access. I suspect that most non-browser host applications will either want to always prompt or never prompt, so I would rather have the logic for generating and optionally storing the salt in WebKit.
We talked with Eric about this morning. We agreed we want to store the salt in WebSiteDataStore. Delegating the generation and storage to apps is not good since this is a getUserMedia implementation detail. Also it should be handled like cookies, in particular removed when clearing cookies. We need to store a boolean as well to remember the decision of persistent access. Only storing the salt is not sufficient since we would clear the salt when clearing cookies. Storing the boolean outside of WebsiteDataStore is consistent with other permission handling, that is probably the simple and easy path. The downside is that apps that remove persistent access should also make sure to remove the salt at the same time and apps will probably forget to do that. We can probably live with that and find some mitigations.
(In reply to youenn fablet from comment #34) > > [...] > > The downside is that apps that remove persistent access should also make > sure to remove the salt at the same time and apps will probably forget to do > that. We can probably live with that and find some mitigations. > If we *always* delete the salt for a domain when the user does not have persistent access as I suggest above, it won't matter much if an app doesn't remember to remove the salt when the permission changes.
(In reply to Eric Carlson from comment #33) > I was picturing something slightly different: > > - when a page calls getUserMedia, WebKit *always* checks with the host > application to see if it allows persistent access for the domain > > - if it does allow persistent access, WebKit looks for a stored salt in a > cache file for that domain (in e.g. > WebsiteDataStore::resolvedMediaPermissionDirectory()). If there is no entry > for that domain, it creates a salt and writes it to the cache file > > - if the host does not allow persistent access, it creates a new salt and > deletes the cache file (if one exists) This seems like a good plan to me. We would need a new API for the browser to hint to WebKit that the storage is persistent, since that's not currently possible via existing API (at least not for WPE/GTK). An example WPE/GTK API: webkit_permission_request_set_persistence_hint() or _set_is_persistent(). (We could add it to WebKitUserMediaPermissionRequest instead of WebKitPermissionRequest if we think it's not likely to be useful for other permission types -- probably not -- but it's so generic it's probably fine in the superclass?) We could avoid the need to add this new API if we instead always store the salt in the cache when permission is granted, and then only delete the cache file if permission is ever denied in the future. Either way would be OK. (And just an implementation reminder: nothing should be written to disk in ephemeral mode.) (In reply to Eric Carlson from comment #35) > If we *always* delete the salt for a domain when the user does not have > persistent access as I suggest above, it won't matter much if an app doesn't > remember to remove the salt when the permission changes. Because the salt will eventually be deleted when the user visits the website next and WebKit notices that permission is denied, right? So the salt can stick around indefinitely only so long as the user never visits the website again? That seems OK.
(In reply to Michael Catanzaro from comment #36) > > We would need a new API for the browser to hint to WebKit that the storage > is persistent, since that's not currently possible via existing API (at > least not for WPE/GTK). An example WPE/GTK API: > webkit_permission_request_set_persistence_hint() or _set_is_persistent(). > (We could add it to WebKitUserMediaPermissionRequest instead of > WebKitPermissionRequest if we think it's not likely to be useful for other > permission types -- probably not -- but it's so generic it's probably fine > in the superclass?) > We need the hash salt for enumerateDevices(), not getUserMedia(), so we need two requests. We currently have two on UIClient, decidePolicyForUserMediaPermissionRequest and checkUserMediaPermissionForOrigin, although both should be changed (decidePolicyForUserMediaPermissionRequest won't work for screen capture, and checkUserMediaPermissionForOrigin asks for the salt and the persistent access pref). > We could avoid the need to add this new API if we > instead always store the salt in the cache when permission is granted, and > then only delete the cache file if permission is ever denied in the future. > Either way would be OK. > I would rather not add the new the API so there is one less thing the host app is required to get right. > (And just an implementation reminder: nothing should be written to disk in > ephemeral mode.) > Absolutely! > (In reply to Eric Carlson from comment #35) > > If we *always* delete the salt for a domain when the user does not have > > persistent access as I suggest above, it won't matter much if an app doesn't > > remember to remove the salt when the permission changes. > > Because the salt will eventually be deleted when the user visits the website > next and WebKit notices that permission is denied, right? So the salt can > stick around indefinitely only so long as the user never visits the website > again? > Yes, exactly.
Thanks everyone for the proposals a comments! It has been very helpful. (In reply to Eric Carlson from comment #33) > I was picturing something slightly different: > > - when a page calls getUserMedia, WebKit *always* checks with the host > application to see if it allows persistent access for the domain > > - if it does allow persistent access, WebKit looks for a stored salt in a > cache file for that domain (in e.g. > WebsiteDataStore::resolvedMediaPermissionDirectory()). If there is no entry > for that domain, it creates a salt and writes it to the cache file > > - if the host does not allow persistent access, it creates a new salt and > deletes the cache file (if one exists) > I like this proposal from Eric for getUserMedia, this way we would leave the salt handling inside the engine, which makes a lot of sense because it is an implementation detail. Actually we could even not generate the salt until the enumerateDevices is called because we will be forced to check with the browser the permissions. The reason we need to always check with the browser for the enumerateDevices call is because the user could have changed the permission in the UI. Basically the call to enumerateDevices would be something like: - WebKit will *always* check with the host application for a call to enumerateDevices, actually we would have to remove the permissions cache vectors we have now in UserMediaPermissionRequestManagerProxy because there are situations where the user could have removed a permission in the UI of the browser but the vector we have is not updated, keeping a cache like this would mean adding API and I think the advantages are small. But checkUserMediaPermissionForOrigin would not request the salt to the browser, just the permission. - If the permission is granted get the salt from the salt storage or generate a new one if the user decided to remove it at some point. - If the permission is denied remove the salt from the storage if it is there and generate a new one, this way we don't even send the same list of device twice for the origins that do not have granted permissions. All the steps are important because we could have a situation where the user granted the getUserMedia but reverted that in the UI of the browser, and later the same origin could request the enumerateDevices. In that case the engine would have a salt but must ask the browser to know the current permissions, and it would be ok to return different IDs considering the user did something to revert the permissions. Also we will connect the cookies removal with the salt removal as proposed in the spec.
IIUC, the proposed model allows implementing the Firefox/Chrome behavior, which is to expose devices and persist device IDs even if getUserMedia access is not persistently granted. While this behavior does not seem great to me, I can live with that, especially since the salt would be flushed with cookies. The name of the callback should state its purpose which is to allow (or not) exposing capture devices by default to a given origin, not about persistent getUserMedia access. The choice for the app would be something like 'Always' or 'AfterFirstCapture'.
Thanks for the comment! (In reply to youenn fablet from comment #39) > IIUC, the proposed model allows implementing the Firefox/Chrome behavior, > which is to expose devices and persist device IDs even if getUserMedia > access is not persistently granted. While this behavior does not seem great > to me, I can live with that, especially since the salt would be flushed with > cookies. > It would not expose persistent IDs, using what Eric is proposing if the permission is not granted it would generate a new ID for every call to the enumerateDevices. And we will not send all the devices like we are doing right now, which if I understand correctly the spec I would say we should clarify it with the W3C working group or request a change because it says: " 2. If list-permission is not "granted", let filteredList be a copy of resultList, and all its elements, where the label member is the empty string." Not sure if there is a previous conversation about this but we are not sending all the elements. Anyway, this is out of the scope of this bug. > The name of the callback should state its purpose which is to allow (or not) > exposing capture devices by default to a given origin, not about persistent > getUserMedia access. The choice for the app would be something like 'Always' > or 'AfterFirstCapture'. The summary of what I understand checking the spec in this situation is that we have to: 1. If any of the local devices are attached to a live MediaStreamTrack we return granted without any other check. 2. If there is no local device producing data check the device-info permission, defined in the permission spec. That device-info permission is basically updated when the user requests permission to use the media devices [1], and it says we have to revoke if we change the IDs which fits with the policy we are designing to reset the IDs when the user denies the access. It does not say though we can not have a specific device-info value that the browser could update independently at any point, which I think it is the more flexible option, just that we have to update it when we have the permission granted for the media device. That means we can add API for deciding a policy for it, we could add something like: - decidePolicyForDeviceInfoPermissionForOrigin, which would request the browser permission manager that permission, the browser can decide to use the same or have a different value, or how to update it. Because we will always ask for it we just need a boolean because the next time we would ask and it is a browser call to create the UI to decide if it forever or just this time. This API will use a DeviceIDHashSaltStorage inside the engine and will just expose the WebsideDataStore API to check them and remove the hash salts. Considering the browser controls the permission manager we can not ask for 'Always' unless we would have a memory structure and API to reset it, and as I said I don't think we want to have a cache inside the engine for the permissions because it adds complexity and I don't see a clear advantage. If at some point we want to move the permission manager to the engine we would need a more complex plan checking all the implementations or creating a completely new API and objects, but I think it is out of the scope of what we want to do now with this bug. [1] "If result is "granted", the UA MUST set the "device-info" permission to "granted" for this realm." Permissions spec
(In reply to Alejandro G. Castro from comment #40) > Thanks for the comment! > > It would not expose persistent IDs, using what Eric is proposing if the > permission is not granted it would generate a new ID for every call to the > enumerateDevices. And we will not send all the devices like we are doing > right now, which if I understand correctly the spec I would say we should > clarify it with the W3C working group or request a change because it says: > I agree that we don’t want device IDs to persist unless the user has given the domain persistent (“no prompt) access to capture devices. > " 2. If list-permission is not "granted", let filteredList be a copy of > resultList, and all its elements, where the label member is the empty > string." > > Not sure if there is a previous conversation about this but we are not > sending all the elements. Anyway, this is out of the scope of this bug. > Yes, the pages does not have an active MediaStream or persistent access we don’t include device labels, but we also filter the list of devices to only include the number of capture devices that a “stock” machine has to avoid adding additional bits of fingerprinting. I talked with other browser engineers about this at the WebRTF face to face in Stockholm earlier this summer and some said they were considering adopting this behavior, but I am fine with making this a compile time option if other WebKit ports do not want to do this. > > The name of the callback should state its purpose which is to allow (or not) > > exposing capture devices by default to a given origin, not about persistent > > getUserMedia access. The choice for the app would be something like 'Always' > > or 'AfterFirstCapture'. > > The summary of what I understand checking the spec in this situation is that > we have to: > 1. If any of the local devices are attached to a live MediaStreamTrack we > return granted without any other check. > We *always* have to ask the host application if the user has granted persistent access, because that will cause us to return persistent device IDs. I’m not sure that what you say conflicts with this, just adding it for clarity.
(In reply to Eric Carlson from comment #41) > (In reply to Alejandro G. Castro from comment #40) > > Thanks for the comment! > > > > It would not expose persistent IDs, using what Eric is proposing if the > > permission is not granted it would generate a new ID for every call to the > > enumerateDevices. And we will not send all the devices like we are doing > > right now, which if I understand correctly the spec I would say we should > > clarify it with the W3C working group or request a change because it says: What I was meaning is that, since we are not tying the enumerateDevice decision to getUserMedia, an application could do: - Always return true to persisting enumerateDevice info - Prompt user for getUserMedia In such a case, we would end up persisting IDs as long as a page is not being denied access to getUserMedia. A potential countermeasure could be that the salt is made persistent only when a getUserMedia call actually succeeds for that origin. In the same vein, we might want to ensure that third-party iframes do not get enumerateDevices information too easily. It makes sense to me to restrict full enumerateDevices access to iframes that are allowed to call successfully getUserMedia. If origin A and origin B both have persistent access, an iframe B inside a top level page A cannot do a successful getUserMedia call except if page A allows getUserMedia on iframe B using feature policy.
(In reply to Eric Carlson from comment #41) > > > > The name of the callback should state its purpose which is to allow (or not) > > > exposing capture devices by default to a given origin, not about persistent > > > getUserMedia access. The choice for the app would be something like 'Always' > > > or 'AfterFirstCapture'. > > > > The summary of what I understand checking the spec in this situation is that > > we have to: > > 1. If any of the local devices are attached to a live MediaStreamTrack we > > return granted without any other check. > > > We *always* have to ask the host application if the user has granted > persistent access, because that will cause us to return persistent device > IDs. I’m not sure that what you say conflicts with this, just adding it for > clarity. Ok, I'll do that, I was understanding the spec added this situation where the engine does not ask because a live MediaStreamTrack means the user allowed and it is active, but it is true the user could have denied the device information. Maybe we can also tell W3C why they did add this. Thanks for the comments!
(In reply to youenn fablet from comment #42) > (In reply to Eric Carlson from comment #41) > > (In reply to Alejandro G. Castro from comment #40) > > > Thanks for the comment! > > > > > > It would not expose persistent IDs, using what Eric is proposing if the > > > permission is not granted it would generate a new ID for every call to the > > > enumerateDevices. And we will not send all the devices like we are doing > > > right now, which if I understand correctly the spec I would say we should > > > clarify it with the W3C working group or request a change because it says: > > What I was meaning is that, since we are not tying the enumerateDevice > decision to getUserMedia, an application could do: > - Always return true to persisting enumerateDevice info > - Prompt user for getUserMedia > In such a case, we would end up persisting IDs as long as a page is not > being denied access to getUserMedia. > A potential countermeasure could be that the salt is made persistent only > when a getUserMedia call actually succeeds for that origin. > I understand. We will consider how to control this situation. > In the same vein, we might want to ensure that third-party iframes do not > get enumerateDevices information too easily. It makes sense to me to > restrict full enumerateDevices access to iframes that are allowed to call > successfully getUserMedia. > > If origin A and origin B both have persistent access, an iframe B inside a > top level page A cannot do a successful getUserMedia call except if page A > allows getUserMedia on iframe B using feature policy. I agree, we added a vector of origins allowed inside the top level origin. On the other hand we have to check if some of this code should be in the permission manager, or we can control it inside the engine with the information we have. Thanks again for the comments!
Created attachment 351077 [details] Patch
After the discussion we had I've reworked the solution, this patch basically does not change anything regarding permissions, as we discussed the permission managers in the browsers control that. It does not change the behavior for COCOA, it is basically just for GTK+/WPE but adds code that could be used in the future by other ports to handle internally the hash salt as we have discussed. It also avoids adding new API for permissions, but adds a new type or permission request for glib. It adds API to control the directory where we store the hash salts, I want something with experience on that kind of API to review that part. It adds a new class DeviceIdHashSaltStorage that generates and stores the device ids hash salts per origin. The hash salts are persistent when the webpage is not ephemeral, they use a new directory where a file is stored with the SecurityOrigin id as the name of the file and the hash salt as the content of the file. The patch also adds a new type of data for the WebsiteDataStore. That way the users can call the WebsiteDataStore to show what origins have hash salt generated and remove them at some point. The DeviceHashSaltStorage class reads the information in disk when it is created and updates the disk storage when some modification is detected. It is also used as the delegate of the WebsiteDataStore API to remove the stored hash salts. For the moment just GTK+ and WPE ports are using this class to generate the hash salts, other ports compile the storage but they do not use it because the change should be synchronized with the current solution in the browser if you have it. For GTK+ and WPE the patch adds code to the checkUserMediaPermissionForOrigin API implementation, it was empty until now for these ports, we did not need to add a new one because this API did not require the salt already. In this function we create an instance of a new class WebKitDeviceInfoPermissionRequest that implements the WebKitPermissionRequestIface interface, that allows the ports to use the current permission managers implemented in the embedders to handle this new kind of request the way they like it. The default implementation is deny. The class WebKitDeviceInfoPermissionRequest takes care of contacting the DeviceIdHashSaltStorage and request/regenerate the hash salts accordingly. There is one addition to the general API to handle the location of the directory where the files are stored to make the hash salt persistent. Thanks everyone for the comments and help!!
Created attachment 351080 [details] Patch
Created attachment 351081 [details] Patch
Comment on attachment 351081 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351081&action=review I've just done a brief review of the new public API, and it looks sane to me. Finally I think this project is close to landing. ;) Note the EWS are sad. > Source/WebKit/UIProcess/API/glib/APIWebsiteDataStoreGLib.cpp:79 > + return websiteDataDirectoryFileSystemRepresentation(BASE_DIRECTORY G_DIR_SEPARATOR_S "deviceidhashsalts"); That's not the easiest to read. "devices"? "deviceids"? > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:114 > + // FIXME: store SecurityOrigins and made them available to the embedder. > + UNUSED_PARAM(userMediaDocumentOrigin); > + UNUSED_PARAM(topLevelDocumentOrigin); Should be easy, we already have a WebKitSecurityOrigin type in the public API, so you can just make them properties of the WebKitDeviceInfoPermissionRequest, and add getter functions for convenience.
Comment on attachment 351081 [details] Patch Patch looks to go in a good direction. Some comments/questions below. View in context: https://bugs.webkit.org/attachment.cgi?id=351081&action=review > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.cpp:61 > + configuration->m_deviceIdHashSaltsStorageDirectory = legacyConfiguration.deviceIdHashSaltsStorageDirectory; There should be no need for this field. The WebsiteDataStore should be configured directly to set the path. C API could be added to WKWebsiteDataStoreRef. > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:51 > + RefPtr<DeviceIdHashSaltStorage> deviceIdHashSaltStorage; Can they be Ref<> or do we need to allow copy constructor? > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:108 > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(UserMediaPermissionCheckProxy& request, API::SecurityOrigin& userMediaDocumentOrigin, API::SecurityOrigin& topLevelDocumentOrigin, DeviceIdHashSaltStorage* deviceIdHashSaltStorage) We probably only need topLevelDocumentOrigin here. userMediaDocumentOrigin should either be equal to topLevelDocumentOrigin or be allowed using feature policy on behalf of the top level origin. > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:14 > + * You shpould have received a copy of the GNU Lesser General Public s/shpould/should > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:42 > +Ref<DeviceIdHashSaltStorage> DeviceIdHashSaltStorage::create(const String& deviceIdHashSaltStorageDirectory, bool isPersistent) String&& > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:52 > + , m_deviceIdHashSaltStorageDirectory(deviceIdHashSaltStorageDirectory) WTFMove(...) Maybe it should be made a const String so that it is left unchanged (we use it in different threads). > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:67 > + Vector<String> originPaths = FileSystem::listDirectory(m_deviceIdHashSaltStorageDirectory, "*"); Use auto there. We should also probably not use origins as filenames as it might easily leak origins in logging. Instead, the file could have a random name and content would be origin+hashsalt, or use the deviceIdHashSalt as filename and origin as content. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:83 > + auto handle = FileSystem::openAndLockFile(originPath, FileSystem::FileOpenMode::Read); This seems like code that is very similar to ResourceLoadStatisticsPersistentStorage code. Maybe this code should be shared. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:108 > +void DeviceIdHashSaltStorage::storeHashSaltToDisk(HashSaltForOrigin* hashSaltForOrigin) HashSaltForOrigin& > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:115 > + FileSystem::PlatformFileHandle handle = FileSystem::openFile(fileFullPath, FileSystem::FileOpenMode::Write); Why do we lock above and not here? > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:119 > + int64_t writtenBytes = FileSystem::writeToFile(handle, hashSaltForOrigin->deviceIdentifierHashSaltForOrigin.utf8().data(), hashSaltForOrigin->deviceIdentifierHashSaltForOrigin.utf8().length()); s/int64_t/auto/ Maybe hashSaltForOrigin->deviceIdentifierHashSaltForOrigin.utf8().length() should be safe casted to an int to remove below static_cast. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:127 > +String DeviceIdHashSaltStorage::getDeviceIdentifierHashSaltForOrigin(UserMediaPermissionCheckProxy* request) UserMediaPermissionCheckProxy& and below. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:140 > + m_deviceIdHashSaltforOrigin.removeIf([topLevelDocumentSecurityOriginData](auto& keyAndValue) { [&topLevelDocumentSecurityOriginData] > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:179 > + if (deviceIdHashSaltforOrigin) { if (auto device..) or better use HashMap::ensure and set deviceIdentifierHashSaltForOrigin/lastTimeUsed. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:205 > +bool DeviceIdHashSaltStorage::deleteHashSaltFromDiskIfNeeded(HashSaltForOrigin* hashSaltForOrigin, bool needsRemoval) HashSaltForOrigin&. I do not really understand the goal of needsRemoval. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:226 > + m_deviceIdHashSaltforOrigin.removeIf([this, copiedOrigins](auto& keyAndValue) { &copiedOrigins > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:31 > +#include <WebCore/SecurityOriginHash.h> Probably do not need all these 3 > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:48 > + HashSaltForOrigin(WebCore::SecurityOrigin& securityOrigin, String& deviceIdentifierHashSaltForOrigin) Use SecurityOrigin&& and needs an indent. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:49 > + : topLevelDocumentSecurityOrigin(securityOrigin) Could probably be renamed to securityOrigin since we only care about topLevel document security origins and should remove the other origin from UI process code. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:54 > + ~HashSaltForOrigin() = default; Not needed. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:495 > + if (m_deviceIdHashSaltStorage && dataTypes.contains(WebsiteDataType::DeviceIdHashSalt)) { Nice to see all these. API tests would be great. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1105 > + if (m_deviceIdHashSaltStorage && dataTypes.contains(WebsiteDataType::DeviceIdHashSalt)) { We also want to enforce that device id hash salts are removed if cookies are removed. It could be let to applications but I feel like they will forget. Adding an additional check here should do the trick. It would be interesting to add a test also. That could be let to a follow-up patch. > LayoutTests/ChangeLog:17 > + http/tests/media/media-stream/resources/enumerate-devices-source-id-frame.html Nice to see that.
(In reply to Michael Catanzaro from comment #49) > Comment on attachment 351081 [details] > Patch > > [...] > > > Source/WebKit/UIProcess/API/glib/APIWebsiteDataStoreGLib.cpp:79 > > + return websiteDataDirectoryFileSystemRepresentation(BASE_DIRECTORY G_DIR_SEPARATOR_S "deviceidhashsalts"); > > That's not the easiest to read. > > "devices"? > > "deviceids"? > It could be hashsalts but maybe there could be other kind we would like to store in the future? > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:114 > > + // FIXME: store SecurityOrigins and made them available to the embedder. > > + UNUSED_PARAM(userMediaDocumentOrigin); > > + UNUSED_PARAM(topLevelDocumentOrigin); > > Should be easy, we already have a WebKitSecurityOrigin type in the public > API, so you can just make them properties of the > WebKitDeviceInfoPermissionRequest, and add getter functions for convenience. Yeah, and we don't need the userMedia. I can try to remove that. Thanks for the review.
Thanks for the review! (In reply to youenn fablet from comment #50) > Comment on attachment 351081 [details] > Patch > > Patch looks to go in a good direction. > Some comments/questions below. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351081&action=review > > > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.cpp:61 > > + configuration->m_deviceIdHashSaltsStorageDirectory = legacyConfiguration.deviceIdHashSaltsStorageDirectory; > > There should be no need for this field. > The WebsiteDataStore should be configured directly to set the path. > C API could be added to WKWebsiteDataStoreRef. > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:51 > > + RefPtr<DeviceIdHashSaltStorage> deviceIdHashSaltStorage; > > Can they be Ref<> or do we need to allow copy constructor? > We can't because we need and empty value when instantiating the class. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:108 > > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(UserMediaPermissionCheckProxy& request, API::SecurityOrigin& userMediaDocumentOrigin, API::SecurityOrigin& topLevelDocumentOrigin, DeviceIdHashSaltStorage* deviceIdHashSaltStorage) > > We probably only need topLevelDocumentOrigin here. > userMediaDocumentOrigin should either be equal to topLevelDocumentOrigin or > be allowed using feature policy on behalf of the top level origin. > Right, and actually they are both in the proxy so we don't need them. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:14 > > + * You shpould have received a copy of the GNU Lesser General Public > > s/shpould/should > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:42 > > +Ref<DeviceIdHashSaltStorage> DeviceIdHashSaltStorage::create(const String& deviceIdHashSaltStorageDirectory, bool isPersistent) > > String&& > In this case the directory String is owned by the WebsiteDataStore and we want to keep it like that. That is why we pass a reference explaining we don't want to own that String, later the object makes a copy of the value in an attribute. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:52 > > + , m_deviceIdHashSaltStorageDirectory(deviceIdHashSaltStorageDirectory) > > WTFMove(...) Ditto. > Maybe it should be made a const String so that it is left unchanged (we use > it in different threads). > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:67 > > + Vector<String> originPaths = FileSystem::listDirectory(m_deviceIdHashSaltStorageDirectory, "*"); > > Use auto there. Right. > We should also probably not use origins as filenames as it might easily leak > origins in logging. > Instead, the file could have a random name and content would be > origin+hashsalt, or use the deviceIdHashSalt as filename and origin as > content. > Good point, I'll try to use the salt. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:83 > > + auto handle = FileSystem::openAndLockFile(originPath, FileSystem::FileOpenMode::Read); > > This seems like code that is very similar to > ResourceLoadStatisticsPersistentStorage code. > Maybe this code should be shared. > I'll check if we can share some code, both open files and read them but it seems in a different way. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:108 > > +void DeviceIdHashSaltStorage::storeHashSaltToDisk(HashSaltForOrigin* hashSaltForOrigin) > > HashSaltForOrigin& > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:115 > > + FileSystem::PlatformFileHandle handle = FileSystem::openFile(fileFullPath, FileSystem::FileOpenMode::Write); > > Why do we lock above and not here? > Initially I thought it was not necessary but I think we can do it in both places. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:119 > > + int64_t writtenBytes = FileSystem::writeToFile(handle, hashSaltForOrigin->deviceIdentifierHashSaltForOrigin.utf8().data(), hashSaltForOrigin->deviceIdentifierHashSaltForOrigin.utf8().length()); > > s/int64_t/auto/ > Maybe hashSaltForOrigin->deviceIdentifierHashSaltForOrigin.utf8().length() > should be safe casted to an int to remove below static_cast. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:127 > > +String DeviceIdHashSaltStorage::getDeviceIdentifierHashSaltForOrigin(UserMediaPermissionCheckProxy* request) > > UserMediaPermissionCheckProxy& and below. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:140 > > + m_deviceIdHashSaltforOrigin.removeIf([topLevelDocumentSecurityOriginData](auto& keyAndValue) { > > [&topLevelDocumentSecurityOriginData] > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:179 > > + if (deviceIdHashSaltforOrigin) { > > if (auto device..) or better use HashMap::ensure and set > deviceIdentifierHashSaltForOrigin/lastTimeUsed. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:205 > > +bool DeviceIdHashSaltStorage::deleteHashSaltFromDiskIfNeeded(HashSaltForOrigin* hashSaltForOrigin, bool needsRemoval) > > HashSaltForOrigin&. Right > I do not really understand the goal of needsRemoval. > It is a way to use the initial condition for 2 things the check inside the function and returning a value for the lambda. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:226 > > + m_deviceIdHashSaltforOrigin.removeIf([this, copiedOrigins](auto& keyAndValue) { > > &copiedOrigins > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:31 > > +#include <WebCore/SecurityOriginHash.h> > > Probably do not need all these 3 > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:48 > > + HashSaltForOrigin(WebCore::SecurityOrigin& securityOrigin, String& deviceIdentifierHashSaltForOrigin) > > Use SecurityOrigin&& and needs an indent. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:49 > > + : topLevelDocumentSecurityOrigin(securityOrigin) > > Could probably be renamed to securityOrigin since we only care about > topLevel document security origins and should remove the other origin from > UI process code. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:54 > > + ~HashSaltForOrigin() = default; > > Not needed. > Right. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:495 > > + if (m_deviceIdHashSaltStorage && dataTypes.contains(WebsiteDataType::DeviceIdHashSalt)) { > > Nice to see all these. > API tests would be great. > Yep. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1105 > > + if (m_deviceIdHashSaltStorage && dataTypes.contains(WebsiteDataType::DeviceIdHashSalt)) { > > We also want to enforce that device id hash salts are removed if cookies are > removed. > It could be let to applications but I feel like they will forget. > Adding an additional check here should do the trick. > It would be interesting to add a test also. > That could be let to a follow-up patch. > Right, I'll add an or clause to make sure the cookies also remove the hash salts. > > LayoutTests/ChangeLog:17 > > + http/tests/media/media-stream/resources/enumerate-devices-source-id-frame.html > > Nice to see that. Yep. Thanks again for the review.
Created attachment 351526 [details] Patch
Created attachment 351531 [details] Patch
Created attachment 351533 [details] Patch
(In reply to Alejandro G. Castro from comment #55) > Created attachment 351533 [details] > Patch We found it is important to store the modification time in disk for the persistency, which is an addition to the last patch. To allow adding support for other platforms and avoid delaying more the patch and making it bigger, I have uploaded this last patch without persitency and we will add the persistency later.
Created attachment 351591 [details] Patch
Comment on attachment 351591 [details] Patch Some style nits and questions below. View in context: https://bugs.webkit.org/attachment.cgi?id=351591&action=review > Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:84 > +#if PLATFORM(COCOA) || PLATFORM(GTK) || PLATFORM(WPE) We might be able to remove all these #if since this is protected by MEDIA_STREAM compile flag. > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:63 > + WebKitDeviceInfoPermissionRequestPrivate* priv = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(request)->priv; auto* and maybe auto&? > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:70 > + auto salt = priv->deviceIdHashSaltStorage->getDeviceIdentifierHashSaltForOrigin(*priv->request.get()); s/.get()// It would be nice to make _WebKitDeviceInfoPermissionRequestPrivate take Ref<>. I do not see where priv is allocated/initialized, maybe this is a limitation of G API as you seem to point in your answer to my previous feedback. > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:85 > + auto salt = priv->deviceIdHashSaltStorage->regenerateDeviceIdentifierHashSaltForOrigin(*priv->request.get()); s/.get()// In case of deny, should we have to regenerate the hash salt? Should we instead just remove one if any and pass an empty salt? > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:108 > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(UserMediaPermissionCheckProxy& request, DeviceIdHashSaltStorage* deviceIdHashSaltStorage) Can deviceIdHashSaltStorage be null? Can we pass a reference? > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:110 > + WebKitDeviceInfoPermissionRequest* deviceInfoPermissionRequest = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(g_object_new(WEBKIT_TYPE_DEVICE_INFO_PERMISSION_REQUEST, nullptr)); auto? > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:113 > + deviceInfoPermissionRequest->priv->deviceIdHashSaltStorage = deviceIdHashSaltStorage; I am not sure how priv is created there but it would be nice to use Ref<> > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:23 > +#include "WebKitDeviceInfoPermissionRequest.h" Can we forward declare classes and remove these includes? > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:25 > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(WebKit::UserMediaPermissionCheckProxy&, WebKit::DeviceIdHashSaltStorage*); Should it return a reference? > Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:196 > + GRefPtr<WebKitDeviceInfoPermissionRequest> deviceInfoPermissionRequest = adoptGRef(webkitDeviceInfoPermissionRequestCreate(permissionRequest, page.websiteDataStore().deviceIdHashSaltStorage())); auto > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:48 > + return deviceIdHashSaltStorage; Change to one-liner. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:52 > + : m_queue(WorkQueue::create("com.apple.WebKit.DeviceIdHashSaltStorage")) The queue is probably not needed right now and will be useful when implementing the persistency? > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:56 > +DeviceIdHashSaltStorage::~DeviceIdHashSaltStorage() Use = default; ? > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:65 > +String DeviceIdHashSaltStorage::regenerateDeviceIdentifierHashSaltForOrigin(UserMediaPermissionCheckProxy& request) I am not totally clear whether we need this one and below or not. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:80 > +String DeviceIdHashSaltStorage::getDeviceIdentifierHashSaltForOrigin(SecurityOrigin& documentOrigin) We might need to make this async in the future as this might require loading data from the filesystem to initialize the map. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:95 > + return newHashSaltForOrigin; or return std::make_unique... > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:96 > + }); s/});/}).iterator->value/ to simplify below calls. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:112 > + deviceIdHashSaltForOrigin.iterator->value->deviceIdentifierHashSaltForOrigin = WTFMove(deviceIdentifierHashSalt); We can probably directly call m_deviceIdHashSaltforOrigin.set here instead of ensure. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:115 > +void DeviceIdHashSaltStorage::getDeviceIdHashSaltOrigins(Function<void(HashSet<SecurityOriginData>&&)>&& completionHandler) s/Function/CompletionHandler/ here and below. CompletionHandler asserts that it will be called at some point which is handy to catch errors. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:117 > + m_queue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable { I would simplify for now. We can probably update m_deviceIdHashSaltforOrigin in the main thread and do file access from a background thread. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:135 > + copiedOrigins.uncheckedAppend(origin.isolatedCopy()); Might be able to write it as copiedOrigins = origins.isolatedCopy() directly in the lambda > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:44 > + HashSaltForOrigin(WebCore::SecurityOrigin& securityOrigin, String&& deviceIdentifierHashSaltForOrigin) SecurityOrigin&& > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:58 > + void getDeviceIdHashSaltOrigins(Function<void(HashSet<WebCore::SecurityOriginData>&&)>&& completionHandler); s/ completionHandler// here and below > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:63 > + explicit DeviceIdHashSaltStorage(); No more need for explicit > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:65 > + String getDeviceIdentifierHashSaltForOrigin(WebCore::SecurityOrigin& documentOrigin); s/ documentOrigin// > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:99 > + , m_deviceIdHashSaltStorage(DeviceIdHashSaltStorage::create()) Do we need m_deviceIdHashSaltStorage to be ref-counted? Can it be a simple DeviceIdHashSaltStorage or optional<DeviceIdHashSaltStorage>? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:177 > + m_resolvedConfiguration.deviceIdHashSaltsStorageDirectory = resolveAndCreateReadWriteDirectoryForSandboxExtension(m_configuration.deviceIdHashSaltsStorageDirectory); Not needed right now, this might be left to the patch doing persistency. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:495 > + m_deviceIdHashSaltStorage->getDeviceIdHashSaltOrigins([callbackAggregator](HashSet<WebCore::SecurityOriginData>&& origins) { Could use auto&& although this is debatable probably. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:820 > + if (m_deviceIdHashSaltStorage && (dataTypes.contains(WebsiteDataType::DeviceIdHashSalt) || (dataTypes.contains(WebsiteDataType::Cookies)))) { Nice! An API test would be nice if possible. Could be added later. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:166 > + DeviceIdHashSaltStorage* deviceIdHashSaltStorage() { return m_deviceIdHashSaltStorage.get(); } Should ideally be a DeviceIdHashSaltStorage&
Thanks for the review! (In reply to youenn fablet from comment #58) > Comment on attachment 351591 [details] > Patch > > Some style nits and questions below. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351591&action=review > > > Source/WebCore/Modules/mediastream/MediaDevicesRequest.cpp:84 > > +#if PLATFORM(COCOA) || PLATFORM(GTK) || PLATFORM(WPE) > > We might be able to remove all these #if since this is protected by > MEDIA_STREAM compile flag. > Right, I'll try to do it. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:63 > > + WebKitDeviceInfoPermissionRequestPrivate* priv = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(request)->priv; > > auto* and maybe auto&? > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:70 > > + auto salt = priv->deviceIdHashSaltStorage->getDeviceIdentifierHashSaltForOrigin(*priv->request.get()); > > s/.get()// > It would be nice to make _WebKitDeviceInfoPermissionRequestPrivate take > Ref<>. > I do not see where priv is allocated/initialized, maybe this is a limitation > of G API as you seem to point in your answer to my previous feedback. > Right, yep, the gobject definition requests an empty value because the object is instantiated without it. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:85 > > + auto salt = priv->deviceIdHashSaltStorage->regenerateDeviceIdentifierHashSaltForOrigin(*priv->request.get()); > > s/.get()// > In case of deny, should we have to regenerate the hash salt? > Should we instead just remove one if any and pass an empty salt? > Right, regenerate removes the current salt and creates a new one. We need a non-empty random salt or the result would be empty, if we change the spec to allow empty result we could do it. Anyway, it is regenerated every time when denied so it is just a random value, not useful for fingerprinting. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:108 > > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(UserMediaPermissionCheckProxy& request, DeviceIdHashSaltStorage* deviceIdHashSaltStorage) > > Can deviceIdHashSaltStorage be null? Can we pass a reference? > It can be null because the WebsiteDataStore can be created without configuration, not sure what situation is that. I'll make sure we control the situation in this class. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:110 > > + WebKitDeviceInfoPermissionRequest* deviceInfoPermissionRequest = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(g_object_new(WEBKIT_TYPE_DEVICE_INFO_PERMISSION_REQUEST, nullptr)); > > auto? > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:113 > > + deviceInfoPermissionRequest->priv->deviceIdHashSaltStorage = deviceIdHashSaltStorage; > > I am not sure how priv is created there but it would be nice to use Ref<> > I tried already, we can't do it because the gobject first creates the object with null values, we would need an empty value for instantiating gobjects to use Refs. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:23 > > +#include "WebKitDeviceInfoPermissionRequest.h" > > Can we forward declare classes and remove these includes? > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:25 > > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(WebKit::UserMediaPermissionCheckProxy&, WebKit::DeviceIdHashSaltStorage*); > > Should it return a reference? > AFAIK for the gobject types I don't think we can do it, probably we should do it for all the code in the project if we try this kind of refactors. > > Source/WebKit/UIProcess/API/glib/WebKitUIClient.cpp:196 > > + GRefPtr<WebKitDeviceInfoPermissionRequest> deviceInfoPermissionRequest = adoptGRef(webkitDeviceInfoPermissionRequestCreate(permissionRequest, page.websiteDataStore().deviceIdHashSaltStorage())); > > auto > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:48 > > + return deviceIdHashSaltStorage; > > Change to one-liner. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:52 > > + : m_queue(WorkQueue::create("com.apple.WebKit.DeviceIdHashSaltStorage")) > > The queue is probably not needed right now and will be useful when > implementing the persistency? > Right, we can add the code handling the queue later. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:56 > > +DeviceIdHashSaltStorage::~DeviceIdHashSaltStorage() > > Use = default; ? > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:65 > > +String DeviceIdHashSaltStorage::regenerateDeviceIdentifierHashSaltForOrigin(UserMediaPermissionCheckProxy& request) > > I am not totally clear whether we need this one and below or not. > Right, it is an addition to structure the code but we can use just one. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:80 > > +String DeviceIdHashSaltStorage::getDeviceIdentifierHashSaltForOrigin(SecurityOrigin& documentOrigin) > > We might need to make this async in the future as this might require loading > data from the filesystem to initialize the map. > Right, we will add it in the persistency patch. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:95 > > + return newHashSaltForOrigin; > > or return std::make_unique... > Right, this code and other situations like this is divided because the persistency code was in the middle. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:96 > > + }); > > s/});/}).iterator->value/ to simplify below calls. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:112 > > + deviceIdHashSaltForOrigin.iterator->value->deviceIdentifierHashSaltForOrigin = WTFMove(deviceIdentifierHashSalt); > > We can probably directly call m_deviceIdHashSaltforOrigin.set here instead > of ensure. > Right, actually this was part of the persistency code, so we can remove it now and add it later. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:115 > > +void DeviceIdHashSaltStorage::getDeviceIdHashSaltOrigins(Function<void(HashSet<SecurityOriginData>&&)>&& completionHandler) > > s/Function/CompletionHandler/ here and below. > CompletionHandler asserts that it will be called at some point which is > handy to catch errors. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:117 > > + m_queue->dispatch([this, protectedThis = makeRef(*this), completionHandler = WTFMove(completionHandler)]() mutable { > > I would simplify for now. > We can probably update m_deviceIdHashSaltforOrigin in the main thread and do > file access from a background thread. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:135 > > + copiedOrigins.uncheckedAppend(origin.isolatedCopy()); > > Might be able to write it as > copiedOrigins = origins.isolatedCopy() directly in the lambda > Right, actually we don't need this if we remove the queue. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:44 > > + HashSaltForOrigin(WebCore::SecurityOrigin& securityOrigin, String&& deviceIdentifierHashSaltForOrigin) > > SecurityOrigin&& > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:58 > > + void getDeviceIdHashSaltOrigins(Function<void(HashSet<WebCore::SecurityOriginData>&&)>&& completionHandler); > > s/ completionHandler// here and below > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:63 > > + explicit DeviceIdHashSaltStorage(); > > No more need for explicit > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:65 > > + String getDeviceIdentifierHashSaltForOrigin(WebCore::SecurityOrigin& documentOrigin); > > s/ documentOrigin// > Right. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:99 > > + , m_deviceIdHashSaltStorage(DeviceIdHashSaltStorage::create()) > > Do we need m_deviceIdHashSaltStorage to be ref-counted? > Can it be a simple DeviceIdHashSaltStorage or > optional<DeviceIdHashSaltStorage>? > It can't because WebsiteDataStore returns references to the DeviceIdHashSaltStorage, the reference is stored in the request until it is used to generate the hash salts. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:177 > > + m_resolvedConfiguration.deviceIdHashSaltsStorageDirectory = resolveAndCreateReadWriteDirectoryForSandboxExtension(m_configuration.deviceIdHashSaltsStorageDirectory); > > Not needed right now, this might be left to the patch doing persistency. > Right. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:495 > > + m_deviceIdHashSaltStorage->getDeviceIdHashSaltOrigins([callbackAggregator](HashSet<WebCore::SecurityOriginData>&& origins) { > > Could use auto&& although this is debatable probably. > Right. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:820 > > + if (m_deviceIdHashSaltStorage && (dataTypes.contains(WebsiteDataType::DeviceIdHashSalt) || (dataTypes.contains(WebsiteDataType::Cookies)))) { > > Nice! > An API test would be nice if possible. Could be added later. > Right, I'll add a new case in the API test I already added. > > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:166 > > + DeviceIdHashSaltStorage* deviceIdHashSaltStorage() { return m_deviceIdHashSaltStorage.get(); } > > Should ideally be a DeviceIdHashSaltStorage& I agree, but like in the case of the storageManager we need to send references to other classes and it can be null in some situations, that makes RefPtr the best option. Thanks again for the review!
Created attachment 351952 [details] Patch
Comment on attachment 351952 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=351952&action=review > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:35 > +#include <wtf/text/WTFString.h> Some of these includes are not needed right now. WTFString.h is probably already included in FileSystem.h or StrginBuilder.h > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:50 > +String DeviceIdHashSaltStorage::getDeviceIdHashSaltForOrigin(UserMediaPermissionCheckProxy& request) s/getDeviceIdHashSaltForOrigin/deviceIdHashSaltForOrigin/ Ditto below. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:67 > +String DeviceIdHashSaltStorage::getDeviceIdHashSaltForOrigin(SecurityOrigin& documentOrigin) can it return a const String& > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:95 > + RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable { For now, we are only main thread based, so we can directly call completionHandler. If we start do these things in a background thread, we might need to isolate copy origins. > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:106 > + RunLoop::main().dispatch(WTFMove(completionHandler)); completionHandler() for now? > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:65 > + String getDeviceIdHashSaltForOrigin(WebCore::SecurityOrigin&); I wonder whether we should not expose directly this one and not the one taking a UserMediaPermissionCheckProxy.
v> > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:108 > > > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(UserMediaPermissionCheckProxy& request, DeviceIdHashSaltStorage* deviceIdHashSaltStorage) > > > > Can deviceIdHashSaltStorage be null? Can we pass a reference? > > > It can be null because the WebsiteDataStore can be created without configuration, not > sure what situation is that. I'll make sure we control the situation in this class. I wonder whether in that case we could not create a default DeviceIdHashSaltStorage that would not store persistently the hashes?
Thanks for the review! (In reply to youenn fablet from comment #61) > Comment on attachment 351952 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=351952&action=review > > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:35 > > +#include <wtf/text/WTFString.h> > > Some of these includes are not needed right now. > WTFString.h is probably already included in FileSystem.h or StrginBuilder.h > Right, leftovers from the persistence part of the patch. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:50 > > +String DeviceIdHashSaltStorage::getDeviceIdHashSaltForOrigin(UserMediaPermissionCheckProxy& request) > > s/getDeviceIdHashSaltForOrigin/deviceIdHashSaltForOrigin/ > Ditto below. > Right. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:67 > > +String DeviceIdHashSaltStorage::getDeviceIdHashSaltForOrigin(SecurityOrigin& documentOrigin) > > can it return a const String& > I'll try it, setUserMediaAccessInfo does not add the const but maybe compiler can solve it. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:95 > > + RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable { > > For now, we are only main thread based, so we can directly call > completionHandler. > If we start do these things in a background thread, we might need to isolate > copy origins. > Unfourtunately we can't, the WebsiteDataStore call aggregator is thread sensitive code and we have to make sure it runs in the main one. Every class implementing it is doing it, even if we don't use the queue. I did a small test and we get inmediate crashes. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.cpp:106 > > + RunLoop::main().dispatch(WTFMove(completionHandler)); > > completionHandler() for now? > Ditto. > > Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h:65 > > + String getDeviceIdHashSaltForOrigin(WebCore::SecurityOrigin&); > > I wonder whether we should not expose directly this one and not the one > taking a UserMediaPermissionCheckProxy. Right, we can remove the other one and force the caller to get the SecurityOrigin.
(In reply to youenn fablet from comment #62) > v> > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:108 > > > > +WebKitDeviceInfoPermissionRequest* webkitDeviceInfoPermissionRequestCreate(UserMediaPermissionCheckProxy& request, DeviceIdHashSaltStorage* deviceIdHashSaltStorage) > > > > > > Can deviceIdHashSaltStorage be null? Can we pass a reference? > > > > > > It can be null because the WebsiteDataStore can be created without configuration, not > > sure what situation is that. I'll make sure we control the situation in this class. > > I wonder whether in that case we could not create a default > DeviceIdHashSaltStorage that would not store persistently the hashes? This could be a good option to try to avoid the pointers, I can try this in a follow-up patch. Thanks the suggestion.
Created attachment 352030 [details] Patch for landing
Comment on attachment 352030 [details] Patch for landing Clearing flags on attachment: 352030 Committed r237031: <https://trac.webkit.org/changeset/237031>
All reviewed patches have been landed. Closing bug.
<rdar://problem/45189218>
Comment on attachment 352030 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=352030&action=review I'm sorry to be late here, but I have a few comments. All the new API here was added without updating the documentation files and without adding new heqaders to the main one. Please remember https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:45 > + * Since: 2.22 2.24 > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:64 > + auto& priv = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(request)->priv; auto* > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:84 > + auto& priv = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(request)->priv; auto* > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:27 > +typedef struct _WebKitDeviceInfoPermissionRequest WebKitDeviceInfoPermissionRequest; > + > +namespace WebKit { > +class DeviceIdHashSaltStorage; > +}; We normally just include the required headers in API private headers. > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:740 > + // We have to remove the hash salts when cookies are removed. > + auto dataTypes = webkit_website_data_get_types(data); > + if (dataTypes & WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT) > + dataTypes = static_cast<WebKitWebsiteDataTypes>(dataTypes | WEBKIT_WEBSITE_DATA_COOKIES); I think this should be done by WebsiteDataStore in cross-platform code. > Source/WebKit/UIProcess/API/gtk/WebKitDeviceInfoPermissionRequest.h:59 > +webkit_device_info_permission_request_get_type (void); Extra spaces between function name and ( > Source/WebKit/UIProcess/API/gtk/WebKitWebsiteData.h:47 > + * @WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT: Hash salt used to generate the device ids used by webpages. Since 2.24 > Source/WebKit/UIProcess/API/wpe/WebKitWebsiteData.h:47 > + * @WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT: Hash salt used to generate the device ids used by webpages. Since 2.24 > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebsiteData.cpp:66 > + const char* enumerateDevicesHTML = "<html><body onload=\"navigator.mediaDevices.enumerateDevices().then(function(devices) { document.title = 'Finished'; })\"></body></html>"; static > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:53 > + WebKitSettings* webkitSettings = webkit_settings_new(); > + webkit_settings_set_enable_media_stream(webkitSettings, TRUE); Do we really want this for all the tests? I think tests needing this should enable this setting themselves. > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:59 > + "settings", webkitSettings, Settings are leaked, you should unref after g_object_new.
Committed r238371: <https://trac.webkit.org/changeset/238371>
Thanks for the comments Carlos! I'll fix them in a follow-up pach. (In reply to Carlos Garcia Campos from comment #69) > Comment on attachment 352030 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=352030&action=review > > I'm sorry to be late here, but I have a few comments. All the new API here > was added without updating the documentation files and without adding new > heqaders to the main one. Please remember > https://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:45 > > + * Since: 2.22 > > 2.24 > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:64 > > + auto& priv = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(request)->priv; > > auto* > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequest.cpp:84 > > + auto& priv = WEBKIT_DEVICE_INFO_PERMISSION_REQUEST(request)->priv; > > auto* > Right. > > Source/WebKit/UIProcess/API/glib/WebKitDeviceInfoPermissionRequestPrivate.h:27 > > +typedef struct _WebKitDeviceInfoPermissionRequest WebKitDeviceInfoPermissionRequest; > > + > > +namespace WebKit { > > +class DeviceIdHashSaltStorage; > > +}; > > We normally just include the required headers in API private headers. > Right. > > Source/WebKit/UIProcess/API/glib/WebKitWebsiteDataManager.cpp:740 > > + // We have to remove the hash salts when cookies are removed. > > + auto dataTypes = webkit_website_data_get_types(data); > > + if (dataTypes & WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT) > > + dataTypes = static_cast<WebKitWebsiteDataTypes>(dataTypes | WEBKIT_WEBSITE_DATA_COOKIES); > > I think this should be done by WebsiteDataStore in cross-platform code. > Unfortunately we can't because in this method we are doing a filtering before calling inside the websiteDataStore. We could try to analyze if there is a better way to do all the method in general. > > Source/WebKit/UIProcess/API/gtk/WebKitDeviceInfoPermissionRequest.h:59 > > +webkit_device_info_permission_request_get_type (void); > > Extra spaces between function name and ( > Right. > > Source/WebKit/UIProcess/API/gtk/WebKitWebsiteData.h:47 > > + * @WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT: Hash salt used to generate the device ids used by webpages. > > Since 2.24 > Right. > > Source/WebKit/UIProcess/API/wpe/WebKitWebsiteData.h:47 > > + * @WEBKIT_WEBSITE_DATA_DEVICE_ID_HASH_SALT: Hash salt used to generate the device ids used by webpages. > > Since 2.24 > Right. > > Tools/TestWebKitAPI/Tests/WebKitGLib/TestWebsiteData.cpp:66 > > + const char* enumerateDevicesHTML = "<html><body onload=\"navigator.mediaDevices.enumerateDevices().then(function(devices) { document.title = 'Finished'; })\"></body></html>"; > > static > Right. > > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:53 > > + WebKitSettings* webkitSettings = webkit_settings_new(); > > + webkit_settings_set_enable_media_stream(webkitSettings, TRUE); > > Do we really want this for all the tests? I think tests needing this should > enable this setting themselves. > Right. > > Tools/TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:59 > > + "settings", webkitSettings, > > Settings are leaked, you should unref after g_object_new. This was already fixed here: https://trac.webkit.org/changeset/237795/webkit Thanks again for the review!