On Apr 20, 2009, at 2:51 AM, Brice Figureau wrote:
>
> 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.
Ah, right; might be worth a comment, then.
>
>>>
>>> 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?
Yep.
>
>>>
>>> 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/
>
>
> >
--
Anyone who considers arithmatical methods of producing random digits
is, of course, in a state of sin. --John Von Neumann
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
