Add support to debug virtual authenticators#7842
Conversation
shs96c
left a comment
shs96c
left a comment
There was a problem hiding this comment.
This is great. Thank you! I love that it has tests too :) I'll wait for your comments before landing this. Feel free to disagree with anything: a lot of my comments are just questions.
|
|
||
| public Map<String, Object> toMap() { | ||
| Base64.Encoder encoder = Base64.getUrlEncoder(); | ||
| HashMap<String, Object> map = new HashMap(); |
There was a problem hiding this comment.
Nit: Map on the LHS.
It doesn't matter a huge amount, but I'm curious why you've used ImmutableMap elsewhere and not here.
There was a problem hiding this comment.
I think an earlier version of my patch modified the map in place on RemoteWebDriver. Changed to ImmutableMap here and in VirtualAuthenticatorOptions for consistency. This means I had to add guava as a dependency to the bazel package -- ptal.
There was a problem hiding this comment.
Ah! We strive to make sure that the selenium-api doesn't need any third party libraries. If we want this to be immutable, the only Java 8 option is to use Collections.unmodifiableMap. The Map.of() method from Java 11 would be better, but we can't use that yet.
There was a problem hiding this comment.
Removed the dependency and used Collections.unmodifiableMap instead. Please take a look.
|
|
nsatragno
force-pushed
the
webauthn_java_followup
branch
from
9b63b59 to
3d7ef7b
Compare
nsatragno
left a comment
nsatragno
left a comment
There was a problem hiding this comment.
Thank you for your quick review! Comments addressed below:
|
|
||
| public Map<String, Object> toMap() { | ||
| Base64.Encoder encoder = Base64.getUrlEncoder(); | ||
| HashMap<String, Object> map = new HashMap(); |
There was a problem hiding this comment.
I think an earlier version of my patch modified the map in place on RemoteWebDriver. Changed to ImmutableMap here and in VirtualAuthenticatorOptions for consistency. This means I had to add guava as a dependency to the bazel package -- ptal.
Also fix some if without braces.
nsatragno
force-pushed
the
webauthn_java_followup
branch
from
080b579 to
059d4e0
Compare
3 participants
Description
This patch adds support for the
addCredential,getCredentials,removeCredential,removeAllCredentialsandsetUserVerifiedcommands of the WebAuthn automation API.To add these methods directly into the
VirtualAuthenticatorclass, I extracted it into an interface and implemented aRemoteVirtualAuthenticatoras inner class ofRemoteWebDriver.Motivation and Context
Support more WebAuthn use cases:
Types of changes
Checklist
Fixes #7829