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

[bidi][js] Allow passing in uri for authentication handlers #14386

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

Conversation

pujagani
Copy link
Contributor

@pujagani pujagani commented Aug 13, 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

Related to #13993

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.

PR Type

Enhancement, Tests


Description

  • Enhanced the Network class to support URI-based authentication handlers.
  • Introduced a new map #authHandlers to store and manage authentication credentials.
  • Added a method getAuthCredentials to fetch credentials based on the request URI.
  • Modified existing methods to integrate the new URI-based authentication logic.
  • Added comprehensive tests to verify the functionality of the new authentication handlers with URI filters.

Changes walkthrough 📝

Relevant files
Enhancement
network.js
Add URI-based authentication handlers to Network class     

javascript/node/selenium-webdriver/lib/network.js

  • Added support for URI-based authentication handlers.
  • Introduced #authHandlers map to store authentication credentials.
  • Implemented getAuthCredentials method to retrieve credentials based on
    URI.
  • Modified addAuthenticationHandler and removeAuthenticationHandler
    methods.
  • +29/-21 
    Tests
    webdriver_network_test.js
    Add tests for URI-based authentication handlers                   

    javascript/node/selenium-webdriver/test/lib/webdriver_network_test.js

  • Added tests for authentication handlers with URI filters.
  • Tested multiple handlers with different and same filters.
  • Verified removal and clearing of authentication handlers.
  • +42/-1   

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    URI Matching
    The method getAuthCredentials uses a simple string matching (uri.match(value.uri)) which might not be robust enough for different URI formats or query parameters. Consider using a more sophisticated URI parsing and matching technique.

    Error Handling
    The authRequired callback does not handle the case where continueWithAuth might fail. It's important to add error handling around this asynchronous operation to manage potential failures gracefully.

    Default URI Pattern
    The default URI pattern in addAuthenticationHandler is set to '//', which is unclear and might not correctly match URIs as intended. It's recommended to clarify or adjust this default value.

    Copy link
    Contributor

    codiumai-pr-agent-pro bot commented Aug 13, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Prevent duplicate URI patterns in authentication handlers to avoid conflicts

    Refactor the addAuthenticationHandler method to ensure that duplicate URI patterns
    are either not allowed or handled explicitly, to prevent potential conflicts or
    unexpected behavior when multiple handlers match the same URI.

    javascript/node/selenium-webdriver/lib/network.js [72]

    -this.#authHandlers.set(id, { username, password, uri })
    +if (Array.from(this.#authHandlers.values()).some(handler => handler.uri === uri)) {
    +  throw new Error(`An authentication handler for the URI '${uri}' already exists.`);
    +}
    +this.#authHandlers.set(id, { username, password, uri });
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Preventing duplicate URI patterns is crucial to avoid conflicts and unexpected behavior. This suggestion addresses a potential source of bugs and improves the reliability of the authentication mechanism.

    9
    Enhancement
    Add error handling for unmatched URI patterns in authentication handlers

    Consider adding error handling for the case where the uri pattern does not match any
    registered handler. This will prevent the system from silently failing without
    authentication when no matching URI is found.

    javascript/node/selenium-webdriver/lib/network.js [60-65]

     for (let [, value] of this.#authHandlers) {
       if (uri.match(value.uri)) {
         return value
       }
     }
    -return null
    +throw new Error(`No matching authentication handler found for URI: ${uri}`);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Adding error handling for unmatched URI patterns improves robustness by preventing silent failures and providing clear feedback when no matching handler is found. This is a significant enhancement for debugging and reliability.

    8
    Maintainability
    ✅ Ensure proper cleanup of resources when removing authentication handlers
    Suggestion Impact:The commit added a check to throw an error if the authentication handler ID is not found, which aligns with the suggestion to ensure proper handling when removing authentication handlers. However, the specific cleanup mechanism was not implemented.

    code diff:

    +    if (this.#authHandlers.has(id)) {
    +      this.#authHandlers.delete(id)
    +    } else {
    +      throw Error(`Callback with id ${id} not found`)
    +    }

    Implement a cleanup mechanism in the removeAuthenticationHandler method to ensure
    that any resources or intercepts associated with the handler are properly cleaned up
    to prevent memory leaks or dangling references.

    javascript/node/selenium-webdriver/lib/network.js [78]

    -this.#authHandlers.delete(id)
    +if (!this.#authHandlers.has(id)) {
    +  throw new Error(`No authentication handler found with ID: ${id}`);
    +}
    +// Perform any necessary cleanup related to the handler here
    +this.#authHandlers.delete(id);
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Implementing a cleanup mechanism when removing authentication handlers is important for maintainability. It prevents memory leaks and dangling references, ensuring the system remains efficient and stable.

    8
    Security
    Use explicit regular expressions for URI matching to avoid unintended matches

    To improve the security and flexibility of the authentication handlers, consider
    using a more robust pattern matching system than the simple match method, which
    might lead to unintended matches due to its reliance on converting the pattern to a
    regular expression.

    javascript/node/selenium-webdriver/lib/network.js [61-62]

    -if (uri.match(value.uri)) {
    +if (new RegExp(`^${value.uri}$`).test(uri)) {
       return value
     }
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Using explicit regular expressions for URI matching enhances security and reduces the risk of unintended matches. This is a good improvement, although the current implementation might be sufficient for many use cases.

    7

    @pujagani
    Copy link
    Contributor Author

    pujagani commented Oct 4, 2024

    @AutomatedTester Can we merge this one if everything looks good?

    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