Allow non-admin accounts to update QS in protected directories. Fixes #521 #1159

Merged
merged 8 commits into from Oct 29, 2012

Conversation

Projects
None yet
2 participants
@pjrobertson
Member

pjrobertson commented Oct 7, 2012

Finally :)
Only took me all weekend…!

Background info on Auth:

Apple is deprecating everything under the sun that allows apps to run in a 'privileged' environment. The currently recommended way is to use a helper app and communicate with this app using XPC or similar (NSMachPorts). See the SMJobBless example project for a little more info.

I chose to use Sparkle to deal with our authentication problems as I thought I'd be an easy solution, unfortunately, they are also using the deprecated methods (as of 10.7) for authentication. Looks like they haven't gone down the route of privileged helper apps (which require independent code signing… ugh!) yet.
I've decided to stick with it. The methods are deprecated, but hopefully if we still close to Sparkle this method won't die too soon

Background on Sparkle:

Since Sparkle is such a large framework, and we only really need one method, I have removed the submodule and added only the required (and butchered) files.
The files have been modified as little as possible, just enough so we don't have to #import the whole project. Hopefully they're still mostly intact that we can easily alter them as Sparkle alter their files.

So background aside, this pull means non admin users can download an update of QS to a protected directory, and be prompted with a dialogue asking for an admin password.
You may notice that in QSUpdateController the following steps are taken

  • Mount the .dmg in a temp folder
  • Copy the Quicksilver.app file in the .dmg to a temp folder in Application support/Quicksilver
  • Chown the Quicksilver.app file from step 2 to give it the right permissions
  • Copy the file from step 2 over the current install

The reason for step 2 is that the .dmg temp folder is read-only, so chown and move methods don't work on it (What Sparkle uses)


To test this:

  • Change the build number in Developer.xcconfig to say 3920
  • build and install in /Applications
  • Log in as a non admin user and run Quicksilver
  • Update from in the app

Sorry for the epic pull message, just… there's a lot to say, and a lot of reasoning behind the few changes!

pjrobertson added some commits Oct 7, 2012

Add the required (and modified) Sparkle files to the project
Modifications include:
* Remove unrequired #imports
* Replace SULog() with NSLog()
Use Sparkle's method for copying updates
* Tidy up and remove unused methods in NSApplication_BLTRExtensions
* Use the Sparkle method for copying with authentication as defined n SUPlainInstallerInternals
* Tidy up and remove unused methods from QSUpdateController
* Add extra checks to ensure the update is successful (no more silently dying)
* Add an extra step in the install/update process: copy the update to a writable location (App Support folder)
       * Required so that the permissions can correctly be set on the new update and 'moved' to the right location
Add a convenience method for creating app notifs (the method taking a…
…n NSDictionary is just too difficult to remember)
@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Oct 7, 2012

Member

P.S. you may need to rm -rf …/Sparkle before you attempt to merge this, sorry

Member

pjrobertson commented Oct 7, 2012

P.S. you may need to rm -rf …/Sparkle before you attempt to merge this, sorry

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Oct 9, 2012

Member

Just trying it under my normal account to start and I get "It was not possible to decompress downloaded file." It appears to download something, but I'm not sure where it's dumping the file, so I can't verify.

Also, probably outside the scope of this pull request, but nothing in QSUpdateController is localized. :-)

Member

skurfer commented Oct 9, 2012

Just trying it under my normal account to start and I get "It was not possible to decompress downloaded file." It appears to download something, but I'm not sure where it's dumping the file, so I can't verify.

Also, probably outside the scope of this pull request, but nothing in QSUpdateController is localized. :-)

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Oct 9, 2012

Member

I've only touched the - (NSString *)installAppFromDiskImage:(NSString *)path method, so I suggest adding a breakpoint there and seeing if it gets triggered/seeing what happens after this.

Member

pjrobertson commented Oct 9, 2012

I've only touched the - (NSString *)installAppFromDiskImage:(NSString *)path method, so I suggest adding a breakpoint there and seeing if it gets triggered/seeing what happens after this.

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Oct 9, 2012

Member

OK, updating from my admin account works. Trying with a non-admin user will have to wait until later.

Member

skurfer commented Oct 9, 2012

OK, updating from my admin account works. Trying with a non-admin user will have to wait until later.

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Oct 12, 2012

Member

I got a chance to test this and it seems to work as advertised. I am concerned about these two lines:

[manager removeItemAtPath:tempDirectory error:nil];
[manager removeItemAtPath:tempHoldDir error:nil];

Both of those paths are created using [NSString uniqueString]. It should never happen, but if for some reason that method returned nothing, those two lines would remove some things they shouldn't. Could we either test that they don't equal what should be their parent directories, or just hard-code some sensible value like @"QSApplicationUpdate"? Or maybe examine the error when creating the directory instead of passing nil? (We should probably be doing that in any case for something like this.)

