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

Update deploy:writable_dirs: When running WITHOUT sudo, exec setfacl once only #442

Merged
merged 3 commits into from Aug 31, 2015

Conversation

vanquang9387
Copy link
Contributor

When run deploy:writable_dirs WITHOUT sudo, exception maybe thrown if setfacl run on files created by http user (in directory that has been setfacl before). These directories/files should be skipped.

Ref: #433 (comment)

@antonmedv
Copy link
Member

Cool! I'm going to merge. Can any one to test it? @oanhnn @deployphp/maintainer ?

run("$sudo setfacl -R -m u:\"$httpUser\":rwX -m u:`whoami`:rwX $dirs");
run("$sudo setfacl -dR -m u:\"$httpUser\":rwX -m u:`whoami`:rwX $dirs");
} else {
// If run without sudo, check each dir
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vanquang9387 can you add more comments here, why this is necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elfet
On first deploy:

setfacl -R -m u:"apache":rwX -m u:`whoami`:rwX app/webroot/images

Then, apache creates files:

$ ll current/app/webroot/images/
total 112
-rw-r--r--. 1 apache apache 34821 Aug 21 17:07 1440144423_main.png
-rw-r--r--. 1 apache apache 34821 Aug 21 17:07 1440144423.png
-rw-rw-r--+ 1 apache apache 34821 Aug 21 17:07 1440144441_55d6dc39ce799.png

Next, exec setfacl on next deploy will throw runtime exception:

Run: cd /var/www/html/project/releases/20150821170801 &&  setfacl -R -m u:"apache":rwX -m u:`whoami`:rwX app/webroot/images

  Unable to setup correct permissions for writable dirs.
  You need co configure sudo's sudoers files to don't prompt for password,
  or setup correct permissions manually.

  [RuntimeException]
  setfacl: app/webroot/images/1440144441_55d6dc39ce799.png: Operation not permitted
  setfacl: app/webroot/images/1440144423_main.png: Operation not permitted
  setfacl: app/webroot/images/1440144423.png: Operation not permitted

I mentioned this problem here: #433 (comment)

Do you need more comments in the code?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Elfet comments added 🌟

@vanquang9387
Copy link
Contributor Author

ping @Elfet

@oanhnn
Copy link
Contributor

oanhnn commented Aug 28, 2015

This PR has some issues:

  • This code dose not care user:www-data:r-- or user:www-data:rwx. It alway return a number greater than 0.
  • This code dose not care user www-data (apache user) and user www-data2 (other user)
  • If setfacl is exist with group ???

And one issue of Deployer:

  • Check deployer user and apache user is same user 😄

@vanquang9387
Copy link
Contributor Author

@oanhnn Please check my new commit.
I think problem that "httpd user is the same user or same group with deployer user" is not related to setfacl. This should be checked at the beginning of writable task if needed.

oanhnn pushed a commit that referenced this pull request Aug 31, 2015
Update deploy:writable_dirs: When running WITHOUT sudo, exec setfacl once only
@oanhnn oanhnn merged commit 55a09b8 into deployphp:master Aug 31, 2015
@oanhnn
Copy link
Contributor

oanhnn commented Aug 31, 2015

Thank you!

@vanquang9387 vanquang9387 deleted the run-setfacl-once branch October 20, 2015 02:55
@svycka
Copy link

svycka commented Oct 16, 2018

This still has a problem it does not check recursively for example with a config like this:

set('shared_dirs', [
    'data/tmp'
]);
set('writable_dirs', [
    'data',
]);

will fail because acl is set on data/tmp and files within but this PR only checks if data/ has acl set or not.

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

Successfully merging this pull request may close these issues.

None yet

4 participants