Bug 174500

Summary: WebRTC data channel only applications require capture permissions for direct connections
Product: WebKit Reporter: Lennart Grahl <lennart.grahl>
Component: WebRTCAssignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, ashley, ben.browitt, cdumez, commit-queue, damien, db, eric.carlson, ews, jana, jonlee, juberti, pascal, philipp.kuederli, rniwa, ryanhaddad, sebastian.schenk, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: Safari Technology Preview   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch for landing
none
Patch none

Description Lennart Grahl 2017-07-14 05:34:18 PDT
Currently, ICE candidates of type 'host' will not be gathered/handed out to the application in case no capture permission has been granted. While this may be a perfect solution for audio/video, it leaves data channel only applications with either srflx/relay only or with the need for an unfitting permission request.

The biggest issue with this approach is: How do you explain to the user that you need capture permissions for a direct peer-to-peer connection? That smells fishy and I understand everyone who will decline such a request for this exact reason.

So, if the user rejects this permission request, secondary issues are that you
a) now always need to provide a STUN server, and
b) always need to provide a TURN server (which itself is rather expensive in terms of operating costs and may not be feasible for small open source applications for this reason) because there are plenty of routers who are not capable of doing NAT loopback (yep, even models from 2017). Thus, one cannot rely on server reflexive candidates.

My proposal would be that the application can request a permission to "establish a direct connection" (maybe also displaying some hints on what this means towards privacy, etc.) which enables the use of host candidates.

(P.S.: You should probably add a WebRTC component to your Bugzilla.)
Comment 1 Radar WebKit Bug Importer 2017-08-29 09:44:45 PDT
<rdar://problem/34134281>
Comment 2 Ben 2017-09-03 03:56:39 PDT
Another use case is a viewer in a webinar that only receive audio/video tracks but doesn't transmit. Requesting mic/cam permission might confuse a user that is only intended to be a viewer.

In addition to the proposed additional permission request type I suggest that if the website is saved to the home screen in iOS the restrictions will be relaxed.
Comment 3 Jon Lee 2017-09-19 19:18:14 PDT
*** Bug 176921 has been marked as a duplicate of this bug. ***
Comment 4 sebastian.schenk 2017-09-26 02:38:38 PDT
In general I wouldn't assume that a WebRTC connection is always bi-directional. In my use case I stream multiple live videos from a server to the client. In this scenario I'm using VPN without any NATs. I also would like to avoid using a TURN server.

