ID-907 Filter/List Resources Improvements (#1280)

broadinstitute · Jan 11, 2024 · c87d723 · c87d723
1 parent d3b2848
commit c87d723
Show file tree

Hide file tree

Showing 13 changed files with 422 additions and 162 deletions.
diff --git a/codegen_java_old/templates/libraries/okhttp-gson/build.sbt.mustache b/codegen_java_old/templates/libraries/okhttp-gson/build.sbt.mustache
@@ -21,7 +21,7 @@ lazy val root = (project in file(".")).
       "org.openapitools" % "jackson-databind-nullable" % "0.2.2",
       {{/openApiNullable}}
       {{#hasOAuthMethods}}
-      "org.apache.oltu.oauth2" % "org.apache.oltu.oauth2.client" % "1.0.1",
+      "org.apache.oltu.oauth2" % "org.apache.oltu.oauth2.client" % "1.0.2",
       {{/hasOAuthMethods}}
       {{#joda}}
       "joda-time" % "joda-time" % "2.9.9" % "compile",
@@ -36,7 +36,7 @@ lazy val root = (project in file(".")).
       "jakarta.annotation" % "jakarta.annotation-api" % "1.3.5" % "compile",
       "com.google.code.findbugs" % "jsr305" % "3.0.2" % "compile",
       "jakarta.annotation" % "jakarta.annotation-api" % "1.3.5" % "compile",
-      "junit" % "junit" % "4.13.1" % "test",
+      "org.junit.jupiter" % "junit-jupiter-api" % "5.9.1" % "test",
       "com.novocode" % "junit-interface" % "0.10" % "test",
       "javax.annotation" % "javax.annotation-api" % "1.3.2"
     )) ++ publishSettings:_*

diff --git a/render_config.sh b/render_config.sh
@@ -7,11 +7,11 @@ gcloud container clusters get-credentials --zone us-central1-a --project broad-d
 kubectl -n terra-dev get secret sam-sa-secret -o 'go-template={{index .data "sam-account.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/sam-account.json
 kubectl -n terra-dev get secret sam-sa-secret -o 'go-template={{index .data "sam-account.pem"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/sam-account.pem
 
-kubectl -n terra-dev get secret admin-sa-secret -o 'go-template={{index .data "admin-service-account-1.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-1.json
-kubectl -n terra-dev get secret admin-one-sa-secret -o 'go-template={{index .data "admin-service-account-2.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-2.json
-kubectl -n terra-dev get secret admin-two-sa-secret -o 'go-template={{index .data "admin-service-account-3.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-3.json
-kubectl -n terra-dev get secret admin-three-sa-secret -o 'go-template={{index .data "admin-service-account-4.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-4.json
-kubectl -n terra-dev get secret admin-four-sa-secret -o 'go-template={{index .data "admin-service-account-5.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-5.json
+kubectl -n terra-dev get secret admin-one-sa-secret -o 'go-template={{index .data "admin-service-account-1.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-1.json
+kubectl -n terra-dev get secret admin-two-sa-secret -o 'go-template={{index .data "admin-service-account-2.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-2.json
+kubectl -n terra-dev get secret admin-three-sa-secret -o 'go-template={{index .data "admin-service-account-3.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-3.json
+kubectl -n terra-dev get secret admin-four-sa-secret -o 'go-template={{index .data "admin-service-account-4.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-4.json
+kubectl -n terra-dev get secret admin-five-sa-secret -o 'go-template={{index .data "admin-service-account-5.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/admin-service-account-5.json
 
 kubectl -n terra-dev get configmap sam-oauth2-configmap -o 'go-template={{index .data "oauth2-config"}}' > ${SERVICE_OUTPUT_LOCATION}/oauth2.conf
 # Local dev uses a macOS-specific docker replacement hostname for locahost, so replace all instances in the proxy config.

diff --git a/scripts/gen_java_client.sh b/scripts/gen_java_client.sh
@@ -1,5 +1,5 @@
 set -e
 
-docker run --rm -v ${PWD}:/local openapitools/openapi-generator-cli:v7.1.0 generate -i /local/src/main/resources/swagger/api-docs.yaml -g java -o /local/codegen_java --api-package org.broadinstitute.dsde.workbench.client.sam.api --model-package org.broadinstitute.dsde.workbench.client.sam.model --template-dir /local/codegen_java/templates --library okhttp-gson --additional-properties useJakartaEe=true,disallowAdditionalPropertiesIfNotPresent=false
+docker run --rm -v ${PWD}:/local openapitools/openapi-generator-cli:v7.2.0 generate -i /local/src/main/resources/swagger/api-docs.yaml -g java -o /local/codegen_java --api-package org.broadinstitute.dsde.workbench.client.sam.api --model-package org.broadinstitute.dsde.workbench.client.sam.model --template-dir /local/codegen_java/templates --library okhttp-gson --additional-properties useJakartaEe=true,disallowAdditionalPropertiesIfNotPresent=false
 cd codegen_java
 sbt test
diff --git a/scripts/gen_java_client_old.sh b/scripts/gen_java_client_old.sh
@@ -1,5 +1,5 @@
 set -e
 
-docker run --rm -v ${PWD}:/local openapitools/openapi-generator-cli:v5.4.0 generate -i /local/src/main/resources/swagger/api-docs.yaml -g java -o /local/codegen_java_old --api-package org.broadinstitute.dsde.workbench.client.sam.api --model-package org.broadinstitute.dsde.workbench.client.sam.model --template-dir /local/codegen_java_old/templates --library okhttp-gson --additional-properties disallowAdditionalPropertiesIfNotPresent=false
+docker run --rm -v ${PWD}:/local openapitools/openapi-generator-cli:v7.2.0 generate -i /local/src/main/resources/swagger/api-docs.yaml -g java -o /local/codegen_java_old --api-package org.broadinstitute.dsde.workbench.client.sam.api --model-package org.broadinstitute.dsde.workbench.client.sam.model --template-dir /local/codegen_java_old/templates --library okhttp-gson --additional-properties disallowAdditionalPropertiesIfNotPresent=false
 cd codegen_java_old
 sbt test
diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml
@@ -1558,21 +1558,34 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/ErrorReport'
-  /api/resources/v2/filter:
+  /api/resources/v2:
     get:
       tags:
         - Resources
-      summary: Filter resources the calling user has access to.
-        This endpoint MUST NOT be used to determine if a user has an action on a resource. This endpoint is ONLY
-        for determining the state of the user's permissions. It does not take into account enabled status, or Terms of Service Compliance.
+      summary: List resources the calling user has been granted policies/roles/actions on.
+        This endpoint MUST NOT be used to determine if a user has an action on a resource. 
+        This endpoint is ONLY for determining the state of the user's permissions. 
+        It does not take into account enabled status, or Terms of Service Compliance, nor does it take into account Auth Domains.
         To check if a user is allowed to execute an action on a resource, use /api/resources/v2/{resourceTypeName}/{resourceId}/action/{action}
         By default, public resources are not included. Including public resources incurs a 3x cost of DB performance.
-      operationId: filterResourcesV3
+      operationId: listResourcesV2
       parameters:
+        - name: format
+          in: query
+          description: Format of the response. If not specified, the response will be
+            a hierarchical list of resources.
+          required: false
+          schema:
+            type: string
+            enum:
+              - flat
+              - hierarchical
+            default: hierarchical
         - name: resourceTypes
           in: query
           description: Types of resources to filter on
           required: false
+          explode: false
           schema:
             type: array
             items:
@@ -1581,6 +1594,7 @@ paths:
           in: query
           description: Names of policies to filter on
           required: false
+          explode: false
           schema:
             type: array
             items:
@@ -1589,6 +1603,7 @@ paths:
           in: query
           description: Names of roles to filter on
           required: false
+          explode: false
           schema:
             type: array
             items:
@@ -1597,6 +1612,7 @@ paths:
           in: query
           description: Names of actions to filter on
           required: false
+          explode: false
           schema:
             type: array
             items:
@@ -1613,9 +1629,9 @@ paths:
           content:
             application/json:
               schema:
-                type: array
-                items:
-                  $ref: '#/components/schemas/FilteredResourcesResponse'
+                oneOf:
+                  - $ref: '#/components/schemas/FilteredResourcesFlatResponse'
+                  - $ref: '#/components/schemas/FilteredResourcesHierarchicalResponse'
   /api/resources/v2/{resourceTypeName}:
     get:
       tags:
@@ -4329,14 +4345,37 @@ components:
           type: boolean
           description: Whether or not the user is enabled
       description: specification of a UpdateUserRequest
-    FilteredResourcesResponse:
+    FilteredResourceResponse:
       type: object
+      required:
+        - format
       properties:
-        resources:
-          type: array
-          items:
-            $ref: '#/components/schemas/FilteredResource'
-    FilteredResource:
+        format:
+          type: string
+      discriminator:
+        propertyName: format
+        mapping:
+          flat: FilteredResourcesFlatResponse
+          hierarchical: FilteredResourcesHierarchicalResponse
+    FilteredResourcesFlatResponse:
+      allOf:
+        - $ref: '#/components/schemas/FilteredResourceResponse'
+        - type: object
+          properties:
+            resources:
+              type: array
+              items:
+                $ref: '#/components/schemas/FilteredFlatResource'
+    FilteredResourcesHierarchicalResponse:
+      allOf:
+        - $ref: '#/components/schemas/FilteredResourceResponse'
+        - type: object
+          properties:
+            resources:
+              type: array
+              items:
+                $ref: '#/components/schemas/FilteredHierarchicalResource'
+    FilteredFlatResource:
       type: object
       properties:
         resourceType:
@@ -4357,6 +4396,50 @@ components:
         isPublic:
           type: string
           description: isPublic is the resource public or not
+    FilteredHierarchicalResource:
+      type: object
+      properties:
+        resourceType:
+          type: string
+          description: resourceType of the resource
+        resourceId:
+          type: string
+          description: resourceType of the resource
+        policies:
+          type: array
+          description: Policies on the resource
+          items:
+            $ref: '#/components/schemas/FilteredHierarchicalResourcePolicy'
+    FilteredHierarchicalResourcePolicy:
+      type: object
+      properties:
+        policy:
+          type: string
+          description: The policy name
+        roles:
+          type: array
+          description: Roles on the policy
+          items:
+            $ref: '#/components/schemas/FilteredHierarchicalResourceRole'
+        actions:
+          type: array
+          description: Actions on the role
+          items:
+            type: string
+        isPublic:
+          type: boolean
+          description: Is this policy on the resource a public policy?
+    FilteredHierarchicalResourceRole:
+      type: object
+      properties:
+        role:
+          type: string
+          description: The role name
+        actions:
+          type: array
+          description: Actions on the role
+          items:
+            type: string
   securitySchemes:
     googleoauth:
       type: oauth2

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
@@ -15,6 +15,8 @@ import org.broadinstitute.dsde.workbench.sam.model.RootPrimitiveJsonSupport._
 import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
 import org.broadinstitute.dsde.workbench.sam.model._
 import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, SamUser}
+import org.broadinstitute.dsde.workbench.sam.model.api.FilteredResourcesHierarchical._
+import org.broadinstitute.dsde.workbench.sam.model.api.FilteredResourcesFlat._
 import org.broadinstitute.dsde.workbench.sam.service.ResourceService
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 import spray.json.DefaultJsonProtocol._
@@ -124,11 +126,9 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM
         }
       } ~
       pathPrefix("resources" / "v2") {
-        pathPrefix("filter") {
-          pathEndOrSingleSlash {
-            get {
-              filterUserResources(samUser, samRequestContext)
-            }
+        pathEnd {
+          get {
+            listUserResources(samUser, samRequestContext)
           }
         } ~
         pathPrefix(Segment) { resourceTypeName =>
@@ -599,19 +599,44 @@ trait ResourceRoutes extends SamUserDirectives with SecurityDirectives with SamM
       }
     }
 
-  def filterUserResources(samUser: SamUser, samRequestContext: SamRequestContext): Route =
-    parameters("resourceTypes".as[String].?, "policies".as[String].?, "roles".as[String].?, "actions".as[String].?, "includePublic" ? false) {
-      (resourceTypes: Option[String], policies: Option[String], roles: Option[String], actions: Option[String], includePublic: Boolean) =>
-        complete(
-          resourceService.filterResources(
-            samUser,
-            resourceTypes.map(_.split(",").map(ResourceTypeName(_)).toSet).getOrElse(Set.empty),
-            policies.map(_.split(",").map(AccessPolicyName(_)).toSet).getOrElse(Set.empty),
-            roles.map(_.split(",").map(ResourceRoleName(_)).toSet).getOrElse(Set.empty),
-            actions.map(_.split(",").map(ResourceAction(_)).toSet).getOrElse(Set.empty),
-            includePublic,
-            samRequestContext
-          )
-        )
+  private def listUserResources(samUser: SamUser, samRequestContext: SamRequestContext): Route =
+    parameters(
+      "resourceTypes".as[String].?,
+      "policies".as[String].?,
+      "roles".as[String].?,
+      "actions".as[String].?,
+      "includePublic" ? false,
+      "format".as[String] ? "hierarchical"
+    ) { (resourceTypes: Option[String], policies: Option[String], roles: Option[String], actions: Option[String], includePublic: Boolean, format: String) =>
+      format match {
+        case "flat" =>
+          complete {
+            resourceService
+              .listResourcesFlat(
+                samUser,
+                resourceTypes.map(_.split(",").map(ResourceTypeName(_)).toSet).getOrElse(Set.empty),
+                policies.map(_.split(",").map(AccessPolicyName(_)).toSet).getOrElse(Set.empty),
+                roles.map(_.split(",").map(ResourceRoleName(_)).toSet).getOrElse(Set.empty),
+                actions.map(_.split(",").map(ResourceAction(_)).toSet).getOrElse(Set.empty),
+                includePublic,
+                samRequestContext
+              )
+              .map(StatusCodes.OK -> _)
+          }
+        case "hierarchical" =>
+          complete {
+            resourceService
+              .listResourcesHierarchical(
+                samUser,
+                resourceTypes.map(_.split(",").map(ResourceTypeName(_)).toSet).getOrElse(Set.empty),
+                policies.map(_.split(",").map(AccessPolicyName(_)).toSet).getOrElse(Set.empty),
+                roles.map(_.split(",").map(ResourceRoleName(_)).toSet).getOrElse(Set.empty),
+                actions.map(_.split(",").map(ResourceAction(_)).toSet).getOrElse(Set.empty),
+                includePublic,
+                samRequestContext
+              )
+              .map(StatusCodes.OK -> _)
+          }
+      }
     }
 }
diff --git a/...main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/...main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala
@@ -40,6 +40,7 @@ class PostgresAccessPolicyDAO(
     * serializable transactions would read all rows of the resource type table and thus always collide with each other).
     */
   private val resourceTypePKsByName: scala.collection.concurrent.Map[ResourceTypeName, ResourceTypePK] = new TrieMap()
+  private val resourceTypeNamesByPK: scala.collection.concurrent.Map[ResourceTypePK, ResourceTypeName] = new TrieMap()
 
   /** Creates or updates all given resource types.
     *
@@ -94,6 +95,9 @@ class PostgresAccessPolicyDAO(
       resourceTypePKsByName
         .addAll(results)
         .filterInPlace((k, v) => results.exists(loaded => (k, v) == loaded))
+      resourceTypeNamesByPK
+        .addAll(results.map(_.swap))
+        .filterInPlace((k, v) => results.map(_.swap).exists(loaded => (k, v) == loaded))
     }
 
   override def loadResourceTypes(resourceTypeNames: Set[ResourceTypeName], samRequestContext: SamRequestContext): IO[Set[ResourceType]] =
@@ -1701,15 +1705,12 @@ class PostgresAccessPolicyDAO(
     val resourceAction = ResourceActionTable.syntax("resourceAction")
     val resource = ResourceTable.syntax("resource")
 
-    val resourceTypePKsToName: Map[ResourceTypePK, ResourceTypeName] =
-      resourceTypeNames.flatMap(name => resourceTypePKsByName.get(name).map(pk => pk -> name)).toMap
-
     val resourceTypeConstraint =
-      if (resourceTypeNames.nonEmpty) samsqls"and ${resource.resourceTypeId} in  (${resourceTypeNames.flatMap(resourceTypePKsByName.get).toSet})"
-      else samsqls""
-    val policyConstraint = if (policies.nonEmpty) samsqls"and ${resourcePolicy.name} in (${policies.toSet})" else samsqls""
-    val roleConstraint = if (roles.nonEmpty) samsqls"and ${resourceRole.role} in (${roles.toSet})" else samsqls""
-    val actionConstraint = if (actions.nonEmpty) samsqls"and ${resourceAction.action} in (${actions.toSet})" else samsqls""
+      if (resourceTypeNames.nonEmpty) samsqls"and ${resource.resourceTypeId} in (${resourceTypeNames.flatMap(resourceTypePKsByName.get)})"
+      else samsqls"and ${resource.resourceTypeId} in (${resourceTypeNamesByPK.keys.map(_.value)})"
+    val policyConstraint = if (policies.nonEmpty) samsqls"and ${resourcePolicy.name} in (${policies})" else samsqls""
+    val roleConstraint = if (roles.nonEmpty) samsqls"and ${resourceRole.role} in (${roles})" else samsqls""
+    val actionConstraint = if (actions.nonEmpty) samsqls"and ${resourceAction.action} in (${actions})" else samsqls""
 
     val policyRoleActionQuery =
       samsqls"""
@@ -1783,7 +1784,7 @@ class PostgresAccessPolicyDAO(
         .map(rs =>
           FilterResourcesResult(
             rs.get[ResourceId](resource.resultName.name),
-            resourceTypePKsToName(rs.get[ResourceTypePK](resource.resultName.resourceTypeId)),
+            resourceTypeNamesByPK(rs.get[ResourceTypePK](resource.resultName.resourceTypeId)),
             rs.stringOpt(resourcePolicy.resultName.name).map(AccessPolicyName(_)),
             rs.stringOpt(resourceRole.resultName.role).map(ResourceRoleName(_)),
             rs.stringOpt(resourceAction.resultName.action).map(ResourceAction(_)),

diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/FilteredResources.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/api/FilteredResources.scala
@@ -1,26 +1,5 @@
 package org.broadinstitute.dsde.workbench.sam.model.api
 
-import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicyName, ResourceAction, ResourceId, ResourceRoleName, ResourceTypeName}
-import spray.json.DefaultJsonProtocol.jsonFormat1
-import spray.json.RootJsonFormat
-import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._
-import spray.json.DefaultJsonProtocol._
-
-object FilteredResources {
-  implicit val FilteredResourcesFormat: RootJsonFormat[FilteredResources] = jsonFormat1(FilteredResources.apply)
-
-}
-case class FilteredResources(resources: Set[FilteredResource])
-
-object FilteredResource {
-  implicit val FilteredResourceFormat: RootJsonFormat[FilteredResource] = jsonFormat6(FilteredResource.apply)
-
+trait FilteredResources {
+  def format: String
 }
-case class FilteredResource(
-    resourceType: ResourceTypeName,
-    resourceId: ResourceId,
-    policies: Set[AccessPolicyName],
-    roles: Set[ResourceRoleName],
-    actions: Set[ResourceAction],
-    isPublic: Boolean
-)