PowerShell / PowerShellGetv2

PowerShellGet is the Package Manager for PowerShell
https://www.PowerShellGallery.com
MIT License
431 stars 138 forks source link

Install-Module should default to CurrentUser #236

Closed indented-automation closed 6 years ago

indented-automation commented 6 years ago

I add a default parameter for Install-Module to set Scope to CurrentUser wherever I use the command. I explicitly set the scope to CurrentUser when I use it in build scripts (CI tooling).

I believe the default install location should be CurrentUser. A system level change should only occur when explicitly requested.

Expected Behavior

Modules installed using Install-Module should be placed in CurrentUser scope unless I explicitly request AllUsers scope.

Current Behavior

Modules installed using Install-Module attempt to install into AllUsers scope, I must explicitly request the less broad scope.

Possible Solution

The current default for the Scope parameter should be changed to CurrentUser:

https://github.com/PowerShell/PowerShellGet/blob/development/PowerShellGet/PSModule.psm1#L1962

markekraus commented 6 years ago

I have this set as a default parameter in my profiles too. I don't have a single line of code anywhere that depends on the ability to install system level modules. Aside from the core PowerShell modules and those provided by Windows/RSAT, all of my automation and code rely on CurrentUser scoped modules. The only time use AllUsers is when I run Install-Module on a system without my profile settings and forget that CurrentUser is not the default. In other words, I only use this scope on accident and never intentionally.

I cannot fathom a reason to have AllUsers as the default scope. Especially since this requires elevation.

nonameneeded commented 6 years ago

This would be awesome!

bergmeister commented 6 years ago

This would not be awesome because I am sure many people forget to specify to use AllUsers scope when installing stuff on CI machines. Because most CI user accounts are non-interactive, it is common practice to remote to the machine and do the installation using an admin account.

markekraus commented 6 years ago

@bergmeister I'm not sure I follow that logic. In every CI scenario I have seen, CurrentUser is having to be set explicitly and that there is no assumption that the required modules will be available in the CI environment. Are you saying that it's more common to have the CI environment preloaded with modules at the system level?

It's pretty clear this would be a breaking change. It's just one I think is worth making.

bergmeister commented 6 years ago

@markekraus The CI systems that I work with are often heavily locked down and managed by our DevOps department (they want to ensure that the build servers only build and nothing else) but we also have complex system tests where around 10 different accounts are being used to test various integration scenarios.

tommymaynard commented 6 years ago

If we think of PowerShell modules as applications, and I think in some respects we can, we begin to appreciate the fact that they're out to benefit the greatest number of people they can on a single system. I wouldn’t be surprised if this was the reason for the AllUsers scope default, and why many application installers default to "C:\Program Files\" or "C:\Program Files (x86)\," vs. "C:\Users\username\AppData\." I’m looking at you, virus writers, as they understand the value of defaulting to a location where the user most certainly has permissions to write. Maybe installing modules should be an administrative task.

Maybe I can be swayed, but I lean toward to leaving it as it is. Initially, I have to wonder if it may be too big a breaking change, and that others -- that expect it work the way it does now -- would only end up trading places.

Probably not possible, but knowing how the Install-Module command was executed more often (AllUsers vs. CurrentUser), when it is run against the PowerShell Gallery, would be some great information to see. Sure, I get the command has a default, so it may not be a fair test. Maybe the parameter should be mandatory, and not have a default value. Or maybe, we should stop trusting the defaults. I write all my non-single use commands, with all the parameter and parameter values I want included, whether or not, they’re default. You never know when someone’s going to come along and try to change the defaults. ;) I don’t know the next person to read my code, but maybe they won’t consider the default parameters and parameter values. A complete command means that it’s not a requirement they do.

Now, maybe what I would get behind more, is a fallback: AllUsers is default; however, if Install-Module fails to write to "C:\Program Files\WindowsPowerShell\Modules\," for instance, it should default to the CurrentUser location. I do this sometimes in my coding. You could check the permissions at the Save location, or do like I’ve done a time or two (and be lazy), and write a temporary file there. If it works, delete the temporary file and proceed. If it doesn’t, alert the user (potentially), and save to the CurrentUser save location.

Just some thoughts.

markekraus commented 6 years ago

I write all my non-single use commands, with all the parameter and parameter values

