Adding authorization hooks to the file_server and

module_files indirection terminus types.  Both hooks
use the fileserver configuration, but the module_files
hook only uses the 'modules' mount.

Also moved all responsibility for knowing whether to
use the 'modules' terminus type to the terminus selector;
it was previously spread between that and the file_server
terminus, which made some things annoyingly complicated.
This normalizes the deprecation notices and the logic about
how we make these decisions.

7 files changed, 207 insertions, 41 deletions

diff --git a/spec/lib/shared_behaviours/file_server_terminus.rb b/spec/lib/shared_behaviours/file_server_terminus.rb
index 93634e7dc..c18d74f81 100644
--- a/spec/lib/shared_behaviours/file_server_terminus.rb
+++ b/spec/lib/shared_behaviours/file_server_terminus.rb
@@ -38,12 +38,4 @@ describe "Puppet::Indirector::FileServerTerminus", :shared => true do
 
         @terminus.find("puppetmounts://myhost/one/my/file").should == :myinstance
     end
-
-    it "should try to use the modules terminus to find files" do
-        path = "puppetmounts://myhost/one/my/file"
-        @modules.stubs(:find).with(path, {}).returns(:myinstance)
-        @terminus.indirection.stubs(:terminus).with(:modules).returns(@modules)
-
-        @terminus.find("puppetmounts://myhost/one/my/file").should == :myinstance
-    end
 end
diff --git a/spec/lib/shared_behaviours/file_serving.rb b/spec/lib/shared_behaviours/file_serving.rb
index 5c1d87015..b0aa14fc0 100644
--- a/spec/lib/shared_behaviours/file_serving.rb
+++ b/spec/lib/shared_behaviours/file_serving.rb
@@ -21,9 +21,11 @@ describe "Puppet::FileServing::Files", :shared => true do
     it "should use the file_server terminus when the 'puppet' URI scheme is used, no host name is present, and the process name is 'puppet'" do
         uri = "puppet:///mymod/my/file"
         Puppet.settings.stubs(:value).with(:name).returns("puppet")
+        Puppet.settings.stubs(:value).with(:modulepath, nil).returns("")
         Puppet.settings.stubs(:value).with(:modulepath).returns("")
         Puppet.settings.stubs(:value).with(:libdir).returns("")
         Puppet.settings.stubs(:value).with(:fileserverconfig).returns("/whatever")
+        Puppet.settings.stubs(:value).with(:environment).returns("")
         @indirection.terminus(:file_server).expects(:find).with(uri)
         @test_class.find(uri)
     end
diff --git a/spec/unit/file_serving/configuration.rb b/spec/unit/file_serving/configuration.rb
index d491447e9..df46b9b6a 100755
--- a/spec/unit/file_serving/configuration.rb
+++ b/spec/unit/file_serving/configuration.rb
@@ -177,3 +177,51 @@ describe Puppet::FileServing::Configuration, " when finding files" do
         @config.file_path("/one/something").should be_nil
     end
 end
