User Agent now reports the OS platform #4937

LDSpits · 2017-09-28T09:29:07Z

This pr fixes #4913, I have not written any tests, If these are required then please notify me of this and Ill add them

iSazonov

@LDSpits Thanks for your contribution!

iSazonov · 2017-09-28T12:03:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

@@ -122,15 +123,24 @@ internal static string Platform
        {
            get
            {
-                return ("Windows NT");
+                if(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)){


I suggest use public static class Platform from CorePsPlatform.cs and rename Platform in PSUserAgent.cs to PSUserAgentPlatform.

Also please use common formatting pattern:

if ( ... ) { ... } else if ( ... ) { ... }

iSazonov · 2017-09-28T12:06:11Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

+                    return "Linux";
+                }else{
+                    // unknown/unsupported platform
+                    throw new PlatformNotSupportedException("Platform is not supported");


Maybe better Debug.Assert and returm String.Empty?

…f unsupported platform, using system.management.automation.Platform

iSazonov · 2017-09-28T13:52:21Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

@@ -123,15 +124,23 @@ internal static string Platform
        {
            get
            {
-                if(RuntimeInformation.IsOSPlatform(OSPlatform.Windows)){
+                if(System.Management.Automation.Platform.IsWindows)


Please add a space "if". Below too.

iSazonov · 2017-09-28T13:53:58Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

                    return "Windows NT";
-                }else if(RuntimeInformation.IsOSPlatform(OSPlatform.OSX)){
+                }
+                else if(System.Management.Automation.Platform.IsMacOS)


Above I suggest rename internal Platform property to exclude using "System.Management.Automation".

any preference on the name? how about OSPlatform? @iSazonov

It is a string name - maybe PlatformName?

dantraMSFT · 2017-09-28T17:37:17Z

Minor Nit: I suggest you change the title of the PR to something like 'User Agent now reports the OS platform'. When I first read the title, I was expecting to find a behavioral change that is dependent on the OS platform.

…mName

daxian-dbw · 2017-09-28T18:15:28Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

+                else
+                {
+                    // unknown/unsupported platform
+                    Debug.Assert(true, "Unable to determine Operating System Platform");


I think this should be Debug.Assert(false, "..."). And it's recommended to use Diagnostics.Assert which is from the System.Management.Automation namespace. See the code here as an example.

@daxian-dbw I have adressed your feedback.

daxian-dbw · 2017-09-29T01:06:02Z

@LDSpits Thanks for the update. Can you please add corresponding tests? You can use https://github.com/PowerShell/PowerShell/blob/master/test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1#L456 as an example to get user-agent.

markekraus

Please add tests.

markekraus · 2017-09-29T07:30:21Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

-                return ("Windows NT");
+                if (Platform.IsWindows)
+                {
+                    return "Windows NT";


I think we should add tests for these. These could either be a separate test or the existing 'User-Agent` tests could be expanded.

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

Line 456 in b07f24e

It "Invoke-WebRequest returns User-Agent" {

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

Line 1308 in b07f24e

It "Invoke-RestMethod returns User-Agent" {

The test should be patform sensetive - I'd prefer new separate test.

@iSazonov I agree.
@LDSpits you can use these tests as a base. Please include tests for both Invoke-WebRequest and Invoke-RestMethod

alright, will start work on them later today.

@LDSpits Also, it looks like the Windows platform part is supposed to include the <major>.<minor> version of windows (6.1, 10.0, etc.). I wouldn't call this a requirement, but if you could add that too it would be great!

@iSazonov I actually personally like parsing [RuntimeInformation]::OSDescription. The parsing is a one-time effort and pretty easy. I know you might concern about the consistency of the string value returned from this property ... :)

Yes. Ex. - What about Unix? I wouldn't trust OSDescription. I'd prefer to discuss all our User-Agent string (properties) before enhance this one.

I don't think we need the version info for Linux. This is what I got when using chrome to send request to http://httpbin.org/user-agent:

Windows: (Windows NT 10.0; Win64; x64)
Ubuntu: (X11; Linux x86_64)

Please see FireFox. Specially MacOS and Android.

@daxian-dbw is correct. The OS version numbers in the platform are only sent for Windows platforms. Linux platforms send either X11 or Linux (Linux is used more in CLI based HTTP agents). macOS sends Machintosh. None of those with versions.

daxian-dbw · 2017-09-29T18:04:55Z

@LDSpits One more thing, when you push new commits, please make sure the message of the last commit contains [Feature] in it so that it will trigger a full test run. Web cmdlets tests by default are not included in CI runs, so we need a full test run to validate changes related to them. Thanks!

markekraus · 2017-10-01T14:55:00Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

+                {
+                    // find the version in the windows operating system description
+                    string versionText = PSUserAgent.OS.Substring(PSUserAgent.OS.TrimEnd().LastIndexOf(" ") +1);
+                    Version windowsPlatformversion = new Version(versionText);


Since this would be dynamically generated each time the Web Cmdlets are called, but is static beyond the initial call, perhaps adding a private field _windowsPlatformversionString (or something) generated once on the first call (if (_windowsPlatformversionString == null)) and then reference it directly every time thereafter. return _windowsPlatformversionString

It is good candidate for private static variable and private static initialization function.

LDSpits · 2017-10-01T15:30:18Z

@iSazonov @daxian-dbw @markekraus Is it really nessesary to move the tests to a new file? shouldn't we add the test to WebCmdlets.tests.ps1 since most of the infrastructure required for the tests is already inside of there?

markekraus · 2017-10-01T15:36:23Z

@LDSpits My preference would be for the tests to remain in WebCmdlets.tests.ps1 along with the other User-Agent test. They should be new It blocks in the same file preferably right after the current ones. Here is the one for Invoke-WebRequest:

PowerShell/test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

Lines 456 to 467 in 2e901e5

    
           It "Invoke-WebRequest returns User-Agent" { 
        
               $uri = Get-WebListenerUrl -Test 'Get' 
        
               $command = "Invoke-WebRequest -Uri '$uri' -TimeoutSec 5" 
        
               $result = ExecuteWebCommand -command $command 
        
               ValidateResponse -response $result 
        
               # Validate response content 
        
               $jsonContent = $result.Output.Content | ConvertFrom-Json 
        
               $jsonContent.headers.'User-Agent' | Should MatchExactly '(?<!Windows)PowerShell\/\d+\.\d+\.\d+.*' 
        
           }

LDSpits · 2017-10-04T21:10:25Z

@iSazonov @daxian-dbw @markekraus can I ask a few questions about tests? I was in the process of adding tests for execute-webrequest. I have copied over most of the code from the Invoke-WebRequest returns User-Agent test with only changing the regex to '.*\(Windows NT \d+\.\d*;.*\) PowerShell\/\d+\.\d+\.\d+.*'. this test failed however. so I tested the regex using powershell (to ensure i was using the same regex parsing engine as pester). However this returned true. So now im puzzled as to why it failed. Could you maybe give some pointers?

powershell code used to test regex:

PS C:\Users\spits\Desktop\Development\Powershell\PowerShell> $bingdings =[System.Reflection.BindingFlags]::NonPublic -b
xor [System.Reflection.BindingFlags]::Static
PS C:\Users\spits\Desktop\Development\Powershell\PowerShell> [Microsoft.PowerShell.Commands.PSUserAgent].GetProperty('Us
erAgent',$bingdings).GetValue($null,$null) -match '.*\(Windows NT \d+\.\d*;.*\) PowerShell\/\d+\.\d+\.\d+.*'
True

Current (windows) test:

It "Invoke-WebRequest returns Correct User-Agent on Windows" {

        $uri = Get-WebListenerUrl -Test 'Get'
        $command = "Invoke-WebRequest -Uri '$uri' -TimeoutSec 5"

        $result = ExecuteWebCommand -command $command
        ValidateResponse -response $result

        # Validate response content
        $jsonContent = $result.Output.Content | ConvertFrom-Json
        $jsonContent.headers.'User-Agent' | Should MatchExactly '.*\(Windows NT \d+\.\d*;.*\) PowerShell\/\d+\.\d+\.\d+.*'
    }

pester test message:

Describing Invoke-WebRequest tests
 [-] Invoke-WebRequest returns Correct User-Agent on MacOSX 6.87s
   Expected: {Mozilla/5.0 (Windows NT; Microsoft Windows 10.0.16296 ; nl-NL) PowerShell/6.0.0} to exactly match the expression {.*\(Macintosh;.*\) PowerShell\/\d+\.\d+\.\d+.*}
   at line: 466 in C:\Users\spits\Desktop\Development\Powershell\PowerShell\test\powershell\Modules\Microsoft.PowerShell
.Utility\WebCmdlets.Tests.ps1
   466:         $jsonContent.headers.'User-Agent' | Should MatchExactly '.*\(Macintosh;.*\) PowerShell\/\d+\.\d+\.\d+.*'
 [-] Invoke-WebRequest returns Correct User-Agent on Linux 1.14s
   Expected: {Mozilla/5.0 (Windows NT; Microsoft Windows 10.0.16296 ; nl-NL) PowerShell/6.0.0} to exactly match the expr
ession {.*\(Linux;.*\) PowerShell\/\d+\.\d+\.\d+.*}
   at line: 479 in C:\Users\spits\Desktop\Development\Powershell\PowerShell\test\powershell\Modules\Microsoft.PowerShell
.Utility\WebCmdlets.Tests.ps1
   479:         $jsonContent.headers.'User-Agent' | Should MatchExactly '.*\(Linux;.*\) PowerShell\/\d+\.\d+\.\d+.*'
 [-] Invoke-WebRequest returns Correct User-Agent on Windows 1.08s
   Expected: {Mozilla/5.0 (Windows NT; Microsoft Windows 10.0.16296 ; nl-NL) PowerShell/6.0.0} to exactly match the expression {.*\(Windows NT \d+\.\d*;.*\) PowerShell\/\d+\.\d+\.\d+.*}
   at line: 492 in C:\Users\spits\Desktop\Development\Powershell\PowerShell\test\powershell\Modules\Microsoft.PowerShell
.Utility\WebCmdlets.Tests.ps1
   492:         $jsonContent.headers.'User-Agent' | Should MatchExactly '.*\(Windows NT \d+\.\d*;.*\) PowerShell\/\d+\.\
d+\.\d+.*'

markekraus · 2017-10-04T21:21:10Z

@LDSpits It looks like you are not using a version of PowerShell Compiled with your changes. The pester output shows Mozilla/5.0 (Windows NT; Microsoft Windows 10.0.16296 ; nl-NL) PowerShell/6.0.0 as the user-agent. note that the <major>.<minor> is missing after Windows NT

iSazonov · 2017-10-06T09:04:10Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

@@ -13,14 +14,17 @@ namespace Microsoft.PowerShell.Commands
    /// </summary>
    public static class PSUserAgent
    {
+
+        private static string _windowsUserAgent;


Please use 's_' prefix for static variables.

iSazonov · 2017-10-06T09:06:13Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

+                if (Platform.IsWindows)
+                {
+                    // only generate the windows user agent once
+                    if(PSUserAgent._windowsUserAgent == null){


Please remove 'PSUserAgent' prefix. Below too.

iSazonov · 2017-10-06T09:08:27Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs

+                    // only generate the windows user agent once
+                    if(PSUserAgent._windowsUserAgent == null){
+                        // find the version in the windows operating system description
+                        string versionText = PSUserAgent.OS.Substring(PSUserAgent.OS.TrimEnd().LastIndexOf(" ") +1);


Do we really need TrimEnd()?
If so it is better mobe it in OS property.

Please remove 'PSUserAgent' prefix.

iSazonov · 2017-10-06T09:10:53Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

@@ -453,8 +453,22 @@ Describe "Invoke-WebRequest tests" -Tags "Feature" {
        $response.Output.Content | Should Not Be $null
    }

-    It "Invoke-WebRequest returns User-Agent" {
+    #User-Agent changes on different platforms, so tests should only be run if on the correct platform
+    It "Invoke-WebRequest returns Correct User-Agent on MacOSX" -Pending:(!$IsMacOS) {


We use Pending to temporary turn out broken tests.
Please use Skip. Below too.

LDSpits · 2017-10-06T12:24:34Z

@iSazonov @daxian-dbw @markekraus the Travis build is erroring? had a problem with the previous build on the http proxy tests...

markekraus · 2017-10-06T12:57:59Z

@LDSpits Its related to #5005. We need to fix the other delay tests. ~~I'll create an issue for it.~~ PR #5035

New commits have been pushed.

daxian-dbw · 2017-10-06T18:22:57Z

#5035 has been merged and I restarted the CI builds. Hopefully the CI will pass this time.

LDSpits · 2017-10-06T19:09:45Z

Let's hope they'll pass this time 😄

markekraus

LGTM

daxian-dbw · 2017-10-06T21:00:35Z

@LDSpits Thanks for your contribution!
@markekraus @iSazonov Thank you for the thorough review!

markekraus · 2017-10-06T21:05:21Z

@LDSpits Thanks for doing this!

LDSpits · 2017-10-06T21:13:34Z

@daxian-dbw @iSazonov @markekraus No problem, expect to see me around 😄

Powershell web useragent now takes the OS into account

21d32db

LDSpits requested review from dantraMSFT, JamesWTruher and PaulHigin as code owners September 28, 2017 09:29

add missing semicolon

5502909

iSazonov reviewed Sep 28, 2017

View reviewed changes

changed code to follow code conventions, using debug.assert to warn o…

7294421

…f unsupported platform, using system.management.automation.Platform

iSazonov reviewed Sep 28, 2017

View reviewed changes

LDSpits changed the title ~~Powershell web useragent now takes the OS into account~~ User Agent now reports the OS platform Sep 28, 2017

further code style improvements, renamed property Platform to Platfor…

91352cb

…mName

daxian-dbw self-assigned this Sep 28, 2017

daxian-dbw previously requested changes Sep 28, 2017

View reviewed changes

now calling assert from correct class with correct parameters

d476ea8

markekraus suggested changes Sep 29, 2017

View reviewed changes

LDSpits changed the title ~~User Agent now reports the OS platform~~ [WIP] User Agent now reports the OS platform Sep 29, 2017

iSazonov mentioned this pull request Sep 29, 2017

Platform should return cross platform values under PSUserAgent #4945

Closed

windows platform string now includes version numbers

510039a

markekraus suggested changes Oct 1, 2017

View reviewed changes

psuseragent now only creates the windows useragent string once

5f3c262

[Feature] Added tests for dynamic user-agents

8a99117

LDSpits changed the title ~~[WIP] User Agent now reports the OS platform~~ User Agent now reports the OS platform Oct 5, 2017

[Feature] fixed Linux Invoke-RestMethod Useragent test

195a018

iSazonov reviewed Oct 6, 2017

View reviewed changes

[Feature] Adressed feedback

3d2358f

markekraus approved these changes Oct 6, 2017

View reviewed changes

[Feature] Possible fix for problem with user-agent on mac

c1d6769

markekraus mentioned this pull request Oct 6, 2017

Fix Delay Test Race Condition #5035

Merged

[Feature] reverted to previous version because of unreliable testrun

56b71af

markekraus approved these changes Oct 6, 2017

View reviewed changes

daxian-dbw merged commit e1a0caf into PowerShell:master Oct 6, 2017

LDSpits deleted the hotfix/correct-useragent branch October 6, 2017 21:14

markekraus mentioned this pull request Oct 26, 2017

Invoke-RestMethod started to fail in v6-beta.9 on a Windows 7 machine #5242

Closed

User Agent now reports the OS platform #4937

User Agent now reports the OS platform #4937

Conversation

LDSpits commented Sep 28, 2017

iSazonov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dantraMSFT commented Sep 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented Sep 29, 2017

markekraus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw Sep 29, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxian-dbw commented Sep 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LDSpits commented Oct 1, 2017 • edited

markekraus commented Oct 1, 2017

LDSpits commented Oct 4, 2017

markekraus commented Oct 4, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LDSpits commented Oct 6, 2017

markekraus commented Oct 6, 2017 • edited

daxian-dbw commented Oct 6, 2017

LDSpits commented Oct 6, 2017

markekraus left a comment

Choose a reason for hiding this comment

daxian-dbw commented Oct 6, 2017

markekraus commented Oct 6, 2017

LDSpits commented Oct 6, 2017 • edited

daxian-dbw Sep 29, 2017 •

edited

LDSpits commented Oct 1, 2017 •

edited

markekraus commented Oct 4, 2017 •

edited

markekraus commented Oct 6, 2017 •

edited

LDSpits commented Oct 6, 2017 •

edited