Add protect feature to avoid update or delete actions by mistake

ningyougang · ningyougang · commit db02599871c7 · 2020-09-07T10:30:37.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.
- Apply unix style permission management
- Make the action's downloadable for the shared user
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
@@ -76,6 +76,16 @@ case class WhiskActionPut(exec: Option[Exec] = None,
       case _ => this
     } getOrElse this
   }
+
+  protected[core] def getPermissions(): Option[String] = {
+    annotations match {
+      case Some(value) =>
+        value
+          .get(WhiskAction.permissionsFieldName)
+          .map(value => value.convertTo[String])
+      case None => None
+    }
+  }
 }
 
 abstract class WhiskActionLike(override val name: EntityName) extends WhiskEntity(name, "action") {
@@ -351,6 +361,49 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[
   val execFieldName = "exec"
   val requireWhiskAuthHeader = "x-require-whisk-auth"
 
+  // annotation permission key name
+  val permissionsFieldName = "permissions"
+
+  val defaultPermissions = "rwxr-x"
+
+  // notes on users, just have 2 type users,
+  // 1. the action's owner
+  // 2. the user (not the owner) who used the shared action directly(e.g. get, invoke), we call it "the shared user"
+  //
+  // Notes on permission control
+  // 1. the owner has read(or download) permission on any situation, but for the shared user,
+  //    in spite of has read permission on any situation, but can set it undownloadable or downloadable
+  // 2. the shared user can't update/delete the action on any situation.
+  // 3. the owner's permission can affect the shared user's permission, e.g
+  //    if the owner is not given execute permission, the shared user can't have execute permission as well.
+  //
+  // Notes on permission values, include below permission value
+  //  1. permission code:rwxr-x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes), this is default
+  //  2. permission code:rwxr--: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+  //  3. permission code:r-xr-x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(yes)
+  //  4. permission code:r-xr--: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(yes)/write(no)/execute(no)
+  //  5. permission code:r--r--: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+  //  6. permission code:rw-r--: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(yes)/write(no)/execute(no)
+  //  7. permission code:rwx--x: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+  //  8. permission code:rwx---: owner:read(yes)/write(yes)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+  //  9. permission code:r-x--x: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(yes)
+  // 10. permission code:r-x---: owner:read(yes)/write(no)/execute(yes)|the shared action's user:download(no)/write(no)/execute(no)
+  // 11. permission code:r-----: owner:read(yes)/write(no)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+  // 12. permission code:rw----: owner:read(yes)/write(yes)/execute(no)|the shared action's user:download(no)/write(no)/execute(no)
+  val permissionList = List(
+    defaultPermissions,
+    "rwxr--",
+    "r-xr-x",
+    "r-xr--",
+    "r--r--",
+    "rw-r--",
+    "rwx--x",
+    "rwx---",
+    "r-x--x",
+    "r-x---",
+    "r-----",
+    "rw----")
+
   override val collectionName = "actions"
   override val cacheEnabled = true
 
diff --git a/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala b/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala
@@ -20,23 +20,16 @@ package org.apache.openwhisk.http
 import scala.concurrent.duration.Duration
 import scala.concurrent.duration.FiniteDuration
 import scala.util.Try
-
 import akka.http.scaladsl.model.StatusCode
 import akka.http.scaladsl.model.StatusCodes.Forbidden
 import akka.http.scaladsl.model.StatusCodes.NotFound
 import akka.http.scaladsl.model.MediaType
 import akka.http.scaladsl.server.Directives
 import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport.sprayJsonMarshaller
 import akka.http.scaladsl.server.StandardRoute
-
 import spray.json._
-
 import org.apache.openwhisk.common.TransactionId
-import org.apache.openwhisk.core.entity.SizeError
-import org.apache.openwhisk.core.entity.ByteSize
-import org.apache.openwhisk.core.entity.Exec
-import org.apache.openwhisk.core.entity.ExecMetaDataBase
-import org.apache.openwhisk.core.entity.ActivationId
+import org.apache.openwhisk.core.entity.{ActivationId, ByteSize, Exec, ExecMetaDataBase, SizeError}
 
 object Messages {
 
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
@@ -218,9 +218,22 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
 
         onComplete(checkAdditionalPrivileges) {
           case Success(_) =>
-            putEntity(WhiskAction, entityStore, entityName.toDocId, overwrite, update(user, request) _, () => {
-              make(user, entityName, request)
-            })
+            val operation = if (overwrite) "update" else "create"
+            onComplete(
+              entitlementProvider
+                .checkActionPermissions(
+                  operation,
+                  user,
+                  entityStore,
+                  entityName,
+                  WhiskAction.get,
+                  content.getPermissions())) {
+              case Success(_) =>
+                putEntity(WhiskAction, entityStore, entityName.toDocId, overwrite, update(user, request) _, () => {
+                  make(user, entityName, request)
+                })
+              case Failure(f) => super.handleEntitlementFailure(f)
+            }
           case Failure(f) =>
             super.handleEntitlementFailure(f)
         }
@@ -241,37 +254,43 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
    */
   override def activate(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
     implicit transid: TransactionId) = {
-    parameter(
-      'blocking ? false,
-      'result ? false,
-      'timeout.as[FiniteDuration] ? controllerActivationConfig.maxWaitForBlockingActivation) {
-      (blocking, result, waitOverride) =>
-        entity(as[Option[JsObject]]) { payload =>
-          getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
-            act: WhiskActionMetaData =>
-              // resolve the action --- special case for sequences that may contain components with '_' as default package
-              val action = act.resolve(user.namespace)
-              onComplete(entitleReferencedEntitiesMetaData(user, Privilege.ACTIVATE, Some(action.exec))) {
-                case Success(_) =>
-                  val actionWithMergedParams = env.map(action.inherit(_)) getOrElse action
-
-                  // incoming parameters may not override final parameters (i.e., parameters with already defined values)
-                  // on an action once its parameters are resolved across package and binding
-                  val allowInvoke = payload
-                    .map(_.fields.keySet.forall(key => !actionWithMergedParams.immutableParameters.contains(key)))
-                    .getOrElse(true)
+    onComplete(
+      entitlementProvider
+        .checkActionPermissions("invoke", user, entityStore, entityName, WhiskAction.get)) {
+      case Success(_) =>
+        parameter(
+          'blocking ? false,
+          'result ? false,
+          'timeout.as[FiniteDuration] ? controllerActivationConfig.maxWaitForBlockingActivation) {
+          (blocking, result, waitOverride) =>
+            entity(as[Option[JsObject]]) { payload =>
+              getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
+                act: WhiskActionMetaData =>
+                  // resolve the action --- special case for sequences that may contain components with '_' as default package
+                  val action = act.resolve(user.namespace)
+                  onComplete(entitleReferencedEntitiesMetaData(user, Privilege.ACTIVATE, Some(action.exec))) {
+                    case Success(_) =>
+                      val actionWithMergedParams = env.map(action.inherit(_)) getOrElse action
+
+                      // incoming parameters may not override final parameters (i.e., parameters with already defined values)
+                      // on an action once its parameters are resolved across package and binding
+                      val allowInvoke = payload
+                        .map(_.fields.keySet.forall(key => !actionWithMergedParams.immutableParameters.contains(key)))
+                        .getOrElse(true)
+
+                      if (allowInvoke) {
+                        doInvoke(user, actionWithMergedParams, payload, blocking, waitOverride, result)
+                      } else {
+                        terminate(BadRequest, Messages.parametersNotAllowed)
+                      }
 
-                  if (allowInvoke) {
-                    doInvoke(user, actionWithMergedParams, payload, blocking, waitOverride, result)
-                  } else {
-                    terminate(BadRequest, Messages.parametersNotAllowed)
+                    case Failure(f) =>
+                      super.handleEntitlementFailure(f)
                   }
-
-                case Failure(f) =>
-                  super.handleEntitlementFailure(f)
-              }
-          })
+              })
+            }
         }
+      case Failure(f) => super.handleEntitlementFailure(f)
     }
   }
 
@@ -333,11 +352,17 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
    * - 500 Internal Server Error
    */
   override def remove(user: Identity, entityName: FullyQualifiedEntityName)(implicit transid: TransactionId) = {
-    deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
+    onComplete(
+      entitlementProvider
+        .checkActionPermissions("remove", user, entityStore, entityName, WhiskAction.get)) {
+      case Success(_) =>
+        deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
+      case Failure(f) => super.handleEntitlementFailure(f)
+    }
   }
 
   /** Checks for package binding case. we don't want to allow get for a package binding in shared package */
-  private def fetchEntity(entityName: FullyQualifiedEntityName, env: Option[Parameters], code: Boolean)(
+  private def fetchEntity(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters], code: Boolean)(
     implicit transid: TransactionId) = {
     val resolvedPkg: Future[Either[String, FullyQualifiedEntityName]] = if (entityName.path.defaultPackage) {
       Future.successful(Right(entityName))
@@ -357,13 +382,19 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
           case Left(f) => terminate(Forbidden, f)
           case Right(_) =>
             if (code) {
-              getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
-                action: WhiskAction =>
-                  val mergedAction = env map {
-                    action inherit _
-                  } getOrElse action
-                  complete(OK, mergedAction)
-              })
+              onComplete(
+                entitlementProvider
+                  .checkActionPermissions("download", user, entityStore, entityName, WhiskAction.get)) {
+                case Success(_) =>
+                  getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
+                    action: WhiskAction =>
+                      val mergedAction = env map {
+                        action inherit _
+                      } getOrElse action
+                      complete(OK, mergedAction)
+                  })
+                case Failure(f) => super.handleEntitlementFailure(f)
+              }
             } else {
               getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
                 action: WhiskActionMetaData =>
@@ -396,7 +427,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
       if (executeOnly && user.namespace.name != entityName.namespace) {
         terminate(Forbidden, forbiddenGetAction(entityName.path.asString))
       } else {
-        fetchEntity(entityName, env, code)
+        fetchEntity(user, entityName, env, code)
       }
     }
   }
diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/Entitlement.scala
@@ -36,9 +36,12 @@ import org.apache.openwhisk.core.loadBalancer.{LoadBalancer, ShardingContainerPo
 import org.apache.openwhisk.http.ErrorResponse
 import org.apache.openwhisk.http.Messages
 import org.apache.openwhisk.core.connector.MessagingProvider
+import org.apache.openwhisk.core.database.ArtifactStore
 import org.apache.openwhisk.spi.SpiLoader
 import org.apache.openwhisk.spi.Spi
 
+import spray.json.DefaultJsonProtocol._
+
 object types {
   type Entitlements = TrieMap[(Subject, String), Set[Privilege]]
 }
@@ -231,6 +234,129 @@ protected[core] abstract class EntitlementProvider(
       .getOrElse(Future.successful(()))
   }
 
+  /**
+   * Checks if action operation(get/write/execute) whether feasible
+   *
+   * @param operation the action operation, e.g. get/write/execute
+   * @param user the user who get/write/execute the action
+   * @param entityStore  store to write the action to
+   * @param entityName entityName
+   * @param permissions the passed permission code
+   * @return a promise that completes with success iff action operation is feasible
+   */
+  protected[core] def checkActionPermissions(
+    operation: String,
+    user: Identity,
+    entityStore: ArtifactStore[WhiskEntity],
+    entityName: FullyQualifiedEntityName,
+    get: (ArtifactStore[WhiskEntity], DocId, DocRevision, Boolean) => Future[WhiskAction],
+    permissions: Option[String] = None)(implicit transid: TransactionId): Future[Unit] = {
+    if (operation == "create") {
+      permissions
+        .map { value =>
+          if (WhiskAction.permissionList.contains(value)) {
+            Future.successful(())
+          } else {
+            val errorInfo =
+              s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+            Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+          }
+        }
+        .getOrElse(Future.successful(()))
+    } else if (operation == "update") {
+      get(entityStore, entityName.toDocId, DocRevision.empty, true).flatMap { whiskAction =>
+        val currentPermissions = whiskAction.annotations
+          .get(WhiskAction.permissionsFieldName)
+          .map(value => value.convertTo[String])
+          .getOrElse(WhiskAction.defaultPermissions)
+
+        val errorInfo = s"have no permission to ${operation} this action"
+        permissions match {
+          case Some(value) =>
+            if (!WhiskAction.permissionList.contains(value)) {
+              val errorInfo =
+                s"give error permission code: ${value}, available permission is in ${WhiskAction.permissionList}"
+              Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+            } else {
+              val passedUpdatePermission = value.charAt(1)
+              if (passedUpdatePermission == 'w') { // make it to modifiable
+                Future.successful(())
+              } else {
+                val currentUpdatePermission = currentPermissions.charAt(1)
+                if (currentUpdatePermission == '-') {
+                  Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+                } else {
+                  Future.successful(())
+                }
+              }
+            }
+          case None =>
+            val currentUpdatePermission = currentPermissions.charAt(1)
+            if (currentUpdatePermission == '-') {
+              Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+            } else {
+              Future.successful(())
+            }
+        }
+      }
+    } else if (operation == "remove") {
+      get(entityStore, entityName.toDocId, DocRevision.empty, true).flatMap { whiskAction =>
+        val currentPermissions = whiskAction.annotations
+          .get(WhiskAction.permissionsFieldName)
+          .map(value => value.convertTo[String])
+          .getOrElse(WhiskAction.defaultPermissions)
+
+        val currentUpdatePermission = currentPermissions.charAt(1)
+        if (currentUpdatePermission == '-') {
+          val errorInfo = s"have no permission to ${operation} this action"
+          Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+        } else {
+          Future.successful(())
+        }
+      }
+    } else if (operation == "invoke") {
+      get(entityStore, entityName.toDocId, DocRevision.empty, true).flatMap { whiskAction =>
+        val currentPermissions = whiskAction.annotations
+          .get(WhiskAction.permissionsFieldName)
+          .map(value => value.convertTo[String])
+          .getOrElse(WhiskAction.defaultPermissions)
+
+        // the user who is owner by default
+        var currentExecutePermission = currentPermissions.charAt(2)
+        var errorInfo = s"have no permission to ${operation} this action"
+        if (user.namespace.name.asString != entityName.path.root.asString) { // the user who invoke the shared action
+          currentExecutePermission = currentPermissions.charAt(5)
+          errorInfo = s"have no permission to ${operation} this shared action"
+        }
+        if (currentExecutePermission == '-') { //have no permission
+          Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+        } else {
+          Future.successful(())
+        }
+      }
+    } else { // download the code
+      get(entityStore, entityName.toDocId, DocRevision.empty, true).flatMap { whiskAction =>
+        val currentPermissions = whiskAction.annotations
+          .get(WhiskAction.permissionsFieldName)
+          .map(value => value.convertTo[String])
+          .getOrElse(WhiskAction.defaultPermissions)
+
+        val errorInfo = s"have no permission to download this shared action"
+        val currentDownloadPermission = currentPermissions.charAt(3)
+        if (user.namespace.name.asString != entityName.path.root.asString) { // the shared user who download the action
+          if (currentDownloadPermission == '-') {
+            Future.failed(RejectRequest(Forbidden, Some(ErrorResponse(errorInfo, transid))))
+          } else {
+            Future.successful(())
+          }
+        } else {
+          // the owner has download permission on any situation
+          Future.successful(())
+        }
+      }
+    }
+  }
+
   /**
    * Checks if a subject has the right to access a specific resource. The entitlement may be implicit,
    * that is, inferred based on namespaces that a subject belongs to and the namespace of the
diff --git a/docs/actions.md b/docs/actions.md
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
