Fixing #531 and #414. This includes pretty much a complete redesign

of the AuthStore class, with (hopefully) all of the edge cases removed,
the code is now much cleaner, the tests are (I think) complete, and
everything is just generally better.



git-svn-id: https://reductivelabs.com/svn/puppet/trunk@2262 980ebf18-57e1-0310-9a29-db15c13687c0

3 files changed, 571 insertions, 265 deletions

diff --git a/lib/puppet/network/authstore.rb b/lib/puppet/network/authstore.rb
index 51ce93d46..a7851d48b 100755
--- a/lib/puppet/network/authstore.rb
+++ b/lib/puppet/network/authstore.rb
@@ -2,29 +2,30 @@
 # the requested resource
 
 require 'ipaddr'
+require 'puppet/util/logging'
 
 module Puppet
     class AuthStoreError < Puppet::Error; end
     class AuthorizationError < Puppet::Error; end
 
     class Network::AuthStore
-        # This has to be an array, not a hash, else it loses its ordering.
-        ORDER = [
-            [:ip, [:ip]],
-            [:name, [:hostname, :domain]]
-        ]
-
-        Puppet::Util.logmethods(self, true)
+        include Puppet::Util::Logging
 
+        # Mark a given pattern as allowed.
         def allow(pattern)
             # a simple way to allow anyone at all to connect
             if pattern == "*"
                 @globalallow = true
             else
-                store(pattern, @allow)
+                store(:allow, pattern)
             end
+
+            return nil
         end
 
+        # Is a given combination of name and ip address allowed?  If either input
+        # is non-nil, then both inputs must be provided.  If neither input
+        # is provided, then the authstore is considered local and defaults to "true".
         def allowed?(name, ip)
             if name or ip
                 # This is probably unnecessary, and can cause some weirdnesses in
@@ -40,186 +41,251 @@ module Puppet
             end
 
             # yay insecure overrides
-            if @globalallow
+            if globalallow?
                 return true
             end
 
-            value = nil
-            ORDER.each { |nametype, array|
-                if nametype == :ip
-                    value = IPAddr.new(ip)
-                else
-                    value = name.split(".").reverse
-                end
-
-
-                array.each { |type|
-                    [[@deny, false], [@allow, true]].each { |ary|
-                        hash, retval = ary
-                        if hash.include?(type)
-                            hash[type].each { |pattern|
-                                if match?(nametype, value, pattern)
-                                    return retval
-                                end
-                            }
-                        end
-                    }
-                }
-            }
+            if decl = @declarations.find { |d| d.match?(name, ip) }
+                return decl.result
+            end
 
             self.info "defaulting to no access for %s" % name
-            # default to false
             return false
         end
 
+        # Deny a given pattern.
         def deny(pattern)
-            store(pattern, @deny)
+            store(:deny, pattern)
+        end
+
+        # Is global allow enabled?
+        def globalallow?
+            @globalallow
         end
 
         def initialize
             @globalallow = nil
-            @allow = Hash.new { |hash, key|
-                hash[key] = []
-            }
-            @deny = Hash.new { |hash, key|
-                hash[key] = []
-            }
+            @declarations = []
         end
 
         private
 
-        def match?(nametype, value, pattern)
-            if value == pattern # simplest shortcut
-                return true
-            end
+        # Store the results of a pattern into our hash.  Basically just
+        # converts the pattern and sticks it into the hash.
+        def store(type, pattern)
+            @declarations << Declaration.new(type, pattern)
+            @declarations.sort!
 
-            case nametype
-            when :ip: matchip?(value, pattern)
-            when :name: matchname?(value, pattern)
-            else
-                raise Puppet::DevError, "Invalid match type %s" % nametype
-            end
+            return nil
         end
 
-        def matchip?(value, pattern)
-            # we're just using builtin stuff for this, thankfully
-            if pattern.include?(value)
-                return true
-            else
-                return false
-            end
-        end
+        # A single declaration.  Stores the info for a given declaration,
+        # provides the methods for determining whether a declaration matches,
+        # and handles sorting the declarations appropriately.
+        class Declaration
+            include Puppet::Util
+            include Comparable
 
-        def matchname?(value, pattern)
-            # yay, horribly inefficient
-            if pattern[-1] != '*' # the pattern has no metachars and is not equal
-                                    # thus, no match
-                #Puppet.info "%s is not equal with no * in %s" % [value, pattern]
-                return false
-            else
-                # we know the last field of the pattern is '*'
-                # if everything up to that doesn't match, we're definitely false
-                if pattern[0..-2] != value[0..pattern.length-2]
-                    #Puppet.notice "subpatterns didn't match; %s vs %s" %
-                    #    [pattern[0..-2], value[0..pattern.length-2]]
-                    return false
+            # The type of declaration: either :allow or :deny
+            attr_reader :type
+
+            # The name: :ip or :domain
+            attr_accessor :name
+
+            # The pattern we're matching against.  Can be an IPAddr instance,
+            # or an array of strings, resulting from reversing a hostname
+            # or domain name.
+            attr_reader :pattern
+
+            # The length.  Only used for iprange and domain.
+            attr_accessor :length
+
+            # Sort the declarations specially.
+            def <=>(other)
+                # Sort first based on whether the matches are exact.
+                if r = compare(exact?, other.exact?)
+                    return r
                 end
 
-                case value.length <=> pattern.length
-                when -1: # value is shorter than pattern
-                    if pattern.length - value.length == 1
-                        # only ever allowed when the value is the domain of a
-                        # splatted pattern
-                        #Puppet.info "allowing splatted domain %s" % [value]
-                        return true
-                    else
-                        return false
-                    end
-                when 0: # value is the same length as pattern
-                    if pattern[-1] == "*"
-                        #Puppet.notice "same length with *"
-                        return true
-                    else
-                        return false
-                    end
-                when 1: # value is longer than pattern
-                    # at this point we've already verified that everything up to
-                    # the '*' in the pattern matches, so we are true
-                    return true
+                # Then by type
+                if r = compare(self.ip?, other.ip?)
+                    return r
+                end
+
+                # Next sort based on length
+                unless self.length == other.length
+                    # Longer names/ips should go first, because they're more
+                    # specific.
+                    return other.length <=> self.length
+                end
+
+                # Then sort deny before allow
+                if r = compare(self.deny?, other.deny?)
+                    return r
+                end
+
+                # We've already sorted by name and length, so all that's left
+                # is the pattern
+                if ip?
+                    return self.pattern.to_s <=> other.pattern.to_s
+                else
+                    return self.pattern <=> other.pattern
                 end
             end
-        end
 
-        def store(pattern, hash)
-            type, value = type(pattern)
+            def deny?
+                self.type == :deny
+            end
 
-            if type and value
-                # this won't work once we get beyond simple stuff...
-                hash[type] << value
-            else
-                raise AuthStoreError, "Invalid pattern %s" % pattern
+            # Are we an exact match?
+            def exact?
+                self.length.nil?
+            end
+
+            def initialize(type, pattern)
+                self.type = type
+                self.pattern = pattern
             end
-        end
 
-        def type(pattern)
-            type = value = nil
-            case pattern
-            when /^(\d+\.){3}\d+$/:
-                type = :ip
-                begin
-                    value = IPAddr.new(pattern)
-                rescue ArgumentError => detail
-                    raise AuthStoreError, "Invalid IP address pattern %s" % pattern
+            # Are we an IP type?
+            def ip?
+                self.name == :ip
+            end
+
+            # Does this declaration match the name/ip combo?
+            def match?(name, ip)
+                if self.ip?
+                    return pattern.include?(IPAddr.new(ip))
+                else
+                    return matchname?(name)
                 end
-            when /^(\d+\.){3}\d+\/(\d+)$/:
-                mask = Integer($2)
-                if mask < 1 or mask > 32
-                    raise AuthStoreError, "Invalid IP mask %s" % mask
+            end
+
+            # Set the pattern appropriately.  Also sets the name and length.
+            def pattern=(pattern)
+                parse(pattern)
+                @orig = pattern
+            end
+
+            # Mapping a type of statement into a return value.
+            def result
+                case @type
+                when :allow: true
+                else
+                    false
                 end
