You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@openwhisk.apache.org by GitBox <gi...@apache.org> on 2020/07/24 14:34:48 UTC
[GitHub] [openwhisk] bkemburu opened a new pull request #4935: Kemburu/execute only backup
bkemburu opened a new pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935
Execute Only For Shared Actions/Packages
Shared packages/bindings of shared packages and actions within shared packages/bindings. currently allows both READ and EXECUTE operations for all users of the system. The config option supports EXECUTE-only on shared packages such that the actions within the package are able to be EXECUTED but not READ by users who do not own the namespace. I wrote tests in the PackageApiTests and PackageActionApiTests that verify the functionality of EXECUTE-only
## Description
<!--- Provide a detailed description of your changes. -->
<!--- Include details of what problem you are solving and how your changes are tested. -->
## Related issue and scope
<!--- Please include a link to a related issue if there is one. -->
- [ ] I opened an issue to propose and discuss this change (#????)
## My changes affect the following components
<!--- Select below all system components are affected by your change. -->
<!--- Enter an `x` in all applicable boxes. -->
- [ ] API
- Controller
- [ ] Message Bus (e.g., Kafka)
- [ ] Loadbalancer
- [ ] Invoker
- [ ] Intrinsic actions (e.g., sequences, conductors)
- [ ] Data stores (e.g., CouchDB)
- Tests
- [ ] Deployment
- CLI
- [ ] General tooling
- Documentation
## Types of changes
<!--- What types of changes does your code introduce? Use `x` in all the boxes that apply: -->
- [ ] Bug fix (generally a non-breaking change which closes an issue).
- Enhancement or new feature (adds new functionality).
- [ ] Breaking change (a bug fix or enhancement which changes existing behavior).
## Checklist:
<!--- Please review the points below which help you make sure you've covered all aspects of the change you're making. -->
- I signed an [Apache CLA](https://github.com/apache/openwhisk/blob/master/CONTRIBUTING.md).
- I reviewed the [style guides](https://github.com/apache/openwhisk/wiki/Contributing:-Git-guidelines#code-readiness) and followed the recommendations (Travis CI will check :).
- I added tests to cover my changes.
- [ ] My changes require further changes to the documentation.
- I updated the documentation where necessary.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] style95 commented on pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
style95 commented on pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#issuecomment-664855498
@bkemburu Thank you for the contribution.
This is a good feature and I have wanted this.
But I hope we can generalize this issue into a permission management.
There are some discussions in the existing thread about Unix-like permission.
https://github.com/apache/openwhisk/issues/3629
https://github.com/apache/openwhisk/pull/4058
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] rabbah commented on pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
rabbah commented on pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#issuecomment-682027713
thanks for the contribution @bkemburu 🎉
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bkemburu commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
bkemburu commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r466556037
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -341,26 +350,58 @@ 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.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 {
Review comment:
Code originally had code match structure so I didn't modify this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r468942130
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -338,6 +336,56 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
}
+ /**GET --save which retrieves all source code for an entity **/
+ private def getEntityWithSourceCode(entityName: FullyQualifiedEntityName, env: Option[Parameters])(
+ implicit transid: TransactionId) = {
+ getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction =>
+ val mergedAction = env map {
+ action inherit _
+ } getOrElse action
+ complete(OK, mergedAction)
+ })
+ }
+
+ /**Standard GET which just retrieves metadata**/
+ private def getEntityMetaData(entityName: FullyQualifiedEntityName, env: Option[Parameters])(
+ implicit transid: TransactionId) = {
+ getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
+ action: WhiskActionMetaData =>
+ val mergedAction = env map {
+ action inherit _
+ } getOrElse action
+ complete(OK, mergedAction)
+ })
+ }
+
+ /** 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) = {
+ if (entityName.path.defaultPackage) {
+ if (code) {
+ getEntityWithSourceCode(entityName, env)
+ } else {
+ getEntityMetaData(entityName, env)
+ }
+ } else {
+ getEntity(
+ WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true),
Review comment:
is there a test for when a bound package goes missing? e.g.
* create a package
* create a shared package binding
* create an action in the package
* delete the package (not the binding)
* get the action via the binding (should fail gracefully with whatever the same error is when getting the package binding when the original package is now gone)
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467296713
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -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 {
+ 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 {
+ getEntity(
+ WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true),
+ Some { pkg: WhiskPackage =>
+ val originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace
+ if (executeOnly && originalPackageLocation != entityName.namespace) {
Review comment:
same - can you DRY this if line up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467307656
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala
##########
@@ -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
Review comment:
is `wp.binding.iterator` guaranteed to not be empty?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r468936217
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala
##########
@@ -155,9 +160,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
Review comment:
remove extra comments, or put them in the javadocs
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467294537
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -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)
Review comment:
Generally the convention so far is to put all defaults into applicaiton.conf file. So I would a) add a default value (false) for `whisk.shared-packages-execute-only` into application.conf, and b) remove the `Try` here since it should always work.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467979681
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala
##########
@@ -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
Review comment:
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
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467296404
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -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 {
+ case true =>
+ //Resolve Binding(Package) of the action
+ if (entityName.path.defaultPackage) {
+ getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
Review comment:
i think this logic is repeated a few times - can you DRY it up?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r468927539
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -338,6 +336,56 @@ trait WhiskActionsApi extends WhiskCollectionAPI with PostActionActivation with
deleteEntity(WhiskAction, entityStore, entityName.toDocId, (a: WhiskAction) => Future.successful({}))
}
+ /**GET --save which retrieves all source code for an entity **/
+ private def getEntityWithSourceCode(entityName: FullyQualifiedEntityName, env: Option[Parameters])(
+ implicit transid: TransactionId) = {
+ getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some { action: WhiskAction =>
+ val mergedAction = env map {
+ action inherit _
+ } getOrElse action
+ complete(OK, mergedAction)
+ })
+ }
+
+ /**Standard GET which just retrieves metadata**/
+ private def getEntityMetaData(entityName: FullyQualifiedEntityName, env: Option[Parameters])(
+ implicit transid: TransactionId) = {
+ getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
+ action: WhiskActionMetaData =>
+ val mergedAction = env map {
+ action inherit _
+ } getOrElse action
+ complete(OK, mergedAction)
+ })
+ }
+
+ /** 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) = {
+ if (entityName.path.defaultPackage) {
+ if (code) {
+ getEntityWithSourceCode(entityName, env)
+ } else {
+ getEntityMetaData(entityName, env)
+ }
+ } 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 {
+ if (code) {
+ getEntityWithSourceCode(entityName, env)
+ } else {
+ getEntityMetaData(entityName, env)
+ }
+ }
+ })
+ }
+ }
+
Review comment:
here's an example that condenses this a bit using `Either` to use either entityName(if package is authorized) or forbidden message (if package is not authorized), and then processes the future (similar to `getEntity`, but not using `getEntity`):
```
/** Checks for package binding case. we don't want to allow get for a package binding in shared package */
private def fetchEntity(entityName: FullyQualifiedEntityName, env: Option[Parameters], code: Boolean)(
implicit transid: TransactionId) = {
val resolvedPkg: Future[Either[String, FullyQualifiedEntityName]] = if (entityName.path.defaultPackage) {
Future.successful(Right(entityName))
} else {
WhiskPackage.resolveBinding(entityStore, entityName.path.toDocId, mergeParameters = true).map { pkg =>
val originalPackageLocation = pkg.fullyQualifiedName(withVersion = false).namespace
if (executeOnly && originalPackageLocation != entityName.namespace) {
Left(forbiddenGetActionBinding(entityName.toDocId.asString))
} else {
Right(entityName)
}
}
}
onComplete(resolvedPkg) {
case Success(pkgFuture) =>
pkgFuture match {
case Left(f) => terminate(Forbidden, f)
case Right(_) =>
if (code) {
getEntity(WhiskAction.resolveActionAndMergeParameters(entityStore, entityName), Some {
action: WhiskAction =>
val mergedAction = env map {
action inherit _
} getOrElse action
complete(OK, mergedAction)
})
} else {
getEntity(WhiskActionMetaData.resolveActionAndMergeParameters(entityStore, entityName), Some {
action: WhiskActionMetaData =>
val mergedAction = env map {
action inherit _
} getOrElse action
complete(OK, mergedAction)
})
}
}
case Failure(t: Throwable) =>
logging.error(this, s"[GET] package ${entityName.path.toDocId} failed: ${t.getMessage}")
terminate(InternalServerError)
}
}
```
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris merged pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris merged pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467290121
##########
File path: 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."
Review comment:
Missing an "is" in this message?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] tysonnorris commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
tysonnorris commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467297052
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala
##########
@@ -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)
Review comment:
same as above - use application.conf for the defaults
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] rabbah commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
rabbah commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r461656865
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -341,26 +350,58 @@ 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.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")
Review comment:
Here, please import `StatusCodes.Forbidden` and use that instead.
Also please move the error message to `org.apache.openwhisk.http.Messages` which is where error responses are consolidated and to facilitate tests without replicating the string.
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -341,26 +350,58 @@ 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.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 {
Review comment:
Nit: I'd suggest the simpler/cleaner if (code) { } else { }.
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -341,26 +350,58 @@ 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.toString != entityName.namespace.toString) {
Review comment:
The toString conversion is is not necessary. `namespace.name` is of type `EntityName` and so is `entityName.namespace`. Since `EntityName` is a value type `!=` is sufficient. In any case when you want string conversion in the code base use `asString` instead of `toString` as this is the idiom used throughout.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bkemburu commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
bkemburu commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r467869753
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Packages.scala
##########
@@ -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
Review comment:
Yes, because this code falls within a binding case so binding has to hold some value.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [openwhisk] bkemburu commented on a change in pull request #4935: Execute Only for Shared Actions
Posted by GitBox <gi...@apache.org>.
bkemburu commented on a change in pull request #4935:
URL: https://github.com/apache/openwhisk/pull/4935#discussion_r466556037
##########
File path: core/controller/src/main/scala/org/apache/openwhisk/core/controller/Actions.scala
##########
@@ -341,26 +350,58 @@ 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.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 {
Review comment:
Code originally had code match structure so I didn't modify this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org