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

Installation fails with "access denied" error if installer not launched elevated #829

Closed
1 task done
chrullrich opened this issue Jul 21, 2016 · 15 comments
Closed
1 task done
Labels

Comments

@chrullrich
Copy link

  • I was not able to find an open
    or closed issue
    matching what I'm seeing

If an earlier version of Git is already installed, the installation fails if the installer was not started while elevated. It complains about not being able to create a directory at "C:\Program Files\Git\tmp". This is unexpected because the installer prompts for elevation at the beginning of the actual installation. It works if the installer is run as elevated.

This behavior is not new in 2.9.2, I have seen it in earlier versions (2.8.x or 2.9.0) as well.

This looks similar to #528, except that issue also mentions a regression from 2.6.2 to 2.6.3 (no elevation prompt during installation anymore) and my error message happens at a different time (between items 5 and 6 on #528's timeline).

git-setup-lua.txt – Installation log, setup started as normal user
git-setup-elevated-short.txt – Installation log, setup started from elevated command prompt, log size reduced by 1750 KiB, full log available on request

Setup

  • Which version of Git for Windows are you using? 32-bit or 64-bit? Include the
    output of git version as well.

    Git-2.9.2-64-bit.exe

$ git --version
git version 2.9.2.windows.1
  • What options did you set as part of the installation? Or did you choose the
    defaults?

    No shell extensions. Plink for the LUA install, the elevated one does not ask that. Defaults otherwise.

  • Any other interesting things about your environment that might be related
    to the issue you're seeing?

    No.

Details

Git-2.9.2-64-bit.exe
  • What did you expect to occur after running these commands?

    Git to be upgraded from 2.9.0 to 2.9.2.

  • What actually happened instead?

    When the "uninstalling Git" progress bar reached the end, there was an error message saying access was denied while trying to create a directory at "C:\Program Files\Git".

@fourpastmidnight
Copy link

I had something similar happen before when upgrading. Can you tell me, did you have Visual Studio open, or some other program that uses your installation of Git open while you were upgrading?

@chrullrich
Copy link
Author

No, nothing like that.

It has nothing to do with any open files anyway. For one thing, the directory is completely empty when the upgrade is done removing the previous installation. For another, I just did a procmon trace, and that looks fishy to me. There are five processes involved, each one starting the next:

Git-2.9.2-64-bit.exe (LUA)
-> Git-2.9.2-64-bit.tmp (LUA)
   -> unins000.exe (LUA)
      -> unins000.exe (elevated)
         -> _is14D2N.exe (elevated)

When the previous version is removed, all file operations are done in the _is14D2N process, and once that is done deleting, it exits, and the unelevated git-2.9.2-64-bit.tmp process (the second one above) attempts (and inevitably fails) to create the "tmp" directory.

@dscho
Copy link
Member

dscho commented Jul 22, 2016

That is strange, indeed. We changed the setting so that the Git installer would always run elevated, hence I am puzzled how your Git-2.9.2-64-bit.tmp is not run elevated...

@dscho
Copy link
Member

dscho commented Jul 22, 2016

FWIW here is the relevant change to require elevation: git-for-windows/build-extra@23672af

@chrullrich
Copy link
Author

Therefore, the none setting is actually exactly what we want: users
who have credentials to write to C:\Program Files are presented with an
elevated installer that let's them do precisely that, users without
those rights will be presented with an installer that let's them install
Git into AppData in their home directory.

That reads like nonadmin users should never be able to install into Program Files, and indeed, if no Git installation exists on the system, the default installation path for a LUA user is AppData. If, however, the installer detects an existing installation, it will not only default to that installation's path, it will not even present the wizard page anymore where the path can be edited.

What I think happens is this: The installer starts up, goes into "upgrade mode" because it detects the existing installation, and skips the wizard page for the installation path together with the check if that path is writable to the current user. It then delegates removing the existing installation to the uninstaller. The uninstaller determines that elevation is required, prompts for it, and does its job. Then the installer takes back over, running unelevated because that is what it does with a nonadmin user, and fails.

@chrullrich
Copy link
Author

I have to amend my bug description above. For

the installation fails if the installer was not started while elevated

please read

the installation fails if the installer was not started as an administrative account

. My everyday user account is not in Administrators at all, so I get the LUA behavior when I start the installer, not the immediate consent prompt that happens when starting it as an unelevated admin.

