-
Notifications
You must be signed in to change notification settings - Fork 904
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.
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
(GH-2044) Fix for changing $env:Temp #2154
Conversation
@vexx32 any chance you can take a look at this one, thanks? |
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.
Overall looks good to me. A few things here and there I think we should clean up a bit before we merge. 🙂
Set-Item "Env:$($_)" -Value (Get-EnvironmentVariable -Scope $scope -Name $_) | ||
#ordering is important here, $user should override $machine... | ||
$Scopes = 'Process', 'Machine' | ||
if (($userName -ne 'SYSTEM') -and ($userName -ne "$($env:COMPUTERNAME)`$")) { |
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 might be clearer to write as:
if (($userName -ne 'SYSTEM') -and ($userName -ne "$($env:COMPUTERNAME)`$")) { | |
if ($userName -notin 'SYSTEM', "${env:COMPUTERNAME}`$") { | |
# but only if not running as the SYSTEM/machine in which case user can be ignored. | ||
$Scopes += 'User' | ||
} | ||
foreach ($Scope in $Scopes) { |
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 generally recommend avoiding plural variables be used alongside singular counterparts; it's very easy to mess things up with a single-character typo and not notice that the logic is totally broken until later. Maybe use $ScopeList
instead for the latter variable?
foreach ($Scope in $Scopes) { | ||
Get-EnvironmentVariableNames -Scope $Scope | | ||
ForEach-Object { | ||
Set-Item "Env:$($_)" -Value (Get-EnvironmentVariable -Scope $Scope -Name $_) |
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.
Unnecessary subexpression wrapper in this string
Set-Item "Env:$($_)" -Value (Get-EnvironmentVariable -Scope $Scope -Name $_) | |
Set-Item "Env:$_" -Value (Get-EnvironmentVariable -Scope $Scope -Name $_) |
I agree with all the comments by @vexx32 and I think I've made the adjustments so they roll into this PR. (I'm not very good with GitHub and how the change management flow works.) |
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.
Looks good to me, thanks for fixing that up! 💖
Running as `SYSTEM`, the `$env:Temp` variable is normally *C:\Windows\Temp*. If the **first** package to ever use `Update-SessionEnvironment.ps1` happens to be running as `SYSTEM`, the location of `$env:Temp` changes to *C:\Windows\system32\config\systemprofile\AppData\Local\Temp* for the duration of the session. This change to `$env:Temp` does not occur in subsequent calls to `Update-SessionEnvironment.ps1`. It is unknown why this change occurs at all and likewise is unknown why it occurs for only the first time. The `Update-SessionEnvironment.ps1` normally gives priority to user-scope environment variables over machine-scope environment variables. Changes in this commit eliminates the gathering of the `User` scope environment variables when running as the `SYSTEM` account on the belief that reason to be operating under the `SYSTEM` account is to do machine-wide activities, and there is no reason to see `SYSTEM` as a "user". On a clean system, the only User-scope environment variables under `SYSTEM` are *path*, *temp*, and *tmp* which are already defined under the Machine-scope, and in general, there shouldn't be any new User-scope variables for the SYSTEM account because it's not a "normal" account. Basically, there should be little reason to expect this change to break anything.
" is not required here, as we are not including any variables.
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.
LGTM!
@teknowledgist thank you very much for taking the time to submit this PR, we really appreciate it! |
(GH-2044) This fixes a problem when a package is installed under the System credentials (typically how centralized management software -- e.g. SCCM -- operate):
Running as
SYSTEM
, the$env:Temp
variable is normally C:\Windows\Temp. If the first package to ever useUpdate-SessionEnvironment.ps1
happens to be running asSYSTEM
, the location of$env:Temp
changes to C:\Windows\system32\config\systemprofile\AppData\Local\Temp for the duration of the session. This change to$env:Temp
does not occur in subsequent calls toUpdate-SessionEnvironment.ps1
. It is unknown why this change occurs at all and likewise is unknown why it occurs for only the first time.The
Update-SessionEnvironment.ps1
normally gives priority to user-scope environment variables over machine-scope environment variables. Changes in this commit eliminates the gathering of theUser
scope environment variables when running as theSYSTEM
account on the belief that reason to be operating under theSYSTEM
account is to do machine-wide activities, and there is no reason to seeSYSTEM
as a "user". On a clean system, the only User-scope environment variables underSYSTEM
are path, temp, and tmp which are already defined under the Machine-scope, and in general, there shouldn't be any new User-scope variables for the SYSTEM account because it's not a "normal" account. Basically, there should be little reason to expect this change to break anything.Fixes #2044