Skip to content
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

Jump to bottom

User Agent now reports the OS platform #4937

Merged
merged 12 commits into from Oct 6, 2017
Merged

User Agent now reports the OS platform #4937

Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
21d32db
Powershell web useragent now takes the OS into account
LDSpits Sep 28, 2017
5502909
add missing semicolon
LDSpits Sep 28, 2017
7294421
changed code to follow code conventions, using debug.assert to warn o…
LDSpits Sep 28, 2017
91352cb
further code style improvements, renamed property Platform to Platfor…
LDSpits Sep 28, 2017
d476ea8
now calling assert from correct class with correct parameters
LDSpits Sep 28, 2017
510039a
windows platform string now includes version numbers
LDSpits Oct 1, 2017
5f3c262
psuseragent now only creates the windows useragent string once
LDSpits Oct 3, 2017
8a99117
[Feature] Added tests for dynamic user-agents
LDSpits Oct 5, 2017
195a018
[Feature] fixed Linux Invoke-RestMethod Useragent test
LDSpits Oct 6, 2017
3d2358f
[Feature] Adressed feedback
LDSpits Oct 6, 2017
c1d6769
[Feature] Possible fix for problem with user-agent on mac
LDSpits Oct 6, 2017
56b71af
[Feature] reverted to previous version because of unreliable testrun
LDSpits Oct 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
37 changes: 28 additions & 9 deletions src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/PSUserAgent.cs
View file
Open in desktop
Expand Up @@ -4,7 +4,9 @@

using System;
using System.Management.Automation;
using System.Runtime.InteropServices;
using System.Globalization;
using System.Diagnostics;

namespace Microsoft.PowerShell.Commands
{
Expand All @@ -20,7 +22,7 @@ internal static string UserAgent
// format the user-agent string from the various component parts
string userAgent = string.Format(CultureInfo.InvariantCulture,
"{0} ({1}; {2}; {3}) {4}",
Compatibility, Platform, OS, Culture, App);
Compatibility, PlatformName, OS, Culture, App);
return (userAgent);
}
}
Expand All @@ -35,7 +37,7 @@ public static string InternetExplorer
// format the user-agent string from the various component parts
string userAgent = string.Format(CultureInfo.InvariantCulture,
"{0} (compatible; MSIE 9.0; {1}; {2}; {3})",
Compatibility, Platform, OS, Culture);
Compatibility, PlatformName, OS, Culture);
return (userAgent);
}
}
Expand All @@ -50,7 +52,7 @@ public static string FireFox
// format the user-agent string from the various component parts
string userAgent = string.Format(CultureInfo.InvariantCulture,
"{0} ({1}; {2}; {3}) Gecko/20100401 Firefox/4.0",
Compatibility, Platform, OS, Culture);
Compatibility, PlatformName, OS, Culture);
return (userAgent);
}
}
Expand All @@ -65,7 +67,7 @@ public static string Chrome
// format the user-agent string from the various component parts
string userAgent = string.Format(CultureInfo.InvariantCulture,
"{0} ({1}; {2}; {3}) AppleWebKit/534.6 (KHTML, like Gecko) Chrome/7.0.500.0 Safari/534.6",
Compatibility, Platform, OS, Culture);
Compatibility, PlatformName, OS, Culture);
return (userAgent);
}
}
Expand All @@ -80,7 +82,7 @@ public static string Opera
// format the user-agent string from the various component parts
string userAgent = string.Format(CultureInfo.InvariantCulture,
"Opera/9.70 ({0}; {1}; {2}) Presto/2.2.1",
Platform, OS, Culture);
PlatformName, OS, Culture);
return (userAgent);
}
}
Expand All @@ -95,7 +97,7 @@ public static string Safari
// format the user-agent string from the various component parts
string userAgent = string.Format(CultureInfo.InvariantCulture,
"{0} ({1}; {2}; {3}) AppleWebKit/533.16 (KHTML, like Gecko) Version/5.0 Safari/533.16",
Compatibility, Platform, OS, Culture);
Compatibility, PlatformName, OS, Culture);
return (userAgent);
}
}
Expand All @@ -118,19 +120,36 @@ internal static string App
}
}

internal static string Platform
internal static string PlatformName
{
get
{
return ("Windows NT");
if (Platform.IsWindows)
{
return "Windows NT";
Copy link
Contributor

Choose a reason for hiding this comment

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

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" {

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

alright, will start work on them later today.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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!

Copy link
Member

Choose a reason for hiding this comment

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

@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 ... :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please see FireFox. Specially MacOS and Android.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

}
else if (Platform.IsMacOS)
{
return "Macintosh";
}
else if (Platform.IsLinux)
{
return "Linux";
}
else
{
// unknown/unsupported platform
Debug.Assert(true, "Unable to determine Operating System Platform");
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@daxian-dbw I have adressed your feedback.

return String.Empty;
}
}
}

internal static string OS
{
get
{
return System.Runtime.InteropServices.RuntimeInformation.OSDescription;
return RuntimeInformation.OSDescription;
}
}

Expand Down