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

File permission checks should return false unless file exists #301

Merged
merged 1 commit into from
Dec 8, 2015

Conversation

adamleff
Copy link
Contributor

@adamleff adamleff commented Dec 7, 2015

Currently, #readable?, #writeable?, and #executable? will incorrectly
return true if the file does not exist.

$ cat test.rb
describe file('/tmp/test.txt') do
  it { should be_readable }
  it { should be_writeable }
  it { should be_executable }
end

$ stat /tmp/test.txt
stat: /tmp/test.txt: stat: No such file or directory

$ inspec exec ./test.rb
...

Finished in 0.03351 seconds (files took 0.12235 seconds to load)
3 examples, 0 failures

This patch forces these three methods to immediately return false unless the file exists.

@chris-rock
Copy link
Contributor

@adamleff great fix. Can you add some tests as well?

@chris-rock chris-rock added the Type: Bug Feature not working as expected label Dec 7, 2015
@adamleff
Copy link
Contributor Author

adamleff commented Dec 8, 2015

@chris-rock sure I can do that. I had trouble locating unit tests for the file resource. Am I missing it or are there no tests for this resource yet?

@chris-rock
Copy link
Contributor

@adamleff You are right, we have no unit test for file because those tests are part of train. What we implemented for file is a integration test:
https://github.com/chef/inspec/blob/master/test/integration/test/integration/default/file_spec.rb

I would love to see this implemented here.

@adamleff
Copy link
Contributor Author

adamleff commented Dec 8, 2015

@chris-rock I started to write tests for the File resource but found it quite difficult based on the layout of the Class. I took the opportunity to refactor it to eliminate some repetitiveness in the code, make some more single-purpose methods, and make it far easier to test. I then included a full unit test for the class. Please check it out and let me know what you think.

@arlimus if you have feedback too, I'm totally open to it!

@chris-rock
Copy link
Contributor

@adamleff awesome improvement. It covers some cleanup that was on my agenda. Great work 💯

Currently, #readable?, #writeable?, and #executable? will incorrectly
return true if the file does not exist.

In addition, I took the opportunity to refactor the File resource to
make it easier to write unit tests and supplied a full unit test
suite for this resource.
chris-rock added a commit that referenced this pull request Dec 8, 2015
File permission checks should return false unless file exists
@chris-rock chris-rock merged commit bada329 into master Dec 8, 2015
@chris-rock chris-rock deleted the adamleff/file-bugfix branch December 8, 2015 19:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Feature not working as expected
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants