[#3860] Checkpoint: todo vendor VALUE

diff --git a/src/hooks/dhcp/radius/client_attribute.h b/src/hooks/dhcp/radius/client_attribute.h
index db00b2e2e5ca9710688d07dc2e2c6a59ca72f36f..8d2879b1caf0079e7adbcfe4c8b343765422fd94 100644 (file)
--- a/src/hooks/dhcp/radius/client_attribute.h
+++ b/src/hooks/dhcp/radius/client_attribute.h
@@ -162,6 +162,7 @@ public:
     /// @brief From Vendor ID and string data with type.
     ///
     /// @note Requires the type to be of a standard vsa attribute.
+    /// @note Used only in unit tests.
     ///
     /// @param type type of attribute.
     /// @param vendor vendor id.
@@ -190,6 +191,8 @@ public:
 
     /// @brief Returns text representation of the attribute.
     ///
+    /// @note Used for logs.
+    ///
     /// @param indent number of spaces before printing text.
     /// @return string with text representation.
     virtual std::string toText(size_t indent = 0) const = 0;
@@ -260,10 +263,15 @@ public:
     /// @brief Type.
     const uint8_t type_;
 
+    /// @note No need for a vendor member as vendor attributes
+    /// are only temporary.
+
 private:
 
     /// @brief From text with definition.
     ///
+    /// Dispatch over the value type.
+    ///
     /// @param def pointer to attribute definition.
     /// @param value textual value.
     /// @return pointer to the attribute.
@@ -273,6 +281,8 @@ private:
 
     /// @brief From bytes with definition.
     ///
+    /// Dispatch over the value type.
+    ///
     /// @param def pointer to attribute definition.
     /// @param value binary value.
     /// @return pointer to the attribute.
@@ -805,6 +815,8 @@ private:
 
 
 /// @brief Collection of attributes.
+///
+/// Designed to not handle vendor attributes so can be keyed by type only.
 class Attributes : public data::CfgToElement {
 public:
 
@@ -891,6 +903,8 @@ public:
 
     /// @brief Returns text representation of the collection.
     ///
+    /// @note Used for logs.
+    ///
     /// @param indent number of spaces before printing text.
     /// @return string with text representation.
     std::string toText(size_t indent = 0) const;

diff --git a/src/hooks/dhcp/radius/client_dictionary.cc b/src/hooks/dhcp/radius/client_dictionary.cc
index 4a48c27b4c107946ce1d0e21a7d9480ddd94ecc3..270611146043de7eeb5ba27a7987a64affdd6912 100644 (file)
--- a/src/hooks/dhcp/radius/client_dictionary.cc
+++ b/src/hooks/dhcp/radius/client_dictionary.cc
@@ -254,11 +254,15 @@ AttrDefs::parseLine(const string& line, uint32_t& vendor, unsigned int depth) {
     }
     // Integer constant definition.
     if (tokens[0] == "VALUE") {
+        if (vendor != 0) {
+            // Ignore vendor constant definitions.
+            return;
+        }
         if (tokens.size() != 4) {
             isc_throw(Unexpected, "expected 4 tokens, got " << tokens.size());
         }
         const string& attr_str = tokens[1];
-        AttrDefPtr attr = getByName(attr_str);
+        AttrDefPtr attr = getByName(attr_str/*, vendor*/);
         if (!attr) {
             isc_throw(Unexpected, "unknown attribute '" << attr_str << "'");
         }

diff --git a/src/hooks/dhcp/radius/radius_parsers.cc b/src/hooks/dhcp/radius/radius_parsers.cc
index 261a8fd448180bf45c9ecf6860cabd3388632ffa..dc4526bbe8bd3e18107a8ede1aad290542ba299a 100644 (file)
--- a/src/hooks/dhcp/radius/radius_parsers.cc
+++ b/src/hooks/dhcp/radius/radius_parsers.cc
@@ -519,7 +519,15 @@ RadiusAttributeParser::parse(const RadiusServicePtr& service,
 
     // vendor.
     uint32_t vendor = 0;
-    const string& vendor_txt = getString(attr, "vendor");
+    const ConstElementPtr& vendor_elem = attr->get("vendor");
+    if (!vendor_elem) {
+        // Should not happen as it is added by setDefaults.
+        isc_throw(Unexpected, "no vendor parameter");
+    } else if (vendor_elem->getType() != Element::string) {
+        // Expected to be a common error.
+        isc_throw(TypeError, "vendor parameter must be a string");
+    }
+    const string& vendor_txt = vendor_elem->stringValue();
     if (!vendor_txt.empty()) {
         IntCstDefPtr vendor_cst =
             AttrDefs::instance().getByName(PW_VENDOR_SPECIFIC, vendor_txt);
@@ -534,7 +542,8 @@ RadiusAttributeParser::parse(const RadiusServicePtr& service,
                 }
                 vendor = static_cast<uint32_t>(val);
             } catch (...) {
-                isc_throw(ConfigError, "can't parse vendor " << vendor_txt);
+                isc_throw(ConfigError, "can't parse vendor '"
+                          << vendor_txt << "'");
             }
         }
     }

diff --git a/src/hooks/dhcp/radius/tests/config_unittests.cc b/src/hooks/dhcp/radius/tests/config_unittests.cc
index 4753d208e415d1c3f7aa22388157332a7a88fdf5..1a9f85943c159812a220c50174e63e552083c30e 100644 (file)
--- a/src/hooks/dhcp/radius/tests/config_unittests.cc
+++ b/src/hooks/dhcp/radius/tests/config_unittests.cc
@@ -53,12 +53,30 @@ public:
     virtual ~ConfigTest() {
         impl_.reset();
         HostDataSourceFactory::deregisterFactory("cache");
+        static_cast<void>(remove(TEST_FILE));
+    }
+
+    /// @brief writes specified content to a file.
+    ///
+    /// @param file_name name of file to be written.
+    /// @param content content to be written to file.
+    void writeFile(const std::string& file_name, const std::string& content) {
+        static_cast<void>(remove(file_name.c_str()));
+        ofstream out(file_name.c_str(), ios::trunc);
+        EXPECT_TRUE(out.is_open());
+        out << content;
+        out.close();
     }
 
     /// @brief Radius implementation.
     RadiusImpl& impl_;
+
+    /// Name of a dictionary file used during tests.
+    static const char* TEST_FILE;
 };
 
+const char* ConfigTest::TEST_FILE  = "test-dictonary";
+
 // Verify that a configuration must be a map.
 TEST_F(ConfigTest, notMap) {
     ElementPtr config = Element::createList();
@@ -177,6 +195,15 @@ TEST_F(ConfigTest, badDictionary) {
     expected += "No such file or directory";
     EXPECT_THROW_MSG(impl_.init(config), ConfigError, expected);
 
+    impl_.reset();
+    string dict = "BEGIN-VENDOR 1234\n";
+    writeFile(TEST_FILE, dict);
+    config = Element::createMap();
+    config->set("dictionary", Element::create(string(TEST_FILE)));
+    expected = "vendor definitions were not properly closed: ";
+    expected += "vendor 1234 is still open";
+    EXPECT_THROW_MSG(impl_.init(config), ConfigError, expected);
+
     impl_.reset();
     config = Element::createMap();
     config->set("dictionary", Element::create(string(TEST_DICTIONARY)));
@@ -730,6 +757,12 @@ TEST_F(ConfigTest, attribute) {
     ElementPtr config = Element::createMap();
     EXPECT_NO_THROW(impl_.init(config));
 
+    // Add vendor too.
+    IntCstDefPtr cstv(new IntCstDef(PW_VENDOR_SPECIFIC, "DSL-Forum", 3561));
+    ASSERT_NO_THROW(AttrDefs::instance().add(cstv));
+    AttrDefPtr defv(new AttrDef(1, "Agent-Circuit-Id", PW_TYPE_STRING, 3561));
+    ASSERT_NO_THROW(AttrDefs::instance().add(defv));
+
     RadiusServicePtr srv(new RadiusService("test"));
     ASSERT_TRUE(srv);
     RadiusAttributeParser parser;
@@ -740,6 +773,17 @@ TEST_F(ConfigTest, attribute) {
                      "get(string) called on a non-map Element");
     EXPECT_TRUE(srv->attributes_.empty());
 
+    // Vendor must be a string.
+    attr = Element::createMap();
+    attr->set("vendor", Element::create(1234));
+    EXPECT_THROW_MSG(parser.parse(srv, attr), TypeError,
+                     "vendor parameter must be a string");
+
+    // Named vendor must be known.
+    attr->set("vendor", Element::create("foobar"));
+    EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
+                     "can't parse vendor 'foobar'");
+
     // Name or type is required.
     attr = Element::createMap();
     attr->set("data", Element::create("foobar"));
@@ -752,12 +796,33 @@ TEST_F(ConfigTest, attribute) {
     EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
                      "attribute 'No-Such-Attribute' is unknown");
 
+    attr->set("name", Element::create("User-Name"));
+    attr->set("vendor", Element::create("DSL-Forum"));
+    EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
+                     "attribute 'User-Name' in vendor 'DSL-Forum' is unknown");
+    attr->set("vendor", Element::create("1234"));
+    EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
+                     "attribute 'User-Name' in vendor '1234' is unknown");
+    attr->set("vendor", Element::create(""));
+
     // Type and name must match.
     attr->set("name", Element::create("User-Name"));
     attr->set("type", Element::create(123));
     EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
                      "'User-Name' attribute has type 1, not 123");
 
