refine aspect update & delete auth logic:

user should has update permission to the record before and after the changes
magdabot · Jan 23, 2022 · 58067a8 · 58067a8
1 parent 1ca117d
commit 58067a8
Show file tree

Hide file tree

Showing 3 changed files with 190 additions and 45 deletions.
diff --git a/magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/Directives.scala b/magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/Directives.scala
@@ -140,7 +140,6 @@ object Directives extends Protocols with SprayJsonSupport {
     * When the record doesn't exist and the record JSON patch was passed (via `newRecordOrJsonPath`), we will response BadRequest response immediately.
     * - as there is no way to process JSON patch without original record data.
     *
-    *
     * @param authApiClient
     * @param recordId
     * @param newRecordOrJsonPath
@@ -229,6 +228,153 @@ object Directives extends Protocols with SprayJsonSupport {
     }
   }
 
+  /**
+    * a request can only pass this directive when the user has permission to perform `update` operation on
+    * both current record and record after the aspect changes
+    *
+    * @param authApiClient
+    * @param recordId
+    * @param aspectId
+    * @param newAspectOrAspectJsonPath
+    * @param session
+    * @return
+    */
+  def requireRecordAspectUpdatePermission(
+      authApiClient: AuthApiClient,
+      recordId: String,
+      aspectId: String,
+      newAspectOrAspectJsonPath: Either[JsObject, JsonPatch],
+      session: DBSession = ReadOnlyAutoSession
+  ): Directive0 = (extractLog & extractExecutionContext).tflatMap { t =>
+    val akkaExeCtx = t._2
+    val log = t._1
+
+    implicit val blockingExeCtx =
+      context.system.dispatchers.lookup("blocking-io-dispatcher")
+
+    onComplete(
+      createRecordContextData(recordId)(blockingExeCtx, log, session)
+    ).flatMap {
+      case Success(currentRecordData) =>
+        val recordContextDataAfterUpdate = newAspectOrAspectJsonPath match {
+          case Left(newAspect) =>
+            JsObject(currentRecordData.fields + (aspectId -> newAspect))
+          case Right(recordJsonPath) =>
+            val newAspectData = recordJsonPath(
+              currentRecordData.fields
+                .get(aspectId)
+                .getOrElse(JsObject())
+            )
+            JsObject(
+              currentRecordData.fields + (aspectId -> newAspectData)
+            )
+        }
+        /*
+          We make sure user has permission to perform "object/record/update" operation on
+          - current record
+          - the record after the aspect data update
+          i.e. A user can't modify a record's data to make it a record that he has no permission to modify.
+         */
+        requirePermission(
+          authApiClient,
+          "object/record/update",
+          input =
+            Some(JsObject("object" -> JsObject("record" -> currentRecordData)))
+        ) & requirePermission(
+          authApiClient,
+          "object/record/update",
+          input = Some(
+            JsObject(
+              "object" -> JsObject(
+                "record" -> recordContextDataAfterUpdate
+              )
+            )
+          )
+        ) & withExecutionContext(akkaExeCtx) & pass
+      case Failure(_: NoRecordFoundException) =>
+        // There is no way to construct full record context data for auth decision without the original record
+        // we will send out BadRequest response immediately
+        complete(
+          BadRequest,
+          s"Cannot locate request record by id: ${recordId}"
+        )
+      case Failure(e: Throwable) =>
+        log.error(
+          "Failed to create record context data for auth decision. record ID: {}. Error: {}",
+          recordId,
+          e
+        )
+        complete(
+          InternalServerError,
+          s"An error occurred while creating record context data for auth decision."
+        )
+    }
+
+  }
+
+  /**
+    * a request can only pass this directive when the user has permission to perform `update` operation on
+    * both current record and record after the aspect is deleted
+    *
+    * @param authApiClient
+    * @param recordId
+    * @param aspectId
+    * @param session
+    * @return
+    */
+  def requireDeleteRecordAspectPermission(
+      authApiClient: AuthApiClient,
+      recordId: String,
+      aspectId: String,
+      session: DBSession = ReadOnlyAutoSession
+  ): Directive0 = (extractLog & extractExecutionContext).tflatMap { t =>
+    val akkaExeCtx = t._2
+    val log = t._1
+
+    implicit val blockingExeCtx =
+      context.system.dispatchers.lookup("blocking-io-dispatcher")
+
+    onComplete(
+      createRecordContextData(recordId)(blockingExeCtx, log, session)
+    ).flatMap {
+      case Success(currentRecordData) =>
+        val recordContextDataAfterUpdate =
+          JsObject(currentRecordData.fields - aspectId)
+        /*
+          Make sure user has permission before & after delete the aspect
+         */
+        requirePermission(
+          authApiClient,
+          "object/record/update",
+          input =
+            Some(JsObject("object" -> JsObject("record" -> currentRecordData)))
+        ) & requirePermission(
+          authApiClient,
+          "object/record/update",
+          input = Some(
+            JsObject(
+              "object" -> JsObject(
+                "record" -> recordContextDataAfterUpdate
+              )
+            )
+          )
+        ) & withExecutionContext(akkaExeCtx) & pass
+      case Failure(_: NoRecordFoundException) =>
+        // when record cannot found, aspect is already gone. We let it go.
+        withExecutionContext(akkaExeCtx) & pass
+      case Failure(e: Throwable) =>
+        log.error(
+          "Failed to create record context data for auth decision. record ID: {}. Error: {}",
+          recordId,
+          e
+        )
+        complete(
+          InternalServerError,
+          s"An error occurred while creating record context data for auth decision."
+        )
+    }
+  }
+
   /**
     * retrieve aspect record from DB and convert into JSON data (to be used as part of policy engine context data)
     * @param aspectId

diff --git a/magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/RecordAspectsService.scala b/magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/RecordAspectsService.scala
@@ -6,14 +6,15 @@ import akka.http.scaladsl.model.headers.RawHeader
 import akka.http.scaladsl.server.Directives._
 import akka.http.scaladsl.server.Route
 import akka.stream.Materializer
-import au.csiro.data61.magda.client.AuthApiClient
 import au.csiro.data61.magda.directives.AuthDirectives.{requireUserId}
 import au.csiro.data61.magda.directives.TenantDirectives.{
-  requiresSpecifiedTenantId,
-  requiresTenantId
+  requiresSpecifiedTenantId
 }
 import au.csiro.data61.magda.model.Registry._
-import au.csiro.data61.magda.registry.Directives.requireRecordPermission
+import au.csiro.data61.magda.registry.Directives.{
+  requireRecordAspectUpdatePermission,
+  requireDeleteRecordAspectPermission
+}
 import com.typesafe.config.Config
 import gnieh.diffson.sprayJson._
 import io.swagger.annotations._
@@ -133,9 +134,14 @@ class RecordAspectsService(
     path(Segment / "aspects" / Segment) {
       (recordId: String, aspectId: String) =>
         requireUserId { userId =>
-          requireRecordPermission(authClient, "object/record/update", recordId) {
-            requiresSpecifiedTenantId { tenantId =>
-              entity(as[JsObject]) { aspect =>
+          requiresSpecifiedTenantId { tenantId =>
+            entity(as[JsObject]) { aspect =>
+              requireRecordAspectUpdatePermission(
+                authClient,
+                recordId,
+                aspectId,
+                Left(aspect)
+              ) {
                 val theResult = DB localTx { session =>
                   recordPersistence.putRecordAspectById(
                     tenantId,
@@ -236,7 +242,7 @@ class RecordAspectsService(
         requireUserId { userId =>
           // delete a record aspect should be considered as update operation for the record
           // we will not implement aspect level auth for now
-          requireRecordPermission(authClient, "object/record/update", recordId) {
+          requireDeleteRecordAspectPermission(authClient, recordId, aspectId) {
             requiresSpecifiedTenantId { tenantId =>
               val theResult = DB localTx { session =>
                 recordPersistence.deleteRecordAspect(
@@ -351,9 +357,14 @@ class RecordAspectsService(
     path(Segment / "aspects" / Segment) {
       (recordId: String, aspectId: String) =>
         requireUserId { userId =>
-          requireRecordPermission(authClient, "object/record/update", recordId) {
-            requiresSpecifiedTenantId { tenantId =>
-              entity(as[JsonPatch]) { aspectPatch =>
+          requiresSpecifiedTenantId { tenantId =>
+            entity(as[JsonPatch]) { aspectPatch =>
+              requireRecordAspectUpdatePermission(
+                authClient,
+                recordId,
+                aspectId,
+                Right(aspectPatch)
+              ) {
                 val theResult = DB localTx { session =>
                   recordPersistence.patchRecordAspectById(
                     tenantId,

diff --git a/magda-scala-common/src/main/scala/au/csiro/data61/magda/util/JsonPathUtils.scala b/magda-scala-common/src/main/scala/au/csiro/data61/magda/util/JsonPathUtils.scala
@@ -126,30 +126,25 @@ object JsonPathUtils {
       recordPatch,
       (aspectId, replaceObj) => {
         //onReplaceAspect
-        patchedRecord = patchedRecord.copy(aspects = patchedRecord.aspects.map {
-          item =>
-            if (item._1 == aspectId) {
-              (aspectId, replaceObj)
-            } else {
-              item
-            }
-        })
+        patchedRecord = patchedRecord.copy(
+          aspects = patchedRecord.aspects + (aspectId -> replaceObj)
+        )
       },
       (aspectId, jsonPatch) => {
         //onPatchAspect
-        patchedRecord = patchedRecord.copy(aspects = patchedRecord.aspects.map {
-          item =>
-            if (item._1 == aspectId) {
-              (aspectId, jsonPatch(item._2).asJsObject)
-            } else {
-              item
-            }
-        })
+        val aspectData =
+          patchedRecord.aspects.get(aspectId).getOrElse(JsObject())
+        patchedRecord = patchedRecord
+          .copy(
+            aspects = patchedRecord.aspects + (aspectId -> jsonPatch(
+              aspectData
+            ).asJsObject)
+          )
       },
       aspectId => {
         // onDeleteAspect
         patchedRecord = patchedRecord.copy(
-          aspects = patchedRecord.aspects.filter(_._1 != aspectId)
+          aspects = patchedRecord.aspects - aspectId
         )
       }
     )
@@ -182,28 +177,21 @@ object JsonPathUtils {
       recordPatch,
       (aspectId, replaceObj) => {
         //onReplaceAspect
-        patchedRecord = new JsObject(patchedRecord.fields.map { item =>
-          if (item._1 == aspectId) {
-            (aspectId, replaceObj)
-          } else {
-            item
-          }
-        })
+        patchedRecord =
+          JsObject(patchedRecord.fields + (aspectId -> replaceObj))
       },
       (aspectId, jsonPatch) => {
         //onPatchAspect
-        patchedRecord = new JsObject(patchedRecord.fields.map { item =>
-          if (item._1 == aspectId) {
-            (aspectId, jsonPatch(item._2).asJsObject)
-          } else {
-            item
-          }
-        })
+        val aspectData =
+          patchedRecord.fields.get(aspectId).getOrElse(JsObject())
+        patchedRecord = JsObject(
+          patchedRecord.fields + (aspectId -> jsonPatch(aspectData).asJsObject)
+        )
       },
       aspectId => {
         // onDeleteAspect
-        patchedRecord = new JsObject(
-          patchedRecord.fields.filter(_._1 != aspectId)
+        patchedRecord = JsObject(
+          patchedRecord.fields - aspectId
         )
       }
     )