Merge pull request #6294 from Icinga/feature/unique-groups-api

Ensure that group memberships on API object creation are unique

lib/base/array-script.cpp

@@ -251,15 +251,7 @@ static Array::Ptr ArrayUnique()
 	ScriptFrame *vframe = ScriptFrame::GetCurrentFrame();
 	Array::Ptr self = static_cast<Array::Ptr>(vframe->Self);
 	REQUIRE_NOT_NULL(self);
-
+	return self->Unique();
-	std::set<Value> result;
-
-	ObjectLock olock(self);
-	for (const Value& item : self) {
-		result.insert(item);
-	}
-
-	return Array::FromSet(result);
 }
 
 static void ArrayFreeze()

lib/base/array.cpp

@@ -305,6 +305,19 @@ String Array::ToString() const
 	return msgbuf.str();
 }
 
+Array::Ptr Array::Unique() const
+{
+	std::set<Value> result;
+
+	ObjectLock olock(this);
+
+	for (const Value& item : m_Data) {
+		result.insert(item);
+	}
+
+	return Array::FromSet(result);
+}
+
 void Array::Freeze()
 {
 	ObjectLock olock(this);

lib/base/array.hpp

@@ -112,6 +112,7 @@ public:
 
 	String ToString() const override;
 
+	Array::Ptr Unique() const;
 	void Freeze();
 
 	Value GetFieldByName(const String& field, bool sandboxed, const DebugInfo& debugInfo) const override;

lib/icinga/hostgroup.cpp

@@ -37,9 +37,9 @@ INITIALIZE_ONCE([]() {
 
 bool HostGroup::EvaluateObjectRule(const Host::Ptr& host, const ConfigItem::Ptr& group)
 {
-	String group_name = group->GetName();
+	String groupName = group->GetName();
 
-	CONTEXT("Evaluating rule for group '" + group_name + "'");
+	CONTEXT("Evaluating rule for group '" + groupName + "'");
 
 	ScriptFrame frame(true);
 	if (group->GetScope())
@@ -50,10 +50,12 @@ bool HostGroup::EvaluateObjectRule(const Host::Ptr& host, const ConfigItem::Ptr&
 		return false;
 
 	Log(LogDebug, "HostGroup")
-		<< "Assigning membership for group '" << group_name << "' to host '" << host->GetName() << "'";
+		<< "Assigning membership for group '" << groupName << "' to host '" << host->GetName() << "'";
 
 	Array::Ptr groups = host->GetGroups();
-	groups->Add(group_name);
+
+	if (groups && !groups->Contains(groupName))
+		groups->Add(groupName);
 
 	return true;
 }

lib/icinga/servicegroup.cpp

@@ -37,9 +37,9 @@ INITIALIZE_ONCE([]() {
 
 bool ServiceGroup::EvaluateObjectRule(const Service::Ptr& service, const ConfigItem::Ptr& group)
 {
-	String group_name = group->GetName();
+	String groupName = group->GetName();
 
-	CONTEXT("Evaluating rule for group '" + group_name + "'");
+	CONTEXT("Evaluating rule for group '" + groupName + "'");
 
 	Host::Ptr host = service->GetHost();
 
@@ -53,10 +53,12 @@ bool ServiceGroup::EvaluateObjectRule(const Service::Ptr& service, const ConfigI
 		return false;
 
 	Log(LogDebug, "ServiceGroup")
-		<< "Assigning membership for group '" << group_name << "' to service '" << service->GetName() << "'";
+		<< "Assigning membership for group '" << groupName << "' to service '" << service->GetName() << "'";
 
 	Array::Ptr groups = service->GetGroups();
-	groups->Add(group_name);
+
+	if (groups && !groups->Contains(groupName))
+		groups->Add(groupName);
 
 	return true;
 }

lib/icinga/usergroup.cpp

@@ -37,9 +37,9 @@ INITIALIZE_ONCE([]() {
 
 bool UserGroup::EvaluateObjectRule(const User::Ptr& user, const ConfigItem::Ptr& group)
 {
-	String group_name = group->GetName();
+	String groupName = group->GetName();
 
-	CONTEXT("Evaluating rule for group '" + group_name + "'");
+	CONTEXT("Evaluating rule for group '" + groupName + "'");
 
 	ScriptFrame frame(true);
 	if (group->GetScope())
@@ -50,10 +50,12 @@ bool UserGroup::EvaluateObjectRule(const User::Ptr& user, const ConfigItem::Ptr&
 		return false;
 
 	Log(LogDebug, "UserGroup")
-		<< "Assigning membership for group '" << group_name << "' to user '" << user->GetName() << "'";
+		<< "Assigning membership for group '" << groupName << "' to user '" << user->GetName() << "'";
 
 	Array::Ptr groups = user->GetGroups();
-	groups->Add(group_name);
+
+	if (groups && !groups->Contains(groupName))
+		groups->Add(groupName);
 
 	return true;
 }

lib/remote/createobjecthandler.cpp

@@ -71,6 +71,14 @@ bool CreateObjectHandler::HandleRequest(const ApiUser::Ptr& user, HttpRequest& r
 		}
 	}
 
+	/* Sanity checks for unique groups array. */
+	if (attrs->Contains("groups")) {
+		Array::Ptr groups = attrs->Get("groups");
+
+		if (groups)
+			attrs->Set("groups", groups->Unique());
+	}
+
 	Dictionary::Ptr result1 = new Dictionary();
 	String status;
 	Array::Ptr errors = new Array();

test/CMakeLists.txt

@@ -63,6 +63,7 @@ add_boost_test(base
     base_array/resize
     base_array/insert
     base_array/remove
+    base_array/unique
     base_array/foreach
     base_array/clone
     base_array/json

test/base-array.cpp

@@ -102,6 +102,27 @@ BOOST_AUTO_TEST_CASE(remove)
 	BOOST_CHECK(array->GetLength() == 0);
 }
 
+BOOST_AUTO_TEST_CASE(unique)
+{
+	Array::Ptr array = new Array();
+	array->Add("group1");
+	array->Add("group2");
+	array->Add("group1");
+	array->Add("group2");
+
+	Array::Ptr result;
+
+	{
+		ObjectLock olock(array);
+		result = array->Unique();
+	}
+
+	BOOST_CHECK(result->GetLength() == 2);
+	result->Sort();
+
+	BOOST_CHECK(result->Get(0) == "group1");
+	BOOST_CHECK(result->Get(1) == "group2");
+}
 BOOST_AUTO_TEST_CASE(foreach)
 {
 	Array::Ptr array = new Array();