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 login helper to manage login item in Mac App Store build #10856

Merged
merged 2 commits into from Nov 14, 2017

Conversation

Projects
None yet
7 participants
@dittos
Contributor

dittos commented Oct 20, 2017

Fixes #7312.

It creates Electron Login Helper.app and copies the helper app into Electron.app/Contents/Library/LoginItems/.
When setLoginItemSettings is called, SMLoginItemSetEnabled API is used to register the login item.
Login helper launches the main app (Electron.app/Contents/Library/LoginItems/Electron Login Helper.app/../../../.., which is Electron.app) and exits.

Note:

  • I did not tried to submit this to Mac App Store. I just tested it locally.
  • CFBundleIdentifier of the login helper should be changed to (main bundle id).loginhelper.
  • The login helper should be signed with parent entitlements as it is launched independently.
  • The app should be installed in /Applications to be tested. (Apple docs) Try to install generated installer package with sudo installer -store -pkg path-to-package.pkg -target /

@dittos dittos requested review from electron/docs as code owners Oct 20, 2017

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 20, 2017

/cc @malept Heads up if this goes in packager will need to update some more values 👍

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Oct 20, 2017

/cc @fab1an Who appears to know how this all works, would be interested what you think of the implementation?

@fab1an

Hi @MarshallOfSound, I'm not an expert on this, but here's what I know:

The login helper should be signed with parent entitlements as it is launched independently.

I think this is a typo. The login helper won't work with inherited entitlements, it just needs com.apple.security.app-sandbox.

The app should be installed in /Applications to be tested.

It says so in the apple docs, but I don't think that's actually necessary. The problem is just that launch service database can get confused if there's multiple applications with that id lying around somewhere on the filesystem.

<plist version="1.0">
<dict>
<key>CFBundleIdentifier</key>
<string>${ATOM_BUNDLE_ID}</string>

This comment has been minimized.

@fab1an

fab1an Oct 21, 2017

Contributor

This needs to be another id, separate of the main-app, that is also registered with apple.

This comment has been minimized.

@dittos

dittos Oct 21, 2017

Contributor

ATOM_BUNDLE_ID is com.github.electron.loginhelper. It should be changed anyway, but there is no good way to automatically customize it. (as prebuilt binaries are distributed unbranded - may go into packaging tools like electron-builder)

@@ -193,19 +194,27 @@
Browser::LoginItemSettings Browser::GetLoginItemSettings(
const LoginItemSettings& options) {
LoginItemSettings settings;
#if defined(MAS_BUILD)
settings.open_at_login = platform_util::GetLoginItemEnabled();
#else

This comment has been minimized.

@fab1an

fab1an Oct 21, 2017

Contributor

As far as I know there's no good way of deciding this. I just set the item and check whether the call returns true.

@@ -110,6 +110,8 @@ codesign -s "$APP_KEY" -f --entitlements "$CHILD_PLIST" "$FRAMEWORKS_PATH/$APP H
codesign -s "$APP_KEY" -f --entitlements "$CHILD_PLIST" "$FRAMEWORKS_PATH/$APP Helper EH.app/"
codesign -s "$APP_KEY" -f --entitlements "$CHILD_PLIST" "$FRAMEWORKS_PATH/$APP Helper NP.app/Contents/MacOS/$APP Helper NP"
codesign -s "$APP_KEY" -f --entitlements "$CHILD_PLIST" "$FRAMEWORKS_PATH/$APP Helper NP.app/"
codesign -s "$APP_KEY" -f --entitlements "$PARENT_PLIST" "$APP_PATH/Contents/Library/LoginItems/$APP Login Helper.app/Contents/MacOS/$APP Login Helper"
codesign -s "$APP_KEY" -f --entitlements "$PARENT_PLIST" "$APP_PATH/Contents/Library/LoginItems/$APP Login Helper.app/"

This comment has been minimized.

@fab1an

fab1an Oct 21, 2017

Contributor

The helper just needs sandbox, nothing else.

@fab1an

This comment has been minimized.

Contributor

fab1an commented Oct 23, 2017

The loginhelper entitlements file should be generated.

@holgersindbaek

This comment has been minimized.

holgersindbaek commented Nov 2, 2017

Cool... I've been waiting for this. Looking forward to seeing it implemented!

@holgersindbaek

This comment has been minimized.

holgersindbaek commented Nov 7, 2017

Any updates on this? What needs to be done before the pull request can be merged?

@zcbenz zcbenz self-assigned this Nov 14, 2017

@zcbenz

zcbenz approved these changes Nov 14, 2017

Looks good to me.

@zcbenz zcbenz merged commit 4b8ab8f into electron:master Nov 14, 2017

6 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-ia32 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-x64 Your tests passed on CircleCI!
Details
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ckerr

This comment has been minimized.

Member

ckerr commented Nov 16, 2017

This was discussed in the electron slack ci channel with the thinking being that merging #10856 caused a test failure on MAS builds, but in that PR MAS checks didn't run in CI and so the failure went unnoticed:

not ok 20 app module app.get/setLoginItemSettings API returns the login item status of the app
  AssertionError [ERR_ASSERTION]: { openAtLogin: [Getter/Setter],
    openAsHidden: [Getter/Setter],
    restoreState: [Getter/Setter],
    wasOpenedAtLogin: [Getter/Se deepEqual { openAtLogin: true,
    openAsHidden: true,
    wasOpenedAtLogin: false,
    wasOpenedAsHidden: false,
    restoreState: false }
      at Context.it (/Users/electron/workspace/electron_add_callbacks_spec-ATX6FQOFDHQ4ANJQILME444AS6DKONYDMVQFP6ZRFKZQUH3TGGTA/spec/api-app-spec.js:361:14)

I'd still like to land this feature, if @dittos or someone else wants to continue work on this and fix the regression

@bpasero

This comment has been minimized.

Contributor

bpasero commented Feb 25, 2018

@dittos @MarshallOfSound is it intentional that the login helper is also included in the non-MAS electron builds?

@MarshallOfSound

This comment has been minimized.

Member

MarshallOfSound commented Feb 25, 2018

@bpasero I don't believe so, there's actually a MAS flag check in the build files to ensure that is only built for the MAS target 🤔 With all these things you're finding in 2.0.x can you add them to the 2.0.x project board? I'll start churning through some of them this coming week 😄

@bpasero

This comment has been minimized.

Contributor

bpasero commented Feb 25, 2018

@MarshallOfSound ok, I have filed #12041 for that 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment