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

Fix #17, ensure the keyfile is writable before creating a user #25

Merged
merged 1 commit into from
Dec 1, 2014

Conversation

jmink
Copy link
Contributor

@jmink jmink commented Dec 1, 2014

chef/chef-server#17 is the related issue.

This just ensures the file exists and is writable before creating the user.

Requires manual testing.

File.open(config[:filename], "w") do |f|
ui.msg "File #{config[:filename]} exists and can be written to."
end
rescue Errno::ENOENT
Copy link
Contributor

Choose a reason for hiding this comment

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

You likely want to rescue Errno::EACCES as well if we are worried about permissions.

@jmink
Copy link
Contributor Author

jmink commented Dec 1, 2014

Passed manual testing:

root@default-ubuntu-1204:/# chef-server-ctl user-create foo1 f1 oo foo1@test.com testtest --filename ./foo/file2.key
FATAL: File ./foo/file2.key is not writable.  Check permissions.
root@default-ubuntu-1204:/# chef-server-ctl user-create foo1 f1 oo foo1@test.com testtest --filename ./file2.key
File ./file2.key exists and can be written to.
root@default-ubuntu-1204:/#

@jmink
Copy link
Contributor Author

jmink commented Dec 1, 2014

Thanks for the comments @stevendanna. I like it a lot better now. The new version has also passed manual testing.

@@ -54,6 +54,14 @@ def run
:password => password
}

# Check the file before creating the user so the api is more transactional.
if config[:filename]
unless File.writable?(config[:filename]), "w")
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm confused on how this works for you.

irb(main):002:0> File.writable?("foo", "w")
ArgumentError: wrong number of arguments (2 for 1)
        from (irb):2:in `writable?'

Unfortunately, if the file doesn't exist, I believe that will return false even if the directory is writable:

 File.writable?("./nothinghere")
=> false

I couldn't find a function in the File class that does this, but I think you need something like:

File.exist?(config[:filename]) ? File.writable?(config[:filename]) : File.writable?(File.dirname(config[:filename]))

@jmink
Copy link
Contributor Author

jmink commented Dec 1, 2014

Thank you again @stevendanna. I was more careful with this test:

root@default-ubuntu-1204:/# chef-server-ctl user-create foo3 f3 oo foo3@test.com testtest --filename ./foo/file4.key
FATAL: File ./foo/file4.key is not writable. Check permissions.
root@default-ubuntu-1204:/# chef-server-ctl user-create foo3 f3 oo foo3@test.com testtest --filename ./file4.key
root@default-ubuntu-1204:/# chef-server-ctl user-create foo4 f4 oo foo4@test.com testtest --filename ./file4.key
root@default-ubuntu-1204:/# chef-server-ctl user-create foo4 f4 oo foo4@test.com testtest --filename ./file4.key
ERROR: Conflict
Response: User 'foo4' already exists
root@default-ubuntu-1204:/#

https://gist.github.com/jmink/e378fd5c408666d07d83

@stevendanna
Copy link
Contributor

👍 Looks good to me.

@jmink jmink merged commit 92d9f1d into master Dec 1, 2014
jmink added a commit that referenced this pull request Dec 1, 2014
@jmink jmink deleted the jm/17 branch December 1, 2014 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants