Execute Only for Shared Actions (#4935)

* Initial Commit of Execute Only

common/scala/src/main/resources/application.conf

@@ -99,6 +99,7 @@ kamon {
 }
 
 whisk {
+    shared-packages-execute-only = false
     metrics {
         # Enable/disable Prometheus support. If enabled then metrics would be exposed at `/metrics` endpoint
         # If Prometheus is enabled then please review `kamon.metric.tick-interval` (set to 1 sec by default above).

common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala

@@ -264,6 +264,7 @@ object ConfigKeys {
   val featureFlags = "whisk.feature-flags"
 
   val whiskConfig = "whisk.config"
+  val sharedPackageExecuteOnly = s"whisk.shared-packages-execute-only"
   val swaggerUi = "whisk.swagger-ui"
 
   val disableStoreResult = s"$activation.disable-store-result"

common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala

@@ -229,6 +229,15 @@ object Messages {
 
   /** Indicates that the container for the action could not be started. */
   val resourceProvisionError = "Failed to provision resources to run the action."
+
+  def forbiddenGetActionBinding(entityDocId: String) =
+    s"GET not permitted for '$entityDocId'. Resource does not exist or is an action in a shared package binding."
+  def forbiddenGetAction(entityPath: String) =
+    s"GET not permitted for '$entityPath' since it's an action in a shared package"
+  def forbiddenGetPackageBinding(packageName: String) =
+    s"GET not permitted since $packageName is a binding of a shared package"
+  def forbiddenGetPackage(packageName: String) =
+    s"GET not permitted for '$packageName' since it's a shared package"
 }
 
 /** Replaces rejections with Json object containing cause and transaction id. */

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala

@@ -43,6 +43,8 @@ import org.apache.openwhisk.http.Messages._
 import org.apache.openwhisk.core.entitlement.Resource
 import org.apache.openwhisk.core.entitlement.Collection
 import org.apache.openwhisk.core.loadBalancer.LoadBalancerException
+import pureconfig._
+import org.apache.openwhisk.core.ConfigKeys
 
 /**
  * A singleton object which defines the properties that must be present in a configuration
@@ -102,6 +104,10 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
   /** Database service to get activations. */
   protected val activationStore: ActivationStore
 
+  /** Config flag for Execute Only for Actions in Shared Packages */
+  protected def executeOnly =
+    loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
+
   /** Entity normalizer to JSON object. */
   import RestApiCommons.emptyEntityToJsObject
 
@@ -330,6 +336,50 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
     deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
   }
 
+  /** 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)(
+    implicit transid: TransactionId) = {
+    val resolvedPkg: Future[Either[String, FullyQualifiedEntityName]] = if (entityName.path.defaultPackage) {
+      Future.successful(Right(entityName))
+    } else {
+      WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true).map { pkg =>
+        val originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace
+        if (executeOnly && originalPackageLocation != entityName.namespace) {
+          Left(forbiddenGetActionBinding(entityName.toDocId.asString))
+        } else {
+          Right(entityName)
+        }
+      }
+    }
+    onComplete(resolvedPkg) {
+      case Success(pkgFuture) =>
+        pkgFuture match {
+          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)
+              })
+            } else {
+              getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
+                action: WhiskActionMetaData =>
+                  val mergedAction = env map {
+                    action inherit _
+                  } getOrElse action
+                  complete(OK, mergedAction)
+              })
+            }
+        }
+      case Failure(t: Throwable) =>
+        logging.error(this, s"[GET] package ${entityName.path.toDocId} failed: ${t.getMessage}")
+        terminate(InternalServerError)
+    }
+  }
+
   /**
    * Gets action. The action name is prefixed with the namespace to create the primary index key.
    *
@@ -341,22 +391,12 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
   override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
     implicit transid: TransactionId) = {
     parameter('code ? true) { code =>
-      code match {
-        case true =>
-          getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction =>
-            val mergedAction = env map {
-              action inherit _
-            } getOrElse action
-            complete(OK, mergedAction)
-          })
-        case false =>
-          getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
-            action: WhiskActionMetaData =>
-              val mergedAction = env map {
-                action inherit _
-              } getOrElse action
-              complete(OK, mergedAction)
-          })
+      //check if execute only is enabled, and if there is a discrepancy between the current user's namespace
+      //and that of the entity we are trying to fetch
+      if (executeOnly && user.namespace.name != entityName.namespace) {
+        terminate(Forbidden, forbiddenGetAction(entityName.path.asString))
+      } else {
+        fetchEntity(entityName, env, code)
       }
     }
   }

core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala

@@ -19,12 +19,10 @@ package org.apache.openwhisk.core.controller
 
 import scala.concurrent.Future
 import scala.util.{Failure, Success}
-
 import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._
 import akka.http.scaladsl.model.StatusCodes._
 import akka.http.scaladsl.server.{RequestContext, RouteResult}
 import akka.http.scaladsl.unmarshalling.Unmarshaller
-
 import org.apache.openwhisk.common.TransactionId
 import org.apache.openwhisk.core.controller.RestApiCommons.{ListLimit, ListSkip}
 import org.apache.openwhisk.core.database.{CacheChangeNotification, DocumentTypeMismatchException, NoDocumentException}
@@ -33,6 +31,9 @@ import org.apache.openwhisk.core.entity._
 import org.apache.openwhisk.core.entity.types.EntityStore
 import org.apache.openwhisk.http.ErrorResponse.terminate
 import org.apache.openwhisk.http.Messages
+import org.apache.openwhisk.http.Messages._
+import pureconfig._
+import org.apache.openwhisk.core.ConfigKeys
 
 trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
   services: WhiskServices =>
@@ -42,6 +43,10 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
   /** Database service to CRUD packages. */
   protected val entityStore: EntityStore
 
+  /** Config flag for Execute Only for Shared Packages */
+  protected def executeOnly =
+    loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
+
   /** Notification service for cache invalidation. */
   protected implicit val cacheChangeNotification: Some[CacheChangeNotification]
 
@@ -157,7 +162,14 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
    */
   override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
     implicit transid: TransactionId) = {
-    getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some { mergePackageWithBinding() _ })
+    if (executeOnly && user.namespace.name != entityName.namespace) {
+      val value = entityName.toString
+      terminate(Forbidden, forbiddenGetPackage(entityName.asString))
+    } else {
+      getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some {
+        mergePackageWithBinding() _
+      })
+    }
   }
 
   /**
@@ -303,9 +315,17 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
           logging.error(this, s"unexpected package binding refers to itself: $docid")
           terminate(UnprocessableEntity, Messages.packageBindingCircularReference(b.fullyQualifiedName.toString))
         } else {
-          getEntity(WhiskPackage.get(entityStore, docid), Some {
-            mergePackageWithBinding(Some { wp }) _
-          })
+
+          /** Here's where I check package execute only case with package binding. */
+          if (executeOnly && wp.namespace.asString != b.namespace.asString) {
+            terminate(Forbidden, forbiddenGetPackageBinding(wp.name.asString))
+          } else {
+            getEntity(WhiskPackage.get(entityStore, docid), Some {
+              mergePackageWithBinding(Some {
+                wp
+              }) _
+            })
+          }
         }
     } getOrElse {
       val pkg = ref map { _ inherit wp.parameters } getOrElse wp

tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala

@@ -187,7 +187,6 @@ class PackageActionsApiTests extends ControllerTestCommon with WhiskActionsApi {
       status should be(BadRequest)
     }
   }