-                type = :ip
-                begin
-                    value = IPAddr.new(pattern)
-                rescue ArgumentError => detail
-                    raise AuthStoreError, "Invalid IP address pattern %s" % pattern
+            end
+
+            def to_s
+                "%s: %s" % [self.type, self.pattern]
+            end
+
+            # Set the declaration type.  Either :allow or :deny.
+            def type=(type)
+                type = symbolize(type)
+                unless [:allow, :deny].include?(type)
+                    raise ArgumentError, "Invalid declaration type %s" % type
                 end
-            when /^(\d+\.){1,3}\*$/: # an ip address with a '*' at the end
-                type = :ip
-                match = $1
-                match.sub!(".", '')
-                ary = pattern.split(".")
-
-                mask = case ary.index(match)
-                when 0: 8
-                when 1: 16
-                when 2: 24
-                else
-                    raise AuthStoreError, "Invalid IP pattern %s" % pattern
+                @type = type
+            end
+
+            private
+
+            # Returns nil if both values are true or both are false, returns
+            # -1 if the first is true, and 1 if the second is true.  Used
+            # in the <=> operator.
+            def compare(me, them)
+                unless me and them
+                    if me
+                        return -1
+                    elsif them
+                        return 1
+                    else
+                        return false
+                    end
                 end
+                return nil
+            end
+
+            # Does the name match our pattern?
+            def matchname?(name)
+                name = munge_name(name)
+                return true if self.pattern == name
 
-                ary.pop
-                while ary.length < 4
-                    ary.push("0")
+                # If it's an exact match, then just return false, since the
+                # exact didn't match.
+                if exact?
+                    return false
                 end
 
-                begin
-                    value = IPAddr.new(ary.join(".") + "/" + mask.to_s)
-                rescue ArgumentError => detail
-                    raise AuthStoreError, "Invalid IP address pattern %s" % pattern
+                # If every field in the pattern matches, then we consider it
+                # a match.
+                pattern.zip(name) do |p,n|
+                    unless p == n
+                        return false
+                    end
                 end
-            when /^[\d.]+$/: # necessary so incomplete IP addresses can't look
-                             # like hostnames
-                raise AuthStoreError, "Invalid IP address pattern %s" % pattern
-            when /^([a-zA-Z][-\w]*\.)+[-\w]+$/: # a full hostname
-                type = :hostname
-                value = pattern.split(".").reverse
-            when /^\*(\.([a-zA-Z][-\w]*)){1,}$/:
-                type = :domain
-                value = pattern.split(".").reverse
-            else
-                raise AuthStoreError, "Invalid pattern %s" % pattern
+
+                return true
             end
 
-            return [type, value]
+            # Convert the name to a common pattern.
+            def munge_name(name)
+                name.downcase.split(".").reverse
+            end
+
+            # Parse our input pattern and figure out what kind of allowal
+            # statement it is.  The output of this is used for later matching.
+            def parse(value)
+                case value
+                when /^(\d+\.){1,3}\*$/: # an ip address with a '*' at the end
+                    @name = :ip
+                    match = $1
+                    match.sub!(".", '')
+                    ary = value.split(".")
+
+                    mask = case ary.index(match)
+                    when 0: 8
+                    when 1: 16
+                    when 2: 24
+                    else
+                        raise AuthStoreError, "Invalid IP pattern %s" % value
+                    end
+
+                    @length = mask
+
+                    ary.pop
+                    while ary.length < 4
+                        ary.push("0")
+                    end
+
+                    begin
+                        @pattern = IPAddr.new(ary.join(".") + "/" + mask.to_s)
+                    rescue ArgumentError => detail
+                        raise AuthStoreError, "Invalid IP address pattern %s" % value
+                    end
+                when /^([a-zA-Z][-\w]*\.)+[-\w]+$/: # a full hostname
+                    @name = :domain
+                    @pattern = munge_name(value)
+                when /^\*(\.([a-zA-Z][-\w]*)){1,}$/: # *.domain.com
+                    @name = :domain
+                    @pattern = munge_name(value)
+                    @pattern.pop # take off the '*'
+                    @length = @pattern.length
+                else
+                    # Else, use the IPAddr class to determine if we've got a
+                    # valid IP address.
+                    if value =~ /\/(\d+)$/
+                        @length = Integer($1)
+                    end
+                    begin
+                        @pattern = IPAddr.new(value)
+                    rescue ArgumentError => detail
+                        raise AuthStoreError, "Invalid pattern %s" % value
+                    end
+                    @name = :ip
+                end
+            end
         end
     end
 end
