Skip to content

Commit

Permalink
Don’t change group of app before moving it to the trash (sparkle-proj…
Browse files Browse the repository at this point in the history 
…ect#752).

Group doesn’t need to match in order to successfully empty the trash.
  • Loading branch information
@nriley
nriley committed Feb 26, 2016
1 parent 168c769 commit 1409a5a
Show file tree
Hide file tree
Showing 2 changed files with 58 additions and 17 deletions.
15 changes: 15 additions & 0 deletions Sparkle/SUFileManager.h
View file
Expand Up @@ -85,6 +85,21 @@
*/
- (BOOL)removeItemAtURL:(NSURL *)url error:(NSError **)error;

/**
* Changes the owner ID of an item at a specified target URL to match another URL
* @param targetURL A URL pointing to the target item whose owner ID to alter. This will be applied recursively if the item is a directory. The item at this URL must exist.
* @param matchURL A URL pointing to the item whose owner ID will be used for changing on the targetURL. The item at this URL must exist.
* @param error If an error occurs, upon returns contains an NSError object that describes the problem. If you are not interested in possible errors, you may pass in NULL.
* @return YES if the target item's owner ID has changed to match the origin's ones, otherwise NO along with a populated error object
*
* If the owner ID matches on the root items of targetURL and matchURL, this method stops and assumes that nothing needs to be done.
* Otherwise this method recursively changes the ID if the target is a directory. If an item in the directory is encountered that is unable to be changed,
* then this method stops and returns NO.
*
* This is not an atomic operation.
*/
- (BOOL)changeOwnerOfItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL *)matchURL error:(NSError **)error;

