Skip to content

Commit

Permalink
refine record update auth logic: we will make sure user has permissio…
Browse files Browse the repository at this point in the history 
…n to update based on both `before` & `after` record data
  • Loading branch information
@t83714
t83714 committed Jan 23, 2022
1 parent d4fea98 commit 4ee80c4
Show file tree
Hide file tree
Showing 8 changed files with 375 additions and 171 deletions.
9 changes: 6 additions & 3 deletions magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/AspectPersistence.scala
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package au.csiro.data61.magda.registry

import au.csiro.data61.magda.model.Auth.AuthDecision
import au.csiro.data61.magda.model.Auth.{
AuthDecision,
UnconditionalTrueDecision
}
import scalikejdbc._
import spray.json.JsonParser

Expand Down Expand Up @@ -88,7 +91,7 @@ object AspectPersistence extends Protocols with DiffsonProtocol {
"The provided ID does not match the aspect's ID."
)
)
oldAspect <- this.getById(id, tenantId) match {
oldAspect <- this.getById(id, tenantId, UnconditionalTrueDecision) match {
case Some(aspect) => Success(aspect)
case None => create(newAspect, tenantId, userId)
}
Expand All @@ -110,7 +113,7 @@ object AspectPersistence extends Protocols with DiffsonProtocol {
userId: String
)(implicit session: DBSession): Try[AspectDefinition] = {
for {
aspect <- this.getById(id, tenantId) match {
aspect <- this.getById(id, tenantId, UnconditionalTrueDecision) match {
case Some(aspect) => Success(aspect)
case None =>
Failure(new RuntimeException("No aspect exists with that ID."))
Expand Down
5 changes: 3 additions & 2 deletions magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/AspectValidator.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import com.typesafe.config.Config
import gnieh.diffson.sprayJson._
import au.csiro.data61.magda.model.TenantId._
import au.csiro.data61.magda.model.Auth.UnconditionalTrueDecision
import au.csiro.data61.magda.util.JsonPathUtils.processRecordPatchOperationsOnAspects

class AspectValidator(config: Config, recordPersistence: RecordPersistence) {
def DEFAULT_META_SCHEMA_URI = "https://json-schema.org/draft-07/schema#"
Expand All @@ -23,7 +24,7 @@ class AspectValidator(config: Config, recordPersistence: RecordPersistence) {
implicit session: DBSession
) {
if (shouldValidate()) {
AspectPersistence.getById(aspectId, tenantId) match {
AspectPersistence.getById(aspectId, tenantId, UnconditionalTrueDecision) match {
case Some(aspectDef) => validateWithDefinition(aspectDef, aspectData)
case None =>
throw new Exception(
Expand Down Expand Up @@ -85,7 +86,7 @@ class AspectValidator(config: Config, recordPersistence: RecordPersistence) {
recordId: String,
tenantId: TenantId
)(implicit session: DBSession): Unit = {
recordPersistence.processRecordPatchOperationsOnAspects(
processRecordPatchOperationsOnAspects(
recordPatch,
(aspectId: String, aspectData: JsObject) => {
validate(aspectId, aspectData, tenantId)(session)
Expand Down
109 changes: 88 additions & 21 deletions magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/Directives.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,11 @@ import akka.http.scaladsl.server.Directives._
import akka.http.scaladsl.server.Directive0
import au.csiro.data61.magda.client.AuthApiClient
import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport
import akka.http.scaladsl.model.StatusCodes.InternalServerError
import akka.http.scaladsl.model.StatusCodes.{BadRequest, InternalServerError}
import akka.event.LoggingAdapter
import scalikejdbc.{DBSession, ReadOnlyAutoSession}
import spray.json.{JsNull, JsNumber, JsObject, JsString, JsValue, JsonParser}
import gnieh.diffson.sprayJson.JsonPatch

import scala.util.{Failure, Success, Try}
import scalikejdbc._
Expand All @@ -16,6 +18,8 @@ import scala.collection.mutable.ListBuffer
import scala.concurrent.{ExecutionContext, Future, blocking}
import au.csiro.data61.magda.directives.AuthDirectives.requirePermission
import au.csiro.data61.magda.model.Registry.{AspectDefinition, Record}
import au.csiro.data61.magda.util.JsonPathUtils.applyJsonPathToRecordContextData
import au.csiro.data61.magda.model.Auth.recordToContextData

object Directives extends Protocols with SprayJsonSupport {

Expand All @@ -32,14 +36,20 @@ object Directives extends Protocols with SprayJsonSupport {
*/
private def createRecordContextData(
recordId: String
)(implicit ec: ExecutionContext, session: DBSession): Future[JsObject] = {
)(
implicit ec: ExecutionContext,
logger: LoggingAdapter,
session: DBSession
): Future[JsObject] = {
Future {
blocking {
val recordJsFields: ListBuffer[(String, JsValue)] = ListBuffer()
var recordId: String = ""
sql"""SELECT * FROM records WHERE recordid=${recordId} LIMIT 1"""
.foreach { rs =>
// JDBC treat column name case insensitive
recordJsFields += ("id" -> JsString(rs.string("recordId")))
recordId = rs.string("recordId")
recordJsFields += ("id" -> JsString(recordId))
recordJsFields += ("name" -> JsString(rs.string("recordName")))
recordJsFields += ("lastUpdate" -> JsNumber(
rs.bigInt("lastupdate")
Expand All @@ -59,8 +69,15 @@ object Directives extends Protocols with SprayJsonSupport {
sql"""SELECT aspectid,data FROM recordaspects WHERE recordid=${recordId}"""
.foreach { rs =>
val aspectId = rs.string("aspectid")
Try(JsonParser(rs.string("data")).asJsObject).foreach { data =>
recordJsFields += (aspectId -> data)
Try(JsonParser(rs.string("data")).asJsObject) match {
case Success(data) => recordJsFields += (aspectId -> data)
case Failure(e) =>
logger.warning(
"Failed to parse aspect Json data while prepare policy evaluation context data. recordId: {} aspectId: {}",
recordId,
aspectId
)
throw e
}
}

Expand Down Expand Up @@ -90,7 +107,7 @@ object Directives extends Protocols with SprayJsonSupport {
context.system.dispatchers.lookup("blocking-io-dispatcher")

(withExecutionContext(blockingExeCtx) & onComplete(
createRecordContextData(recordId)(blockingExeCtx, session)
createRecordContextData(recordId)(blockingExeCtx, log, session)
)).tflatMap {
case Tuple1(Success(recordData)) =>
requirePermission(
Expand All @@ -115,17 +132,25 @@ object Directives extends Protocols with SprayJsonSupport {
* a request can only pass this directive when:
* - the record exist and the user has `object/record/update` permission to the record
* - the current record data will be supplied to policy engine as context data
* - the record doesn't exist and the use has `object/record/create` permission to create the record
* - the new record data will be supplied to policy engine as context data
* - the record doesn't exist:
* - the new record data was passed (via `newRecordOrJsonPath`) and the use has `object/record/create` permission to create the record
* - i.e. it's a non-patch request
* - the new record data will be supplied to policy engine as context data for this case
*
* 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 newRecord
* @param recordId
* @param newRecordOrJsonPath
* @param session
* @return
*/
def requireRecordUpdateWithNonExistCreatePermission(
def requireRecordUpdateOrCreateWhenNonExistPermission(
authApiClient: AuthApiClient,
newRecord: Record,
recordId: String,
newRecordOrJsonPath: Either[Record, JsonPatch],
session: DBSession = ReadOnlyAutoSession
): Directive0 = (extractLog & extractExecutionContext).tflatMap { t =>
val akkaExeCtx = t._2
Expand All @@ -134,25 +159,67 @@ object Directives extends Protocols with SprayJsonSupport {
implicit val blockingExeCtx =
context.system.dispatchers.lookup("blocking-io-dispatcher")
onComplete(
createRecordContextData(newRecord.id)(blockingExeCtx, session)
createRecordContextData(recordId)(blockingExeCtx, log, session)
).flatMap {
case Success(recordData) =>
case Success(currentRecordData) =>
val recordContextDataAfterUpdate = newRecordOrJsonPath match {
case Left(newRecord) => recordToContextData(newRecord)
case Right(recordJsonPath) =>
applyJsonPathToRecordContextData(currentRecordData, recordJsonPath).asJsObject
}
/*
We make sure user has permission to perform "object/record/update" operation on
- current record
- the record after update
i.e. A user can't modify a record's data to make it a record that he has no permission to modify.
Useful use case would be: a user with permission to only `update` draft dataset can't modify modify the draft dataset
to make it a publish dataset, unless he also has permission to update publish datasets.
*/
requirePermission(
authApiClient,
"object/record/update",
input = Some(JsObject("object" -> JsObject("record" -> recordData)))
) & withExecutionContext(akkaExeCtx) & pass
case Failure(exception: NoRecordFoundException) =>
requirePermission(
authApiClient,
"object/record/create",
input =
Some(JsObject("object" -> JsObject("record" -> newRecord.toJson)))
Some(JsObject("object" -> JsObject("record" -> currentRecordData)))
) & requirePermission(
authApiClient,
"object/record/update",
input = Some(
JsObject(
"object" -> JsObject(
"record" -> recordContextDataAfterUpdate
)
)
)
) & withExecutionContext(akkaExeCtx) & pass
case Failure(_: NoRecordFoundException) =>
newRecordOrJsonPath match {
case Left(newRecord) =>
// if a new record data was passed (i.e. non json patch request),
// we will assess if user has permission to create the record as well
requirePermission(
authApiClient,
"object/record/create",
input = Some(
JsObject(
"object" -> JsObject(
"record" -> recordToContextData(newRecord)
)
)
)
) & withExecutionContext(akkaExeCtx) & pass
case Right(_) =>
// When record Json Patch was passed (i.e. a patch request),
// there is no way to create the record when it doesn't exist (as we don't know the current data)
// 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: {}",
newRecord.id,
recordId,
e
)
complete(
Expand Down
106 changes: 2 additions & 104 deletions magda-registry-api/src/main/scala/au/csiro/data61/magda/registry/RecordPersistence.scala
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import scala.util.{Failure, Success, Try}
import com.typesafe.config.Config
import scalikejdbc.interpolation.SQLSyntax
import au.csiro.data61.magda.util.SQLUtils
import au.csiro.data61.magda.util.JsonPathUtils.processRecordPatchOperationsOnAspects

trait RecordPersistence {

Expand Down Expand Up @@ -123,13 +124,6 @@ trait RecordPersistence {
forceSkipAspectValidation: Boolean = false
)(implicit session: DBSession): Try[(Record, Long)]

def processRecordPatchOperationsOnAspects[T](
recordPatch: JsonPatch,
onReplaceAspect: (String, JsObject) => T,
onPatchAspect: (String, JsonPatch) => T,
onDeleteAspect: String => T
): Iterable[T]

def patchRecordById(
tenantId: SpecifiedTenantId,
id: String,
Expand Down Expand Up @@ -245,7 +239,7 @@ class DefaultRecordPersistence(config: Config)
/**
* Given a list of recordIds, filter out any record that the current user has not access and return the rest of ids
* @param tenantId
* @param opaRecordQueries
* @param opaRecordQueriesƒ
* @param recordIds
* @param session
* @return Seq[String]
Expand Down Expand Up @@ -674,102 +668,6 @@ class DefaultRecordPersistence(config: Config)
)
}

def processRecordPatchOperationsOnAspects[T](
recordPatch: JsonPatch,
onReplaceAspect: (String, JsObject) => T,
onPatchAspect: (String, JsonPatch) => T,
onDeleteAspect: String => T
): Iterable[T] = {
recordPatch.ops
.groupBy(
op =>
op.path match {
case "aspects" / (name / _) => Some(name)
case _ => None
}
)
.filterKeys(_.isDefined)
.map {
// Create or patch each aspect.
// We create if there's exactly one ADD operation and it's adding an entire aspect.
case (
Some(aspectId),
List(Add("aspects" / (_ / rest), aValue))
) =>
if (rest == Pointer.Empty)
onReplaceAspect(
aspectId,
aValue.asJsObject
)
else
onPatchAspect(
aspectId,
JsonPatch(Add(rest, aValue))
)
// We delete if there's exactly one REMOVE operation and it's removing an entire aspect.
case (
Some(aspectId),
List(Remove("aspects" / (_ / rest), old))
) =>
if (rest == Pointer.Empty) {
onDeleteAspect(aspectId)
} else {
onPatchAspect(
aspectId,
JsonPatch(Remove(rest, old))
)
}
// We patch in all other scenarios.
case (Some(aspectId), operations) =>
onPatchAspect(
aspectId,
JsonPatch(operations.map({
// Make paths in operations relative to the aspect instead of the record
case Add("aspects" / (_ / rest), aValue) =>
Add(rest, aValue)
case Remove("aspects" / (_ / rest), old) =>
Remove(rest, old)
case Replace("aspects" / (_ / rest), aValue, old) =>
Replace(rest, aValue, old)
case Move(
"aspects" / (sourceName / sourceRest),
"aspects" / (destName / destRest)
) =>
if (sourceName != destName)
// We can relax this restriction, and the one on Copy below, by turning a cross-aspect
// Move into a Remove on one and an Add on the other. But it's probably not worth
// the trouble.
throw new RuntimeException(
"A patch may not move values between two different aspects."
)
else
Move(sourceRest, destRest)
case Copy(
"aspects" / (sourceName / sourceRest),
"aspects" / (destName / destRest)
) =>
if (sourceName != destName)
throw new RuntimeException(
"A patch may not copy values between two different aspects."
)
else
Copy(sourceRest, destRest)
case Test("aspects" / (_ / rest), aValue) =>
Test(rest, aValue)
case _ =>
throw new RuntimeException(
"The patch contains an unsupported operation for aspect " + aspectId
)
}))
)

case _ =>
throw new RuntimeException(
"Aspect ID is missing (this shouldn't be possible)."
)
}
}

def patchRecordById(
tenantId: SpecifiedTenantId,
id: String,
Expand Down
Loading

0 comments on commit 4ee80c4

Please sign in to comment.