Right and that's not the issue. The issue is when you are doing this as a user for your own uses. If a user is on a system where they are not the administrator, the default action is confusing. Nearly all of the documentation for installing a module with Install-Module without the -Scope present. This prompts a very frequent question on forums, slack, and github repos "How can I install modules form the gallery without admin rights?" IMO, this design decision breaks the principle of least astonishment.

I don't think of modules as "applications" and in fact, at the last community call it was made clear that they shouldn't be used as such and that the PSGallery was not to be used for application deployment. I think of them as extensions to MY code, not the system's code, but MY code (where "MY" may not literally be me, but the task runner, automation job, microservice, etc). Since it's extending MY code, it should be in MY environment by default. if it's in my profile, that can (potentially) migrate with me. If it's in the system, I have to reinstall it on every system I run MY code.

vexx32 commented 6 years ago

Perhaps a simpler change is required, then.

Instead of an outright error, warn users that do not have administrative rights that they can install a module just for themselves using -Scope CurrentUser if they wish, otherwise administrative rights are required.

markekraus commented 6 years ago

The problem still remains that I do not think the default behavior that most desire is to install modules at the system level. I'm still on board for making that clear via warning or error. But when I have -Scope CurrentUser in over 30 automation scripts and in $PSDefaultParameterValues['Install-Module:Scope'] = 'CurrentUser' my profiles on 7 different machines, and have never once wanted to install a module at the system level.... I have to question the default behavior.

bergmeister commented 6 years ago

Wouldn't it be cleaner to have 2 dedicated cmdlets: Install-ModuleForCurrentUser and Install-ModuleForAllUsers. It looks ugly at first but it is right in the face of the user so that they know what they are about to do. A cmdlet should, like a GUI, be only a very thin layer/wrapper to its underlying methods anyway.

markekraus commented 6 years ago

I don't think having 2 cmdlets that do the same thing is cleaner. I think keeping the same cmdlet and changing the default behavior is cleaner, even though it is a breaking change. IMO, this is bad design from the start and we shouldn't have to suffer bad design just because correcting it is a breaking change.

tommymaynard commented 6 years ago

Both of your paragraphs were convincing @markekraus, almost as if they should've been included on the first pass. Since you mentioned it, I do recall being one of those users. I also had a pause at being unable to download and install a module from the PSGallery with the default command. Ugh... so now I'm little more torn on whether or not the default should be changed -- good arguments. So, maybe we've long needed a C:\Users\Public\Documents\WindowsPowerShell\Modules\ directory.

bergmeister commented 6 years ago

What about having to specify a mandatory parameter with the type of installation then. Defaults are opinionated and can often be bad (although I usually advocate that stuff should work out of the box without having to specify loads of parameters)

markekraus commented 6 years ago

@bergmeister I'm ok with a mandatory parameter... but.. then there will be a massive amount of documentation and examples that will no longer work... not that I think that is a reason to avoid doing this, but if one of my motivations is to decrease new user questions related to Install-Module, then a mandatory parameter change might have the opposite affect as a result of ingrained documentation practices.

bergmeister commented 6 years ago

What if it used currentuser when not being elevated but only prompts to specify the type when being elevated?

fMichaleczek commented 6 years ago

install-module should default to CurrentUser. I only found one situation to AllUsers : DSC

markekraus commented 6 years ago

@bergmeister That might not work well. RunAs and remoting puts the session in elevated mode when the session user is an admin. Even as a my admin user, I don't want the default as AllUsers in 99% of cases. for the 1% that do, I would rather have to specify I want it to install as AllUsers. The split behavior might be surprising.

markekraus commented 6 years ago

another note on the split behavior: I haven't run into this in awhile so maybe it has been fixed... but when you install a module as an admin then as a user try to update the module to a newer version, you get the same kind of issue of being unable to. Then there is some kind of weird behavior where installing it i the user scope and trying to update it later it errors. Last time I ran into this I had to get on my admin account, remove the module, and then i could finally update the module in the user scope.... this was an unintended consequence as I forgot my admin use profile didn't have the $PSDefaultParameter set so i had expected the module to install in the admin user's scope and not system.. My own fault... but it mimics what might happen in a split-behavior scenario and is exacerbated by other issues with the cmdltes in this module.

ghost commented 6 years ago

@bergmeister for your example regarding build servers doing various integration testing. In my environment we have accounts with access to a specific environment of that application (dev/qa/etc...) As we roll out new versions of our modules (typically to follow application changes) they follow that same path of install into each environment. So, for me, installing into the AllUsers scope isn't an option.

