PowerShell transcripts should include the configuration name in the transcript header#2890
PowerShell transcripts should include the configuration name in the transcript header#2890TravisEz13 merged 2 commits intoPowerShell:masterfrom chunqingchen:bugfix0
Conversation
There was a problem hiding this comment.
This may be more efficient for extracting the configuration name from the Shell Uri since it only allocates one string instead of multiple string objects:
string shellPrefix = System.Management.Automation.Remoting.Client.WSManNativeApi.ResourceURIPrefix;
int index = shelluri.IndexOf(shellPrefix, StringComparison.OrdinalIgnoreCase);
return (index == 0) ? shelluri.Substring(shellPrefix.Length) : string.Empty;
``` #Resolved
There was a problem hiding this comment.
It would be better to parse the configuration name from the shellUri (configurationProcviderId) here rather than parse it each time later when a transcript header is written. #Resolved
There was a problem hiding this comment.
You should prefer auto-properties where possible. #Resolved
|
Thanks @lzybkr, @PaulHigin ! I have incorporated the suggested change. #Resolved |
There was a problem hiding this comment.
psConfigurationName [](start = 20, length = 19)
I believe that this cannot be null. Please make it default to empty string.
|
@PaulHigin the request has been addressed. |
|
@chunqingchen Please include tests in this PR |
|
@chunqingchen You need to add yourself to Microsoft on GitHub |
|
@chunqingchen I suspect you're already in the orgs, you just need to do this (for both PowerShell and Microsoft, though doing Microsoft is what will fix the CLA bot): https://help.github.com/articles/publicizing-or-hiding-organization-membership/ |
|
There are open comments that still need to be addressed. See: #2890 (comment) |
|
We have been waiting on author response to PR comments for over a week.
|
|
working on the test now |
|
@chunqingchen Please don't dismiss (delete) other people reviews. If you believe someone review is bad enough to be dismissed, please talk to a maintainer. |
|
@TravisEz13 I meant the reviews are resolved. So dismiss is not the way to say it has been resolved. Got it. |
|
@PaulHigin Hi Paul, I talked with Travis and he agrees to merge the commit if you are fine. do you have any more comment about this change? |
There was a problem hiding this comment.
Please use more informative title.
Sample - "Transcript header".
There was a problem hiding this comment.
Yes, please replace bug fix test with something more descriptive.
There was a problem hiding this comment.
This is recommended, but not required.
There was a problem hiding this comment.
Please replace -force with -Force.
There was a problem hiding this comment.
Please replace aliases with full cmdlet names.
There was a problem hiding this comment.
Please use Should BeLike or Should BeLikeExactly
There was a problem hiding this comment.
Please replace -force with -Force.
|
I agree with @iSazonov comments. Otherwise LGTM. |
I agree with @iSazonov comments. Otherwise LGTM.
TravisEz13
left a comment
There was a problem hiding this comment.
I agree with @iSazonov comments. Otherwise LGTM.
…uration name in the transcript header #2890
|
@iSazonov @TravisEz13 comments are all resolved |
|
Thanks! |
|
Great change! |
Steps to reproduce
Expected behavior
PowerShell transcripts currently do not log the configuration name the user used to connect to and manage the machine. For JEA scenarios, this means an auditor trying to understand how someone was able to do a certain command will not know through which endpoint the user entered and was assigned those privileges.
Suggestion is to add a new line to the transcript header similar to the following:
ConfigurationName: MyJEA
[…]
RunAs User: WinRM Virtual Users\WinRM VA_10_PRIV_priv.demo
Configuration Name: JEA
Machine: DC (Microsoft Windows NT 10.0.14393.0)
[…]
Actual behavior
[…]
RunAs User: WinRM Virtual Users\WinRM VA_10_PRIV_priv.demo
Machine: DC (Microsoft Windows NT 10.0.14393.0)
[…]
Environment data