Improve Chocolatey setup as administrator and add Test-ProcessAdminRights helper #486
Conversation
|
That's weird. Commits eaf8ab2 and 3ef2604 are shown here in the wrong order. 3ef2604 ("setup tests: add simulated standard...") should come first, just after ef5395e ("Enable mocking Test-AdminRights..."), then eaf8ab2 ("setup tests: with admin rights..."), then 6dfbc44 ("when running..."). My source branch for this PR has them in the correct order. |
| if (Test-Path Variable:\ChocolateyTestsMockAdminRights) { | ||
| Write-Debug "Test-AdminRights: returning mocked value $ChocolateyTestsMockAdminRights" | ||
| return $ChocolateyTestsMockAdminRights | ||
| } |
There was a problem hiding this comment.
You mentioned this somewhere as test code leaking into production code. You probably already know this won't be able to be pulled in as it is. I think we can find another way here, we just haven't discovered it yet.
There was a problem hiding this comment.
I figured it was more a guideline than a rule ;-)
But seriously, you're completely right. It was a quick and dirty prototype; I just needed an inspiration to think of a way and you in fact provided me with one in a later comment. Fixed!
|
Yes, the cruft in chocolateysetup has been reduced but definitely requires more work. It's nice to see that you are putting in some work in that respect. It's very much appreciated but it does show that we need some mocking or abstraction in some cases here as these tests would actually alter the state of the system/user environment variables. The last time there was something like that in chocolatey it killed out my entire path (that I later rebuilt from a backup). I didn't introduce the tests that did that, but they were fixed in a way where mocking was made easier. This is a good start. I have started conversations in some areas where I'm trying to understand what is going on or where I have specific concerns, especially with the use of test code in the production code. I think even though we explicitly import the helpers during installation, some of that code stands to be changed as well due to another error. I think we can find a smarter way to go about this, perhaps put the mocked code where the real code would have been for tests. |
|
Thanks for the encouragement :) I've reimplemented admin rights mocking in a clean way and added an environment manipulation abstraction, together with mock implementation. As an unplanned benefit, tests using those mocking facilities are now actually runnable as standard user - that gives additional security against making systemwide changes due to bugs or test interruption. Re your first question, for the record: no, this change will not trigger the UAC prompt. The actual, current process rights (I try to avoid the term 'privileges' as it has a very specific meaning on Windows) are examined. |
|
That was fun, for future reference |
|
This is in locally by the way. I haven't pushed it up yet as I'm verifying it all the way back to win2003 |
|
(I guess my earlier comment by e-mail didn't make it through...) Here's one (quite important) test improvement and the helper name change suggested earlier. |
|
Okay, let me back up the commits and set that up. |
1) Execute-WithEnvironmentBackup - to be able to freely manipulate the environment during tests. Exception-safe. 2) Add-EnvironmentVariable - set the variable at given scope, making sure to update it also at Process scope if necessary 3) Remove-EnvironmentVariable - remove from all scopes 4) Add-DirectoryToPath - add to path at given scope, making sure to update the path also at Process scope 5) Remove-DirectoryFromPath - remove it from path at all scopes 6) Assert-OnPath, Assert-NotOnPath - verify the directory is or is not present on path at given scope
Note: the tests currently assume the test suite is run with administrative rights (i.e. elevated) with UAC enabled. Unfortunately, it is not possible to test the most common scenario, i.e. first installation on a clean machine with no existing environment variables or arguments to the installer, without making specific assumptions about the test machine. (If the tests did that, the primary Chocolatey installation on the developer machine could get overwritten.) The default installation tests will thus only be performed if no existing Chocolatey installation is detected in the default install location.
Default installation (no arguments or preexisting environment variables) can be tested only when the test machine does not already have Chocolatey installed in the default location (otherwise it would get overwritten). To enable these tests, make sure your Chocolatey is installed in a custom location.
This allows testing setup behavior both with UAC enabled and disabled. The "mocking" mechanism actually changes the UAC registry setting, which is read by UAC checking code in Chocolatey. Since the change requires OS restart to take effect, it is indeed a mockup. However, the test suite should not be interrupted, so that the setting is changed back to the original value. No effort has been made to support running the tests on XP. A modern OS is assumed.
This helper tests whether the current user identity is a member of the Administrators group, i.e. if the current process has administrator rights. On systems with UAC enabled, this helper returns true only if the current process is running elevated (so it actually has admin rights). All existing admin checks in Chocolatey code are refactored to take advantage of this helper.
To check group membership, only TOKEN_QUERY is needed ([1]). The token, however, needs to be an impersonation token, so in certain circumstances TOKEN_DUPLICATE is also needed ([2]). Requesting the WindowsIdentity with minimum access level is slightly better from a security standpoint. [1] CheckTokenMembership: http://msdn.microsoft.com/en-us/library/windows/desktop/aa376389.aspx [2] WindowsPrincipal source, bool IsInRole (SecurityIdentifier sid): http://referencesource.microsoft.com/#mscorlib/system/security/principal/windowsprincipal.cs
As Chocolatey setup code explicitly imports the helpers module, Pester mocking mechanisms do not work (the mocks are overridden when the helpers are imported again). Mocking the admin rights check in setup tests is critical to making it possible to test Chocolatey setup at various user right levels. For setup tests, substitute Test-AdminRights implementation in the installation package with a stub returning a canned answer. This slightly reduces the accuracy of setup tests, as not 100% of production code is tested, but keeping production code pure is more important. As an added benefit, this is the first step in enabling the tests to be run as a standard user.
With the facility of mocking admin rights tests in place, it is now possible to test setup behavior for limited users (and un-elevated admins).
…vel regardless of UAC Provided the process is running elevated (i.e. with admin rights), Chocolatey should always be set up at machine (all users) level. There is no need to punish security-conscious users who run their systems with UAC enabled.
… Machine scope The CurrentPrincipal::IsInRole(Administrators) check is sufficient to ascertain administrative rights, whether UAC is enabled or not. In particular, if UAC is enabled, this check will only return true if running elevated. This change enables per-machine installation on systems with UAC enabled (only if the user installs/invokes Chocolatey from an elevated shell, so UAC prompt is not triggered), while not disrupting the behavior for standard users.
Since UAC status is no longer a factor influencing setup behavior, remove effectively duplicated tests. Tampering with UAC settings is also no longer necessary, which eliminates the threat of interrupted tests leaving the OS with changed UAC status.
Separated from the previous commit so that the previous diff is easier to review.
As per @ferventcoder's suggestion. Longer name, but makes it more explicit the current process rights are tested (in contrast to e.g. the user account being member of the Administrators group).
The three added functions wrap [Environment]::Get/SetEnvironmentVariable and obtaining variable names from the registry. The purpose is to enable mocking environment manipulation in tests. The functions are exported from the module, otherwise core Chocolatey (Chocolatey-Python, Chocolatey-RubyGem) would not be able to use them. Nevertheless, packages should in most circumstances use the high-level Install-ChocolateyEnvironmentVariable and Install-ChocolateyPath helpers. All Chocolatey code is changed to use these new functions.
Add implementations of environment manipulation functions that operate on an object stored in a global variable, instead of on the live system environment. All test code that might manipulate the environment should be wrapped in a call to Execute-WithEnvironmentProtection. This function takes a snapshot of the environment, initializes the mocked environment state based on that snapshot, executes test code and reverts any modifications to the environment to the state from the snapshot. Reverting environment changes is important for two reasons: 1) although the mocking mechanism prevents changes to the actual Machine and User environment (by cooperating code), Process environment still needs to be restored. 2) there is the possibility that some un-cooperating code will execute that will, by bug or omission, bypass the environment management functions and modify the live environment. The environment restoring code will mitigate the damage and will emit a warning in such case, so that the culprit may be caught. If a test case tries to alter the environment without using the mocking/protection facility, an error will occur, instructing to use the appropriate construct in the test case. Together with mocking admin rights tests implemented earlier, this commit makes it possible to run Chocolatey setup and Update-SessionEnvironment tests as standard user, with no fear of corrupting systemwide state. Implementation note: diagnostic messages are emitted at Verbose level, because Debug failed to appear during setup tests (no time to investigate).
|
Squashed somewhat (primarily bugfixes and the earlier Test-AdminRights mocking attempt) and rebased on stable. |
jberezanski
changed the title
|
Merged into stable at 02eccc9 |
This expands upon GH-453 by supporting the scenario where UAC is enabled but Chocolatey installer is running with administrator rights (i.e. from an elevated shell).
First commit adds a couple of helper functions that make it easier and safer to write tests altering the environment.
Commits 2 and 3 introduce tests for Chocolatey installation, assuming running with admin rights, but with UAC enabled (i.e. the default case on my system).
Commit 4 introduces tests for scenarios with UAC disabled. It is done by temporarily (for the duration of each test case) adjusting the UAC setting in the registry. Such a change does not take effect until the system is restarted, but Chocolatey code that tests UAC status is fooled.
Next two commits add a helper, Test-AdminRights, that implements the logic to check if current user identity is a member of the Administrators group. Apart from the obvious DRY benefit of centralizing that logic in one place, this also makes it possible to mock that test and simulate standard user scenarios even when running with administrator rights (as the test suite is run).
The mocking facility is implemented in the next commit, using a global variable to control whether the helper should return the truth or a fixed answer. Ugly, I know, but the only way I found that would work while testing Chocolatey installation (the installer explicitly imports the helpers, so Pester mocks get overridden).
With admin rights mocking infrastructure in place, it is now possible to test Chocolatey installation in standard user scenarios (commit 8).
Commits 9 and 10 implement (TDD-style) the gist of this PR. Namely, if Chocolatey installation is performed with administative privileges, environment variables are set at Machine scope. UAC status is irrelevant, only the actual rights of the process matter.
Since UAC statuc is no longer a factor influencing Chocolatey behavior, the last two commits remove the UAC mocking facility and eliminate duplicate tests.
Incidentally, in-depth testing of Chocolatey installation uncovered some cases (marked with comments in the tests), where the installer behavior with respect to environment variables is, in my opinion, suboptimal. I will bring those issues to discussion separately, once this PR is verified and accepted.