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

Fix incorrect file permissions in file.line #30212

Merged
merged 1 commit into from Jan 19, 2016

Conversation

Projects
None yet
4 participants
@abednarik
Contributor

abednarik commented Jan 7, 2016

Looks like atomic_open() doesn't take care of preserving file permissions. It will create
a new file, edit it and then move this temp file to the original file ignoring file permissions.
Also updated the check to be sure file exist and is a file. Otherwise code will try to update
directory content and will fail badly and throw a python error.

Fixes #30150.

Hope this one don't break anything :(

@bailsman

This comment has been minimized.

Show comment
Hide comment
@bailsman

bailsman Jan 8, 2016

Contributor

When atomic_open is called from the file module, the file always exists. However, when atomic_open() is called from returners, the file does not exist yet. Should atomic_open() make the assumption that the file exists? If yes, you should fix the calling code in salt/returners/local_cache.py and perhaps other places.

If atomic_open should NOT assume the file exists, I think it's a bit nicer to check if the file exists rather than simply swallowing exceptions. For example:

        if os.path.isfile(self._filename):
            shutil.copymode(self._filename, self._tmp_filename)

After all, you still want to know if something unexpected happens, and not silently continue.

Contributor

bailsman commented Jan 8, 2016

When atomic_open is called from the file module, the file always exists. However, when atomic_open() is called from returners, the file does not exist yet. Should atomic_open() make the assumption that the file exists? If yes, you should fix the calling code in salt/returners/local_cache.py and perhaps other places.

If atomic_open should NOT assume the file exists, I think it's a bit nicer to check if the file exists rather than simply swallowing exceptions. For example:

        if os.path.isfile(self._filename):
            shutil.copymode(self._filename, self._tmp_filename)

After all, you still want to know if something unexpected happens, and not silently continue.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 8, 2016

Contributor

Agree. Thanks @codehotter

We are not checking tmp file, which is also a good idea I think. What do you think? Otherwise it will fail badly.

Contributor

abednarik commented Jan 8, 2016

Agree. Thanks @codehotter

We are not checking tmp file, which is also a good idea I think. What do you think? Otherwise it will fail badly.

@bailsman

This comment has been minimized.

Show comment
Hide comment
@bailsman

bailsman Jan 8, 2016

Contributor

The temp file should be guaranteed to exist here. It's not necessary to check for it.

Contributor

bailsman commented Jan 8, 2016

The temp file should be guaranteed to exist here. It's not necessary to check for it.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 8, 2016

Contributor

Thanks again @codehotter. I just updated this PR with your suggestion.

Contributor

abednarik commented Jan 8, 2016

Thanks again @codehotter. I just updated this PR with your suggestion.

Show outdated Hide outdated salt/modules/file.py Outdated
@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 8, 2016

Contributor

Is the way i send in the PR

Contributor

abednarik commented Jan 8, 2016

Is the way i send in the PR

@bailsman

This comment has been minimized.

Show comment
Hide comment
@bailsman

bailsman Jan 8, 2016

Contributor

In your previous version it was fine, but now it says "isfle". Look in your github repository on the fix_file_line_permissions branch:
https://github.com/abednarik/salt/blob/fix_file_line_permissions/salt/modules/file.py#L1471

If it's correct locally, maybe you can try to push the branch again?

Contributor

bailsman commented Jan 8, 2016

In your previous version it was fine, but now it says "isfle". Look in your github repository on the fix_file_line_permissions branch:
https://github.com/abednarik/salt/blob/fix_file_line_permissions/salt/modules/file.py#L1471

If it's correct locally, maybe you can try to push the branch again?

Fix incorrect file permissions in file.line
Looks like atomic_open() doesn't take care of preserving file permissions.  It will create
a new file, edit it and then move this temp file to the original file ignoring file permissions.
Also updated the check to be sure file exist and is a file. Otherwise code will try to update
directory content and will fail badly and throw a python error.

Fixes #30150.
@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 8, 2016

Contributor

You are right. Now should be fixed.

Thanks.

Contributor

abednarik commented Jan 8, 2016

You are right. Now should be fixed.

Thanks.

@Grokzen

This comment has been minimized.

Show comment
Hide comment
@Grokzen

Grokzen Jan 12, 2016

@abednarik Would this also fix the following issue? #28320 This issue behaves in the same way as mentioned issue does.

Grokzen commented Jan 12, 2016

@abednarik Would this also fix the following issue? #28320 This issue behaves in the same way as mentioned issue does.

@abednarik

This comment has been minimized.

Show comment
Hide comment
@abednarik

abednarik Jan 12, 2016

Contributor

Hi @Grokzen don't' think so, since comment_line is using other function _mkstemp_copy. Anyway, should be fair easy to apply the same I did here.
Will send a PR today, if I can.

Contributor

abednarik commented Jan 12, 2016

Hi @Grokzen don't' think so, since comment_line is using other function _mkstemp_copy. Anyway, should be fair easy to apply the same I did here.
Will send a PR today, if I can.

cachedout added a commit that referenced this pull request Jan 19, 2016

Merge pull request #30212 from abednarik/fix_file_line_permissions
Fix incorrect file permissions in file.line

@cachedout cachedout merged commit 0af5e16 into saltstack:2015.8 Jan 19, 2016

2 of 5 checks passed

default Merged build finished.
Details
jenkins/salt-pr-rs-cent7-n Salt PR - RS CentOS 7 #11192 — FAILURE
Details
jenkins/salt-pr-rs-ubuntu14.04-n Salt PR - RS Ubuntu 14 #8684 — FAILURE
Details
jenkins/salt-pr-clone Salt PR - Clone Repository #12603 — SUCCESS
Details
jenkins/salt-pr-lint-n Salt PR - Code Lint #12302 — SUCCESS
Details

@abednarik abednarik deleted the abednarik:fix_file_line_permissions branch Jan 19, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment