On Apr 14, 2009, at 4:33 AM, Brice Figureau wrote:
>
> On 13/04/09 22:25, Luke Kanies wrote:
>> Hi Brice,
>>
>> This is the only commit in this series I've got comments remaining
>> on,
>> and I want to stress: These comments are optional, and the goal with
>> them is that the code be clearer and that less of the code knows
>> about
>> other parts of the code. If you think my recommendations don't
>> result
>> in cleaner, more maintainable code, then please ignore them. You're
>> the one writing the code, so you're the in best position to judge
>> what
>> the most maintainable form is.
>
> No, I think you're right. I'll see what's possible.
>
>> On Apr 11, 2009, at 12:39 PM, 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 | 8 ++--
>>> lib/puppet/network/http/handler.rb | 13 ++++--
>>> lib/puppet/network/rest_authorization.rb | 43 +++++++++
>>> ++
>>> ++------
>>> lib/puppet/network/rights.rb | 22 +++++-----
>>> 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/http/handler.rb | 8 ++--
>>> spec/unit/network/rest_authorization.rb | 20 +++++++--
>>> spec/unit/network/rights.rb | 41 +++++++++
>>> ++
>>> ++++---
>>> test/network/rights.rb | 6 ++-
>>> 13 files changed, 117 insertions(+), 56 deletions(-)
>>>
>>> diff --git a/lib/puppet/network/authconfig.rb b/lib/puppet/network/
>>> authconfig.rb
>>> index 3e0807a..3aaab9a 100644
>>> --- a/lib/puppet/network/authconfig.rb
>>> +++ b/lib/puppet/network/authconfig.rb
>>> @@ -28,13 +28,13 @@ module Puppet
>>>
>>> read()
>>>
>>> + res = false
>>> if @rights.include?(name)
>>> - return @rights[name].allowed?(request.name,
>>> request.ip)
>>> + res, acl = @rights[name].allowed?(request.name,
>>> request.ip)
>>> elsif @rights.include?(namespace)
>>> - return @rights[namespace].allowed?(request.name,
>>> request.ip)
>>> - else
>>> - return false
>>> + res, acl = @rights[namespace].allowed?
>>> (request.name, request.ip)
>>
>> I think it's a bit weird that you're passing what is otherwise a
>> private instance (the acl) up the chain.
>
> I was sure you would argue that wasn't a good idea :-)
>
>
>> I feel bad that I keeping
>> pushing you on this, since it's a relatively small thing, but I think
>> something like this might be more appropriate:
>>
>> if right = @rights[name] || @rights[namespace]
>> right.fail_unless_allowed(request.name, request.ip)
>> else
>> raise AuthorizationError, "Access to %s denied by default" %
>> [....]
>> end
>
> Yes, this works for this part of the code which only checks for the
> so-called namespace rights, because there we can find easily the
> matching right and it is unique. For the REST authorization, we have
> to
> cycle through all the sub-rights before finding the only one that
> matches. What you propose can't be done without breaking completely
> the
> current allowed? API.
> In fact I tried to keep the allowed? API entry we already have.
> Now if we are willing to break this API (and raise on deny as deeply
> as
> in the rights instance), then that'd really simpler.
> I can push the check code even deeper than it is now.
I'm comfortable breaking the existing api, since it's an entirely
internal api.
>
>> That way the ACL isn't exposed anywhere it shouldn't be, and the
>> ACL's
>> failure method can take into account any info it does or doesn't have
>> available:
>>
>> def fail_unless_allowed(name, ip)
>> return if allowed?(name, ip)
>> msg = "Access to %s denied to %s(%s)" % [path, name, ip]
>> msg += " in file %s" % file if file
>> msg += " at line %s" % line if line
>>
>> raise AuthorizationError, msg
>> end
>
> I think I can move fail_unless_allowed in Rights (not Rights::Right),
> without breaking allowed? too much.
> Hmm, let me try this. I'll repost (only) this patch when done to see
> if
> that's better for you.
That sounds good.
>
>> Note that Puppet::Error actually has line/file attributes, and it
>> will
>> add them automatically, so if your AuthorizationError inherits from
>> it, you could do:
>>
>> ...
>> error = Puppet::Error.new("Access....")
>
> shouldn't it be:
> error = AuthorizationError.new(...) ?
Ayup.
>
>> error.line = self.line
>> error.file = self.file
>> raise error
>>
>> Then... (below)...
>>
>>> end
>>> + return res
>>> end
>>>
>>> # Does the file exist? Puppetmasterd does not require it,
>>> but
>>> diff --git a/lib/puppet/network/http/handler.rb b/lib/puppet/
>>> network/
>>> http/handler.rb
>>> index 20234b2..12cdf9b 100644
>>> --- a/lib/puppet/network/http/handler.rb
>>> +++ b/lib/puppet/network/http/handler.rb
>>> @@ -40,11 +40,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 +58,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_authorization.rb b/lib/puppet/
>>> network/rest_authorization.rb
>>> index 3278640..3d84cdd 100644
>>> --- a/lib/puppet/network/rest_authorization.rb
>>> +++ b/lib/puppet/network/rest_authorization.rb
>>> @@ -2,12 +2,13 @@ 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
>>> +
>>> + # this exception is thrown when a request is not authenticated
>>> + class AuthorizationError < RuntimeError; end
>>> +
>>> module RestAuthorization
>>>
>>> +
>>> # Create our config object if necessary. If there's no
>>> configuration file
>>> # we install our defaults
>>> def authconfig
>>> @@ -18,30 +19,44 @@ module Puppet::Network
>>> @authconfig
>>> end
>>>
>>> + def authenticated_to_s(request)
>>> + request.authenticated? ? "authenticated" :
>>> "unauthenticated"
>>> + end
>>> +
>>> # Verify that our client has access. We allow untrusted
>>> access to
>>> # certificates terminus but no others.
>>> - def authorized?(request)
>>> + def check_authorization(request)
>>> msg = "%s client %s access to %s [%s]" %
>>> - [ request.authenticated? ? "authenticated" :
>>> "unauthenticated",
>>> + [ authenticated_to_s(request),
>>> (request.node.nil? ? request.ip :
>>> "#{request.node}(#{request.ip})"),
>>> request.indirection_name, request.method ]
>>>
>>> - if request.authenticated?
>>> - res = authenticated_authorized?(request, msg )
>>> - else
>>> - res = unauthenticated_authorized?(request, msg)
>>> + method = authenticated_to_s(request) + "_authorized?"
>>> +
>>> + res, acl = send(method.to_sym, request)
>>> + unless res
>>> + unless acl.nil?
>>> + msg += " matching rule: '#{acl.name}'"
>>> + if acl.line != 0 and authconfig.exists?
>>> + msg += " of #{authconfig} line:
>>> #{acl.line}"
>>> + else
>>> + msg += " from default ACLs"
>>> + end
>>> + end
>>> + Puppet.notice("Denying "+ msg)
>>> + raise AuthorizationError, "Request forbidden by
>>> configuration %s %s" % [request.indirection_name, request.key]
>>
>> Keeping in mind that you've got to inform both sides of the
>> connection
>> (server and client), I'd prefer if each side got exactly the same
>> information. And shouldn't the server side should get the log as a
>> 'warning', rather than notice?
>
> Fair enough. Note that it is a notice in the namespaceauth code, so I
> kept it. Warning makes more sense I think.
Ah; yeah, that should be changed, then.
>
>> Basically, I would do something like:
>>
>> Puppet.warning "Denying " + msg
>> raise AuthorizationError, "Request forbidden: %s" % msg
>>
>> I expect this will entail a slight separation between the server and
>> client messages, but they should be able to be almost the same.
>
> OK.
>
>> I just know that, for some reason, people tend not to look at server
>> logs when they see exceptions on the client, so we have to both log
>> useful info on the server *and* propagate that same info to the
>> client.
>
> OK.
>
>> If you follow my advice from above (failing locally), then you could
>> just do something like this:
>>
>> begin
>> ....perform authorization checks....
>> rescue AuthorizationError => error
>> # Log on the server side
>> Puppet.warning "Client %s(%s) denied: %s" % [request.name,
>> request.ip, error]
>> raise # send the error back to the client
>> end
>>
>>> end
>>> - Puppet.notice((res ? "Allowing " : "Denying ") + msg)
>>> - return res
>>> +
>>> + Puppet.notice("Allowing "+ msg)
>>> 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..959b004 100755
>>> --- a/lib/puppet/network/rights.rb
>>> +++ b/lib/puppet/network/rights.rb
>>> @@ -18,6 +18,7 @@ class Puppet::Network::Rights
>>> end
>>> end
>>>
>>> + # Check that name is allowed or not
>>> def allowed?(name, *args)
>>> res = :nomatch
>>> right = @rights.find do |acl|
>>> @@ -27,14 +28,15 @@ class Puppet::Network::Rights
>>> if match = acl.match?(name)
>>> args << match
>>> if (res = acl.allowed?(*args)) != :dunno
>>> - return res
>>> + return res, acl
>>> end
>>> + args.pop
>>> end
>>> false
>>> end
>>>
>>> # if allowed or denied, tell it to the world
>>> - return res unless res == :nomatch
>>> + return res, nil unless (res == :nomatch or res == :dunno)
>>>
>>> # there were no rights allowing/denying name
>>> # if name is not a path, let's throw
>>> @@ -42,7 +44,7 @@ class Puppet::Network::Rights
>>>
>>> # but if this was a path, we implement a deny all policy by
>>> default
>>> # on unknown rights.
>>> - return false
>>> + return false, nil
>>> end
>>>
>>> def initialize()
>>> @@ -144,14 +146,12 @@ class Puppet::Network::Rights
>>> 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
>>> + begin
>>> + # make sure any capture are replaced if needed
>>> + interpolate(match) if acl_type == :regex and match
>>> + res = super(name,ip)
>>> + ensure
>>> + reset_interpolation if acl_type == :regex
>>> end
>>> res
>>> 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/http/handler.rb b/spec/unit/network/
>>> http/handler.rb
>>> index 7b7ef47..743830a 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)
>>>
>>> - @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_authorization.rb b/spec/unit/
>>> network/rest_authorization.rb
>>> index 15351b1..46274bb 100644
>>> --- a/spec/unit/network/rest_authorization.rb
>>> +++ b/spec/unit/network/rest_authorization.rb
>>> @@ -29,14 +29,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 +47,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 request is
>>> not allowed" do
>>> +
>>> @authconfig.expects(:allowed?).with(@request).returns(false)
>>> +
>>> + 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..6d7577e 100644
>>> --- a/spec/unit/network/rights.rb
>>> +++ b/spec/unit/network/rights.rb
>>> @@ -244,7 +244,18 @@ describe Puppet::Network::Rights do
>>> @long_acl.stubs(:match?).returns(true)
>>> @long_acl.stubs(:allowed?).returns(:returned)
>>>
>>> - @right.allowed?("/path/to/there/and/
>>> there", :args).should == :returned
>>> + res, acl = @right.allowed?("/path/to/there/and/
>>> there", :args)
>>> + res.should == :returned
>>> + end
>>> +
>>> + it "should return the matched acl" do
>>> + @right.newright("/path/to/there", 0)
>>> +
>>> + @long_acl.stubs(:match?).returns(true)
>>> + @long_acl.stubs(:allowed?).returns(:returned)
>>> +
>>> + res, acl = @right.allowed?("/path/to/there/and/
>>> there", :args)
>>> + acl.should == @long_acl
>>> end
>>>
>>> it "should not raise an error if this path acl doesn't
>>> exist" do
>>> @@ -252,7 +263,8 @@ describe Puppet::Network::Rights do
>>> end
>>>
>>> it "should return false if no path match" do
>>> - @right.allowed?("/path", :args).should be_false
>>> + res, acl = @right.allowed?("/path", :args)
>>> + res.should be_false
>>> end
>>> end
>>>
>>> @@ -313,7 +325,18 @@ describe Puppet::Network::Rights do
>>> @regex_acl1.stubs(:match?).returns(true)
>>> @regex_acl1.stubs(:allowed?).returns(:returned)
>>>
>>> - @right.allowed?("/files/repository/myfile/
>>> other", :args).should == :returned
>>> + res, acl = @right.allowed?("/files/repository/
>>> myfile/other", :args)
>>> + res.should == :returned
>>> + end
>>> +
>>> + it "should return the matched acl" do
>>> + @right.newright("~ /files/(.*)/myfile", 0)
>>> +
>>> + @regex_acl1.stubs(:match?).returns(true)
>>> + @regex_acl1.stubs(:allowed?).returns(:returned)
>>> +
>>> + res, acl = @right.allowed?("/files/repository/
>>> myfile/other", :args)
>>> + acl.should == @regex_acl1
>>> end
>>>
>>> it "should not raise an error if no regex acl match" do
>>> @@ -321,7 +344,8 @@ describe Puppet::Network::Rights do
>>> end
>>>
>>> it "should return false if no regex match" do
>>> - @right.allowed?("/path", :args).should be_false
>>> + res, acl = @right.allowed?("/path", :args)
>>> + res.should be_false
>>> end
>>>
>>> end
>>> @@ -403,14 +427,16 @@ 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
>>> + res,acl = @acl.allowed?("me","127.0.0.1", :save)
>>> + res.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
>>> + res, acl = @acl.allowed?("me","127.0.0.1", :save)
>>> + res.should be_true
>>> end
>>>
>>> it "should return :dunno if this right is not restricted
>>> to the given environment" do
>>> @@ -418,7 +444,8 @@ describe Puppet::Network::Rights do
>>>
>>> @acl.restrict_environment(:production)
>>>
>>> - @acl.allowed?
>>> ("me","127.0.0.1", :save, :development).should == :dunno
>>> + res, acl = @acl.allowed?
>>> ("me","127.0.0.1", :save, :development)
>>> + res.should == :dunno
>>> end
>>>
>>> it "should interpolate allow/deny patterns with the
>>> given match" do
>>> diff --git a/test/network/rights.rb b/test/network/rights.rb
>>> index e1d9f8a..89c6797 100755
>>> --- a/test/network/rights.rb
>>> +++ b/test/network/rights.rb
>>> @@ -23,9 +23,13 @@ class TestRights < Test::Unit::TestCase
>>> @store.newright(:write)
>>> }
>>>
>>> - assert(! @store.allowed?(:write, "host.madstop.com",
>>> "0.0.0.0"),
>>> + res, acl = @store.allowed?(:write, "host.madstop.com",
>>> "0.0.0.0")
>>> + assert(! res,
>>> "Defaulted to allowing access")
>>>
>>> + assert(! acl.nil?,
>>> + "returned matching rights is not nil")
>>> +
>>> assert_nothing_raised {
>>> @store[:write].info "This is a log message"
>>> }
>>> --
>>> 1.6.0.2
>>>
>>>
>>
>>
>
>
> --
> Brice Figureau
> Days of Wonder
> http://www.daysofwonder.com
>
> >
--
Don't throw away the old bucket until you know whether the new one
holds water. -- Swedish Proverb
---------------------------------------------------------------------
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
-~----------~----~----~----~------~----~------~--~---