@dscho
Copy link
Member

dscho commented Jul 22, 2016

Ah, that explains a lot. I guess most people upgrade using the same account as when they installed the initial version... So they will never see that problem.

Maybe it would help to simply force to show the page with the target directory?

@chrullrich
Copy link
Author

I don't think so. It would prevent a non-admin upgrade attempt, but that is not what should happen here. After all, non-admin users can initiate a first installation (getting the elevation prompt when it becomes necessary), so why should they not be able to start an upgrade?

And what will you do if the user continues from the page with a dfferent path than the preset? Install a second copy? Move-and-upgrade the existing one?

@ghost
Copy link

ghost commented Jul 26, 2016

C:\Program Files - is secure folder that need administrative account to get write privileges.
You can change C:\Program Files\Git permissions in security tab in folder properties to make it writable by non-admin.

@dscho
Copy link
Member

dscho commented Apr 4, 2017

I don't think so. It would prevent a non-admin upgrade attempt, but that is not what should happen here. After all, non-admin users can initiate a first installation (getting the elevation prompt when it becomes necessary), so why should they not be able to start an upgrade?

And what will you do if the user continues from the page with a dfferent path than the preset? Install a second copy? Move-and-upgrade the existing one?

@chrullrich So what do you suggest we should do to fix this problem?

@chrullrich
Copy link
Author

chrullrich commented Apr 5, 2017

A good question I was hoping to avoid ...

My first preference would be a complex prompt of up to four options (system-wide/per-user) * (upgrade/new installation), depending on what exists on the system. Since that is the opposite of user-friendliness, it won't work.

So instead I think the installer should use <requestedExecutionLevel level="highestAvailable"/>, i. e. automatic consent prompt, but no elevation prompt unless "runas", and then act purely on a per-user scope when started as unelevated, and on a system-wide scope otherwise. This prevents non-admins from initiating a system-wide upgrade, and prevents admins from creating a per-user installation. That is contrary to what I said before, but what I would like and what makes sense are clearly different things.

@fourpastmidnight
Copy link

What appears that needs to happen here is that the installer needs to do a better job of detecting a per-user or per-machine install on the system during upgrade. But I'm going to contradict myself and say that the installer should be either per-user or per-machine, but not both.

My first reason for saying so is that this is the guidance for creating installers on MSDN. But that reason alone is not enough for some, so I have more practical reasons.

Another reason is similar to some scenarios laid out above, but slightly different. Imagine a user has installed GfW, and they are non-admin, so this is a per-user install. Now an admin comes along to install GfW on the machine (maybe corporate policy changed and all dev machines will have GfW installed). What should happen? For one, installers can't detect installed programs for other users, so the admin installation would never know a per-user installation existed on the machine already. This is already a problem, because the per-machine installation will clobber the existing per-user installation in terms of the system-wide configuration files, and (potentially) system-wide PATH changes. There's also the consideration of Windows Explorer context menu items and desktop shortcuts. And finally, the registration of various file types, too.

It would seem to me, that we already have a solution for a "per-user" "installation"--and that is portable git. No machine-wide configuration is done with that package; it's not registered as installed by Windows, etc. So why try to support per-user installs in the installer? It's complicated to get right, if not, in this case (as I demonstrated in my example), impossible. I fear these types of issues will not abate if we continue to ship an installer that does both per-machine and per-user installations.

@dscho
Copy link
Member

dscho commented Apr 5, 2017

It would seem to me, that we already have a solution for a "per-user" "installation"--and that is portable git.

The only problem with that: it does not install into the Start Menu and neither associates the .sh extension with the Bash, nor does it offer the rudimentary Windows Explorer integration offered by the Git for Windows installer...

But yes, I agree that this is a complicated issue. Unless we find a viable strategy, I am inclined to leave things as-are.

@dscho dscho added the unclear label Apr 5, 2017
@fourpastmidnight
Copy link

@dscho, I'm inclined to agree--if we want to allow the installer to perform both per-user and per-machine installations, then there will always inherently be some risk that some combination of installations will not work correctly.

@dscho
Copy link
Member

dscho commented Apr 6, 2017

Okay, I'll close this ticket tentatively.

It is easy enough to reopen it and follow up on any clever idea once it comes around.

@dscho dscho closed this as completed Apr 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants