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

Implement auto updater for Windows and Mac #582

Merged
merged 41 commits into from
Dec 19, 2018

Conversation

yuya-oc
Copy link
Contributor

@yuya-oc yuya-oc commented Sep 4, 2017

Before submitting, please confirm you've

Please provide the following information:

Summary
Implement auto updater.

This is a first prototyping of auto-updater. At this time, this PR only considers the Windows installer.

NSIS installer is introduced. It integrates 32-bit and 64-bit installers into one installer file. And the new installation folder becomes %USERPROFILE%\AppData\Local\Programs\mattermost (Per user install). We can also choose C:\Program Files (Per machine install) by modifying electron-builder settings.

updater-dialog

Issue link
#15

Test Cases
I prepared a version, v9.9.9 for testing: https://circleci.com/gh/yuya-oc/desktop/366#artifacts

A. simple case with CircleCI

  1. Install the app.
  2. Edit %USERPROFILE%\AppData\Local\Programs\mattermost\resources\app-update.yml
    • Set 'https://366-67276967-gh.circle-artifacts.com/0/tmp/circle-artifacts.0utXb6g/' to the value of url
  3. The app will check updates to CircleCI URL.

B. manual testing for various cases.

  1. Prepare a http server to locate latest.yml and mattermost-setup-9.9.9.exe.
    Example: http-server
    1. Install Node.js.
    2. Execute npm install -g http-server
    3. Execute http-server -p 8081 under the folder where artifacts are located.
  2. The app will check updates to http://localhost:8081/latest.yml.
  3. You can change the test version and release date by editing latest.yml.

Additional Notes
https://circleci.com/gh/yuya-oc/desktop/365#artifacts

@jasonblais
Copy link
Contributor

Wow wow wow!!!

Thanks! Will begin the review soon (we're cutting the release candidate for the next server release tomorrow so it might be a couple of days)

return (
<div>
<Navbar>
<h1 className='UpdaterPage-heading'>{'New update is available'}</h1>
Copy link
Contributor

@MusikPolice MusikPolice Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these strings need to be translated into the users' language? Is it possible to access the localization system from here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessary because the desktop app doesn't have any localization system for now.

const AutoLaunch = require('auto-launch');

async function upgradeAutoLaunch() {
if (process.platform === 'darwin') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Darwin is MacOS, right? What does this do on Linux platforms? If it's only supported on Windows, I wonder if a whitelist approach might be best here.

Copy link
Contributor Author

@yuya-oc yuya-oc Sep 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, darwin means macOS. On Linux, this function should behave as well as Windows: Reconfigure auto-start for the current version. The feature is not supported on macOS, so it returns soon.

}

function isUpdateApplicable(appState, updateInfo) {
if (appState.updateCheckedDate.value - (new Date(updateInfo.releaseDate)).value < interval48hours) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these two dates always in a fixed time zone? Like are they both UTC dates? If either is based on the user's timezone, then we may not actually wait 48 hours.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UTC is always used.

  • updateCheckedDate: Date.prototype.toISOString()
  • releaseDate: I can't show in source code. But I confirmed that the date uses UTC ('Z' in ISO format)

@jasonblais
Copy link
Contributor

We can also choose C:\Program Files (Per machine install) by modifying electron-builder settings.

Does this mean we as in "us" or can the admin modify this in anyway (I assume it's "us" but thought to ask).

As for testing, wondering if there's anything I can test (except review the UI you posted above) until we have a local http server of some kind

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Sep 6, 2017

Yeah, it means "us". We can choose one of them:

  • Per user (under AppData) (current)
  • Per machine (under C:\Program Files)
  • Choose one of them in install wizard

Whichever we choose one of them, we can add the option to select install location by adding install wizard.

This should be discussed at #491, but I wanted to tell a little here.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Sep 7, 2017

Updated the artifacts link and test cases. You would be able to see the dialog after 48 hours by following the steps A.

@jasonblais
Copy link
Contributor

@yuya-oc Let's not merge #582 until PRs contributed by @csduarte are merged to avoid conflicts for those PRs

https://github.com/mattermost/desktop/pulls/csduarte

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Sep 9, 2017

@jasonblais Okay, I will continue to work on this branch.

@jasonblais
Copy link
Contributor

Thanks @yuya-oc! I'll also get feedback from others about our approach

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Sep 19, 2017

Rebased and added a feature for Windows zip. When using zip packages, "Install Update" button becomes "Download Update" button and it opens https://about.mattermost.com/download/#mattermostApps in user's default browser.

https://circleci.com/gh/yuya-oc/desktop/382#artifacts

@yuya-oc yuya-oc changed the title WIP: Implement auto updater Implement auto updater for Windows and Mac Sep 21, 2017
@yuya-oc yuya-oc added the MacOS label Sep 21, 2017
@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 18, 2017

Rebased: https://circleci.com/gh/yuya-oc/desktop/500#artifacts

@jasonblais To test Mac, we need code-signing for newer app (v9.9.9: https://circleci.com/gh/yuya-oc/desktop/501#artifacts) Would you help this? (automation is not needed for now)

@jasonblais
Copy link
Contributor

@yuya-oc Awesome! Thanks. I've set time for myself to test this PR thoroughly, and planning to ask other team members to help as well.

I'll ask Joram or Jonathan to help code-signing for the Mac

@jasonblais
Copy link
Contributor

@yuya-oc I installed the Windows .exe file and edited the URL for app-update.yml as https://366-67276967-gh.circle-artifacts.com/0/tmp/circle-artifacts.0utXb6g/.

I'll wait for 48 hours to see if the update prompt comes up Tuesday evening.

Few initial questions/thoughts:

  1. Do we have a flag somewhere to turn off auto-updates by the admin (e.g. a customer deploying a custom fork of the app, or a package manager like Chocoleatey distributing their own package)?
  • if not, would this be added to BuildConfig.json, perhaps?
  1. Can you help update NOTICE.txt with new dependancies?
  2. Can you help edit the upgrade prompt interval from 48 hours to 5 minutes for testing purposes? It'll make it faster to test all the different packages, as I'm planning to set aside time for Monday/Tuesday to go through all 4 variations for Windows, and one for Mac, and hopefully get other people to test it out too.
  3. Can you help add a "Check for Updates..." option in the "Help" menu bar, as described in slides 13 and 14 of the spec
  4. How much work will be involved for adding the auto-update prompt for Linux, similar to what's implemented for Windows .zip files? (Just so I know)

@jasonblais
Copy link
Contributor

Also, just want to reiterate a big thanks for your help on this PR. It'll be a great feature to add for v4.0.

My aim is to get all testing and feedback summarized here in the next few days, and prepare to merge this PR within the next two weeks for v4.0 🎉

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 20, 2017

@jasonblais Sorry, probably I had mistake when rebasing. Please stop test for a moment.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 20, 2017

Updated: https://circleci.com/gh/yuya-oc/desktop/502#artifacts

  1. Now I'm considering several options.
  • Package maintainers: By adding an extra file (app-updater-config.json) into packages, they can override auto-updater parameters.
  • Admins: They can override parameters via pre-configuration.
  1. Okay, I'll do later.
  2. 5min: https://circleci.com/gh/yuya-oc/desktop/503#artifacts
    v9.9.9: https://circleci.com/gh/yuya-oc/desktop/504#artifacts
  3. It was missing due to my mistake. It should appear at current artifacts.
  4. Prototype is ready. However it's written from zero. So for now, I would like to focus on Windows and Mac.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 11, 2018

Hi @esethna @hmhealey, Updated the branch with production config.

@esethna
Copy link
Contributor

esethna commented Nov 12, 2018

Thanks @yuya-oc! @hmhealey please help review the changes?

Copy link
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's hard to follow exactly what this is doing as I don't have any experience with electron.
Are we checking the signatures of the downloaded binaries? Where is that done?

onClickRemind: propTypes.func.isRequired,
onClickSkip: propTypes.func.isRequired,
*/
const appName = 'Storybook App';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this about?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for storybook previewing. It appears as application name as a part of dialog sentences. To white labeling the app (e.g. uchat), the app name is injected to the component as a prop.

You can see this name at http://localhost:9001 while running npm run storybook.

notifyOnly={false}
isDownloading={false}
progress={0}
onClickInstall={action('clicked install')}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why doesn't this match anything? Is it supposed to be click-install?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. On the actual app, click-install ipc message is sent from the updater dialog to the main process in order to invoke the function of electron-updater.

@hmhealey hmhealey removed their assignment Nov 13, 2018
@esethna
Copy link
Contributor

esethna commented Nov 14, 2018

@crspeller feedback above ^

@crspeller
Copy link
Member

@yuya-oc Looking for answer to my question so we can get stuff resolved and merged.

@wget
Copy link
Collaborator

wget commented Nov 15, 2018

Like discussed/agreed during the videoconf dev meeting yesterday:
(cf. slides: mattermost_msi_project_dev_meeting_presentation_2018-11-14.pdf)

On Windows:

The new msi installers are writing to Program Files which requires admin rights to write into. This auto updater feature is brought by electron and is running in user mode. Therefore, it hasn't rights to write in the Program Files folder.
For the 4.3.0 release happening soon: notify the user a new version is available but WITHOUT the "Install Update" button.
For the next iteration of the msi installer, provide a resident Windows service checking for new versions periodically and installing updates on the fly/silently (a bit like Mozilla Firefox or Google Chrome do).

On my side, the development of the Windows service will start soon.

On macOS:

Keep this auto updater feature like this WITH the "Install Update" button.

On GNU/Linux:

Like Windows: provide a notification, but WITHOUT offering the ability to update (GNU/Linux distribution have package managers and we really don't want to conflict with them).

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 15, 2018

@crspeller Sorry, I missed the question. Checksum and signature are automatically verified by electron-updater module when a binary has been downloaded. If the verification fails, the module emits an error event.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 15, 2018

Thanks @wget. Please let me confirm.

Windows

Should v4.2 app always use "Download Update" button for the dialog? The button opens a web page, https://mattermost.com/download/

download

macOS

Agree. In addition, recent brew cask package manager has a parameter to know whether the app has auto-updater. So they would not conflict.

Linux

Agree. However, currently there is no implementation for Linux because I had thought we need to finalize Windows/macOS solution first. Can we continue to discuss on another issue?

@crspeller
Copy link
Member

@yuya-oc I don't see our public key anywhere in this PR, how does the signature verification work?

@wget
Copy link
Collaborator

wget commented Nov 16, 2018

@yuya-oc

Windows

Should v4.2 app always use "Download Update" button for the dialog? The button opens a web page, https://mattermost.com/download/

If this is just a link to a web page. No issue.

macOS

Agree. In addition, recent brew cask package manager has a parameter to know whether the app has auto-updater. So they would not conflict.

Ok. Understood.

Linux

Agree. However, currently there is no implementation for Linux because I had thought we need to finalize Windows/macOS solution first. Can we continue to discuss on another issue?

Ok. Understood. GNU/Linux is indeed low priority now because its the only OS having excellent package managers. End users won't suffer from a missing auto updater (especially on Arch Linux, since the packages are well maintained <3).

@wget
Copy link
Collaborator

wget commented Nov 16, 2018

@yuya-oc I don't see our public key anywhere in this PR, how does the signature verification work?

Indeed this needs to get sorted out. This is a feature I need for msi installer as well. Please provide info how you are singing the applications. So that I can follow the same path for my msi.

This is what happens with non signed msi installers that have been downloaded from the web:

screenshot_20181105_092311

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 17, 2018

Yeah, “Download Update” button behaves like a link. The download page is opened in their default web browser.

@wget @crspeller In my understanding, signature verification is done by Windows itself. So what we need to do in ourselves is verify checksum. Codesining is done by our jenkins, https://build.mattermost.com. So the script to sign exe is included in the private repository (shared pipeline), not in this repository. The script gets private files from Mattermost’s Amazon S3 which stores certificates.

The script manually signs the code with osslsigncode. However electron-builder has a mechanism to do code-signing while building the app. We move to AppVeyor, so I believe we can use it.
https://www.electron.build/code-signing#windows

@wget
Copy link
Collaborator

wget commented Nov 17, 2018

@yuya-oc Thanks. Super. I will add these pieces of info together and will try to figure out to add the code signing in the AppVeyor pipeline.

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 19, 2018

Updated with "Download Update" (notifyOnly) configuration for Windows.

Copy link
Member

@crspeller crspeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok sounds good. Thanks.

@esethna esethna closed this Nov 19, 2018
@esethna esethna reopened this Nov 19, 2018
@esethna
Copy link
Contributor

esethna commented Nov 19, 2018

@yuya-oc can you please address the build issue when you can ^ Then I think we are good to merge this?

This should be the last PR before we cut the RC for 4.2?

@yuya-oc
Copy link
Contributor Author

yuya-oc commented Nov 20, 2018

@esethna Now the error is resolved, so I think this is ready to merge.

I'll make another PR. That's the final PR for v4.2: #841 (comment)

@esethna esethna modified the milestones: v4.2.0, v4.3.0 Nov 21, 2018
@yuya-oc
Copy link
Contributor Author

yuya-oc commented Dec 19, 2018

Merged the master branch. For now I merge this branch to suppress more conflicts. We can disable the feature when we need.

@yuya-oc yuya-oc merged commit 3505ce9 into mattermost:master Dec 19, 2018
@yuya-oc yuya-oc deleted the auto-updater branch December 19, 2018 14:53
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.

10 participants