fix: windows auto-update

[lidel]

This PR aims to close #1570
Context: #1570 (comment)

WIP

• revert fix: windows auto-update feature when selecting install for all users #1556 to reduce noise
• figure out local setup for testing autoupdate on windows
  • We won't know for sure, unless we test in production, but we don't need to use this app
  • I created sandbox electron app, which uses electron-build config similar to one in ipfs-desktop and basically the same CI setup
  • Next step is to copy autoupdate code from ipfs-desktop to that app, and see how it behaves on Windows.
• see if the latest electron-builder fixed the original problem upstream
  • no, still broken fix: windows auto-update #1679 (comment)
• if still broken: fix windows autoupdate
  • fix: fix: windows auto-update #1679 (comment)
• confirm it works when installed for single user
• confirm it works when installed for every user

cc @jessicaschilling @rafaelramalho19

[jessicaschilling]

@lidel - is this related to #1677?

[lidel]

@jessicaschilling unclear. that error could be a side effect of autoupdate issue, or a duplicate of #1676 – asked for more information.

[lidel]

Closing the shop for today, but quick update:

I looked into setting up local update server, but it effectively becomes Rube Goldberg machine and I don't believe it is a reliable way of testing and debugging this. To be sure we test the right thing, we literally need to test in production.

For that reason I created a test app with CI and electron config similar to Desktop.

I'll copy relevant autoupdate code from Desktop tomorrow, so it is identical, and then we have setup to run some end-to-end tests.

[lidel]

Tested with altschmerz v0.0.3 → v0.0.4) and the simplified autoupdate provided by electron-updater on windows and it works fine when app is installed for current user, but fails to update when "all users" option is selected during install:

The failure is the same as one described in #1514, so the original bug is still present in electron-updater when these NSIS settings are used.

Next:

• check if a viable fix exists,
  • if not, we have to either disable "all users" option
    or detect it and disable automatic install, asking user for manual update instead"

[lidel]

Disclaimer: I am not a Windows person, so this may be obvious stuff, but took me a while to understand all the moving pieces. Having the altschmerz test app helped a lot.

I believe the crux of the problem was that we use custom NSIS settings, namely:

nsis:
  oneClick: false
  warningsAsErrors: false
  perMachine: false
  allowToChangeInstallationDirectory: true
  allowElevation: true # note: this is true by default, even when not defined in config

By default we install per user (perMachine: false), but we also give user an option to change that via the screen from my previous comment (oneClick: false).

We don't want to require admin all the time, because users should be able to install IPFS on machines where they don't have Admin (schools, libraries). That is why isAdminRightsRequired is false in the update manifest (as described in original bug report #1570 (comment)).

electron-builder has a bunch of default macros for NSIS, and some higher level flags that control some Windows-specific behaviours, for example, electron-builder/**/nsis/NsisTarget.ts#L438-L444 sets:

    if (options.perMachine === true) {
      defines.INSTALL_MODE_PER_ALL_USERS = null
    }

    if (!oneClick || options.perMachine === true) {
      defines.INSTALL_MODE_PER_ALL_USERS_REQUIRED = null
    }

INSTALL_MODE_PER_ALL_USERS and INSTALL_MODE_PER_ALL_USERS_REQUIRED are dynamic flags that control various things, one of them is deciding if installer will ask windows for elevated privileges before doing its thing.

In our case, INSTALL_MODE_PER_ALL_USERS is ON and INSTALL_MODE_PER_ALL_USERS_REQUIRED is OFF, and UAC_RunElevated is only used in _REQUIRED scenario – see electron-builder/**/nsis/multiUser.nsh#L52-L62

TLDR the fix here is to

• (a) fix upstream
• (b) extend assets/build/nsis.nsh with customInit script that requests UAC_RunElevated when ${if} $installMode == "all" (independent of INSTALL_MODE_PER_ALL_USERS_REQUIRED)

I'll prepare (b) first, as (a) will take long time before the fix lands upstream, and I still don't trust there won't be any regressions, as we had so many in the past, unfortunately.

[lidel]

Tested in lidel/electron-updater-altschmerz#1 and ported the fix in 0bf120d – ok to merge and ship patch release?

[rafaelramalho19]

Tested and looks good, but we can only really find out on the next version... so 🚢 it !

[witcher112]

Hi guys!

I encountered this PR while resolving some of the UAC issues with electron-builder auto updater.

Just wanted to let you know that there might be an alternative solution - modifying the access rights after the installation, to grant full access permissions to the non-admin users (to be honest not my idea, I just observed what Steam does) so no administrative rights are required for the updater.

Here's the snippet for custom NSIS include:

!macro customInstall
  AccessControl::GrantOnFile "$INSTDIR" "(S-1-5-32-545)" "FullAccess"
  pop $0
  AccessControl::GrantOnFile "$INSTDIR" "(S-1-5-18)" "FullAccess"
  pop $0
!macroend

S-1-5-32-545 is a SID for Users group
S-1-5-18 is a SID for SYSTEM group

Note that you need to link AccessControl plugin to NSIS either through !addplugindir or by putting it into build/x86-unicode (source).

Cheers and good luck with your project!

[lidel]

@witcher112 thank you, useful to know there is an alternative approach in case current one stops working.

Maybe I misunderstood the meaning of GrantOnFile (or just lacking Windows background), but.. isn't granting write access to non-admin users effectively a decrease in security?

IIUC one user could overwrite binary that other users run.