Skip to content

Microsoft.PowerShell.Commands.WebProxy replace System.Net.IWebProxy with System.Net.WebProxy #19609

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.

Sign up for GitHub

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

Closed
CarloToso wants to merge 8 commits into from
Closed

Microsoft.PowerShell.Commands.WebProxy replace System.Net.IWebProxy with System.Net.WebProxy #19609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
fb22f7f
Replace Microsoft.PowerShell.Commands.WebProxy with System.Net.WebProxy
CarloToso May 3, 2023
dc452cc
Merge branch 'master' into proxy
CarloToso May 4, 2023
4375138
add back Microsoft.PowerShell.Commands.WebProxy
CarloToso May 6, 2023
f474bca
use == operator for Address
CarloToso May 6, 2023
25c6ef8
More in depth WebProxy comparison
CarloToso May 8, 2023
bebe5b3
more in depth equality fix; cleanup nested if
CarloToso May 8, 2023
8be91a2
compare BypassList
CarloToso May 8, 2023
adbafe8
Merge branch 'PowerShell:master' into proxy
CarloToso May 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 16 additions & 19 deletions ...owerShell.Commands.Utility/commands/utility/WebCmdlet/Common/WebRequestPSCmdlet.Common.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -958,28 +958,25 @@ internal virtual void PrepareSession()
{
WebSession.NoProxy = true;
}
else
else if (Proxy is not null)
{
if (Proxy is not null)
WebProxy webProxy = new(Proxy);

if (ProxyCredential is not null)
{
WebProxy webProxy = new(Proxy);
webProxy.BypassProxyOnLocal = false;
if (ProxyCredential is not null)
{
webProxy.Credentials = ProxyCredential.GetNetworkCredential();
}
else
{
webProxy.UseDefaultCredentials = ProxyUseDefaultCredentials;
}
webProxy.Credentials = ProxyCredential.GetNetworkCredential();
}
else
{
webProxy.UseDefaultCredentials = ProxyUseDefaultCredentials;
}

// We don't want to update the WebSession unless the proxies are different
// as that will require us to create a new HttpClientHandler and lose connection
// persistence.
if (!webProxy.Equals(WebSession.Proxy))
{
WebSession.Proxy = webProxy;
}
// We don't want to update the WebSession unless the proxies are different
// as that will require us to create a new HttpClientHandler and lose connection
// persistence.
if (!webProxy.Equals(WebSession.Proxy))
Copy link
Collaborator

@iSazonov iSazonov May 9, 2023

Choose a reason for hiding this comment

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

Really we use 3 parameters to create webProxy. So we could keep them in WebSession
to track WebSession.Proxy changes. This also exclude creating proxy in line 963 if no need.

Copy link
Contributor

@stevenebutler stevenebutler May 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

At simplest level, couldn't the comparison be something like this (excuse the possible namespace confusion, I've typed this directly into GitHub comment uncompiled/untested)?

if (!string.IsNullOrEmpty(Proxy) && Proxy != (WebSession?.Proxy as System.Net.WebProxy)?.Address)
{
    if (WebSession.Proxy is System.Net.WebProxy wsProxy)
    {
        wsProxy.Address = Proxy;
    }
    else
    {
        // Proxy type is unexpected so replace with standard proxy (since the user supplied the Proxy parameter)
        WebSession.Proxy = new System.Net.WebProxy(Proxy, true);
    }
}

This may not pass the existing tests because reassigning Address from Proxy parameter when they're different won't cause the HttpClient to be recreated, but it should work, and that test's expected result could be updated accordingly.

If you're also trying to deal with the BypassProxyOnLocal parameter (did you add that to the CmdLet because there's no way in current PowerShell to affect that setting), you'll need some extra logic for that too, but it can also be modified
directly on the Proxy class without needing to recreate it (provided the stored WebSession.Proxy is a System.Net.WebProxy object and not some custom Proxy class designed by the user).

Edit: I just noticed I used Address instead of Proxy in the new System.Net.WebProxy(Proxy, true); (third last line)

{
WebSession.Proxy = webProxy;
}
}

Expand Down
52 changes: 12 additions & 40 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/WebProxy.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,62 +4,34 @@
#nullable enable