+
+describe Puppet::FileServing::Configuration, " when checking authorization" do
+    include FSConfigurationTesting
+
+    before do
+        @parser = mock 'parser'
+        @parser.stubs(:changed?).returns true
+        FileTest.stubs(:exists?).with(@path).returns(true)
+        Puppet::FileServing::Configuration::Parser.stubs(:new).returns(@parser)
+
+        @mount1 = stub 'mount', :name => "one"
+        @mounts = {"one" => @mount1}
+        @parser.stubs(:parse).returns(@mounts)
+
+        Facter.stubs(:value).with("hostname").returns("whatever")
+
+        @config = Puppet::FileServing::Configuration.create
+    end
+
+    it "should return false if the mount cannot be found" do
+        @config.authorized?("/nope/my/file").should be_false
+    end
+
+    it "should use the mount to determine authorization" do
+        @mount1.expects(:allowed?)
+        @config.authorized?("/one/my/file")
+    end
+
+    it "should pass the client's name to the mount if provided" do
+        @mount1.expects(:allowed?).with("myhost", nil)
+        @config.authorized?("/one/my/file", :node => "myhost")
+    end
+
+    it "should pass the client's IP to the mount if provided" do
+        @mount1.expects(:allowed?).with("myhost", "myip")
+        @config.authorized?("/one/my/file", :node => "myhost", :ipaddress => "myip")
+    end
+
+    it "should return true if the mount allows the client" do
+        @mount1.expects(:allowed?).returns(true)
+        @config.authorized?("/one/my/file").should be_true
+    end
+
+    it "should return false if the mount denies the client"  do
+        @mount1.expects(:allowed?).returns(false)
+        @config.authorized?("/one/my/file").should be_false
+    end
+end
diff --git a/spec/unit/file_serving/terminus_selector.rb b/spec/unit/file_serving/terminus_selector.rb
index c5520e2f8..046f71bdc 100755
--- a/spec/unit/file_serving/terminus_selector.rb
+++ b/spec/unit/file_serving/terminus_selector.rb
@@ -30,24 +30,36 @@ describe Puppet::FileServing::TerminusSelector, " when being used to select term
         @object.select_terminus("puppet://host/module/file").should == :rest
     end
 
-    it "should choose :modules when the protocol is 'puppetmounts' and the mount name is 'modules'" do
-        @object.select_terminus("puppetmounts://host/modules/mymod/file").should == :modules
-    end
-
-    it "should choose :modules when no server name is provided, the process name is 'puppet', and the mount name is 'modules'" do
-        Puppet.settings.expects(:value).with(:name).returns("puppet")
-        @object.select_terminus("puppet:///modules/mymod/file").should == :modules
-    end
-
     it "should choose :file_server when the protocol is 'puppetmounts' and the mount name is not 'modules'" do
+        modules = mock 'modules'
+        @object.stubs(:terminus).with(:modules).returns(modules)
+        modules.stubs(:find_module).returns(nil)
+
         @object.select_terminus("puppetmounts://host/notmodules/file").should == :file_server
     end
 
     it "should choose :file_server when no server name is provided, the process name is 'puppet', and the mount name is not 'modules'" do
+        modules = mock 'modules'
+        @object.stubs(:terminus).with(:modules).returns(modules)
+        modules.stubs(:find_module).returns(nil)
+
         Puppet.settings.expects(:value).with(:name).returns("puppet")
         @object.select_terminus("puppet:///notmodules/file").should == :file_server
     end
 
+    it "should choose :modules if it would normally choose :file_server but the mount name is 'modules'" do
+        @object.select_terminus("puppetmounts://host/modules/mymod/file").should == :modules
+    end
+
+    it "should choose :modules it would normally choose :file_server but a module exists with the mount name" do
+        modules = mock 'modules'
+
+        @object.expects(:terminus).with(:modules).returns(modules)
+        modules.expects(:find_module).with("mymod", nil).returns(:thing)
+
+        @object.select_terminus("puppetmounts://host/mymod/file").should == :modules
+    end
+
     it "should choose :rest when no server name is provided and the process name is not 'puppet'" do
         Puppet.settings.expects(:value).with(:name).returns("puppetd")
         @object.select_terminus("puppet:///module/file").should == :rest
@@ -70,3 +82,29 @@ describe Puppet::FileServing::TerminusSelector, " when being used to select term
         proc { @object.select_terminus("http:///module/file") }.should raise_error(ArgumentError)
     end
 end