Jaykul commented 6 years ago

It makes sense to default module installs to current user -- but I have conflicted feelings about changing this now, where it's already been in use for years.

You will cause confusion, but it's probably worth it.

I think you'd want to warn the users each time Install-Module ran (to remind them module was installed for current user, not at the machine scope as before). You might have some way of disabling the warning for the user (i.e. configuration) after they've seen it a few times.

In particular, you might want to warn --even if we didn't change the default-- when DSC resources are installed to current user locations 🙁

I don't think you'd want it to magically change to AllUsers just because they're elevated -- that way leads to even more confusion.

ghost commented 6 years ago

I could get onboard with Install-Module using write-output, for each module installed, where it was installed. Which I think is part of the verbose output now. This could also be beneficial to update-Module.

ghost commented 6 years ago

We discussed the defaults for this at length during design, a bit over 3 years ago. The default to be AllUsers scope was based on what, at that time, we understood to be the most common scenario: installing an item onto a server, from some PSRepository. In that situation, you want all processes to be able to immediately leverage the installed code - hence the decision to go with AllUsers. What has changed most is the advent of CI/CD pipelines.
As for changing the default when not logged in as admin, that is the kind of hidden behavior that is most likely to result in confusing errors. Imagine the telephone call that starts with: When this module was installed, were you logged in as administrator or a single user? Nobody is going to know, and no-one wants to answer that question when troubleshooting. Regarding taking a breaking change - this behavior is clearly inconvenient for a group of users. It sounds like it was confusing to troubleshoot. Sorry to hear that, and if we can reduce the amount of confusion, we should. However, the need to add "-Scope CurrentUser" does not make the bar for instituting a breaking change for a huge number of users.

vexx32 commented 6 years ago

I'm still thinking perhaps a middle ground is nice... Just confirm either way, with a clear indication of the action being taken. For example, in administrative mode, have a confirmation prompt with some text along the lines of "This will install the module for all users on this machine. Confirm Y/N/All."

And then in regular user mode, instead of outright spitting out an error when it fails, simply issue warning text instead, stating "Unable to install to AllUsers scope." and then a confirmation as to whether or not to install it to the CurrentUser scope.

This has both the clarity of always knowing the action being taken, and the convenience of being able to install to just the current user when not running as administrator. Naturally, for people who hate confirmations, the -Confirm:$False option should remain available, and should in any standard implementation I can think of happening here.

markekraus commented 6 years ago

this behavior is clearly inconvenient for a group of users.

Lets see how big that group is before saying it's not large enough to justify the breaking change. Also, lets get some data on the impact. Since this does require admin privileges and since use cases outlined here for lend themselves fo being in situations where parameters are specific regardless of defaults, the breaking change may only affect a limited sub set of interactive admin users who may not even realize or care this behavior makes a system wide change.

SwarfegaGit commented 6 years ago

I'm somewhat mixed on this. I agree it should be the default but I think it's been around too long now that this would be quite a large breaking change.

Looking back at Export-CSV the NoTypeInformation breaking change, this is similar but I feel that more people specify this parameter than they do the scope parameter for this cmdlet.

markekraus commented 6 years ago

@SwarfegaGit I disagree.

Hereinafter, "users" may refer to people or processes.

I believe that in most instances where users intend to install to the AllUsers scope, users are doing so with explicit parameters on Install-Module (i.e. -Scope AllUsers would be present in those situations). Those instances and use cases lend themselves to practices where "explicit is better than implicit". I believe users who rely on modules being installed in the AllUsers scope and also rely on the default scope setting to be a small minority.

I believe that most users who are running elevated sessions and running Install-Module with the default scope are actually indifferent about the location of module. It just happens to install to the AllUsers scope because that is the default behavior. Their scripts and automation likely don't hinge on the location of the module because they likely operate under the "always admin" model.

I believe that most users who are using Install-Module are doing so in unevaluated sessions. I believe a portion of those users are operating under the "never admin" model. I believe some users are using -Scope CurrentUser because they have read or asked about the error in one of the many PowerShell support areas. I believe many users are rerunning under an elevated session because that's what the error message tells them they need to do. I believe that in those instances where the elevated session is used, the users ae actually indifferent to the location of the module and are only following instructions or are unaware of the -Scope parameter.

