On Sun, 2009-04-19 at 22:13 -0500, Luke Kanies wrote:
> +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?
Because the underlying P::N::Rights code is used by two different
clients calling it with different Requests implementation (i.e. the
XMLRPC uses a ClientRequest and the RESt part uses an indirection
request).
If we ever get rid of the XMLRPC we could push deeper the request.
> >
> > 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.
Yes, I completely forgot this one.
Did it enter master finally?
> >
> > 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
> >
> >
> > >
>
--
Brice Figureau
My Blog: http://www.masterzen.fr/
--~--~---------~--~----~------------~-------~--~----~
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
-~----------~----~----~----~------~----~------~--~---