+
+describe Puppet::FileServing::TerminusSelector, " when looking for a module whose name matches the mount name" do
+    before do
+        @object = Object.new
+        @object.extend(Puppet::FileServing::TerminusSelector)
+
+        @modules = mock 'modules'
+        @object.stubs(:terminus).with(:modules).returns(@modules)
+    end
+
+    it "should use the modules terminus to look up the module" do
+        @modules.expects(:find_module).with("mymod", nil)
+        @object.select_terminus("puppetmounts://host/mymod/my/file")
+    end
+
+    it "should pass the node name to the modules terminus" do
+        @modules.expects(:find_module).with("mymod", nil)
+        @object.select_terminus("puppetmounts://host/mymod/my/file")
+    end
+
+    it "should log a deprecation warning if a module is found" do
+        @modules.expects(:find_module).with("mymod", nil).returns(:something)
+        Puppet.expects(:warning)
+        @object.select_terminus("puppetmounts://host/mymod/my/file")
+    end
+end
diff --git a/spec/unit/indirector/file_server.rb b/spec/unit/indirector/file_server.rb
index 5a53610e4..7bd55c650 100755
--- a/spec/unit/indirector/file_server.rb
+++ b/spec/unit/indirector/file_server.rb
@@ -35,29 +35,6 @@ end
 describe Puppet::Indirector::FileServer, " when finding files" do
     include FileServerTerminusTesting
 
-    it "should see if the modules terminus has the file" do
-        @module_server.expects(:find).with(@uri, {})
-        @configuration.stubs(:file_path)
-        @file_server.find(@uri)
-    end
-
-    it "should pass the client name to the modules terminus if one is provided" do
-        @module_server.expects(:find).with(@uri, :node => "mynode")
-        @configuration.stubs(:file_path)
-        @file_server.find(@uri, :node => "mynode")
-    end
-
-    it "should return any results from the modules terminus" do
-        @module_server.expects(:find).with(@uri, {}).returns(:myinstance)
-        @file_server.find(@uri).should == :myinstance
-    end
-
-    it "should produce a deprecation notice if it finds a file in the module terminus" do
-        @module_server.expects(:find).with(@uri, {}).returns(:myinstance)
-        Puppet.expects(:warning)
-        @file_server.find(@uri)
-    end
-
     it "should use the path portion of the URI as the file name" do
         @configuration.expects(:file_path).with("/my/local/file", :node => nil)
         @module_server.stubs(:find).returns(nil)
@@ -104,3 +81,49 @@ describe Puppet::Indirector::FileServer, " when returning file paths" do
 
     it "should ignore links if the links option is not set to follow"
 end
+
+describe Puppet::Indirector::FileServer, " when checking authorization" do
+    include FileServerTerminusTesting
+
+    it "should have an authorization hook" do
+        @file_server.should respond_to(:authorized?)
+    end
+
+    it "should deny the :destroy method" do
+        @file_server.authorized?(:destroy, "whatever").should be_false
+    end
+
+    it "should deny the :save method" do
+        @file_server.authorized?(:save, "whatever").should be_false
+    end
+
+    it "should use the file server configuration to determine authorization" do
+        @configuration.expects(:authorized?)
+        @file_server.authorized?(:find, "puppetmounts://host/my/file")
+    end
+
+    it "should pass the file path from the URI to the file server configuration" do
+        @configuration.expects(:authorized?).with { |uri, *args| uri == "/my/file" }
+        @file_server.authorized?(:find, "puppetmounts://host/my/file")
+    end
+
+    it "should pass the node name to the file server configuration" do
+        @configuration.expects(:authorized?).with { |key, options| options[:node] == "mynode" }
+        @file_server.authorized?(:find, "puppetmounts://host/my/file", :node => "mynode")
+    end
+
+    it "should pass the IP address to the file server configuration" do
+        @configuration.expects(:authorized?).with { |key, options| options[:ipaddress] == "myip" }
+        @file_server.authorized?(:find, "puppetmounts://host/my/file", :ipaddress => "myip")
+    end
+
+    it "should return false if the file server configuration denies authorization" do
+        @configuration.expects(:authorized?).returns(false)
+        @file_server.authorized?(:find, "puppetmounts://host/my/file").should be_false
+    end
+
+    it "should return true if the file server configuration approves authorization" do
+        @configuration.expects(:authorized?).returns(true)
+        @file_server.authorized?(:find, "puppetmounts://host/my/file").should be_true
+    end
+end
diff --git a/spec/unit/indirector/indirection.rb b/spec/unit/indirector/indirection.rb
index 9b1f556a6..57f672d9b 100755
--- a/spec/unit/indirector/indirection.rb
+++ b/spec/unit/indirector/indirection.rb
@@ -20,7 +20,7 @@ module IndirectionTesting
 end
 
 describe Puppet::Indirector::Indirection, " when initializing" do
