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

Why would a small minority of users get prompted by macOS for an admin password on quitAndInstall and then not receive the update? #6120

Open
aguynamedben opened this issue Aug 1, 2021 · 4 comments

Comments

@aguynamedben
Copy link
Contributor

@aguynamedben aguynamedben commented Aug 1, 2021

  • Electron-Builder Version: 22.11.1
  • Electron-Updater Version: 4.4.3
  • Node Version: 14.16.0
  • Electron Version: 12.0.6
  • Electron Type (current, beta, nightly): current
  • Target: macOS 10.16.0, 11.5.0

I have 2 users that work at the same company that report a similar issue during auto-updates. The end result is these 2 users are in a loop that never lets them receive the auto-update. Every time we release a new version of our app, they have to manually install it by re-downloading our app from our website.

  • Our electron-updater process kicks off on a 4 hour timer.
  • It seems the new version is downloaded successfully.
  • Around autoUpdater.quitAndInstall(), in response to updated-downloaded, both users report a macOS admin password popup appears in order to request permission for the update.
  • One user reports that before they can type the admin password, the app quits.
  • The other user reports that they type their admin password, then the app quits.
  • Both users don't get the new version of the app.
  • Our electron-updater process kicks off on a 4 hour timer.
  • ... rinse and repeat. The end result is these users get stuck on an old version of our app, and our app does this really annoying password/quit thing that is perceived by them as a crash.

What would cause macOS to ask for a password during quitAndInstall? These users work at the same company so I'm wondering if they have any locked-down corporate permissions. Or is it something with the ShipIt directory file permissions?

Any ideas are welcome. 97% of our users get auto-updated correctly, so I know things generally work with notarization, signing, etc. but it seems like there is something with these users' environment that is producing a bad user experienced with auto-updating.

If asked them to ls -alh a bunch of directories to see permissions and paste their ShipIt stderr logs, I'll report back when I get those.

$200 donation incoming! Thank you for all the open-source work you do. 🙏 🙏 🙏

@aguynamedben aguynamedben changed the title Why would a small minority of users get prompted by macOS for an admin password on quitAndInstall? Why would a small minority of users get prompted by macOS for an admin password on quitAndInstall and then not receive the update? Aug 1, 2021
@develar
Copy link
Member

@develar develar commented Aug 1, 2021

Around autoUpdater.quitAndInstall(), in response to updated-downloaded, both users report a macOS admin password popup appears in order to request permission for the update.

I suspect due to using of proxy — due to limitation of Squirrel.Mac, electron-updater have to start a temporary proxy server.

server.listen(0, "127.0.0.1", () => {

Loading

@develar
Copy link
Member

@develar develar commented Aug 1, 2021

To understand what's going on, additional debug logging was added (#6122, not yet released),

But current logging maybe also enough to better understand the reason of the issue.
Do you use electron-log? Please provide electron-updater debug logging. See https://www.electron.build/auto-update#examples about

    const log = require("electron-log")
    log.transports.file.level = "debug"
    autoUpdater.logger = log

Loading

develar added a commit that referenced this issue Aug 1, 2021
…acUpdater to investigate #6120 (#6122)

* fix(electron-updater): small cleanup and add more debug logging for MacUpdater to investigate #6120

* Create stale-chairs-dress.md
@aguynamedben
Copy link
Contributor Author

@aguynamedben aguynamedben commented Aug 5, 2021

Hey @develar, thank for you so much for the quick response. I'm 99% sure I figured this out, and I'm 99% sure it's user error. 🤦‍♂️

tl;dr Don't call app.exit() after autoUpdater.quit()

Lets say there are 2 different users on a Mac:

  • ben, an admin user
  • music, a non-admin user

If an app was installed by the admin user (ben) but the auto-update is triggered by the non-admin user (music), autoUpdater.quitAndInstall() results in macOS showing a credentials pop-up. Windows are killed, but the main process is still running and blocked until this credentials pop-up is dealt with. If everything is setup right, the non-admin can enter admin credentials—i.e. ben/<password>—after which the app quits, the update is installed, and the app restarts.

(Side note: It's helpful to turn on Fast User Switching and to use Minio as a fake S3 host to reproduce admin vs. non-admin auto-updater situations.)

Here's where it went wrong in our case: We've had previous difficult-to-decipher issue where the auto-update process failed to quit/restart the app reliably, so one of us changed this code:

// ... in response to `update-downloaded`
setTimeout(() => {
  log.info(`quitting and installing now`);
  autoUpdater.quitAndInstall();
}, 4000);

to this

setTimeout(() => {
  log.info(`quitting and installing now`);
  autoUpdater.quitAndInstall();
  app.exit();
}, 4000);

In an effort to ensure the app actually got quit.

The addition of app.exit(), however, causes this to happen:

  • Auto-update triggered by non-admin user (music)
  • updated-downloaded event fires
  • The macOS admin dialog is shown to music
  • Before music has change to enter admin credentials, the app quits
  • The app is not restarted
  • The app is still installed, but isn't updated

This resulted in a loop where these users could never successfully auto-update. The users reported this behavior as as "the app crashing" because every 4 hours we check for updates and this sequence would be triggered.

The fix was to switch to the official (?) @develar restart code from this comment.

// long history of this not working well
// https://github.com/electron-userland/electron-builder/issues/3271
// https://github.com/electron-userland/electron-builder/issues/3269
// do it just like develar says: https://github.com/electron-userland/electron-builder/issues/1604#issuecomment-306709572
log.info(`quitting and installing now`);
setImmediate(() => {
  app.removeAllListeners('window-all-closed');
  const browserWindows = BrowserWindow.getAllWindows();
  log.info(`closing ${browserWindows.length} BrowserWindows for autoUpdater.quitAndInstall`);
  for (const browserWindow of browserWindows) {
    browserWindow.close();
  }
  autoUpdater.quitAndInstall(false);
});

The key change is really just that we're not calling app.exit() immediately after autoUpdater.quitAndInstall().

💡 My recommendation is that the electron-updater README should have a few implementations to show how people can author a safe handler for the update-downloaded event. I think we've messed up the implementation of that function 2-3 times over the past few years. What do you think about that recommendation? I'm happy to submit a PR.

We're going to ship a new version of our app with this change. I'll report back if this fixes it. Thanks again for all the work you do on this project. It's well worth our donation and we really love this library. 🙏 🙏 🙏

Loading

@mmaietta
Copy link
Collaborator

@mmaietta mmaietta commented Aug 8, 2021

@aguynamedben please let us know how that goes!
I like your recommendation and would happily review a PR.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants