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

Check for App Management permissions before updating apps. #15483

Merged
merged 4 commits into from
May 29, 2023

Conversation

JBYoshi
Copy link
Contributor

@JBYoshi JBYoshi commented May 25, 2023

  • Have you followed the guidelines in our Contributing document?
  • Have you checked to ensure there aren't other open Pull Requests for the same change?
  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes? Here's an example.
  • Have you successfully run brew style with your changes locally?
  • Have you successfully run brew typecheck with your changes locally?
  • Have you successfully run 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:

  • Both apps must be signed by the same developer
  • The app being updated needs to explicitly allow the updater app
  • The updater app needs to be granted "Full Disk Access" permission
  • The updater app needs to be granted a new "App Management" permission

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

@carlocab carlocab requested a review from a team May 25, 2023 03:57
@SMillerDev
Copy link
Member

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.

@razvanazamfirei
Copy link
Member

Agreed! Would be a fan of having the old strategy as a fall-back when the permissions are not granted.

@ywwry66
Copy link
Contributor

ywwry66 commented May 25, 2023

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.

@JBYoshi
Copy link
Contributor Author

JBYoshi commented May 25, 2023

@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.

@SMillerDev
Copy link
Member

Could just default to the old way if the check for permissions fails right?

@MikeMcQuaid
Copy link
Member

Could just default to the old way if the check for permissions fails right?

Yes, this would be better than an environment variable 👍🏻

@JBYoshi
Copy link
Contributor Author

JBYoshi commented May 26, 2023

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?

@MikeMcQuaid
Copy link
Member

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 👍🏻

@carlocab carlocab changed the title Make Homebrew check for App Management permissions before updating apps. Check for App Management permissions before updating apps. May 26, 2023
@ywwry66
Copy link
Contributor

ywwry66 commented May 26, 2023

What about printing a message somewhere in install.sh as well?

@JBYoshi
Copy link
Contributor Author

JBYoshi commented May 26, 2023

What about printing a message somewhere in install.sh as well?

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.

@SMillerDev
Copy link
Member

That would be nice, but I don't think we have a way to reliably do this until after someone's installed a cask.

Additionally, people are already not reading the existing ones. Adding more won't improve anything

@JBYoshi
Copy link
Contributor Author

JBYoshi commented May 27, 2023

By the way, I did push an update to switch to a warning message. Here's the message I've set up for now:

Your terminal does not have App Management permissions, so Homebrew will delete and reinstall the app. This may result in some configurations (like notification settings or location in the Dock/Launchpad) being lost. To fix this, go to Settings > Security and Privacy > App Management and turn on the switch for your terminal.

@SMillerDev
Copy link
Member

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.

@JBYoshi
Copy link
Contributor Author

JBYoshi commented May 28, 2023

@SMillerDev I've added that information.

Copy link
Member

@MikeMcQuaid MikeMcQuaid left a 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.

Library/Homebrew/cask/quarantine.rb Outdated Show resolved Hide resolved
Library/Homebrew/cask/quarantine.rb Outdated Show resolved Hide resolved
docs/FAQ.md Outdated Show resolved Hide resolved
docs/FAQ.md Outdated Show resolved Hide resolved
@MikeMcQuaid MikeMcQuaid merged commit 89dfe4f into Homebrew:master May 29, 2023
@github-actions github-actions bot added the outdated PR was locked due to age label Jun 29, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 29, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
outdated PR was locked due to age
Projects
None yet
6 participants