[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172628041
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -719,7 +741,8 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
private def confirmExportedAction(actionLookup: Future[WhiskActionMetaData],
authenticated: Boolean)(
implicit transid: TransactionId): Future[WhiskActionMetaData] = {
actionLookup flatMap { action =>
- val requiresAuthenticatedUser =
action.annotations.getAs[Boolean]("require-whisk-auth").exists(identity)
+ val requiresAuthenticatedUser =
+
action.annotations.getAs[Boolean](WhiskAction.requireWhiskAuthAnnotation).exists(identity)
Review comment:
let's discuss after the refactoring into a separate method.. @markusthoemmes
wanted to be sure we minimized the deserialization of the json values.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action URL: https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172507692 ## File path: common/scala/src/main/scala/whisk/core/entity/WhiskAction.scala ## @@ -313,6 +313,9 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[ override val cacheEnabled = true + val requireWhiskAuthAnnotation = "require-whisk-auth" Review comment: since a web action is actually a "kind" of `WhiskAction`, and since the annotation is set within the context of an action create/update, imho it makes sense to define all action annotation constants as part of `WhiskAction` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172513170
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,29 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+// If the require-whisk-auth annotation is either an
integer or a string, secure the web action by enforcing
+// require-whisk-auth annotation value == request header
x-require-whisk-auth value
+// If the require-whisk-auth annotation is a boolean, skip
the request header x-require-whisk-auth check
+val requireWhiskHeaderAuthenticationFailed =
action.annotations
+ .get(WhiskAction.requireWhiskAuthAnnotation)
+ .flatMap {
+case JsString(authStr) => Some(authStr)
+case JsNumber(authNum) => Some(authNum.toInt.toString)
Review comment:
i just played a bit with the BigDecimal data type, and i think that `toInt`
should be removed to support any json number, not just integers.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action URL: https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172507692 ## File path: common/scala/src/main/scala/whisk/core/entity/WhiskAction.scala ## @@ -313,6 +313,9 @@ object WhiskAction extends DocumentFactory[WhiskAction] with WhiskEntityQueries[ override val cacheEnabled = true + val requireWhiskAuthAnnotation = "require-whisk-auth" Review comment: since a web action is actually a "kind" of `WhiskAction, and since the annotation is set within the context of an action create/update, imho it makes sense to define all action annotation constants as part of `WhiskAction` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172235982
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,24 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+val requireWebAuthIsBool =
(action.annotations.getAs[Boolean]("require-whisk-auth") != None)
+val annotationRequireWebAuthIsIntOrString =
((action.annotations
+ .getAs[String]("require-whisk-auth") != None) ||
(action.annotations
+ .getAs[Int]("require-whisk-auth") != None))
+val annotationRequireWebAuth = (action.annotations
+ .getAs[Int]("require-whisk-auth")
+
.getOrElse(action.annotations.getAs[String]("require-whisk-auth").getOrElse("")))
+ .toString
+val enforceWhiskAuthHdr = (!requireWebAuthIsBool &&
annotationRequireWebAuthIsIntOrString)
+val headerWhiskAuthSeq =
context.headers.filter(_.lowercaseName == "x-require-whisk-auth")
Review comment:
yes, that works much better. thanks!
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172191333
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,24 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+val requireWebAuthIsBool =
(action.annotations.getAs[Boolean]("require-whisk-auth") != None)
Review comment:
@markusthoemmes that is correct. i've updated the description to make this
more clear.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172189903
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,24 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+val requireWebAuthIsBool =
(action.annotations.getAs[Boolean]("require-whisk-auth") != None)
+val annotationRequireWebAuthIsIntOrString =
((action.annotations
+ .getAs[String]("require-whisk-auth") != None) ||
(action.annotations
+ .getAs[Int]("require-whisk-auth") != None))
+val annotationRequireWebAuth = (action.annotations
+ .getAs[Int]("require-whisk-auth")
+
.getOrElse(action.annotations.getAs[String]("require-whisk-auth").getOrElse("")))
+ .toString
Review comment:
i like it :+1:
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] mdeuser commented on a change in pull request #3388: Update require-whisk-auth behavior to secure web action
mdeuser commented on a change in pull request #3388: Update require-whisk-auth
behavior to secure web action
URL:
https://github.com/apache/incubator-openwhisk/pull/3388#discussion_r172078120
##
File path:
core/controller/src/main/scala/whisk/core/controller/WebActions.scala
##
@@ -483,7 +483,24 @@ trait WhiskWebActionsApi extends Directives with
ValidateRequestSize with PostAc
provide(fullyQualifiedActionName(actionName)) { fullActionName =>
onComplete(verifyWebAction(fullActionName,
onBehalfOf.isDefined)) {
case Success((actionOwnerIdentity, action)) =>
-if
(!action.annotations.getAs[Boolean]("web-custom-options").exists(identity)) {
+val requireWebAuthIsBool =
(action.annotations.getAs[Boolean]("require-whisk-auth") != None)
Review comment:
@rabbah - i think `isTruthy` would return true for a "true" boolean value,
non-zero integers and non-empty strings; whereas this check is just determining
if the web action is configured with a boolean (true or false) to determine if
the existing `require-whisk-auth` behavior should be enforced.
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
