Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions common/scala/src/main/resources/application.conf
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,7 @@ kamon {
}

whisk {
shared-packages-execute-only = false
metrics {
# Enable/disable Prometheus support. If enabled then metrics would be exposed at `/metrics` endpoint
# If Prometheus is enabled then please review `kamon.metric.tick-interval` (set to 1 sec by default above).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,7 @@ object ConfigKeys {
val featureFlags = "whisk.feature-flags"

val whiskConfig = "whisk.config"
val sharedPackageExecuteOnly = s"whisk.shared-packages-execute-only"
val swaggerUi = "whisk.swagger-ui"

val disableStoreResult = s"$activation.disable-store-result"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Comment thread
bkemburu marked this conversation as resolved.
Outdated
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. */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,8 @@ import org.apache.openwhisk.http.Messages._
import org.apache.openwhisk.core.entitlement.Resource
import org.apache.openwhisk.core.entitlement.Collection
import org.apache.openwhisk.core.loadBalancer.LoadBalancerException
import pureconfig._
import org.apache.openwhisk.core.ConfigKeys

/**
* A singleton object which defines the properties that must be present in a configuration
Expand Down Expand Up @@ -102,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 executeOnly =
Try({
loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
}).getOrElse(false)
Comment thread
bkemburu marked this conversation as resolved.
Outdated

/** Entity normalizer to JSON object. */
import RestApiCommons.emptyEntityToJsObject

Expand Down Expand Up @@ -341,22 +349,69 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
implicit transid: TransactionId) = {
parameter('code ? true) { code =>
code match {
case true =>
getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction =>
val mergedAction = env map {
action inherit _
} getOrElse action
complete(OK, mergedAction)
})
case false =>
getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
action: WhiskActionMetaData =>
val mergedAction = env map {
action inherit _
} getOrElse action
complete(OK, mergedAction)
})
//check if execute only is enabled, and if there is a discrepancy between the current user's namespace
//and that of the entity we are trying to fetch

if (executeOnly && user.namespace.name != entityName.namespace) {
terminate(Forbidden, forbiddenGetAction(entityName.path.asString))
} else {
code match {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I'd suggest the simpler/cleaner if (code) { } else { }.

case true =>
//Resolve Binding(Package) of the action
if (entityName.path.defaultPackage) {
getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
Comment thread
bkemburu marked this conversation as resolved.
action: WhiskAction =>
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) {
Comment thread
bkemburu marked this conversation as resolved.
Outdated
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)
})
}
})
}
}
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,11 @@
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.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}
Expand All @@ -33,6 +31,9 @@ import org.apache.openwhisk.core.entity._
import org.apache.openwhisk.core.entity.types.EntityStore
import org.apache.openwhisk.http.ErrorResponse.terminate
import org.apache.openwhisk.http.Messages
import org.apache.openwhisk.http.Messages._
import pureconfig._
import org.apache.openwhisk.core.ConfigKeys

trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
services: WhiskServices =>
Expand All @@ -42,6 +43,12 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
/** Database service to CRUD packages. */
protected val entityStore: EntityStore

/** Config flag for Execute Only for Shared Packages */
protected def executeOnly =
Try({
loadConfigOrThrow[Boolean](ConfigKeys.sharedPackageExecuteOnly)
}).getOrElse(false)
Comment thread
bkemburu marked this conversation as resolved.
Outdated

/** Notification service for cache invalidation. */
protected implicit val cacheChangeNotification: Some[CacheChangeNotification]

Expand Down Expand Up @@ -155,9 +162,18 @@ 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
Comment thread
bkemburu marked this conversation as resolved.
Outdated
override def fetch(user: Identity, entityName: FullyQualifiedEntityName, env: Option[Parameters])(
implicit transid: TransactionId) = {
getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some { mergePackageWithBinding() _ })
if (executeOnly && user.namespace.name != entityName.namespace) {
val value = entityName.toString
terminate(Forbidden, forbiddenGetPackage(entityName.asString))
} else {
getEntity(WhiskPackage.get(entityStore, entityName.toDocId), Some {
mergePackageWithBinding() _
})
}
}

/**
Expand Down Expand Up @@ -303,9 +319,20 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
logging.error(this, s"unexpected package binding refers to itself: $docid")
terminate(UnprocessableEntity, Messages.packageBindingCircularReference(b.fullyQualifiedName.toString))
} else {
getEntity(WhiskPackage.get(entityStore, docid), Some {
mergePackageWithBinding(Some { wp }) _
})

/** Here's where I check package execute only case with package binding. */
val packagePath = wp.namespace.asString
val bindingPath = wp.binding.iterator.next().toString
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is wp.binding.iterator guaranteed to not be empty?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, because this code falls within a binding case so binding has to hold some value.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah not enough context shown in the diff; then I think you can replace wp.binding.iterator.next().toString with b.toString to make this more clear

val indexOfSlash = bindingPath.indexOf('/')
if (executeOnly && packagePath != bindingPath.take(indexOfSlash)) {
terminate(Forbidden, forbiddenGetPackageBinding(wp.name.asString))
} else {
getEntity(WhiskPackage.get(entityStore, docid), Some {
mergePackageWithBinding(Some {
wp
}) _
})
}
}
} getOrElse {
val pkg = ref map { _ inherit wp.parameters } getOrElse wp
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
}