Add protect feature to avoid update or delete actions by mistake

ningyougang · ningyougang · commit 68cb28a87eb6 · 2020-05-12T13:08:08.000+08:00
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.
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
@@ -56,7 +56,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)
@@ -349,6 +350,7 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
   import WhiskActivation.instantSerdes
   val execFieldName = "exec"
   val requireWhiskAuthHeader = "x-require-whisk-auth"
+  val lockFieldName = "lock"
 
   override val collectionName = "actions"
   override val cacheEnabled = true
@@ -643,5 +645,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
@@ -209,12 +209,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)
         }
@@ -552,7 +553,10 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
       limits,
       content.version getOrElse action.version.upPatch,
       content.publish getOrElse action.publish,
-      WhiskActionsApi.amendAnnotations(content.annotations getOrElse action.annotations, exec, create = false))
+      WhiskActionsApi
+        .amendAnnotations(content.annotations getOrElse action.annotations, exec, create = false) ++ content.unlock
+        .map(u => Parameters(WhiskAction.lockFieldName, JsBoolean(!u)))
+        .getOrElse(Parameters()))
       .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
@@ -25,18 +25,22 @@ 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, JsValue, RootJsonFormat}
 import org.apache.openwhisk.common.Logging
 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.{ActivationId, ActivationLogs, DocId, WhiskActivation, WhiskDocument}
+import org.apache.openwhisk.core.entity.{
+  ActivationId,
+  ActivationLogs,
+  DocId,
+  WhiskAction,
+  WhiskActivation,
+  WhiskDocument
+}
 import org.apache.openwhisk.http.ErrorResponse
 import org.apache.openwhisk.http.ErrorResponse.terminate
 import org.apache.openwhisk.http.Messages._
@@ -310,7 +314,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],
@@ -335,8 +340,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) =>
@@ -390,11 +411,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
@@ -658,6 +658,28 @@ 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)
+
+You can lock the existing unlocked action again
+```
+wsk action update greeting --annotation lock true
+```
 
 ## 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
@@ -434,6 +434,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 {
