Skip to content

Check for App Management permissions before updating apps. - #15483

Merged
MikeMcQuaid merged 4 commits into
Homebrew:masterfrom
JBYoshi:cask-prompt-permissions
May 29, 2023
Merged

Check for App Management permissions before updating apps.#15483
MikeMcQuaid merged 4 commits into
Homebrew:masterfrom
JBYoshi:cask-prompt-permissions

Conversation

@JBYoshi

@JBYoshi JBYoshi commented May 25, 2023

Copy link
Copy Markdown
Contributor
  • 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
Copy Markdown
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
Copy Markdown
Contributor

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

@ywwry66

ywwry66 commented May 25, 2023

Copy link
Copy Markdown
Contributor

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

JBYoshi commented May 25, 2023

Copy link
Copy Markdown
Contributor Author

@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
Copy Markdown
Member

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

@MikeMcQuaid

Copy link
Copy Markdown
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

JBYoshi commented May 26, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

ywwry66 commented May 26, 2023

Copy link
Copy Markdown
Contributor

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

@JBYoshi

JBYoshi commented May 26, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

JBYoshi commented May 27, 2023

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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

JBYoshi commented May 28, 2023

Copy link
Copy Markdown
Contributor Author

@SMillerDev I've added that information.

@MikeMcQuaid MikeMcQuaid left a comment

Copy link
Copy Markdown
Member

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.

Comment thread Library/Homebrew/cask/quarantine.rb Outdated
Comment thread Library/Homebrew/cask/quarantine.rb
Comment thread docs/FAQ.md Outdated
Comment thread docs/FAQ.md Outdated
@MikeMcQuaid
MikeMcQuaid enabled auto-merge May 29, 2023 08:02
@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