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 WebDriver.AuthenticatorId #14814

Merged
merged 3 commits into from
Nov 27, 2024

Conversation

RenderMichael
Copy link
Contributor

@RenderMichael RenderMichael commented Nov 26, 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 the WebDriver.AuthenticatorId ID to reflect the currently active authenticator ID.

Motivation and Context

The property is currently completely broken, returning null exclusively. Another victim of the non-auto property typo.

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, Tests


Description

  • Fixed the WebDriver.AuthenticatorId to correctly reflect the active authenticator ID by replacing the field with a property.
  • Updated all relevant methods in WebDriver to use the new AuthenticatorId property.
  • Added a test in VirtualAuthenticatorTest to verify that AuthenticatorId is set correctly and is null after removal.

Changes walkthrough 📝

Relevant files
Bug fix
WebDriver.cs
Fix and refactor `AuthenticatorId` handling in WebDriver 

dotnet/src/webdriver/WebDriver.cs

  • Replaced authenticatorId field with a property AuthenticatorId.
  • Updated methods to use the AuthenticatorId property.
  • Ensured AuthenticatorId is set and retrieved correctly.
  • +10/-11 
    Tests
    VirtualAuthenticatorTest.cs
    Add test for `AuthenticatorId` functionality in WebDriver

    dotnet/test/common/VirtualAuthn/VirtualAuthenticatorTest.cs

  • Added assertion to verify AuthenticatorId is set correctly.
  • Ensured test checks for correct removal of authenticator.
  • +2/-0     

    💡 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: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Parameter Validation
    The RemoveVirtualAuthenticator method takes an authenticatorId parameter but ignores it, using this.AuthenticatorId instead. This could lead to removing the wrong authenticator if the parameter value differs.

    Null Check
    Methods using AuthenticatorId property should validate that it's not null before using it. Otherwise, operations could fail with NullReferenceException.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add validation to prevent null reference exceptions when no authenticator is active

    Add null check for AuthenticatorId before using it in methods that require an active
    authenticator to prevent NullReferenceException.

    dotnet/src/webdriver/WebDriver.cs [1073-1079]

     public void AddCredential(Credential credential)
     {
    +    if (this.AuthenticatorId == null)
    +        throw new InvalidOperationException("No active authenticator. Call AddVirtualAuthenticator first.");
         Dictionary<string, object> parameters = new Dictionary<string, object>(credential.ToDictionary());
         parameters.Add("authenticatorId", this.AuthenticatorId);
         this.Execute(driverCommandToExecute: DriverCommand.AddCredential, parameters);
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This is a critical defensive programming suggestion that would prevent runtime errors by validating the authenticator state before operations. The improved error message would help developers identify the root cause quickly.

    9
    General
    Remove unused parameter that is inconsistent with the method implementation

    The RemoveVirtualAuthenticator method ignores its authenticatorId parameter and uses
    the instance's AuthenticatorId property instead. Either remove the unused parameter
    or use the provided parameter value.

    dotnet/src/webdriver/WebDriver.cs [1056-1062]

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

    Why: The method's parameter is misleading as it's never used, which could cause confusion and maintenance issues. The suggestion correctly identifies a design inconsistency that should be fixed for better API clarity.

    8

    💡 Need additional feedback ? start a PR chat

    @RenderMichael
    Copy link
    Contributor Author

    RenderMichael commented Nov 26, 2024

    Both AI suggestions are completely correct

    1. RemoveVirtualAuthenticator does not even use its parameter
    2. There's no validation to make sure a virtual authenticator is in place (unhelpful exception message)

    I want to fix these, but I don't want this to become a singular "improve virtual authenticator" PR. This PR fixes a bug, and a follow-up PR can improve anything else that could use improvement.

    Copy link
    Member

    @nvborisenko nvborisenko left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Thanks, nice catch!

    @nvborisenko
    Copy link
    Member

    //dotnet/test/common:VirtualAuthn/VirtualAuthenticatorTest-chrome is failing, please take a look.

    @RenderMichael
    Copy link
    Contributor Author

    Funny. The one test I changed for this, is the only one passing.

    image

    This is because the test teardown removes the VA, an operation which is guarded by webDriver.AuthenticatorId != null and only now has begun to work. Let me fix this.

    @RenderMichael
    Copy link
    Contributor Author

    Fixed

    image

    @nvborisenko nvborisenko merged commit cfaa8c4 into SeleniumHQ:trunk Nov 27, 2024
    10 checks passed
    @RenderMichael RenderMichael deleted the authenticator-id branch November 27, 2024 19:35
    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