-
-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
Check for App Management permissions before updating apps. #15483
Conversation
There needs to be a way to use the old system because I'm not sure the Homebrew users in my company will be able to grant that permission. |
Agreed! Would be a fan of having the old strategy as a fall-back when the permissions are not granted. |
Does this restriction only affect certain apps? I don't have those permissions granted to terminal.app (which only has permissions to Desktop, Documents and Downloads folders), while I had no issue upgrading rstudio cask. |
@ywwry66 I just tried that on RStudio and you seem to be right. I'm not sure why. Apple hasn't really documented the feature beyond their WWDC talk. @SMillerDev @razvanazamfirei I can add an environment variable for that. |
Could just default to the old way if the check for permissions fails right? |
Yes, this would be better than an environment variable 👍🏻 |
The reason I was considering an environment variable is because macOS doesn't let apps prompt for the App Management permission and then wait for the user to accept or deny it (as it does for permissions like the microphone); the permission is just automatically denied by default. If we ignore failures, first-time Homebrew users won't have a chance to grant those permissions before the first upgrade happens. We wouldn't be able to tell the difference between a user who hasn't decided whether to grant permissions or not and a user who has chosen not to grant permissions. An alternative option would be to just print a warning message during the upgrade if the permission check fails. While the first upgrade would be done using the old behavior, the user could notice the warning and take action to grant the permissions for next time. Would that work? |
Yeh, this makes sense 👍🏻 |
What about printing a message somewhere in |
That would be nice, but I don't think we have a way to reliably do this until after someone's installed a cask. The terminal also doesn't appear in the App Management page until someone tries to modify an app that isn't one of thne built-in apps (which are under the System partition and can't be modified even with Full Disk Access). The best we could do is try it with an app like Pages that comes preinstalled but is managed by the App Store, and that would still fail if someone happened to have uninstalled the app in question. |
Additionally, people are already not reading the existing ones. Adding more won't improve anything |
By the way, I did push an update to switch to a warning message. Here's the message I've set up for now:
|
Sounds great, thanks for all the work here @JBYoshi. One thing I'm thinking is that it might be good to add "why does my app move in launchpad" or something to the FAQ doc and give this permission as an answer. |
@SMillerDev I've added that information. |
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.
Great work! A few tiny nits I've applied myself.
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?macOS Ventura added new restrictions on apps being allowed to update other apps. In order for one app to update another, at least one of the following must be true:
This didn't show up until #15138 was merged because prior to that, macOS considered Homebrew's app updates to be uninstalls and reinstalls, which discarded app data. Since #15138, macOS now recognizes that Homebrew is updating apps, so anyone who hasn't already granted the proper permissions to their terminal will get errors while copying the app files. This PR adds a check before app updates to ensure that the terminal has been granted permissions. If the terminal hasn't been granted permissions, Homebrew will exit and give the user instructions to grant their terminal the necessary permissions.
When setting up unit tests for this, I also moved the existing "upgrade properly handles non-writable directories" to a new group "when the directory is owned by root" to clarify the meaning and to better group the tests.
Fixes Homebrew/homebrew-cask#147383
Fixes Homebrew/homebrew-cask#147789
Fixes #15484