Added constrains to id generator
This commit prevents id with more than one '/'. Slash can be used to
create namespaces to avoid id conflicts between multiple users.
Changed generator logic to take provided id as a name if no name was
provided.
Tested:
- Tested that id and name generates correctly
- It is no longer possible to add id with more than one '/'
Signed-off-by: Krzysztof Grobelny <krzysztof.grobelny@intel.com>
Change-Id: Ieef32ddb71b5a4870117aab0d624cbd46b5893e6
diff --git a/src/report.cpp b/src/report.cpp
index ed869b9..2b7134a 100644
--- a/src/report.cpp
+++ b/src/report.cpp
@@ -250,7 +250,6 @@
"ReportUpdates", std::string(),
sdbusplus::vtable::property_::emits_change,
[this](auto newVal, auto& oldVal) {
- ReportManager::verifyReportUpdates(utils::toReportUpdates(newVal));
setReportUpdates(utils::toReportUpdates(newVal));
oldVal = newVal;
return 1;
diff --git a/src/report_manager.cpp b/src/report_manager.cpp
index 18636f8..7ac8537 100644
--- a/src/report_manager.cpp
+++ b/src/report_manager.cpp
@@ -124,16 +124,6 @@
reports.end());
}
-void ReportManager::verifyReportIdLength(const std::string& reportId)
-{
- if (reportId.length() > maxReportIdLength)
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Report id exceeds maximum length");
- }
-}
-
void ReportManager::verifyAddReport(
const std::string& reportId, const std::string& reportName,
const ReportingType reportingType, Milliseconds interval,
@@ -154,20 +144,6 @@
"Reached maximal report count");
}
- verifyReportIdLength(reportId);
- utils::verifyIdCharacters(reportId);
-
- for (const auto& report : reports)
- {
- if (report->getId() == reportId)
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::file_exists), "Duplicate report");
- }
- }
-
- verifyReportUpdates(reportUpdates);
-
if (reportingType == ReportingType::periodic && interval < minInterval)
{
throw sdbusplus::exception::SdBusError(
@@ -222,17 +198,11 @@
std::vector<LabeledMetricParameters> labeledMetricParams,
const bool enabled)
{
- std::string name = reportName;
- if (name.empty())
- {
- name = "Report";
- }
-
const auto existingReportIds = utils::transform(
reports, [](const auto& report) { return report->getId(); });
- std::string id =
- utils::generateId(reportId, name, existingReportIds, maxReportIdLength);
+ auto [id, name] = utils::generateId(reportId, reportName, reportNameDefault,
+ existingReportIds, maxReportIdLength);
verifyAddReport(id, name, reportingType, interval, reportUpdates,
labeledMetricParams);
@@ -304,14 +274,3 @@
}
}
}
-
-void ReportManager::verifyReportUpdates(const ReportUpdates reportUpdates)
-{
- if (std::find(supportedReportUpdates.begin(), supportedReportUpdates.end(),
- reportUpdates) == supportedReportUpdates.end())
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid ReportUpdates");
- }
-}
diff --git a/src/report_manager.hpp b/src/report_manager.hpp
index 5a850f0..34f98e0 100644
--- a/src/report_manager.hpp
+++ b/src/report_manager.hpp
@@ -39,7 +39,6 @@
std::unique_ptr<sdbusplus::asio::dbus_interface> reportManagerIface;
std::vector<std::unique_ptr<interfaces::Report>> reports;
- void verifyReportIdLength(const std::string& reportName);
void verifyAddReport(
const std::string& reportId, const std::string& reportName,
const ReportingType reportingType, Milliseconds interval,
@@ -70,9 +69,5 @@
"xyz.openbmc_project.Telemetry.ReportManager";
static constexpr const char* reportManagerPath =
"/xyz/openbmc_project/Telemetry/Reports";
- static constexpr std::array<ReportUpdates, 3> supportedReportUpdates = {
- ReportUpdates::overwrite, ReportUpdates::appendStopsWhenFull,
- ReportUpdates::appendWrapsWhenFull};
-
- static void verifyReportUpdates(const ReportUpdates reportUpdates);
+ static constexpr const char* reportNameDefault = "Report";
};
diff --git a/src/trigger_manager.cpp b/src/trigger_manager.cpp
index 358ee78..c0b20d7 100644
--- a/src/trigger_manager.cpp
+++ b/src/trigger_manager.cpp
@@ -61,7 +61,6 @@
"Reached maximal trigger count");
}
- verifyTriggerIdLength(triggerId);
utils::verifyIdCharacters(triggerId);
for (const auto& trigger : triggers)
@@ -74,26 +73,6 @@
}
}
-void TriggerManager::verifyTriggerIdLength(const std::string& triggerId)
-{
- if (triggerId.length() > maxTriggerIdLength)
- {
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Trigger id exceeds maximum length");
- }
-}
-
-std::string TriggerManager::generateId(const std::string& prefix,
- const std::string& triggerName) const
-{
- const auto existingTriggerIds = utils::transform(
- triggers, [](const auto& trigger) { return trigger->getId(); });
-
- return utils::generateId(prefix, triggerName, existingTriggerIds,
- maxTriggerIdLength);
-}
-
interfaces::Trigger& TriggerManager::addTrigger(
const std::string& triggerIdIn, const std::string& triggerNameIn,
const std::vector<std::string>& triggerActions,
@@ -101,19 +80,18 @@
const std::vector<std::string>& reportIds,
const LabeledTriggerThresholdParams& labeledThresholdParams)
{
- std::string triggerName = triggerNameIn;
- if (triggerName.empty())
- {
- triggerName = triggerNameDefault;
- }
+ const auto existingTriggerIds = utils::transform(
+ triggers, [](const auto& trigger) { return trigger->getId(); });
- std::string triggerId = generateId(triggerIdIn, triggerName);
+ auto [id, name] =
+ utils::generateId(triggerIdIn, triggerNameIn, triggerNameDefault,
+ existingTriggerIds, maxTriggerIdLength);
- verifyAddTrigger(triggerId, triggerName);
+ verifyAddTrigger(id, name);
triggers.emplace_back(triggerFactory->make(
- triggerId, triggerName, triggerActions, reportIds, *this,
- *triggerStorage, labeledThresholdParams, labeledSensorsInfo));
+ id, name, triggerActions, reportIds, *this, *triggerStorage,
+ labeledThresholdParams, labeledSensorsInfo));
return *triggers.back();
}
diff --git a/src/trigger_manager.hpp b/src/trigger_manager.hpp
index d11e57a..72b6274 100644
--- a/src/trigger_manager.hpp
+++ b/src/trigger_manager.hpp
@@ -33,9 +33,6 @@
void verifyAddTrigger(const std::string& triggerId,
const std::string& triggerName) const;
- std::string generateId(const std::string& prefix,
- const std::string& triggerName) const;
- static void verifyTriggerIdLength(const std::string& triggerId);
interfaces::Trigger&
addTrigger(const std::string& triggerId, const std::string& triggerName,
diff --git a/src/types/report_updates.hpp b/src/types/report_updates.hpp
index a47e8fb..2640bf0 100644
--- a/src/types/report_updates.hpp
+++ b/src/types/report_updates.hpp
@@ -13,8 +13,7 @@
{
overwrite,
appendStopsWhenFull,
- appendWrapsWhenFull,
- newReport
+ appendWrapsWhenFull
};
namespace utils
@@ -31,16 +30,13 @@
}
};
-constexpr std::array<std::pair<std::string_view, ReportUpdates>, 4>
- convDataReportUpdates = {
- {std::make_pair<std::string_view, ReportUpdates>(
- "Overwrite", ReportUpdates::overwrite),
- std::make_pair<std::string_view, ReportUpdates>(
- "AppendStopsWhenFull", ReportUpdates::appendStopsWhenFull),
- std::make_pair<std::string_view, ReportUpdates>(
- "AppendWrapsWhenFull", ReportUpdates::appendWrapsWhenFull),
- std::make_pair<std::string_view, ReportUpdates>(
- "NewReport", ReportUpdates::newReport)}};
+constexpr auto convDataReportUpdates =
+ std::array{std::make_pair<std::string_view, ReportUpdates>(
+ "Overwrite", ReportUpdates::overwrite),
+ std::make_pair<std::string_view, ReportUpdates>(
+ "AppendStopsWhenFull", ReportUpdates::appendStopsWhenFull),
+ std::make_pair<std::string_view, ReportUpdates>(
+ "AppendWrapsWhenFull", ReportUpdates::appendWrapsWhenFull)};
inline ReportUpdates
toReportUpdates(std::underlying_type_t<ReportUpdates> value)
diff --git a/src/utils/generate_id.cpp b/src/utils/generate_id.cpp
index adb1a5b..6631e69 100644
--- a/src/utils/generate_id.cpp
+++ b/src/utils/generate_id.cpp
@@ -12,28 +12,44 @@
static constexpr std::string_view allowedCharactersInId =
"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789_/";
-}
-
-void verifyIdCharacters(std::string_view triggerId)
+size_t countDigits(size_t value)
{
- if (triggerId.find_first_not_of(details::allowedCharactersInId) !=
- std::string::npos)
+ size_t result = 1;
+ while (value >= 10)
{
- throw sdbusplus::exception::SdBusError(
- static_cast<int>(std::errc::invalid_argument),
- "Invalid character in id");
+ ++result;
+ value /= 10;
}
+ return result;
}
-std::string generateId(std::string_view prefix, std::string_view name,
+std::string generateId(std::string_view id, std::string_view name,
const std::vector<std::string>& conflictIds,
size_t maxLength)
{
- verifyIdCharacters(prefix);
+ verifyIdCharacters(id);
- if (!prefix.empty() && !prefix.ends_with('/'))
+ if (id.starts_with('/'))
{
- return std::string(prefix);
+ id = id.substr(1);
+ }
+
+ if ((id.length() > maxLength) ||
+ (id.ends_with('/') && id.length() >= maxLength))
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument), "Id too long");
+ }
+
+ if (!id.empty() && !id.ends_with('/'))
+ {
+ if (std::find(conflictIds.begin(), conflictIds.end(), id) !=
+ conflictIds.end())
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::file_exists), "Duplicated id");
+ }
+ return std::string(id);
}
std::string strippedId(name);
@@ -45,7 +61,7 @@
std::string_view::npos;
}),
strippedId.end());
- strippedId = std::string(prefix) + strippedId;
+ strippedId = std::string(id) + strippedId;
size_t idx = 0;
std::string tmpId = strippedId.substr(0, maxLength);
@@ -54,11 +70,65 @@
conflictIds.end() ||
tmpId.empty())
{
- tmpId = strippedId.substr(0, maxLength - std::to_string(idx).length()) +
- std::to_string(idx);
+ size_t digitsInIdx = countDigits(idx);
+
+ if (digitsInIdx > maxLength)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::file_exists),
+ "Unique indices are depleted");
+ }
+
+ tmpId =
+ strippedId.substr(0, maxLength - digitsInIdx) + std::to_string(idx);
++idx;
}
+
return tmpId;
}
+} // namespace details
+
+void verifyIdCharacters(std::string_view id)
+{
+ if (id.find_first_not_of(details::allowedCharactersInId) !=
+ std::string::npos)
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Invalid character in id");
+ }
+
+ if (auto pos = id.find_first_of("/");
+ pos != std::string::npos && pos != id.find_last_of("/"))
+ {
+ throw sdbusplus::exception::SdBusError(
+ static_cast<int>(std::errc::invalid_argument),
+ "Too many '/' in id");
+ }
+}
+
+std::pair<std::string, std::string> generateId(
+ std::string_view id, std::string_view name, std::string_view defaultName,
+ const std::vector<std::string>& conflictIds, const size_t maxLength)
+{
+ if (name.empty() && !id.ends_with('/'))
+ {
+ name = id;
+
+ if (auto pos = name.find_last_of("/"); pos != std::string::npos)
+ {
+ name = name.substr(pos + 1);
+ }
+ }
+
+ if (name.empty())
+ {
+ name = defaultName;
+ }
+
+ return std::make_pair(details::generateId(id, name, conflictIds, maxLength),
+ std::string{name});
+}
+
} // namespace utils
diff --git a/src/utils/generate_id.hpp b/src/utils/generate_id.hpp
index aa9508e..94952dc 100644
--- a/src/utils/generate_id.hpp
+++ b/src/utils/generate_id.hpp
@@ -7,9 +7,9 @@
namespace utils
{
-void verifyIdCharacters(std::string_view triggerId);
-std::string generateId(std::string_view prefix, std::string_view triggerName,
- const std::vector<std::string>& conflictIds,
- size_t maxLength);
+void verifyIdCharacters(std::string_view id);
+std::pair<std::string, std::string> generateId(
+ std::string_view id, std::string_view name, std::string_view defaultName,
+ const std::vector<std::string>& conflictIds, const size_t maxLength);
} // namespace utils
diff --git a/tests/src/test_generate_id.cpp b/tests/src/test_generate_id.cpp
index 0bfabf5..412fc9b 100644
--- a/tests/src/test_generate_id.cpp
+++ b/tests/src/test_generate_id.cpp
@@ -1,67 +1,171 @@
#include "utils/generate_id.hpp"
+#include <sdbusplus/exception.hpp>
+
#include <gmock/gmock.h>
using namespace testing;
+using namespace std::literals::string_literals;
-class TestGenerateId : public Test
+class ScenarioBase : public Test
{
public:
+ std::string_view defaultName = "defName";
+ size_t maxSize = 32;
std::vector<std::string> conflicts;
};
-TEST_F(TestGenerateId, returnPrefixWhenPrefixIsId)
+class ScenarioNameProvided : public ScenarioBase
{
- constexpr size_t maxSize = 32;
- EXPECT_THAT(utils::generateId("prefix", "name", conflicts, maxSize),
- Eq("prefix"));
- EXPECT_THAT(utils::generateId("pre", "name123", conflicts, maxSize),
- Eq("pre"));
- EXPECT_THAT(utils::generateId("prefix/abc", "name", conflicts, maxSize),
- Eq("prefix/abc"));
- EXPECT_THAT(utils::generateId("prefix/abc", "name",
- {"conflicts", "prefix/abc"}, maxSize),
- Eq("prefix/abc"));
+ public:
+ auto generateId(std::string_view id, std::string_view name) const
+ {
+ return utils::generateId(id, name, defaultName, conflicts, maxSize);
+ }
+};
+
+class TestGenerateIdNameProvided : public ScenarioNameProvided
+{};
+
+TEST_F(TestGenerateIdNameProvided, usesIdWhenProvided)
+{
+ const std::string name = "name";
+
+ EXPECT_THAT(this->generateId("id0", name), Eq(std::pair{"id0"s, name}));
+ EXPECT_THAT(this->generateId("/id1", name), Eq(std::pair{"id1"s, name}));
+ EXPECT_THAT(this->generateId("prefix/id2", name),
+ Eq(std::pair{"prefix/id2"s, name}));
}
-TEST_F(TestGenerateId, returnsNameWithPrefixWhenPrefixIsNamesapce)
+TEST_F(TestGenerateIdNameProvided, usedDefaultWhenNothingProvided)
{
- constexpr size_t maxSize = 32;
- EXPECT_THAT(utils::generateId("prefix/", "name", conflicts, maxSize),
- Eq("prefix/name"));
- EXPECT_THAT(utils::generateId("pre/", "name", conflicts, maxSize),
- Eq("pre/name"));
+ this->defaultName = "def";
+
+ EXPECT_THAT(this->generateId("", ""), Eq(std::pair{"def"s, "def"s}));
+ EXPECT_THAT(this->generateId("/", ""), Eq(std::pair{"def"s, "def"s}));
+ EXPECT_THAT(this->generateId("abc/", ""),
+ Eq(std::pair{"abc/def"s, "def"s}));
}
-TEST_F(TestGenerateId, returnsOriginalNameWithoutInvalidCharacters)
+class ScenarioNameNotProvided : public ScenarioBase
{
- constexpr size_t maxSize = 32;
- EXPECT_THAT(utils::generateId("", "n#a$m@e", conflicts, maxSize),
- Eq("name"));
- EXPECT_THAT(utils::generateId("/", "n!^aŹme", conflicts, maxSize),
- Eq("/name"));
- EXPECT_THAT(utils::generateId("p/", "n!^aŹm*(e", conflicts, maxSize),
- Eq("p/name"));
+ public:
+ auto generateId(std::string_view id, std::string_view name) const
+ {
+ return utils::generateId(id, "", name, conflicts, maxSize);
+ }
+};
+
+class TestGenerateIdNameNotProvided : public ScenarioNameNotProvided
+{};
+
+TEST_F(TestGenerateIdNameNotProvided, usesIdAsNameWhenProvided)
+{
+ EXPECT_THAT(this->generateId("id0", defaultName),
+ Eq(std::pair{"id0"s, "id0"s}));
+ EXPECT_THAT(this->generateId("/id1", defaultName),
+ Eq(std::pair{"id1"s, "id1"s}));
+ EXPECT_THAT(this->generateId("prefix/id2", defaultName),
+ Eq(std::pair{"prefix/id2"s, "id2"s}));
}
-TEST_F(TestGenerateId, returnsUniqueIdWhenConflictExist)
-{
- constexpr size_t maxSize = 32;
+template <class Scenario>
+class TestGenerateId : public Scenario
+{};
- EXPECT_THAT(utils::generateId("p/", "name",
- {"conflicts", "p/name", "p/name1"}, maxSize),
- Eq("p/name0"));
- EXPECT_THAT(utils::generateId("p/", "name",
- {"conflicts", "p/name", "p/name0", "p/name1"},
- maxSize),
- Eq("p/name2"));
+using TestScenarios =
+ ::testing::Types<ScenarioNameProvided, ScenarioNameNotProvided>;
+TYPED_TEST_SUITE(TestGenerateId, TestScenarios);
+
+TEST_F(TestGenerateIdNameNotProvided, leadingSlashDoesntCountToLengthLimit)
+{
+ this->maxSize = 2;
+
+ EXPECT_THAT(this->generateId("12", "default").first, Eq("12"));
+ EXPECT_THAT(this->generateId("/23", "default").first, Eq("23"));
+ EXPECT_THROW(this->generateId("123", "trigger"),
+ sdbusplus::exception::SdBusError);
}
-TEST_F(TestGenerateId, returnsUniqueIdWithingMaxSize)
+TYPED_TEST(TestGenerateId, throwsWhenProvidedIdIsTooLong)
{
- constexpr size_t maxSize = 4;
+ this->maxSize = 4;
- EXPECT_THAT(utils::generateId("", "trigger", {""}, maxSize), Eq("trig"));
- EXPECT_THAT(utils::generateId("", "trigger", {"trig"}, maxSize),
- Eq("tri0"));
+ EXPECT_THROW(this->generateId("12345", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->generateId("12/45", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->generateId("123/", "trigger"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestGenerateId, throwsWhenThereAreNoFreeIds)
+{
+ this->maxSize = 1;
+ this->conflicts = {"n", "0", "1", "2", "3", "4", "5", "6", "7", "8", "9"};
+
+ EXPECT_THROW(this->generateId("", "n"), sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestGenerateId, throwsWhenIdContainsMoreThanOneSlash)
+{
+ EXPECT_THROW(this->generateId("/12/", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->generateId("12//", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->generateId("12//123", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->generateId("12/12/123", "name"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestGenerateId, usesNamePartOfNameWhenIdNotProvidedAndNameIsTooLong)
+{
+ this->maxSize = 4;
+
+ this->conflicts = {};
+ EXPECT_THAT(this->generateId("", "trigger"),
+ Eq(std::pair{"trig"s, "trigger"s}));
+ EXPECT_THAT(this->generateId("a/", "trigger"),
+ Eq(std::pair{"a/tr"s, "trigger"s}));
+
+ this->conflicts = {"trig"};
+ EXPECT_THAT(this->generateId("", "trigger"),
+ Eq(std::pair{"tri0"s, "trigger"s}));
+
+ this->conflicts = {"trig", "tri0"};
+ EXPECT_THAT(this->generateId("", "trigger"),
+ Eq(std::pair{"tri1"s, "trigger"s}));
+}
+
+TYPED_TEST(TestGenerateId, throwsWhenProvidedIdIsTaken)
+{
+ this->conflicts = {"id", "prefix/id"};
+
+ EXPECT_THROW(this->generateId("id", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->generateId("/id", "name"),
+ sdbusplus::exception::SdBusError);
+ EXPECT_THROW(this->generateId("prefix/id", "name"),
+ sdbusplus::exception::SdBusError);
+}
+
+TYPED_TEST(TestGenerateId, usesNameWhenIdNotProvided)
+{
+ EXPECT_THAT(this->generateId("", "name"), Eq(std::pair{"name"s, "name"s}));
+ EXPECT_THAT(this->generateId("/", "name"), Eq(std::pair{"name"s, "name"s}));
+ EXPECT_THAT(this->generateId("abc/", "name"),
+ Eq(std::pair{"abc/name"s, "name"s}));
+ EXPECT_THAT(this->generateId("123/", "name"),
+ Eq(std::pair{"123/name"s, "name"s}));
+}
+
+TYPED_TEST(TestGenerateId, usesNameWithoutInvalidCharactersWhenIdNotProvided)
+{
+ EXPECT_THAT(this->generateId("", "n#a$/m@e"),
+ Eq(std::pair{"name"s, "n#a$/m@e"s}));
+ EXPECT_THAT(this->generateId("", "n!^aŹ/me"),
+ Eq(std::pair{"name"s, "n!^aŹ/me"s}));
+ EXPECT_THAT(this->generateId("p/", "n!^aŹ/m*(e"),
+ Eq(std::pair{"p/name"s, "n!^aŹ/m*(e"s}));
}