+1, a couple of comments below. On Apr 19, 2009, at 11:38 AM, Brice Figureau wrote:
> > 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) Why not just pass the whole request in? > > 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 Looks like this class should use the FileCollection::Lookup class. Probably not much of an optimization in this case, though, I guess. > > 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 > > > > -- Those who speak most of progress measure it by quantity and not by quality. --George Santayana --------------------------------------------------------------------- Luke Kanies | http://reductivelabs.com | http://madstop.com --~--~---------~--~----~------------~-------~--~----~ 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 -~----------~----~----~----~------~----~------~--~---
