Skip to content

[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

Open
wants to merge 6 commits into
base: trunk
Choose a base branch
from

Conversation

VietND96
Copy link
Member

@VietND96 VietND96 commented Apr 16, 2025

User description

🔗 Related Issues

💥 What does this PR do?

Example tests in selenium docs failed.

    @Test
    fun testCreateAndAddNonResidentialKey() {
        val options = VirtualAuthenticatorOptions()
            .setProtocol(VirtualAuthenticatorOptions.Protocol.U2F)
            .setHasResidentKey(false)
        val authenticator = (driver as HasVirtualAuthenticator?)!!.addVirtualAuthenticator(options)
        val credentialId = byteArrayOf(1, 2, 3, 4)
        val nonResidentCredential = Credential.createNonResidentCredential(
            credentialId, "localhost", ec256PrivateKey,  /*signCount=*/0
        )
        authenticator.addCredential(nonResidentCredential)
        val credentialList = authenticator.credentials
        Assertions.assertEquals(1, credentialList.size)
        val credential = credentialList[0]
        Assertions.assertArrayEquals(credentialId, credential.id)
    }
[ERROR] dev.selenium.virtualauthenticator.VirtualAuthenticatorTest.testCreateAndAddNonResidentialKey -- Time elapsed: 0.768 s <<< ERROR!
java.lang.IllegalArgumentException: rpId must be set
        at org.openqa.selenium.internal.Require.nonNull(Require.java:64)
        at org.openqa.selenium.virtualauthenticator.Credential.fromMap(Credential.java:69)
        at java.base/java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:215)
        at java.base/java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1709)
        at java.base/java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:570)
        at java.base/java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:560)
        at java.base/java.util.stream.ReduceOps$ReduceOp.evaluateSequential(ReduceOps.java:921)
        at java.base/java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:265)
        at java.base/java.util.stream.ReferencePipeline.collect(ReferencePipeline.java:727)
        at org.openqa.selenium.remote.RemoteWebDriver$RemoteVirtualAuthenticator.getCredentials(RemoteWebDriver.java:1264)
        at dev.selenium.virtualauthenticator.VirtualAuthenticatorTest.testCreateAndAddNonResidentialKey(VirtualAuthenticatorTest.kt:120)
        at java.base/java.lang.reflect.Method.invoke(Method.java:580)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)
        at java.base/java.util.ArrayList.forEach(ArrayList.java:1597)

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Bug fix, Tests


Description

  • Fixed rpId null check in Credential.fromMap method.

  • Added calls to authenticator.getCredentials() in multiple test cases.

  • Enhanced test coverage for non-resident and resident credentials.


Changes walkthrough 📝

Relevant files
Bug fix
Credential.java
Made `rpId` optional in `Credential.fromMap`                         

