GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Enable WSManCredSSP cmdlets by SteveL-MSFT · Pull Request #4336 · PowerShell/PowerShell

Conversation

@SteveL-MSFT

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

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT Steve Lee (SteveL-MSFT) assigned Aditya Patwardhan (adityapatwardhan) Jul 24, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Mike Richmond (mirichmo) July 24, 2017 23:15
@msftclas Microsoft Contribution License Agreements (msftclas) added the cla-not-required label Jul 24, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the credssp branch from 74ca739 to b049a00 Compare July 24, 2017 23:15
@SteveL-MSFT
Steve Lee (SteveL-MSFT) requested a review from Aditya Patwardhan (adityapatwardhan) July 24, 2017 23:33
@SteveL-MSFT

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

Copy link
Copy Markdown
Member Author

AppVeyor failure waiting on #4323
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mirichmo
Mike Richmond (mirichmo) suggested changes Jul 26, 2017
View reviewed changes
Comment thread src/Microsoft.WSMan.Management/CredSSP.cs Outdated

@mirichmo Mike Richmond (mirichmo) Jul 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why were these two checks removed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

line 486 and 890 already does the check

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.WSMan.Management/WsManHelper.cs Outdated

@mirichmo Mike Richmond (mirichmo) Jul 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can do

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread src/Microsoft.WSMan.Management/CredSSP.cs Outdated

@mirichmo Mike Richmond (mirichmo) Jul 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Why were these two checks removed?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

check happens twice

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP-nonAdmin.Tests.ps1 Outdated

@mirichmo Mike Richmond (mirichmo) Jul 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

  1. Does this line work on non-Windows platforms?
  2. Code should not be outside of a Pester block. This should be moved within a BeforeAll
  3. Check out the guidelines on Admin for Windows
  4. Consider using the pattern for skipping tests as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

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

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

  1. will wrap in $IsWindows
  2. will wrap in BeforeAll
  3. this is the opposite of requiring admin, this test requires NOT admin (there isn't a RequireNotAdminOnWindows)
  4. two different test cases here where the first is only applicable when NOT admin, the second one works either way

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1 Outdated

@mirichmo Mike Richmond (mirichmo) Jul 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1 Outdated

@mirichmo Mike Richmond (mirichmo) Jul 26, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The same comments apply to this file as well

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the credssp branch from b049a00 to f8612e2 Compare July 26, 2017 03:32
SteveL-MSFT
Steve Lee (SteveL-MSFT) commented Jul 26, 2017
View reviewed changes
Comment thread src/Microsoft.WSMan.Management/CredSSP.cs Outdated

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

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the credssp branch 2 times, most recently from 1e9b8af to a06b389 Compare July 26, 2017 16:25
@SteveL-MSFT

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

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) requested changes Jul 28, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1 Outdated

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Please check for en-US before comparing.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1 Outdated

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1 Outdated

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same as above.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1 Outdated

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) Jul 28, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Same as above

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@SteveL-MSFT Steve Lee (SteveL-MSFT) Jul 28, 2017

Copy link
Copy Markdown
Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

will fix

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@SteveL-MSFT
Steve Lee (SteveL-MSFT) force-pushed the credssp branch 2 times, most recently from 1501d34 to c74be60 Compare August 1, 2017 00:48
@SteveL-MSFT

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

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
adityapatwardhan
Aditya Patwardhan (adityapatwardhan) approved these changes Aug 1, 2017
View reviewed changes
@SteveL-MSFT

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

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
mirichmo
Mike Richmond (mirichmo) suggested changes Aug 7, 2017
View reviewed changes
Comment thread test/powershell/Modules/Microsoft.WSMan.Management/CredSSP.Tests.ps1 Outdated

@mirichmo Mike Richmond (mirichmo) Aug 7, 2017

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

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.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@mirichmo
Mike Richmond (mirichmo) force-pushed the credssp branch from 741fef2 to 0f93089 Compare August 7, 2017 22:56
mirichmo
Mike Richmond (mirichmo) approved these changes Aug 7, 2017
View reviewed changes
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Aug 8, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Steve Lee (SteveL-MSFT) and others added 4 commits August 8, 2017 09:23
@SteveL-MSFT @mirichmo
[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
@SteveL-MSFT @mirichmo
[Feature]
e61c05a 
merged tests to one file, use runas to have relevant tests run as non-elevated
@SteveL-MSFT @mirichmo
[Feature]
efa769f 
Get-CredSSP returns text so only run that test if current culture is en-US
@mirichmo
[Feature] Update test to fix code exclusion
7d1c523
@mirichmo
Mike Richmond (mirichmo) force-pushed the credssp branch from 0f93089 to 7d1c523 Compare August 8, 2017 16:23
@adityapatwardhan

Aditya Patwardhan (adityapatwardhan) commented Aug 8, 2017

Copy link
Copy Markdown
Member

Tests are failing on Linux.
image
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@mirichmo
[Feature] Fix Skipping on test cases that already had a Skip value
cf9ee3f
@mirichmo

Mike Richmond (mirichmo) commented Aug 9, 2017

Copy link
Copy Markdown
Member

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@adityapatwardhan
Aditya Patwardhan (adityapatwardhan) merged commit eea3885 into PowerShell:master Aug 10, 2017
@SteveL-MSFT
Steve Lee (SteveL-MSFT) deleted the credssp branch August 14, 2017 06:25
@ltamaster

Luis Toledo (ltamaster) commented Nov 22, 2017

Copy link
Copy Markdown

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.

screenshot 2017-11-22 17 16 30

Thanks
Luis
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

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

Copy link
Copy Markdown
Member Author

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 23, 2017

Copy link
Copy Markdown
Collaborator

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 ..."
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@SteveL-MSFT

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

Copy link
Copy Markdown
Member Author

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@iSazonov

Ilya (iSazonov) commented Nov 24, 2017

Copy link
Copy Markdown
Collaborator

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".
SteveL-MSFT reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Thatgfsj (Thatgfsj) pushed a commit to Thatgfsj-contribs/PowerShell that referenced this pull request Aug 6, 2026
@SteveL-MSFT @adityapatwardhan
Enable WSManCredSSP cmdlets (PowerShell#4336)
1f30a88 
* [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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@adityapatwardhan Aditya Patwardhan (adityapatwardhan) adityapatwardhan approved these changes
+1 more reviewer
@mirichmo Mike Richmond (mirichmo) mirichmo approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@adityapatwardhan Aditya Patwardhan (adityapatwardhan)

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@SteveL-MSFT @adityapatwardhan @mirichmo @ltamaster @iSazonov @msftclas
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.