As @Lennart Grahl already mentioned, asking the user for cam/mic permission without even really using it is definitely not a good work around.
Comment 5 Jana 2017-12-01 10:02:24 PST
As @Ben noted, the important use case of streaming video to a user via WebRTC doesn't fit with the permissions request required. To tell a user that they have to grant access to their camera/mic doesn't make any sense when all they are trying to do is view a video without sending any media back. No user will understand why they have to give permission in this scenario. Basically, getUserMedia shouldn't require camera/mic permissions when the data is not bidirectional. Developers should be able to specify that the data is streaming to the user only, and therefore doesn't require permission. If you decide that it's necessary to request permission, the permission should be fit for the purpose rather than requiring them to grant access to permissions for components that aren't being used.
Comment 6 youenn fablet 2018-03-27 22:12:40 PDT
Created attachment 336643 [details]
Patch
Comment 7 Lennart Grahl 2018-03-28 07:05:54 PDT
(In reply to youenn fablet from comment #6)
> Created attachment 336643 [details]
> Patch

Youenn,

using mDNS with UUIDs seems like a clever approach to me and is definitely a step forward which I appreciate. If you plan on suggesting to specify this in ICE (or as an extension to ICE) - great! If not, then this is a proprietary solution limited to Webkit.

Cheers
Lennart
Comment 8 youenn fablet 2018-03-28 08:51:54 PDT
> using mDNS with UUIDs seems like a clever approach to me and is definitely a
> step forward which I appreciate. If you plan on suggesting to specify this
> in ICE (or as an extension to ICE) - great! If not, then this is a
> proprietary solution limited to Webkit.

This is an experiment. If proved to be successful, we hope it could be made interoperable with other browsers.
Comment 9 youenn fablet 2018-03-28 09:50:32 PDT
Created attachment 336667 [details]
Patch
Comment 10 youenn fablet 2018-03-28 10:01:22 PDT
Created attachment 336670 [details]
Patch
Comment 11 youenn fablet 2018-03-28 10:24:52 PDT
Created attachment 336673 [details]
Patch
Comment 12 Build Bot 2018-03-28 11:29:52 PDT
Comment on attachment 336673 [details]
Patch

Attachment 336673 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/7126070

New failing tests:
webrtc/datachannel/mdns-ice-candidates.html
Comment 13 Build Bot 2018-03-28 11:29:53 PDT
Created attachment 336681 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 14 youenn fablet 2018-03-28 11:46:03 PDT
Created attachment 336685 [details]
Patch
Comment 15 Build Bot 2018-03-28 13:28:20 PDT
Comment on attachment 336685 [details]
Patch

Attachment 336685 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7127141

New failing tests:
webrtc/datachannel/mdns-ice-candidates.html
Comment 16 Build Bot 2018-03-28 13:28:21 PDT
Created attachment 336709 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6
Comment 17 youenn fablet 2018-03-28 14:01:10 PDT
Created attachment 336716 [details]
Patch
Comment 18 youenn fablet 2018-03-28 14:16:53 PDT
Created attachment 336719 [details]
Patch
Comment 19 youenn fablet 2018-03-28 14:29:45 PDT
Created attachment 336720 [details]
Patch
Comment 20 youenn fablet 2018-03-28 20:51:58 PDT
Putting under review.
It seems that on iOS simulator the 250 ms MDNS timeout might be too small.
Comment 21 Eric Carlson 2018-03-30 07:30:39 PDT
Comment on attachment 336720 [details]
Patch

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

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:289
> +            provider.resolveMDNSName(name, [peerConnection = makeRef(m_peerConnection), this, name, iceCandidate = makeRef(*iceCandidate), promise = WTFMove(promise)] (auto&& result) mutable {

Nit: I think "auto" is not a good choice here because the only way for someone that isn't already familiar with the code to figure out "result"'s type is to dig through the libWebRTCProvider header.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:295
> +                    // FIXME: Log error.

It is probably worth logging the error to the console in this patch, even if that isn't the long term solution.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:435
> +        // FIXME: We might need to clear al pending candidates when setting again local description.

Nits: "al" => "all"
"when setting again local description" => "when setting local description again"

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:467
> +    provider.registerMDNSName(document, ipAddress, [peerConnection = makeRef(m_peerConnection), this, ipAddress] (auto&& result) {

Ditto the "auto" comment above.

> Source/WebCore/Modules/mediastream/PeerConnectionBackend.cpp:473
> +            // FIXME: Log error.

Ditto the comment above about logging the error.

> Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:78
> +    virtual void unregisterMDNSNames(Document&) { }
> +
> +    virtual void registerMDNSName(Document& document, const String& ipAddress, CompletionHandler<void(MDNSNameOrError&&)>&& callback)

It is a layering violation to use Document here. Can you just use the identifier?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:80
> +        request->connection->send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Why not log the error here, or pass the error value to be logged in the web process?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:92
> +            m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:96
> +            m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:104
> +    String baseName = createCanonicalUUIDString();

Nit: is the local variable necessary?

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:109
> +    if (ip == ( in_addr_t)(-1)) {

Nit: unnecessary space after the "(".

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:110
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto the comment above about logging or passing a meaningful error to the web process.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:129
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedRegisteringMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:154
> +        connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:173
> +        request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:181
> +        request->connection->send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { request->requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:193
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:207
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:214
> +        m_connection.connection().send(Messages::WebMDNSRegister::FinishedResolvingMDNSName { requestIdentifier, makeUnexpected(MDNSRegisterError::General) }, 0);

Ditto.

> Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:88
> +        finishedRegisteringMDNSName(m_pendingRequestsIdentifier, makeUnexpected(MDNSRegisterError::General));

Ditto.

> Source/WebKit/WebProcess/Network/webrtc/WebMDNSRegister.cpp:97
> +        finishedResolvingMDNSName(m_pendingRequestsIdentifier, makeUnexpected(MDNSRegisterError::General));

Ditto.
Comment 22 youenn fablet 2018-04-04 16:28:49 PDT
Created attachment 337239 [details]
Patch for landing
Comment 23 Build Bot 2018-04-04 16:30:21 PDT
Attachment 337239 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Network/webrtc/LibWebRTCProvider.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:60:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
ERROR: Source/WebKit/NetworkProcess/webrtc/NetworkMDNSRegister.cpp:95:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 3 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 youenn fablet 2018-04-04 16:30:37 PDT
Thanks for the review, I took it all.
Next step is integrating the MDNS resolution at some lower level so that we do not add this unnecessary delay, ideally at libwebrtc level, otherwise at the WebCore/libwebrtc binding level.
Comment 25 youenn fablet 2018-04-04 17:14:10 PDT
Created attachment 337243 [details]
Patch for landing
Comment 26 Build Bot 2018-04-04 17:17:19 PDT
Attachment 337243 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/Network/webrtc/LibWebRTCProvider.cpp:49:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCProvider.h:60:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 2 in 37 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 27 WebKit Commit Bot 2018-04-04 19:05:07 PDT
Comment on attachment 337243 [details]
Patch for landing

Clearing flags on attachment: 337243

Committed r230290: <https://trac.webkit.org/changeset/230290>
Comment 28 WebKit Commit Bot 2018-04-04 19:05:09 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 youenn fablet 2018-04-05 10:17:06 PDT
Reopening to attach new patch.
Comment 30 youenn fablet 2018-04-05 10:17:06 PDT
Created attachment 337275 [details]
Patch
Comment 31 WebKit Commit Bot 2018-04-05 10:43:40 PDT
Comment on attachment 337275 [details]
Patch

Clearing flags on attachment: 337275

Committed r230307: <https://trac.webkit.org/changeset/230307>
Comment 32 WebKit Commit Bot 2018-04-05 10:43:42 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Lennart Grahl 2018-04-06 12:43:53 PDT
So, was the experiment a success? Would you like to pursue this further and incorporate it into the spec?
Comment 34 youenn fablet 2018-06-07 14:47:42 PDT
*** Bug 186302 has been marked as a duplicate of this bug. ***