feat: Add support for macOS app API key notarization

[JohnStarich]

• I have read the contribution documentation for this project.
• I agree to follow the code of conduct that this project follows, as appropriate.
• The changes are appropriately documented (if applicable).
• The changes have sufficient test coverage (if applicable).
• 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, 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]

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:

• Follow the JavaScript coding style.
• 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 Report

Merging #1127 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #1127   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          14      14           
  Lines         703     698    -5     
======================================
- Hits          703     698    -5

Impacted Files	Coverage Δ
src/mac.js	100% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 366df35...8d6702d. Read the comment docs.

[malept]

I appreciate the work you've done here! To answer your questions:

Should this validation be present in electron-packager? Should it instead be deferred to the notarization library?

I think you're right, all of the validations should go into electron-notarize rather than be in Electron Packager.

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.

Currently, Packager outputs a warning and moves on if the bundled app cannot be signed. It seems reasonable to do something similar for notarizing.

CC: @MarshallOfSound, if you have differing opinions

[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 ^^): electron/notarize#35

Also ran the following diff + test successfully with the PR's version of notarize:

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 plist = require('plist')
-const { notarize } = require('electron-notarize')
+const { validateAuthorizationArgs, notarize } = require('electron-notarize')
 const { signAsync } = require('electron-osx-sign')
@@ -396,6 +396,13 @@
 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
+  }

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

[malept]

I'll merge this once I commit the suggestions below.

Thanks for your patience!

[welcome]

Thanks for your contribution! 🎉

[JohnStarich]

Excellent, thank you! 🚀