The idea is to raise an AuthorizationException at the same place we check the authorization instead of in an upper level to be able to spot where the authorization took place in the exception backtrace.
Moreover, this changes also makes Rights::allowed? to return the matching acl so that the upper layer can have a chance to report which ACL resulted in the match. Signed-off-by: Brice Figureau <[email protected]> --- lib/puppet/network/authconfig.rb | 5 +- lib/puppet/network/http/handler.rb | 14 ++- lib/puppet/network/rest_authconfig.rb | 9 ++- lib/puppet/network/rest_authorization.rb | 27 ++--- lib/puppet/network/rights.rb | 93 ++++++++++----- spec/integration/indirector/certificate/rest.rb | 2 +- .../indirector/certificate_request/rest.rb | 2 +- .../indirector/certificate_revocation_list/rest.rb | 2 +- spec/integration/indirector/report/rest.rb | 2 +- spec/integration/indirector/rest.rb | 4 +- spec/unit/network/authconfig.rb | 28 ++-- spec/unit/network/http/handler.rb | 8 +- spec/unit/network/rest_authconfig.rb | 2 +- spec/unit/network/rest_authorization.rb | 21 +++- spec/unit/network/rights.rb | 129 ++++++++++++-------- 15 files changed, 211 insertions(+), 137 deletions(-) diff --git a/lib/puppet/network/authconfig.rb b/lib/puppet/network/authconfig.rb index 3e0807a..3e40c9d 100644 --- a/lib/puppet/network/authconfig.rb +++ b/lib/puppet/network/authconfig.rb @@ -32,9 +32,8 @@ module Puppet return @rights[name].allowed?(request.name, request.ip) elsif @rights.include?(namespace) return @rights[namespace].allowed?(request.name, request.ip) - else - return false end + false end # Does the file exist? Puppetmasterd does not require it, but @@ -111,7 +110,7 @@ module Puppet name = $3 end name.chomp! - right = newrights.newright(name, count) + right = newrights.newright(name, count, @file) when /^\s*(allow|deny|method|environment)\s+(.+)$/ parse_right_directive(right, $1, $2, count) else diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/network/http/handler.rb index 20234b2..c6d34fe 100644 --- a/lib/puppet/network/http/handler.rb +++ b/lib/puppet/network/http/handler.rb @@ -3,6 +3,7 @@ end require 'puppet/network/http/api/v1' require 'puppet/network/rest_authorization' +require 'puppet/network/rights' module Puppet::Network::HTTP::Handler include Puppet::Network::HTTP::API::V1 @@ -40,11 +41,9 @@ module Puppet::Network::HTTP::Handler def process(request, response) indirection_request = uri2indirection(http_method(request), path(request), params(request)) - if authorized?(indirection_request) - send("do_%s" % indirection_request.method, indirection_request, request, response) - else - return do_exception(response, "Request forbidden by configuration %s %s" % [indirection_request.indirection_name, indirection_request.key], 403) - end + check_authorization(indirection_request) + + send("do_%s" % indirection_request.method, indirection_request, request, response) rescue Exception => e return do_exception(response, e) end @@ -60,6 +59,11 @@ module Puppet::Network::HTTP::Handler end def do_exception(response, exception, status=400) + if exception.is_a?(Puppet::Network::AuthorizationError) + # make sure we return the correct status code + # for authorization issues + status = 403 if status == 400 + end if exception.is_a?(Exception) puts exception.backtrace if Puppet[:trace] Puppet.err(exception) diff --git a/lib/puppet/network/rest_authconfig.rb b/lib/puppet/network/rest_authconfig.rb index e3fd517..22be8b0 100644 --- a/lib/puppet/network/rest_authconfig.rb +++ b/lib/puppet/network/rest_authconfig.rb @@ -23,9 +23,16 @@ module Puppet end # check wether this request is allowed in our ACL + # raise an Puppet::Network::AuthorizedError if the request + # is denied. def allowed?(request) read() - return @rights.allowed?(build_uri(request), request.node, request.ip, request.method, request.environment) + + @rights.fail_on_deny(build_uri(request), + :node => request.node, + :ip => request.ip, + :method => request.method, + :environment => request.environment) end def initialize(file = nil, parsenow = true) diff --git a/lib/puppet/network/rest_authorization.rb b/lib/puppet/network/rest_authorization.rb index 3278640..e6f62d9 100644 --- a/lib/puppet/network/rest_authorization.rb +++ b/lib/puppet/network/rest_authorization.rb @@ -2,12 +2,10 @@ require 'puppet/network/client_request' require 'puppet/network/rest_authconfig' module Puppet::Network - # Most of our subclassing is just so that we can get - # access to information from the request object, like - # the client name and IP address. - class InvalidClientRequest < Puppet::Error; end + module RestAuthorization + # Create our config object if necessary. If there's no configuration file # we install our defaults def authconfig @@ -20,28 +18,25 @@ module Puppet::Network # Verify that our client has access. We allow untrusted access to # certificates terminus but no others. - def authorized?(request) - msg = "%s client %s access to %s [%s]" % - [ request.authenticated? ? "authenticated" : "unauthenticated", - (request.node.nil? ? request.ip : "#{request.node}(#{request.ip})"), - request.indirection_name, request.method ] - + def check_authorization(request) if request.authenticated? - res = authenticated_authorized?(request, msg ) + authenticated_authorized?(request) else - res = unauthenticated_authorized?(request, msg) + unless unauthenticated_authorized?(request) + msg = "%s access to %s [%s]" % [ (request.node.nil? ? request.ip : "#{request.node}(#{request.ip})"), request.indirection_name, request.method ] + Puppet.warning("Denying access: " + msg) + raise AuthorizationError.new( "Forbidden request:" + msg ) + end end - Puppet.notice((res ? "Allowing " : "Denying ") + msg) - return res end # delegate to our authorization file - def authenticated_authorized?(request, msg) + def authenticated_authorized?(request) authconfig.allowed?(request) end # allow only certificate requests when not authenticated - def unauthenticated_authorized?(request, msg) + def unauthenticated_authorized?(request) request.indirection_name == :certificate or request.indirection_name == :certificate_request end end diff --git a/lib/puppet/network/rights.rb b/lib/puppet/network/rights.rb index 7f4bed7..75005d8 100755 --- a/lib/puppet/network/rights.rb +++ b/lib/puppet/network/rights.rb @@ -1,10 +1,16 @@ require 'puppet/network/authstore' +require 'puppet/error' + +module Puppet::Network + +# this exception is thrown when a request is not authenticated +class AuthorizationError < Puppet::Error; end # Define a set of rights and who has access to them. # There are two types of rights: # * named rights (ie a common string) # * path based rights (which are matched on a longest prefix basis) -class Puppet::Network::Rights +class Rights # We basically just proxy directly to our rights. Each Right stores # its own auth abilities. @@ -18,31 +24,57 @@ class Puppet::Network::Rights end end + # Check that name is allowed or not def allowed?(name, *args) + begin + fail_on_deny(name, *args) + rescue AuthorizationError + return false + rescue ArgumentError + # the namespace contract says we should raise this error + # if we didn't find the right acl + raise + end + return true + end + + def fail_on_deny(name, args = {}) res = :nomatch right = @rights.find do |acl| + found = false # an acl can return :dunno, which means "I'm not qualified to answer your question, # please ask someone else". This is used when for instance an acl matches, but not for the # current rest method, where we might think some other acl might be more specific. if match = acl.match?(name) - args << match - if (res = acl.allowed?(*args)) != :dunno - return res + args[:match] = match + if (res = acl.allowed?(args[:node], args[:ip], args)) != :dunno + # return early if we're allowed + return if res + # we matched, select this acl + found = true end end - false + found end - # if allowed or denied, tell it to the world - return res unless res == :nomatch - - # there were no rights allowing/denying name - # if name is not a path, let's throw - raise ArgumentError, "Unknown namespace right '%s'" % name unless name =~ /^\// + # if we end here, then that means we either didn't match + # or failed, in any case will throw an error to the outside world + if name =~ /^\// + # we're a patch ACL, let's fail + msg = "%s access to %s [%s]" % [ (args[:node].nil? ? args[:ip] : "#{args[:node]}(#{args[:ip]})"), name, args[:method] ] - # but if this was a path, we implement a deny all policy by default - # on unknown rights. - return false + error = AuthorizationError.new("Forbidden request: " + msg) + if right + error.file = right.file + error.line = right.line + end + Puppet.warning("Denying access: " + error.to_s) + else + # there were no rights allowing/denying name + # if name is not a path, let's throw + error = ArgumentError.new "Unknown namespace right '%s'" % name + end + raise error end def initialize() @@ -62,8 +94,8 @@ class Puppet::Network::Rights end # Define a new right to which access can be provided. - def newright(name, line=nil) - add_right( Right.new(name, line) ) + def newright(name, line=nil, file=nil) + add_right( Right.new(name, line, file) ) end private @@ -88,18 +120,19 @@ class Puppet::Network::Rights # A right. class Right < Puppet::Network::AuthStore - attr_accessor :name, :key, :acl_type, :line + attr_accessor :name, :key, :acl_type, :line, :file attr_accessor :methods, :environment ALL = [:save, :destroy, :find, :search] Puppet::Util.logmethods(self, true) - def initialize(name, line) + def initialize(name, line, file) @methods = [] @environment = [] @name = name @line = line || 0 + @file = file case name when Symbol @@ -140,18 +173,16 @@ class Puppet::Network::Rights # if this right is too restrictive (ie we don't match this access method) # then return :dunno so that upper layers have a chance to try another right # tailored to the given method - def allowed?(name, ip, method = nil, environment = nil, match = nil) - return :dunno if acl_type == :regex and not @methods.include?(method) - return :dunno if acl_type == :regex and @environment.size > 0 and not @environment.include?(environment) - - if acl_type == :regex and match # make sure any capture are replaced - interpolate(match) - end - - res = super(name,ip) - - if acl_type == :regex - reset_interpolation + def allowed?(name, ip, args) + return :dunno if acl_type == :regex and not @methods.include?(args[:method]) + return :dunno if acl_type == :regex and @environment.size > 0 and not @environment.include?(args[:environment]) + + begin + # make sure any capture are replaced if needed + interpolate(args[:match]) if acl_type == :regex and args[:match] + res = super(name,ip) + ensure + reset_interpolation if acl_type == :regex end res end @@ -222,4 +253,4 @@ class Puppet::Network::Rights end end - +end diff --git a/spec/integration/indirector/certificate/rest.rb b/spec/integration/indirector/certificate/rest.rb index c42bafb..8512fa9 100755 --- a/spec/integration/indirector/certificate/rest.rb +++ b/spec/integration/indirector/certificate/rest.rb @@ -48,7 +48,7 @@ describe "Certificate REST Terminus" do @mock_model = stub('faked model', :name => "certificate") Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/integration/indirector/certificate_request/rest.rb b/spec/integration/indirector/certificate_request/rest.rb index 1381876..a1dc5c0 100755 --- a/spec/integration/indirector/certificate_request/rest.rb +++ b/spec/integration/indirector/certificate_request/rest.rb @@ -53,7 +53,7 @@ describe "Certificate Request REST Terminus" do @mock_model = stub('faked model', :name => "certificate request") Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/integration/indirector/certificate_revocation_list/rest.rb b/spec/integration/indirector/certificate_revocation_list/rest.rb index a1ba4f9..dce0cf0 100755 --- a/spec/integration/indirector/certificate_revocation_list/rest.rb +++ b/spec/integration/indirector/certificate_revocation_list/rest.rb @@ -57,7 +57,7 @@ describe "Certificate REST Terminus" do @mock_model = stub('faked model', :name => "certificate") Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/integration/indirector/report/rest.rb b/spec/integration/indirector/report/rest.rb index 1150075..7903ac9 100644 --- a/spec/integration/indirector/report/rest.rb +++ b/spec/integration/indirector/report/rest.rb @@ -50,7 +50,7 @@ describe "Report REST Terminus" do @mock_model = stub_everything 'faked model', :name => "report", :convert_from => @report Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization) end after do diff --git a/spec/integration/indirector/rest.rb b/spec/integration/indirector/rest.rb index 34619c4..ede073d 100755 --- a/spec/integration/indirector/rest.rb +++ b/spec/integration/indirector/rest.rb @@ -78,7 +78,7 @@ describe Puppet::Indirector::REST do Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) # do not trigger the authorization layer - Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::WEBrickREST.any_instance.stubs(:check_authorization).returns(true) end describe "when finding a model instance over REST" do @@ -311,7 +311,7 @@ describe Puppet::Indirector::REST do Puppet::Indirector::Request.any_instance.stubs(:model).returns(@mock_model) # do not trigger the authorization layer - Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:authorized?).returns(true) + Puppet::Network::HTTP::MongrelREST.any_instance.stubs(:check_authorization).returns(true) end after do diff --git a/spec/unit/network/authconfig.rb b/spec/unit/network/authconfig.rb index 186d30c..21af89b 100644 --- a/spec/unit/network/authconfig.rb +++ b/spec/unit/network/authconfig.rb @@ -114,7 +114,7 @@ describe Puppet::Network::AuthConfig do it "should increment line number even on commented lines" do @fd.stubs(:each).multiple_yields(' # comment','[puppetca]') - @rights.expects(:newright).with('[puppetca]', 2) + @rights.expects(:newright).with('[puppetca]', 2, 'dummy') @authconfig.read end @@ -130,7 +130,7 @@ describe Puppet::Network::AuthConfig do it "should increment line number even on blank lines" do @fd.stubs(:each).multiple_yields(' ','[puppetca]') - @rights.expects(:newright).with('[puppetca]', 2) + @rights.expects(:newright).with('[puppetca]', 2, 'dummy') @authconfig.read end @@ -146,7 +146,7 @@ describe Puppet::Network::AuthConfig do it "should not throw an error if the current path right already exist" do @fd.stubs(:each).yields('path /hello') - @rights.stubs(:newright).with("/hello",1) + @rights.stubs(:newright).with("/hello",1, 'dummy') @rights.stubs(:include?).with("/hello").returns(true) lambda { @authconfig.read }.should_not raise_error @@ -155,7 +155,7 @@ describe Puppet::Network::AuthConfig do it "should create a new right for found namespaces" do @fd.stubs(:each).yields('[puppetca]') - @rights.expects(:newright).with("[puppetca]", 1) + @rights.expects(:newright).with("[puppetca]", 1, 'dummy') @authconfig.read end @@ -163,8 +163,8 @@ describe Puppet::Network::AuthConfig do it "should create a new right for each found namespace line" do @fd.stubs(:each).multiple_yields('[puppetca]', '[fileserver]') - @rights.expects(:newright).with("[puppetca]", 1) - @rights.expects(:newright).with("[fileserver]", 2) + @rights.expects(:newright).with("[puppetca]", 1, 'dummy') + @rights.expects(:newright).with("[fileserver]", 2, 'dummy') @authconfig.read end @@ -172,7 +172,7 @@ describe Puppet::Network::AuthConfig do it "should create a new right for each found path line" do @fd.stubs(:each).multiple_yields('path /certificates') - @rights.expects(:newright).with("/certificates", 1) + @rights.expects(:newright).with("/certificates", 1, 'dummy') @authconfig.read end @@ -180,7 +180,7 @@ describe Puppet::Network::AuthConfig do it "should create a new right for each found regex line" do @fd.stubs(:each).multiple_yields('path ~ .rb$') - @rights.expects(:newright).with("~ .rb$", 1) + @rights.expects(:newright).with("~ .rb$", 1, 'dummy') @authconfig.read end @@ -189,7 +189,7 @@ describe Puppet::Network::AuthConfig do acl = stub 'acl', :info @fd.stubs(:each).multiple_yields('[puppetca]', 'allow 127.0.0.1') - @rights.stubs(:newright).with("[puppetca]", 1).returns(acl) + @rights.stubs(:newright).with("[puppetca]", 1, 'dummy').returns(acl) acl.expects(:allow).with('127.0.0.1') @@ -200,7 +200,7 @@ describe Puppet::Network::AuthConfig do acl = stub 'acl', :info @fd.stubs(:each).multiple_yields('[puppetca]', 'deny 127.0.0.1') - @rights.stubs(:newright).with("[puppetca]", 1).returns(acl) + @rights.stubs(:newright).with("[puppetca]", 1, 'dummy').returns(acl) acl.expects(:deny).with('127.0.0.1') @@ -212,7 +212,7 @@ describe Puppet::Network::AuthConfig do acl.stubs(:acl_type).returns(:regex) @fd.stubs(:each).multiple_yields('path /certificates', 'method search,find') - @rights.stubs(:newright).with("/certificates", 1).returns(acl) + @rights.stubs(:newright).with("/certificates", 1, 'dummy').returns(acl) acl.expects(:restrict_method).with('search') acl.expects(:restrict_method).with('find') @@ -225,7 +225,7 @@ describe Puppet::Network::AuthConfig do acl.stubs(:acl_type).returns(:regex) @fd.stubs(:each).multiple_yields('[puppetca]', 'method search,find') - @rights.stubs(:newright).with("puppetca", 1).returns(acl) + @rights.stubs(:newright).with("puppetca", 1, 'dummy').returns(acl) lambda { @authconfig.read }.should raise_error end @@ -235,7 +235,7 @@ describe Puppet::Network::AuthConfig do acl.stubs(:acl_type).returns(:regex) @fd.stubs(:each).multiple_yields('path /certificates', 'environment production,development') - @rights.stubs(:newright).with("/certificates", 1).returns(acl) + @rights.stubs(:newright).with("/certificates", 1, 'dummy').returns(acl) acl.expects(:restrict_environment).with('production') acl.expects(:restrict_environment).with('development') @@ -248,7 +248,7 @@ describe Puppet::Network::AuthConfig do acl.stubs(:acl_type).returns(:regex) @fd.stubs(:each).multiple_yields('[puppetca]', 'environment env') - @rights.stubs(:newright).with("puppetca", 1).returns(acl) + @rights.stubs(:newright).with("puppetca", 1, 'dummy').returns(acl) lambda { @authconfig.read }.should raise_error end diff --git a/spec/unit/network/http/handler.rb b/spec/unit/network/http/handler.rb index 7b7ef47..0786d37 100755 --- a/spec/unit/network/http/handler.rb +++ b/spec/unit/network/http/handler.rb @@ -49,7 +49,7 @@ describe Puppet::Network::HTTP::Handler do @result = stub 'result', :render => "mytext" - @handler.stubs(:authorized?).returns(true) + @handler.stubs(:check_authorization) stub_server_interface end @@ -97,7 +97,7 @@ describe Puppet::Network::HTTP::Handler do @handler.expects(:do_mymethod).with(request, @request, @response) - @handler.expects(:authorized?).with(request).returns(true) + @handler.expects(:check_authorization).with(request) @handler.process(@request, @response) end @@ -108,9 +108,9 @@ describe Puppet::Network::HTTP::Handler do @handler.expects(:do_mymethod).never - @handler.expects(:authorized?).with(request).returns(false) + @handler.expects(:check_authorization).with(request).raises(Puppet::Network::AuthorizationError.new("forbindden")) - @handler.expects(:set_response)#.with { |response, body, status| status == 403 } + @handler.expects(:set_response).with { |response, body, status| status == 403 } @handler.process(@request, @response) end diff --git a/spec/unit/network/rest_authconfig.rb b/spec/unit/network/rest_authconfig.rb index ea5a82c..7dc9738 100644 --- a/spec/unit/network/rest_authconfig.rb +++ b/spec/unit/network/rest_authconfig.rb @@ -33,7 +33,7 @@ describe Puppet::Network::RestAuthConfig do end it "should ask for authorization to the ACL subsystem" do - @acl.expects(:allowed?).with("/path/to/resource", "me", "127.0.0.1", :save, :env) + @acl.expects(:fail_on_deny).with("/path/to/resource", :node => "me", :ip => "127.0.0.1", :method => :save, :environment => :env) @authconfig.allowed?(@request) end diff --git a/spec/unit/network/rest_authorization.rb b/spec/unit/network/rest_authorization.rb index 15351b1..3acd23f 100644 --- a/spec/unit/network/rest_authorization.rb +++ b/spec/unit/network/rest_authorization.rb @@ -18,6 +18,7 @@ describe Puppet::Network::RestAuthorization do @request = stub_everything 'request' @request.stubs(:method).returns(:find) @request.stubs(:node).returns("node") + @request.stubs(:ip).returns("ip") end describe "when testing request authorization" do @@ -29,14 +30,14 @@ describe Puppet::Network::RestAuthorization do [ :certificate, :certificate_request].each do |indirection| it "should allow #{indirection}" do @request.stubs(:indirection_name).returns(indirection) - @auth.authorized?(@request).should be_true + lambda { @auth.check_authorization(@request) }.should_not raise_error Puppet::Network::AuthorizationError end end [ :facts, :file_metadata, :file_content, :catalog, :report, :checksum, :runner ].each do |indirection| it "should not allow #{indirection}" do @request.stubs(:indirection_name).returns(indirection) - @auth.authorized?(@request).should be_false + lambda { @auth.check_authorization(@request) }.should raise_error Puppet::Network::AuthorizationError end end end @@ -47,9 +48,21 @@ describe Puppet::Network::RestAuthorization do end it "should delegate to the current rest authconfig" do - @authconfig.expects(:allowed?).with(@request) + @authconfig.expects(:allowed?).with(@request).returns(true) - @auth.authorized?(@request) + @auth.check_authorization(@request) + end + + it "should raise an AuthorizationError if authconfig raises an AuthorizationError" do + @authconfig.expects(:allowed?).with(@request).raises(Puppet::Network::AuthorizationError.new("forbidden")) + + lambda { @auth.check_authorization(@request) }.should raise_error Puppet::Network::AuthorizationError + end + + it "should not raise an AuthorizationError if request is allowed" do + @authconfig.expects(:allowed?).with(@request).returns(true) + + lambda { @auth.check_authorization(@request) }.should_not raise_error Puppet::Network::AuthorizationError end end end diff --git a/spec/unit/network/rights.rb b/spec/unit/network/rights.rb index 97094f8..be1db72 100644 --- a/spec/unit/network/rights.rb +++ b/spec/unit/network/rights.rb @@ -145,55 +145,75 @@ describe Puppet::Network::Rights do before :each do @right.stubs(:right).returns(nil) - @pathacl = stub 'pathacl', :acl_type => :path + @pathacl = stub 'pathacl', :acl_type => :regex, :"<=>" => 1, :line => 0, :file => 'dummy' Puppet::Network::Rights::Right.stubs(:new).returns(@pathacl) end + it "should delegate to fail_on_deny" do + @right.expects(:fail_on_deny).with("namespace", :args) + + @right.allowed?("namespace", :args) + end + + it "should return true if fail_on_deny doesn't fail" do + @right.stubs(:fail_on_deny) + @right.allowed?("namespace", :args).should be_true + end + + it "should return false if fail_on_deny raises an AuthorizationError" do + @right.stubs(:fail_on_deny).raises(Puppet::Network::AuthorizationError.new("forbidden")) + @right.allowed?("namespace", :args1, :args2).should be_false + end + it "should first check namespace rights" do acl = stub 'acl', :acl_type => :name, :key => :namespace Puppet::Network::Rights::Right.stubs(:new).returns(acl) @right.newright("[namespace]") acl.expects(:match?).returns(true) - acl.expects(:allowed?).with(:args, true).returns(true) + acl.expects(:allowed?).with { |node,ip,h| node == "node" and ip == "ip" }.returns(true) - @right.allowed?("namespace", :args) + @right.fail_on_deny("namespace", { :node => "node", :ip => "ip" } ) end it "should then check for path rights if no namespace match" do - acl = stub 'acl', :acl_type => :name, :match? => false + acl = stub 'nmacl', :acl_type => :name, :key => :namespace, :"<=>" => -1, :line => 0, :file => 'dummy' + acl.stubs(:match?).returns(false) + Puppet::Network::Rights::Right.stubs(:new).with("[namespace]").returns(acl) - acl.expects(:allowed?).with(:args).never - @right.newright("/path/to/there") + @right.newright("[namespace]") + @right.newright("/path/to/there", 0, nil) @pathacl.stubs(:match?).returns(true) - @pathacl.expects(:allowed?) - @right.allowed?("/path/to/there", :args) + acl.expects(:allowed?).never + @pathacl.expects(:allowed?).returns(true) + + @right.fail_on_deny("/path/to/there", {}) end it "should pass the match? return to allowed?" do @right.newright("/path/to/there") @pathacl.expects(:match?).returns(:match) - @pathacl.expects(:allowed?).with(:args, :match) + @pathacl.expects(:allowed?).with { |node,ip,h| h[:match] == :match }.returns(true) - @right.allowed?("/path/to/there", :args) + @right.fail_on_deny("/path/to/there", {}) end describe "with namespace acls" do it "should raise an error if this namespace right doesn't exist" do - lambda{ @right.allowed?("namespace") }.should raise_error + lambda{ @right.fail_on_deny("namespace") }.should raise_error end end describe "with path acls" do before :each do - @long_acl = stub 'longpathacl', :name => "/path/to/there", :acl_type => :regex - Puppet::Network::Rights::Right.stubs(:new).with("/path/to/there", 0).returns(@long_acl) + @long_acl = stub 'longpathacl', :name => "/path/to/there", :acl_type => :regex, :line => 0, :file => 'dummy' + Puppet::Network::Rights::Right.stubs(:new).with("/path/to/there", 0, nil).returns(@long_acl) - @short_acl = stub 'shortpathacl', :name => "/path/to", :acl_type => :regex - Puppet::Network::Rights::Right.stubs(:new).with("/path/to", 0).returns(@short_acl) + @short_acl = stub 'shortpathacl', :name => "/path/to", :acl_type => :regex, :line => 0, :file => 'dummy' + Puppet::Network::Rights::Right.stubs(:new).with("/path/to", 0, nil).returns(@short_acl) @long_acl.stubs(:"<=>").with(@short_acl).returns(0) @short_acl.stubs(:"<=>").with(@long_acl).returns(0) @@ -209,20 +229,20 @@ describe Puppet::Network::Rights do @long_acl.expects(:allowed?).returns(true) @short_acl.expects(:allowed?).never - @right.allowed?("/path/to/there/and/there", :args) + @right.fail_on_deny("/path/to/there/and/there", {}) end it "should select the first match that doesn't return :dunno" do - @right.newright("/path/to/there", 0) - @right.newright("/path/to", 0) + @right.newright("/path/to/there", 0, nil) + @right.newright("/path/to", 0, nil) @long_acl.stubs(:match?).returns(true) @short_acl.stubs(:match?).returns(true) @long_acl.expects(:allowed?).returns(:dunno) - @short_acl.expects(:allowed?) + @short_acl.expects(:allowed?).returns(true) - @right.allowed?("/path/to/there/and/there", :args) + @right.fail_on_deny("/path/to/there/and/there", {}) end it "should not select an ACL that doesn't match" do @@ -233,36 +253,41 @@ describe Puppet::Network::Rights do @short_acl.stubs(:match?).returns(true) @long_acl.expects(:allowed?).never - @short_acl.expects(:allowed?) + @short_acl.expects(:allowed?).returns(true) - @right.allowed?("/path/to/there/and/there", :args) + @right.fail_on_deny("/path/to/there/and/there", {}) end - it "should return the result of the acl" do + it "should not raise an AuthorizationError if allowed" do @right.newright("/path/to/there", 0) @long_acl.stubs(:match?).returns(true) - @long_acl.stubs(:allowed?).returns(:returned) + @long_acl.stubs(:allowed?).returns(true) - @right.allowed?("/path/to/there/and/there", :args).should == :returned + lambda { @right.fail_on_deny("/path/to/there/and/there", {}) }.should_not raise_error(Puppet::Network::AuthorizationError) end - it "should not raise an error if this path acl doesn't exist" do - lambda{ @right.allowed?("/path", :args) }.should_not raise_error + it "should raise an AuthorizationError if the match is denied" do + @right.newright("/path/to/there", 0, nil) + + @long_acl.stubs(:match?).returns(true) + @long_acl.stubs(:allowed?).returns(false) + + lambda{ @right.fail_on_deny("/path/to/there", {}) }.should raise_error(Puppet::Network::AuthorizationError) end - it "should return false if no path match" do - @right.allowed?("/path", :args).should be_false + it "should raise an AuthorizationError if no path match" do + lambda { @right.fail_on_deny("/nomatch", {}) }.should raise_error(Puppet::Network::AuthorizationError) end end describe "with regex acls" do before :each do - @regex_acl1 = stub 'regex_acl1', :name => "/files/(.*)/myfile", :acl_type => :regex - Puppet::Network::Rights::Right.stubs(:new).with("~ /files/(.*)/myfile", 0).returns(@regex_acl1) + @regex_acl1 = stub 'regex_acl1', :name => "/files/(.*)/myfile", :acl_type => :regex, :line => 0, :file => 'dummy' + Puppet::Network::Rights::Right.stubs(:new).with("~ /files/(.*)/myfile", 0, nil).returns(@regex_acl1) - @regex_acl2 = stub 'regex_acl2', :name => "/files/(.*)/myfile/", :acl_type => :regex - Puppet::Network::Rights::Right.stubs(:new).with("~ /files/(.*)/myfile/", 0).returns(@regex_acl2) + @regex_acl2 = stub 'regex_acl2', :name => "/files/(.*)/myfile/", :acl_type => :regex, :line => 0, :file => 'dummy' + Puppet::Network::Rights::Right.stubs(:new).with("~ /files/(.*)/myfile/", 0, nil).returns(@regex_acl2) @regex_acl1.stubs(:"<=>").with(@regex_acl2).returns(0) @regex_acl2.stubs(:"<=>").with(@regex_acl1).returns(0) @@ -278,7 +303,7 @@ describe Puppet::Network::Rights do @regex_acl1.expects(:allowed?).returns(true) @regex_acl2.expects(:allowed?).never - @right.allowed?("/files/repository/myfile/other", :args) + @right.fail_on_deny("/files/repository/myfile/other", {}) end it "should select the first match that doesn't return :dunno" do @@ -289,9 +314,9 @@ describe Puppet::Network::Rights do @regex_acl2.stubs(:match?).returns(true) @regex_acl1.expects(:allowed?).returns(:dunno) - @regex_acl2.expects(:allowed?) + @regex_acl2.expects(:allowed?).returns(true) - @right.allowed?("/files/repository/myfile/other", :args) + @right.fail_on_deny("/files/repository/myfile/other", {}) end it "should not select an ACL that doesn't match" do @@ -302,26 +327,26 @@ describe Puppet::Network::Rights do @regex_acl2.stubs(:match?).returns(true) @regex_acl1.expects(:allowed?).never - @regex_acl2.expects(:allowed?) + @regex_acl2.expects(:allowed?).returns(true) - @right.allowed?("/files/repository/myfile/other", :args) + @right.fail_on_deny("/files/repository/myfile/other", {}) end - it "should return the result of the acl" do + it "should not raise an AuthorizationError if allowed" do @right.newright("~ /files/(.*)/myfile", 0) @regex_acl1.stubs(:match?).returns(true) - @regex_acl1.stubs(:allowed?).returns(:returned) + @regex_acl1.stubs(:allowed?).returns(true) - @right.allowed?("/files/repository/myfile/other", :args).should == :returned + lambda { @right.fail_on_deny("/files/repository/myfile/other", {}) }.should_not raise_error(Puppet::Network::AuthorizationError) end - it "should not raise an error if no regex acl match" do - lambda{ @right.allowed?("/path", :args) }.should_not raise_error + it "should raise an error if no regex acl match" do + lambda{ @right.fail_on_deny("/path", {}) }.should raise_error(Puppet::Network::AuthorizationError) end - it "should return false if no regex match" do - @right.allowed?("/path", :args).should be_false + it "should raise an AuthorizedError on deny" do + lambda { @right.fail_on_deny("/path", {}) }.should raise_error(Puppet::Network::AuthorizationError) end end @@ -329,7 +354,7 @@ describe Puppet::Network::Rights do describe Puppet::Network::Rights::Right do before :each do - @acl = Puppet::Network::Rights::Right.new("/path",0) + @acl = Puppet::Network::Rights::Right.new("/path",0, nil) end describe "with path" do @@ -352,7 +377,7 @@ describe Puppet::Network::Rights do describe "with regex" do before :each do - @acl = Puppet::Network::Rights::Right.new("~ .rb$",0) + @acl = Puppet::Network::Rights::Right.new("~ .rb$",0, nil) end it "should say it's a regex ACL" do @@ -403,14 +428,14 @@ describe Puppet::Network::Rights do it "should return :dunno if this right is not restricted to the given method" do @acl.restrict_method(:destroy) - @acl.allowed?("me","127.0.0.1", :save).should == :dunno + @acl.allowed?("me","127.0.0.1", { :method => :save } ).should == :dunno end it "should return allow/deny if this right is restricted to the given method" do @acl.restrict_method(:save) @acl.allow("127.0.0.1") - @acl.allowed?("me","127.0.0.1", :save).should be_true + @acl.allowed?("me","127.0.0.1", { :method => :save }).should be_true end it "should return :dunno if this right is not restricted to the given environment" do @@ -418,19 +443,19 @@ describe Puppet::Network::Rights do @acl.restrict_environment(:production) - @acl.allowed?("me","127.0.0.1", :save, :development).should == :dunno + @acl.allowed?("me","127.0.0.1", { :method => :save, :environment => :development }).should == :dunno end it "should interpolate allow/deny patterns with the given match" do @acl.expects(:interpolate).with(:match) - @acl.allowed?("me","127.0.0.1", :save, nil, :match) + @acl.allowed?("me","127.0.0.1", :method => :save, :match => :match) end it "should reset interpolation after the match" do @acl.expects(:reset_interpolation) - @acl.allowed?("me","127.0.0.1", :save, nil, :match) + @acl.allowed?("me","127.0.0.1", :method => :save, :match => :match) end # mocha doesn't allow testing super... -- 1.6.0.2 --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected] To unsubscribe from this group, send email to [email protected] For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en -~----------~----~----~----~------~----~------~--~---
