-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support Mac OS X Sandboxing #169
Conversation
I am going to go over this nearly-1000 line code review with a fine toothed comb, I tell you! |
Hey, at least I deleted 116 lines! |
Let me state for the record that this is a huge help for our Mac users 👍. At the very least, maybe I can try to build mixxx on my mac and try it out. |
Drag and drop works now. |
@@ -101,6 +102,11 @@ | |||
} | |||
|
|||
void BaseTrackPlayer::slotLoadTrack(TrackPointer track, bool bPlay) { | |||
// Before loading the track, ensure we have access: | |||
if (track && !Sandbox::askForAccess(track->getCanonicalLocation())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you also check if track is not NULL to avoid segfaults when calling getCanonicalLocation
. I get that it works because of lazy evaluation. Is this usually written like that?
Can't we have a small wrapper for askForAccess so that is accepts trackpointers. That would make it more readable the lazy evaluation is not super obvious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, this is a pretty common pattern of validating a pointer before dereferencing it. I think a helper would make sense if this was common but this is the only time we pass a trackpointer to askForAccess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe add a comment what happens here for C++ newbies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
Nice work rryan. I'm mostly nit-picking Do we need to inform users that they might be asked for file access on individual files that are not in the library folder when they upgrade to 1.12? Ideally you should collect all those as well on the first start and ask for file permissions Also your branch does not compile on linux
|
@@ -85,6 +86,18 @@ | |||
pConfig->getValueString(ConfigKey("[Library]","ShowTraktorLibrary"),"1").toInt()) { | |||
addFeature(new TraktorFeature(this, m_pTrackCollection)); | |||
} | |||
|
|||
// On startup we need to check if all of the user's library folders are | |||
// accessible to us. If the user is using a database from <1.12.0 with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't this go into src/upgrade.cpp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yea, it seemed like sane thing to do at boot every time though in case they somehow lost their sandbox.cfg file (or we corrupted it or something).
Ok, playlist import won't work if we don't have permission to the files in the playlist. My plan is to get the files you don't have access to, then calculate the largest common prefix from their file paths to determine what directory to ask for access to. This is such a bad situation. |
OS X 10.8.5 scons
./mixxx --controllerDebug --developer --resourcePath res Tracks do not load to deck, not by dnd from Mixxx library/Finder nor from the libraries context menu. Same with tracks from the iTunes library. Debug [Main]: createSecurityToken "/Users/user/Music/The Bug - London Zoo (2008)/01 Angry (feat. Tippa Irie).mp3" false
Warning [Main]: QSqlQuery::value: not positioned on a valid record
Debug [Main]: openSecurityToken QFileInfo "/Users/maxi/Music/The Bug - London Zoo (2008)/01 Angry (feat. Tippa Irie).mp3" true
Debug [Main]: New BeatGrid
Debug [Main]: Successfully deserialized BeatGrid
Debug [Main]: m_sTracks.count() = 1
Debug [Main]: Sandbox::askForAccess "/Users/user/Music/The Bug - London Zoo (2008)/01 Angry (feat. Tippa Irie).mp3" |
Oops -- I broke that today. Fixed. |
BTW -- to test the sandbox mode you have to bundle the app and sign it with Mixxx's developer private key (which only I and the build server have). Once the build server is back up I'll enable sandboxed builds for testing. In the meantime, if you test outside of a sandbox just check that things aren't broken (you know.. obscure use cases like loading tracks to a deck :P). |
If there are no objections I'd like to merge this and fix any issues in master going forward. It works pretty well for me -- no sandbox security warnings in Console.app. Playlist import is the only major remaining TODO. |
#ifndef SANDBOX_H | ||
#define SANDBOX_H | ||
|
||
#include <unistd.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this header is not included in msvs
I'll wait for #173 to prevent causing @loadstar81 any conflicts. |
To avoid possible misunderstandings: We wont be able to test any future changes on OSX once this got merged, unless we use a signed bundle from the build server? |
Yea, that's right. :-/ you can test that it didn't break anything for non-sandboxed usage. |
What are the impacts for future contributions? Also Capt. Obvious asks: When will the build server return, and how can we help to make it happen? |
Oh sorry -- I misunderstood you. This change only affects OS X builds that are sandboxed. Non-sandboxed builds will work as usual. So a developer who is working on changes unrelated to sandboxing can just build and run as usual. Even if you create a .app bundle that doesn't enable sandboxing unless you run scons with the codesign build action. To test a change related to sandboxing the binary has to be signed with the Mixxx Mac Developer private key. I can setup a build target for someone's branch so they can test but it will be annoying because they will have to wait for the build server to finish building each time. We may need to do another fundraiser to buy new hardware for the build server :(. |
Thanks for clarification.
Why not. Better now then never. |
Yea, true. We ran the numbers on SAAS options. Since compiling Mixxx is very CPU-heavy it will be costly to use a service like Amazon AWS. VPS or a similar option also exceeds our current amount of donations per month so that one isn't sustainable. |
…losing of security tokens.
* Clean up / unify processing of dropped URLs across all drop handlers. * Support dropping of playlists on the analysis, playlists, crate, autodj, and track table sections of the library (previously was only supported in playlists).
…on Support/Mixxx". Instead use QDesktopStorage::DataLocation.
…thod for dragging tracks.
On OS X 10.9, codesign order matters more. We have to codesign the frameworks and plugins first and then the binaries and bundle.
Merging -- will fix any issues directly in master. Thanks to all for reviewing. |
Netlify: Fix discourse comments blocked by CSP
This branch implements Apple's sandboxing and security scoped bookmarks. This fixes Bug #1257340 and should complete the Mac App Store blueprint.
I have a few more things to do but the code is in a pretty stable state so I'd appreciate some extra eyes or testers.
Summary:
Cases currently handled:
Migration:
Still TODO:
[1] https://developer.apple.com/library/mac/documentation/security/conceptual/AppSandboxDesignGuide/AboutAppSandbox/AboutAppSandbox.html#//apple_ref/doc/uid/TP40011183-CH1-SW1
[2] https://bugs.launchpad.net/mixxx/+bug/1257340
[3] https://blueprints.launchpad.net/mixxx/+spec/macappstore