diff --git a/test/network/authstore.rb b/test/network/authstore.rb
index 99763bb3b..ae407c96e 100755
--- a/test/network/authstore.rb
+++ b/test/network/authstore.rb
@@ -3,11 +3,12 @@
 $:.unshift("../lib") if __FILE__ =~ /\.rb$/
 
 require 'puppettest'
-
+require 'mocha'
 require 'puppet/network/authstore'
 
 class TestAuthStore < Test::Unit::TestCase
 	include PuppetTest
+    Declaration = Puppet::Network::AuthStore::Declaration
     def mkstore
         store = nil
         assert_nothing_raised {
@@ -17,116 +18,56 @@ class TestAuthStore < Test::Unit::TestCase
         return store
     end
 
-    def test_localallow
-        store = mkstore
+    def setup
+        super
+        @store = mkstore
+    end
 
+    def test_localallow
+        Puppet[:trace] = false
         assert_nothing_raised {
-            assert(store.allowed?(nil, nil), "Store disallowed local access")
+            assert(@store.allowed?(nil, nil), "Store disallowed local access")
         }
 
         assert_raise(Puppet::DevError) {
-            store.allowed?("kirby.madstop.com", nil)
+            @store.allowed?("kirby.madstop.com", nil)
         }
 
         assert_raise(Puppet::DevError) {
-            store.allowed?(nil, "192.168.0.1")
-        }
-    end
-
-    def test_hostnames
-        store = mkstore
-
-        %w{
-            kirby.madstop.com
-            luke.madstop.net
-            name-other.madstop.net
-        }.each { |name|
-            assert_nothing_raised("Failed to store simple name %s" % name) {
-                store.allow(name)
-            }
-            assert(store.allowed?(name, "192.168.0.1"), "Name %s not allowed" % name)
-        }
-
-        %w{
-            invalid
-            ^invalid!
-            inval$id
-        
-        }.each { |pat|
-            assert_raise(Puppet::AuthStoreError,
-                "name '%s' was allowed" % pat) {
-                store.allow(pat)
-            }
-        }
-    end
-
-    def test_domains
-        store = mkstore
-
-        assert_nothing_raised("Failed to store domains") {
-            store.allow("*.a.very.long.domain.name.com")
-            store.allow("*.madstop.com")
-            store.allow("*.some-other.net")
-            store.allow("*.much.longer.more-other.net")
-        }
-
-        %w{
-            madstop.com
-            culain.madstop.com
-            kirby.madstop.com
-            funtest.some-other.net
-            ya-test.madstop.com
-            some.much.much.longer.more-other.net
-        }.each { |name|
-            assert(store.allowed?(name, "192.168.0.1"), "Host %s not allowed" % name)
-        }
-
-        assert_raise(Puppet::AuthStoreError) {
-            store.allow("domain.*.com")
-        }
-
-        assert(!store.allowed?("very.long.domain.name.com", "1.2.3.4"),
-            "Long hostname allowed")
-
-        assert_raise(Puppet::AuthStoreError) {
-            store.allow("domain.*.other.com")
+            @store.allowed?(nil, "192.168.0.1")
         }
     end
 
     def test_simpleips
-        store = mkstore
-
         %w{
             192.168.0.5
             7.0.48.7
         }.each { |ip|
-            assert_nothing_raised("Failed to store IP address %s" % ip) {
-                store.allow(ip)
+            assert_nothing_raised("Failed to @store IP address %s" % ip) {
+                @store.allow(ip)
             }
 
-            assert(store.allowed?("hosttest.com", ip), "IP %s not allowed" % ip)
+            assert(@store.allowed?("hosttest.com", ip), "IP %s not allowed" % ip)
         }
 
         #assert_raise(Puppet::AuthStoreError) {
-        #    store.allow("192.168.674.0")
+        #    @store.allow("192.168.674.0")
         #}
 
         assert_raise(Puppet::AuthStoreError) {
-            store.allow("192.168.0")
+            @store.allow("192.168.0")
         }
     end
 
     def test_ipranges
-        store = mkstore
-
         %w{
             192.168.0.*
             192.168.1.0/24
             192.178.*
             193.179.0.0/8
         }.each { |range|
-            assert_nothing_raised("Failed to store IP range %s" % range) {
-                store.allow(range)
+            assert_nothing_raised("Failed to @store IP range %s" % range) {
+                @store.allow(range)
             }
         }
 
@@ -136,55 +77,47 @@ class TestAuthStore < Test::Unit::TestCase
             192.178.0.5
             193.0.0.1
         }.each { |ip|
-            assert(store.allowed?("fakename.com", ip), "IP %s is not allowed" % ip)
+            assert(@store.allowed?("fakename.com", ip), "IP %s is not allowed" % ip)
         }
     end
 
     def test_iprangedenials
-        store = mkstore
-
-        assert_nothing_raised("Failed to store overlapping IP ranges") {
-            store.allow("192.168.0.0/16")
-            store.deny("192.168.0.0/24")
+        assert_nothing_raised("Failed to @store overlapping IP ranges") {
+            @store.allow("192.168.0.0/16")
+            @store.deny("192.168.0.0/24")
         }
 
-        assert(store.allowed?("fake.name", "192.168.1.50"), "/16 ip not allowed")
-        assert(! store.allowed?("fake.name", "192.168.0.50"), "/24 ip allowed")
+        assert(@store.allowed?("fake.name", "192.168.1.50"), "/16 ip not allowed")
+        assert(! @store.allowed?("fake.name", "192.168.0.50"), "/24 ip allowed")
     end
 
     def test_subdomaindenails
-        store = mkstore
-
-        assert_nothing_raised("Failed to store overlapping IP ranges") {
-            store.allow("*.madstop.com")
-            store.deny("*.sub.madstop.com")
+        assert_nothing_raised("Failed to @store overlapping IP ranges") {
+            @store.allow("*.madstop.com")
+            @store.deny("*.sub.madstop.com")
         }
 
-        assert(store.allowed?("hostname.madstop.com", "192.168.1.50"),
+        assert(@store.allowed?("hostname.madstop.com", "192.168.1.50"),
             "hostname not allowed")
-        assert(! store.allowed?("name.sub.madstop.com", "192.168.0.50"),
+        assert(! @store.allowed?("name.sub.madstop.com", "192.168.0.50"),
             "subname name allowed")
     end
 
     def test_orderingstuff
-        store = mkstore
-
-        assert_nothing_raised("Failed to store overlapping IP ranges") {
-            store.allow("*.madstop.com")
-            store.deny("192.168.0.0/24")
+        assert_nothing_raised("Failed to @store overlapping IP ranges") {
+            @store.allow("*.madstop.com")
+            @store.deny("192.168.0.0/24")
         }
 
-        assert(store.allowed?("hostname.madstop.com", "192.168.1.50"),
+        assert(@store.allowed?("hostname.madstop.com", "192.168.1.50"),
             "hostname not allowed")
-        assert(! store.allowed?("hostname.madstop.com", "192.168.0.50"),
+        assert(! @store.allowed?("hostname.madstop.com", "192.168.0.50"),
             "Host allowed over IP")
     end
 
     def test_globalallow
-        store = mkstore
-
         assert_nothing_raised("Failed to add global allow") {
-            store.allow("*")
+            @store.allow("*")
         }
 
         [
@@ -193,17 +126,323 @@ class TestAuthStore < Test::Unit::TestCase
             %w{localhost 127.0.0.1}
             
         ].each { |ary|
-            assert(store.allowed?(*ary), "Failed to allow %s" % [ary.join(",")])
+            assert(@store.allowed?(*ary), "Failed to allow %s" % [ary.join(",")])
         }
     end
 
+    def test_store
+        assert_nothing_raised do
+            assert_nil(@store.send(:store, :allow, "*.host.com"),
+                "store did not return nil")
+        end
+        assert_equal([Declaration.new(:allow, "*.host.com")],
+            @store.send(:instance_variable_get, "@declarations"),
+            "Did not store declaration")
+
+        # Now add another one and make sure it gets sorted appropriately
+        assert_nothing_raised do
+            assert_nil(@store.send(:store, :allow, "me.host.com"),
+                "store did not return nil")
+        end
+
+        assert_equal([
+            Declaration.new(:allow, "me.host.com"),
+            Declaration.new(:allow, "*.host.com")
+        ],
+            @store.send(:instance_variable_get, "@declarations"),
+            "Did not sort declarations")
+    end
+
+    def test_allow_and_deny
+        store = Puppet::Network::AuthStore.new
+        store.expects(:store).with(:allow, "host.com")
+        store.allow("host.com")
+
+        store = Puppet::Network::AuthStore.new
+        store.expects(:store).with(:deny, "host.com")
+        store.deny("host.com")
+
+        store = Puppet::Network::AuthStore.new
+        assert_nothing_raised do
+            assert_nil(store.allow("*"),
+                "allow did not return nil")
+        end
+
+        assert(store.globalallow?,
+            "did not enable global allow")
+    end
+
+    def test_hostnames
+        Puppet[:trace] = false
+        %w{
+            kirby.madstop.com
+            luke.madstop.net
+            name-other.madstop.net
+        }.each { |name|
+            assert_nothing_raised("Failed to @store simple name %s" % name) {
+                @store.allow(name)
+            }
+            assert(@store.allowed?(name, "192.168.0.1"), "Name %s not allowed" % name)
+        }
+
+        %w{
+            invalid
+            ^invalid!
+            inval$id
+        
+        }.each { |pat|
+            assert_raise(Puppet::AuthStoreError,
+                "name '%s' was allowed" % pat) {
+                @store.allow(pat)
+            }
+        }
+    end
+
+    def test_domains
+        assert_nothing_raised("Failed to @store domains") {
+            @store.allow("*.a.very.long.domain.name.com")
+            @store.allow("*.madstop.com")
+            @store.allow("*.some-other.net")
+            @store.allow("*.much.longer.more-other.net")
+        }
+
+        %w{
+            madstop.com
+            culain.madstop.com
+            kirby.madstop.com
+            funtest.some-other.net
+            ya-test.madstop.com
+            some.much.much.longer.more-other.net
+        }.each { |name|
+            assert(@store.allowed?(name, "192.168.0.1"), "Host %s not allowed" % name)
+        }
+
+        assert_raise(Puppet::AuthStoreError) {
+            @store.allow("domain.*.com")
+        }
+
+        assert(!@store.allowed?("very.long.domain.name.com", "1.2.3.4"),
+            "Long hostname allowed")
+
+        assert_raise(Puppet::AuthStoreError) {
+            @store.allow("domain.*.other.com")
+        }
+    end
+
+    # #531
+    def test_case_insensitivity
+        @store.allow("hostname.com")
+
+        %w{hostname.com Hostname.COM hostname.Com HOSTNAME.COM}.each do |name|
+            assert(@store.allowed?(name, "127.0.0.1"),
+                "did not allow %s" % name)
+        end
+    end
+
+    def test_allowed?
+        Puppet[:trace] = false
+        assert(@store.allowed?(nil, nil),
+            "Did not default to true for local checks")
+        assert_raise(Puppet::DevError, "did not fail on one input") do
+            @store.allowed?("host.com", nil)
+        end
+        assert_raise(Puppet::DevError, "did not fail on one input") do
+            @store.allowed?(nil, "192.168.0.1")
+        end
+
+    end
+
+    # Make sure more specific allows and denies win over generalities
+    def test_specific_overrides
+        @store.allow("host.madstop.com")
+        @store.deny("*.madstop.com")
+
+        assert(@store.allowed?("host.madstop.com", "192.168.0.1"),
+            "More specific allowal by name failed")
+
+        @store.allow("192.168.0.1")
+        @store.deny("192.168.0.0/24")
+
+        assert(@store.allowed?("host.madstop.com", "192.168.0.1"),
+            "More specific allowal by ip failed")
+    end
+end
+
+class TestAuthStoreDeclaration < PuppetTest::TestCase
+	include PuppetTest
+    Declaration = Puppet::Network::AuthStore::Declaration
+
+    def setup
+        super
+        @decl = Declaration.new(:allow, "hostname.com")
+    end
+
+    def test_parse
+        {
+            "192.168.0.1" =>        [:ip, IPAddr.new("192.168.0.1"), nil],
+            "2001:700:300:1800::" => [:ip, IPAddr.new("2001:700:300:1800::"), nil],
+            "2001:700:300:1800::/64" => [:ip, IPAddr.new("2001:700:300:1800::/64"), 64],
+            "192.168.0.1/32" =>     [:ip, IPAddr.new("192.168.0.1/32"), 32],
+            "192.168.0.1/24" =>     [:ip, IPAddr.new("192.168.0.1/24"), 24],
+            "192.*" =>              [:ip, IPAddr.new("192.0.0.0/8"), 8],
+            "192.168.*" =>          [:ip, IPAddr.new("192.168.0.0/16"), 16],
+            "192.168.0.*" =>        [:ip, IPAddr.new("192.168.0.0/24"), 24],
+            "hostname.com" =>       [:domain, %w{com hostname}, nil],
+            "Hostname.COM" =>       [:domain, %w{com hostname}, nil],
+            "billy.Hostname.COM" => [:domain, %w{com hostname billy}, nil],
+            "billy-jean.Hostname.COM" => [:domain, %w{com hostname billy-jean}, nil],
+            "*.hostname.COM" => [:domain, %w{com hostname}, 2],
+            "*.hostname.COM" => [:domain, %w{com hostname}, 2]
+        }.each do |input, output|
+
+            # Create a new decl each time, so values aren't cached.
+            assert_nothing_raised do
+                @decl = Declaration.new(:allow, input)
+            end
+
+            [:name, :pattern, :length].zip(output).each do |method, value|
+                assert_equal(value, @decl.send(method),
+                    "Got incorrect value for %s from %s" % [method, input])
+            end
+        end
+
+        %w{192.168 hostname -hostname.com hostname.*}.each do |input|
+            assert_raise(Puppet::AuthStoreError, "Did not fail on %s" % input) do
+                @decl.pattern = input
+            end
+        end
+
+        ["hostname .com", "192.168 .0.1"].each do |input|
+            assert_raise(Puppet::AuthStoreError, "Did not fail on %s" % input) do
+                @decl.pattern = input
+            end
+        end
+    end
+
+    def test_result
+        ["allow", :allow].each do |val|
+            assert_nothing_raised { @decl.type = val }
+            assert_equal(true, @decl.result, "did not result to true with %s" %
+                val.inspect)
+        end
+
+        [:deny, "deny"].each do |val|
+            assert_nothing_raised { @decl.type = val }
+            assert_equal(false, @decl.result,
+                "did not result to false with %s" % val.inspect)
+        end
+
+        ["yay", 1, nil, false, true].each do |val|
+            assert_raise(ArgumentError, "Did not fail on %s" % val.inspect) do
+                @decl.type = val
+            end
+        end
+    end
+
+    def test_munge_name
+        {
+            "hostname.com" => %w{com hostname},
+            "alley.hostname.com" => %w{com hostname alley},
+            "*.hostname.com" => %w{com hostname *},
+            "*.HOSTNAME.Com" => %w{com hostname *},
+            "*.HOSTNAME.Com" => %w{com hostname *},
+
+        }.each do |input, output|
+            assert_equal(output, @decl.send(:munge_name, input),
+                "munged %s incorrectly" % input)
+        end
+    end
+
     # Make sure people can specify TLDs
     def test_match_tlds
-        store = mkstore
-
         assert_nothing_raised {
-            store.allow("*.tld")
+            @decl.pattern = "*.tld"
         }
+
+        assert_equal(%w{tld}, @decl.pattern, "Failed to allow custom tld")
+    end
+
+    # Make sure we sort correctly.
+    def test_sorting
+        # Make sure declarations with no length sort first.
+        host_exact = Declaration.new(:allow, "host.com")
+        host_range = Declaration.new(:allow, "*.host.com")
+
+        ip_exact = Declaration.new(:allow, "192.168.0.1")
+        ip_range = Declaration.new(:allow, "192.168.0.*")
+
+        assert_equal(-1, host_exact <=> host_range,
+            "exact name match did not sort first")
+
+        assert_equal(-1, ip_exact <=> ip_range,
+            "exact ip match did not sort first")
+
+        # Next make sure we sort by length
+        ip_long = Declaration.new(:allow, "192.168.*")
+        assert_equal(-1, ip_range <=> ip_long, "/16 sorted before /24 in ip")
+
+        # Now try it using masks
+        ip24 = Declaration.new(:allow, "192.168.0.0/24")
+        ip16 = Declaration.new(:allow, "192.168.0.0/16")
+
+        assert_equal(-1, ip24 <=> ip16, "/16 sorted before /24 in ip with masks")
+
+        # Make sure ip checks sort before host checks
+        assert_equal(-1, ip_exact <=> host_exact,
+            "IP exact did not sort before host exact")
+
+        assert_equal(-1, ip_range <=> host_range,
+            "IP range did not sort before host range")
+
+        host_long = Declaration.new(:allow, "*.domain.host.com")
+
+        assert_equal(-1, host_long <=> host_range, "did not sort by domain length")
+
+        # Now make sure denies sort before allows, for equivalent
+        # declarations.
+        host_deny = Declaration.new(:deny, "host.com")
+        assert_equal(-1, host_deny <=> host_exact, "deny did not sort before allow when exact")
+
+        host_range_deny = Declaration.new(:deny, "*.host.com")
+        assert_equal(-1, host_range_deny <=> host_range,
+            "deny did not sort before allow when ranged")
+
+        ip_allow = Declaration.new(:allow, "192.168.0.0/16")
+        ip_deny = Declaration.new(:deny, "192.168.0.0/16")
+
+        assert_equal(-1, ip_deny <=> ip_allow,
+            "deny did not sort before allow in ip range")
+
+        %w{host.com *.domain.com 192.168.0.1 192.168.0.1/24}.each do |decl|
+            assert_equal(0, Declaration.new(:allow, decl) <=>
+                Declaration.new(:allow, decl),
+                "Equivalent declarations for %s were considered different" %
+                decl
+            )
+        end
+    end
+
+    def test_match?
+        host = Declaration.new(:allow, "host.com")
+        host.expects(:matchname?).with("host.com")
+        host.match?("host.com", "192.168.0.1")
+
+        ip = Declaration.new(:allow, "192.168.0.1")
+        ip.pattern.expects(:include?)
+        ip.match?("host.com", "192.168.0.1")
+    end
+
+    def test_matchname?
+        host = Declaration.new(:allow, "host.com")
+        assert(host.send(:matchname?, "host.com"), "exact did not match")
+        assert(! host.send(:matchname?, "yay.com"), "incorrect match")
+
+        domain = Declaration.new(:allow, "*.domain.com")
+        %w{host.domain.com domain.com very.long.domain.com very-long.domain.com
+        }.each do |name|
+            assert(domain.send(:matchname?, name),
+                "Did not match %s" % name)
+        end
     end
 end
 
diff --git a/test/network/rights.rb b/test/network/rights.rb
index a9b61545f..edbcaac80 100755
--- a/test/network/rights.rb
+++ b/test/network/rights.rb
@@ -8,28 +8,29 @@ require 'puppet/network/rights'
 class TestRights < Test::Unit::TestCase
 	include PuppetTest
 
-    def test_rights
-        store = nil
-        assert_nothing_raised {
-            store = Puppet::Network::Rights.new
-        }
+    def setup
+        super
+        @store = Puppet::Network::Rights.new
+    end
 
-        assert(store, "Did not create store")
 
+    def test_rights
         assert_raise(ArgumentError, "Did not fail on unknown right") {
-            store.allowed?(:write, "host.madstop.com", "0.0.0.0")
+            @store.allowed?(:write, "host.madstop.com", "0.0.0.0")
         }
 
         assert_nothing_raised {
-            store.newright(:write)
+            @store.newright(:write)
         }
 
-        assert(! store.allowed?(:write, "host.madstop.com", "0.0.0.0"),
+        assert(! @store.allowed?(:write, "host.madstop.com", "0.0.0.0"),
             "Defaulted to allowing access")
 
         assert_nothing_raised {
-            store[:write].info "This is a log message"
+            @store[:write].info "This is a log message"
         }
+
+        assert_logged(:info, /This is a log message/, "did not log from Rights")
     end
 end