-    # LAK:FIXME I've no idea how to test this, really.
+    # (LAK) I've no idea how to test this, really.
     it "should store a reference to itself before it consumes its options" do
         proc { @indirection = Puppet::Indirector::Indirection.new(Object.new, :testingness, :not_valid_option) }.should raise_error
         Puppet::Indirector::Indirection.instance(:testingness).should be_instance_of(Puppet::Indirector::Indirection)
@@ -247,6 +247,13 @@ describe Puppet::Indirector::Indirection, " when a select_terminus hook is avail
         @indirection.find(@uri).should == :whatever
     end
 
+    it "should pass all arguments to the :select_terminus hook" do
+        @indirection.expects(:select_terminus).with(@uri, :node => "johnny").returns(:other)
+        @other_terminus.stubs(:find)
+
+        @indirection.find(@uri, :node => "johnny")
+    end
+
     it "should pass the original key to the terminus rather than a modified key" do
         # This is the same test as before
         @other_terminus.expects(:find).with(@uri).returns(@result)
diff --git a/spec/unit/indirector/module_files.rb b/spec/unit/indirector/module_files.rb
index 2e1373748..a6c27b84b 100755
--- a/spec/unit/indirector/module_files.rb
+++ b/spec/unit/indirector/module_files.rb
@@ -94,3 +94,59 @@ describe Puppet::Indirector::ModuleFiles, " when returning file paths" do
 
     it "should ignore links if the links option is not set to follow"
 end
+
+describe Puppet::Indirector::ModuleFiles, " when authorizing" do
+    include ModuleFilesTerminusTesting
+
+    before do
+        @configuration = mock 'configuration'
+        Puppet::FileServing::Configuration.stubs(:create).returns(@configuration)
+    end
+
+    it "should have an authorization hook" do
+        @module_files.should respond_to(:authorized?)
+    end
+
+    it "should deny the :destroy method" do
+        @module_files.authorized?(:destroy, "whatever").should be_false
+    end
+
+    it "should deny the :save method" do
+        @module_files.authorized?(:save, "whatever").should be_false
+    end
+
+    it "should use the file server configuration to determine authorization" do
+        @configuration.expects(:authorized?)
+        @module_files.authorized?(:find, "puppetmounts://host/my/file")
+    end
+
+    it "should use the path directly from the URI if it already includes /modules" do
+        @configuration.expects(:authorized?).with { |uri, *args| uri == "/modules/my/file" }
+        @module_files.authorized?(:find, "puppetmounts://host/modules/my/file")
+    end
+
+    it "should add /modules to the file path if it's not included in the URI" do
+        @configuration.expects(:authorized?).with { |uri, *args| uri == "/modules/my/file" }
+        @module_files.authorized?(:find, "puppetmounts://host/my/file")
+    end
+
+    it "should pass the node name to the file server configuration" do
+        @configuration.expects(:authorized?).with { |key, options| options[:node] == "mynode" }
+        @module_files.authorized?(:find, "puppetmounts://host/my/file", :node => "mynode")
+    end
+
+    it "should pass the IP address to the file server configuration" do
+        @configuration.expects(:authorized?).with { |key, options| options[:ipaddress] == "myip" }
+        @module_files.authorized?(:find, "puppetmounts://host/my/file", :ipaddress => "myip")
+    end
+
+    it "should return false if the file server configuration denies authorization" do
+        @configuration.expects(:authorized?).returns(false)
+        @module_files.authorized?(:find, "puppetmounts://host/my/file").should be_false
+    end
+
+    it "should return true if the file server configuration approves authorization" do
+        @configuration.expects(:authorized?).returns(true)
+        @module_files.authorized?(:find, "puppetmounts://host/my/file").should be_true
+    end
+end