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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement App-Scoped Security scoped bookmarks #11711

Merged
merged 2 commits into from Feb 12, 2018

Conversation

Projects
None yet
6 participants
@acheronfail
Copy link
Contributor

acheronfail commented Jan 24, 2018

Reviewer edit: Fixes #10950


This PR implements app-scoped bookmarks for mas builds of electron.

Basically it allows electron applications that are signed and distributed via the Mac App Store to have access to files across restarts.

Before:

// Choose a file outside mas sandbox.
let filepath;
dialog.showOpenDialog(null, {}, (filepaths) => {
  // We can access the file since the user chose it with the native panel.
  filepath = filepaths[0];
  fs.readFileSync(filepath); // Works 馃帀
});

// ... restart app ...

// User can no longer access file since the app was restarted.
fs.readFileSync(filepath); // Error EPERM 馃憥

After:

let filepath;
let bookmark;
dialog.showOpenDialog(null, { securityScopedBookmarks: true }, (filepaths, bookmarks) => {
  // We can access the file since the user chose it with the native panel.
  filepath = filepaths[0];
  bookmark = bookmarks[0];
  fs.readFileSync(filepath); // Works 馃帀
});

// ... restart app ...

// User can use the bookmark data given to re-open the file outside the sandbox.
const stopAccessingSecurityScopedResource = app.startAccessingSecurityScopedResource(bookmark);
fs.readFileSync(filepath); // Works 馃帀
stopAccessingSecurityScopedResource();

This should close #4637.

Explanation

Note that in order to test this, one must build electron for mas and the app must be codesigned with the entitlements "com.apple.security.files.bookmarks.app-scope" and "com.apple.security.files.user-selected.read-write" or "com.apple.security.files.user-selected.read".

In order to access files outside the macOS sandbox, one must use the PowerBox API - which basically means one must use NSOpenPanel and/or NSSavePanel which are transparently swapped out with PowerBox calls when an application is sandboxed.

The way in which we can persist access to these files outside the sandbox is by using a special call on the NSURL instances returned by the PowerBox API (namely -bookmarkDataWithOptions). This will give us an NSData instance which is a special block of data (call it a "bookmark") that will allow us to access the file again when the app is re-sandboxed (after a restart).

After a restart, the application's sandbox is reset, and as such no longer has any access to files outside of the sandbox. The way in which we can use a bookmark, is to re-create an NSURL from it (via the -URLByResolvingBookmarkData initialiser) and use that NSURL to allow read/write access from the file.

Changes

This PR modifies electron's dialog API so that these bookmarks are returned as base64 encoded strings and the user may choose where to store them. It also provides a startAccessingSecurityScopedResource method on app which will create an NSData instance from the base64 encoded string, and then in turn use that to create an NSURL which is used to open the sandbox temporarily to that file.

