12 files changed, 258 insertions, 56 deletions

diff --git a/lib/puppet/file_serving/configuration.rb b/lib/puppet/file_serving/configuration.rb
index 03be1b9dd..ccf0957d1 100644
--- a/lib/puppet/file_serving/configuration.rb
+++ b/lib/puppet/file_serving/configuration.rb
@@ -28,6 +28,14 @@ class Puppet::FileServing::Configuration
 
     private_class_method  :new
 
+    # Verify that the client is allowed access to this file.
+    def authorized?(file, options = {})
+        mount, file_path = split_path(file, options[:node])
+        # If we're not serving this mount, then access is denied.
+        return false unless mount
+        return mount.allowed?(options[:node], options[:ipaddress])
+    end
+
     # Search for a file.
     def file_path(key, options = {})
         mount, file_path = split_path(key, options[:node])
@@ -81,6 +89,7 @@ class Puppet::FileServing::Configuration
             return
         end
 
+        # Don't assign the mounts hash until we're sure the parsing succeeded.
         begin
             newmounts = @parser.parse
             @mounts = newmounts
diff --git a/lib/puppet/file_serving/terminus_selector.rb b/lib/puppet/file_serving/terminus_selector.rb
index 5952cfffa..aa08f087e 100644
--- a/lib/puppet/file_serving/terminus_selector.rb
+++ b/lib/puppet/file_serving/terminus_selector.rb
@@ -12,7 +12,7 @@ module Puppet::FileServing::TerminusSelector
     PROTOCOL_MAP = {"puppet" => :rest, "file" => :local, "puppetmounts" => :file_server}
 
     # Pick an appropriate terminus based on the protocol.
-    def select_terminus(full_uri)
+    def select_terminus(full_uri, options = {})
         # Short-circuit to :local if it's a fully-qualified path.
         return PROTOCOL_MAP["file"] if full_uri =~ /^#{::File::SEPARATOR}/
         begin
@@ -29,8 +29,14 @@ module Puppet::FileServing::TerminusSelector
             terminus = :file_server
         end
 
-        if uri.path =~ /^\/modules\b/ and terminus == :file_server
-            terminus = :modules
+        if terminus == :file_server and uri.path =~ %r{^/([^/]+)\b}
+            modname = $1
+            if modname == "modules"
+                terminus = :modules
+            elsif terminus(:modules).find_module(modname, options[:node])
+                Puppet.warning "DEPRECATION NOTICE: Found file '%s' in module without using the 'modules' mount; please prefix path with '/modules'" % uri.path
+                terminus = :modules
+            end
         end
 
         return terminus
diff --git a/lib/puppet/indirector/file_server.rb b/lib/puppet/indirector/file_server.rb
index 1b2e047e8..51e53d8c9 100644
--- a/lib/puppet/indirector/file_server.rb
+++ b/lib/puppet/indirector/file_server.rb
@@ -10,16 +10,19 @@ require 'puppet/indirector/terminus'
 class Puppet::Indirector::FileServer < Puppet::Indirector::Terminus
     include Puppet::Util::URIHelper
 
+    # Is the client authorized to perform this action?
+    def authorized?(method, key, options = {})
+        return false unless [:find, :search].include?(method)
+
+        uri = key2uri(key)
+
+        configuration.authorized?(uri.path, :node => options[:node], :ipaddress => options[:ipaddress])
+    end
+
     # Find our key using the fileserver.
     def find(key, options = {})
         uri = key2uri(key)
 
-        # First try the modules mount, at least for now.
-        if instance = indirection.terminus(:modules).find(key, options)
-            Puppet.warning "DEPRECATION NOTICE: Found file in module without using the 'modules' mount; please fix"
-            return instance
-        end
-
         return nil unless path = configuration.file_path(uri.path, :node => options[:node]) and FileTest.exists?(path)
 
         return model.new(path)
diff --git a/lib/puppet/indirector/indirection.rb b/lib/puppet/indirector/indirection.rb
index 313117b25..f464f846f 100644
--- a/lib/puppet/indirector/indirection.rb
+++ b/lib/puppet/indirector/indirection.rb
@@ -104,7 +104,7 @@ class Puppet::Indirector::Indirection
         # of URI that the indirection can use for routing to the appropriate
         # terminus.
         if respond_to?(:select_terminus)
-            terminus_name = select_terminus(key)
+            terminus_name = select_terminus(key, *args)
         else
             terminus_name = terminus_class
         end
diff --git a/lib/puppet/indirector/module_files.rb b/lib/puppet/indirector/module_files.rb
index e0374d7a4..739d7b7b5 100644
--- a/lib/puppet/indirector/module_files.rb
+++ b/lib/puppet/indirector/module_files.rb
@@ -4,11 +4,24 @@
 
 require 'puppet/util/uri_helper'
 require 'puppet/indirector/terminus'
+require 'puppet/file_serving/configuration'
 
 # Look files up in Puppet modules.
 class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus
     include Puppet::Util::URIHelper
 
+    # Is the client allowed access to this key with this method?
+    def authorized?(method, key, options = {})
+        return false unless [:find, :search].include?(method)
+
+        uri = key2uri(key)
+
+        # Make sure our file path starts with /modules
+        path = uri.path =~ /^\/modules/ ? uri.path : "/modules" + uri.path
+
+        configuration.authorized?(path, :node => options[:node], :ipaddress => options[:ipaddress])
+    end
+
     # Find our key in a module.
     def find(key, options = {})
         uri = key2uri(key)
@@ -27,7 +40,17 @@ class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus
         return model.new(path)
     end
 
+    # Try to find our module.
+    def find_module(module_name, node_name)
+        Puppet::Module::find(module_name, environment(node_name))
+    end
+
     private
+
+    # Our fileserver configuration, if needed.
+    def configuration
+        Puppet::FileServing::Configuration.create
+    end
     
     # Determine the environment to use, if any.
     def environment(node_name)
@@ -39,9 +62,4 @@ class Puppet::Indirector::ModuleFiles < Puppet::Indirector::Terminus
             nil
         end
     end
-
-    # Try to find our module.
-    def find_module(module_name, node_name)
-        Puppet::Module::find(module_name, environment(node_name))
-    end
 end
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