From 1039d88b7beed1a94fa7b75ba7442c1f56b31e91 Mon Sep 17 00:00:00 2001 From: Bhavesh Kemburu Date: Fri, 17 Jul 2020 10:52:38 -0400 Subject: [PATCH 1/8] Initial Commit of Execute Only --- .../scala/src/main/resources/application.conf | 1 + .../apache/openwhisk/core/WhiskConfig.scala | 3 +- .../openwhisk/core/controller/Actions.scala | 111 +++++++++++++++--- .../openwhisk/core/controller/Packages.scala | 55 ++++++--- .../test/ExecuteOnlyActionsTests.scala | 62 ++++++++++ .../test/ExecuteOnlyPackagesTests.scala | 97 +++++++++++++++ 6 files changed, 293 insertions(+), 36 deletions(-) create mode 100644 tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala create mode 100644 tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala diff --git a/common/scala/src/main/resources/application.conf b/common/scala/src/main/resources/application.conf index 6e4a88b16a7..0f2efd0f3f8 100644 --- a/common/scala/src/main/resources/application.conf +++ b/common/scala/src/main/resources/application.conf @@ -99,6 +99,7 @@ kamon { } whisk { + #packages-execute-only = true 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). diff --git a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala index 6a54f6344f9..9b029656387 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala +++ b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala @@ -106,7 +106,6 @@ object WhiskConfig { propfile(base.getParent, true) else null } else null - val dir = sys.props.get("user.dir") if (dir.isDefined) { propfile(dir.get, true) @@ -264,6 +263,7 @@ object ConfigKeys { val featureFlags = "whisk.feature-flags" val whiskConfig = "whisk.config" + val packageExecuteOnly = s"whisk.packages-execute-only" val swaggerUi = "whisk.swagger-ui" val disableStoreResult = s"$activation.disable-store-result" @@ -272,4 +272,5 @@ object ConfigKeys { val apacheClientConfig = "whisk.apache-client" val parameterStorage = "whisk.parameter-storage" + } 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 056aadf4390..5cb21dbc823 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 @@ -16,8 +16,8 @@ */ package org.apache.openwhisk.core.controller - -import scala.concurrent.Future +import scala.language.postfixOps +import scala.concurrent.{Await, Future} import scala.concurrent.duration._ import scala.util.{Failure, Success, Try} import org.apache.kafka.common.errors.RecordTooLargeException @@ -26,6 +26,7 @@ import akka.http.scaladsl.model.StatusCodes._ import akka.http.scaladsl.server.RequestContext import akka.http.scaladsl.server.RouteResult import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ +import akka.http.scaladsl.model.StatusCode import akka.http.scaladsl.unmarshalling._ import spray.json._ import spray.json.DefaultJsonProtocol._ @@ -167,7 +168,6 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with if (right == Privilege.READ || right == Privilege.ACTIVATE) { wp: WhiskPackage => val actionResource = Resource(wp.fullPath, collection, Some(innername)) dispatchOp(user, right, actionResource) - } else { // these packaged action operations do not need merging with the package, // but may not be permitted if this is a binding, or if the subject does @@ -233,6 +233,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with * - 502 Bad Gateway * - 500 Internal Server Error */ + override def activate(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { parameter( @@ -251,10 +252,10 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with // 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 { @@ -330,6 +331,17 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({})) } + /** Load Config Option "packageExecuteOnly" as a boolean variable*/ +private val executeOnly = { + import pureconfig._ + import org.apache.openwhisk.core.ConfigKeys + try { + loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) + }catch{ + case ex: Exception => false + } +} + /** * Gets action. The action name is prefixed with the namespace to create the primary index key. * @@ -337,28 +349,89 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with * - 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 => code match { case true => - getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction => - 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 == true && user.namespace.name.toString != entityName.namespace.toString) { + val value = entityName.path + terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's an action in a shared package not owned by the current user") + } else { + + //Resolve Binding(Package) of the action + val pkgDocid = entityName.path.toDocId + val wp = WhiskPackage.resolveBinding(entityStore, pkgDocid, mergeParameters = true) + + var originalPackageLocation = "" + + //Retrieve the root package of the action(if there's a binding, then this will retrieve the original package + Try(Await.result(wp, 10 seconds)) match{ + case Success(pkg) => { + originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace.toString + } + case Failure(e) => { + terminate(StatusCode.int2StatusCode(500), "Failed to resolve package binding") + } + } + + //check the following conditions: execute only enabled, original entity location same as that of our entity now + //and do we have a binding here in the first place. + if (executeOnly == true && originalPackageLocation != entityName.namespace.toString && !entityName.path.defaultPackage) { + val value = entityName.toDocId + terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value'. The resource either does not exist or it's an action in a package binding, and the original package is not owned by the current user") + } else { + 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) - }) - } - } + //and that of the entity we are trying to fetch + if (executeOnly == true && user.namespace.name.toString != entityName.namespace.toString) { + val value = entityName.toString + terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's an action in a shared package not owned by the current user") + } else { + //Resolve Binding(Package) of the action + val pkgDocid = entityName.path.toDocId + val wp = WhiskPackage.resolveBinding(entityStore, pkgDocid, mergeParameters = true) + var originalPackageLocation = "" + + //Retrieve the root package of the action(if there's a binding, then this will retrieve the original package + Try(Await.result(wp, 10 seconds)) match{ + case Success(pkg) => { + originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace.toString + } + case Failure(e) => { + terminate(StatusCode.int2StatusCode(500), "Failed to resolve package binding") + } + } + + //check the following conditions: execute only enabled, original entity location same as that of our entity now + //and do we have a binding here in the first place. + if (executeOnly == true && originalPackageLocation != entityName.namespace.toString && !entityName.path.defaultPackage) { + val value = entityName.toDocId + terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value'. The resource either does not exist or it's an action in a package binding, and the original package is not owned by the current user") + } else { + getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { + action: WhiskActionMetaData => + val mergedAction = env map { + action inherit _ + } getOrElse action + complete(OK, mergedAction) + }) + } + } + } + } } /** diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala index b1e555bbbef..09dde97f7d6 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala +++ b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala @@ -19,12 +19,11 @@ 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.StatusCode 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} @@ -34,6 +33,8 @@ import org.apache.openwhisk.core.entity.types.EntityStore import org.apache.openwhisk.http.ErrorResponse.terminate import org.apache.openwhisk.http.Messages +import scala.collection.immutable.Set + trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { services: WhiskServices => @@ -98,13 +99,13 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { } } } - /** * Activating a package is not supported. This method is not permitted and is not reachable. * * Responses are one of (Code, Message) * - 405 Not Allowed */ + override def activate(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { logging.error(this, "activate is not permitted on packages") @@ -145,7 +146,16 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { } }) } - + private val executeOnly = { + import pureconfig._ + import pureconfig.generic.auto._ + import org.apache.openwhisk.core.ConfigKeys + try { + loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) + }catch{ + case ex: Exception => false + } + } /** * Gets package/binding. * The package/binding name is prefixed with the namespace to create the primary index key. @@ -155,11 +165,20 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { * - 404 Not Found * - 500 Internal Server Error */ + + //get method that also checks for execute only to deny access to shared package, but package binding is handled + //within the mergePackageWithBinding() method override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { - getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some { mergePackageWithBinding() _ }) + if (executeOnly == true && user.namespace.name.toString != entityName.namespace.toString) { + val value = entityName.toString + terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's a shared package not owned by the current user") + }else { + getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some { + mergePackageWithBinding() _ + }) + } } - /** * Gets all packages/bindings in namespace. * @@ -192,7 +211,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { } } } - /** * Validates that a referenced binding exists. */ @@ -210,7 +228,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { case _ => Future.successful({}) } } - /** * Creates a WhiskPackage from PUT content, generating default values where necessary. * If this is a binding, confirm the referenced package exists. @@ -220,7 +237,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { val validateBinding = content.binding map { b => checkBinding(b.fullyQualifiedName) } getOrElse Future.successful({}) - validateBinding map { _ => WhiskPackage( pkgName.path, @@ -235,7 +251,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { ++ bindingAnnotation(content.binding)) } } - /** Updates a WhiskPackage from PUT content, merging old package/binding where necessary. */ private def update(content: WhiskPackagePut)(wp: WhiskPackage)( implicit transid: TransactionId): Future[WhiskPackage] = { @@ -274,7 +289,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { case _ => super.handleEntitlementFailure(failure) } } - /** * Constructs a "binding" annotation. This is redundant with the binding * information available in WhiskPackage but necessary for some clients which @@ -287,7 +301,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { Parameters(WhiskPackage.bindingFieldName, Binding.serdes.write(b)) } getOrElse Parameters() } - /** * Constructs a WhiskPackage that is a merger of a package with its packing binding (if any). * If this is a binding, fetch package for binding, merge parameters then emit. @@ -298,14 +311,25 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { wp.binding map { case b: Binding => val docid = b.fullyQualifiedName.toDocId + logging.debug(this, s"fetching package '$docid' for reference") if (docid == wp.docid) { 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. */ + val packagePath = wp.namespace.toString() + val bindingPath = wp.binding.iterator.next().toString() + val indexOfSlash = bindingPath.indexOf('/') + if (executeOnly == true && packagePath != bindingPath.take(indexOfSlash)){ + terminate(StatusCode.int2StatusCode(403), s"GET not permitted since ${wp.name.toString} is a binding of a shared package that does not belong to the current user") + }else { + getEntity(WhiskPackage.get(entityStore, docid), Some { + mergePackageWithBinding(Some { + wp + }) _ + }) + } } } getOrElse { val pkg = ref map { _ inherit wp.parameters } getOrElse wp @@ -323,7 +347,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { new IllegalStateException(s"unexpected result in package action lookup: $t") } } - onComplete(actions) { case Success(p) => logging.debug(this, s"[GET] entity success") diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala new file mode 100644 index 00000000000..36738e90864 --- /dev/null +++ b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala @@ -0,0 +1,62 @@ +package org.apache.openwhisk.core.controller.test + +import akka.http.scaladsl.model.StatusCodes._ +import akka.http.scaladsl.server.Route +import org.junit.runner.RunWith +import org.scalatest.junit.JUnitRunner +import org.apache.openwhisk.core.entity._ +import org.apache.openwhisk.core.controller.WhiskActionsApi + + +@RunWith(classOf[JUnitRunner]) +class ExecuteOnlyActionsTests extends ControllerTestCommon with WhiskActionsApi{ + behavior of("Execute Only for Actions") + + val creds = WhiskAuthHelpers.newIdentity() + val namespace = EntityPath(creds.subject.asString) + val collectionPath = s"/${EntityPath.DEFAULT}/${collection.path}" + def aname() = MakeName.next("package_action_tests") + + private val executeOnly = { + import pureconfig._ + import pureconfig.generic.auto._ + import org.apache.openwhisk.core.ConfigKeys + try { + loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) + }catch{ + case ex: Exception => false + } + } + + it should("deny access to get of action in binding of shared package when config option is enabled") in { + if (executeOnly == 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) + } + } + } + + it should("allow access to get of action in binding of shared package when config option is disabled") in { + if (executeOnly == 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) + } + } + } +} \ No newline at end of file diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala new file mode 100644 index 00000000000..b7ac2ac058e --- /dev/null +++ b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala @@ -0,0 +1,97 @@ +package org.apache.openwhisk.core.controller.test + +import akka.http.scaladsl.model.StatusCodes._ +import akka.http.scaladsl.server.Route +import org.junit.runner.RunWith +import org.scalatest.junit.JUnitRunner +import org.apache.openwhisk.core.controller.WhiskPackagesApi +import org.apache.openwhisk.core.entity._ + + + +@RunWith(classOf[JUnitRunner]) +class ExecuteOnlyPackagesTests extends ControllerTestCommon with WhiskPackagesApi{ + behavior of("Execute Only for Packages") + + /** Initialize a user identity, and some relevant information */ + val creds = WhiskAuthHelpers.newIdentity() + val namespace = EntityPath(creds.subject.asString) + val collectionPath = s"/${EntityPath.DEFAULT}/${collection.path}" + def aname() = MakeName.next("execute_only_tests") + val parametersLimit = Parameters.sizeLimit + + private def bindingAnnotation(binding: Binding) = { + Parameters(WhiskPackage.bindingFieldName, Binding.serdes.write(binding)) + } + + + /** Read in config Option from the config key */ + private val executeOnly = { + import pureconfig._ + import pureconfig.generic.auto._ + import org.apache.openwhisk.core.ConfigKeys + try { + loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) + }catch{ + case ex: Exception => false + } + } + + it should("deny access to get of shared package when config option is enabled") in { + //set config option to true + //create shared package within current identity + if (executeOnly == 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) + } + } + + } + + it should("allow access to get of shared package when config option is disabled") in { + //set config option to false + if (executeOnly == 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 { + if (executeOnly == 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("allow access to get of shared package binding when config option is disabled") in { + if (executeOnly == 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) + } + } + } + +} \ No newline at end of file From c7208797372974d65692bf91aaf2faba0c21a08f Mon Sep 17 00:00:00 2001 From: Bhavesh Kemburu Date: Fri, 17 Jul 2020 19:13:12 -0400 Subject: [PATCH 2/8] Changed name of config flag/Updated logic of retrieving information about package binding --- .../apache/openwhisk/core/WhiskConfig.scala | 6 +- .../openwhisk/core/controller/Actions.scala | 153 +++++++----------- .../openwhisk/core/controller/Packages.scala | 46 +++--- 3 files changed, 90 insertions(+), 115 deletions(-) diff --git a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala index 9b029656387..e0ebf79f493 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala +++ b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala @@ -106,6 +106,7 @@ object WhiskConfig { propfile(base.getParent, true) else null } else null + val dir = sys.props.get("user.dir") if (dir.isDefined) { propfile(dir.get, true) @@ -263,7 +264,7 @@ object ConfigKeys { val featureFlags = "whisk.feature-flags" val whiskConfig = "whisk.config" - val packageExecuteOnly = s"whisk.packages-execute-only" + val sharedPackageExecuteOnly = s"whisk.shared-packages-execute-only" val swaggerUi = "whisk.swagger-ui" val disableStoreResult = s"$activation.disable-store-result" @@ -272,5 +273,4 @@ object ConfigKeys { val apacheClientConfig = "whisk.apache-client" val parameterStorage = "whisk.parameter-storage" - -} +} \ No newline at end of file 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 5cb21dbc823..847467c1dcd 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 @@ -16,8 +16,8 @@ */ package org.apache.openwhisk.core.controller -import scala.language.postfixOps -import scala.concurrent.{Await, Future} + +import scala.concurrent.Future import scala.concurrent.duration._ import scala.util.{Failure, Success, Try} import org.apache.kafka.common.errors.RecordTooLargeException @@ -44,7 +44,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 * in order to implement the actions API. @@ -103,6 +104,12 @@ 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 configureExecuteOnly = executeOnly + protected val executeOnly = { + Try({loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)}).getOrElse(false) + } + /** Entity normalizer to JSON object. */ import RestApiCommons.emptyEntityToJsObject @@ -168,6 +175,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with if (right == Privilege.READ || right == Privilege.ACTIVATE) { wp: WhiskPackage => val actionResource = Resource(wp.fullPath, collection, Some(innername)) dispatchOp(user, right, actionResource) + } else { // these packaged action operations do not need merging with the package, // but may not be permitted if this is a binding, or if the subject does @@ -233,7 +241,6 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with * - 502 Bad Gateway * - 500 Internal Server Error */ - override def activate(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { parameter( @@ -252,10 +259,10 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with // 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 { @@ -331,16 +338,7 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({})) } - /** Load Config Option "packageExecuteOnly" as a boolean variable*/ -private val executeOnly = { - import pureconfig._ - import org.apache.openwhisk.core.ConfigKeys - try { - loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) - }catch{ - case ex: Exception => false - } -} + /** * Gets action. The action name is prefixed with the namespace to create the primary index key. @@ -349,91 +347,62 @@ private val executeOnly = { * - 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 => - code match { - case true => - //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 == true && user.namespace.name.toString != entityName.namespace.toString) { - val value = entityName.path - terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's an action in a shared package not owned by the current user") - } else { - - //Resolve Binding(Package) of the action - val pkgDocid = entityName.path.toDocId - val wp = WhiskPackage.resolveBinding(entityStore, pkgDocid, mergeParameters = true) - - var originalPackageLocation = "" - - //Retrieve the root package of the action(if there's a binding, then this will retrieve the original package - Try(Await.result(wp, 10 seconds)) match{ - case Success(pkg) => { - originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace.toString - } - case Failure(e) => { - terminate(StatusCode.int2StatusCode(500), "Failed to resolve package binding") - } - } - - //check the following conditions: execute only enabled, original entity location same as that of our entity now - //and do we have a binding here in the first place. - if (executeOnly == true && originalPackageLocation != entityName.namespace.toString && !entityName.path.defaultPackage) { - val value = entityName.toDocId - terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value'. The resource either does not exist or it's an action in a package binding, and the original package is not owned by the current user") - } else { - getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) - } - } - - case false => - //and that of the entity we are trying to fetch - if (executeOnly == true && user.namespace.name.toString != entityName.namespace.toString) { - val value = entityName.toString - terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's an action in a shared package not owned by the current user") - } else { - //Resolve Binding(Package) of the action - val pkgDocid = entityName.path.toDocId - val wp = WhiskPackage.resolveBinding(entityStore, pkgDocid, mergeParameters = true) - var originalPackageLocation = "" - - //Retrieve the root package of the action(if there's a binding, then this will retrieve the original package - Try(Await.result(wp, 10 seconds)) match{ - case Success(pkg) => { - originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace.toString - } - case Failure(e) => { - terminate(StatusCode.int2StatusCode(500), "Failed to resolve package binding") + //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.toString != entityName.namespace.toString) { + val value = entityName.path + terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's an action in a shared package") + } else { + code match { + case true => + //Resolve Binding(Package) of the action + 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(StatusCode.int2StatusCode(403), + s"GET not permitted for '${entityName.toDocId}'. Resource does not exist or an action in a shared package binding") + }else{ + getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { + action: WhiskAction => + val mergedAction = env map { + action inherit _ + } getOrElse action + complete(OK, mergedAction) + }) + } + } + ) + case false => + 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(StatusCode.int2StatusCode(403), + s"GET not permitted for '${entityName.toDocId}'. Resource does not exist or an action in a shared package binding") + }else{ + getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { + action: WhiskActionMetaData => + val mergedAction = env map { + action inherit _ + } getOrElse action + complete(OK, mergedAction) + }) + } } - } - - //check the following conditions: execute only enabled, original entity location same as that of our entity now - //and do we have a binding here in the first place. - if (executeOnly == true && originalPackageLocation != entityName.namespace.toString && !entityName.path.defaultPackage) { - val value = entityName.toDocId - terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value'. The resource either does not exist or it's an action in a package binding, and the original package is not owned by the current user") - } else { - getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { - action: WhiskActionMetaData => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) - } - } - } + ) + } } + } } + /** * Gets all actions in a path. * diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala index 09dde97f7d6..0a1c380dc25 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala +++ b/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} +import scala.util.{Failure, Success, Try} import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ import akka.http.scaladsl.model.StatusCode import akka.http.scaladsl.model.StatusCodes._ @@ -32,8 +32,8 @@ 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 scala.collection.immutable.Set +import pureconfig._ +import org.apache.openwhisk.core.ConfigKeys trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { services: WhiskServices => @@ -43,6 +43,14 @@ 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 configureExecuteOnly = executeOnly + private val executeOnly = { + Try({ + loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) + }).getOrElse(false) + } + /** Notification service for cache invalidation. */ protected implicit val cacheChangeNotification: Some[CacheChangeNotification] @@ -99,13 +107,13 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { } } } + /** * Activating a package is not supported. This method is not permitted and is not reachable. * * Responses are one of (Code, Message) * - 405 Not Allowed */ - override def activate(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { logging.error(this, "activate is not permitted on packages") @@ -146,16 +154,8 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { } }) } - private val executeOnly = { - import pureconfig._ - import pureconfig.generic.auto._ - import org.apache.openwhisk.core.ConfigKeys - try { - loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) - }catch{ - case ex: Exception => false - } - } + + /** * Gets package/binding. * The package/binding name is prefixed with the namespace to create the primary index key. @@ -165,20 +165,20 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { * - 404 Not Found * - 500 Internal Server Error */ - //get method that also checks for execute only to deny access to shared package, but package binding is handled //within the mergePackageWithBinding() method override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { - if (executeOnly == true && user.namespace.name.toString != entityName.namespace.toString) { + if (executeOnly && user.namespace.name.toString != entityName.namespace.toString) { val value = entityName.toString - terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's a shared package not owned by the current user") + terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's a shared package") }else { getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some { mergePackageWithBinding() _ }) } } + /** * Gets all packages/bindings in namespace. * @@ -211,6 +211,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { } } } + /** * Validates that a referenced binding exists. */ @@ -228,6 +229,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { case _ => Future.successful({}) } } + /** * Creates a WhiskPackage from PUT content, generating default values where necessary. * If this is a binding, confirm the referenced package exists. @@ -237,6 +239,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { val validateBinding = content.binding map { b => checkBinding(b.fullyQualifiedName) } getOrElse Future.successful({}) + validateBinding map { _ => WhiskPackage( pkgName.path, @@ -251,6 +254,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { ++ bindingAnnotation(content.binding)) } } + /** Updates a WhiskPackage from PUT content, merging old package/binding where necessary. */ private def update(content: WhiskPackagePut)(wp: WhiskPackage)( implicit transid: TransactionId): Future[WhiskPackage] = { @@ -289,6 +293,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { case _ => super.handleEntitlementFailure(failure) } } + /** * Constructs a "binding" annotation. This is redundant with the binding * information available in WhiskPackage but necessary for some clients which @@ -301,6 +306,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { Parameters(WhiskPackage.bindingFieldName, Binding.serdes.write(b)) } getOrElse Parameters() } + /** * Constructs a WhiskPackage that is a merger of a package with its packing binding (if any). * If this is a binding, fetch package for binding, merge parameters then emit. @@ -311,7 +317,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { wp.binding map { case b: Binding => val docid = b.fullyQualifiedName.toDocId - logging.debug(this, s"fetching package '$docid' for reference") if (docid == wp.docid) { logging.error(this, s"unexpected package binding refers to itself: $docid") @@ -321,8 +326,8 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { val packagePath = wp.namespace.toString() val bindingPath = wp.binding.iterator.next().toString() val indexOfSlash = bindingPath.indexOf('/') - if (executeOnly == true && packagePath != bindingPath.take(indexOfSlash)){ - terminate(StatusCode.int2StatusCode(403), s"GET not permitted since ${wp.name.toString} is a binding of a shared package that does not belong to the current user") + if (executeOnly && packagePath != bindingPath.take(indexOfSlash)){ + terminate(StatusCode.int2StatusCode(403), s"GET not permitted since ${wp.name.toString} is a binding of a shared package") }else { getEntity(WhiskPackage.get(entityStore, docid), Some { mergePackageWithBinding(Some { @@ -347,6 +352,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { new IllegalStateException(s"unexpected result in package action lookup: $t") } } + onComplete(actions) { case Success(p) => logging.debug(this, s"[GET] entity success") From f7e4bf06a1d32d0fcf261207a0da9b8526c828f2 Mon Sep 17 00:00:00 2001 From: Bhavesh Kemburu Date: Mon, 20 Jul 2020 10:34:35 -0400 Subject: [PATCH 3/8] Modification of testing and merge into existing Unit Test Methods --- .../openwhisk/core/controller/Actions.scala | 7 +-- .../openwhisk/core/controller/Packages.scala | 10 ++-- .../test/PackageActionsApiTests.scala | 34 ++++++++++- .../controller/test/PackagesApiTests.scala | 56 ++++++++++++++++++- 4 files changed, 95 insertions(+), 12 deletions(-) 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 847467c1dcd..da48f2eae6a 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 @@ -105,10 +105,9 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with protected val activationStore: ActivationStore /** Config flag for Execute Only for Actions in Shared Packages */ - protected def configureExecuteOnly = executeOnly - protected val executeOnly = { - Try({loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)}).getOrElse(false) - } + protected def executeOnly = Try({ + loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) + }).getOrElse(false) /** Entity normalizer to JSON object. */ import RestApiCommons.emptyEntityToJsObject diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala index 0a1c380dc25..d3a656aca85 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala +++ b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala @@ -44,12 +44,10 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { protected val entityStore: EntityStore /** Config flag for Execute Only for Shared Packages */ - protected def configureExecuteOnly = executeOnly - private val executeOnly = { - Try({ - loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) - }).getOrElse(false) - } + + protected def executeOnly = Try({ + loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) + }).getOrElse(false) /** Notification service for cache invalidation. */ protected implicit val cacheChangeNotification: Some[CacheChangeNotification] diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala index a645e87c3ee..ec9f020e06e 100644 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala +++ b/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) + } + } } diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala index e59121f4373..1012a66d3ba 100644 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala +++ b/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) + } + } +} \ No newline at end of file From 5c193e6d102715d6c2f57c1c92d83109c086437b Mon Sep 17 00:00:00 2001 From: kemburu Date: Mon, 20 Jul 2020 10:39:33 -0400 Subject: [PATCH 4/8] Delete ExecuteOnlyActionsTests.scala Tests merged with PackageActionsApiTests.scala --- .../test/ExecuteOnlyActionsTests.scala | 62 ------------------- 1 file changed, 62 deletions(-) delete mode 100644 tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala deleted file mode 100644 index 36738e90864..00000000000 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyActionsTests.scala +++ /dev/null @@ -1,62 +0,0 @@ -package org.apache.openwhisk.core.controller.test - -import akka.http.scaladsl.model.StatusCodes._ -import akka.http.scaladsl.server.Route -import org.junit.runner.RunWith -import org.scalatest.junit.JUnitRunner -import org.apache.openwhisk.core.entity._ -import org.apache.openwhisk.core.controller.WhiskActionsApi - - -@RunWith(classOf[JUnitRunner]) -class ExecuteOnlyActionsTests extends ControllerTestCommon with WhiskActionsApi{ - behavior of("Execute Only for Actions") - - val creds = WhiskAuthHelpers.newIdentity() - val namespace = EntityPath(creds.subject.asString) - val collectionPath = s"/${EntityPath.DEFAULT}/${collection.path}" - def aname() = MakeName.next("package_action_tests") - - private val executeOnly = { - import pureconfig._ - import pureconfig.generic.auto._ - import org.apache.openwhisk.core.ConfigKeys - try { - loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) - }catch{ - case ex: Exception => false - } - } - - it should("deny access to get of action in binding of shared package when config option is enabled") in { - if (executeOnly == 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) - } - } - } - - it should("allow access to get of action in binding of shared package when config option is disabled") in { - if (executeOnly == 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) - } - } - } -} \ No newline at end of file From 989ad61ee4ec1ad3dcd6851305fcb7657eafff6c Mon Sep 17 00:00:00 2001 From: kemburu Date: Mon, 20 Jul 2020 10:40:11 -0400 Subject: [PATCH 5/8] Delete ExecuteOnlyPackagesTests.scala Tests merged with PackagesApiTests.scala --- .../test/ExecuteOnlyPackagesTests.scala | 97 ------------------- 1 file changed, 97 deletions(-) delete mode 100644 tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala deleted file mode 100644 index b7ac2ac058e..00000000000 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/ExecuteOnlyPackagesTests.scala +++ /dev/null @@ -1,97 +0,0 @@ -package org.apache.openwhisk.core.controller.test - -import akka.http.scaladsl.model.StatusCodes._ -import akka.http.scaladsl.server.Route -import org.junit.runner.RunWith -import org.scalatest.junit.JUnitRunner -import org.apache.openwhisk.core.controller.WhiskPackagesApi -import org.apache.openwhisk.core.entity._ - - - -@RunWith(classOf[JUnitRunner]) -class ExecuteOnlyPackagesTests extends ControllerTestCommon with WhiskPackagesApi{ - behavior of("Execute Only for Packages") - - /** Initialize a user identity, and some relevant information */ - val creds = WhiskAuthHelpers.newIdentity() - val namespace = EntityPath(creds.subject.asString) - val collectionPath = s"/${EntityPath.DEFAULT}/${collection.path}" - def aname() = MakeName.next("execute_only_tests") - val parametersLimit = Parameters.sizeLimit - - private def bindingAnnotation(binding: Binding) = { - Parameters(WhiskPackage.bindingFieldName, Binding.serdes.write(binding)) - } - - - /** Read in config Option from the config key */ - private val executeOnly = { - import pureconfig._ - import pureconfig.generic.auto._ - import org.apache.openwhisk.core.ConfigKeys - try { - loadConfigOrThrow[Boolean](ConfigKeys.packageExecuteOnly) - }catch{ - case ex: Exception => false - } - } - - it should("deny access to get of shared package when config option is enabled") in { - //set config option to true - //create shared package within current identity - if (executeOnly == 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) - } - } - - } - - it should("allow access to get of shared package when config option is disabled") in { - //set config option to false - if (executeOnly == 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 { - if (executeOnly == 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("allow access to get of shared package binding when config option is disabled") in { - if (executeOnly == 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) - } - } - } - -} \ No newline at end of file From 38880d97ef33caab02b7a306a5f6d264064d0d39 Mon Sep 17 00:00:00 2001 From: kemburu Date: Mon, 20 Jul 2020 10:45:37 -0400 Subject: [PATCH 6/8] Update application.conf Changed Flag to shared-packages-execute-only with default value set to false. --- .../scala/src/main/resources/application.conf | 2 +- .../apache/openwhisk/core/WhiskConfig.scala | 2 +- .../apache/openwhisk/http/ErrorResponse.scala | 9 ++ .../openwhisk/core/controller/Actions.scala | 97 ++++++++++--------- .../openwhisk/core/controller/Packages.scala | 30 +++--- .../test/PackageActionsApiTests.scala | 28 +++--- .../controller/test/PackagesApiTests.scala | 80 +++++++-------- 7 files changed, 130 insertions(+), 118 deletions(-) diff --git a/common/scala/src/main/resources/application.conf b/common/scala/src/main/resources/application.conf index 0f2efd0f3f8..5bb17600fb3 100644 --- a/common/scala/src/main/resources/application.conf +++ b/common/scala/src/main/resources/application.conf @@ -99,7 +99,7 @@ kamon { } whisk { - #packages-execute-only = true + 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). diff --git a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala index e0ebf79f493..dc04c5b68d5 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala +++ b/common/scala/src/main/scala/org/apache/openwhisk/core/WhiskConfig.scala @@ -273,4 +273,4 @@ object ConfigKeys { val apacheClientConfig = "whisk.apache-client" val parameterStorage = "whisk.parameter-storage" -} \ No newline at end of file +} 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 index 1198cdc9141..9dc841a9a04 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala +++ b/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 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. */ 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 da48f2eae6a..135290230b4 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 @@ -26,7 +26,6 @@ import akka.http.scaladsl.model.StatusCodes._ import akka.http.scaladsl.server.RequestContext import akka.http.scaladsl.server.RouteResult import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ -import akka.http.scaladsl.model.StatusCode import akka.http.scaladsl.unmarshalling._ import spray.json._ import spray.json.DefaultJsonProtocol._ @@ -46,6 +45,7 @@ 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 * in order to implement the actions API. @@ -105,9 +105,10 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with protected val activationStore: ActivationStore /** Config flag for Execute Only for Actions in Shared Packages */ - protected def executeOnly = Try({ - loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) - }).getOrElse(false) + protected def executeOnly = + Try({ + loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) + }).getOrElse(false) /** Entity normalizer to JSON object. */ import RestApiCommons.emptyEntityToJsObject @@ -337,8 +338,6 @@ 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. * @@ -352,56 +351,60 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with 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.toString != entityName.namespace.toString) { - val value = entityName.path - terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's an action in a shared package") + + if (executeOnly && user.namespace.name != entityName.namespace) { + terminate(Forbidden, forbiddenGetAction(entityName.path.asString)) } else { - code match { - case true => - //Resolve Binding(Package) of the action - 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(StatusCode.int2StatusCode(403), - s"GET not permitted for '${entityName.toDocId}'. Resource does not exist or an action in a shared package binding") - }else{ - getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { - action: WhiskAction => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) - } + val getEntityWhiskActionCase = + getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction => + val mergedAction = env map { + action inherit _ + } getOrElse action + complete(OK, mergedAction) + }) + val getEntityWhiskMetaDataCase = + getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { + action: WhiskActionMetaData => + val mergedAction = env map { + action inherit _ + } getOrElse action + complete(OK, mergedAction) + }) + if (code) { + if (entityName.path.defaultPackage) { + getEntityWhiskActionCase + } 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 { + getEntityWhiskActionCase } - ) - case false => + }) + } + } else { + if (entityName.path.defaultPackage) { + getEntityWhiskMetaDataCase + } else { getEntity( WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true), - Some {pkg: WhiskPackage => + Some { pkg: WhiskPackage => val originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace - if (executeOnly && originalPackageLocation != entityName.namespace){ - terminate(StatusCode.int2StatusCode(403), - s"GET not permitted for '${entityName.toDocId}'. Resource does not exist or an action in a shared package binding") - }else{ - getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { - action: WhiskActionMetaData => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) + if (executeOnly && originalPackageLocation != entityName.namespace) { + terminate(Forbidden, forbiddenGetActionBinding(entityName.toDocId.asString)) + } else { + getEntityWhiskMetaDataCase } - } - ) - } - } + }) + } + } + } } } - /** * Gets all actions in a path. * diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala index d3a656aca85..ecfa10ccf20 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala +++ b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala @@ -20,7 +20,6 @@ package org.apache.openwhisk.core.controller import scala.concurrent.Future import scala.util.{Failure, Success, Try} import akka.http.scaladsl.marshallers.sprayjson.SprayJsonSupport._ -import akka.http.scaladsl.model.StatusCode import akka.http.scaladsl.model.StatusCodes._ import akka.http.scaladsl.server.{RequestContext, RouteResult} import akka.http.scaladsl.unmarshalling.Unmarshaller @@ -32,6 +31,7 @@ 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 @@ -44,10 +44,10 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { protected val entityStore: EntityStore /** Config flag for Execute Only for Shared Packages */ - - protected def executeOnly = Try({ - loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) - }).getOrElse(false) + protected def executeOnly = + Try({ + loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly) + }).getOrElse(false) /** Notification service for cache invalidation. */ protected implicit val cacheChangeNotification: Some[CacheChangeNotification] @@ -153,7 +153,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { }) } - /** * Gets package/binding. * The package/binding name is prefixed with the namespace to create the primary index key. @@ -167,10 +166,10 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { //within the mergePackageWithBinding() method override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { - if (executeOnly && user.namespace.name.toString != entityName.namespace.toString) { - val value = entityName.toString - terminate(StatusCode.int2StatusCode(403), s"GET not permitted for '$value' since it's a shared package") - }else { + 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() _ }) @@ -320,13 +319,14 @@ 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 { + /** Here's where I check package execute only case with package binding. */ - val packagePath = wp.namespace.toString() - val bindingPath = wp.binding.iterator.next().toString() + val packagePath = wp.namespace.asString + val bindingPath = wp.binding.iterator.next().toString val indexOfSlash = bindingPath.indexOf('/') - if (executeOnly && packagePath != bindingPath.take(indexOfSlash)){ - terminate(StatusCode.int2StatusCode(403), s"GET not permitted since ${wp.name.toString} is a binding of a shared package") - }else { + if (executeOnly && packagePath != bindingPath.take(indexOfSlash)) { + terminate(Forbidden, forbiddenGetPackageBinding(wp.name.asString)) + } else { getEntity(WhiskPackage.get(entityStore, docid), Some { mergePackageWithBinding(Some { wp diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala index ec9f020e06e..71d821c13c4 100644 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala +++ b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackageActionsApiTests.scala @@ -639,7 +639,7 @@ class PackageActionsApiTests extends ControllerTestCommon with WhiskActionsApi { 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 { + 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() @@ -654,18 +654,18 @@ class PackageActionsApiTests extends ControllerTestCommon with WhiskActionsApi { } } - 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) - } + 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) + } } } diff --git a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala index 1012a66d3ba..c52ba07500d 100644 --- a/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala +++ b/tests/src/test/scala/org/apache/openwhisk/core/controller/test/PackagesApiTests.scala @@ -888,54 +888,54 @@ class PackagesApiTests extends ControllerTestCommon with WhiskPackagesApi { 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 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) + 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) - } + 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) - } + 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) + 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) - } + Get(s"/$namespace/${collection.path}/${provider.name}") ~> Route.seal(routes(auser)) ~> check { + status should be(Forbidden) + } } -} \ No newline at end of file +} From e11d2297611fde100c7f4d0f2df23c439a6fad6e Mon Sep 17 00:00:00 2001 From: Bhavesh Kemburu Date: Thu, 6 Aug 2020 13:00:46 -0400 Subject: [PATCH 7/8] Change Actions.scala code to handle specific unit test failures --- .../openwhisk/core/controller/Actions.scala | 97 +++++++++++-------- 1 file changed, 54 insertions(+), 43 deletions(-) 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 135290230b4..fb51b951a7b 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 @@ -355,51 +355,62 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with if (executeOnly && user.namespace.name != entityName.namespace) { terminate(Forbidden, forbiddenGetAction(entityName.path.asString)) } else { - val getEntityWhiskActionCase = - getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) - val getEntityWhiskMetaDataCase = - getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some { - action: WhiskActionMetaData => - val mergedAction = env map { - action inherit _ - } getOrElse action - complete(OK, mergedAction) - }) - if (code) { - if (entityName.path.defaultPackage) { - getEntityWhiskActionCase - } 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 { - getEntityWhiskActionCase - } + code match { + case true => + //Resolve Binding(Package) of the action + if (entityName.path.defaultPackage) { + getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { + action: WhiskAction => + val mergedAction = env map { + action inherit _ + } getOrElse action + complete(OK, mergedAction) }) - } - } else { - if (entityName.path.defaultPackage) { - getEntityWhiskMetaDataCase - } 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 { - getEntityWhiskMetaDataCase - } + } 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) + }) + } + }) + } } } } From 3b0338ae53e7008a676cfa72e26cd6c49a802220 Mon Sep 17 00:00:00 2001 From: Bhavesh Kemburu Date: Mon, 10 Aug 2020 15:54:55 -0400 Subject: [PATCH 8/8] Improved logic of fetching actions --- .../apache/openwhisk/http/ErrorResponse.scala | 2 +- .../openwhisk/core/controller/Actions.scala | 103 ++++++++---------- .../openwhisk/core/controller/Packages.scala | 13 +-- 3 files changed, 48 insertions(+), 70 deletions(-) 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 index 9dc841a9a04..5424dd309f7 100644 --- a/common/scala/src/main/scala/org/apache/openwhisk/http/ErrorResponse.scala +++ b/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) = 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 fb51b951a7b..89161c7ae96 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 @@ -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,25 +365,6 @@ 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 { @@ -393,25 +372,31 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with } 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) } } } diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala index ecfa10ccf20..ffc992f3235 100644 --- a/core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala +++ b/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] @@ -162,8 +160,6 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities { * - 404 Not Found * - 500 Internal Server Error */ - //get method that also checks for execute only to deny access to shared package, but package binding is handled - //within the mergePackageWithBinding() method override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])( implicit transid: TransactionId) = { if (executeOnly && user.namespace.name != entityName.namespace) { @@ -321,10 +317,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 {