[tor-commits] [stem/master] Separating bridge scrubbing validation into method

[atagar at torproject.org]

commit 3a5e9979e40a400b8f4861406bc75af32578c30c
Author: Damian Johnson <atagar at torproject.org>
Date:   Sat May 5 15:27:59 2012 -0700

    Separating bridge scrubbing validation into method
    
    Bridge descriptor scrubbing is a moving target and it's quite possible that
    we'll need to parse descriptors that conform to a newer or older scheme. Hence
    making scrubbing validation part of the constructor is inappropriate. Moving it
    to a separate method instead.
    
    This includes two methods, one to simply check if we think that the scrubbing
    is right, and another to get descriptions of the issues. This is of limited
    usefulness to callers, so we might need to switch to an exception hierchy
    later. However, we don't have any use cases that care to check the scrubbing
    yet so leaving that alone for now.
---
 stem/descriptor/server_descriptor.py      |   75 +++++++++++++++++++----------
 test/unit/descriptor/server_descriptor.py |   11 ++--
 2 files changed, 54 insertions(+), 32 deletions(-)

diff --git a/stem/descriptor/server_descriptor.py b/stem/descriptor/server_descriptor.py
index f3023a2..d56b453 100644
--- a/stem/descriptor/server_descriptor.py
+++ b/stem/descriptor/server_descriptor.py
@@ -14,6 +14,8 @@ ServerDescriptorV3 - Tor server descriptor, version 3.
   |  |  +- digest - calculates the digest value for our content
   |  |
   |  +- BridgeDescriptorV3 - Scrubbed server descriptor for a bridge.
+  |     |- is_scrubbed - checks if our content has been properly scrubbed
+  |     +- get_scrubbing_issues - description of issues with our scrubbing
   |
   |- get_unrecognized_lines - lines with unrecognized content
   |- get_annotations - dictionary of content prior to the descriptor entry
@@ -705,6 +707,7 @@ class BridgeDescriptorV3(ServerDescriptorV3):
   
   def __init__(self, raw_contents, validate = True, annotations = None):
     self.address_alt = []
+    self._scrubbing_issues = None
     ServerDescriptorV3.__init__(self, raw_contents, validate, annotations)
   
   def _parse(self, entries, validate):
@@ -742,36 +745,56 @@ class BridgeDescriptorV3(ServerDescriptorV3):
         del entries["or-address"]
     
     ServerDescriptorV3._parse(self, entries, validate)
-    if validate: self._check_scrubbing()
   
-  def _check_scrubbing(self):
+  def is_scrubbed(self):
     """
-    Checks that our attributes have been scrubbed in accordance with the bridge
-    descriptor specification.
+    Checks if we've been properly scrubbed in accordance with the bridge
+    descriptor specification. Validation is a moving target so this may not
+    be fully up to date.
+    
+    Returns:
+      True if we're scrubbed, False otherwise
+    """
+    
+    return self.get_scrubbing_issues() == []
+  
+  def get_scrubbing_issues(self):
+    """
+    Provides issues with our scrubbing.
+    
+    Returns:
+      list of strings which describe issues we have with our scrubbing, this
+      list is empty if we're properly scrubbed
     """
     
-    if self.nickname != "Unnamed":
-      raise ValueError("Router line's nickname should be scrubbed to be 'Unnamed': %s" % self.nickname)
-    elif not self.address.startswith("10."):
-      raise ValueError("Router line's address should be scrubbed to be '10.x.x.x': %s" % self.address)
-    elif self.contact and self.contact != "somebody":
-      raise ValueError("Contact line should be scrubbed to be 'somebody', but instead had '%s'" % self.contact)
-    
-    for address, _, is_ipv6 in self.address_alt:
-      if not is_ipv6 and not address.startswith("10."):
-        raise ValueError("or-address line's address should be scrubbed to be '10.x.x.x': %s" % address)
-      elif is_ipv6 and not address.startswith("fd9f:2e19:3bcf::"):
-        # TODO: this check isn't quite right because we aren't checking that
-        # the next grouping of hex digits contains 1-2 digits
-        raise ValueError("or-address line's address should be scrubbed to be 'fd9f:2e19:3bcf::xx:xxxx': %s" % address)
-    
-    for line in self.get_unrecognized_lines():
-      if line.startswith("onion-key "):
-        raise ValueError("Bridge descriptors should have their onion-key scrubbed: %s" % line)
-      elif line.startswith("signing-key "):
-        raise ValueError("Bridge descriptors should have their signing-key scrubbed: %s" % line)
-      elif line.startswith("router-signature "):
-        raise ValueError("Bridge descriptors should have their signature scrubbed: %s" % line)
+    if self._scrubbing_issues == None:
+      issues = []
+      if self.nickname != "Unnamed":
+        issues.append("Router line's nickname should be scrubbed to be 'Unnamed': %s" % self.nickname)
+      elif not self.address.startswith("10."):
+        issues.append("Router line's address should be scrubbed to be '10.x.x.x': %s" % self.address)
+      elif self.contact and self.contact != "somebody":
+        issues.append("Contact line should be scrubbed to be 'somebody', but instead had '%s'" % self.contact)
+      
+      for address, _, is_ipv6 in self.address_alt:
+        if not is_ipv6 and not address.startswith("10."):
+          issues.append("or-address line's address should be scrubbed to be '10.x.x.x': %s" % address)
+        elif is_ipv6 and not address.startswith("fd9f:2e19:3bcf::"):
+          # TODO: this check isn't quite right because we aren't checking that
+          # the next grouping of hex digits contains 1-2 digits
+          issues.append("or-address line's address should be scrubbed to be 'fd9f:2e19:3bcf::xx:xxxx': %s" % address)
+      
+      for line in self.get_unrecognized_lines():
+        if line.startswith("onion-key "):
+          issues.append("Bridge descriptors should have their onion-key scrubbed: %s" % line)
+        elif line.startswith("signing-key "):
+          issues.append("Bridge descriptors should have their signing-key scrubbed: %s" % line)
+        elif line.startswith("router-signature "):
+          issues.append("Bridge descriptors should have their signature scrubbed: %s" % line)
+      
+      self._scrubbing_issues = issues
+    
+    return self._scrubbing_issues
   
   def _required_fields(self):
     # bridge required fields are the same as a relay descriptor, minus items
diff --git a/test/unit/descriptor/server_descriptor.py b/test/unit/descriptor/server_descriptor.py
index fceb773..101cdcd 100644
--- a/test/unit/descriptor/server_descriptor.py
+++ b/test/unit/descriptor/server_descriptor.py
@@ -346,11 +346,9 @@ class TestServerDescriptor(unittest.TestCase):
     ]
     
     for attr in unsanitized_attr:
-      try:
-        desc_text = _make_descriptor(attr, is_bridge = True)
-        BridgeDescriptorV3(desc_text)
-        self.fail("Unsanitized attribute wasn't detected: %s %s" % attr.items()[0])
-      except ValueError: pass
+      desc_text = _make_descriptor(attr, is_bridge = True)
+      desc = BridgeDescriptorV3(desc_text)
+      self.assertFalse(desc.is_scrubbed())
   
   def test_bridge_unsanitized_relay(self):
     """
@@ -359,7 +357,8 @@ class TestServerDescriptor(unittest.TestCase):
     """
     
     desc_text = _make_descriptor()
-    self.assertRaises(ValueError, BridgeDescriptorV3, desc_text)
+    desc = BridgeDescriptorV3(desc_text)
+    self.assertFalse(desc.is_scrubbed())
   
   def test_or_address_v4(self):
     """