search

OpenVPN / tap-windows6

Windows TAP driver (NDIS 6)
Other
788 stars 237 forks source link

Alternate approach for setting scripts to run as admin #23

Closed selvanair closed 7 years ago

selvanair commented 7 years ago

The second commit essentially reverts the first one and uses ShellLink.dll to set the run as admin tag.

Only the shortcuts are set to run as admin -- those who want to run the batch files directly or tapinstall.exe should know how to open an elevated command prompt first.

Note 1: The source code of ShellLink.dll is available, the code looks clean and I could do a test build using mingw-w64 (the dll from upstream is used in the commit). This should be easier to maintain than ShellLink.nsh used in the first commit.

mattock commented 7 years ago

As mentioned here, the basic approach here is a good one. I will test this PR after OpenVPN 2.4.0 is out (=later this week).

@selvanair: After merging this PR I think we should append a version number to the installers. Something along these lines:

tap-windows-9.21.2-I001.exe

For example this PR does not actually change any code, so incrementing the actual driver version number would be confusing. I can implement the installer version thing and issue a PR.

Also, do you think this PR is enough to fix Trac#153? Or should we still use the external manifest from PR#22? I don't have any strong opinions on the matter myself...

Thoughts?

selvanair commented 7 years ago

Yes, incrementing the installer version when there are behavioural changes in utilities like this one sounds good.

For Trac#153, we could keep the external manifest as well which makes it possible to get UAC prompt for admin when the .bat or .exe is run directly. Making the link to run as admin (this PR) should still work independent of that.

That said, a lay user is supposed to use the link in that start menu and it will provide a better user experience (no multiple cmd windows that flash by). A more experienced user should know to run tapinstall.exe from an elevated command prompt. There are so many Windows admin tools that need to be run from an elevated prompt. Although Trac #153 specifically asks for a manifest, adding one internally or externally for a console application is going to create a new cmd window that cannot be paused and is not a good way of running tapinstall.exe. Instead, it would be nice to have tapinstall itself print an error message saying that admin rights is required instead of an unhelpful failure message.

chipitsine commented 7 years ago

there's uac plugin

http://nsis.sourceforge.net/UAC_plug-in

shall we switch to it?

selvanair commented 7 years ago

On Thu, Dec 29, 2016 at 12:47 AM, Ilya Shipitsin notifications@github.com wrote:

there's uac plugin

http://nsis.sourceforge.net/UAC_plug-in

shall we switch to it?

For what? The issue we have here on tap-windows is about making tapinstall.exe always run as admin. There are no issues about needing both user and admin rights during install which is what UAC plugin provides.

Selva

mattock commented 7 years ago

@selvanair : I tested the latest version of this PR and things worked properly. Can you squash the commits together, so that I can merge this?

selvanair commented 7 years ago

@mattock : squashed the two commits and added a link to the dll in the commit message.