From 94d2b46387376c78531a45465827e52f5aaaf490 Mon Sep 17 00:00:00 2001 From: Marcin Procyk Date: Wed, 6 Jul 2022 08:17:10 +0200 Subject: [PATCH] fix(value): make impossible to set list root node as a value (DEV-973) (#2088) * add method checking if list is a root node or not * improve ignoring trig files * refactor HierarchicalListValueContentV2 object * cleanup * add list root checkiing method to ValueResponderV2 + test * remove list root checking from ValueMessangerV2 * add new query to handle 2 checks + merge checking methods * move throwing to responder * fix formatting & bad merge * fix failing test * refactor check method to return value + move error handling to responder * add UNIT tests for util method * remove unused template * review changes * cleanup --- .gitignore | 2 +- .../valuemessages/ValueMessagesV2.scala | 6 +- .../webapi/responders/v2/ResourceUtilV2.scala | 65 ++++++++++++++----- .../responders/v2/ResourcesResponderV2.scala | 26 ++++++-- .../responders/v2/ValuesResponderV2.scala | 52 ++++++++++++--- .../responders/v2/ResourceUtilV2Spec.scala | 48 ++++++++++++++ .../responders/v2/ValuesResponderV2Spec.scala | 27 +++++++- 7 files changed, 190 insertions(+), 36 deletions(-) create mode 100644 webapi/src/test/scala/org/knora/webapi/responders/v2/ResourceUtilV2Spec.scala diff --git a/.gitignore b/.gitignore index 009d44d69d..a784514809 100644 --- a/.gitignore +++ b/.gitignore @@ -36,7 +36,7 @@ sipi/test *.bak *.rdb .sbtrc -/*.trig +**/*.trig dependencies.txt /client-test-data.zip diff --git a/webapi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala b/webapi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala index 6f3fb4aca3..6ea5b95b43 100644 --- a/webapi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala +++ b/webapi/src/main/scala/org/knora/webapi/messages/v2/responder/valuemessages/ValueMessagesV2.scala @@ -6,15 +6,16 @@ package org.knora.webapi.messages.v2.responder.valuemessages import akka.actor.ActorRef -import com.typesafe.scalalogging.Logger import akka.http.scaladsl.util.FastFuture import akka.pattern._ import akka.util.Timeout -import org.knora.webapi._ +import com.typesafe.scalalogging.Logger import dsp.errors.AssertionException import dsp.errors.BadRequestException import dsp.errors.NotImplementedException import dsp.errors.SipiException +import dsp.valueobjects.IriErrorMessages +import org.knora.webapi._ import org.knora.webapi.messages.IriConversions._ import org.knora.webapi.messages.OntologyConstants import org.knora.webapi.messages.ResponderRequest.KnoraRequestV2 @@ -41,7 +42,6 @@ import java.time.Instant import java.util.UUID import scala.concurrent.ExecutionContext import scala.concurrent.Future -import dsp.valueobjects.IriErrorMessages /** * A tagging trait for requests handled by [[org.knora.webapi.responders.v2.ValuesResponderV2]]. diff --git a/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala b/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala index dcaa4ba528..f00b7fe4da 100644 --- a/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala +++ b/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourceUtilV2.scala @@ -9,17 +9,23 @@ import akka.actor.ActorRef import akka.pattern._ import akka.util.Timeout import com.typesafe.scalalogging.Logger -import org.knora.webapi.IRI +import dsp.errors.BadRequestException import dsp.errors.ForbiddenException -import dsp.errors.NotFoundException +import org.knora.webapi.IRI +import org.knora.webapi.messages.OntologyConstants import org.knora.webapi.messages.SmartIri +import org.knora.webapi.messages.StringFormatter import org.knora.webapi.messages.admin.responder.permissionsmessages.DefaultObjectAccessPermissionsStringForPropertyGetADM import org.knora.webapi.messages.admin.responder.permissionsmessages.DefaultObjectAccessPermissionsStringResponseADM import org.knora.webapi.messages.admin.responder.usersmessages.UserADM import org.knora.webapi.messages.store.sipimessages.DeleteTemporaryFileRequest import org.knora.webapi.messages.store.sipimessages.MoveTemporaryFileToPermanentStorageRequest +import org.knora.webapi.messages.store.triplestoremessages.LiteralV2 import org.knora.webapi.messages.store.triplestoremessages.SparqlAskRequest import org.knora.webapi.messages.store.triplestoremessages.SparqlAskResponse +import org.knora.webapi.messages.store.triplestoremessages.SparqlExtendedConstructRequest +import org.knora.webapi.messages.store.triplestoremessages.SparqlExtendedConstructResponse +import org.knora.webapi.messages.store.triplestoremessages.SubjectV2 import org.knora.webapi.messages.util.KnoraSystemInstances import org.knora.webapi.messages.util.PermissionUtilADM import org.knora.webapi.messages.util.PermissionUtilADM.EntityPermission @@ -29,6 +35,7 @@ import org.knora.webapi.messages.v2.responder.resourcemessages.ReadResourceV2 import org.knora.webapi.messages.v2.responder.valuemessages.FileValueContentV2 import org.knora.webapi.messages.v2.responder.valuemessages.ReadValueV2 import org.knora.webapi.messages.v2.responder.valuemessages.ValueContentV2 + import scala.concurrent.ExecutionContext import scala.concurrent.Future import scala.util.Failure @@ -133,27 +140,49 @@ object ResourceUtilV2 { } yield defaultObjectAccessPermissionsResponse.permissionLiteral /** - * Checks whether a list node exists, and throws [[NotFoundException]] otherwise. + * Checks whether a list node exists and if is a root node. * - * @param listNodeIri the IRI of the list node. + * @param nodeIri the IRI of the list node. + * @param appActor ActorRef + * @return Future of Either None for nonexistent, true for root and false for child node. */ - def checkListNodeExists(listNodeIri: IRI, appActor: ActorRef)(implicit + def checkListNodeExistsAndIsRootNode(nodeIri: IRI, appActor: ActorRef)(implicit timeout: Timeout, executionContext: ExecutionContext - ): Future[Unit] = + ): Future[Either[Option[Nothing], Boolean]] = { + implicit val stringFormatter: StringFormatter = StringFormatter.getGeneralInstance + for { - askString <- Future( - org.knora.webapi.messages.twirl.queries.sparql.admin.txt - .checkListNodeExistsByIri(listNodeIri = listNodeIri) - .toString - ) - - checkListNodeExistsResponse <- appActor.ask(SparqlAskRequest(askString)).mapTo[SparqlAskResponse] - - _ = if (!checkListNodeExistsResponse.result) { - throw NotFoundException(s"<$listNodeIri> does not exist or is not a ListNode") - } - } yield () + sparqlQuery <- + Future( + org.knora.webapi.messages.twirl.queries.sparql.admin.txt + .getListNode(nodeIri = nodeIri) + .toString() + ) + + listNodeResponse <- + appActor + .ask( + SparqlExtendedConstructRequest( + sparql = sparqlQuery + ) + ) + .mapTo[SparqlExtendedConstructResponse] + + statements: Map[SubjectV2, Map[SmartIri, Seq[LiteralV2]]] = listNodeResponse.statements + + maybeList = + if (statements.nonEmpty) { + val propToCheck: SmartIri = stringFormatter.toSmartIri(OntologyConstants.KnoraBase.IsRootNode) + val isRootNode: Boolean = statements.map(_._2.contains(propToCheck)).head + + Right(isRootNode) + } else { + Left(None) + } + + } yield maybeList + } /** * Given a future representing an operation that was supposed to update a value in a triplestore, checks whether diff --git a/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala b/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala index eec88760e4..a0a91955be 100644 --- a/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala +++ b/webapi/src/main/scala/org/knora/webapi/responders/v2/ResourcesResponderV2.scala @@ -8,9 +8,8 @@ package org.knora.webapi.responders.v2 import akka.http.scaladsl.util.FastFuture import akka.pattern._ import akka.stream.Materializer -import org.knora.webapi._ import dsp.errors._ - +import org.knora.webapi._ import org.knora.webapi.messages.IriConversions._ import org.knora.webapi.messages.OntologyConstants import org.knora.webapi.messages.SmartIri @@ -1161,9 +1160,26 @@ class ResourcesResponderV2(responderData: ResponderData) extends ResponderWithSt Future .sequence( - listNodesThatShouldExist - .map(listNodeIri => ResourceUtilV2.checkListNodeExists(listNodeIri, appActor)) - .toSeq + listNodesThatShouldExist.map { listNodeIri => + for { + checkNode <- ResourceUtilV2.checkListNodeExistsAndIsRootNode(listNodeIri, appActor) + + _ = checkNode match { + // it doesn't have isRootNode property - it's a child node + case Right(false) => () + // it does have isRootNode property - it's a root node + case Right(true) => + throw BadRequestException( + s"<${listNodeIri}> is a root node. Root nodes cannot be set as values." + ) + // it deosn't exists or isn't valid list + case Left(_) => + throw NotFoundException( + s"<${listNodeIri}> does not exist, or is not a ListNode." + ) + } + } yield () + }.toSeq ) .map(_ => ()) } diff --git a/webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala b/webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala index 26b1cf720e..83a56f1e51 100644 --- a/webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala +++ b/webapi/src/main/scala/org/knora/webapi/responders/v2/ValuesResponderV2.scala @@ -7,9 +7,8 @@ package org.knora.webapi.responders.v2 import akka.http.scaladsl.util.FastFuture import akka.pattern._ -import org.knora.webapi._ import dsp.errors._ - +import org.knora.webapi._ import org.knora.webapi.messages.IriConversions._ import org.knora.webapi.messages.OntologyConstants import org.knora.webapi.messages.SmartIri @@ -190,11 +189,29 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde requestingUser = createValueRequest.requestingUser ) - // If this is a list value, check that it points to a real list node. - + // If it is a list value, check that it points to a real list node which is not a root node. _ <- submittedInternalValueContent match { case listValue: HierarchicalListValueContentV2 => - ResourceUtilV2.checkListNodeExists(listValue.valueHasListNode, appActor) + for { + checkNode <- + ResourceUtilV2.checkListNodeExistsAndIsRootNode(listValue.valueHasListNode, appActor) + + _ = checkNode match { + // it doesn't have isRootNode property - it's a child node + case Right(false) => () + // it does have isRootNode property - it's a root node + case Right(true) => + throw BadRequestException( + s"<${listValue.valueHasListNode}> is a root node. Root nodes cannot be set as values." + ) + // it deosn't exists or isn't valid list + case Left(_) => + throw NotFoundException( + s"<${listValue.valueHasListNode}> does not exist or is not a ListNode." + ) + } + } yield () + case _ => FastFuture.successful(()) } @@ -325,6 +342,7 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde unverifiedValue = unverifiedValue, requestingUser = createValueRequest.requestingUser ) + } yield CreateValueResponseV2( valueIri = verifiedValue.newValueIri, valueType = verifiedValue.value.valueType, @@ -1171,11 +1189,29 @@ class ValuesResponderV2(responderData: ResponderData) extends Responder(responde requestingUser = updateValueRequest.requestingUser ) - // If this is a list value, check that it points to a real list node. - + // If it is a list value, check that it points to a real list node which is not a root node. _ <- submittedInternalValueContent match { case listValue: HierarchicalListValueContentV2 => - ResourceUtilV2.checkListNodeExists(listValue.valueHasListNode, appActor) + for { + checkNode <- + ResourceUtilV2.checkListNodeExistsAndIsRootNode(listValue.valueHasListNode, appActor) + + _ = checkNode match { + // it doesn't have isRootNode property - it's a child node + case Right(false) => () + // it does have isRootNode property - it's a root node + case Right(true) => + throw BadRequestException( + s"<${listValue.valueHasListNode}> is a root node. Root nodes cannot be set as values." + ) + // it deosn't exists or isn't valid list + case Left(_) => + throw NotFoundException( + s"<${listValue.valueHasListNode}> does not exist or is not a ListNode." + ) + } + } yield () + case _ => FastFuture.successful(()) } diff --git a/webapi/src/test/scala/org/knora/webapi/responders/v2/ResourceUtilV2Spec.scala b/webapi/src/test/scala/org/knora/webapi/responders/v2/ResourceUtilV2Spec.scala new file mode 100644 index 0000000000..eb31be3c17 --- /dev/null +++ b/webapi/src/test/scala/org/knora/webapi/responders/v2/ResourceUtilV2Spec.scala @@ -0,0 +1,48 @@ +/* + * Copyright © 2021 - 2022 Swiss National Data and Service Center for the Humanities and/or DaSCH Service Platform contributors. + * SPDX-License-Identifier: Apache-2.0 + */ + +package org.knora.webapi.responders.v2 + +import akka.util.Timeout +import org.knora.webapi.CoreSpec +import org.knora.webapi.messages.store.triplestoremessages.RdfDataObject + +import scala.concurrent.ExecutionContextExecutor + +object ResourceUtilV2Spec {} + +class ResourceUtilV2Spec extends CoreSpec() { + implicit val timeout: Timeout = settings.defaultTimeout + implicit val ec: ExecutionContextExecutor = system.dispatcher + + override lazy val rdfDataObjects = List( + RdfDataObject(path = "test_data/all_data/anything-data.ttl", name = "http://www.knora.org/data/0001/anything") + ) + + "ResourceUtil" when { + "called `checkListNodeExistsAndIsRootNode` method" should { + "return FALSE for list child node" in { + val nodeIri = "http://rdfh.ch/lists/0001/otherTreeList01" + val checkNode = ResourceUtilV2.checkListNodeExistsAndIsRootNode(nodeIri, appActor) + + checkNode.onComplete(_.get should be(Right(false))) + } + + "return TRUE for list root node" in { + val nodeIri = "http://rdfh.ch/lists/0001/otherTreeList" + val checkNode = ResourceUtilV2.checkListNodeExistsAndIsRootNode(nodeIri, appActor) + + checkNode.onComplete(_.get should be(Right(true))) + } + + "should return NONE for nonexistent list" in { + val nodeIri = "http://rdfh.ch/lists/0001/otherTreeList77" + val checkNode = ResourceUtilV2.checkListNodeExistsAndIsRootNode(nodeIri, appActor) + + checkNode.onComplete(_.get should be(Left(None))) + } + } + } +} diff --git a/webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala b/webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala index f17b01c5be..d68bfa3f3b 100644 --- a/webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala +++ b/webapi/src/test/scala/org/knora/webapi/responders/v2/ValuesResponderV2Spec.scala @@ -6,9 +6,9 @@ package org.knora.webapi.responders.v2 import akka.testkit.ImplicitSender +import dsp.errors._ import org.knora.webapi._ import org.knora.webapi.config.AppConfig -import dsp.errors._ import org.knora.webapi.messages.IriConversions._ import org.knora.webapi.messages.OntologyConstants import org.knora.webapi.messages.SmartIri @@ -1754,6 +1754,31 @@ class ValuesResponderV2Spec extends CoreSpec() with ImplicitSender { } } + "not create a list value that is a root list node" in { + val resourceIri = "http://rdfh.ch/0001/a-blue-thing" + val resourceClassIri = "http://www.knora.org/ontology/0001/anything#BlueThing".toSmartIri + val propertyIri: SmartIri = "http://0.0.0.0:3333/ontology/0001/anything/v2#hasListItem".toSmartIri + val valueHasListNode = "http://rdfh.ch/lists/0001/otherTreeList" + + appActor ! CreateValueRequestV2( + CreateValueV2( + resourceIri = resourceIri, + resourceClassIri = resourceClassIri, + propertyIri = propertyIri, + valueContent = HierarchicalListValueContentV2( + ontologySchema = ApiV2Complex, + valueHasListNode = valueHasListNode + ) + ), + requestingUser = anythingUser1, + apiRequestID = UUID.randomUUID + ) + + expectMsgPF(timeout) { case msg: akka.actor.Status.Failure => + assert(msg.cause.isInstanceOf[BadRequestException]) + } + } + "create a color value" in { // Add the value.