Also, I see that setUpdateTimer returns tempHoldDir, but the path referred to by that string was just removed a few lines earlier, so what good does it do?

All in all nice, though. :-)

Member

skurfer commented Oct 12, 2012

I got a chance to test this and it seems to work as advertised. I am concerned about these two lines:

[manager removeItemAtPath:tempDirectory error:nil];
[manager removeItemAtPath:tempHoldDir error:nil];

Both of those paths are created using [NSString uniqueString]. It should never happen, but if for some reason that method returned nothing, those two lines would remove some things they shouldn't. Could we either test that they don't equal what should be their parent directories, or just hard-code some sensible value like @"QSApplicationUpdate"? Or maybe examine the error when creating the directory instead of passing nil? (We should probably be doing that in any case for something like this.)

Also, I see that setUpdateTimer returns tempHoldDir, but the path referred to by that string was just removed a few lines earlier, so what good does it do?

All in all nice, though. :-)

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Oct 25, 2012

Member

Good points, I've made the changes as you suggest.

Also, I see that setUpdateTimer returns tempHoldDir, but the path referred to by that string was just removed a few lines earlier, so what good does it do?

I assume you meant

-(NSString *)installAppFromDiskImage:(NSString *)path;

Returning anything signals to the calling method that it went OK. Probably having a BOOL is as simpler option, so I've done this

Member

pjrobertson commented Oct 25, 2012

Good points, I've made the changes as you suggest.

Also, I see that setUpdateTimer returns tempHoldDir, but the path referred to by that string was just removed a few lines earlier, so what good does it do?

I assume you meant

-(NSString *)installAppFromDiskImage:(NSString *)path;

Returning anything signals to the calling method that it went OK. Probably having a BOOL is as simpler option, so I've done this

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Oct 25, 2012

Member

I assume you meant

-(NSString *)installAppFromDiskImage:(NSString *)path;

Yeah, sorry. Xcode doesn't parse that file correctly for some reason, so it was showing the wrong method up top.

Code looks good, but enough has changed that I feel like I need to actually test the process again, so I'll try to do that later.

Member

skurfer commented Oct 25, 2012

I assume you meant

-(NSString *)installAppFromDiskImage:(NSString *)path;

Yeah, sorry. Xcode doesn't parse that file correctly for some reason, so it was showing the wrong method up top.

Code looks good, but enough has changed that I feel like I need to actually test the process again, so I'll try to do that later.

@pjrobertson

This comment has been minimized.

Show comment Hide comment
@pjrobertson

pjrobertson Oct 25, 2012

Member

Code looks good, but enough has changed that I feel like I need to
actually test the process again, so I'll try to do that later

I was going to, but my head started hurting so I went back to bed. I hoped
you would ;-)

Thanks

On 25 October 2012 13:21, Rob McBroom notifications@github.com wrote:

I assume you meant

-(NSString *)installAppFromDiskImage:(NSString *)path;

Yeah, sorry. Xcode doesn't parse that file correctly for some reason, so
it was showing the wrong method up top.

Code looks good, but enough has changed that I feel like I need to
actually test the process again, so I'll try to do that later.


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1159#issuecomment-9775383.

Member

pjrobertson commented Oct 25, 2012

Code looks good, but enough has changed that I feel like I need to
actually test the process again, so I'll try to do that later

I was going to, but my head started hurting so I went back to bed. I hoped
you would ;-)

Thanks

On 25 October 2012 13:21, Rob McBroom notifications@github.com wrote:

I assume you meant

-(NSString *)installAppFromDiskImage:(NSString *)path;

Yeah, sorry. Xcode doesn't parse that file correctly for some reason, so
it was showing the wrong method up top.

Code looks good, but enough has changed that I feel like I need to
actually test the process again, so I'll try to do that later.


Reply to this email directly or view it on GitHubhttps://github.com/quicksilver/Quicksilver/pull/1159#issuecomment-9775383.

@skurfer

This comment has been minimized.

Show comment Hide comment
@skurfer

skurfer Oct 25, 2012

Member

Anything to keep me away from these plug-ins. I hate dealing with compilers and headers and crap. :-)

Member

skurfer commented Oct 25, 2012

Anything to keep me away from these plug-ins. I hate dealing with compilers and headers and crap. :-)

skurfer added a commit that referenced this pull request Oct 29, 2012

Merge pull request #1159 from pjrobertson/adminRights
Allow non-admin accounts to update QS in protected directories. Fixes #521

@skurfer skurfer merged commit 765fa06 into quicksilver:master Oct 29, 2012

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