[ruby] feat: Added virtual authenticator

[TamsilAmani]

Description

Relates #10541

• Added virtual authenticator for js.
• Added tests for the same.

Motivation and Context

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[p0deje]

Rather than using positional arguments, let's use keyword arguments:

Suggested change

	def initialize(*args)
	def initialize(id:, is_resident_credential:, rp_id:, user_handle:, private_key:, sign_count:)

If any of these parameters is optional, we can just set the default value to nil.

[TamsilAmani]

I have made the arguments as positional. But in context of default values there's a doubt.
Required parameters are : id, rp_id, private_key, sign_count

The remaining two parameter i.e is_resident_credential and user_handle are used in combination. For example,
When is_resident_credential = true, user_handle != nil
When is_resident_credential = false, user_handle == nil

I don't suggest making either of them optional as case may arise when user puts is_resident_credential as true but doesn't assign user_handle.

[p0deje]

In this case, I would make only id, rp_id, private_key, sign_count required and then add additional validations for arguments that are connected:

msg = "Both is_resident_credential and user_handle should be passed together"
raise ArgumentError, msg if is_resident_credential && user_handle.nil?
raise ArgumentError, msg if !is_resident_credential && !user_handle.nil?

[p0deje]

Not all the drivers support this, so I think it would be better to keep these methods in a separate module (e.g. Selenium::WebDriver::DriverExtensions::HasVirtualAuthenticator) and then include it in the drivers supporting it. Similar to how we work with HasBiDi extension.

[p0deje]

Could we also add some # @example that demonstrate how to use the APIs?

[titusfortner]

The argument is that it is part of the spec, so the methods should be implemented across the board.

My issue is that the naming gets confusing here when tacked onto Driver class directly. I'm thinking we should have a separate class for these methods:

Something like:

driver.virtual_authenticator.id
driver.virtual_authenticator.add
driver.virtual_authenticator.remove
driver.virtual_authenticator.credentials=
driver.virtual_authenticator.credentials
driver.virtual_authenticator.user_verified=

[TamsilAmani]

@p0deje as discussed with @AutomatedTester, virtual authenticator is present only in chrome for now. It may happen in future that firefox and other browsers implement it. So to keep the code changes minimal in future for other browsers, I have added virtual authenticator directly under the driver. And tests are run only in chrome, not in firefox or others. If for example, firefox decided to implement it, then we only need to include it in the test suite.

# @examples will be added in the documentation. That will be a separate PR. Is that what you mean?

[TamsilAmani]

@titusfortner I have created a separate class VirtualAuthenticator.rb and all the methods have been taken there as you suggested. Please review for changes. We can now directly call:

authenticator.add_credential
authenticator.remove_credential

instead of

driver.virtual_authenticator.add
driver.virtual_authenticator.remove

This approach will help to add multiple authenticators.

[p0deje]

The bridge is supposed to be mostly stateless, so what if instead, we change an interface to return the virtual authenticator when it's added and allow to remove it later as a parameter? Something like this:

authenticator = driver.add_virtual_authenticator(options)
driver. remove_virtual_authenticator(authenticator)

This way we can avoid storing authenticator ID internally. It would also allow us to have multiple virtual authenticators.

[TamsilAmani]

I cleared bridge of the authenticator methods and created a separate class VirtualAuthenticator as you suggested. Now we can get and remove the authenticator object without storing its id in bridge.

[titusfortner]

Thanks for your work on this. It's looking good!

I'm going to merge it, then update things to get this released in 4.4, but for reference.

• #as_json needs to have keys as String not Symbol by convention.
• Predicate methods explicitly avoid is_ and has_ prefixes and use a ? at the end (my approach is to use attr_accessor without the prefix, then alias the getter with a ?)
• Ruby tries to be less verbose so long as the return is "obvious." So Credential.resident class method that returns an instance is obviously the same as the. more verbose Credential.create_resident_credential
• For the Bridge#execute method, there are 3 parameters: command name, the variables in the endpoint that need to be replaced (e.g. ":authenticatorId" with the actual ID), and the key/values that go into the payload. Some of these were mixed up in this PR

Then I'm making a couple other personal preference changes, just because. :-D