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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sparkle requesting authentication when it doesn't need it #752

Closed
nriley opened this issue Feb 16, 2016 · 6 comments · Fixed by #790
Closed

Sparkle requesting authentication when it doesn't need it #752

nriley opened this issue Feb 16, 2016 · 6 comments · Fixed by #790

Comments

@nriley
Copy link
Contributor

nriley commented Feb 16, 2016

I just updated an app to Sparkle 1.13.1. Now, when I try to update it, it prompts me for my password even though it's installed in a writable directory. This prompt seems to happen after the update is installed.
sparkle authentication

The corresponding log entries appear to be:

2/15/16 7:46:00.800 PM Autoupdate[33661]: Sparkle: ===== Autoupdate =====
2/15/16 7:46:00.801 PM Autoupdate[33661]: Sparkle: Failed to move file:///private/var/folders/68/3m4rmkdj5hqcnlnpqvrp8_hw0000gn/T/TemporaryItems/(A%20Document%20Being%20Saved%20By%20Autoupdate%202)/Shroud%20(10).app to trash with error Error Domain=SUSparkleErrorDomain Code=4001 "Failed to change owner:group 501:501 on Shroud (10).app with authentication." UserInfo=0x7f88e2396a50 {NSLocalizedDescription=Failed to change owner:group 501:501 on Shroud (10).app with authentication.}

Is Sparkle trying to change ownership on a file in the Trash?

@zorgiepoo
Copy link
Member

Hm. It was trying to change ownership on your old app (currently in a temporary directory) to match the user's trash's directory, before attempting to move it into the user's trash. Are permissions set up properly on the user's trash directory?

Relevant code: https://github.com/sparkle-project/Sparkle/blob/master/Sparkle/SUFileManager.m#L885

@nriley
Copy link
Contributor Author

nriley commented Feb 16, 2016

I wonder if it's caring about the group being different? Looks like my Trash permissions are left over from OS X using per-user groups.

% ls -ld ~/.Trash
drwx------  5 nicholas  501   170B Feb 15 21:52 /Users/nicholas/.Trash/
% id
uid=501(nicholas) gid=20(staff) groups=20(staff),12(everyone),61(localaccounts),79(_appserverusr),80(admin),81(_appserveradm),98(_lpadmin),103(com.apple.access_ssh-disabled),33(_appstore),100(_lpoperator),204(_developer),395(com.apple.access_ftp),398(com.apple.access_screensharing)

Maybe I'm missing something security-wise but I don't understand why this matters enough to request authentication.

@zorgiepoo
Copy link
Member

Yeah. It's trying to change owner and group of the app to match the trash directory. I am guessing you don't have an actual group name corresponding to gid 501. For me, the group assigned to ~/.Trash is staff.

As I understand it, we prefer to mv files because of atomicity, but it has the behavior (unlike of cp) in that it doesn't change owner/group. On OS X, new files that are created use the parent directory's group ID. So if we cp'ed it, it'd use the trash's gid. But we use mv instead, and the question then becomes if we should want to change the group (which requires auth if not already requested).

nriley added a commit to nriley/Sparkle-GitHub that referenced this issue Feb 26, 2016
…ect#752).

Group doesn’t need to match in order to successfully empty the trash.
@nriley
Copy link
Contributor Author

nriley commented Feb 26, 2016

OK, after some false starts on tackling this problem I think I have an idea how this could work. If the owner doesn't match, clearly we need to authenticate no matter what, and that's fine. If the owner matches and the group is different yet one of the owner's groups, then we can change its group without authenticating. If the owner matches and the group is not one of the owner's groups (as in this case where I'm stuck an old group 501 which no longer exists on the system), I think it makes sense not to change the group at all. Does this make sense?

@zorgiepoo
Copy link
Member

Just want to note changing the owner & group is also used when installing the new update (it uses the owner & group from the old app). In this particular case, I think it sounds reasonable to match the group.

Is the owner more important? What about vise versa: if the owner doesn't match, but the group does and we don't really have to authenticate, but do anyway. Can that happen?

Perhaps the safest assuming thing is not changing owner & group at all. This could be a little unfortunate though because I assume the user may have to enter a password to empty their trash.

There's also the argument/perspective that user's trash directory needs to be repaired.

@zorgiepoo
Copy link
Member

Submitted a pull request in #790 that is intended to "fix" this. Though the app may not be successfully moved to the trash in this scenario, it won't request authorization during cleanup which I think is more important.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants