On Sun, Mar 22, 2009 at 2:24 AM, Luke Kanies <[email protected]> wrote:
>
> 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?
Done. You should be able to see my changes now.
>
> All of the traps should be in the daemon.rb class, I think.
I haven't fixed the traps in daemon.rb. I guess I'm being driven right
now by whether or not I can run puppet. If I don't hit a choke point,
I don't fix it, yet. And after rebasing with the master, I haven't hit
any issues with the traps in daemon.rb.
> > 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
Done.
> > 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.
Will do.
> > 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.
Done. Not as cleanly as I would like, but I'll refactor in a bit.
> > + 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.
Very interesting idea.
> > + 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.
I haven't moved the tests to rspec yet, but I will before adding any
more providers.
> > +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
-~----------~----~----~----~------~----~------~--~---