I also believe that the majority of PowerShell environments where Install-Module is used with any kind of regularity are not shared user environments where multiple users would either want to or be able to benefit from AllUsers scoped modules. Instead, I believe the majority to be single user environments where CurrentUser scoped modules would be as effective as AllUser modules and significantly easier to maintain the module lifecycle as CurrentUser scoped.

With all that in mind, I believe that only a very small number of users would be broken by this change. I believe that if the change were accepted at a major version increase, since significant effort would be required to actually upgrade anyway, that properly communicating this breaking change would result in negligible impact if any. The greater impact to the user experience and overall security landscape of not inviting users to elevate their sessions needlessly would be a marked improvement.

fMichaleczek commented 6 years ago

@markekraus thank you for all yours explanations. I'm totally agree with you.

SwarfegaGit commented 6 years ago

@markekraus that's a pretty good argument there. So much so you convinced me this is a breaking change worth making.

Also, can I add I count 11 "I believe"s in your post? :)

KevinMarquette commented 6 years ago

I agree with making this change. From the new user's point of view, it should just work without elevation.

The people that knows that they need it system wide are more than likely system administrators that should have an easier time understanding these nuances.

I also have CurrentUser littered throughout scripts and as a default in my profile.

I keep having to coach my coworkers to use CurrentUser. We use a lot of Powershell, but it is rare that we need to be in an elevated prompt on our local system. This one action occasionally failing for my users because they are not elevated is often brought to my attention.

If I wanted a module installed on a server, I would use DSC.

ghost commented 6 years ago

I agree that, had we heard some of these arguments when we started we might have made a different design choice. However, breaking changes are not something we undertake when something is frustrating (which I understand that for some of you, this is). There needs to be a damaging impact with the current implementation before we would make a change that breaks all the currently working scripts, for users who either simply tolerate this behavior (which may be the case) or actually prefer it. My audience currently provides 11 million downloads per month. There's some really irritating behavior in the current design, I get that - but it is not damaging enough to break working scripts.

There are a couple of proposals that would not be breaking, would like to focus our attentions on those.

I thought perhaps an environment variable that could be set to change the defaults, but when you start troubleshooting that's going to be hidden from the person trying to understand why the default behaviors are reversed - so I'm not sure.

What would you folks suggest that is non-breaking but would accomplish your purposes?

markekraus commented 6 years ago

However, breaking changes are not something we undertake when something is frustrating

I am asking... no.. imploring you.. to very seriously reconsider that statement.

Users should not have to suffer bad design simply because fixing it would be a breaking change.

Making breaking changes for convenience is acceptable at a major version increase.

The idea that this needs to be suffered indefinitely by a majority of users simply because it doesn't meet some arbitrary benchmark or another is just ridiculous.

The negative impact of the change, as I suspect, is minimal. The positive impact of the change is worth it.

devblackops commented 6 years ago

RE: CI environments. You should not assume the CI environment has modules you depend on. That goes against the practice of explicitly declaring your dependencies in order to build and test your software in an accurate and consistent way. Your build script should be responsible for installing ALL your dependencies.

If you need to ensure modules are installed on a machine and available to all users, you should use DSC/Chef/Puppet.

RamblingCookieMonster commented 6 years ago

Hiyo!

I think this is a good idea, and think it would be worth a breaking change.

Here's the thing about this as a breaking change:

Given that this would absolutely not be a breaking change for all users, nor all users relying on the AllUsers default, and that it seems to have solid logic and community consensus behind it, IMHO it should at least be considered.

The module is versioned, right? Just bump the major version. I realize this is naive, but if folks run into an issue, you'll be helping them out, by pointing out their poor dependency management practices.

Cheers!

KevinMarquette commented 6 years ago

One option that I would like to put forward is a new cmdlet called Install-UserModule that just makes a proxy call to Install-Module with the scope predefined. Just like mkdir if you ever looked at it.

Those of us that understand the issue can easily adjust to that and user focused documentation could be updated to reference the new command over time.

I would rather the default was changed, but this is a workable alternative.

kilasuit commented 6 years ago

I think that there should be some internal logic that both fixes this and would not break existing functionality

So for a fix I'd propose that

How does that sound?

markekraus commented 6 years ago

@kilasuit It's actually been suggested previous in the issue.

I think there is a great potential for that behavior to break the Principle of Least Astonishment. Though, I suspect the users who will be affected by that may already being explicitly supplying it anyway.

rkeithhill commented 6 years ago

Adding to @kilasuit suggestion:

