Enable WSManCredSSP cmdlets by SteveL-MSFT · Pull Request #4336 · PowerShell/PowerShell

Steve Lee (SteveL-MSFT) · 2017-07-24T23:15:14Z

Fixed error message using a null resourcemanager
With most recent CoreFx supporting STA, removed special CoreClr code paths
Added test coverage
Removed some dead commented out code that was never used

Part of #4158

Fix #3353

Steve Lee (SteveL-MSFT) · 2017-07-25T16:36:39Z

AppVeyor failure waiting on #4323

Mike Richmond (mirichmo) · 2017-07-26T00:59:26Z

Why were these two checks removed?

line 486 and 890 already does the check

Mike Richmond (mirichmo) · 2017-07-26T01:08:15Z

Ca you rename this variable to remove the initial "g"?

Mike Richmond (mirichmo) · 2017-07-26T01:08:30Z

Why were these two checks removed?

check happens twice

Mike Richmond (mirichmo) · 2017-07-26T01:11:05Z

Does this line work on non-Windows platforms?

Code should not be outside of a Pester block. This should be moved within a BeforeAll

Check out the guidelines on Admin for Windows

Consider using the pattern for skipping tests as well.

will wrap in $IsWindows

will wrap in BeforeAll

this is the opposite of requiring admin, this test requires NOT admin (there isn't a RequireNotAdminOnWindows)

two different test cases here where the first is only applicable when NOT admin, the second one works either way

Mike Richmond (mirichmo) · 2017-07-26T01:17:52Z

Why are these tests separated into separate files? We have been grouping tests for each cmdlet into a single file.

I suppose I can combine them. I thought to separate them because at least one test case requires non-admin.

Just tried it combined and start-pspester only ran the tests as elevated so that one test gets skipped. I'll have to have the non-admin tests as separate.

Actually it appears you have explicitly run start-pspester with -unelevated. I'll have to take care of this myself in the script then.

Mike Richmond (mirichmo) · 2017-07-26T01:18:23Z

The same comments apply to this file as well

Steve Lee (SteveL-MSFT) · 2017-07-26T03:33:32Z

this is redundant from line 486

Steve Lee (SteveL-MSFT) · 2017-07-26T20:49:51Z

CI failure is due to one of the same failures in the nightly run unrelated to this PR. I'll fix that first.

Aditya Patwardhan (adityapatwardhan) · 2017-07-28T21:07:23Z

Please check for en-US before comparing.

Aditya Patwardhan (adityapatwardhan) · 2017-07-28T21:07:50Z

Same as above

Aditya Patwardhan (adityapatwardhan) · 2017-07-28T21:08:29Z

Same as above.

Aditya Patwardhan (adityapatwardhan) · 2017-07-28T21:08:47Z

Same as above

Steve Lee (SteveL-MSFT) · 2017-08-01T04:39:39Z

Mike Richmond (@mirichmo) Aditya Patwardhan (@adityapatwardhan) feedback addressed

Steve Lee (SteveL-MSFT) · 2017-08-04T02:26:54Z

Mike Richmond (@mirichmo) can you complete your review?

Mike Richmond (mirichmo) · 2017-08-07T21:41:51Z

As a general rule when using this pattern, a test that is skipped in an environment should not execute any code.

Lines 11 through 15 should not be executed if the test is skipped. Putting them in an else {} would solve this problem.

Aditya Patwardhan (adityapatwardhan) · 2017-08-08T00:33:44Z

Mike Richmond (@mirichmo) Can you rebase, it should fix the test failures.

Fixed error message using a null resourcemanager With most recent CoreFx supporting STA, removed special CoreClr code paths Added test coverage Removed some dead commented out code that was never used

merged tests to one file, use runas to have relevant tests run as non-elevated

Get-CredSSP returns text so only run that test if current culture is en-US

Aditya Patwardhan (adityapatwardhan) · 2017-08-08T17:14:39Z

Tests are failing on Linux.

Mike Richmond (mirichmo) · 2017-08-09T16:40:11Z

All the CredSSP tests are now passing in AppVeyor and Travis.

Luis Toledo (ltamaster) · 2017-11-22T20:37:05Z

Hi All,
I was looking to use the WSManCredSSP functionalities.
has this PR enabled in a release?
I checked the v6.0.0-beta.6 because it mentions that it was added, but it didn't work for me.

Thanks
Luis

Steve Lee (SteveL-MSFT) · 2017-11-22T23:50:06Z

Luis Toledo (@ltamaster) CredSSP is only supported on Windows

Ilya (iSazonov) · 2017-11-23T04:00:05Z

Steve Lee (@SteveL-MSFT) It seems PowerShell docs haven't "Supported platforms: Windows, Linux, MacOS" in cmdlet's descriptions.
Also we could make the error message more clear: "The term Cmdlet is not supported on the platform or is not recognized ..."

Steve Lee (SteveL-MSFT) · 2017-11-23T21:20:12Z

Ilya (@iSazonov) can you point me to the docs that say it's supported on Linux/macOS? Don't see that line here

I think the error message is technically correct as that cmdlet doesn't exist on non-Windows. It's a generic error message if a cmdlet is not found, so implying it's not supported may cause confusion particularly for typos.

Ilya (iSazonov) · 2017-11-24T03:39:58Z

Steve Lee (@SteveL-MSFT) I agree that we can leave the generic message "as is" if we enhance our docs.

can you point me to the docs that say it's supported on Linux/macOS?

It is just my request to enhance our docs - add new section "Supported platforms".

* [Feature] Fixed error message using a null resourcemanager With most recent CoreFx supporting STA, removed special CoreClr code paths Added test coverage Removed some dead commented out code that was never used * [Feature] merged tests to one file, use runas to have relevant tests run as non-elevated * [Feature] Get-CredSSP returns text so only run that test if current culture is en-US * [Feature] Update test to fix code exclusion * [Feature] Fix Skipping on test cases that already had a Skip value

Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan) Jul 24, 2017

