Refactor rest authorization to raise exceptions deeper

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 <brice-puppet@daysofwonder.com>

5 files changed, 113 insertions, 75 deletions

diff --git a/spec/unit/network/authconfig.rb b/spec/unit/network/authconfig.rb
index 186d30ce3..21af89bfd 100755
--- 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 7b7ef4722..0786d37d2 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 ea5a82cce..7dc97382c 100755
--- 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 15351b172..3acd23f77 100755
--- 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 97094f8e5..be1db723d 100755
--- 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...