If it running in a non-elevated state - default to CurrentUser (as the AllUsers Scope requires elevation to install to) but also provide user input to inform that this is the action that is happening (non-interactively)

I'd tweak this a bit and suggest that in this scenario, the command inform the user that they have to be running as admin to install modules into the "default" AllUsers location. If they would like to proceed and install the module only for the current user, press Y (or N to cancel). Then add some helpful text like: "In order to avoid this prompt in the future, run from an elevated context to install modules for all users or supply the parameter -Scope CurrentUser". That, or -Confirm:0. And for extra credit, provide a prompt option to remember the user's choice and do not prompt again.

There is a concern with prompting but A) you should be able to check for non-interactive mode and B) this scenario would error anyway. The non-interactive case would just fail as it currently does.

SteveL-MSFT commented 6 years ago

I don't own this module, but I'll add my 2c as a user. In general, we should not require elevation/sudo where it's not absolutely required. In this case, I think it makes sense it should have been CurrentUser as the default, but breaking changes should not be taken lightly. If we think 5-10 years from now, I would expect that the majority of users will be frustrated that they have to add -Scope because it fails the first time they run it.

Adding heuristics whether the user is elevated or not will lead to confusing mistakes in my opinion.

rkeithhill commented 6 years ago

we should not require elevation/sudo where it's not absolutely required

Totally agree.

but breaking changes should not be taken lightly

Agreed but in this case how badly breaking is this really? The module should still install - just for the current user rather than all users. Do scripts that install modules really care if a module is only installed for the current user? I can only speak for myself in which case, no. My scripts run for me and on build systems where, if the module is installed for the current user, that is good enough. Then again, I don't write scripts to provision new PCs and servers. Maybe this is more important in that scenario.

If we think 5-10 years from now, I would expect that the majority of users will be frustrated that they have to add -Scope because it fails the first time they run it.

You don't have to wait 5-10 years from now. They've been frustrated ever since v5 shipped. :-)

kilasuit commented 6 years ago

this was raised initially in #2 and really should have been fixed then

Jaykul commented 6 years ago

I think that the root complaint here is about the process of installing -- not about where it installs and whether it's available to all users or not.

For the vast majority of people to be happy, all that's really needed is for Install-Module to fork an elevated process to do the install, instead of failing.

We would get simple install (for most people, just click yes on the UAC prompt), without compromising on either the "security" of having modules installed to a protected location, or the "functionality" of having modules installed to a shared location.

Everything could stay the way it is, default-wise -- and the only time you'd have a problem is when you were setting up (non-interactive) automation to install things. In that case, you can explicitly opt-in to the user install location, or rethink your automation plan and separate install from whatever else you're doing

edyoung commented 6 years ago

See pull request #315 which implements the suggestion by @kilasuit . FYI @alerickson

bmanikm commented 6 years ago

315 Implements below functionality

PS 3.0/4.0/5.0/5.1 : Default scope is based on the current user privileges. PS Core 6+: Default scope is always CurrentUser (it includes Windows as well). Users can still specify the -Scope parameter if they would like to install to the required scope path.

bmanikm commented 6 years ago

Resolved with #315.

TomBonnerAtTFL commented 6 years ago

Don't suppose we could ask for some clarity on the sudden perspective change @ Microsoft on this issue? This was issue #2 on this repo since 2016 and @JKeithB made it sound here as though the behaviour wasn't going to change. I think it's a great change to see though, so thank you.

kilasuit commented 6 years ago

@TomBonnerAtTFL essentially there were lots of conversations happened with a number of MVP's like myself and the team to push this change through as well as issues like this

edyoung commented 6 years ago

It's not really that there has been a sudden change - there's been a lot of spirited debate on this one both online and offline. We gradually came round to this being the best path forward.

edyoung commented 5 years ago

To restate the design rationale for the new behavior as of 2.0:

The main desire here is to make Install-Module more convenient when running interactively: on all platforms, you should not run as an elevated user all the time, and it is annoying to have to specify -Scope CurrentUser all the time to avoid an error. As desired, now if you don't specify the scope, Install-Module should succeed on all platforms whether you are elevated or not.

However we're concerned that there may be scripts that people run under a system account in order to provision packages which will then be run by other users - we know from other issues that have been raised that people want to register repositories that are usable by other users. So we did not change the behavior when running as an elevated account in Windows PowerShell only - that still installs for all users.