Steve Lee (SteveL-MSFT) requested a review from Mike Richmond (mirichmo) July 24, 2017 23:15

Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 24, 2017

Steve Lee (SteveL-MSFT) force-pushed the credssp branch from 74ca739 to b049a00 Compare July 24, 2017 23:15

Steve Lee (SteveL-MSFT) requested a review from Aditya Patwardhan (adityapatwardhan) July 24, 2017 23:33

Mike Richmond (mirichmo) suggested changes Jul 26, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) force-pushed the credssp branch from b049a00 to f8612e2 Compare July 26, 2017 03:32

Steve Lee (SteveL-MSFT) commented Jul 26, 2017

View reviewed changes

Comment thread src/Microsoft.WSMan.Management/CredSSP.cs Outdated

Steve Lee (SteveL-MSFT) Jul 26, 2017

Copy link
Copy Markdown

Member Author

Choose a reason for hiding this comment

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

this is redundant from line 486

Steve Lee (SteveL-MSFT) force-pushed the credssp branch 2 times, most recently from 1e9b8af to a06b389 Compare July 26, 2017 16:25

Aditya Patwardhan (adityapatwardhan) requested changes Jul 28, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) force-pushed the credssp branch 2 times, most recently from 1501d34 to c74be60 Compare August 1, 2017 00:48

Aditya Patwardhan (adityapatwardhan) approved these changes Aug 1, 2017

View reviewed changes

Mike Richmond (mirichmo) suggested changes Aug 7, 2017

View reviewed changes

Mike Richmond (mirichmo) force-pushed the credssp branch from 741fef2 to 0f93089 Compare August 7, 2017 22:56

Mike Richmond (mirichmo) approved these changes Aug 7, 2017

View reviewed changes

Steve Lee (SteveL-MSFT) and others added 4 commits August 8, 2017 09:23

[Feature]

a6b8347

Fixed error message using a null resourcemanager With most recent CoreFx supporting STA, removed special CoreClr code paths Added test coverage Removed some dead commented out code that was never used

[Feature]

e61c05a

merged tests to one file, use runas to have relevant tests run as non-elevated

[Feature]

efa769f

Get-CredSSP returns text so only run that test if current culture is en-US

[Feature] Update test to fix code exclusion

7d1c523

Mike Richmond (mirichmo) force-pushed the credssp branch from 0f93089 to 7d1c523 Compare August 8, 2017 16:23

[Feature] Fix Skipping on test cases that already had a Skip value

cf9ee3f

Aditya Patwardhan (adityapatwardhan) merged commit eea3885 into PowerShell:master Aug 10, 2017

Steve Lee (SteveL-MSFT) deleted the credssp branch August 14, 2017 06:25

Uh oh!

Conversation

Steve Lee (SteveL-MSFT) commented Jul 24, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Jul 25, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) Jul 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Jul 26, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 1, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Aug 4, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Aug 8, 2017

Uh oh!

Aditya Patwardhan (adityapatwardhan) commented Aug 8, 2017

Uh oh!

Mike Richmond (mirichmo) commented Aug 9, 2017

Uh oh!

Luis Toledo (ltamaster) commented Nov 22, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Nov 22, 2017

Uh oh!

Ilya (iSazonov) commented Nov 23, 2017

Uh oh!

Steve Lee (SteveL-MSFT) commented Nov 23, 2017

Uh oh!

Ilya (iSazonov) commented Nov 24, 2017

Uh oh!

Reviewers

Assignees

Steve Lee (SteveL-MSFT) Jul 26, 2017 •

edited

Loading

Steve Lee (SteveL-MSFT) Jul 26, 2017 •

edited

Loading