1005152 - Update fingerprint handling for identity

Status: RESOLVED FIXED

(Core :: WebRTC, defect)

Description

[Martin Thomson [:mt:]]

New specs for identity include the ability to include multiple fingerprints under an assertion.  That changes the protocol syntax and the way that we check for a match.  PeerConnectionIdp.jsm and the test IdP need to be updated to allow for this, even if we don't intend to use the feature.

Comment 1

[Martin Thomson [:mt:]]

Attachment: Permitting the use of multiple fingerprints

Comment 2

[Martin Thomson [:mt:]]

Attachment: Testing the addition of multiple fingerprints

Comment 3

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8443624 [details] [diff] [review]
Permitting the use of multiple fingerprints

Review of attachment 8443624 [details] [diff] [review]:
-----------------------------------------------------------------

r=me.

::: dom/media/PeerConnectionIdp.jsm
@@ +81,5 @@
>      this._dispatchEvent(ev);
>    },
>  
> +  _getFingerprintsFromSdp: function(sdp) {
> +    let fingerprints = {};

Why not let fingerprints = []; ?

@@ +83,5 @@
>  
> +  _getFingerprintsFromSdp: function(sdp) {
> +    let fingerprints = {};
> +    let m = sdp.match(PeerConnectionIdp._fingerprintPattern);
> +    while (m) {

while(!!(m = sdp.match(PeerConnectionIdp._fingerprintPattern)))

would save you two lines. The obvious one,

@@ +86,5 @@
> +    let m = sdp.match(PeerConnectionIdp._fingerprintPattern);
> +    while (m) {
> +      fingerprints[m[0]] = { algorithm: m[1], digest: m[2] };
> +      sdp = sdp.substring(m.index + m[0].length);
> +      m = sdp.match(PeerConnectionIdp._fingerprintPattern);

and this one (I concede that whether this is a win may be subjective).

@@ +91,3 @@
>      }
>  
> +    return Object.keys(fingerprints).map(k => fingerprints[k]);

Again, why not have fingerprints be an array to begin with?

@@ +200,5 @@
>          if (error) {
>            warn(error);
>          } else {
>            return true;
>          }

I find the if-else logic a little hard to follow here, though that arguably precedes this patch. Would a fail-and-bail pattern perhaps read better?

Comment 4

[Martin Thomson [:mt:]]

(In reply to Jan-Ivar Bruaroey [:jib] from comment #3)
> Why not let fingerprints = []; ?

Because I'm relying on the map behaviour of objects.  Namely, that repeated calls to f[x] results in just one value being recorded.  Otherwise I'd need to sort|uniq the result when multiple identical fingerprints are present.

> while(!!(m = sdp.match(PeerConnectionIdp._fingerprintPattern)))
> 
> would save you two lines. The obvious one,

I know, and it's really quite tempting.  But it has an assignment in a place I usually try to avoid having an assignment.  Given that it doesn't even scope `m` to the block, it seemed like a poor trade to me.

> @@ +200,5 @@
> >          if (error) {
> >            warn(error);
> >          } else {
> >            return true;
> >          }
> 
> I find the if-else logic a little hard to follow here, though that arguably
> precedes this patch. Would a fail-and-bail pattern perhaps read better?

I'm in two minds on this too.  Fail and bail is the same number of lines, but no `else`.  I think that I'll leave as-is, unless I get another shot at the file (which is quite likely).

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Makes sense. Thanks for the explanation.

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8443744 [details] [diff] [review]
Testing the addition of multiple fingerprints

Review of attachment 8443744 [details] [diff] [review]:
-----------------------------------------------------------------

lgtm. My only thought was whether this could be rewritten to use the template queue we use in other tests, to reduce reduncancy, but you have two peerConnections so that might get messy.

::: dom/media/tests/identity/test_fingerprints.html
@@ +49,5 @@
> +  return fingerprints;
> +}
> +
> +var fingerprintRegex = new RegExp("^a=fingerprint:(\\S+) (\\S+)", "m");
> +var identityRegex = new RegExp("^a=identity:(\\S+)", "m");

Why not inline syntax? Less escaping helps when reading regular expressions.

var fingerprintRegex = /^a=fingerprint:(\S+) (\S+)/m;
var identityRegex = /^a=identity:(\S+)/m;

@@ +72,5 @@
> +    SimpleTest.finish();
> +  }
> +
> +  navigator.mozGetUserMedia({ audio: true, fake: true }, function(stream) {
> +    ok(stream, "Got fake stream");

This one seems redundant. Tested elsewhere many times.

@@ +76,5 @@
> +    ok(stream, "Got fake stream");
> +    pcDouble.addStream(stream);
> +
> +    pcDouble.createOffer(function(offer) {
> +      ok(offer, "Got offer");

This one too.

Comment 7

[Martin Thomson [:mt:]]

> lgtm. My only thought was whether this could be rewritten to use the
> template queue we use in other tests, to reduce reduncancy, but you have two
> peerConnections so that might get messy.

I could probably use the template: it has two PCs too.  But there is a lot of mashing to reduce this down to the bare minimum.  Hacking the SDP the way that this does isn't something that is easy to get with the template code.  This is easier to follow, I think.

> > +var fingerprintRegex = new RegExp("^a=fingerprint:(\\S+) (\\S+)", "m");
> > +var identityRegex = new RegExp("^a=identity:(\\S+)", "m");
> 
> Why not inline syntax? Less escaping helps when reading regular expressions.

I copied these from elsewhere and line length restrictions there prevented the use of the inline form there.  Not here though, since I removed the allowance for uppercase characters.  I'll change 'em.

> > +  navigator.mozGetUserMedia({ audio: true, fake: true }, function(stream) {
> > +    ok(stream, "Got fake stream");
> 
> This one seems redundant. Tested elsewhere many times.

I wanted logs for progress, in case things break.  Coupling that with an assertion seemed like harmless redundancy.

Comment 8

[Martin Thomson [:mt:]]

Attachment: Testing the addition of multiple fingerprints r=jib

Comment 9

[Martin Thomson [:mt:]]

Comment on attachment 8448283 [details] [diff] [review]
Testing the addition of multiple fingerprints r=jib

Carrying r=jib

Comment 10

[Carsten Book [:Tomcat]]

Hi Martin,
could you provide a Try link. Suggestions for what to run if you haven't
yet can be found here:
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

Comment 11

[Martin Thomson [:mt:]]

I ran the test, but didn't share it, my bad.

https://tbpl.mozilla.org/?tree=Try&rev=1abc5b3461b5

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54c26ba6be84
https://hg.mozilla.org/integration/mozilla-inbound/rev/c809c60a0b62

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/54c26ba6be84
https://hg.mozilla.org/mozilla-central/rev/c809c60a0b62