Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

(PUP-1801) Manage owner if specified #4

Merged
merged 1 commit into from

2 participants

@ferventcoder

Owner should only be managed if specified. It can also vary depending on the
context of the user that creates the securable item and puppet should respect
that instead of trying to force a default.

lib/puppet/type/acl/constants.rb
@@ -1,5 +1,6 @@
class Puppet::Type::Acl
module Constants
GROUP_UNSPECIFIED = 'UserDefaultGroup'
@peterhuene Owner

Just a thought on this (probably should have mentioned this in the last review): what if users wanted a group called "UserDefaultGroup" or an owner named "UserDefaultOwner". Is there a reason we don't use nil to signify "not specified" for these two properties? Obviously the validation logic for each would need to change to raise ArgumentError, "A non-empty [owner|group] must be specified." if value and value.empty? and insync would just return true if nil.

There's probably some obvious reason for doing it this way that I'm just not seeing.

@peterhuene Owner

@ferventcoder Any thoughts on this?

@ferventcoder Owner

That might be a better way of doing it. There may or may not be a reason due to the way puppet handles properties and requires a value to be specified. If there is not it may be better to use something even less likely to be called.

@ferventcoder Owner

Out of curiosity, what would a person entering the value in the manifest expect for owner=> ''? I would want to validate that and throw an error.

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

Other than that comment, it looks pretty much identical to the previous group change. :+1:

@ferventcoder ferventcoder (PUP-1801) Manage owner if specified
Owner should only be managed if specified. It can also vary depending on the
context of the user that creates the securable item and puppet should respect
that instead of trying to force a default.
c7f765b
@peterhuene peterhuene merged commit 30b97d1 into puppetlabs:master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Mar 4, 2014
  1. @ferventcoder

    (PUP-1801) Manage owner if specified

    ferventcoder authored
    Owner should only be managed if specified. It can also vary depending on the
    context of the user that creates the securable item and puppet should respect
    that instead of trying to force a default.
This page is out of date. Refresh to see the latest.
View
24 lib/puppet/type/acl.rb
@@ -27,6 +27,10 @@ def initialize(*args)
self[:target] = self[:name]
end
+ if self[:owner].nil? then
+ self[:owner] = Puppet::Type::Acl::Constants::OWNER_UNSPECIFIED
+ end
+
if self[:group].nil? then
self[:group] = Puppet::Type::Acl::Constants::GROUP_UNSPECIFIED
end
@@ -110,8 +114,9 @@ def is_to_s(currentvalue)
that is said to own the particular acl/security descriptor. This
can be in the form of: 1. User - e.g. 'Bob' or 'TheNet\\Bob',
2. Group e.g. 'Administrators' or 'BUILTIN\\Administrators', 3.
- SID (Security ID) e.g. 'S-1-5-18'. Defaults to 'S-1-5-32-544'
- (Administrators) on Windows."
+ SID (Security ID) e.g. 'S-1-5-18'. Defaults to not specified on
+ Windows. This allows owner to stay set to whatever it is currently
+ set to (owner can vary depending on the original CREATOR OWNER)."
validate do |value|
if value.nil? or value.empty?
@@ -119,10 +124,9 @@ def is_to_s(currentvalue)
end
end
- #todo check platform and return specific default - this may not always be windows
- defaultto 'S-1-5-32-544'
-
def insync?(current)
+ return true if should == Puppet::Type::Acl::Constants::OWNER_UNSPECIFIED
+
if provider.respond_to?(:owner_insync?)
return provider.owner_insync?(current, should)
end
@@ -228,11 +232,13 @@ def insync?(current)
provider.class.send(:define_method,'get_account_name', &return_same_value)
end
- owner_name = provider.get_account_name(self[:owner])
+ unless self[:owner] == Puppet::Type::Acl::Constants::OWNER_UNSPECIFIED
+ owner_name = provider.get_account_name(self[:owner])
- # add both qualified and unqualified items
- required_users << "User[#{self[:owner]}]"
- required_users << "User[#{owner_name}]"
+ # add both qualified and unqualified items
+ required_users << "User[#{self[:owner]}]"
+ required_users << "User[#{owner_name}]"
+ end
permissions = self[:permissions]
unless permissions.nil?
View
3  lib/puppet/type/acl/constants.rb
@@ -1,5 +1,6 @@
class Puppet::Type::Acl
module Constants
- GROUP_UNSPECIFIED = 'UserDefaultGroup'
+ GROUP_UNSPECIFIED = 'UserDefaultGroup+-=!'
+ OWNER_UNSPECIFIED = 'UserDefaultOwner+-=!'
end
end
View
7 spec/unit/provider/acl/windows_spec.rb
@@ -56,7 +56,8 @@ def test_should_not_set_autorequired_user(user_name)
test_should_not_set_autorequired_user('Administrators')
end
- it "should autorequire BUILTIN\\Administrators if owner is set to the default Administrators SID" do
+ it "should autorequire BUILTIN\\Administrators if owner is set to the Administrators SID" do
+ resource[:owner] = 'S-1-5-32-544'
test_should_set_autorequired_user('BUILTIN\Administrators')
end
@@ -72,8 +73,8 @@ def test_should_not_set_autorequired_user(user_name)
end
context ":owner" do
- it "should be set to Administrator SID by default" do
- resource[:owner].must == 'S-1-5-32-544'
+ it "should be set to the default unspecified value by default" do
+ resource[:owner].must == Puppet::Type::Acl::Constants::OWNER_UNSPECIFIED
end
context ".insync?" do
View
11 spec/unit/type/acl_spec.rb
@@ -107,7 +107,12 @@ def test_should_not_set_autorequired_user(user_name)
reqs.must be_empty
end
- it "should autorequire owner" do
+ it "should not autorequire owner when set to unspecified" do
+ test_should_not_set_autorequired_user('Administrators')
+ end
+
+ it "should autorequire owner when set to Administrators" do
+ resource[:owner] = 'Administrators'
test_should_set_autorequired_user(resource[:owner])
end
@@ -233,8 +238,8 @@ def test_should_set_autorequired_file(resource_path,file_path)
end
context "property :owner" do
- it "should default to S-1-5-32-544 (Administrators)" do
- resource[:owner].must == 'S-1-5-32-544'
+ it "should default to use the default unspecified group" do
+ resource[:owner].must == Puppet::Type::Acl::Constants::OWNER_UNSPECIFIED
end
it "should accept bob" do
Something went wrong with that request. Please try again.