using System;
using System.Net;
using System.Collections;
using System.Collections.Generic;

namespace Microsoft.PowerShell.Commands
{
internal class WebProxy : IWebProxy, IEquatable<WebProxy>
internal class WebProxy : System.Net.WebProxy, IEquatable<WebProxy>
Copy link
Collaborator

@iSazonov iSazonov May 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I would assume that reference equality is enough to make the cache work for most real-world scenarios. Then we could still remove this type.

Copy link
Contributor Author

@CarloToso CarloToso May 7, 2023

Choose a reason for hiding this comment

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

Using ReferenceEquals does not work, the tests fail

Copy link
Contributor

@stevenebutler stevenebutler May 7, 2023

Choose a reason for hiding this comment

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

I think it is still possible to remove the class - the Address property on System.Net.WebProxy is available and could be used to compare with the passed in Proxy parameter (if one is provided).

The Address property on System.Net.WebProxy is also settable, and can be changed without causing exceptions between requests from HttpClient so it may be possible to simply update the Address property on the existing Proxy provided:

  • the WebSession already has a Proxy (i.e. this is not a brand new WebSession)
  • the type of the Proxy is System.Net.WebProxy

Updating the proxy Address property does cause a new connection to be made (to connect to the new proxy), but this doesn't require creation of a new HttpClientHandler.

Note that no new connection is required if the user updates the Address property to the same value it already had.

Copy link
Collaborator

@iSazonov iSazonov May 8, 2023

Choose a reason for hiding this comment

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

Using ReferenceEquals does not work, the tests fail

I don't see the commit.

Copy link
Contributor Author

@CarloToso CarloToso May 8, 2023

Choose a reason for hiding this comment

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

I tested in in local

Copy link
Collaborator

@iSazonov iSazonov May 8, 2023

Choose a reason for hiding this comment

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

I see no reason why it wouldn't work. Unless there happens to be wrapping in PSObject which can be bypassed.

Copy link
Contributor

@stevenebutler stevenebutler May 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't work because there is a test that checks that passing the same -Proxy repeatedly with a persistent websession doesn't cause a new httpclienthandler to be created. If you use referenceequals then this will not be true.

The code from the Equals() could be moved into the CmdLet if you want to retain the existing behaviour but remove the internal WebProxy class completely.

Also note that you can modify properties on the WebProxy without impacting the HttpClientHandler so there's no need to really check all the properties. The only thing that really makes a difference is the Proxy Address.

{
private ICredentials? _credentials;
private readonly Uri _proxyAddress;

internal WebProxy(Uri address)
internal WebProxy(Uri? address)
{
ArgumentNullException.ThrowIfNull(address);

_proxyAddress = address;
Address = address;
}

public override bool Equals(object? obj) => Equals(obj as WebProxy);

public override int GetHashCode() => HashCode.Combine(_proxyAddress, _credentials, BypassProxyOnLocal);
public override int GetHashCode() => HashCode.Combine(Address, Credentials, BypassProxyOnLocal);

public bool Equals(WebProxy? other)
{
if (other is null)
{
return false;
}

// _proxyAddress cannot be null as it is set in the constructor
return other._credentials == _credentials
&& _proxyAddress.Equals(other._proxyAddress)
&& BypassProxyOnLocal == other.BypassProxyOnLocal;

return Address == other.Address
&& Credentials == other.Credentials
&& BypassProxyOnLocal == other.BypassProxyOnLocal
&& UseDefaultCredentials == other.UseDefaultCredentials
&& (BypassList as IStructuralEquatable).Equals(other.BypassList, EqualityComparer<string>.Default);
}

public ICredentials? Credentials
{
get => _credentials;

set => _credentials = value;
}

internal bool BypassProxyOnLocal { get; set; }

internal bool UseDefaultCredentials
{
get => _credentials == CredentialCache.DefaultCredentials;

set => _credentials = value ? CredentialCache.DefaultCredentials : null;
}

public Uri GetProxy(Uri destination)
{
ArgumentNullException.ThrowIfNull(destination);

return destination.IsLoopback ? destination : _proxyAddress;
}

public bool IsBypassed(Uri host) => host.IsLoopback;
}
}