tests and tweaks

broadinstitute · Oct 26, 2020 · f888018 · f888018
1 parent 0882adb
commit f888018
Show file tree

Hide file tree

Showing 7 changed files with 116 additions and 7 deletions.
diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml
@@ -1659,8 +1659,14 @@ paths:
           description: Successfully created resource
           content: { }
         400:
-          description: Invalid policies, invalid auth domain, or resource type allows
-            id reuse
+          description: Invalid policies, invalid auth domain, parent specified but not allowed, or resource type allows
+            auth domains and id reuse
+          content:
+            application/json:
+              schema:
+                $ref: '#/components/schemas/ErrorReport'
+        403:
+          description: Parent does not exist or user does not have add_child permission on parent
           content:
             application/json:
               schema:

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutes.scala
@@ -247,7 +247,7 @@ trait ResourceRoutes extends UserInfoDirectives with SecurityDirectives with Sam
   def postResource(resourceType: ResourceType, userInfo: UserInfo, samRequestContext: SamRequestContext): server.Route =
     post {
       entity(as[CreateResourceRequest]) { createResourceRequest =>
-        requireCreateWithParent(createResourceRequest.parent, resourceType, userInfo.userId, samRequestContext) {
+        requireCreateWithOptionalParent(createResourceRequest.parent, resourceType, userInfo.userId, samRequestContext) {
           if (resourceType.reuseIds && resourceType.isAuthDomainConstrainable) {
             throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, "this api may not be used for resource types that allow both authorization domains and id reuse"))
           }

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SecurityDirectives.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SecurityDirectives.scala
@@ -51,7 +51,7 @@ trait SecurityDirectives {
       }
     }
 
-  def requireCreateWithParent(maybeParent: Option[FullyQualifiedResourceId], resourceType: ResourceType, userId: WorkbenchUserId, samRequestContext: SamRequestContext): Directive0 = {
+  def   requireCreateWithOptionalParent(maybeParent: Option[FullyQualifiedResourceId], resourceType: ResourceType, userId: WorkbenchUserId, samRequestContext: SamRequestContext): Directive0 = {
     maybeParent match {
       case None => Directives.pass // no parent specified, proceed
       case Some(parent) =>

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/openam/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/openam/PostgresAccessPolicyDAO.scala
@@ -225,7 +225,7 @@ class PostgresAccessPolicyDAO(protected val dbRef: DbReference,
         // note that when setting the parent we are not checking for circular hierarchies but that should be ok
         // since this is a new resource and should not be a parent of another so no circles can be possible
         samsql"""insert into ${ResourceTable.table} (${resourceTableColumn.name}, ${resourceTableColumn.resourceTypeId}, ${resourceTableColumn.resourceParentId})
-               value (${resource.resourceId}, (${loadResourceTypePK(resource.resourceTypeName)}), $parentPK)"""
+               values (${resource.resourceId}, (${loadResourceTypePK(resource.resourceTypeName)}), $parentPK)"""
     }
 
     Try {

diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV2Spec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/ResourceRoutesV2Spec.scala
@@ -130,7 +130,7 @@ class ResourceRoutesV2Spec extends FlatSpec with Matchers with TestSupport with
     }
   }
 
-  "POST /api/resources/v2/{resourceType} with createResource = true" should "201 create resource with content" in {
+  "POST /api/resources/v2/{resourceType} with returnResource = true" should "201 create resource with content" in {
     val resourceType = ResourceType(ResourceTypeName("rt"), Set(ResourceActionPattern("run", "", false)), Set(ResourceRole(ResourceRoleName("owner"), Set(ResourceAction("run")))), ResourceRoleName("owner"))
     val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType))
 
@@ -150,7 +150,7 @@ class ResourceRoutesV2Spec extends FlatSpec with Matchers with TestSupport with
     }
   }
 
-  "POST /api/resources/v2/{resourceType} with createResource = false" should "204 create resource with content" in {
+  "POST /api/resources/v2/{resourceType} with returnResource = false" should "204 create resource with content" in {
     val resourceType = ResourceType(ResourceTypeName("rt"), Set(ResourceActionPattern("run", "", false)), Set(ResourceRole(ResourceRoleName("owner"), Set(ResourceAction("run")))), ResourceRoleName("owner"))
     val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType))
 
@@ -160,6 +160,61 @@ class ResourceRoutesV2Spec extends FlatSpec with Matchers with TestSupport with
     }
   }
 
+  it should "204 create resource with content with parent" in {
+    val resourceType = ResourceType(ResourceTypeName("rt"), Set(ResourceActionPattern(SamResourceActions.setParent.value, "", false)), Set(ResourceRole(ResourceRoleName("owner"), Set(SamResourceActions.setParent, SamResourceActions.addChild))), ResourceRoleName("owner"))
+    val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType))
+
+    val createParentResourceRequest = CreateResourceRequest(ResourceId("parent"), Map(AccessPolicyName("goober") -> AccessPolicyMembership(Set(defaultUserInfo.userEmail), Set.empty, Set(resourceType.ownerRoleName))), Set.empty, Some(false))
+    Post(s"/api/resources/v2/${resourceType.name}", createParentResourceRequest) ~> samRoutes.route ~> check {
+      status shouldEqual StatusCodes.NoContent
+    }
+
+    val createResourceRequest = CreateResourceRequest(ResourceId("foo"), Map(AccessPolicyName("goober") -> AccessPolicyMembership(Set(defaultUserInfo.userEmail), Set.empty, Set(resourceType.ownerRoleName))), Set.empty, Some(false), Some(FullyQualifiedResourceId(resourceType.name, createParentResourceRequest.resourceId)))
+    Post(s"/api/resources/v2/${resourceType.name}", createResourceRequest) ~> samRoutes.route ~> check {
+      status shouldEqual StatusCodes.NoContent
+    }
+  }
+
+  it should "400 with parent when parents not allowed" in {
+    val resourceType = ResourceType(ResourceTypeName("rt"), Set(ResourceActionPattern(SamResourceActions.setParent.value, "", false)), Set(ResourceRole(ResourceRoleName("owner"), Set(SamResourceActions.addChild))), ResourceRoleName("owner"))
+    val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType))
+
+    val createParentResourceRequest = CreateResourceRequest(ResourceId("parent"), Map(AccessPolicyName("goober") -> AccessPolicyMembership(Set(defaultUserInfo.userEmail), Set.empty, Set(resourceType.ownerRoleName))), Set.empty, Some(false))
+    Post(s"/api/resources/v2/${resourceType.name}", createParentResourceRequest) ~> samRoutes.route ~> check {
+      status shouldEqual StatusCodes.NoContent
+    }
+
+    val createResourceRequest = CreateResourceRequest(ResourceId("foo"), Map(AccessPolicyName("goober") -> AccessPolicyMembership(Set(defaultUserInfo.userEmail), Set.empty, Set(resourceType.ownerRoleName))), Set.empty, Some(false), Some(FullyQualifiedResourceId(resourceType.name, createParentResourceRequest.resourceId)))
+    Post(s"/api/resources/v2/${resourceType.name}", createResourceRequest) ~> samRoutes.route ~> check {
+      status shouldEqual StatusCodes.BadRequest
+    }
+  }
+
+  it should "403 with parent when add_child not allowed on parent" in {
+    val resourceType = ResourceType(ResourceTypeName("rt"), Set(ResourceActionPattern(SamResourceActions.setParent.value, "", false)), Set(ResourceRole(ResourceRoleName("owner"), Set(SamResourceActions.setParent))), ResourceRoleName("owner"))
+    val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType))
+
+    val createParentResourceRequest = CreateResourceRequest(ResourceId("parent"), Map(AccessPolicyName("goober") -> AccessPolicyMembership(Set(defaultUserInfo.userEmail), Set.empty, Set(resourceType.ownerRoleName))), Set.empty, Some(false))
+    Post(s"/api/resources/v2/${resourceType.name}", createParentResourceRequest) ~> samRoutes.route ~> check {
+      status shouldEqual StatusCodes.NoContent
+    }
+
+    val createResourceRequest = CreateResourceRequest(ResourceId("foo"), Map(AccessPolicyName("goober") -> AccessPolicyMembership(Set(defaultUserInfo.userEmail), Set.empty, Set(resourceType.ownerRoleName))), Set.empty, Some(false), Some(FullyQualifiedResourceId(resourceType.name, createParentResourceRequest.resourceId)))
+    Post(s"/api/resources/v2/${resourceType.name}", createResourceRequest) ~> samRoutes.route ~> check {
+      status shouldEqual StatusCodes.Forbidden
+    }
+  }
+
+  it should "403 with parent when parent does not exist" in {
+    val resourceType = ResourceType(ResourceTypeName("rt"), Set(ResourceActionPattern(SamResourceActions.setParent.value, "", false)), Set(ResourceRole(ResourceRoleName("owner"), Set(SamResourceActions.setParent))), ResourceRoleName("owner"))
+    val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType))
+
+    val createResourceRequest = CreateResourceRequest(ResourceId("foo"), Map(AccessPolicyName("goober") -> AccessPolicyMembership(Set(defaultUserInfo.userEmail), Set.empty, Set(resourceType.ownerRoleName))), Set.empty, Some(false), Some(FullyQualifiedResourceId(resourceType.name, ResourceId("parent"))))
+    Post(s"/api/resources/v2/${resourceType.name}", createResourceRequest) ~> samRoutes.route ~> check {
+      status shouldEqual StatusCodes.Forbidden
+    }
+  }
+
   it should "204 when valid auth domain is provided and the resource type is constrainable" in {
     val resourceType = ResourceType(ResourceTypeName("rt"), Set(ResourceActionPattern("run", "", true)), Set(ResourceRole(ResourceRoleName("owner"), Set(ResourceAction("run")))), ResourceRoleName("owner"))
     val samRoutes = TestSamRoutes(Map(resourceType.name -> resourceType, managedGroupResourceType.name -> managedGroupResourceType))

diff --git a/...test/scala/org/broadinstitute/dsde/workbench/sam/openam/PostgresAccessPolicyDAOSpec.scala b/...test/scala/org/broadinstitute/dsde/workbench/sam/openam/PostgresAccessPolicyDAOSpec.scala
@@ -290,6 +290,22 @@ class PostgresAccessPolicyDAOSpec extends FreeSpec with Matchers with BeforeAndA
 
         exception.getSQLState shouldEqual PSQLStateExtensions.NULL_CONSTRAINT_VIOLATION
       }
+
+      "creates resource with parent" in {
+        val child = Resource(resourceType.name, ResourceId("child"), Set.empty, parent = Option(resource.fullyQualifiedId))
+        dao.createResourceType(resourceType, samRequestContext).unsafeRunSync()
+        dao.createResource(resource, samRequestContext).unsafeRunSync() shouldEqual resource
+        dao.createResource(child, samRequestContext).unsafeRunSync() shouldEqual child
+        dao.getResourceParent(child.fullyQualifiedId, samRequestContext).unsafeRunSync() shouldBe Option(resource.fullyQualifiedId)
+      }
+
+      "raises error when parent does not exist" in {
+        val child = Resource(resourceType.name, ResourceId("child"), Set.empty, parent = Option(resource.fullyQualifiedId))
+        dao.createResourceType(resourceType, samRequestContext).unsafeRunSync()
+        val exception = intercept[WorkbenchException] {
+          dao.createResource(child, samRequestContext).unsafeRunSync()
+        }
+      }
     }
 
     "loadResourceAuthDomain" - {
@@ -1247,6 +1263,20 @@ class PostgresAccessPolicyDAOSpec extends FreeSpec with Matchers with BeforeAndA
 
         exception.errorReport.statusCode shouldBe Option(StatusCodes.BadRequest)
       }
+
+      "errors if parent does not exist" in {
+        val resource = Resource(resourceType.name, ResourceId("resource"), Set.empty)
+        val doesNotExist = Resource(resourceType.name, ResourceId("doesNotExist"), Set.empty)
+        val testResult = for {
+          _ <- dao.createResourceType(resourceType, samRequestContext)
+          _ <- dao.createResource(resource, samRequestContext)
+          _ <- dao.setResourceParent(resource.fullyQualifiedId, doesNotExist.fullyQualifiedId, samRequestContext)
+        } yield ()
+
+        val exception = intercept[WorkbenchException] {
+          testResult.unsafeRunSync()
+        }
+      }
     }
 
     "deleteResourceParent" - {

diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala
@@ -343,6 +343,24 @@ class ResourceServiceSpec extends FlatSpec with Matchers with ScalaFutures with
     exception2.errorReport.statusCode shouldEqual Option(StatusCodes.BadRequest)
   }
 
+  it should "create ownerless resource with parent" in {
+    val ownerRoleName = ResourceRoleName("owner")
+    val resourceType = ResourceType(ResourceTypeName(UUID.randomUUID().toString), Set.empty, Set(ResourceRole(ownerRoleName, Set.empty)), ownerRoleName)
+    val parentResourceName = ResourceId("parent")
+    val childResourceName = ResourceId("child")
+
+    val test = for {
+      _ <- service.createResourceType(resourceType, samRequestContext)
+      parent <- service.createResource(resourceType, parentResourceName, dummyUserInfo, samRequestContext)
+      child <- service.createResource(resourceType, childResourceName, Map.empty, Set.empty, Option(parent.fullyQualifiedId), dummyUserInfo.userId, samRequestContext)
+      actualParent <- policyDAO.getResourceParent(child.fullyQualifiedId, samRequestContext)
+    } yield {
+      actualParent shouldBe Option(parent.fullyQualifiedId)
+    }
+
+    test.unsafeRunSync()
+  }
+
   private def assertResourceExists(
       resource: FullyQualifiedResourceId, resourceType: ResourceType, policyDao: AccessPolicyDAO) = {
     val resultingPolicies = policyDao.listAccessPolicies(resource, samRequestContext).unsafeRunSync().map(_.copy(email = WorkbenchEmail("policy-randomuuid@example.com")))