+    attr->set("name", Element::create("Agent-Circuit-Id"));
+    attr->set("vendor", Element::create("DSL-Forum"));
+    string expected = "'Agent-Circuit-Id' attribute in vendor 'DSL-Forum' ";
+    expected += "has type 1, not 123";
+    EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, expected);
+    attr->set("vendor", Element::create("3561"));
+    expected = "'Agent-Circuit-Id' attribute in vendor '3561' ";
+    expected += "has type 1, not 123";
+    EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError, expected);
+    attr->set("name", Element::create("User-Name"));
+    attr->set("vendor", Element::create(""));
+
     // Type must be between 0 and 255.
     attr = Element::createMap();
     attr->set("data", Element::create("foobar"));
@@ -777,6 +842,12 @@ TEST_F(ConfigTest, attribute) {
     attr->set("type", Element::create(234));
     EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
                      "attribute type 234 is unknown");
+    attr->set("vendor", Element::create("DSL-Forum"));
+    EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
+                     "attribute type 234 in vendor 'DSL-Forum' is unknown");
+    attr->set("vendor", Element::create("1234"));
+    EXPECT_THROW_MSG(parser.parse(srv, attr), ConfigError,
+                     "attribute type 234 in vendor '1234' is unknown");
 
     // Set a type.
     attr = Element::createMap();
