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: implement signApp function for macPackager #3912

Merged
merged 2 commits into from
May 25, 2019

Conversation

Kilian
Copy link
Contributor

@Kilian Kilian commented May 21, 2019

As per #3504, afterSign was called before the sign function was called on mac. I tracked this down to a logic error where signApp wasn't implemented for macPackager and thus immediately resolved. Mac then called the sign function directly, leading to the incorrect behaviour.

I was able to successfully build with this, and have the afterSign function called at the right time, with the changes below. I'm however not familiar with typescript (or electron-builders code standards) so likely I'll need to make some changes. This can serve as a first setup.

Kilian added 2 commits May 21, 2019 14:44
afterSign hook now is called after sign instead of before sign on Mac.
@martani
Copy link

martani commented May 23, 2019

I'm interested in testing this, how can I fork a local version of electron builder and reference in from my app?

It doesn't seem to work when I simply reference the package this way in package.json:

"devDependencies": {
    "electron-builder": "file:../electron-builder/packages/electron-builder"
}

@Kilian Kilian mentioned this pull request May 23, 2019
@delusive
Copy link

I tried this and I am still receiving "Unnotarized Developer ID" from this line https://github.com/electron-userland/electron-builder/pull/3912/files#diff-2f245a25ceda557ecf2530ec7e88c805R315, before it even runs the afterSign hook. Maybe I am doing something wrong here.

@delusive
Copy link

I tried this and I am still receiving "Unnotarized Developer ID" from this line https://github.com/electron-userland/electron-builder/pull/3912/files#diff-2f245a25ceda557ecf2530ec7e88c805R315, before it even runs the afterSign hook. Maybe I am doing something wrong here.

I believe this might be due to the "hardenedRuntime": true functionality here: #3858

@martani
Copy link

martani commented May 24, 2019

@delusive how were you able to test this? I can notarize my app manually but need to test doing it in the afterSign step.

@delusive
Copy link

delusive commented May 25, 2019

@delusive how were you able to test this? I can notarize my app manually but need to test doing it in the afterSign step.

I cloned Kilian:patch-1, and then compiled the code, I then replaced packages/app-builder-lib/src/macPackager.ts in my local node_modules with the new one. There is probably a better way, but for some reason I could yarn add the package from my local directory where I had it compiled, it wouldn't build the node_modules/.bin/electron-builder in my case. I was probably doing something wrong.

@zaherg
Copy link

zaherg commented May 25, 2019

I tried this and I am still receiving "Unnotarized Developer ID" from this line https://github.com/electron-userland/electron-builder/pull/3912/files#diff-2f245a25ceda557ecf2530ec7e88c805R315, before it even runs the afterSign hook. Maybe I am doing something wrong here.

I believe this might be due to the "hardenedRuntime": true functionality here: #3858

@delusive You will need to use the pre-released version of electron-builder which is version 20.41.0 and add the "hardenedRuntime": true, option to your mac build section.
let me know if it will work with you, am stuck with the rejection all the time now.

Thanks

@develar
Copy link
Member

develar commented May 25, 2019

Explanation: sign is removed from the promise chain after doPack, but signApp is called as part of doPack.

@develar develar merged commit 99ac3d4 into electron-userland:master May 25, 2019
@develar
Copy link
Member

develar commented May 25, 2019

Our code sign tests not executed locally due to security reasons, buy I set CSC_KEY_PASSWORD and verify it manually in addition to CI tests on Travis.

Thank you! I will prepare release today.

@zaherg
Copy link

zaherg commented May 25, 2019

Hey @Kilian may I ask you to show your afterSign script? for some reason now that am using the prerelease version of electron-builder with your fix, I can't any result from the script, always get :

/Users/zaher/Sites/electron/my app.app: rejected
source=Unnotarized Developer ID

@delusive
Copy link

I built the newest version from master and installed from the local filesystem into my project. I have added "afterSign": "./after_sign_hook.js" in the build section of package.json along with "hardenedRuntime": true under the mac build section of package.json. My after_sign hook contains the following code:

const enotarize = require("electron-notarize");

exports.default = async function(context) {
    options = {
        appBundleId: "***",
        appPath: context.appOutDir,
        appleId: "********",
        appleIdPassword: "******"        
    };

    console.log("Notarizing application: " + options)

    await enotarize.notarize(options);

};

When I run electron-builder I get the Unnotarized Developer ID error. The interesting thing is that I don't get my output message "Notarizing application: " in the output, meaning it hasn't tried to notorize the application before returning the error.

Any help/example would be greatly appreciated.

Thanks.

@Kilian
Copy link
Contributor Author

Kilian commented May 26, 2019

@delusive your app path needs the actual app in addition to the directory! so something like

AppPath: `${context.appOutDir}/myApp.app`

@martani
Copy link

martani commented May 26, 2019

This is the afterSign hook I use for notarization. It works well with the .app output. (Side note: it also appears that notarizing the dmg also notarizes the .app inside.)

const fs = require('fs');
const path = require('path');
var electron_notarize = require('electron-notarize');

module.exports = async function (params) {
    // Only notarize the app on Mac OS only.
    if (process.platform !== 'darwin') {
        return;
    }
    console.log('afterSign hook triggered', params);

    // Same appId in electron-builder.
    let appId = '<Change this to your appId ad defined in your electron-builder config>'

    let appPath = path.join(params.appOutDir, `${params.packager.appInfo.productFilename}.app`);
    if (!fs.existsSync(appPath)) {
        throw new Error(`Cannot find application at: ${appPath}`);
    }

    console.log(`Notarizing ${appId} found at ${appPath}`);

    try {
        await electron_notarize.notarize({
            appBundleId: appId,
            appPath: appPath,
            appleId: process.env.appleId,
            appleIdPassword: process.env.appleIdPassword,
        });
    } catch (error) {
        console.error(error);
    }

    console.log(`Done notarizing ${appId}`);
};

@Kilian
Copy link
Contributor Author

Kilian commented May 26, 2019

@martani it does, but I don't think it staples the .app, which means it can't be verified offline. So notarizing the app and stapling it before packaging it into the dmg (and then notarizing that) is probably best.

@martani
Copy link

martani commented May 26, 2019

Thanks @Kilian, that's what I ended up doing. This adds a lot of latency to the build process, notarizing the app takes ~10 minutes + an extra ~10 minutes for the dmg.

Unfortunately, that does not seem to fully solve the issue of users not able to open the dmg on macOS 10.14.5 as I reported here: #3828 (comment)

@DuBistKomisch
Copy link

I think this has broken our use case of passing electron-builder a pre-built but unsigned .app and turning it into a .pkg, signing both the .app and .pkg. It no longer signs the .app at all now. I guess this is a bit of a dumb use case anyway, I can just run electron-osx-sign and electron-notarize first instead, but just mentioning anyway.

@damianobarbati
Copy link

@martani @delusive where did you find the appleId and applePassword?

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

Successfully merging this pull request may close these issues.

8 participants