Change default scope in Install-Module and Install-Script #315

alerickson · 2018-08-22T21:43:42Z

No description provided.

edyoung

Thanks!

edyoung · 2018-08-22T21:52:57Z

PowerShellGet/public/psgetfunctions/Install-Module.ps1

+        if (-not ($Scope)) 
        {
-            # Throw an error when Install-Module is used as a non-admin user and '-Scope CurrentUser' is not specified
-            $message = $LocalizedData.InstallModuleNeedsCurrentUserScopeParameterForNonAdminUser -f @($script:programFilesModulesPath, $script:MyDocumentsModulesPath)


Consider: when the user is not an admin and they explicitly specify -Scope AllUsers shouldn't they see this message?

They still see the error message because this function calls PackageManagement\Install-Module, which has the same error message. I don't think it's necessary to keep it in there unless there's a specific situation where it's advantageous to throw the error early on.

edyoung · 2018-08-22T22:19:40Z

Tests/PowerShellGet.Tests.ps1

    }

-    It "Should install a module correctly to the required location with CurrentUser scope" {
+    It "Should install a module correctly to the required location with CurrentUser scope and no elevated privileges" {


do these tests always run without elevated privileges, or just with the default privileges of how you start the shell? ie if I run these tests in an Administrator window won't they fail?

The tests will run based off of the default privileges of the shell. The previous install-module/install-script tests were already split up into "Module tests" and "Module tests (Admin)" (the latter is tagged with "RequireAdminOnWindows"). Running without privileges causes all the non-elevated privilege tests to pass and vice versa.

edyoung · 2018-08-23T02:59:54Z

Lgtm

bmanikm · 2018-08-23T20:28:29Z

PowerShellGet/public/psgetfunctions/Install-Module.ps1

-                        -CallerPSCmdlet $PSCmdlet `
-                        -ErrorCategory InvalidArgument
+            $Scope = "CurrentUser"
+            if(Test-RunningAsElevated)


if(Test-RunningAsElevated) [](start = 12, length = 26)

On non-Windows OS platforms, this function returns true as there is no straight forward way to identify whether PS is running as admin. I think, the default scope the non-Windows platforms should be CurrentUser.

bmanikm · 2018-08-23T20:32:50Z

Tests/PowerShellGet.Tests.ps1

+
+        $installedModuleInfo | Should Not Be $null
+        $installedModuleInfo.Name | Should Be $ContosoServer
+        $installedModuleInfo.InstalledLocation.StartsWith($script:MyDocumentsModulesPath, [System.StringComparison]::OrdinalIgnoreCase) | Should Be $true


This tests in this file gets executed only in the daily test pass and it is sanity tests file for PS core builds.
In PowerShellGet daily test runs, all the tests in this file are executed with admin privileges.

@bmanikm So what should I do to ensure both admin and non-admin tests run? Should I only include admin tests?

Please take a look at https://github.com/PowerShell/PowerShellGet/blob/development/Tests/PSGetInstallModule.Tests.ps1#L522 for running the tests as non-admin.

bmanikm · 2018-08-23T20:34:16Z

PowerShellGet/public/psgetfunctions/Install-Module.ps1

-                        -ErrorId "InstallModuleNeedsCurrentUserScopeParameterForNonAdminUser" `
-                        -CallerPSCmdlet $PSCmdlet `
-                        -ErrorCategory InvalidArgument
+            $Scope = "CurrentUser"


$Scope = "CurrentUser" [](start = 12, length = 22)

It is also required to change the default scope value in the Install-Package provider function as users can use the PackageManagement Install-Package cmdlet with PowerShellGet provider name.

I think that whilst yes you could do that, the majority would not drop down to use Install-Package instead of Install-Module unless really needed, which I can't think of a reason off the top of my head as to why anyone would

bmanikm

🕐

alerickson · 2018-09-10T18:08:50Z

@bmanikm can you review recent changes? Core tests are temporarily skipped until dotnet in appveyor environment is updated.

bmanikm · 2018-09-10T18:36:43Z

PowerShellGet/private/functions/Install-PackageUtility.ps1

    $Scope = $null
    $NoPathUpdate = $false
    $AcceptLicense = $false
+    $script:IsWindowsOS = (-not (Get-Variable -Name IsWindows -ErrorAction Ignore)) -or $IsWindows


$script:IsWindowsOS = (-not (Get-Variable -Name IsWindows -ErrorAction Ignore)) -or $IsWindows [](start = 4, length = 94)

Any specific reason for not using $script:IsWindows defined in .\PowerShellGet\private\modulefile\PartOne.ps1 ?

I made that change, please let me know if there's other changes I should make before pushing.

bmanikm · 2018-09-10T19:28:54Z

Tests/PSGetInstallModule.Tests.ps1

+        Start-Process $PSprocess -ArgumentList '$null = Install-PackageProvider -Name NuGet -MinimumVersion 2.8.5.201 -Force -Scope CurrentUser;
+                                                              $null = Import-PackageProvider -Name NuGet -MinimumVersion 2.8.5.201 -Force;
+                                                              if(-not (Get-PSRepository -Name INTGallery -ErrorAction SilentlyContinue)) {
+                                                                Register-PSRepository -Name INTGallery -SourceLocation https://dtlgalleryint.cloudapp.net/api/v2/ -InstallationPolicy Trusted


https://dtlgalleryint.cloudapp.net/api/v2/ [](start = 119, length = 42)

Please use www.poshtestgallery.com here

…dule

alerickson · 2018-09-12T01:12:12Z

@JKeithB can you review the changelog? I included scope changes made to "install-package", but it's a provider function so I'm not sure how that should be treated.

ghost · 2018-09-12T01:29:40Z

CHANGELOG.md

+## 2.0.0
+Breaking Change
+- For both administrative privileges and non-administrative privileges, default user scope for Install-Module, Install-Script, and Install-Package is now current user when running PowerShell version 6.0.0 and up.  For versions less than 6.0.0 administrative privileges will default to all users and non-administrative privileges will default to current user.
+


I would separate these differently. Suggested rewording accounts for the fact that most users are on PowerShell 5.1:
Default user scope for Install-Module, Install-Script, and Install-Package has changed. For Windows PowerShell (version 5.1), the default user scope is AllUsers when running in an elevated session, and CurrentUser at all other times.
For PowerShell version 6.0.0 and above, the default user scope is always CurrentUser.

bmanikm · 2018-09-12T17:29:25Z

PowerShellGet/public/psgetfunctions/Install-Script.ps1

-            $AdminPrivilegeErrorMessage = $LocalizedData.InstallScriptNeedsCurrentUserScopeParameterForNonAdminUser -f @($script:ProgramFilesScriptsPath, $script:MyDocumentsScriptsPath)
-            $AdminPrivilegeErrorId = 'InstallScriptNeedsCurrentUserScopeParameterForNonAdminUser'
+            # Throw an error when Install-Script is used as a non-admin user and '-Scope AllUsers'
+            $message = $LocalizedData.InstallScriptAdminPrivilegeRequiredForAllUsersScope -f @($script:programFilesModulesPath, $script:MyDocumentsModulesPath)


@($script:programFilesModulesPath, $script:MyDocumentsModulesPath) [](start = 93, length = 66)

These vars are for modules, please use @($script:ProgramFilesScriptsPath, $script:MyDocumentsScriptsPath) here. #Closed

alerickson · 2018-09-13T00:06:25Z

@bmanikm the tests are no longer hanging. I believe everything should be good to go now.

alerickson added 2 commits August 22, 2018 14:42

Change default scope in Install-Module and Install-Script

ed3ac13

Delete whitespace

3191108

edyoung reviewed Aug 22, 2018

View reviewed changes

edyoung mentioned this pull request Aug 23, 2018

Install-Module should default to CurrentUser #236

Closed

bmanikm reviewed Aug 23, 2018

View reviewed changes

bmanikm suggested changes Aug 23, 2018

View reviewed changes

alerickson added 21 commits August 24, 2018 13:39

Change default scope for non-Windows to CurrentUser

d891b92

Add tests to install module/script test files, delete previous tests

913d71c

Update tests

d58b423

Added scope changes to Install-PackageUtility, modified tests

11322c4

Add scope condition for non-windows, update tests

92e8003

Add unix credentials

c9e9f15

Travis test

47d00e1

Add print statements to tests for location installation verification

0224557

Add print statements

976a2e6

Add cleanup for unix user

2797552

Change installed location assertions in tests

ac42abe

Add tests for Install-Package user scope

ba01168

Update current user scope for non-admin tests

a8a9014

Update tests for linux

5e7c1e8

Update tests

33139df

Update tests for windows

351ff97

Change for windows test

c06ebb6

Change scope to current user for install-packageprovider in tests

b780be3

Reset tests to older version

56bd61c

Change Install-PackageProvider scope to CurrentUser

45c1aeb

root*

a12d347

alerickson added 8 commits September 8, 2018 16:51

Add print statement-- tests passing locally failing on AppVeyor

92d711e

Update print statements

5821a60

Update print statements for test

854fd34

Test appveyor

cce47b8

Update allusers test for install script

851e116

Added ErrorVariable to tests

ac8a3a7

Change Windows variable for tests

2fa6983

Comment out linux code, rerun tests

c6e26af

bmanikm reviewed Sep 10, 2018

View reviewed changes

alerickson added 6 commits September 10, 2018 18:00

InstallModuleNeedsCurrentUserScopeParameterForNonAdminUser

8249794

Incorporate changes from code review

58359fe

Add pwsh.exe if console is PSCore

a01e9cc

Incorporate changes from code review

89d1e15

Merge remote-tracking branch 'upstream/master' into scopeForInstallMo…

7bb9ad0

…dule

Update changelog

65e63bf

ghost reviewed Sep 12, 2018

View reviewed changes

bmanikm reviewed Sep 12, 2018

View reviewed changes

alerickson added 5 commits September 12, 2018 11:03

Update changelog, minor changes to tests and install-script

67b1de7

Enable AppVeyor RDP for current branch

46920d1

Add Skip for Core tests, remove RDP for branch

f525eca

Change 'is windows' check to 'is not core' in install-packageutility

3a09f5d

Add -NoPathUpdate to Install-Package cmd in Publish-Script test

702b5e2

bmanikm approved these changes Sep 13, 2018

View reviewed changes

bmanikm merged commit f0728e0 into PowerShell:development Sep 13, 2018

alerickson deleted the scopeForInstallModule branch September 13, 2018 17:21

Change default scope in Install-Module and Install-Script #315

Change default scope in Install-Module and Install-Script #315

Uh oh!

Conversation

alerickson commented Aug 22, 2018

Uh oh!

edyoung left a comment

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!

edyoung commented Aug 23, 2018

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!

bmanikm left a comment

Choose a reason for hiding this comment

Uh oh!

alerickson commented Sep 10, 2018

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!

alerickson commented Sep 12, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmanikm Sep 12, 2018 • 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!

alerickson commented Sep 13, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bmanikm Sep 12, 2018 •

edited

Loading