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

chmod with corrent umask #507

Conversation

ihortymoshenko
Copy link

No description provided.

umask($old_umask);
}
// set file permissions
chmod($_filepath, 0666 & ~umask());
Copy link
Author

Choose a reason for hiding this comment

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

Fixed chmod mode to apply the umask correctly

Copy link
Contributor

Choose a reason for hiding this comment

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

If I were you, I'd simply remove all calls to chmod()/chown(). This is not a file management code, and unless you do something drastic, you should be able to read files you've created. Meaning, no chmod required.

If there's still a problem, then fix your FS settings. All modern FS supports ACL, you can get results much better than ancient POSIX permissions.

Copy link
Author

@ihortymoshenko ihortymoshenko Feb 25, 2020

Choose a reason for hiding this comment

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

@AnrDaemon,

let's imagine:

  • you compile your templates while deploying a project (for example using Jenkins), compiled templates will have chmod 0644, only owner has permissions to write.
  • you need to compile/re-compile your template after deployment, you cannot do it as chmod 0644 and you are not the owner (PHP is run as a www-data).

chmod 0666 means that the owner, group and other users have read and write permissions.
umask 0002 (in your code) means that chmod will be 0664, that means that other users does not have write permissions.

Using chmod 0666 with umask (for example, 0002) solves the issue when different users need permissions to write.

A similar issue was in Twig in 2012.

This bug fix needs to be merged, without it there are problems with permissions that cannot be solved in another way

Copy link
Contributor

Choose a reason for hiding this comment

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

Again, both your cases fall into "fix your FS permissions already" category.

setfacl -Rm "d:u:builder-user:rwX,d:u:fpm-user:rwX,d:m::rwx" -- /templates/directory

Copy link
Author

Choose a reason for hiding this comment

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

@AnrDaemon,

setfacl is one of the ways to solve the issue with permissions.

I suggest to improve work with chmod and add the ability to influence on chmod which is hardcoded

Copy link

Choose a reason for hiding this comment

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

@AnrDaemon
Please also note that it is possible to set the default umask for php-fpm, for instance.
I usually do that and set umask to 0027.

smarty will nonetheless always create cache files with 0644.
Having explicitly set a umask, my expectation is that this default is honored and used.

It is certainly true that you can fine-tune things using ACLs in ways file permissions don't allow for. However, dismissing POSIX as ancient and thus something that can be ignored is in and of itself a rather disturbing point of view, to be frank.

@wisskid
Copy link
Member

wisskid commented Jul 16, 2022

Okay, I've created #772 to at least remove the code that checks Smarty's obsolete properties. Now remains that question of why Smarty (seemingly intentionally) disables the current umask setting when writing files and restores the setting afterwards. I can trace it back to a commit 7 years ago by @uwetews, but there is not specific explanation for doing this. I'm a little reluctant to change this without grasping the consequences.

@ihortymoshenko
Copy link
Author

ihortymoshenko commented Jul 18, 2022

@wisskid,

I see no reason to do this.

I think it's better to create a directory with permissions 777 and files with permissions 666 - umask.

See how it's done in Twig here and here.

@AnrDaemon
Copy link
Contributor

AnrDaemon commented Jul 18, 2022

I see no reason to touch permissions at all without a very specific need for that. Since we are not a file manager, it is not our area of responsibility.
I see why we would need to specify 0777 for mkdir() call, but trying to explicitly chmod() at best make no difference, and at worst - break the permissions set by the administrator.

@ihortymoshenko
Copy link
Author

ihortymoshenko commented Jul 18, 2022

@AnrDaemon,

I've described the issue here, it's a real case, you cannot call the chmod function to fix permissions with umask without specifying some permissions (666).

@AnrDaemon
Copy link
Contributor

I already said that your deployment script should have been executed compilation from web user or have fixed permissions after compilation. Either way, not really a Smarty issue.

@ihortymoshenko
Copy link
Author

@AnrDaemon,

Okay, as you wish, 4 years is too long for this question.

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

Successfully merging this pull request may close these issues.

4 participants