Review feedback - Switch to 422 response code and error during permis…

…sions check
apache · Nov 28, 2018 · 5db5977 · 5db5977
1 parent f6ce8c7
commit 5db5977
Show file tree

Hide file tree

Showing 3 changed files with 11 additions and 5 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
@@ -112,6 +112,7 @@ object Messages {
   val requestedBindingIsNotValid = "Cannot bind to a resource that is not a package."
   val notAllowedOnBinding = "Operation not permitted on package binding."
   def packageNameIsReserved(name: String) = s"Package name '$name' is reserved."
+  def packageBindingCircularReference(name: String) = s"Package binding '$name' contains a circular reference."
 
   /** Error messages for triggers */
   def triggerWithInactiveRule(rule: String, action: String) = {

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
@@ -242,7 +242,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
     val validateBinding = content.binding map { binding =>
       wp.binding map {
         // pre-existing entity is a binding, check that new binding is valid
-        b =>
+        _ =>
           checkBinding(binding.fullyQualifiedName)
       } getOrElse {
         // pre-existing entity is a package, cannot make it a binding
@@ -301,7 +301,7 @@ trait WhiskPackagesApi extends WhiskCollectionAPI with ReferencedEntities {
         logging.debug(this, s"fetching package '$docid' for reference")
         if (docid == wp.docid) {
           logging.error(this, "unexpected package binding refers to itself: $docid")
-          terminate(InternalServerError)
+          terminate(UnprocessableEntity, Messages.packageBindingCircularReference(b.fullyQualifiedName.toString))
         } else {
           getEntity(WhiskPackage.get(entityStore, docid), Some {
             mergePackageWithBinding(Some { wp }) _

diff --git a/core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/PackageCollection.scala b/core/controller/src/main/scala/org/apache/openwhisk/core/entitlement/PackageCollection.scala
@@ -98,10 +98,15 @@ class PackageCollection(entityStore: EntityStore)(implicit logging: Logging) ext
           val pkgOwner = namespaces.contains(binding.namespace.asString)
           val pkgDocid = binding.docid
           logging.debug(this, s"checking subject has privilege '$right' for bound package '$pkgDocid'")
-          if (doc == pkgDocid)
-            Future.successful(true)
-          else
+          if (doc == pkgDocid) {
+            logging.error(this, "unexpected package binding refers to itself: $docid")
+            Future.failed(
+              RejectRequest(
+                UnprocessableEntity,
+                Messages.packageBindingCircularReference(binding.fullyQualifiedName.toString)))
+          } else {
             checkPackageReadPermission(namespaces, pkgOwner, pkgDocid)
+          }
         } else {
           logging.debug(this, s"entitlement check on package binding, '$right' allowed?: false")
           Future.successful(false)