-
Notifications
You must be signed in to change notification settings - Fork 682
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
adding by_user permissions support for windows #1215
Conversation
Signed-off-by: Jeremy J. Miller <jm@chef.io>
@chris-rock @arlimus thoughts? |
Signed-off-by: Jeremy J. Miller <jm@chef.io>
@file = inspec.backend.file(path) | ||
return skip_resource 'The `file` resource is not supported on your OS yet.' if @perms_provider.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should not skip the resource if the no perm_provider is available. Most of the file implementation is done in train anyway. I would prefer to move this skip return to the individual functions that cannot be executed aka readable?
etc
@@ -52,19 +67,19 @@ def contain(*_) | |||
def readable?(by_usergroup, by_specific_user) | |||
return false unless exist? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the place where we should return the skip the resource
end | ||
|
||
def file_permission_granted?(access, by_usergroup, by_specific_user) | ||
return nil if @perms_provider.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should fail here, once we have the skip as the first statement of eg. in readable?
this method should never be called if perms_provider is not defined.
'w' | ||
when 'execute' | ||
'x' | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should return an error if the access_type is unknown
Awesome work @jeremymv2 I had some minor comments. Nothing difficult to improve. On top, I would prefer if we could add some integration tests as well in https://github.com/chef/inspec/blob/master/test/integration/default/file_spec.rb#L173-L200 |
Signed-off-by: Jeremy J. Miller <jm@chef.io>
@@ -51,20 +65,23 @@ def contain(*_) | |||
|
|||
def readable?(by_usergroup, by_specific_user) | |||
return false unless exist? | |||
return skip_resource '`readable?` is not supported on your OS yet.' if @perms_provider.nil? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Awesome work @jeremymv2, lets rebase, fix the tests and I'll merge it. Let me know if I can help with fixing the tests. |
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Signed-off-by: Jeremy J. Miller <jm@chef.io>
@chris-rock unit and integration tests added to the PR. |
Signed-off-by: Jeremy J. Miller <jm@chef.io>
Awesome @jeremymv2 |
This is a first stab at adding mvp support for checking read/write/execute file permissions, for a specific user.
The above results in the following when
vagrant-2012-r2\jdoe
was denied access to the file:I believe it addresses #783
It will fail unit tests since they will have to be refactored. If this passes muster I'll refactor the tests.
Signed-off-by: Jeremy J. Miller jm@chef.io