-
Notifications
You must be signed in to change notification settings - Fork 189
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
feat: Add support for Relax SSL validation #922
Conversation
Added a new property `RelaxSslValidation` to the `AppiumClientConfig` class to allow users to specify whether SSL validation should be relaxed. Introduced a new method `IsModified` that checks if the current configuration has been modified from the default configuration, specifically comparing the `DirectConnect` and `RelaxSslValidation` properties.
Introduce AppiumHttpCommandExecutor in OpenQA.Selenium.Appium.Service. This class inherits from HttpCommandExecutor and includes two constructors for initializing with a Uri and TimeSpan, with an optional AppiumClientConfig. Override CreateHttpClientHandler to customize HttpClientHandler with a callback for server certificate validation. Add ModifyHttpClientHandler method to return the customized handler. Include necessary using directives.
Replaced instances of HttpCommandExecutor with AppiumHttpCommandExecutor in AppiumCommandExecutor.cs. Updated CreateRealExecutor and GetNewExecutorWithDirectConnect methods to return AppiumHttpCommandExecutor. Modified ModifyNewSessionHttpRequestHeader to cast commandExecutor to AppiumHttpCommandExecutor and added a call to ModifyHttpClientHandler with ClientConfig. These changes enhance the command execution process by leveraging Appium-specific functionalities.
Updated CreateRealExecutor to accept AppiumClientConfig and pass it to AppiumHttpCommandExecutor. Modified constructors of AppiumCommandExecutor to include clientConfig. Reformatted Execute and HandleCommandException methods for consistency. Cleaned up ModifyNewSessionHttpRequestHeader method to remove commented-out code and ensure IdempotencyHeader is added. Updated AppiumHttpCommandExecutor constructor to optionally accept clientConfig and initialize _clientConfig. Adjusted CreateHttpClientHandler to set ServerCertificateCustomValidationCallback based on RelaxSslValidation property. Removed obsolete ModifyHttpClientHandler method.
A new test method `SetAndGetRelaxSSLValidation` was added to the `AppiumClientConfigTest` class within the `Appium.Net.Integration.Tests.ServerTests` namespace. This test verifies the `RelaxSslValidation` property of the `AppiumClientConfig` class. It asserts that the default value of `RelaxSslValidation` is `False`, then sets it to `True` and asserts the updated value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for relaxing SSL validation in the Appium .NET client while refactoring the command executor implementation.
- Introduces a new RelaxSslValidation property in AppiumClientConfig and corresponding integration test.
- Implements a new AppiumHttpCommandExecutor that integrates the RelaxSslValidation setting into HTTP client handler creation.
- Updates AppiumCommandExecutor to utilize AppiumHttpCommandExecutor in various execution flows.
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/Appium.Net/Appium/Service/AppiumHttpCommandExecutor.cs | New executor class that configures SSL validation based on AppiumClientConfig. |
src/Appium.Net/Appium/Service/AppiumClientConfig.cs | Added RelaxSslValidation property to control SSL certificate handling. |
src/Appium.Net/Appium/Service/AppiumCommandExecutor.cs | Updated instantiation and casting to use the new executor class. |
test/integration/ServerTests/AppiumClientConfigTest.cs | Added test to verify RelaxSslValidation property behavior. |
@nvborisenko, Your input is always welcomed. |
Just test it when a client is targeted to |
Elaborating more: I concern it works good in |
Refactored the `AppiumHttpCommandExecutor` class to simplify initialization: - Moved constructor parameters directly into the class definition. - Initialized `_clientConfig` field at the point of declaration. - Removed redundant constructor definition and initialization of `_clientConfig` within the constructor body.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new configuration property to allow relaxing SSL validation and refactors command executor handling to use a new executor class.
- Added a new RelaxSslValidation property to AppiumClientConfig and corresponding tests.
- Introduced a new AppiumHttpCommandExecutor that overrides the HTTP handler creation for relaxed SSL validation.
- Updated AppiumCommandExecutor to instantiate the new executor class and pass through the updated client configuration.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
test/integration/ServerTests/RelaxSslValidationTest.cs | New integration tests to validate relaxed SSL validation behavior. |
src/Appium.Net/Appium/Service/AppiumHttpCommandExecutor.cs | New executor class implementing relaxed SSL validation logic. |
test/integration/ServerTests/AppiumClientConfigTest.cs | Added test for the new RelaxSslValidation property. |
src/Appium.Net/Appium/Service/AppiumClientConfig.cs | Added RelaxSslValidation property to client configuration. |
src/Appium.Net/Appium/Service/AppiumCommandExecutor.cs | Refactored executor creation to instantiate AppiumHttpCommandExecutor. |
Comments suppressed due to low confidence (1)
test/integration/ServerTests/RelaxSslValidationTest.cs:62
- Accessing nested inner exceptions without null checks may lead to a NullReferenceException. Consider using e.GetBaseException().Message or add proper null checks.
var innerExceptionMessage = e.InnerException.InnerException.Message;
List of changes
This pull request introduces several changes to the Appium .NET client to enhance SSL validation handling and refactor the command executor. The most important changes include adding a new configuration option for relaxing SSL validation, creating a new
AppiumHttpCommandExecutor
class, and updating the command executor to use this new class.Enhancements to SSL validation handling:
src/Appium.Net/Appium/Service/AppiumClientConfig.cs
: Added a new propertyRelaxSslValidation
to theAppiumClientConfig
class.src/Appium.Net/Appium/Service/AppiumHttpCommandExecutor.cs
: Created a newAppiumHttpCommandExecutor
class that inherits fromHttpCommandExecutor
and includes logic to handle theRelaxSslValidation
property.Refactoring command executor:
src/Appium.Net/Appium/Service/AppiumCommandExecutor.cs
: Updated theAppiumCommandExecutor
class to useAppiumHttpCommandExecutor
instead ofHttpCommandExecutor
in multiple methods, includingCreateRealExecutor
,HandleCommandException
, andGetNewExecutorWithDirectConnect
. [1] [2] [3] [4]Testing:
test/integration/ServerTests/AppiumClientConfigTest.cs
: Added a new testSetAndGetRelaxSSLValidation
to verify the functionality of theRelaxSslValidation
property inAppiumClientConfig
.Types of changes
What types of changes are you proposing/introducing to the .NET client?
Put an
x
in the boxes that applyDocumentation
This can be done by navigating to the documentation section on http://appium.io selecting the appropriate command/endpoint and clicking the 'Edit this doc' link to update the C# example
Integration tests
Details
Please provide more details about changes if necessary. You can provide code samples showing how they work and possible use cases if there are new features. Also, you can create gists with pasted C# code samples or put them here using markdown.
About markdown please read Mastering markdown and Writing on GitHub