Skip to content
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

[dotnet] Fix Virtual Authenticator removal, annotate NRT #14822

Merged
merged 6 commits into from
Dec 1, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 27, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

This fixes a bug where the RemoveVirtualAuthenticator method does not use its parameter, and instead uses the cached authenticator ID value. This blocks scenarios where multiple VA's are running at once.

Additionally, Nullable Reference Type support (as well as improved null checks) have been added. Exceptional scenarios have been documented.

Motivation and Context

Contributes to #14640

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.

PR Type

Bug fix, Enhancement, Tests


Description

  • Fixed a bug in RemoveVirtualAuthenticator method to correctly use its parameter, allowing multiple virtual authenticators to be managed.
  • Enhanced the Credential class and other related classes with nullable reference type support and added null checks.
  • Improved documentation across several classes, detailing exceptions and method behaviors.
  • Added new tests to verify handling of multiple virtual authenticators and invalid arguments.

Changes walkthrough 📝

Relevant files
Enhancement
Credential.cs
Enhance Credential class with null checks and NRT support

dotnet/src/webdriver/VirtualAuth/Credential.cs

  • Enabled nullable reference types.
  • Added null checks and exceptions for constructor parameters.
  • Updated property implementations to use expression-bodied members.
  • Improved method documentation with exception details.
  • +37/-49 
    Documentation
    IHasVirtualAuthenticator.cs
    Add exception handling documentation to interface methods

    dotnet/src/webdriver/VirtualAuth/IHasVirtualAuthenticator.cs

  • Enabled nullable reference types.
  • Added exception documentation for methods.
  • +14/-0   
    VirtualAuthenticatorOptions.cs
    Enhance VirtualAuthenticatorOptions with detailed documentation

    dotnet/src/webdriver/VirtualAuth/VirtualAuthenticatorOptions.cs

  • Enabled nullable reference types.
  • Improved method documentation with exception details.
  • Added remarks and completion list tags for methods.
  • +31/-25 
    Bug fix
    WebDriver.cs
    Fix and enhance WebDriver virtual authenticator methods   

    dotnet/src/webdriver/WebDriver.cs

  • Enabled nullable reference types.
  • Added null checks and exceptions for method parameters.
  • Improved method documentation with exception details.
  • Fixed bug in RemoveVirtualAuthenticator method to use parameter.
  • +63/-16 
    Tests
    VirtualAuthenticatorTest.cs
    Add tests for virtual authenticator scenarios and error handling

    dotnet/test/common/VirtualAuthn/VirtualAuthenticatorTest.cs

  • Added tests for multiple virtual authenticators.
  • Added tests for invalid argument handling.
  • +55/-1   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Possible Bug
    The AddVirtualAuthenticator method assigns the id before returning it, which could lead to race conditions if multiple authenticators are added concurrently

    Code Smell
    The userHandle field is cloned in the UserHandle property but not in the constructor, which could lead to inconsistent defensive copying

    Performance Issue
    The GetCredentials method creates a new list with known capacity but still uses Add() instead of setting indices directly

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Fix incorrect authenticator ID being used in removal request

    The RemoveVirtualAuthenticator method is using the wrong authenticator ID in the
    parameters dictionary. It should use the provided authenticatorId parameter instead
    of this.AuthenticatorId.

    dotnet/src/webdriver/WebDriver.cs [1066-1077]

     public void RemoveVirtualAuthenticator(string authenticatorId)
     {
         Dictionary<string, object> parameters = new Dictionary<string, object>();
    -    parameters.Add("authenticatorId", this.AuthenticatorId);
    +    parameters.Add("authenticatorId", authenticatorId);
         this.Execute(DriverCommand.RemoveVirtualAuthenticator, parameters);
         this.AuthenticatorId = null;
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 10

    Why: Critical bug fix - the method was using the cached authenticator ID instead of the provided parameter, which would cause removal of the wrong authenticator and potential security issues.

    10
    Add response type validation to prevent runtime casting exceptions

    The GetCredentials method should validate the response value type before casting to
    avoid potential runtime exceptions.

    dotnet/src/webdriver/WebDriver.cs [1117-1118]

    -object[] commandResponse = (object[])this.Execute(driverCommandToExecute: DriverCommand.GetCredentials, parameters).Value;
    +Response getCredentialsResponse = this.Execute(driverCommandToExecute: DriverCommand.GetCredentials, parameters);
    +if (getCredentialsResponse.Value is not object?[] credentialsList)
    +{
    +    throw new WebDriverException($"Get credentials call succeeded, but the response was not a list of credentials: {getCredentialsResponse.Value}");
    +}
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Important defensive programming improvement that prevents potential runtime exceptions by validating the response type before casting, making the code more robust and maintainable.

    8

    💡 Need additional feedback ? start a PR chat

    @nvborisenko
    Copy link
    Member

    Ci is failing, I guess because of some invisible character.

    @RenderMichael
    Copy link
    Contributor Author

    @nvborisenko fixed🤦
    image

    @nvborisenko
    Copy link
    Member

    /automerge, thanks Michael for contribution.

    @nvborisenko nvborisenko merged commit b256c5f into SeleniumHQ:trunk Dec 1, 2024
    10 checks passed
    @RenderMichael RenderMichael deleted the virtual-auth branch December 1, 2024 19:01
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Dec 2, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants