This is really fantastic - the code looks great, and provides a great starting point for people who want to provide other aspects of Windows support.
Paul's already done a once-over on this, but there are a couple of other points worth making. On Mar 20, 2009, at 3:01 PM, finalprefix wrote: > > Hi, > > This patch has basic support for users and groups on Windows. > > * It for version 0.24.7. > * It does not include support for active directory. > * It does not work with the 0.24.8 beta. > > The code is up at http://github.com/finalprefix/puppet/tree/0.24.7- > win. > > I'd love any feedback you might have! > > Thanks, > Joel. > > diff --git a/lib/puppet.rb b/lib/puppet.rb > index 4b0091c..77dbdfe 100644 > --- a/lib/puppet.rb > +++ b/lib/puppet.rb > @@ -190,53 +190,57 @@ module Puppet > # Trap a couple of the main signals. This should probably be > handled > # in a way that anyone else can register callbacks for traps, > but, eh. > def self.settraps > - [:INT, :TERM].each do |signal| > - trap(signal) do > - Puppet.notice "Caught #{signal}; shutting down" > - Puppet.debug "Signal caught here:" > - caller.each { |l| Puppet.debug l } > - Puppet.shutdown > - end > - end > - > - # Handle restarting. > - trap(:HUP) do > - if client = @services.find { |s| s.is_a? > Puppet::Network::Client.master } and client.running? > - client.restart > - else > - Puppet.restart > - end > - end > All of this code was completely redone in the master branch, so I'm interested in seeing how your rebase looks; can you publish it? All of the traps should be in the daemon.rb class, I think. > - # Provide a hook for running clients where appropriate > - trap(:USR1) do > - done = 0 > - Puppet.notice "Caught USR1; triggering client run" > - @services.find_all { |s| s.is_a? > Puppet::Network::Client }.each do |client| > - if client.respond_to? :running? > - if client.running? > - Puppet.info "Ignoring running %s" % > client.class > - else > - done += 1 > - begin > - client.runnow > - rescue => detail > - Puppet.err "Could not run client: %s" % > detail > - end > - end > - else > - Puppet.info "Ignoring %s; cannot test whether it > is running" % > - client.class > - end > - end > - > - unless done > 0 > - Puppet.notice "No clients were run" > - end > - end > - > - trap(:USR2) do > - Puppet::Util::Log.reopen > + begin > + [:INT, :TERM].each do |signal| > + trap(signal) do > + Puppet.notice "Caught #{signal}; shutting down" > + Puppet.debug "Signal caught here:" > + caller.each { |l| Puppet.debug l } > + Puppet.shutdown > + end > + end > + > + # Handle restarting. > + trap(:HUP) do > + if client = @services.find { |s| s.is_a? > Puppet::Network::Client.master } and client.running? > + client.restart > + else > + Puppet.restart > + end > + end > + > + # Provide a hook for running clients where appropriate > + trap(:USR1) do > + done = 0 > + Puppet.notice "Caught USR1; triggering client run" > + @services.find_all { |s| s.is_a? Puppet::Network::Client > }.each > do |client| > + if client.respond_to? :running? > + if client.running? > + Puppet.info "Ignoring running %s" % client.class > + else > + done += 1 > + begin > + client.runnow > + rescue => detail > + Puppet.err "Could not run client: %s" % > detail > + end > + end > + else > + Puppet.info "Ignoring %s; cannot test whether it is > running" % > + client.class > + end > + end > + > + unless done > 0 > + Puppet.notice "No clients were run" > + end > + end > + > + trap(:USR2) do > + Puppet::Util::Log.reopen > + end > + rescue ArgumentError => e > + raise if e.to_s.index('SIGHUP') == nil > end > end > > diff --git a/lib/puppet/provider/group/groupadd_win.rb b/lib/puppet/ > provider/group/groupadd_win.rb > new file mode 100644 > index 0000000..c1fa7f4 > --- /dev/null > +++ b/lib/puppet/provider/group/groupadd_win.rb > @@ -0,0 +1,30 @@ > +require 'puppet/util/windows_system' > + > +Puppet::Type.type(:group).provide :groupadd_win do > + desc "Group management for windows" > + > + defaultfor :operatingsystem => :windows You should have a 'confine' here that fkeeps this from being used anywhere that doesn't have Windows::Group; I think it's something like: confine :operatingsystem => :windows > > + has_features :manages_members > + > + def members > + Windows::Group.new(@resource[:name]).members > + end > + > + def members=(members) > + Windows::Group.new(@resource[:name]).set_members(members) > + end > + > + def create > + group = Windows::Group.create(@resource[:name]) > + group.set_members(@resource[:members]) > + end > + > + def exists? > + Windows::Group.exists?(@resource[:name]) > + end > + > + def delete > + Windows::Group.delete(@resource[:name]) > + end > +end > diff --git a/lib/puppet/provider/user/useradd_win.rb b/lib/puppet/ > provider/user/useradd_win.rb > new file mode 100644 > index 0000000..5f9b969 > --- /dev/null > +++ b/lib/puppet/provider/user/useradd_win.rb > @@ -0,0 +1,42 @@ > +require 'puppet/provider' > +require 'puppet/util/windows_system' > + > +raise "ERROR: A windowsuser resource can only be configured on > Windows. This OS is is #{Facter['kernel'].value}" if Facter > ['kernel'].value != 'windows' Same as above, you should use 'confine' to do this restriction, rather than raising this exception. These files get loaded by every host, in order to find the default provider, so this exception would get triggered on every host. > > +Puppet::Type.type(:user).provide :useradd_win do > + desc "User management for windows" > + > + defaultfor :operatingsystem => :windows > + > + has_features :manages_passwords > + > + def password > + name, password = @resource[:name], @resource[:password] > + Windows::User.new(name).password_is?(password) ?password :"" > rescue :absent > + end > + > + def password=(pwd) > + Windows::User.new(@resource[:name]).password = > @resource[:password] > + end > + > + def groups > + Windows::User.new(@resource[:name]).groups.join(',') rescue > :absent > + end > + > + def groups=(groups) > + Windows::User.new(@resource[:name]).set_groups(groups) > + end > + > + def create > + user = Windows::User.create(@resource[:name], > @resource[:password]) > + user.set_groups(@resource[:groups], @resource[:membership] > == :minimum) > + end > + > + def exists? > + return Windows::User.exists?(@resource[:name]) > + end > + > + def delete > + Windows::User.delete(@resource[:name]) > + end > +end > diff --git a/lib/puppet/util/windows_system.rb b/lib/puppet/util/ > windows_system.rb > new file mode 100644 > index 0000000..83bc52e > --- /dev/null > +++ b/lib/puppet/util/windows_system.rb > @@ -0,0 +1,205 @@ > +if Facter.operatingsystem == "windows" > + require 'win32ole' > + require 'Win32API' > +end > + > +module ADSI > + def ADSI.connectable?(uri) > + begin > + adsi_obj = WIN32OLE.connect(uri) > + return adsi_obj != nil; > + rescue > + end > + > + return false > + end > +end > + > +module Windows Given where this file is located, I think the module should be Puppet::Util::Windows. You can alias it in your providers for less typing, but when I see 'Windows::User', I assume it's an external dependency. > > + class Resource > + def Resource.uri(resource_name) > + "#{Computer.resource_uri}/#{resource_name}" > + end > + end > + > + class User > + def initialize(username, native_adsi_obj = nil) > + @username = username > + @user = native_adsi_obj > + end > + > + def user > + @user = WIN32OLE.connect(User.resource_uri(@username)) > if @user == > nil > + return @user > + end > + > + def password_is?(password) > + fLOGON32_LOGON_NETWORK_CLEARTEXT = 8 > + fLOGON32_PROVIDER_DEFAULT = 0 > + > + logon_user = Win32API.new("advapi32", "LogonUser", > ['P', 'P', 'P', > 'L', 'L', 'P'], 'L') > + close_handle = Win32API.new("kernel32", "CloseHandle", > ['P'], 'V') > + > + token = ' ' * 4 > + if logon_user.call(@username, "", password, > fLOGON32_LOGON_NETWORK_CLEARTEXT, fLOGON32_PROVIDER_DEFAULT, token) == > 1 > + close_handle.call(token.unpack('L')[0]) > + return true > + end > + > + return false > + end > + > + def add_flag(flag_name, value) > + flag = 0 > + > + begin > + flag = user.Get(flag_name) > + rescue > + end > + > + user.Put(flag_name, flag | value) > + user.SetInfo > + end > + > + def password=(password) > + user.SetPassword(password) > + user.SetInfo > + > + fADS_UF_DONT_EXPIRE_PASSWD = 0x10000 > + add_flag("UserFlags", fADS_UF_DONT_EXPIRE_PASSWD) > + end > + > + def groups > + groups = [] > + user.Groups.each {|group| groups << group.name } > + return groups > + end > + > + def add_to_groups(group_names) > + group_names.each {|name| > Group.new(name).add_user(@username) } if > group_names.length > 0 > + end > + > + def remove_from_groups(group_names) > + group_names.each {|name| > Group.new(name).remove_user(@username) } > if group_names.length > 0 > + end > + > + def set_groups(names, minimal = true) > + return if names == nil || names.strip.length == 0 > + > + names = names.strip.split(',') > + current_groups = groups > + > + names.find_all {|name| !current_groups.include?(name) > }.tap {| > names_to_add| add_to_groups(names_to_add) } > + current_groups.find_all {|name| !names.include?(name) > }.tap {| > names_to_remove| remove_from_groups(names_to_remove) } if minimal == > false > + end > + > + def User.resource_uri(username) > + return "#{Resource.uri(username)},user" > + end > + > + def User.exists?(username) > + return ADSI::connectable?(User.resource_uri(username)) > + end > + > + def User.create(username, password) > + User.new(username, Computer.create("user", > username)).tap {|user| > user.password = password; yield user if block_given? } > + end > + > + def User.delete(username) > + Computer.delete("user", username) > + end > + end > + > + class Group > + def initialize(groupname, native_adsi_obj = nil) > + @groupname = groupname > + @group = native_adsi_obj > + end > + > + def resource_uri > + Group.resource_uri(@groupname) > + end > + > + def Group.resource_uri(name) > + "#{Resource.uri(name)},group" > + end > + > + def group > + @group = WIN32OLE.connect(resource_uri) if @group == nil > + return @group > + end > + > + def add_user(username) > + group.Add(User.resource_uri(username)) > + group.SetInfo > + end > + > + def remove_user(username) > + group.Remove(User.resource_uri(username)) > + group.SetInfo > + end > + > + def add_member(name) > + group.Add(Resource.uri(name)) > + group.SetInfo > + end > + > + def remove_member(name) > + group.Remove(Resource.uri(name)) > + group.SetInfo > + end > + > + def members > + list = [] > + group.Members.each {|member| list << member.Name } > + list > + end > + > + def set_members(members) > + return nil if members == nil || members.length == 0 > + > + current_members = self.members > + > + members.inject([]) {|members_to_add, member| > current_members.include?(member) ? members_to_add : members_to_add << > member }.each {|member| add_member(member) } > + current_members.inject([]) {|members_to_remove, member| > members.include?(member) ? members_to_remove : members_to_remove << > member }.each {|member| remove_member(member) } > + end > + > + def Group.create(name) > + Windows::Group.new(name, Computer.create("group", > name)).tap {| > group| yield group if block_given? } > + end > + > + def Group.exists?(name) > + return ADSI::connectable?(Group.resource_uri(name)) > + end > + > + def Group.delete(name) > + Computer.delete("group", name) > + end > + end This stuff all looks really good - minimal, straightforward, and provides a really clean interface for the providers. In fact, it might be possible to make this even simpler by making the Windows::User and Windows::Group classes into modules that could be included in the providers - there's a small amount of duplication as it stands, but this would allow your modules to implement the minimal provider API but outside the provider hierarchy, just like it currently is. > > + class Computer > + def Computer.name > + name = " " * 128 > + size = "128" > + > Win32API.new('kernel32','GetComputerName',['P','P'],'I').call > (name,size) > + return name.unpack("A*") > + end > + > + def Computer.resource_uri > + computer_name = Computer.name > + return "WinNT://#{computer_name}" > + end > + > + def Computer.api > + return WIN32OLE.connect(Computer.resource_uri) > + end > + > + def Computer.create(resource_type, name) > + Computer.api.create(resource_type, name).SetInfo > + end > + > + def Computer.delete(resource_type, name) > + Computer.api.Delete(resource_type, name) > + end > + end > +end > diff --git a/test/ral/providers/group_win.rb b/test/ral/providers/ > group_win.rb > new file mode 100644 > index 0000000..d341fbd > --- /dev/null > +++ b/test/ral/providers/group_win.rb > @@ -0,0 +1,39 @@ > +#!/usr/bin/env ruby As Paul mentioned, we're doing all new test development in RSpec, whose tests are in spec/. We're glad to help you figure out how to use it, though. I'll skip reviewing the tests until they're ported to RSpec. > > +require File.dirname(__FILE__) + '/../../lib/puppettest' > +require 'windowstest' > + > +require File.dirname(__FILE__) + '/../../../lib/puppet/provider/ > group/ > groupadd_win.rb' > + > +class TestGroupProvider < Test::Unit::TestCase > + include WindowsTest > + > + def group_provider(resource_configuration) > + Puppet::Type::Group::ProviderGroupadd_win.new.tap {|provider| > provider.resource = resource_configuration } > + end > + > + def test_groupGetsCreated > + groupname = "randomgroup" > + register_group groupname > + > + expected_members = ["test1", "test2"] > + mkusers(expected_members) > + > + provider = group_provider :name => groupname, :members => ['test1', > 'test2'] > + > + assert_nothing_raised { provider.create } > + assert_no_missing_member(group(groupname), expected_members) > + end > + > + def test_groupMembersGetSet > + groupname = "randomgroup" > + group = mkgroup(groupname) > + expected_members = ["test1", "test2"] > + mkusers(expected_members) > + > + provider = group_provider :name => groupname, :members => ['test1', > 'test2'] > + > + assert_nothing_raised { provider.members = ['test1', 'test2'] } > + assert_no_missing_member(group, expected_members) > + end > +end > diff --git a/test/ral/providers/syslog.rb b/test/ral/providers/ > syslog.rb > new file mode 100644 > index 0000000..396b45c > --- /dev/null > +++ b/test/ral/providers/syslog.rb > @@ -0,0 +1,4 @@ > +# The purpose of this file is merely to fake the syslog module on > windows. > +# I placed it here since I was running the tests from here. > +# I'll probably leave the file here until I wrap the windows event > log in a syslogish module for windows. > +# I haven't thought about it much. I'm open to better ideas too. > \ No newline at end of file > diff --git a/test/ral/providers/user/useradd_win.rb b/test/ral/ > providers/user/useradd_win.rb > new file mode 100644 > index 0000000..11d31e7 > --- /dev/null > +++ b/test/ral/providers/user/useradd_win.rb > @@ -0,0 +1,63 @@ > +#!/usr/bin/env ruby > + > +require File.dirname(__FILE__) + '/../../../lib/puppettest' > +require 'windowstest' > + > +require File.dirname(__FILE__) + '/../../../../lib/puppet/provider/ > user/useradd_win.rb' > + > +class TestUserProvider < Test::Unit::TestCase > + include WindowsTest > + > + def user_provider(resource_configuration) > + Puppet::Type::User::ProviderUseradd_win.new.tap {|provider| > provider.resource = resource_configuration } > + end > + > + def test_userIsCreated > + expected_groups = ["randomgroup1", "randomgroup2"] > + mkgroups(expected_groups) > + > + username = "testuser" > + register_user username > + > + password = "1234" > + > + provider = user_provider :name => username, :password => > password, :groups => expected_groups.join(",") > + > + assert_nothing_raised { provider.create } > + > + user = Windows::User.new(username) > + assert(user.password_is?(password), "Password of user > #{username} > should be #{password}") > + user.groups.tap {|groups| > + expected_groups.each {|expected_group| > assert(groups.include? > (expected_group), "User should be a member of #{expected_group}") } > + assert(expected_groups.length == groups.length, "The > user should > be a member of #{expected_groups.length} groups.") > + } > + end > + > + def test_userGroupsAreSet > + expected_groups = ["randomgroup1", "randomgroup2"] > + mkgroups expected_groups > + > + username = "testuser" > + mkuser username > + > + provider = user_provider :name => username > + provider.groups = expected_groups.join(",") > + > + provider.groups.split(',').collect {|group| group.strip }.tap {| > groups| > + assert(groups.length == expected_groups.length, "The > user should > be a member of #{expected_groups.length} groups.") > + groups.each {|group| > assert(expected_groups.include?(group), "The > user should be a member of #{group}") } > + } > + end > + > + def test_usersPasswordIsSet > + username = "testuser" > + password = "11112222" > + > + user = mkuser username, password > + > + provider = user_provider :name => username, :password => > password > + provider.password = password > + > + assert(user.password_is?(password), "User #{username}'s password > should be #{password}.") > + end > +end > diff --git a/test/ral/providers/windowstest.rb b/test/ral/providers/ > windowstest.rb > new file mode 100644 > index 0000000..b9ed1e5 > --- /dev/null > +++ b/test/ral/providers/windowstest.rb > @@ -0,0 +1,95 @@ > +require File.dirname(__FILE__) + '/../../../lib/puppet/util/ > windows_system.rb' > + > +module WindowsTest > + class List > + def initialize > + @list = [] > + end > + > + def clear > + destroy > + @list = [] > + end > + > + def register(item) > + @list << item > + end > + end > + > + class Groups < List > + def destroy > + @list.each {|group| > + begin > + Windows::Group.delete(group) > + rescue > + puts "Group #{group} not found" > + end > + } > + end > + end > + > + class Users < List > + def destroy > + @list.each {|user| > + begin > + Windows::User.delete(user) > + rescue > + puts "User #{user} not found" > + end > + } > + end > + end > + > + def helper_users > + @users = Users.new if @users == nil > + @users > + end > + > + def helper_groups > + @groups = Groups.new if @groups == nil > + @groups > + end > + > + def clear > + helper_groups.clear > + helper_users.clear > + end > + > + def register_group(name) > + helper_groups.register name > + end > + > + def register_user(name) > + helper_users.register name > + end > + > + def mkuser(name, password = "1234567") > + Windows::User.create(name, password) { register_user name } > + end > + > + def mkgroup(name) > + Windows::Group.create(name) { register_group name } > + end > + > + def mkusers(names) > + names.collect {|name| mkuser name } > + end > + > + def mkgroups(names) > + names.collect {|name| mkgroup name } > + end > + > + def group(name) > + Windows::Group.new(name) > + end > + > + def assert_no_missing_member(group, expected_members) > + group.members.tap {|members| > + expected_members.each {|member| > assert(members.include?(member), "# > {member} should be a member") } > + } > + end > + > + def teardown > + clear > + end > +end > > > -- I cannot and will not cut my conscience to fit this year's fashions. -- Lillian Hellman --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
