Improved logic of fetching actions

Author: Bhavesh Kemburu

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

@@ -231,7 +231,7 @@ object Messages {
   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 an action in a shared package binding."
+    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) =

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

@@ -106,9 +106,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
 
   /** Config flag for Execute Only for Actions in Shared Packages */
   protected def executeOnly =
-    Try({
-      loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
-    }).getOrElse(false)
+    loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
 
   /** Entity normalizer to JSON object. */
   import RestApiCommons.emptyEntityToJsObject
@@ -338,27 +336,27 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
     deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
   }
 
-  /**
-   * Gets action. The action name is prefixed with the namespace to create the primary index key.
-   *
-   * Responses are one of (Code, Message)
-   * - 200 WhiskAction has JSON
-   * - 404 Not Found
-   * - 500 Internal Server Error
-   */
-  override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
+  /** 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) = {
-    parameter('code ? true) { code =>
-      //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 {
-        code match {
-          case true =>
-            //Resolve Binding(Package) of the action
-            if (entityName.path.defaultPackage) {
+    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 {
@@ -367,51 +365,38 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
                   complete(OK, mergedAction)
               })
             } else {
-              getEntity(
-                WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true),
-                Some { pkg: WhiskPackage =>
-                  val originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace
-                  if (executeOnly && originalPackageLocation != entityName.namespace) {
-                    terminate(Forbidden, forbiddenGetActionBinding(entityName.toDocId.asString))
-                  } else {
-                    getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
-                      action: WhiskAction =>
-                        val mergedAction = env map {
-                          action inherit _
-                        } getOrElse action
-                        complete(OK, mergedAction)
-                    })
-                  }
-                })
-            }
-          case false =>
-            if (entityName.path.defaultPackage) {
               getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
                 action: WhiskActionMetaData =>
                   val mergedAction = env map {
                     action inherit _
                   } getOrElse action
                   complete(OK, mergedAction)
               })
-            } else {
-              getEntity(
-                WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true),
-                Some { pkg: WhiskPackage =>
-                  val originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace
-                  if (executeOnly && originalPackageLocation != entityName.namespace) {
-                    terminate(Forbidden, forbiddenGetActionBinding(entityName.toDocId.asString))
-                  } 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.
+   *
+   * Responses are one of (Code, Message)
+   * - 200 WhiskAction has JSON
+   * - 404 Not Found
+   * - 500 Internal Server Error
+   */
+  override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
+    implicit transid: TransactionId) = {
+    parameter('code ? true) { code =>
+      //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

@@ -18,7 +18,7 @@
 package org.apache.openwhisk.core.controller
 
 import scala.concurrent.Future
-import scala.util.{Failure, Success, Try}
+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}
@@ -45,9 +45,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
 
   /** Config flag for Execute Only for Shared Packages */
   protected def executeOnly =
-    Try({
-      loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
-    }).getOrElse(false)
+    loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
 
   /** Notification service for cache invalidation. */
   protected implicit val cacheChangeNotification: Some[CacheChangeNotification]
@@ -321,10 +319,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
         } else {
 
           /** Here's where I check package execute only case with package binding. */
-          val packagePath = wp.namespace.asString
-          val bindingPath = wp.binding.iterator.next().toString
-          val indexOfSlash = bindingPath.indexOf('/')
-          if (executeOnly && packagePath != bindingPath.take(indexOfSlash)) {
+          if (executeOnly && wp.namespace.asString != b.namespace.asString) {
             terminate(Forbidden, forbiddenGetPackageBinding(wp.name.asString))
           } else {
             getEntity(WhiskPackage.get(entityStore, docid), Some {