Skip to content
This repository has been archived by the owner. It is now read-only.

Improve Chocolatey setup as administrator and add Test-ProcessAdminRights helper #486

Closed

Conversation

@jberezanski
Copy link
Contributor

jberezanski commented Jun 13, 2014

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.

@jberezanski

This comment has been minimized.

Copy link
Contributor Author

jberezanski commented Jun 13, 2014

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.
a00ec70 and 05d3e22 are swapped too. What the...?

if (Test-Path Variable:\ChocolateyTestsMockAdminRights) {
Write-Debug "Test-AdminRights: returning mocked value $ChocolateyTestsMockAdminRights"
return $ChocolateyTestsMockAdminRights
}

This comment has been minimized.

Copy link
@ferventcoder

ferventcoder Jun 14, 2014

Contributor

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.

This comment has been minimized.

Copy link
@jberezanski

jberezanski Jun 14, 2014

Author Contributor

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!

@@ -0,0 +1,30 @@
function Test-AdminRights {

This comment has been minimized.

Copy link
@ferventcoder

ferventcoder Jun 14, 2014

Contributor

This may be better named Test-ProcessAdminRights for clarity.

This comment has been minimized.

Copy link
@jberezanski

jberezanski Jun 14, 2014

Author Contributor

Perhaps, yes. I think I like my version slightly better (less typing), but as code is written once and read a lot of times, clarity wins.
In any case, I'll leave it for dessert.

Write-Debug "Deleting environment variable $_ at Process scope"
[Environment]::SetEnvironmentVariable($_, $null, 'Process')
}
}

This comment has been minimized.

Copy link
@ferventcoder

ferventcoder Jun 14, 2014

Contributor

Tests that are actually making changes to the environment scare me a bit. A glitch where it doesn't get to clean up is going to leave the system in an unusable state.

This comment has been minimized.

Copy link
@ferventcoder

ferventcoder Jun 14, 2014

Contributor

If we could find a way to mock out the actual calls to the system and test the behavior of chocolatey based on what is returned from those calls, I think we can call these unit tests.

Actual hits to the system are considered physical integration tests and should probably be moved somewhere else.

This comment has been minimized.

Copy link
@jberezanski

jberezanski Jun 14, 2014

Author Contributor

I implemented a very thin abstraction layer, wrapping [Environment]::Get/SetEnvironmentVariable and obtaining a list of environment variable names (needed by one helper). In test code I can now replace that layer with mocks that read and write in-memory state.
I've left the existing backup/restore mechanism in place, first to protect against bugs and code not going through my layer (no such code now, but someone might forget and write a direct call to [Environment] in the future), second to restore process-level environment between test cases.
The restore mechanism will now warn if it detects a change at user or machine level, because that means some code did not use the layer and should be fixed.

$script:tmpDir = 'TestDrive:\chocotmp'

Get-ChildItem "$base\nuget\tools" | Copy-Item -Destination $tmpDir -Recurse -Force
Get-ChildItem "$base\src" | Copy-Item -Destination "$tmpDir\chocolateyInstall" -Recurse -Force

This comment has been minimized.

Copy link
@ferventcoder

ferventcoder Jun 14, 2014

Contributor

$script:tmpDir for these two variables.

This comment has been minimized.

Copy link
@jberezanski

jberezanski Jun 14, 2014

Author Contributor

Right. To be precise, it does not make a difference now, but for robustness, yes.


function Setup-ChocolateyInstall($path, $targetScope)
{
Remove-EnvironmentVariable 'ChocolateyInstall'

This comment has been minimized.

Copy link
@ferventcoder

ferventcoder Jun 14, 2014

Contributor

Does this do what I think it does?

This comment has been minimized.

Copy link
@jberezanski

jberezanski Jun 14, 2014

Author Contributor

Each test case has specific requirements as to where (at what scope) should ChocolateyInstall be set and to what value. This line removes the variable from all scopes (machine, user, process), to prepare a clean initial state as a base for further test setup code.

@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Jun 14, 2014

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.

@jberezanski

This comment has been minimized.

Copy link
Contributor Author

jberezanski commented Jun 14, 2014

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.

@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Jun 17, 2014

That was fun, for future reference git rebase --onto stable master while in the feature branch.

@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Jun 17, 2014

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

@jberezanski

This comment has been minimized.

Copy link
Contributor Author

jberezanski commented Jun 17, 2014

(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.
Some of the commits could also be squashed for a more streamlined PR - let me know if you'd prefer that.

@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Jun 18, 2014

Okay, let me back up the commits and set that up.

jberezanski added 15 commits Jun 13, 2014
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.
…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.
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).
… 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).
@jberezanski

This comment has been minimized.

Copy link
Contributor Author

jberezanski commented Jun 19, 2014

Squashed somewhat (primarily bugfixes and the earlier Test-AdminRights mocking attempt) and rebased on stable.
Original commits available for reference here.

@jberezanski jberezanski changed the title Improve Chocolatey setup as administrator and add Test-AdminRights helper Improve Chocolatey setup as administrator and add Test-ProcessAdminRights helper Jun 24, 2014
@ferventcoder

This comment has been minimized.

Copy link
Contributor

ferventcoder commented Jun 25, 2014

Merged into stable at 02eccc9

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
2 participants
You can’t perform that action at this time.