-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
[java]: Fix addCredential()
in VirtualAuthenticator
#15633
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Conversation
Signed-off-by: Viet Nguyen Duc <[email protected]>
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
CI Feedback 🧐(Feedback updated until commit e2d02a1)A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
I don't understand what this PR is doing. Do we need to allow rpId to be null? Otherwise we should be using the same pattern as everything else. I don't understand why we are adding getCredentials method to each test? I'm sure I'm missing context. |
@titusfortner this is used to fix example tests failed in docs repo - https://github.com/SeleniumHQ/seleniumhq.github.io/actions/runs/14504046362/job/40689974955 |
As per the spec, https://www.w3.org/TR/webauthn-2/#sctn-automation-add-credential, its needs to be present else a WebDriver error is thrown. |
Oh ok, so something is wrong in
|
addCredential()
in VirtualAuthenticator
@titusfortner, the root cause was in method |
.collect(Collectors.toUnmodifiableMap(Map.Entry::getKey, Map.Entry::getValue))); | ||
Map<String, Object> map = new HashMap<>(credential.toMap()); | ||
map.put("authenticatorId", id); | ||
execute(DriverCommand.ADD_CREDENTIAL, map); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want Map.copyOf(map)
here to ensure immutability? Not certain it matters.
I guess I'm confused why we need the added As a slightly separate note, I'm not sure about the value of having these in our docs, either, since it doesn't really give any useful information on how credentials work, or when you need them or what is a real-world scenario, so they just read like unit tests. |
User description
🔗 Related Issues
💥 What does this PR do?
Example tests in selenium docs failed.
🔧 Implementation Notes
💡 Additional Considerations
🔄 Types of changes
PR Type
Bug fix, Tests
Description
Fixed
rpId
null check inCredential.fromMap
method.Added calls to
authenticator.getCredentials()
in multiple test cases.Enhanced test coverage for non-resident and resident credentials.
Changes walkthrough 📝
Credential.java
Made `rpId` optional in `Credential.fromMap`
java/src/org/openqa/selenium/virtualauthenticator/Credential.java
rpId
.rpId
to be optional inCredential.fromMap
.VirtualAuthenticatorTest.java
Enhanced tests for VirtualAuthenticator credentials
java/test/org/openqa/selenium/virtualauthenticator/VirtualAuthenticatorTest.java
authenticator.getCredentials()
calls to validate credentialaddition.