Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

valet change folder ownership #1220

Closed
PATROMO opened this issue Mar 28, 2022 · 8 comments
Closed

valet change folder ownership #1220

PATROMO opened this issue Mar 28, 2022 · 8 comments

Comments

@PATROMO
Copy link

PATROMO commented Mar 28, 2022

After fresh brew php installation:
image

After valet usage:
image

This causes problems when upgrading the php version in the future.
image

@nicoverbruggen
Copy link
Contributor

nicoverbruggen commented Mar 28, 2022

Hi, all!

I wanted to chime in on this one, since I've written about this as well when I was dealing with this issue in PHP Monitor and even shipped a feature/workaround that "fixes" this, called "Fix Homebrew Permissions".

I don't think anything can be done about the ownership (it's a consequence of running php as root). However, reinstating the original permissions when you run valet stop might be a good idea.

If implemented, you could then feasibly run valet stop && brew upgrade php, for example, without Homebrew then creating .reinstall folders, which would be awesome.

Technical details

In phpmon, I restore the ownership like so...

chown -R <username>:staff /path/to/directory (default Homebrew permissions give the current user (accessible via whoami, and the staff group access IIRC)

... for the following directories:

let directories = [
    "\(brewDir)/Cellar/\(formula)"
    "\(brewDir)/opt/\(formula)"
    "\(brewDir)/var/homebrew/linked/\(formula)"
];

So this has to happen for all installed PHP versions/formulae as well as for nginx and dnsmasq. I was going to take a look at this one myself and open a PR but I haven't had the time yet, unfortunately.

@driesvints
Copy link
Member

@PATROMO What are the reasons for running php as root? (Thanks for chiming in @nicoverbruggen)

@nicoverbruggen
Copy link
Contributor

I believe running all those services as root is required in order to successfully bind to port 80 on localhost, IIRC.

@PATROMO
Copy link
Author

PATROMO commented Mar 29, 2022

@nicoverbruggen thanks for explaining :)
Yes, we need root to bind port 80. But I think that the folders should not change the ownership. :)

@nicoverbruggen
Copy link
Contributor

That's a Homebrew thing, though. I don't think we are supposed to change those permissions as long as the services are running, or we risk breaking the services in question.

I believe one of the reasons was that for the services to start at boot (which happens via a plist file), the executable must be owned by the user who will execute the process. If you were to change the ownership, Valet's services won't work correctly the next time you reboot your machine until you manually intervene.

I was also able to find this explanation:

Q: didn't we chown the files when running services as sudo?
A: Yes, we do that because it's a security vulnerability to not do so. You'll need to sudo brew services stop (which fixes the permissions) before upgrading.

@driesvints
Copy link
Member

Thanks @nicoverbruggen. Looks like we probably won't be intervening here if it's a Homebrew thing.

@mattstauffer
Copy link
Collaborator

I'm just going to do a quick recap to make sure A) I'm understanding it correctly and B) this is clear to others in the future.

  • We have to change these permissions in order for Valet to be able to restart on reboot.
  • If there's any change to be made, it would be, as @nicoverbruggen suggested, "[R]einstating the original permissions when you run valet stop"
  • Nico may PR ^^ one day but for right now, this actual issue--that changing the folder ownership is an issue--is not something we can actually change and still expect Valet to continue operating as it is.

@nicoverbruggen
Copy link
Contributor

@mattstauffer As far as I understand it, your recap is correct! Since it was not a difficult issue to fix, I've opened an MR with a my proposed enhancement/fix 😙

# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

No branches or pull requests

4 participants