Fix Bug #1629

A refactoring of ssh_authorized_key parsed provider was needed and tests
were improved. flush method has been split for clarity.

2 files changed, 116 insertions, 12 deletions

diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb
index 77af58ef5..5604ba32a 100644
--- a/lib/puppet/provider/ssh_authorized_key/parsed.rb
+++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb
@@ -40,25 +40,55 @@ Puppet::Type.type(:ssh_authorized_key).provide(:parsed,
         # This was done in the type class but path expansion was failing for
         # not yet existing users, the only workaround I found was to move that
         # in the provider.
-        if user = @resource.should(:user)
-            target = File.expand_path("~%s/.ssh/authorized_keys" % user)
-            @property_hash[:target] = target
-            @resource[:target] = target
-        end
+        @resource[:target] = target
 
         super
     end
 
+    def target
+        if user
+            File.expand_path("~%s/.ssh/authorized_keys" % user)
+        elsif target = @resource.should(:target)
+            target
+        end
+    end
+
+    def user
+        @resource.should(:user)
+    end
+
+    def dir_perm
+        # Determine correct permission for created directory and file
+        # we can afford more restrictive permissions when the user is known
+        if target
+            if user
+                0700
+            else
+                0755
+            end
+        end
+    end
+
+    def file_perm
+        if target
+            if user
+                0600
+            else
+                0644
+            end
+        end
+    end
+
     def flush
         # As path expansion had to be moved in the provider, we cannot generate new file
         # resources and thus have to chown and chmod here. It smells hackish.
-        
+
         # Create target's parent directory if nonexistant
-        if target = @property_hash[:target]
-            dir = File.dirname(@property_hash[:target])
+        if target
+            dir = File.dirname(target)
             if not File.exist? dir
                 Puppet.debug("Creating directory %s which did not exist" % dir)
-                Dir.mkdir(dir, 0700)
+                Dir.mkdir(dir, dir_perm)
             end
         end
 
@@ -66,9 +96,19 @@ Puppet::Type.type(:ssh_authorized_key).provide(:parsed,
         super
 
         # Ensure correct permissions
-        if target and user = @property_hash[:user]
-            File.chown(Puppet::Util.uid(user), nil, dir)
-            File.chown(Puppet::Util.uid(user), nil, @property_hash[:target])
+        if target and user
+            uid = Puppet::Util.uid(user)
+
+            if uid
+                File.chown(uid, nil, dir)
+                File.chown(uid, nil, target)
+            else
+                raise Puppet::Error, "Specified user does not exist"
+            end
+        end
+
+        if target
+            File.chmod(file_perm, target)
         end
     end
 
diff --git a/spec/unit/provider/ssh_authorized_key/parsed.rb b/spec/unit/provider/ssh_authorized_key/parsed.rb
index bf0697eb8..ec8f2b96a 100755
--- a/spec/unit/provider/ssh_authorized_key/parsed.rb
+++ b/spec/unit/provider/ssh_authorized_key/parsed.rb
@@ -100,3 +100,67 @@ describe provider_class do
         @provider.parse_options(optionstr).should == options
     end
 end
+
+describe provider_class do
+    before :each do
+        @resource = stub("resource", :name => "foo")
+        @resource.stubs(:[]).returns "foo"
+        @provider = provider_class.new(@resource)
+    end
+
+    describe "when flushing" do
+        before :each do
+            # Stub file and directory operations
+            Dir.stubs(:mkdir)
+            File.stubs(:chmod)
+            File.stubs(:chown)
+        end
+
+        describe "and a user has been specified" do
+            before :each do
+                @resource.stubs(:should).with(:user).returns "nobody"
+                @resource.stubs(:should).with(:target).returns nil
+           end
+
+            it "should create the directory" do
+                Dir.expects(:mkdir).with(File.expand_path("~nobody/.ssh"), 0700)
+                @provider.flush
+            end
+
+            it "should chown the directory to the user" do
+                uid = Puppet::Util.uid("nobody")
+                File.expects(:chown).with(uid, nil, File.expand_path("~nobody/.ssh"))
+                @provider.flush
+            end
+
+            it "should chown the key file to the user" do
+                uid = Puppet::Util.uid("nobody")
+                File.expects(:chown).with(uid, nil, File.expand_path("~nobody/.ssh/authorized_keys"))
+                @provider.flush
+            end
+
+            it "should chmod the key file to 0600" do
+                File.chmod(0600, File.expand_path("~nobody/.ssh/authorized_keys"))
+                @provider.flush
+            end
+        end
+
+        describe "and a target has been specified" do
+            before :each do
+                @resource.stubs(:should).with(:user).returns nil
+                @resource.stubs(:should).with(:target).returns "/tmp/.ssh/authorized_keys"
+            end
+
+            it "should make the directory" do
+                Dir.expects(:mkdir).with("/tmp/.ssh", 0755)
+                @provider.flush
+            end
+
+            it "should chmod the key file to 0644" do
+                File.expects(:chmod).with(0644, "/tmp/.ssh/authorized_keys")
+                @provider.flush
+            end
+        end
+
+    end
+end