From 62baf5cfe18d14ffddac4334a904357c99b1261d Mon Sep 17 00:00:00 2001 From: Clark Andrianasolo Date: Thu, 19 Dec 2024 15:37:20 +0100 Subject: [PATCH] Fixes #26075: Trying to save a group with empty criteria removes all entries --- .../repository/ldap/LDAPDiffMapper.scala | 2 +- .../repository/ldap/LDAPEntityMapper.scala | 3 +- .../services/queries/CmdbQueryParser.scala | 43 +++++++++-- .../com/normation/rudder/MockServices.scala | 10 +-- .../ldap/GlobalParamMigration61Test.scala | 7 +- .../marshalling/TestXmlUnserialisation.scala | 10 +-- .../queries/TestNodeFactQueryProcessor.scala | 5 +- .../src/test/resources/api/api_groups.yml | 72 +++++++++++++++++++ .../bootstrap/liftweb/RudderConfig.scala | 9 ++- .../web/components/SearchNodeComponent.scala | 17 ++--- 10 files changed, 130 insertions(+), 48 deletions(-) diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPDiffMapper.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPDiffMapper.scala index cc3bdde43e3..6acbb444bc0 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPDiffMapper.scala +++ b/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 diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala index d5b9dce4fc9..47f191d880d 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/repository/ldap/LDAPEntityMapper.scala +++ b/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) diff --git a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/services/queries/CmdbQueryParser.scala b/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/services/queries/CmdbQueryParser.scala index d812fcdfabf..46ef9df9ca6 100644 --- a/webapp/sources/rudder/rudder-core/src/main/scala/com/normation/rudder/services/queries/CmdbQueryParser.scala +++ b/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,18 +115,23 @@ 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) @@ -116,7 +139,7 @@ trait DefaultStringQueryParser extends StringQueryParser { } } 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 * diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/MockServices.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/MockServices.scala index c6253db11a9..19f071c98f1 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/MockServices.scala +++ b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/MockServices.scala @@ -2986,20 +2986,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" diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/repository/ldap/GlobalParamMigration61Test.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/repository/ldap/GlobalParamMigration61Test.scala index a6e788cdf90..0e60957b333 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/repository/ldap/GlobalParamMigration61Test.scala +++ b/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) } diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/marshalling/TestXmlUnserialisation.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/marshalling/TestXmlUnserialisation.scala index 21b611faa6d..24247d739da 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/marshalling/TestXmlUnserialisation.scala +++ b/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 diff --git a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/queries/TestNodeFactQueryProcessor.scala b/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/queries/TestNodeFactQueryProcessor.scala index 52a0169f786..e05187f5589 100644 --- a/webapp/sources/rudder/rudder-core/src/test/scala/com/normation/rudder/services/queries/TestNodeFactQueryProcessor.scala +++ b/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]) diff --git a/webapp/sources/rudder/rudder-rest/src/test/resources/api/api_groups.yml b/webapp/sources/rudder/rudder-rest/src/test/resources/api/api_groups.yml index 1cb91a4d061..eaca2787d58 100644 --- a/webapp/sources/rudder/rudder-rest/src/test/resources/api/api_groups.yml +++ b/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 diff --git a/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala b/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala index 28a1440520e..70fd2c3708c 100644 --- a/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala +++ b/webapp/sources/rudder/rudder-web/src/main/scala/bootstrap/liftweb/RudderConfig.scala @@ -2459,7 +2459,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( @@ -2524,13 +2524,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 diff --git a/webapp/sources/rudder/rudder-web/src/main/scala/com/normation/rudder/web/components/SearchNodeComponent.scala b/webapp/sources/rudder/rudder-web/src/main/scala/com/normation/rudder/web/components/SearchNodeComponent.scala index b7c3b8798a0..a4d14d89205 100644 --- a/webapp/sources/rudder/rudder-web/src/main/scala/com/normation/rudder/web/components/SearchNodeComponent.scala +++ b/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 } /**