-
   it should "reject put action in package binding" in {
     implicit val tid = transid()
     val provider = WhiskPackage(namespace, aname(), None, publish = true)
@@ -636,4 +635,37 @@ class PackageActionsApiTests extends ControllerTestCommon with WhiskActionsApi {
       responseAs[ErrorResponse].error shouldBe Messages.corruptedEntity
     }
   }
+
+  var testExecuteOnly = false
+  override def executeOnly = testExecuteOnly
+
+  it should ("allow access to get of action in binding of shared package when config option is disabled") in {
+    testExecuteOnly = false
+    implicit val tid = transid()
+    val auser = WhiskAuthHelpers.newIdentity()
+    val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true)
+    val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B"))
+    val action = WhiskAction(provider.fullPath, aname(), jsDefault("??"), Parameters("a", "A"))
+    put(entityStore, provider)
+    put(entityStore, binding)
+    put(entityStore, action)
+    Get(s"$collectionPath/${binding.name}/${action.name}") ~> Route.seal(routes(auser)) ~> check {
+      status should be(OK)
+    }
+  }
+
+  it should ("deny access to get of action in binding of shared package when config option is enabled") in {
+    testExecuteOnly = true
+    implicit val tid = transid()
+    val auser = WhiskAuthHelpers.newIdentity()
+    val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true)
+    val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B"))
+    val action = WhiskAction(provider.fullPath, aname(), jsDefault("??"), Parameters("a", "A"))
+    put(entityStore, provider)
+    put(entityStore, binding)
+    put(entityStore, action)
+    Get(s"$collectionPath/${binding.name}/${action.name}") ~> Route.seal(routes(auser)) ~> check {
+      status should be(Forbidden)
+    }
+  }
 }

tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala

@@ -884,4 +884,58 @@ class PackagesApiTests extends ControllerTestCommon with WhiskPackagesApi {
       responseAs[ErrorResponse].error shouldBe Messages.corruptedEntity
     }
   }
+
+  var testExecuteOnly = false
+  override def executeOnly = testExecuteOnly
+
+  it should ("allow access to get of shared package binding when config option is disabled") in {
+    testExecuteOnly = false
+    implicit val tid = transid()
+    val auser = WhiskAuthHelpers.newIdentity()
+    val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true)
+    val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B"))
+    put(entityStore, provider)
+    put(entityStore, binding)
+    Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check {
+      status should be(OK)
+    }
+  }
+
+  it should ("allow access to get of shared package when config option is disabled") in {
+    testExecuteOnly = false
+    implicit val tid = transid()
+    val auser = WhiskAuthHelpers.newIdentity()
+    val provider = WhiskPackage(namespace, aname(), None, publish = true)
+    put(entityStore, provider)
+
+    Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check {
+      status should be(OK)
+    }
+  }
+
+  it should ("deny access to get of shared package binding when config option is enabled") in {
+    testExecuteOnly = true
+    implicit val tid = transid()
+    val auser = WhiskAuthHelpers.newIdentity()
+    val provider = WhiskPackage(namespace, aname(), None, Parameters("p", "P"), publish = true)
+    val binding = WhiskPackage(EntityPath(auser.subject.asString), aname(), provider.bind, Parameters("b", "B"))
+    put(entityStore, provider)
+    put(entityStore, binding)
+    Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check {
+      status should be(Forbidden)
+    }
+
+  }
+
+  it should ("deny access to get of shared package when config option is enabled") in {
+    testExecuteOnly = true
+    implicit val tid = transid()
+    val auser = WhiskAuthHelpers.newIdentity()
+    val provider = WhiskPackage(namespace, aname(), None, publish = true)
+    put(entityStore, provider)
+
+    Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check {
+      status should be(Forbidden)
+    }
+  }
 }