Fix Web Cmdlets for .NET Core 2.1 - #6806
Conversation
Mark Kraus (markekraus)
added
the
Breaking-Change
|
I restarted CI Travis MacOs - it seems temporary failed. |
Mark Kraus (markekraus)
changed the title
|
Travis Plunk (@TravisEz13) can you ping someone to review? I'd definitely like this to be merged before the next preview or RC since without it, there are effectively known regressions in master. |
|
Ilya (@iSazonov) Could you review this change? |
Ilya (iSazonov)
left a comment
Ilya (iSazonov)
left a comment
There was a problem hiding this comment.
Mark Kraus (@markekraus) Please look https://www.codefactor.io/repository/github/powershell/powershell/pull/6806# It is not request to fix.
| using (client = GetHttpClient(false)) | ||
| using (HttpRequestMessage redirectRequest = GetRequest(currentUri, stripAuthorization:true)) | ||
| // Continue to handle redirection | ||
| using (client = GetHttpClient(true)) |
There was a problem hiding this comment.
Usually we use named parameter GetHttpClient(handleRedirect:true) - it is more readable.
There was a problem hiding this comment.
Not "usually". there are plenty of examples where the pattern is not used. It's also not listed in the guidance. Please add it to the guidance if you expect it in submissions. I will change it.
There was a problem hiding this comment.
We have so many patterns that it is impossible to document them all. So we have activated CodeFactor and will do fine tuning. We should then update our guidances.
|
Ilya (@iSazonov) I have #5610 for tracking refactoring the Web Cmdlets. There is much work to be done there.. but every time I try, the PRs are a nightmare. I'm pretty discourage from wasting my time on it further until I can get clear answers on the way to move forward. |
|
Note: there are CodeFactor issues. We should try to fix these error in the new and changed code. This was added after you started this PR so I think we can defer this. |
closes PowerShell#6728 Breaking change approved in PowerShell#6728 This PR switches the logic of when the Web Cmdlets handle redirects when the Authorization header is present. .NET Core 2.1 no longer sends the Authorization header by default (dotnet/corefx#26864). however, we introduced the ability to do so leveraging the previous default behavior through the use of the -PreserveAuthorizationOnRedirect switch. This PR also corrects a bug introduced 6.0.0 where certain redirect types redirect from POST to GET were set which should have passed through POST to POST and some were improperly passing through POST to POST which should have been doing POST to GET. This correction is a breaking change. It was made apparent as now the redirection behavior is being managed by CoreFX which is doing the correct behavior, tests were added for both when CoreFX and the Web Cmdlets manage redirection.
4 participants
PR Summary
This PR switches the logic of when the Web Cmdlets handle redirects when the Authorization header is present. .NET Core 2.1 no longer sends the Authorization header by default (dotnet/corefx#26864). however, we introduced the ability to do so leveraging the previous default behavior through the use of the
-PreserveAuthorizationOnRedirectswitch.This PR also corrects a bug introduced 6.0.0 where certain redirect types redirect from POST to GET were set which should have passed through POST to POST and some were improperly passing through POST to POST which should have been doing POST to GET. This correction is a breaking change. It was made apparent as now the redirection behavior is being managed by CoreFX which is doing the correct behavior, test were added for both when CoreFX and the Web Cmdlets manage redirection.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.[feature]if the change is significant or affects feature tests