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.

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

Add Add-PoshGitToProfile -AllUsers support #504

Merged
merged 6 commits into from
Jan 1, 2018
Merged

Add Add-PoshGitToProfile -AllUsers support #504

merged 6 commits into from
Jan 1, 2018

Conversation

dahlbyk
Copy link
Owner

@dahlbyk dahlbyk commented Dec 8, 2017

Toward including an option to install posh-git with Git for Windows (git-for-windows/git#1384), this adds an option to add to profile for all users.

Note that, as expected, -AllUsers from a non-elevated shell fails:

Add-Content : Access to the path
'C:\Windows\System32\WindowsPowerShell\v1.0\Microsoft.PowerShell_profile.ps1' is denied.

or

Add-Content : Access to the path
'C:\Windows\System32\WindowsPowerShell\v1.0\profile.ps1' is denied.

Running PowerShell as Administrator allows the global install.

Copy link
Collaborator

@rkeithhill rkeithhill left a comment

Choose a reason for hiding this comment

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

LGTM

@rkeithhill
Copy link
Collaborator

BTW we do have this code to determine if the process is elevated or not.

posh-git/src/GitPrompt.ps1

Lines 115 to 124 in 65658bf

# PowerShell 5.x only runs on Windows so use .NET types to determine isAdminProcess
# Or if we are on v6 or higher, check the $IsWindows pre-defined variable.
if (($PSVersionTable.PSVersion.Major -le 5) -or $IsWindows) {
$currentUser = [Security.Principal.WindowsPrincipal]([Security.Principal.WindowsIdentity]::GetCurrent())
$isAdminProcess = $currentUser.IsInRole([Security.Principal.WindowsBuiltInRole]::Administrator)
}
else {
# Must be Linux or OSX, so use the id util. Root has userid of 0.
$isAdminProcess = 0 -eq (id -u)
}

We could check if the process is not elevated and the user specified -AllHosts and give a better error message. One that tells them they need to be running as admin in order to update AllHosts profiles.

@dahlbyk
Copy link
Owner Author

dahlbyk commented Dec 27, 2017

We could check if the process is not elevated and the user specified -AllHosts and give a better error message.

Done.

Installing posh-git for all users requires an elevated host.

src/Utils.ps1 Outdated
@@ -104,9 +125,18 @@ function Add-PoshGitToProfile {
$TestParams
)

if ($AllUsers -and !(Test-Administrator)) {
throw 'Installing posh-git for all users requires an elevated host.'
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about rewording that as 'Adding posh-git to an AllUsers profile requires an elevated host.'?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Other than that, this PR looks good to me.

@dahlbyk dahlbyk merged commit bf0ec4a into master Jan 1, 2018
@dahlbyk dahlbyk deleted the AllUsers branch January 1, 2018 20:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants