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

fix: Windows update fails for custom paths that require admin rights #6073

Merged
merged 3 commits into from
Jul 24, 2021

Conversation

hvuntokrul
Copy link
Contributor

Proposed fix to the situation on Windows where an Electron is installed for all users of the machine e.g. into Program Files, but fails to update (installed app version is deleted but the update is never installed) due to failure to request admin rights.

node v12.16.2
npm v6.14.4

Current build.nsis settings in package.json:
{
"oneClick": false,
"allowToChangeInstallationDirectory": true,
"allowElevation": true,
...
}

Proposed fix to the situation where an Electron is installed for all users of the machine e.g. into Program Files, but fails to update (installed app version is deleted but the update is never installed) due to failure to request admin rights.

Current build.nsis settings in package.json: 
{
	"oneClick": false,
	"allowToChangeInstallationDirectory": true,
	"allowElevation": true,
	...
}
@hvuntokrul hvuntokrul changed the title Update BaseUpdater.ts Update BaseUpdater.ts to fix update fails due to lack of admin rights Jul 21, 2021
@hvuntokrul hvuntokrul changed the title Update BaseUpdater.ts to fix update fails due to lack of admin rights Fix: Update fails due to lack of admin rights Jul 21, 2021
@hvuntokrul hvuntokrul changed the title Fix: Update fails due to lack of admin rights fix: Update fails due to lack of admin rights Jul 21, 2021
@mmaietta mmaietta linked an issue Jul 21, 2021 that may be closed by this pull request
@mmaietta mmaietta changed the title fix: Update fails due to lack of admin rights fix: Windows update fails for custom paths that require admin rights Jul 21, 2021
@mmaietta mmaietta linked an issue Jul 21, 2021 that may be closed by this pull request
Fix errors resulting in rest failure, add new variable to the log
Update temp file name to avoid conflicts and remove after successful write
@mmaietta mmaietta merged commit 45fc0a0 into electron-userland:master Jul 24, 2021
@jmeinke
Copy link
Contributor

jmeinke commented Aug 23, 2021

I don't know for sure but I suppose there might be an error related to this PR:
After upgrading electron-builder to v22.11.11 and updater to the version 4.4.3 the elevated rights are being requested even if the app has been installed per-user into the standard path on Windows 10.

We'll downgrade electron-updater to version 4.3.10 in order to check, if this PR has introduced this unexpected behaviour. I'll add another post when we have the results.

@jmeinke
Copy link
Contributor

jmeinke commented Aug 26, 2021

@mmaietta I can confirm that this PR (electron-updater v4.4.3 as opposed to v4.3.10) will result in elevated rights being requested even for a per-user update via quitAndInstall -> should not. IMO this improvement introduces a regression and should be improved / fixed.

@hvuntokrul
Copy link
Contributor Author

hvuntokrul commented Aug 27, 2021

@jmeinke Thank you for your reply.
Could you please:

  • tell if this behaviour has been reproduced on more than 1 machine;
  • confirm that the user is in no way limited in their read/write rights;
  • try to write out the exact steps to reproduce this behaviour?

Code review and our own testing did not confirm this bug nor any obvious reason for it to exist (the entire PR is basically just one file write/delete operation).

Thank you:)

@sandeep1995
Copy link
Contributor

I can confirm this issue. It is happening with a lot of our users.

    "electron-builder": "22.11.11",
    "electron-updater": "4.4.3",

@mmaietta
Copy link
Collaborator

Please update to latest electron-updater to leverage this fix.

mmaietta pushed a commit that referenced this pull request Nov 30, 2021
…nstall/updates (#6450)

* fix(nsis): fix per-machine installs

Add @krisdages changes from PR #6438 to elevate silent per-machine installs.
Remove PR #6073 that incorrectly elevates all per-machine installs, breaking interactive per-machine installs.

Closes #6425, #5468

Co-authored-by: Robert Patrick
Co-authored-by: Kris Dages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Auto-updater only running uninstaller as administrator
4 participants