@@ -786,21 +857,43 @@ TEST_F(ConfigTest, attribute) {
 
     // Get the option to look at into.
     EXPECT_FALSE(srv->attributes_.empty());
+    EXPECT_EQ(1, srv->attributes_.size());
+    EXPECT_TRUE(srv->attributes_.getDef(1));
     EXPECT_FALSE(srv->attributes_.getExpr(1));
     EXPECT_EQ("", srv->attributes_.getTest(1));
-    const Attributes& attrs = srv->attributes_.getAll();
-    ASSERT_EQ(1, attrs.size());
-    const ConstAttributePtr& first = *attrs.cbegin();
-    ASSERT_TRUE(first);
-    EXPECT_EQ("User-Name='foobar'", first->toText());
+    ConstAttributePtr got = srv->attributes_.get(1);
+    ASSERT_TRUE(got);
+    EXPECT_EQ("User-Name='foobar'", got->toText());
 
     // Another way to check.
-    string expected = "[ { "
+    expected = "[ { "
         " \"name\": \"User-Name\", "
         " \"type\": 1, "
         " \"data\": \"foobar\" } ]";
     runToElementTest<CfgAttributes>(expected, srv->attributes_);
 
+    // Check with a vendor.
+    srv->attributes_.clear();
+    attr = Element::createMap();
+    attr->set("vendor", Element::create("DSL-Forum"));
+    attr->set("data", Element::create("foobar"));
+    attr->set("type", Element::create(1));
+    EXPECT_NO_THROW(parser.parse(srv, attr));
+    EXPECT_FALSE(srv->attributes_.empty());
+    EXPECT_EQ(1, srv->attributes_.size());
+    EXPECT_TRUE(srv->attributes_.getDef(1, 3561));
+    EXPECT_FALSE(srv->attributes_.getExpr(1, 3561));
+    EXPECT_EQ("", srv->attributes_.getTest(1, 3561));
+    got = srv->attributes_.get(1, 3561);
+    ASSERT_TRUE(got);
+    EXPECT_EQ("Vendor-Specific=[3561]0x0108666F6F626172", got->toText());
+    expected = "[ { "
+        " \"name\": \"Vendor-Specific\", "
+        " \"type\": 26, "
+        " \"vendor\": \"3561\", "
+        " \"vsa-raw\": \"0108666F6F626172\" } ]";
+    runToElementTest<CfgAttributes>(expected, srv->attributes_);
+
     // Vendor-Specific (26) does not support textual data.
     srv->attributes_.clear();
     attr = Element::createMap();

diff --git a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc
index 9b16e7051d8e81fd5f8771e0e132bf07760dbfec..436a02722c20221804523206db57584cc60b0bac 100644 (file)
--- a/src/hooks/dhcp/radius/tests/dictionary_unittests.cc
+++ b/src/hooks/dhcp/radius/tests/dictionary_unittests.cc
@@ -47,7 +47,7 @@ public:
     /// @brief Destructor.
     virtual ~DictionaryTest() {
         AttrDefs::instance().clear();
-        static_cast<void>(remove(TEST_DICT));
+        static_cast<void>(remove(TEST_FILE));
     }
 
     /// @brief Parse a line.
@@ -95,10 +95,10 @@ public:
     }
 
     /// Name of a dictionary file used during tests.