After the user has read/written to the file, they must call the stopAccessingSecurityScopedResource function (which is returned from app.startAccessingSecurityScopedResource in order to close the bookmark which had temporarily opened the sandbox. If they continually fail to close opened bookmarks then macOS will enforce a stricter sandbox on the application until it has restarted (basically the app will no longer be able to use these bookmarks, etc).

Things to note:

  • To enable bookmarks, provide { securityScopedBookmarks: true } in the dialog options
  • The callback to showOpenDialog will now return both the filenames array and an added bookmarks array
    • The bookmarks array will be empty if the app is not bundled for mas
    • If there was an error obtaining the bookmark, an empty string will be returned ""
      • Errors may be because of signing issues, incorrect entitlements, etc
    • For showSaveDialog the filename is returned along with a bookmark string
  • In the case of dialog.showSaveDialog if the chosen file didn't exist then a blank one is created since a bookmark cannot be created for a non-existent file.

@acheronfail acheronfail requested review from as code owners Jan 24, 2018

@welcome

This comment has been minimized.

Copy link

welcome bot commented Jan 24, 2018

馃挅 Thanks for opening this pull request! 馃挅

Here is a list of things that will help get it across the finish line:

  • Follow the JavaScript, C++, and Python coding style.
  • Run npm run lint locally to catch formatting errors earlier.
  • Document any user-facing changes you've made following the documentation styleguide.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.
    We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can.
@@ -54,6 +54,9 @@ struct Converter<file_dialog::DialogSettings> {
dict.Get("filters", &(out->filters));
dict.Get("properties", &(out->properties));
dict.Get("showsTagField", &(out->shows_tag_field));
#if defined(OS_MACOSX) && defined(MAS_BUILD)

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

MAS_BUILD can be defined on MacOS only, so you don't need to check both.

@@ -103,6 +103,7 @@
'atom/app/node_main.h',
'atom/app/uv_task_runner.cc',
'atom/app/uv_task_runner.h',
'atom/browser/api/atom_api_app_mac.mm',

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

This file should be included only if mas_build==1, check the conditional includes at the very end of the file.


namespace api {

#if defined(OS_MACOSX) && defined(MAS_BUILD)

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

This check is redundant. The whole file should be excluded from the builds if it's not a MAS build.
Check the comment below how to do that.

else
open_callback_.Run(false, std::vector<base::FilePath>());
open_callback_.Run(false, std::vector<base::FilePath>(),
std::vector<std::string>());

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

All these look awful.
Since that bookmark stuff is Mac-specific OpenDialogCallback and SaveDialogCallback should probably be different on different platforms.

const wrappedCallback = typeof callback === 'function' ? function (success, result) {
return callback(success ? result : void 0)
const wrappedCallback = typeof callback === 'function' ? function (success, result, bookmarkData) {
return success ? callback(result, bookmarkData) : callback(void 0) // eslint-disable-line

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

Why do you need eslint-disable-line here?

This comment has been minimized.

@acheronfail

acheronfail Jan 30, 2018

Author Contributor

Both of them were giving me an error regarding the callback format - but removing void 0 has fixed it.

const wrappedCallback = typeof callback === 'function' ? function (success, result) {
return callback(success ? result : void 0)
const wrappedCallback = typeof callback === 'function' ? function (success, result, bookmarkData) {
return success ? callback(result, bookmarkData) : callback(void 0) // eslint-disable-line

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

There is absolutely no reasons to use void 0. Just call the callback without arguments.
(Which doesn't make much sense either, but since the API already looks like that it's ok.)

This comment has been minimized.

@acheronfail

acheronfail Jan 30, 2018

Author Contributor

Cool, I'm happy with that. I originally used void 0 since that was what was there before my changes.

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

I originally used void 0 since that was what was there before my changes.

Sure, I totally understand it. But let's leave the code better than we found it.

This comment has been minimized.

@acheronfail

acheronfail Jan 30, 2018

Author Contributor

馃憤

@@ -175,6 +181,12 @@ module.exports = {
throw new TypeError('Message must be a string')
}

if (securityScopedBookmarks == null) {

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

It can be true only in two cases. If securityScopedBookmarks is undefined, or it's null.

null is obviously an invalid value, you should throw an error in this case.
undefined means it is not provided, you can simply use default value in the destructuring assignment:

let {securityScopedBookmarks = false} = options
}

const wrappedCallback = typeof callback === 'function' ? function (success, result, bookmarkData) {
return success ? callback(result, bookmarkData) : callback(void 0) // eslint-disable-line

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

// eslint-disable-line

Why do you need it?

@@ -260,7 +262,7 @@ void ShowSaveDialog(const DialogSettings& settings,
const SaveDialogCallback& callback) {
RunState run_state;
if (!CreateDialogThread(&run_state)) {
callback.Run(false, base::FilePath());
callback.Run(false, base::FilePath(), "");

This comment has been minimized.

@alexeykuzmin

alexeykuzmin Jan 30, 2018

Contributor

Mac-specific stuff shouldn't affect other platforms.

bool result,
const base::FilePath& path)> SaveDialogCallback;
#endif

This comment has been minimized.

@acheronfail

acheronfail Jan 30, 2018

Author Contributor

@alexeykuzmin Not sure if this is the best way to go about changing OpenDialogCallback and SaveDialogCallback?
If you have any other suggestions let me know.

This comment has been minimized.

@acheronfail

acheronfail Jan 30, 2018

Author Contributor

Hmmm if we conditionally change this here on MAS_BUILDs, then lines 56 and 77 atom/browser/web_dialog_helper.cc also need to reflect that (since they use the same call signature - hence why the builds are currently failing).

All done, now it's only available on mas builds and nothing else 馃帀

@codebytere codebytere merged commit d1d50a4 into electron:master Feb 12, 2018

8 checks passed

ci/circleci: electron-linux-arm Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm-test Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64 Your tests passed on CircleCI!
Details
ci/circleci: electron-linux-arm64-test 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/pr AppVeyor build succeeded
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
@welcome

This comment has been minimized.

Copy link

welcome bot commented Feb 12, 2018

Congrats on merging your first pull request! 馃帀馃帀馃帀

sethlu added a commit to sethlu/electron that referenced this pull request May 3, 2018

Implement App-Scoped Security scoped bookmarks (electron#11711)
* implementation of security scoped bookmarks

* option is now only available on mas builds
@hezhuojie

This comment has been minimized.

Copy link

hezhuojie commented Jul 4, 2018

@acheronfail Hi! Thanks bookmark feature from your pull request.
Now I face an issue that bookmark data should also be produced while user drop file to dock.
Do you have any idea? Thanks!

@acheronfail

This comment has been minimized.

Copy link
Contributor Author

acheronfail commented Jul 4, 2018

@hezhuojie you'll have to add that functionality into electron yourself, a quick glance over the repo brings to light these paths that you might be able to use:

Bookmarks for the Dock

Bookmarks for the Tray

A similar path can be seen for the macOS Tray API as well (dragging and dropping files onto the menu bar icon).

Be sure to look at the changes made in this PR for details on how to extract the bookmark data and convert it to base64, etc.

@rr326

This comment has been minimized.

Copy link

rr326 commented Oct 16, 2018

@acheronfail - Not sure if this is the best place for this, but I'm wondering if you have any tips on how to debug this. I followed all the steps you outlined (including building for mas (mas-dev) via electron-builder), and I'm getting [] for my bookmarks, suggesting it is not built for mas. (Yet I see ebuilder is using electron-v3.0.4-mas-x64.zip. So it looks like it isn't an error, but somehow thinks it isn't mas.

@acheronfail

This comment has been minimized.

Copy link
Contributor Author

acheronfail commented Oct 17, 2018

@rr326 this is something that can indeed be very difficult to debug.

But, most of the time the issues I had were:

  1. is it a mas build?
  2. has it been correctly codesigned?

Looks like you're already checking for 1., so I would recommend that you verify if the code signature is correct, signed by a valid and active certificate, and covers all binaries loaded and used from electron. For more information about codesigning electron applications I'd look at electron-osx-sign and electron's documentation.

It can be tricky, but this should work just fine if you have the correct build and your application is correctly signed.

@rr326

This comment has been minimized.

Copy link

rr326 commented Oct 23, 2018

@acheronfail I've been struggling with this for a while and would love some insight. I've tried a ton, including trying to manually select the mas build (no difference) and rebuild electron mas myself (failed).

First, are sure that it will return [] if it is not a mas build? I think not. When I intentionally build for non-mas, I get undefined returned. And if you look at file_dialog_mac.mm the code path is consistent with getting undefined from non-mas. The only way to get [] as far as I can tell is to have a mas build AND have { securityScopedBookmarks: false }.

If that's the case, then I AM using a mas build AND may be codesigning properly (which I've tried to verify other ways) and the problem is somehow even though I set { securityScopedBookmarks: true } it is somehow getting ignored / changed.

That said, I can't see how that would happen. I can't find anything that might cause that. I'd appreciate any tips - I'm at wits end.

And here is my code, btw:

dialog.showOpenDialog({
  title: 'Select directory to store your FileSimple data and files',
  defaultPath: app.getPath('home'),
  buttonLabel: 'Select',
  properties: ["openDirectory", "createDirectory"],
  securityScopedBookmarks: true,
}, (filePaths, bookmarks) => { ... })
@shaoju

This comment has been minimized.

Copy link

shaoju commented Nov 13, 2018

@rr326 did you solve your problem eventually? I am having exactly the same issue as you described (MAS build + correctly signed, but it returns [] all the time).
I have no clue on how to make it work. Is there anything else worth trying ?

@rr326

This comment has been minimized.

Copy link

rr326 commented Nov 13, 2018

I'm pretty sure this is a bug in electron. Unfortunately I haven't been able to get a custom electron build to work properly, so I'm currently waiting for 4.0.0-beta.8 to release so I can test it, confirm, and file a bug. Hopefully in just a couple of days.

I'm glad I'm not the only one!

@rr326

This comment has been minimized.

Copy link

rr326 commented Dec 10, 2018

@shaoju I finally figured out my problem. Yes - you can be confident that security scoped bookmarks DO work in 4.0 and 3.x. If it is not working, there is a problem with your configuration.

I built a small test app to help me debug it here. I'd recommend cloning that and getting it to work on your system, and then figuring out how your real app differs from the test app. Good luck! (And feel free to ping me at my profile if you are stuck. It's a pain!)

@shaoju

This comment has been minimized.

Copy link

shaoju commented Dec 14, 2018

This is Cooooool!! Thanks @rr326 !
I had to build a workaround which wasn't really user-friendly. I'll try it again as soon as possible.

@shaoju

This comment has been minimized.

Copy link

shaoju commented Dec 18, 2018

@rr326 Finally made it work with electron 4.0! However it seems to only working when the show/open dialog is modaless. If I make it modal by adding a parent window to it, then the bookmark will return empty again.
This is not a blocker but still not perfect (user can still interact with the main window). Any chance that you may also come across this and have found a solution?

@rr326

This comment has been minimized.

Copy link

rr326 commented Dec 18, 2018

That sounds super weird. I have no idea. I can tell you my problem was doing something really non-standard, that worked fine non-mas, but apparently messed up MAS. And I had totally forgotten I was doing it. So the only tip I can give is trace the chain of action a bit and see if this modal/non-modal might be exposing something else upstream. Good luck.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.