Repository: electron/electron-packager
Pull request #1127 (2020-02-20 02:59:25)

feat: Add support for macOS app API key notarization
* [x] I have read the [contribution documentation](https://github.com/electron/electron-packager/blob/master/CONTRIBUTING.md) for this project.
* [x] I agree to follow the [code of conduct](https://github.com/electron/electron/blob/master/CODE_OF_CONDUCT.md) that this project follows, as appropriate.
* [x] The changes are appropriately documented (if applicable).
* [x] The changes have sufficient test coverage (if applicable).
* [x] The testsuite passes successfully on my local machine (if applicable).

**Summarize your changes:**
Hi all, thanks for this wonderful tool. It's saved me countless hours in developing my app 👏 

This PR adds support for macOS app [API key notarization](https://github.com/electron/electron-notarize/pull/18), which is pretty nice when building a pipeline in open source code. This way I can limit the API key's permissions and completely revoke it if compromised.

The existing notarization validation failed when providing an App Store Connect API key and issuer in place of the expected Apple ID and password combo. This PR loosens it up to accept either pair of credentials.

I came up with a couple questions while writing this up:
* Should this validation be present in electron-packager? Should it instead be deferred to the notarization library?
* Should the failure mode only produce warnings and continue the build without notarization? This seemed odd to me, but perhaps that's normal behavior for electron-packager.

welcome[bot]: Thanks for opening a pull request!

Here are some highlighted action items that will help get it across the finish line, from the
[pull request guidelines](https://github.com/electron/electron-packager/blob/master/CONTRIBUTING.md#filing-pull-requests):
- Follow the [JavaScript coding style](https://standardjs.com/).
- Run `npm run lint` locally to catch formatting errors earlier.
- Document any user-facing changes in `NEWS.md` and other docs.
- Include tests when adding/changing behavior.

Development and triage is community-driven, so please be patient and we will get back to you as soon as we can.

codecov-io: # [Codecov](https://codecov.io/gh/electron/electron-packager/pull/1127?src=pr&el=h1) Report
> Merging [#1127](https://codecov.io/gh/electron/electron-packager/pull/1127?src=pr&el=desc) into [master](https://codecov.io/gh/electron/electron-packager/commit/c994b5765b6b522ff2db7c3915de73738cfc621a?src=pr&el=desc) will **decrease** coverage by `1.1%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/electron/electron-packager/pull/1127/graphs/tree.svg?width=650&token=sfG3Wa1sru&height=150&src=pr)](https://codecov.io/gh/electron/electron-packager/pull/1127?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1127      +/-   ##
==========================================
- Coverage     100%   98.89%   -1.11%     
==========================================
  Files          14       14              
  Lines         712      726      +14     
==========================================
+ Hits          712      718       +6     
- Misses          0        8       +8
```


| [Impacted Files](https://codecov.io/gh/electron/electron-packager/pull/1127?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [src/mac.js](https://codecov.io/gh/electron/electron-packager/pull/1127/diff?src=pr&el=tree#diff-c3JjL21hYy5qcw==) | `97.19% <100%> (-2.81%)` | :arrow_down: |
| [src/unzip.js](https://codecov.io/gh/electron/electron-packager/pull/1127/diff?src=pr&el=tree#diff-c3JjL3VuemlwLmpz) | `83.33% <0%> (-16.67%)` | :arrow_down: |
| [src/ignore.js](https://codecov.io/gh/electron/electron-packager/pull/1127/diff?src=pr&el=tree#diff-c3JjL2lnbm9yZS5qcw==) | `98% <0%> (-2%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/electron/electron-packager/pull/1127?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/electron/electron-packager/pull/1127?src=pr&el=footer). Last update [c994b57...6b41b29](https://codecov.io/gh/electron/electron-packager/pull/1127?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

JohnStarich: @malept Thanks for the quick review! I've removed the validation in 8569536, if that's the way we want to go.
After reading through electron-notarize's source, I'm not sure it performs strict validation there today:
https://github.com/electron/electron-notarize/blob/ce868f89895f20bafc3108f1eb0653dc09dcd268/src/index.ts#L40-L49

Perhaps it would be valuable to copy validation from 6b41b29 into Notarize in a follow-up PR?
JohnStarich: Hey @malept, just checking in! Are we waiting on input from @MarshallOfSound?
malept: I think the way forward is:

1. Create a pull request for `electron-notarize` to move the validations from here to there.
2. Get a new release of `electron-notarize`.
3. Update this pull request to update the `electron-notarize` dependency.
JohnStarich: I think the reason validation doesn't exist in Notarize is because it's doing simple translation to CLI args, which would fail only when `notarize` is run (during the actual notarization step).

I'm guessing these validation steps were in Packager to catch failures _before_ running Notarize, since Notarize doesn't have a validation step. I'll investigate adding such a step to Notarize. if you have any suggestions for how that should look, please let me know! Time to learn some TypeScript 😉 
JohnStarich: Ok, kicked off a PR on notarize (sorry for all the noise ^^): https://github.com/electron/electron-notarize/pull/35

Also ran the following diff + test successfully with the PR's version of notarize:

```diff
diff --git a/src/mac.js b/src/mac.js
index e9e6575..9329c73 100644
--- a/src/mac.js
+++ b/src/mac.js
@@ -6,7 +6,7 @@ const debug = require('debug')('electron-packager')
 const fs = require('fs-extra')
 const path = require('path')
 const plist = require('plist')
-const { notarize } = require('electron-notarize')
+const { validateAuthorizationArgs, notarize } = require('electron-notarize')
 const { signAsync } = require('electron-osx-sign')
 
 class MacApp extends App {
@@ -396,6 +396,13 @@ function createSignOpts (properties, platform, app, version, notarize, quiet) {
 function createNotarizeOpts (properties, appBundleId, appPath, quiet) {
   const notarizeOpts = properties
 
+  try {
+    validateAuthorizationArgs(notarizeOpts)
+  } catch (e) {
+    common.warning(`Failed validation, notarize will not run: ${e.message}`)
+    return
+  }
+
   // osxNotarize options are handed off to the electron-notarize module, but with a few
   // additions from the main options. The user may think they can pass bundle ID or appPath,
   // but they will be ignored.
diff --git a/test/darwin.js b/test/darwin.js
index afa5db1..51446d8 100644
--- a/test/darwin.js
+++ b/test/darwin.js
@@ -262,6 +262,13 @@ if (!(process.env.CI && process.platform === 'win32')) {
     t.deepEqual(obj.CFBundleURLTypes, expected, 'CFBundleURLTypes did not contain specified protocol schemes and names')
   }))
 
+  test('osxNotarize: missing appleIdPassword', t => {
+    util.setupConsoleWarnSpy()
+    const notarizeOpts = mac.createNotarizeOpts({ appleId: '' })
+    t.falsy(notarizeOpts, 'does not generate options')
+    util.assertWarning(t, 'WARNING: Failed validation, notarize will not run: The appleId property is required when using notarization with appleIdPassword')
+  })
+
   test('osxNotarize: appBundleId not overwritten', t => {
     const args = { appleId: '1', appleIdPassword: '2', appBundleId: 'no' }
     const notarizeOpts = mac.createNotarizeOpts(args, 'yes', 'appPath', true)
```
malept: I've updated the `electron-notarize` dependency on master. Additionally, in #1131 I moved the API docs from a Markdown file to a TypeScript definition file, so you'll have to fix those merge conflicts.
JohnStarich: Thanks for the approval / merge!

Ok, fixed! It seems the bullet points under authentication options were removed in the migration to the new format. I've added them back in my commit ^ but I can definitely remove them if we only want to direct folks to notarize's doc instead 👍  
malept: > It seems the bullet points under authentication options were removed in the migration to the new format. I've added them back in my commit ^ but I can definitely remove them if we only want to direct folks to notarize's doc instead 👍

I think I'd rather direct folks to the `electron-notarize` docs.
JohnStarich: Roger that, updated

Patch:
diff --git a/src/index.d.ts b/src/index.d.ts
index 2561f87..de98e64 100644
--- a/src/index.d.ts
+++ b/src/index.d.ts
@@ -397,9 +397,9 @@ declare namespace electronPackager {
      */
     name?: string;
     /**
-     * If present, notarizes macOS target apps when the host platform is macOS and XCode is
-     * installed. See [`electron-notarize`](https://github.com/electron/electron-notarize#method-notarizeopts-promisevoid)
-     * for sub-option descriptions and how to use the `appleIdPassword` sub-option safely.
+     * If present, notarizes macOS target apps when the host platform is macOS and XCode is installed.
+     * See [`electron-notarize`](https://github.com/electron/electron-notarize#method-notarizeopts-promisevoid)
+     * for option descriptions, such as how to use `appleIdPassword` safely or obtain an API key.
      *
      * **Requires the [[osxSign]] option to be set.**
      *
diff --git a/src/mac.js b/src/mac.js
index 9214ff1..2571032 100644
--- a/src/mac.js
+++ b/src/mac.js
@@ -6,7 +6,7 @@ const debug = require('debug')('electron-packager')
 const fs = require('fs-extra')
 const path = require('path')
 const plist = require('plist')
-const { notarize } = require('electron-notarize')
+const { notarize, validateAuthorizationArgs } = require('electron-notarize')
 const { signAsync } = require('electron-osx-sign')
 
 class MacApp extends App {
@@ -394,28 +394,19 @@ function createSignOpts (properties, platform, app, version, notarize, quiet) {
 }
 
 function createNotarizeOpts (properties, appBundleId, appPath, quiet) {
-  const notarizeOpts = properties
-  let notarize = true
-
-  if (!notarizeOpts.appleId) {
-    common.warning('The appleId sub-property is required when using notarization, notarize will not run')
-    notarize = false
-  }
-
-  if (!notarizeOpts.appleIdPassword) {
-    common.warning('The appleIdPassword sub-property is required when using notarization, notarize will not run')
-    notarize = false
-  }
-
-  if (notarize) {
-    // osxNotarize options are handed off to the electron-notarize module, but with a few
-    // additions from the main options. The user may think they can pass bundle ID or appPath,
-    // but they will be ignored.
-    common.subOptionWarning(notarizeOpts, 'osxNotarize', 'appBundleId', appBundleId, quiet)
-    common.subOptionWarning(notarizeOpts, 'osxNotarize', 'appPath', appPath, quiet)
-
-    return notarizeOpts
-  }
+  try {
+    validateAuthorizationArgs(properties)
+  } catch (e) {
+    common.warning(`Failed validation, notarization will not run: ${e.message}`)
+    return
+  }
+
+  // osxNotarize options are handed off to the electron-notarize module, but with a few
+  // additions from the main options. The user may think they can pass bundle ID or appPath,
+  // but they will be ignored.
+  common.subOptionWarning(properties, 'osxNotarize', 'appBundleId', appBundleId, quiet)
+  common.subOptionWarning(properties, 'osxNotarize', 'appPath', appPath, quiet)
+  return properties
 }
 
 module.exports = {
diff --git a/usage.txt b/usage.txt
index 442a23b..9a71444 100644
--- a/usage.txt
+++ b/usage.txt
@@ -95,6 +95,8 @@ osx-notarize       (macOS host platform only, requires --osx-sign) Whether to no
                    Properties supported include:
                    - appleId: should contain your apple ID username / email
                    - appleIdPassword: should contain the password for the provided apple ID
+                   - appleApiKey: should contain an App Store Connect API key
+                   - appleApiIssuer: should contain the API key's issuer
 protocol           URL protocol scheme to register the app as an opener of.
                    For example, `--protocol=myapp` would register the app to open
                    URLs such as `myapp://path`. This argument requires a `--protocol-name`


Test patch:
diff --git a/test/darwin.js b/test/darwin.js
index a32edd0..659fb0b 100644
--- a/test/darwin.js
+++ b/test/darwin.js
@@ -262,18 +262,11 @@ if (!(process.env.CI && process.platform === 'win32')) {
     t.deepEqual(obj.CFBundleURLTypes, expected, 'CFBundleURLTypes did not contain specified protocol schemes and names')
   }))
 
-  test('osxNotarize: missing appleId', t => {
-    util.setupConsoleWarnSpy()
-    const notarizeOpts = mac.createNotarizeOpts({ appleIdPassword: '' })
-    t.falsy(notarizeOpts, 'does not generate options')
-    util.assertWarning(t, 'WARNING: The appleId sub-property is required when using notarization, notarize will not run')
-  })
-
   test('osxNotarize: missing appleIdPassword', t => {
     util.setupConsoleWarnSpy()
     const notarizeOpts = mac.createNotarizeOpts({ appleId: '' })
     t.falsy(notarizeOpts, 'does not generate options')
-    util.assertWarning(t, 'WARNING: The appleIdPassword sub-property is required when using notarization, notarize will not run')
+    util.assertWarning(t, 'WARNING: Failed validation, notarization will not run: The appleId property is required when using notarization with appleIdPassword')
   })
 
   test('osxNotarize: appBundleId not overwritten', t => {