java/src/org/openqa/selenium/virtualauthenticator/Credential.java

  • Removed mandatory null check for rpId.
  • Allowed rpId to be optional in Credential.fromMap.
  • +1/-1     
    Tests
    VirtualAuthenticatorTest.java
    Enhanced tests for VirtualAuthenticator credentials           

    java/test/org/openqa/selenium/virtualauthenticator/VirtualAuthenticatorTest.java

  • Added authenticator.getCredentials() calls to validate credential
    addition.
  • Updated multiple test cases for non-resident and resident credentials.
  • Improved test assertions for credential handling.
  • +4/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @VietND96 VietND96 requested a review from pujagani April 16, 2025 17:48
    @selenium-ci selenium-ci added the C-java Java Bindings label Apr 16, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where Selenium 2.48 doesn't trigger JavaScript in link's href on click()

    Requires further human verification:

    • Need to verify if the VirtualAuthenticator changes have any relation to the JavaScript click issue in Firefox

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating ChromeDriver

    Requires further human verification:

    • Need to verify if the VirtualAuthenticator changes have any relation to the ChromeDriver connection issues

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Potential Regression

    Making rpId optional might cause issues with other credential types or implementations that expect rpId to be present. Need to verify this change doesn't break existing functionality.

    Object rpId = map.get("rpId");
    Object privateKey = Require.nonNull("privateKey", map.get("privateKey"));

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 16, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Prevent null rpId
    Suggestion Impact:The suggestion recommended handling null rpId values, and the commit addressed this by using Require.nonNull() to ensure rpId is not null, which achieves the same goal of preventing NullPointerExceptions but with a different implementation approach

    code diff:

    -    Object rpId = map.get("rpId");
    +    Object rpId = Require.nonNull("rpId", map.get("rpId"));

    The code now allows rpId to be null, but the rest of the method likely assumes
    it's non-null. This could lead to NullPointerExceptions when the value is used
    later in the method or in other parts of the code that expect rpId to be valid.

    java/src/org/openqa/selenium/virtualauthenticator/Credential.java [69]

     Object rpId = map.get("rpId");
    +if (rpId == null) {
    +  rpId = "";  // or another appropriate default value
    +}

    [Suggestion has been applied]

    Suggestion importance[1-10]: 8

    __

    Why: This suggestion addresses a potential NullPointerException that could occur after removing the null check for rpId. Since the PR explicitly makes rpId optional, providing a default value when it's null is important for maintaining robust behavior and preventing runtime errors.

    Medium
    • Update

    Copy link
    Contributor

    qodo-merge-pro bot commented Apr 16, 2025

    CI Feedback 🧐

    (Feedback updated until commit 8803c94)

    A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

    Action: Ruby / Unit Tests (3.2.8, windows) / Unit Tests (3.2.8, windows)

    Failed stage: Run Bazel [❌]

    Failed test name: //rb/spec/unit/selenium/webdriver/edge:service

    Failure summary:

    The action failed because the test //rb/spec/unit/selenium/webdriver/edge:service timed out after
    1800 seconds (30 minutes). This was the only failing test out of 66 tests that were executed. The
    test appears to have been stuck or running for too long, causing it to exceed the maximum allowed
    execution time.

    Relevant error logs:
    1:  ##[group]Operating System
    2:  Microsoft Windows Server 2022
    ...
    
    499:  -> Locally signed 5 keys.
    500:  ==> Importing owner trust values...
    501:  gpg: setting ownertrust to 4
    502:  gpg: setting ownertrust to 4
    503:  gpg: setting ownertrust to 4
    504:  gpg: setting ownertrust to 4
    505:  gpg: setting ownertrust to 4
    506:  ==> Disabling revoked keys in keyring...
    507:  -> Disabled 4 keys.
    508:  ==> Updating trust database...
    509:  gpg: marginals needed: 3  completes needed: 1  trust model: pgp
    510:  gpg: depth: 0  valid:   1  signed:   5  trust: 0-, 0q, 0n, 0m, 0f, 1u
    511:  gpg: depth: 1  valid:   5  signed:   7  trust: 0-, 0q, 0n, 5m, 0f, 0u
    512:  gpg: depth: 2  valid:   4  signed:   2  trust: 4-, 0q, 0n, 0m, 0f, 0u
    513:  gpg: next trustdb check due at 2025-08-13
    514:  gpg: error retrieving '[email protected]' via WKD: No data
    515:  gpg: error reading key: No data
    516:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    517:  �[32mAnalyzing:�[0m 66 targets (292 packages loaded, 7463 targets configured)
    518:  gpg: key F40D263ECA25678A: "Alexey Pavlov (Alexpux) <[email protected]>" not changed
    519:  gpg: Total number processed: 1
    520:  gpg:              unchanged: 1
    521:  gpg: error retrieving '[email protected]' via WKD: No data
    522:  gpg: error reading key: No data
    523:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    524:  gpg: key 790AE56A1D3CFDDC: "David Macek (MSYS2 master key) <[email protected]>" not changed
    525:  gpg: Total number processed: 1
    526:  gpg:              unchanged: 1
    527:  �[32mAnalyzing:�[0m 66 targets (318 packages loaded, 7950 targets configured)
    528:  gpg: error retrieving '[email protected]' via WKD: No data
    529:  gpg: error reading key: No data
    530:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    531:  �[35mWARNING: �[0mD:/_bazel/external/io_bazel_rules_closure/java/com/google/javascript/jscomp/BUILD:19:13: in java_library rule @@io_bazel_rules_closure//java/com/google/javascript/jscomp:jscomp: target '@@io_bazel_rules_closure//java/com/google/javascript/jscomp:jscomp' depends on deprecated target '@@io_bazel_rules_closure//java/io/bazel/rules/closure:build_info_java_proto': Use java_proto_library from com_google_protobuf
    532:  gpg: key DA7EF2ABAEEA755C: "Martell Malone (martell) <[email protected]>" not changed
    533:  gpg: Total number processed: 1
    534:  gpg:              unchanged: 1
    535:  gpg: error retrieving '[email protected]' via WKD: No data
    536:  gpg: error reading key: No data
    537:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    538:  �[32mAnalyzing:�[0m 66 targets (340 packages loaded, 8647 targets configured)
    539:  gpg: key 755B8182ACD22879: "Christoph Reiter (MSYS2 master key) <[email protected]>" not changed
    540:  gpg: Total number processed: 1
    541:  gpg:              unchanged: 1
    542:  gpg: error retrieving '[email protected]' via WKD: No data
    543:  gpg: error reading key: No data
    544:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    545:  gpg: key 9F418C233E652008: "Ignacio Casal Quinteiro <[email protected]>" not changed
    546:  gpg: Total number processed: 1
    547:  gpg:              unchanged: 1
    548:  gpg: error retrieving '[email protected]' via WKD: No data
    549:  gpg: error reading key: No data
    550:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    551:  gpg: key BBE514E53E0D0813: "Ray Donnelly (MSYS2 Developer - master key) <[email protected]>" not changed
    552:  gpg: Total number processed: 1
    553:  gpg:              unchanged: 1
    554:  gpg: error retrieving '[email protected]' via WKD: No data
    555:  gpg: error reading key: No data
    556:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    557:  gpg: key 5F92EFC1A47D45A1: "Alexey Pavlov (Alexpux) <[email protected]>" not changed
    558:  gpg: Total number processed: 1
    559:  gpg:              unchanged: 1
    560:  gpg: error retrieving '[email protected]' via WKD: No data
    561:  gpg: error reading key: No data
    562:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    563:  gpg: key 974C8BE49078F532: "David Macek <[email protected]>" not changed
    564:  gpg: Total number processed: 1
    565:  gpg:              unchanged: 1
    566:  gpg: error retrieving '[email protected]' via WKD: No data
    567:  gpg: error reading key: No data
    568:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    569:  gpg: key FA11531AA0AA7F57: "Christoph Reiter (MSYS2 development key) <[email protected]>" not changed
    570:  gpg: Total number processed: 1
    571:  gpg:              unchanged: 1
    572:  gpg: error retrieving '[email protected]' via WKD: General error
    573:  gpg: error reading key: General error
    574:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    575:  gpg: key 794DCF97F93FC717: "Martell Malone (martell) <[email protected]>" not changed
    576:  gpg: Total number processed: 1
    577:  gpg:              unchanged: 1
    578:  gpg: error retrieving '[email protected]' via WKD: No data
    579:  gpg: error reading key: No data
    580:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    581:  gpg: key D595C9AB2C51581E: "Martell Malone (MSYS2 Developer) <[email protected]>" not changed
    582:  gpg: Total number processed: 1
    583:  gpg:              unchanged: 1
    584:  gpg: error retrieving '[email protected]' via WKD: No data
    585:  gpg: error reading key: No data
    586:  gpg: refreshing 1 key from hkps://keyserver.ubuntu.com
    ...
    
    889:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    890:  �[32mINFO: �[0mFrom Compiling absl/strings/internal/cord_rep_btree_reader.cc [for tool]:
    891:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    892:  �[32mINFO: �[0mFrom Compiling absl/strings/internal/cord_rep_btree_navigator.cc [for tool]:
    893:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    894:  �[32mINFO: �[0mFrom Compiling absl/strings/internal/cord_rep_btree.cc [for tool]:
    895:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    896:  �[32mINFO: �[0mFrom Compiling absl/strings/internal/cord_internal.cc [for tool]:
    897:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    898:  �[32mINFO: �[0mFrom Compiling absl/strings/internal/cordz_info.cc [for tool]:
    899:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    900:  �[32mINFO: �[0mFrom Compiling absl/strings/cord_buffer.cc [for tool]:
    901:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    902:  �[32mINFO: �[0mFrom Compiling absl/strings/cord_analysis.cc [for tool]:
    903:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    904:  �[32mINFO: �[0mFrom Compiling absl/base/internal/strerror.cc [for tool]:
    905:  cl : Command line warning D9002 : ignoring unknown option '-std=c++14'
    ...
    
    1951:  �[32m[1,267 / 1,268]�[0m 65 / 66 tests;�[0m Testing //rb/spec/unit/selenium/webdriver/edge:service; 1477s local, disk-cache
    1952:  �[32m[1,267 / 1,268]�[0m 65 / 66 tests;�[0m Testing //rb/spec/unit/selenium/webdriver/edge:service; 1537s local, disk-cache
    1953:  �[32m[1,267 / 1,268]�[0m 65 / 66 tests;�[0m Testing //rb/spec/unit/selenium/webdriver/edge:service; 1598s local, disk-cache
    1954:  �[32m[1,267 / 1,268]�[0m 65 / 66 tests;�[0m Testing //rb/spec/unit/selenium/webdriver/edge:service; 1658s local, disk-cache
    1955:  �[32m[1,267 / 1,268]�[0m 65 / 66 tests;�[0m Testing //rb/spec/unit/selenium/webdriver/edge:service; 1718s local, disk-cache
    1956:  �[32m[1,267 / 1,268]�[0m 65 / 66 tests;�[0m Testing //rb/spec/unit/selenium/webdriver/edge:service; 1778s local, disk-cache
    1957:  �[32m[1,267 / 1,268]�[0m 65 / 66 tests;�[0m Testing //rb/spec/unit/selenium/webdriver/edge:service; 1800s local, disk-cache
    1958:  �[31m�[1mTIMEOUT: �[0m//rb/spec/unit/selenium/webdriver/edge:service (Summary)
    1959:  D:/_bazel/execroot/_main/bazel-out/x64_windows-fastbuild/testlogs/rb/spec/unit/selenium/webdriver/edge/service/test.log
    1960:  ==================== Test output for //rb/spec/unit/selenium/webdriver/edge:service:
    1961:  �[32mINFO: �[0mFrom Testing //rb/spec/unit/selenium/webdriver/edge:service:
    1962:  ================================================================================
    1963:  �[32mINFO: �[0mFound 66 test targets...
    1964:  �[32mINFO: �[0mElapsed time: 2394.804s, Critical Path: 2255.05s
    1965:  �[32mINFO: �[0m1268 processes: 552 disk cache hit, 575 internal, 141 local.
    1966:  �[32mINFO: �[0mBuild completed, 1 test FAILED, 1268 total actions
    1967:  //rb/spec/unit/selenium:devtools                                         �[0m�[32mPASSED�[0m in 5.3s
    ...
    
    2019:  //rb/spec/unit/selenium/webdriver/ie:service                             �[0m�[32mPASSED�[0m in 5.6s
    2020:  //rb/spec/unit/selenium/webdriver/remote:bridge                          �[0m�[32mPASSED�[0m in 4.3s
    2021:  //rb/spec/unit/selenium/webdriver/remote:capabilities                    �[0m�[32mPASSED�[0m in 4.9s
    2022:  //rb/spec/unit/selenium/webdriver/remote:driver                          �[0m�[32mPASSED�[0m in 5.1s
    2023:  //rb/spec/unit/selenium/webdriver/remote/http:common                     �[0m�[32mPASSED�[0m in 4.7s
    2024:  //rb/spec/unit/selenium/webdriver/remote/http:curb                       �[0m�[32mPASSED�[0m in 4.9s
    2025:  //rb/spec/unit/selenium/webdriver/remote/http:default                    �[0m�[32mPASSED�[0m in 4.6s
    2026:  //rb/spec/unit/selenium/webdriver/safari:driver                          �[0m�[32mPASSED�[0m in 4.9s
    2027:  //rb/spec/unit/selenium/webdriver/safari:options                         �[0m�[32mPASSED�[0m in 4.0s
    2028:  //rb/spec/unit/selenium/webdriver/safari:service                         �[0m�[32mPASSED�[0m in 4.8s
    2029:  //rb/spec/unit/selenium/webdriver/support:color                          �[0m�[32mPASSED�[0m in 4.9s
    2030:  //rb/spec/unit/selenium/webdriver/support:event_firing                   �[0m�[32mPASSED�[0m in 5.0s
    2031:  //rb/spec/unit/selenium/webdriver/support:select                         �[0m�[32mPASSED�[0m in 5.1s
    2032:  //rb/spec/unit/selenium/webdriver/edge:service                          �[0m�[31m�[1mTIMEOUT�[0m in 1800.0s
    2033:  D:/_bazel/execroot/_main/bazel-out/x64_windows-fastbuild/testlogs/rb/spec/unit/selenium/webdriver/edge/service/test.log
    2034:  Executed 66 out of 66 tests: 65 tests pass and �[0m�[31m�[1m1 fails locally�[0m.
    2035:  �[0m
    2036:  ##[error]Process completed with exit code 1.
    2037:  Post job cleanup.
    

    @titusfortner
    Copy link
    Member

    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.

    @VietND96
    Copy link
    Member Author

    VietND96 commented Apr 17, 2025

    @titusfortner this is used to fix example tests failed in docs repo - https://github.com/SeleniumHQ/seleniumhq.github.io/actions/runs/14504046362/job/40689974955
    This is a broken after another PR enforce check nonNull - #15119 (which just increases the code quality).
    I add method getCredentials() to every test to align with example tests in docs repo. I think this helps detect the breakage early in this repo itself before shipping and seeing error in other places.
    The test author is @pujagani, so let her feedback on this, we update code here, or update the example tests in docs repo.

    @pujagani
    Copy link
    Contributor

    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.

    @VietND96
    Copy link
    Member Author

    Oh ok, so something is wrong in addCredential(), since a credential was created with rpId = "localhost", but it becomes null after that.

            val nonResidentCredential = Credential.createNonResidentCredential(
                credentialId, "localhost", ec256PrivateKey,  /*signCount=*/0
            )
            authenticator.addCredential(nonResidentCredential)
            val credentialList = authenticator.credentials
    

    @VietND96 VietND96 changed the title [java]: Fix getCredentials from VirtualAuthenticator [java]: Fix addCredential from VirtualAuthenticator Apr 18, 2025
    @VietND96 VietND96 changed the title [java]: Fix addCredential from VirtualAuthenticator [java]: Fix addCredential() in VirtualAuthenticator Apr 18, 2025
    @VietND96
    Copy link
    Member Author

    @titusfortner, the root cause was in method addCredential(), which tries to add a credential (where rpId is set properly) to authenticator. Not sure why much more steps were added, while a credential supports toMap(). Those were added in a commit that tried to remove Guava.
    Now, check nonNull is added back for rpId, so I think tests make sense to test both add and get credential to see if it really works. What do you think?

    .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);
    Copy link
    Member

    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.

    @titusfortner
    Copy link
    Member

    titusfortner commented Apr 18, 2025

    I guess I'm confused why we need the added getCredentials() calls since we're already have a testGetCredentials that should catch the problem. If it doesn't catch the problem, why not?

    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.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants