[rb] Add Bidi network commands for authentication and interception

[aguspe]

Description

This PR adds the following:

• A Network bidi class that allows to add, remove and clear auth handlers
• A Network class to be call from driver
• Unit and integration tests

Motivation and Context

Based on #13993 this PR follows the agreed defined API methods implemented for the Ruby binding, now is possible to call driver.network.add_authentication_handler

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

• Introduced a new Network class within the BiDi module to handle network events and authentication.
• Added methods for intercepting network requests and managing authentication handlers.
• Implemented integration and unit tests for the new Network class to ensure functionality.
• Updated type signatures for the Network class and related methods.

---

Changes walkthrough 📝

	Relevant files
Enhancement	5 files bidi.rb Add autoload for Network in BiDi class                                      rb/lib/selenium/webdriver/bidi.rb Added autoload for Network in the BiDi class. +1/-0      network.rb Introduce Network class with intercept and auth handling  rb/lib/selenium/webdriver/bidi/network.rb Introduced a new Network class within BiDi. Defined network events and phases. Implemented methods for intercepting network requests and handling authentication. +70/-0    common.rb Add require statement for network                                                rb/lib/selenium/webdriver/common.rb Added require statement for network. +1/-0      driver.rb Add network method to Driver class                                              rb/lib/selenium/webdriver/common/driver.rb Added network method to return a Network instance. +9/-0      network.rb Create Network class for authentication management              rb/lib/selenium/webdriver/common/network.rb Created a Network class for managing authentication handlers. Implemented methods to add and remove authentication handlers. +46/-0
Formatting	1 files struct.rb Fix comment formatting in BiDi module                                        rb/lib/selenium/webdriver/bidi/struct.rb Fixed comment formatting for the BiDi module. +1/-3
Tests	3 files network_spec.rb Add integration tests for BiDi Network class                          rb/spec/integration/selenium/webdriver/bidi/network_spec.rb Added integration tests for Network class in BiDi. Tested intercept addition, removal, and authentication continuation. +65/-0    network_spec.rb Add integration tests for Network class                                    rb/spec/integration/selenium/webdriver/network_spec.rb Added integration tests for Network class. Tested adding and removing authentication handlers. +48/-0    network_spec.rb Add unit tests for BiDi Network class                                        rb/spec/unit/selenium/webdriver/bidi/network_spec.rb Added unit tests for Network class in BiDi. Mocked tests for intercept and authentication handling. +63/-0
Documentation	3 files network.rbs Add type signatures for BiDi Network class                              rb/sig/lib/selenium/webdriver/bidi/network.rbs Added type signatures for Network class in BiDi. +23/-0    driver.rbs Add type signature for network method                                        rb/sig/lib/selenium/webdriver/common/driver.rbs Added type signature for network method. +2/-0      network.rbs Add type signatures for Network class                                        rb/sig/lib/selenium/webdriver/common/network.rbs Added type signatures for Network class. +15/-0

---

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

[codiumai-pr-agent-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 PR contains tests

🔒 Security concerns Sensitive information exposure: The add_auth_handler method in rb/lib/selenium/webdriver/common/network.rb stores username and password in memory. While this is necessary for the functionality, it's important to ensure that these credentials are not logged or exposed in any way. Consider implementing additional safeguards to protect this sensitive information, such as encrypting the stored credentials or clearing them from memory as soon as they're no longer needed.

⚡ Recommended focus areas for review Potential Race Condition The AUTH_CALLBACKS hash is shared across all instances of the Network class. This could lead to race conditions if multiple threads are adding or removing auth handlers simultaneously. Error Handling The class lacks error handling for network operations. Consider adding error handling for network failures or invalid responses. Memory Leak The AUTH_CALLBACKS hash might grow indefinitely if auth handlers are added but not removed. Consider implementing a cleanup mechanism.

[codiumai-pr-agent-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Score
Enhancement	✅ Improve thread safety for managing authentication callbacks Suggestion Impact: The commit replaced the class variable AUTH_CALLBACKS with an instance variable @auth_callbacks, which improves encapsulation and thread safety, aligning with the suggestion's intent to enhance thread safety. code diff: - AUTH_CALLBACKS = {} + attr_reader :auth_callbacks + def initialize(bridge) @network = BiDi::Network.new(bridge.bidi) + @auth_callbacks = {} end def add_auth_handler(username, password) @@ -31,15 +33,19 @@ request_id = event['requestId'] @network.continue_with_auth(request_id, username, password) end - AUTH_CALLBACKS[auth_id] = intercept + @auth_callbacks[auth_id] = intercept auth_id end def remove_auth_handler(id) - intercept = AUTH_CALLBACKS[id] + intercept = @auth_callbacks[id] @network.remove_intercept(intercept['intercept']) - AUTH_CALLBACKS.delete(id) + @auth_callbacks.delete(id) Consider using a more thread-safe approach for managing AUTH_CALLBACKS. Instead of using a class variable, you could use a thread-local variable or a synchronized data structure to ensure thread safety in multi-threaded environments. rb/lib/selenium/webdriver/common/network.rb [23-26] -AUTH_CALLBACKS = {} +def self.auth_callbacks + Thread.current[:auth_callbacks] ||= {} +end + def initialize(bridge) @network = BiDi::Network.new(bridge.bidi) end Apply this suggestion Suggestion importance[1-10]: 8 Why: The suggestion to use a thread-local variable for AUTH_CALLBACKS enhances thread safety, which is crucial in multi-threaded environments. This change addresses a potential concurrency issue, making it a significant improvement.	8
	Add test case for handling removal of non-existent intercepts Consider adding a test case for handling invalid or non-existent intercepts when trying to remove them. This will ensure that the error handling for such cases is properly implemented and tested. rb/spec/integration/selenium/webdriver/bidi/network_spec.rb [39-45] it 'removes an intercept' do reset_driver!(web_socket_url: true) do |driver| network = described_class.new(driver.bidi) intercept = network.add_intercept(phases: [described_class::PHASES[:before_request]]) expect(network.remove_intercept(intercept['intercept'])).to be_empty end end +it 'handles removal of non-existent intercept' do + reset_driver!(web_socket_url: true) do |driver| + network = described_class.new(driver.bidi) + expect { network.remove_intercept('non_existent_intercept') }.to raise_error(StandardError) + end +end + Apply this suggestion Suggestion importance[1-10]: 6 Why: Adding a test case for removing non-existent intercepts ensures that the error handling logic is properly tested. This enhances the test coverage and reliability of the codebase.	6
Possible issue	Add error handling for non-existent authentication handlers during removal Consider adding error handling for cases where the authentication handler or intercept might not exist when trying to remove them. This can prevent potential runtime errors if an invalid ID is provided. rb/lib/selenium/webdriver/common/network.rb [39-43] def remove_auth_handler(id) intercept = AUTH_CALLBACKS[id] - @network.remove_intercept(intercept['intercept']) - AUTH_CALLBACKS.delete(id) + if intercept + @network.remove_intercept(intercept['intercept']) + AUTH_CALLBACKS.delete(id) + else + raise KeyError, "No authentication handler found for id: #{id}" + end end Apply this suggestion Suggestion importance[1-10]: 7 Why: Adding error handling for cases where an authentication handler might not exist prevents potential runtime errors. This is a valuable enhancement that improves the robustness of the code.	7
Best practice	Improve consistency in constant naming convention for event types Consider using a more descriptive constant name for 'FETCH_ERROR' to maintain consistency with other event names. For example, 'fetch_error' would align better with the naming convention used for other events. rb/lib/selenium/webdriver/bidi/network.rb [24-30] EVENTS = { before_request: 'network.beforeRequestSent', response_started: 'network.responseStarted', response_completed: 'network.responseCompleted', auth_required: 'network.authRequired', - FETCH_ERROR: 'network.fetchError' + fetch_error: 'network.fetchError' }.freeze Apply this suggestion Suggestion importance[1-10]: 5 Why: The suggestion to change 'FETCH_ERROR' to 'fetch_error' improves consistency with the naming convention used for other events in the EVENTS hash. While this is a minor stylistic improvement, it enhances readability and uniformity in the codebase.	5

💡 Need additional feedback ? start a PR chat