Pass through AuthenticatorAttestationResponse.getTransports()

[emlun]

Fixes one part of #25 .

This includes and builds upon a related change in how clientExtensionResults are declared in the schema, by introducing a derived option for properties alongside the existing required and optional. I think this captures the idea a bit more cleanly, and it also allows some test inputs to define getClientExtensionResults() instead of injecting a clientExtensionResults property. Then the transports field works the same way using this new schema feature. Plumbing it through for the "extended' target gets a bit ugly, but... it's kind of okay I guess?

If you don't agree with that change, I can drop it and rework the transports addition to inject a new property in the same way createExtendedResponseToJSON() currently does for clientExtensionResults for example. The new tests should apply well enough in that case too.

In case the client does not provide the getTransports() method, the converter will fall back to returning an empty sequence. This is because the spec says clients are to return "[...] or an empty sequence if the information is unavailable", so that seems like a sensible fallback value (since the information is indeed unavailable).

[emlun]

@lgarron any idea when you might have time to look at this?

[lgarron]

Ahoy! Apologies for the delay.

If you don't agree with that change, I can drop it and rework the transports addition to inject a new property in the same way createExtendedResponseToJSON() currently does for clientExtensionResults for example. The new tests should apply well enough in that case too.

I think the current approach in this PR is clever, but I still don't feel comfortable merging it. Per the project name, the goal is to make it so that the client only has to think about JSON. Having objects that are "almost JSON but not quite" is specifically what we're trying to avoid, even if they act like JSON in most situations.

So I think adding a transports field directly would be best. I think it should probably also go in the default implementation (not just the extended one), since this would encourage UX best practices by virtue of receiving the transports on the server at at registration time by default.

Would you be up for those changes?

[emlun]

Sure! I'll get started on the rework as noted in my initial comment.

However, I feel like there might be a bit of misunderstanding, so let me clarify one thing: the new "derived" thing will still generate output that is pure JSON. What the change does is integrate the special case for clientExtensionResults into the schema declaration, so that the "conversion" from getClientExtensionResults() to clientExtensionResults can be done by the schema convert function. So it's just the converter inputs that are extended to support "almost JSON but not quite" (which is what PublicKeyCredential is), while the outputs are still pure JSON. And then, since transports is derived in a similar way from getTransports(), the new feature can be used for that too. This also makes the tests a bit cleaner since they don't have to pre-convert those properties.

Does that make it more palatable? If not, I'm still happy to rework it as mentioned above.

[lgarron]

Does that make it more palatable? If not, I'm still happy to rework it as mentioned above.

Oh, hmm, I seem to have tested it wrong yesterday; I'm getting something more like you described now. I suspect Parcel reused an old cache when I was testing. (I really should move us to esbuild at this point...)
Let me take another look.

[lgarron]

Thanks for the PR!

Apologies for taking so long. I was a bit concerned about an unsustainable change to the "schema" mechanism, but really it shouldn't be an issue in practice.

[lgarron]

Review note: I was a little concerned that empty transports field might mess with the authentication prompt, but it doesn't seem to be an issue. I'm having trouble finding a place to back that up in the spec, though, so I'm just going off of Chrome/FirefoxSafari on macOS.

[emlun]

The backing for this in the spec is in the definition of the getTransports() method: 🙂

These values are the transports that the authenticator is believed to support, or an empty sequence if the information is unavailable.

If the getTransports method doesn't exist, then the information is indeed not available, so I think this should be an appropriate fallback value.

See also the definition of navigator.credentials.get() - any credential descriptor with an empty transports member will simply not affect the distinctTransports variable being computed there.

[emlun]

Thank you too, I'm glad I could help!