-    static const char* TEST_DICT;
+    static const char* TEST_FILE;
 };
 
-const char* DictionaryTest::TEST_DICT  = "test-dict";
+const char* DictionaryTest::TEST_FILE  = "test-dictionary";
 
 // Verifies standards definitions can be read from the dictionary.
 TEST_F(DictionaryTest, standard) {
@@ -420,15 +420,15 @@ TEST_F(DictionaryTest, include) {
 
 // Verifies the $INCLUDE entry can't eat the stack.
 TEST_F(DictionaryTest, includeLimit) {
-    string include = "$INCLUDE " + string(TEST_DICT) + "\n";
-    writeFile(TEST_DICT, include);
+    string include = "$INCLUDE " + string(TEST_FILE) + "\n";
+    writeFile(TEST_FILE, include);
     string expected = "Too many nested $INCLUDE ";
-    expected += "at line 1 in dictionary 'test-dict', ";
-    expected += "at line 1 in dictionary 'test-dict', ";
-    expected += "at line 1 in dictionary 'test-dict', ";
-    expected += "at line 1 in dictionary 'test-dict', ";
+    expected += "at line 1 in dictionary 'test-dictionary', ";
+    expected += "at line 1 in dictionary 'test-dictionary', ";
+    expected += "at line 1 in dictionary 'test-dictionary', ";
+    expected += "at line 1 in dictionary 'test-dictionary', ";
     expected += "at line 1";
-    EXPECT_THROW_MSG(parseLine(string("$INCLUDE ") + string(TEST_DICT)),
+    EXPECT_THROW_MSG(parseLine(string("$INCLUDE ") + string(TEST_FILE)),
                      BadValue, expected);
 }