Fix #2753 - Do not "global allow" plugins/modules mount if some rules have been parsed

When fixing #2424, we were adding a global allow (ie allow(*)) to
the plugins/modules mount.
Unfortunately global allow always win against any other rules that
can be defined in fileserver.conf.

This patch makes sure we add those global allow entries only if
we didn't get any rules from fileserver.conf

Signed-off-by: Brice Figureau <brice-puppet@daysofwonder.com>

4 files changed, 53 insertions, 4 deletions

diff --git a/lib/puppet/file_serving/configuration.rb b/lib/puppet/file_serving/configuration.rb
index ac54a7a8a..9034caed1 100644
--- a/lib/puppet/file_serving/configuration.rb
+++ b/lib/puppet/file_serving/configuration.rb
@@ -96,9 +96,9 @@ class Puppet::FileServing::Configuration
 
     def mk_default_mounts
         @mounts["modules"] ||= Mount::Modules.new("modules")
-        @mounts["modules"].allow('*')
+        @mounts["modules"].allow('*') if @mounts["modules"].empty?
         @mounts["plugins"] ||= Mount::Plugins.new("plugins")
-        @mounts["plugins"].allow('*')
+        @mounts["plugins"].allow('*') if @mounts["plugins"].empty?
     end
 
     # Read the configuration file.
diff --git a/lib/puppet/network/authstore.rb b/lib/puppet/network/authstore.rb
index ab31faec8..fb3d0145b 100755
--- a/lib/puppet/network/authstore.rb
+++ b/lib/puppet/network/authstore.rb
@@ -63,6 +63,11 @@ module Puppet
             @globalallow
         end
 
+        # does this auth store has any rules?
+        def empty?
+            @globalallow.nil? && @declarations.size == 0
+        end
+
         def initialize
             @globalallow = nil
             @declarations = []
diff --git a/spec/unit/file_serving/configuration.rb b/spec/unit/file_serving/configuration.rb
index f6acfad39..4621a0c82 100755
--- a/spec/unit/file_serving/configuration.rb
+++ b/spec/unit/file_serving/configuration.rb
@@ -104,17 +104,31 @@ describe Puppet::FileServing::Configuration do
 
         it "should allow all access to modules and plugins if no fileserver.conf exists" do
             FileTest.expects(:exists?).returns false # the file doesn't exist
-            modules = stub 'modules'
+            modules = stub 'modules', :empty? => true
             Puppet::FileServing::Mount::Modules.stubs(:new).returns(modules)
             modules.expects(:allow).with('*')
 
-            plugins = stub 'plugins'
+            plugins = stub 'plugins', :empty? => true
             Puppet::FileServing::Mount::Plugins.stubs(:new).returns(plugins)
             plugins.expects(:allow).with('*')
 
             Puppet::FileServing::Configuration.create
         end
 
+        it "should not allow access from all to modules and plugins if the fileserver.conf provided some rules" do
+            FileTest.expects(:exists?).returns false # the file doesn't exist
+
+            modules = stub 'modules', :empty? => false
+            Puppet::FileServing::Mount::Modules.stubs(:new).returns(modules)
+            modules.expects(:allow).with('*').never
+
+            plugins = stub 'plugins', :empty? => false
+            Puppet::FileServing::Mount::Plugins.stubs(:new).returns(plugins)
+            plugins.expects(:allow).with('*').never
+
+            Puppet::FileServing::Configuration.create
+        end
+
         it "should add modules and plugins mounts even if they are not returned by the parser" do
             @parser.expects(:parse).returns("one" => mock("mount"))
             FileTest.expects(:exists?).returns true # the file doesn't exist
diff --git a/spec/unit/network/authstore.rb b/spec/unit/network/authstore.rb
index 55b2c7bbc..4087b28ed 100644
--- a/spec/unit/network/authstore.rb
+++ b/spec/unit/network/authstore.rb
@@ -4,6 +4,36 @@ require File.dirname(__FILE__) + '/../../spec_helper'
 
 require 'puppet/network/authconfig'
 
+describe Puppet::Network::AuthStore do
+    describe "when checking if the acl has some entries" do
+        before :each do
+            @authstore = Puppet::Network::AuthStore.new
+        end
+
+        it "should be empty if no ACE have been entered" do
+            @authstore.should be_empty
+        end
+
+        it "should not be empty if it is a global allow" do
+            @authstore.allow('*')
+
+            @authstore.should_not be_empty
+        end
+
+        it "should not be empty if at least one allow has been entered" do
+            @authstore.allow('1.1.1.*')
+
+            @authstore.should_not be_empty
+        end
+
+        it "should not be empty if at least one deny has been entered" do
+            @authstore.deny('1.1.1.*')
+
+            @authstore.should_not be_empty
+        end
+    end
+end
+
 describe Puppet::Network::AuthStore::Declaration do
 
     ['100.101.99.98','100.100.100.100','1.2.3.4','11.22.33.44'].each { |ip|