Fix for #2736, target doesn't work for ssh_authorized_keys

There were a number of problems here (duplicated code, meaningless
tests, etc.) but the core was that the last definition of target
ignored the provided value if there was also a user specified.

Signed-off-by: Markus Roberts <Markus@reality.com>

2 files changed, 29 insertions, 86 deletions

diff --git a/lib/puppet/provider/ssh_authorized_key/parsed.rb b/lib/puppet/provider/ssh_authorized_key/parsed.rb
index 6df7f8ae0..28a68b364 100644
--- a/lib/puppet/provider/ssh_authorized_key/parsed.rb
+++ b/lib/puppet/provider/ssh_authorized_key/parsed.rb
@@ -36,14 +36,6 @@ Puppet::Type.type(:ssh_authorized_key).provide(:parsed,
         :rts      => /^\s+/,
         :match    => /^(?:(.+) )?(\d+) (\d+) (\d+)(?: (.+))?$/
 
-    def target
-        @resource.should(:target)
-    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
@@ -67,39 +59,13 @@ Puppet::Type.type(:ssh_authorized_key).provide(:parsed,
     end
 
     def target
-        if user
-            File.expand_path("~%s/.ssh/authorized_keys" % user)
-        elsif target = @resource.should(:target)
-            target
-        end
+        @resource.should(:target) || File.expand_path("~%s/.ssh/authorized_keys" % user)
     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.
diff --git a/spec/unit/provider/ssh_authorized_key/parsed.rb b/spec/unit/provider/ssh_authorized_key/parsed.rb
index 0f32a6f12..ade738b32 100755
--- a/spec/unit/provider/ssh_authorized_key/parsed.rb
+++ b/spec/unit/provider/ssh_authorized_key/parsed.rb
@@ -95,85 +95,61 @@ describe provider_class do
             File.stubs(:chown)
         end
 
-        describe "and a user has been specified" do
+        describe "and both a user and a target have been specified" do
             before :each do
-                @resource.stubs(:should).with(:user).returns "nobody"
-                target = File.expand_path("~nobody/.ssh/authorized_keys")
+                Puppet::Util.stubs(:uid).with("random_bob").returns 12345
+                @resource.stubs(:should).with(:user).returns "random_bob"
+                target = "/tmp/.ssh_dir/place_to_put_authorized_keys"
                 @resource.stubs(:should).with(:target).returns target
            end
 
             it "should create the directory" do
-                Dir.expects(:mkdir).with(File.expand_path("~nobody/.ssh"), 0700)
+                File.stubs(:exist?).with("/tmp/.ssh_dir").returns false
+                Dir.expects(:mkdir).with("/tmp/.ssh_dir", 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"))
+                uid = Puppet::Util.uid("random_bob")
+                File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir")
                 @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"))
+                uid = Puppet::Util.uid("random_bob")
+                File.expects(:chown).with(uid, nil, "/tmp/.ssh_dir/place_to_put_authorized_keys")
                 @provider.flush
             end
 
             it "should chmod the key file to 0600" do
-                File.expects(:chmod).with(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")
+                File.expects(:chmod).with(0600, "/tmp/.ssh_dir/place_to_put_authorized_keys")
                 @provider.flush
             end
         end
 
-    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
+        describe "and a user has been specified with no target" do
             before :each do
                 @resource.stubs(:should).with(:user).returns "nobody"
                 @resource.stubs(:should).with(:target).returns nil
+                # 
+                # I'd like to use random_bob here and something like
+                #
+                #    File.stubs(:expand_path).with("~random_bob/.ssh").returns "/users/r/random_bob/.ssh"
+                #
+                # but mocha objects strenuously to stubbing File.expand_path
+                # so I'm left with using nobody.
+                @dir = File.expand_path("~nobody/.ssh")
            end
 
             it "should create the directory" do
-                Dir.expects(:mkdir).with(File.expand_path("~nobody/.ssh"), 0700)
+                File.stubs(:exist?).with(@dir).returns false
+                Dir.expects(:mkdir).with(@dir,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"))
+                File.expects(:chown).with(uid, nil, @dir)
                 @provider.flush
             end
 
@@ -189,19 +165,20 @@ describe provider_class do
             end
         end
 
-        describe "and a target has been specified" do
+        describe "and a target has been specified with no user" do
             before :each do
                 @resource.stubs(:should).with(:user).returns nil
-                @resource.stubs(:should).with(:target).returns "/tmp/.ssh/authorized_keys"
+                @resource.stubs(:should).with(:target).returns("/tmp/.ssh_dir/place_to_put_authorized_keys")
             end
 
             it "should make the directory" do
-                Dir.expects(:mkdir).with("/tmp/.ssh", 0755)
+                File.stubs(:exist?).with("/tmp/.ssh_dir").returns false
+                Dir.expects(:mkdir).with("/tmp/.ssh_dir", 0755)
                 @provider.flush
             end
 
             it "should chmod the key file to 0644" do
-                File.expects(:chmod).with(0644, "/tmp/.ssh/authorized_keys")
+                File.expects(:chmod).with(0644, "/tmp/.ssh_dir/place_to_put_authorized_keys")
                 @provider.flush
             end
         end