From 63a2aaf30f1dd2e602b4580c9ea47a78bb48e4bc Mon Sep 17 00:00:00 2001 From: "ning.yougang" Date: Fri, 28 Sep 2018 17:44:29 +0800 Subject: [PATCH] Add protect feature to avoid update or delete actions by mistake As more and more production system uses openwhisk, Users will need some features to protect their service safe from mistake. If we have this feature, user can protect their actions from updating or deletion by mistake. --- .../openwhisk/core/entity/WhiskAction.scala | 6 +- .../openwhisk/core/controller/Actions.scala | 39 +++++++---- .../openwhisk/core/controller/ApiUtils.scala | 56 ++++++++++++---- docs/actions.md | 17 +++++ .../controller/test/ActionsApiTests.scala | 64 +++++++++++++++++++ 5 files changed, 158 insertions(+), 24 deletions(-) diff --git a/common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala b/common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala index 38d1a2fd361..ed7b478817c 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala +++ b/common/scala/src/main/scala/org/apache/openwhisk/core/entity/WhiskAction.scala @@ -55,7 +55,8 @@ case class WhiskActionPut(exec: Option[Exec] = None, limits: Option[ActionLimitsOption] = None, version: Option[SemVer] = None, publish: Option[Boolean] = None, - annotations: Option[Parameters] = None) { + annotations: Option[Parameters] = None, + unlock: Option[Boolean] = None) { protected[core] def replace(exec: Exec) = { WhiskActionPut(Some(exec), parameters, limits, version, publish, annotations) @@ -308,6 +309,7 @@ case class ExecutableWhiskActionMetaData(namespace: EntityPath, object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[WhiskAction] with DefaultJsonProtocol { val execFieldName = "exec" + val lockFieldName = "lock" val finalParamsAnnotationName = "final" override val collectionName = "actions" @@ -591,5 +593,5 @@ object ActionLimitsOption extends DefaultJsonProtocol { } object WhiskActionPut extends DefaultJsonProtocol { - implicit val serdes = jsonFormat6(WhiskActionPut.apply) + implicit val serdes = jsonFormat7(WhiskActionPut.apply) } diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala index 3587a7e1b72..371d3642cba 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala +++ b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala @@ -190,12 +190,13 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with val checkAdditionalPrivileges = entitleReferencedEntities(user, Privilege.READ, request.exec).flatMap { case _ => entitlementProvider.check(user, content.exec) } + val unlock = content.unlock.getOrElse(false) onComplete(checkAdditionalPrivileges) { case Success(_) => putEntity(WhiskAction, entityStore, entityName.toDocId, overwrite, update(user, request) _, () => { make(user, entityName, request) - }) + }, unlock = unlock) case Failure(f) => super.handleEntitlementFailure(f) } @@ -523,16 +524,32 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with val exec = content.exec getOrElse action.exec - WhiskAction( - action.namespace, - action.name, - exec, - parameters, - limits, - content.version getOrElse action.version.upPatch, - content.publish getOrElse action.publish, - (content.annotations getOrElse action.annotations) ++ execAnnotation(exec)) - .revision[WhiskAction](action.docinfo.rev) + if (content.unlock.getOrElse(false)) { + WhiskAction( + action.namespace, + action.name, + exec, + parameters, + limits, + content.version getOrElse action.version.upPatch, + content.publish getOrElse action.publish, + (content.annotations getOrElse action.annotations) ++ execAnnotation(exec) ++ Parameters( + WhiskAction.lockFieldName, + JsBoolean(false))) + .revision[WhiskAction](action.docinfo.rev) + } else { + //keep original value not changed + WhiskAction( + action.namespace, + action.name, + exec, + parameters, + limits, + content.version getOrElse action.version.upPatch, + content.publish getOrElse action.publish, + (content.annotations getOrElse action.annotations) ++ execAnnotation(exec)) + .revision[WhiskAction](action.docinfo.rev) + } } /** diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/ApiUtils.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/ApiUtils.scala index 71c64148a1a..e8483f509ef 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/ApiUtils.scala +++ b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/ApiUtils.scala @@ -23,10 +23,7 @@ import scala.util.Failure import scala.util.Success import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ import akka.http.scaladsl.model.StatusCode -import akka.http.scaladsl.model.StatusCodes.Conflict -import akka.http.scaladsl.model.StatusCodes.InternalServerError -import akka.http.scaladsl.model.StatusCodes.NotFound -import akka.http.scaladsl.model.StatusCodes.OK +import akka.http.scaladsl.model.StatusCodes._ import akka.http.scaladsl.server.{Directives, RequestContext, RouteResult} import spray.json.DefaultJsonProtocol._ import spray.json.JsObject @@ -37,6 +34,7 @@ import org.apache.openwhisk.common.TransactionId import org.apache.openwhisk.core.controller.PostProcess.PostProcessEntity import org.apache.openwhisk.core.database._ import org.apache.openwhisk.core.entity.DocId +import org.apache.openwhisk.core.entity.WhiskAction import org.apache.openwhisk.core.entity.WhiskDocument import org.apache.openwhisk.http.ErrorResponse import org.apache.openwhisk.http.ErrorResponse.terminate @@ -242,7 +240,8 @@ trait WriteOps extends Directives { update: A => Future[A], create: () => Future[A], treatExistsAsConflict: Boolean = true, - postProcess: Option[PostProcessEntity[A]] = None)( + postProcess: Option[PostProcessEntity[A]] = None, + unlock: Boolean = false)( implicit transid: TransactionId, format: RootJsonFormat[A], notifier: Option[CacheChangeNotification], @@ -267,8 +266,24 @@ trait WriteOps extends Directives { } flatMap { case (old, a) => logging.debug(this, s"[PUT] entity created/updated, writing back to datastore") - factory.put(datastore, a, old) map { _ => - a + if (overwrite && !unlock && old.getOrElse(None).isInstanceOf[WhiskAction]) { + val oldWhiskAction = old.getOrElse(None).asInstanceOf[WhiskAction] + oldWhiskAction.annotations.get(WhiskAction.lockFieldName) match { + case Some(value) if (value.convertTo[Boolean]) => { + Future failed RejectRequest( + MethodNotAllowed, + s"this action can't be updated until ${WhiskAction.lockFieldName} annotation is updated to false") + } + case _ => { + factory.put(datastore, a, old) map { _ => + a + } + } + } + } else { + factory.put(datastore, a, old) map { _ => + a + } } }) { case Success(entity) => @@ -322,11 +337,30 @@ trait WriteOps extends Directives { notifier: Option[CacheChangeNotification], ma: Manifest[A]) = { onComplete(factory.get(datastore, docid) flatMap { entity => - confirm(entity) flatMap { - case _ => - factory.del(datastore, entity.docinfo) map { _ => - entity + if (entity.isInstanceOf[WhiskAction]) { + val whiskAction = entity.asInstanceOf[WhiskAction] + whiskAction.annotations.get(WhiskAction.lockFieldName) match { + case Some(value) if (value.convertTo[Boolean]) => { + Future failed RejectRequest( + MethodNotAllowed, + s"this action can't be deleted until ${WhiskAction.lockFieldName} annotation is updated to false") + } + case _ => { + confirm(entity) flatMap { + case _ => + factory.del(datastore, entity.docinfo) map { _ => + entity + } + } } + } + } else { + confirm(entity) flatMap { + case _ => + factory.del(datastore, entity.docinfo) map { _ => + entity + } + } } }) { case Success(entity) => diff --git a/docs/actions.md b/docs/actions.md index f03e708a35f..ddb501c1de9 100644 --- a/docs/actions.md +++ b/docs/actions.md @@ -598,6 +598,23 @@ You can clean up by deleting actions that you do not want to use. actions /guest/mySequence private sequence ``` +## Protect action updated or deleted by mistake + +If your action is very important, you can add `--annotation lock true` to protect it +``` +wsk action create greeting greeting.js --annotation lock true +``` +Then update or delete this action will be not allowed + +You can add `{"unlock":true}` to enable `update or delete operation`, for example: +``` +curl -X PUT -H "Content-type: application/json" +--user 23bc46b1-71f6-4ed5-8c54-816aa4f8c502:123zO3xZCLrMN6v2BKK1dXYFpXlPkccOFqm12CdAsMgRU4VrNZ9lyGVCGuMDGIwP +-d '{"unlock":true}' +'/api/v1/namespaces/guest/actions/hello?overwrite=true' +``` + +TODO(add unlock operation to wsk, for example: wsk action update hello --unlock) ## Accessing action metadata within the action body diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiTests.scala index ce8a1bf73c3..fd2ecaa249b 100644 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiTests.scala +++ b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ActionsApiTests.scala @@ -401,6 +401,70 @@ class ActionsApiTests extends ControllerTestCommon with WhiskActionsApi { } } + it should "update action not allowed when lock annotation is true" in { + implicit val tid = transid() + val action = + WhiskAction( + namespace, + aname(), + jsDefault(""), + annotations = Parameters(WhiskAction.lockFieldName, JsBoolean(true))) + val content = JsObject( + "exec" -> JsObject("code" -> "".toJson, "kind" -> action.exec.kind.toJson), + "annotations" -> action.annotations.toJson) + Put(s"$collectionPath/${action.name}", content) ~> Route.seal(routes(creds)) ~> check { + status should be(OK) + } + + //Update not allowed + Put(s"$collectionPath/${action.name}?overwrite=true", content) ~> Route.seal(routes(creds)) ~> check { + status should be(MethodNotAllowed) + } + + //unlock the action + val unlockContent = s"""{"unlock":true}""".stripMargin.parseJson.asJsObject + Put(s"$collectionPath/${action.name}?overwrite=true", unlockContent) ~> Route.seal(routes(creds)) ~> check { + status should be(OK) + } + + //Update allowed after unlock the action + Put(s"$collectionPath/${action.name}?overwrite=true", content) ~> Route.seal(routes(creds)) ~> check { + status should be(OK) + } + } + + it should "delete action not allowed when lock annotation is true" in { + implicit val tid = transid() + val action = + WhiskAction( + namespace, + aname(), + jsDefault(""), + annotations = Parameters(WhiskAction.lockFieldName, JsBoolean(true))) + val content = JsObject( + "exec" -> JsObject("code" -> "".toJson, "kind" -> action.exec.kind.toJson), + "annotations" -> action.annotations.toJson) + Put(s"$collectionPath/${action.name}", content) ~> Route.seal(routes(creds)) ~> check { + status should be(OK) + } + + //Delete not allowed + Delete(s"$collectionPath/${action.name}") ~> Route.seal(routes(creds)) ~> check { + status should be(MethodNotAllowed) + } + + //unlock the action + val unlockContent = s"""{"unlock":true}""".stripMargin.parseJson.asJsObject + Put(s"$collectionPath/${action.name}?overwrite=true", unlockContent) ~> Route.seal(routes(creds)) ~> check { + status should be(OK) + } + + //Delete allowed after unlock the action + Delete(s"$collectionPath/${action.name}") ~> Route.seal(routes(creds)) ~> check { + status should be(OK) + } + } + it should "report NotFound for delete non existent action" in { implicit val tid = transid() Delete(s"$collectionPath/xyz") ~> Route.seal(routes(creds)) ~> check {