/**
* Changes the owner and group IDs of an item at a specified target URL to match another URL
* @param targetURL A URL pointing to the target item whose owner and group IDs to alter. This will be applied recursively if the item is a directory. The item at this URL must exist.
Expand Down
60 changes: 43 additions & 17 deletions Sparkle/SUFileManager.m
View file
Expand Up @@ -558,19 +558,19 @@ - (BOOL)_changeOwnerAndGroupOfItemAtURL:(NSURL *)targetURL ownerID:(NSNumber *)o
return YES;
}

- (BOOL)changeOwnerAndGroupOfItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL *)matchURL error:(NSError * __autoreleasing *)error
- (BOOL)changeOwnerAndGroup:(BOOL)changeGroup ofItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL *)matchURL error:(NSError * __autoreleasing *)error
{
BOOL isTargetADirectory = NO;
if (![self _itemExistsAtURL:targetURL isDirectory:&isTargetADirectory]) {
if (error != NULL) {
*error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileNoSuchFileError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Failed to change owner & group IDs because %@ does not exist.", targetURL.path.lastPathComponent] }];
*error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileNoSuchFileError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Failed to change %@ because %@ does not exist.", changeGroup ? @"owner & group IDs" : @"owner ID", targetURL.path.lastPathComponent] }];
}
return NO;
}

if (![self _itemExistsAtURL:matchURL]) {
if (error != NULL) {
*error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileNoSuchFileError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Failed to match owner & group IDs because %@ does not exist.", matchURL.path.lastPathComponent] }];
*error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileNoSuchFileError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Failed to match %@ because %@ does not exist.", changeGroup ? @"owner & group IDs" : @"owner ID", matchURL.path.lastPathComponent] }];
}
return NO;
}
Expand Down Expand Up @@ -604,22 +604,33 @@ - (BOOL)changeOwnerAndGroupOfItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL
return NO;
}

NSNumber *groupID = matchFileAttributes[NSFileGroupOwnerAccountID];
if (groupID == nil) {
// shouldn't be possible to error here, but just in case
if (error != NULL) {
*error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileReadNoPermissionError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Group ID could not be read from %@.", matchURL.path.lastPathComponent] }];
NSNumber *groupID;
if (changeGroup) {
groupID = matchFileAttributes[NSFileGroupOwnerAccountID];
if (groupID == nil) {
// shouldn't be possible to error here, but just in case
if (error != NULL) {
*error = [NSError errorWithDomain:NSCocoaErrorDomain code:NSFileReadNoPermissionError userInfo:@{ NSLocalizedDescriptionKey: [NSString stringWithFormat:@"Group ID could not be read from %@.", matchURL.path.lastPathComponent] }];
}
return NO;
}
return NO;
} else {
groupID = @-1;
}

NSNumber *targetOwnerID = targetFileAttributes[NSFileOwnerAccountID];
NSNumber *targetGroupID = targetFileAttributes[NSFileGroupOwnerAccountID];

if ((targetOwnerID != nil && [ownerID isEqualToNumber:targetOwnerID]) && (targetGroupID != nil && [groupID isEqualToNumber:targetGroupID])) {
// Assume they're the same even if we don't check every file recursively
// Speeds up the common case
return YES;
// Assume they're the same even if we don't check every file recursively
// Speeds up the common case
if (targetOwnerID != nil && [ownerID isEqualToNumber:targetOwnerID]) {
if (changeGroup) {
NSNumber *targetGroupID = targetFileAttributes[NSFileGroupOwnerAccountID];
if (targetGroupID != nil && [groupID isEqualToNumber:targetGroupID]) {
return YES;
}
} else {
return YES;
}
}

BOOL needsAuth = NO;
Expand Down Expand Up @@ -654,7 +665,12 @@ - (BOOL)changeOwnerAndGroupOfItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL
}

char userAndGroup[100];
int written = snprintf(userAndGroup, sizeof(userAndGroup), "%u:%u", ownerID.unsignedIntValue, groupID.unsignedIntValue);
int written;
if (changeGroup) {
written = snprintf(userAndGroup, sizeof(userAndGroup), "%u:%u", ownerID.unsignedIntValue, groupID.unsignedIntValue);
} else {
written = snprintf(userAndGroup, sizeof(userAndGroup), "%u", ownerID.unsignedIntValue);
}
if (written < 0 || written >= 100) {
return NO; // No custom error, because it's too unlikely to ever happen
}
Expand All @@ -665,13 +681,23 @@ - (BOOL)changeOwnerAndGroupOfItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL

BOOL success = AuthorizationExecuteWithPrivilegesAndWait(_auth, "/usr/sbin/chown", kAuthorizationFlagDefaults, (char *[]){ "-R", userAndGroup, targetPath, NULL });
if (!success && error != NULL) {
NSString *errorMessage = [NSString stringWithFormat:@"Failed to change owner:group %@ on %@ with authentication.", [NSString stringWithUTF8String:userAndGroup], targetURL.path.lastPathComponent];
NSString *errorMessage = [NSString stringWithFormat:@"Failed to change %@ %@ on %@ with authentication.", changeGroup ? @"owner:group" : @"owner", [NSString stringWithUTF8String:userAndGroup], targetURL.path.lastPathComponent];
*error = [NSError errorWithDomain:SUSparkleErrorDomain code:SUAuthenticationFailure userInfo:@{ NSLocalizedDescriptionKey: errorMessage }];
}

return success;
}

- (BOOL)changeOwnerOfItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL *)matchURL error:(NSError * __autoreleasing *)error
{
return [self changeOwnerAndGroup:NO ofItemAtRootURL:targetURL toMatchURL:matchURL error:error];
}

- (BOOL)changeOwnerAndGroupOfItemAtRootURL:(NSURL *)targetURL toMatchURL:(NSURL *)matchURL error:(NSError * __autoreleasing *)error
{
return [self changeOwnerAndGroup:YES ofItemAtRootURL:targetURL toMatchURL:matchURL error:error];
}

// /usr/bin/touch can be used to update an application, as described in:
// https://developer.apple.com/library/mac/documentation/Carbon/Conceptual/LaunchServicesConcepts/LSCConcepts/LSCConcepts.html
// The document says LSRegisterURL() can be used as well but this hasn't worked out well for me in practice
Expand Down Expand Up @@ -882,7 +908,7 @@ - (BOOL)moveItemAtURLToTrash:(NSURL *)url error:(NSError *__autoreleasing *)erro
return NO;
}

if (![self changeOwnerAndGroupOfItemAtRootURL:tempItemURL toMatchURL:trashURL error:error]) {
if (![self changeOwnerOfItemAtRootURL:tempItemURL toMatchURL:trashURL error:error]) {
// Removing the item inside of the temp directory is better than trying to move the item to the trash with incorrect ownership
[self removeItemAtURL:tempDirectory error:NULL];
return NO;
Expand Down

1 comment on commit 1409a5a

@nriley
Copy link
Owner Author

@nriley nriley commented on 1409a5a Feb 26, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ignore this commit, it doesn't solve the problem the right way.

Please sign in to comment.