Fixes #26075: Trying to save a group with empty criteria removes all entries

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPDiffMapper.scala

@@ -75,7 +75,7 @@ import scala.util.control.NonFatal
 
 class LDAPDiffMapper(
     mapper:          LDAPEntityMapper,
-    cmdbQueryParser: CmdbQueryParser
+    cmdbQueryParser: CmdbQueryParser & RawStringQueryParser
 ) extends Loggable {
 
   // utility method for safe non null

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala

@@ -108,7 +108,7 @@ class LDAPEntityMapper(
     rudderDit:       RudderDit,
     nodeDit:         NodeDit,
     inventoryDit:    InventoryDit,
-    cmdbQueryParser: CmdbQueryParser,
+    cmdbQueryParser: CmdbQueryParser & RawStringQueryParser, // used to read from LDAP, in which case we do not need validation
     inventoryMapper: InventoryMapper
 ) extends NamedZioLogger {
 
@@ -678,7 +678,6 @@ class LDAPEntityMapper(
       for {
         id         <- e.required(A_NODE_GROUP_UUID).flatMap(NodeGroupId.parse(_).left.map(MalformedDN.apply))
         name       <- e.required(A_NAME)
-        query       = e(A_QUERY_NODE_GROUP)
         nodeIds     = e.valuesFor(A_NODE_UUID).map(x => NodeId(x))
         query      <- e(A_QUERY_NODE_GROUP) match {
                         case None    => Right(None)

webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/services/queries/CmdbQueryParser.scala

@@ -76,6 +76,24 @@ object CmdbQueryParser {
   val ATTRIBUTE  = "attribute"
   val COMPARATOR = "comparator"
   val VALUE      = "value"
+
+  /**
+    * Use JSON parser for the query, using strict validation (of comparator values), with criterion objects.
+    */
+  def jsonStrictParser(objects: Map[String, ObjectCriterion]): CmdbQueryParser & StringQueryParser & JsonQueryLexer = {
+    new CmdbQueryParser with DefaultStringQueryParser with JsonQueryLexer {
+      override val criterionObjects: Map[String, ObjectCriterion] = objects
+    }
+  }
+
+  /**
+    * Use the specific JSON parser that bypass validation of criterion values.
+    */
+  def jsonRawParser(objects: Map[String, ObjectCriterion]): CmdbQueryParser & RawStringQueryParser & JsonQueryLexer = {
+    new CmdbQueryParser with RawStringQueryParser with JsonQueryLexer {
+      override val criterionObjects: Map[String, ObjectCriterion] = objects
+    }
+  }
 }
 
 trait QueryLexer {
@@ -88,7 +106,7 @@ trait StringQueryParser {
   def parse(query: StringQuery): Box[Query]
 }
 
-trait CmdbQueryParser extends StringQueryParser with QueryLexer {
+sealed trait CmdbQueryParser extends StringQueryParser with QueryLexer {
   def apply(query: String): Box[Query] = for {
     sq <- lex(query)
     q  <- parse(sq)
@@ -97,26 +115,31 @@ trait CmdbQueryParser extends StringQueryParser with QueryLexer {
 
 /**
  * Some default behaviour:
+ * - validation of criterion value being empty or not is applied
  * - default composition is AND
- *
+ * - transformation is not inverted
  */
 trait DefaultStringQueryParser extends StringQueryParser {
 
+  protected def needValidation:        Boolean              = true
+  protected def defaultComposition:    CriterionComposition = And
+  protected def defaultTransformation: ResultTransformation = ResultTransformation.Identity
+
   def criterionObjects: Map[String, ObjectCriterion]
 
   override def parse(query: StringQuery): Box[Query] = {
 
     for {
       comp  <- query.composition match {
-                 case None    => Full(And)
+                 case None    => Full(defaultComposition)
                  case Some(s) =>
                    CriterionComposition.parse(s) match {
                      case Some(x) => Full(x)
                      case None    => Failure(s"The requested composition '${query.composition}' is not know")
                    }
                }
       trans <- query.transform match {
-                 case None    => Full(ResultTransformation.Identity)
+                 case None    => Full(defaultTransformation)
                  case Some(x) => ResultTransformation.parse(x).toBox
                }
       lines <- traverse(query.criteria)(parseLine)
@@ -145,12 +168,13 @@ trait DefaultStringQueryParser extends StringQueryParser {
 
       /*
        * Only validate the fact that if the comparator requires a value, then a value is provided.
-       * Providing an error when none is required is not an error
+       * An empty String is allowed because "" could have a different meaning from being absent.
+       * Providing an error when none is required is not an error.
        */
       value      <- line.value match {
                       case Some(x) => Right(x)
                       case None    =>
-                        if (comparator.hasValue)
+                        if (needValidation && comparator.hasValue)
                           Left("Missing required value for comparator '%s' in line '%s'".format(line.comparator, line))
                         else Right("")
                     }
@@ -160,6 +184,13 @@ trait DefaultStringQueryParser extends StringQueryParser {
 
 }
 
+/**
+  * The query parser that does not apply validation
+  */
+trait RawStringQueryParser extends DefaultStringQueryParser {
+  final override val needValidation: Boolean = false
+}
+
 /**
  * This lexer read and valid syntax of JSON query
  *

webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/MockServices.scala

@@ -2977,20 +2977,14 @@ class MockLdapQueryParsing(mockGit: MockGitConfigRepo, mockNodeGroups: MockNodeG
   val getSubGroupChoices = new DefaultSubGroupComparatorRepository(mockNodeGroups.groupsRepo)
   val nodeQueryData      = new NodeQueryCriteriaData(() => getSubGroupChoices)
   val ditQueryDataImpl   = new DitQueryData(acceptedNodesDitImpl, nodeDit, rudderDit, nodeQueryData)
-  val queryParser: CmdbQueryParser with DefaultStringQueryParser with JsonQueryLexer = new CmdbQueryParser
-    with DefaultStringQueryParser with JsonQueryLexer {
-    override val criterionObjects = Map[String, ObjectCriterion]() ++ ditQueryDataImpl.criteriaMap
-  }
+  val queryParser        = CmdbQueryParser.jsonStrictParser(ditQueryDataImpl.criteriaMap.toMap)
 
   val xmlEntityMigration: XmlEntityMigration      = new XmlEntityMigration {
     override def getUpToDateXml(entity: Elem): Box[Elem] = Full(entity)
   }
   val groupRevisionRepo:  GroupRevisionRepository = new GitParseGroupLibrary(
     new NodeGroupCategoryUnserialisationImpl(),
-    new NodeGroupUnserialisationImpl(new CmdbQueryParser {
-      override def parse(query: StringQuery): Box[Query]       = ???
-      override def lex(query:   String):      Box[StringQuery] = ???
-    }),
+    new NodeGroupUnserialisationImpl(CmdbQueryParser.jsonStrictParser(Map.empty)),
     mockGit.gitRepo,
     xmlEntityMigration,
     "groups"

webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/repository/ldap/GlobalParamMigration61Test.scala

@@ -49,10 +49,7 @@ import com.normation.rudder.domain.RudderDit
 import com.normation.rudder.domain.RudderLDAPConstants.*
 import com.normation.rudder.domain.properties.GenericProperty.*
 import com.normation.rudder.domain.properties.GlobalParameter
-import com.normation.rudder.domain.queries.ObjectCriterion
 import com.normation.rudder.services.queries.CmdbQueryParser
-import com.normation.rudder.services.queries.DefaultStringQueryParser
-import com.normation.rudder.services.queries.JsonQueryLexer
 import com.typesafe.config.ConfigValueType
 import com.unboundid.ldap.sdk.DN
 import org.junit.runner.*
@@ -97,9 +94,7 @@ class GlobalParamMigration61Test extends Specification {
 
     val rudderDit       = new RudderDit(new DN("ou=Rudder, cn=rudder-configuration"))
     val nodeDit         = new NodeDit(new DN("ou=Nodes, ou=Rudder, cn=rudder-configuration"))
-    val cmdbQueryParser = new CmdbQueryParser with DefaultStringQueryParser with JsonQueryLexer {
-      val criterionObjects: Map[String, ObjectCriterion] = Map()
-    }
+    val cmdbQueryParser = CmdbQueryParser.jsonRawParser(Map.empty)
 
     new LDAPEntityMapper(rudderDit, nodeDit, acceptedNodesDitImpl, cmdbQueryParser, inventoryMapper)
   }

webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/marshalling/TestXmlUnserialisation.scala

@@ -16,12 +16,9 @@ import com.normation.rudder.domain.policies.SectionVal
 import com.normation.rudder.domain.policies.Tags
 import com.normation.rudder.domain.properties.GroupProperty
 import com.normation.rudder.domain.queries.*
-import com.normation.rudder.domain.queries.ObjectCriterion
 import com.normation.rudder.domain.queries.ResultTransformation.*
 import com.normation.rudder.services.policies.TestNodeConfiguration
 import com.normation.rudder.services.queries.CmdbQueryParser
-import com.normation.rudder.services.queries.DefaultStringQueryParser
-import com.normation.rudder.services.queries.JsonQueryLexer
 import net.liftweb.common.Empty
 import net.liftweb.common.Failure
 import net.liftweb.common.Full
@@ -36,11 +33,8 @@ import scala.xml.Elem
 @RunWith(classOf[JUnitRunner])
 class TestXmlUnserialisation extends Specification with BoxSpecMatcher {
 
-  val queryParser: CmdbQueryParser with DefaultStringQueryParser with JsonQueryLexer = new CmdbQueryParser
-    with DefaultStringQueryParser with JsonQueryLexer {
-    override val criterionObjects = Map[String, ObjectCriterion]()
-  }
-  val directiveUnserialisation = new DirectiveUnserialisationImpl
+  val queryParser                      = CmdbQueryParser.jsonStrictParser(Map.empty)
+  val directiveUnserialisation         = new DirectiveUnserialisationImpl
   val nodeGroupCategoryUnserialisation = new NodeGroupCategoryUnserialisationImpl
   val nodeGroupUnserialisation         = new NodeGroupUnserialisationImpl(queryParser)
   val ruleUnserialisation              = new RuleUnserialisationImpl

webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/queries/TestNodeFactQueryProcessor.scala

@@ -139,10 +139,7 @@ class TestNodeFactQueryProcessor {
 
   val queryProcessor = new NodeFactQueryProcessor(nodeRepository, subGroupComparatorRepo, internalLDAPQueryProcessor)
 
-  val parser: CmdbQueryParser with DefaultStringQueryParser with JsonQueryLexer = new CmdbQueryParser
-    with DefaultStringQueryParser with JsonQueryLexer {
-    override val criterionObjects = queryData.criteriaMap.toMap
-  }
+  val parser = CmdbQueryParser.jsonStrictParser(queryData.criteriaMap.toMap)
 
   case class TestQuery(name: String, query: Query, awaited: Seq[NodeId])
 

webapp/sources/rudder/rudder-rest/src/test/resources/api/api_groups.yml

@@ -1122,6 +1122,78 @@ response:
       }
     }
 ---
+description: Update a node group query with empty query comparator value
+method: POST
+url: /api/latest/groups/00000000-cb9d-4f7b-abda-ca38c5d643ea
+headers:
+  - "Content-Type: application/json"
+body: >-
+  {
+    "id": "00000000-cb9d-4f7b-abda-ca38c5d643ea",
+    "query": {
+      "select": "nodeAndPolicyServer",
+      "composition": "and",
+      "transform": "invert",
+      "where":[
+        {
+          "objectType": "node",
+          "attribute": "nodeId",
+          "comparator": "eq",
+          "value": ""
+        }
+      ]
+    }
+  }
+response:
+  code: 200
+  content: >-
+    {
+      "action":"updateGroup",
+      "id":"00000000-cb9d-4f7b-abda-ca38c5d643ea",
+      "result":"success",
+      "data":{
+        "groups":[
+          {
+            "id":"00000000-cb9d-4f7b-abda-ca38c5d643ea",
+            "displayName":"clone from api of debian group",
+            "description":"Some long description",
+            "category":"GroupRoot",
+            "query": {
+              "select": "nodeAndPolicyServer",
+              "composition": "and",
+              "transform": "invert",
+              "where":[
+                {
+                  "objectType": "node",
+                  "attribute": "nodeId",
+                  "comparator": "eq",
+                  "value": ""
+                }
+              ]
+            },
+            "nodeIds":[],
+            "dynamic":true,
+            "enabled":true,
+            "groupClass":[
+              "group_00000000_cb9d_4f7b_abda_ca38c5d643ea",
+              "group_clone_from_api_of_debian_group"
+            ],
+            "properties":[
+              {
+                "name":"os",
+                "value":{
+                  "name":"debian",
+                  "nickname":"Buster"
+                }
+              }
+            ],
+            "target":"group:00000000-cb9d-4f7b-abda-ca38c5d643ea",
+            "system":false
+          }
+        ]
+      }
+    }
+---
 description: Move a node group to a new category
 method: POST
 url: /api/latest/groups/3fa29229-1a4b-4fd6-9edd-af114289fc9a

webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala

@@ -2472,7 +2472,7 @@ object RudderConfigInit {
     lazy val uuidGen                   = stringUuidGenerator
     lazy val systemVariableSpecService = new SystemVariableSpecServiceImpl()
     lazy val ldapEntityMapper: LDAPEntityMapper =
-      new LDAPEntityMapper(rudderDitImpl, nodeDitImpl, acceptedNodesDitImpl, queryParser, inventoryMapper)
+      new LDAPEntityMapper(rudderDitImpl, nodeDitImpl, acceptedNodesDitImpl, queryRawParser, inventoryMapper)
 
     ///// items serializer - service that transforms items to XML /////
     lazy val ruleSerialisation:                    RuleSerialisation                    = new RuleSerialisationImpl(
@@ -2537,13 +2537,12 @@ object RudderConfigInit {
     lazy val getSubGroupChoices = new DefaultSubGroupComparatorRepository(roLdapNodeGroupRepository)
     lazy val nodeQueryData      = new NodeQueryCriteriaData(() => getSubGroupChoices)
     lazy val ditQueryDataImpl   = new DitQueryData(acceptedNodesDitImpl, nodeDit, rudderDit, nodeQueryData)
-    lazy val queryParser        = new CmdbQueryParser with DefaultStringQueryParser with JsonQueryLexer {
-      override val criterionObjects = Map[String, ObjectCriterion]() ++ ditQueryDataImpl.criteriaMap
-    }
+    lazy val queryParser        = CmdbQueryParser.jsonStrictParser(Map.empty[String, ObjectCriterion] ++ ditQueryDataImpl.criteriaMap)
+    lazy val queryRawParser     = CmdbQueryParser.jsonRawParser(Map.empty[String, ObjectCriterion] ++ ditQueryDataImpl.criteriaMap)
     lazy val inventoryMapper: InventoryMapper =
       new InventoryMapper(inventoryDitService, pendingNodesDitImpl, acceptedNodesDitImpl, removedNodesDitImpl)
 
-    lazy val ldapDiffMapper = new LDAPDiffMapper(ldapEntityMapper, queryParser)
+    lazy val ldapDiffMapper = new LDAPDiffMapper(ldapEntityMapper, queryRawParser)
 
     lazy val activeTechniqueCategoryUnserialisation = new ActiveTechniqueCategoryUnserialisationImpl
     lazy val activeTechniqueUnserialisation         = new ActiveTechniqueUnserialisationImpl

webapp/sources/rudder/rudder-web/src/main/scala/com/normation/rudder/web/components/SearchNodeComponent.scala

@@ -173,7 +173,7 @@ class SearchNodeComponent(
       }
       query = Some(Query(rType, composition, transform, lines.toList))
       initUpdate = false
-      ajaxCriteriaRefresh(isGroupsPage)
+      ajaxCriteriaRefresh(isGroupsPage, preventSave = true)
     }
 
     def removeLine(i: Int): JsCmd = {
@@ -190,7 +190,7 @@ class SearchNodeComponent(
         query = Some(Query(rType, composition, transform, lines.toList))
       }
       initUpdate = false
-      ajaxCriteriaRefresh(isGroupsPage)
+      ajaxCriteriaRefresh(isGroupsPage, preventSave = true)
     }
 
     def processForm(isGroupPage: Boolean): JsCmd = {
@@ -220,16 +220,16 @@ class SearchNodeComponent(
         srvList = Empty
         searchFormHasError = true
       }
-      ajaxCriteriaRefresh(isGroupsPage) & ajaxGridRefresh(isGroupsPage)
+      ajaxCriteriaRefresh(isGroupsPage, preventSave = false) & gridResult(isGroupPage)
       // ajaxGroupCriteriaRefresh & ajaxNodesTableRefresh()
     }
 
     /**
      * Refresh the query parameter part
      */
-    def ajaxCriteriaRefresh(isGroupPage: Boolean):                           JsCmd   = {
+    def ajaxCriteriaRefresh(isGroupPage: Boolean, preventSave: Boolean):     JsCmd   = {
       lines.clear()
-      SetHtml("SearchForm", displayQuery(content, isGroupPage)) & activateButtonOnChange()
+      SetHtml("SearchForm", displayQuery(content, isGroupPage)) & activateButtonOnChange(preventSave)
     }
     def displayQueryLine(cl: CriterionLine, index: Int, addRemove: Boolean): NodeSeq = {
 
@@ -386,7 +386,7 @@ class SearchNodeComponent(
    * @return
    */
   def ajaxGridRefresh(isGroupPage: Boolean): JsCmd = {
-    activateButtonOnChange() &
+    activateButtonOnChange(preventSave = false) &
     gridResult(isGroupPage)
   }
 
@@ -418,7 +418,7 @@ class SearchNodeComponent(
    * When we change the form, we can update the query
    * @return
    */
-  def activateButtonOnChange(): JsCmd = {
+  def activateButtonOnChange(preventSave: Boolean): JsCmd = {
     // If saved button id is not defined do not disable save button
     val disableGridOnChange: JsCmd = if (saveButtonId != "") {
       JE.JsRaw(
@@ -428,7 +428,8 @@ class SearchNodeComponent(
     } else {
       Noop
     }
-    onSearchCallback(searchFormHasError, query) & disableGridOnChange
+    val disableSave = searchFormHasError || preventSave
+    onSearchCallback(disableSave, query) & disableGridOnChange
   }
 
   /**