From dd54e03b2b8baeb0e9c3fe181555b7b60ac77ffa Mon Sep 17 00:00:00 2001 From: tlangs Date: Fri, 13 Sep 2024 13:14:12 -0400 Subject: [PATCH 1/9] Remove project-specific Pet Service Accounts --- .../sam/provider/SamProviderSpec.scala | 2 +- render_config.sh | 33 ++-- .../dsde/sam/liquibase/changelog.xml | 1 + .../20240911_sam_pet_service_agents_table.xml | 33 ++++ src/main/resources/sam.conf | 2 + .../dsde/workbench/sam/api/AdminRoutes.scala | 59 ++++--- .../sam/config/GoogleServicesConfig.scala | 6 +- .../sam/dataAccess/DirectoryDAO.scala | 37 +++- .../sam/dataAccess/PostgresDirectoryDAO.scala | 126 ++++++++++++++ .../workbench/sam/db/SamTypeBinders.scala | 5 + .../sam/db/tables/PetServiceAgentsTable.scala | 40 +++++ .../sam/google/GoogleExtensionRoutes.scala | 28 ++-- .../sam/google/GoogleExtensions.scala | 158 ++++++++++++++---- .../dsde/workbench/sam/model/SamModel.scala | 14 ++ .../dsde/workbench/sam/TestSupport.scala | 1 + .../sam/dataAccess/MockDirectoryDAO.scala | 58 ++++++- .../dataAccess/PostgresDirectoryDAOSpec.scala | 85 ++++++++++ .../google/GoogleExtensionRoutesSpec.scala | 82 +++++---- .../google/GoogleExtensionRoutesV1Spec.scala | 18 +- .../sam/google/NewGoogleExtensionsSpec.scala | 51 +++--- 20 files changed, 690 insertions(+), 149 deletions(-) create mode 100644 src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20240911_sam_pet_service_agents_table.xml create mode 100644 src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/PetServiceAgentsTable.scala diff --git a/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/provider/SamProviderSpec.scala b/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/provider/SamProviderSpec.scala index e924f93abd..5d13200d4e 100644 --- a/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/provider/SamProviderSpec.scala +++ b/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/provider/SamProviderSpec.scala @@ -135,7 +135,7 @@ class SamProviderSpec private def mockGetArbitraryPetServiceAccountToken(): IO[Unit] = for { _ <- IO( when { - googleExt.getArbitraryPetServiceAccountToken(any[SamUser], any[Set[String]], any[SamRequestContext]) + googleExt.getSingletonPetServiceAccountToken(any[SamUser], any[Set[String]], any[SamRequestContext]) } thenReturn { IO.pure("aToken") } diff --git a/render_config.sh b/render_config.sh index 398500c6fc..1878656eae 100755 --- a/render_config.sh +++ b/render_config.sh @@ -2,21 +2,22 @@ ENV=${1:-dev} SERVICE_OUTPUT_LOCATION="$(dirname "$0")/src/main/resources/rendered" SECRET_ENV_VARS_LOCATION="${SERVICE_OUTPUT_LOCATION}/secrets.env" -gcloud container clusters get-credentials --zone us-central1-a --project broad-dsde-dev terra-dev +gcloud container clusters get-credentials --zone us-central1-a --project broad-dsde-"${ENV}" terra-"${ENV}" -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-"${ENV}" get secret sam-sa-secret -o 'go-template={{index .data "sam-account.json"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/sam-account.json +kubectl -n terra-"${ENV}" 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-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-"${ENV}" 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-"${ENV}" 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-"${ENV}" 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-"${ENV}" 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-"${ENV}" 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. -kubectl -n terra-dev get configmap sam-proxy-configmap -o 'go-template={{index .data "apache-httpd-proxy-config"}}' | sed 's/localhost/host\.docker\.internal/g' > ${SERVICE_OUTPUT_LOCATION}/site.conf +kubectl -n terra-"${ENV}" 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 localhost, so replace all instances in the proxy config. +kubectl -n terra-"${ENV}" get configmap sam-proxy-configmap -o 'go-template={{index .data "apache-httpd-proxy-config"}}' | sed 's/localhost/host\.docker\.internal/g' > ${SERVICE_OUTPUT_LOCATION}/site.conf +gcloud container clusters get-credentials --zone us-central1-a --project broad-dsde-dev terra-dev kubectl -n local-dev get secrets local-dev-cert -o 'go-template={{index .data "tls.crt"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/server.crt kubectl -n local-dev get secrets local-dev-cert -o 'go-template={{index .data "tls.key"}}' | base64 --decode > ${SERVICE_OUTPUT_LOCATION}/server.key @@ -25,11 +26,11 @@ if [ -f "${SECRET_ENV_VARS_LOCATION}" ]; then fi { -echo export AZURE_MANAGED_APP_CLIENT_ID="$(vault read -field=client-id secret/dsde/terra/azure/dev/sam/managed-app-publisher)"; -echo export AZURE_MANAGED_APP_CLIENT_SECRET="$(vault read -field=client-secret secret/dsde/terra/azure/dev/sam/managed-app-publisher)"; -echo export AZURE_MANAGED_APP_TENANT_ID="$(vault read -field=tenant-id secret/dsde/terra/azure/dev/sam/managed-app-publisher)"; -echo export LEGACY_GOOGLE_CLIENT_ID="$(vault read -format=json -field=data secret/dsde/firecloud/dev/common/refresh-token-oauth-credential.json | jq -r '.web.client_id')"; -echo export OIDC_CLIENT_ID="$(vault read -field=value secret/dsde/terra/azure/dev/b2c/application_id)"; +echo export AZURE_MANAGED_APP_CLIENT_ID="$(vault read -field=client-id secret/dsde/terra/azure/"${ENV}"/sam/managed-app-publisher)"; +echo export AZURE_MANAGED_APP_CLIENT_SECRET="$(vault read -field=client-secret secret/dsde/terra/azure/"${ENV}"/sam/managed-app-publisher)"; +echo export AZURE_MANAGED_APP_TENANT_ID="$(vault read -field=tenant-id secret/dsde/terra/azure/"${ENV}"/sam/managed-app-publisher)"; +echo export LEGACY_GOOGLE_CLIENT_ID="$(vault read -format=json -field=data secret/dsde/firecloud/"${ENV}"/common/refresh-token-oauth-credential.json | jq -r '.web.client_id')"; +echo export OIDC_CLIENT_ID="$(vault read -field=value secret/dsde/terra/azure/"${ENV}"/b2c/application_id)"; echo export SERVICE_ACCOUNT_CLIENT_EMAIL="$(cat ${SERVICE_OUTPUT_LOCATION}/sam-account.json | jq .client_email)"; echo export SERVICE_ACCOUNT_CLIENT_ID="$(cat ${SERVICE_OUTPUT_LOCATION}/sam-account.json | jq .client_id)"; diff --git a/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml index 6239c45435..32247168ba 100644 --- a/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml +++ b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml @@ -33,4 +33,5 @@ + diff --git a/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20240911_sam_pet_service_agents_table.xml b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20240911_sam_pet_service_agents_table.xml new file mode 100644 index 0000000000..bef85fc162 --- /dev/null +++ b/src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20240911_sam_pet_service_agents_table.xml @@ -0,0 +1,33 @@ + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/src/main/resources/sam.conf b/src/main/resources/sam.conf index 0951adb47d..28dc2611ed 100644 --- a/src/main/resources/sam.conf +++ b/src/main/resources/sam.conf @@ -117,6 +117,8 @@ googleServices { samplingProbability = ${?OPENCENSUS_SAMPLING_PROBABILITY} # for backwards compatibility samplingProbability = ${?GOOGLE_TRACE_SAMPLING_PROBABILITY} } + + serviceAgents = ${?SERVICE_AGENTS} } db { diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala index 19fde18687..1b6fe82ab2 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala @@ -161,40 +161,53 @@ trait AdminRoutes extends SecurityDirectives with SamRequestContextDirectives wi } def adminResourcesRoutes(user: SamUser, samRequestContext: SamRequestContext): server.Route = - pathPrefix("resources" / Segment / Segment / "policies") { case (resourceTypeName, resourceId) => + pathPrefix("resources" / Segment / Segment) { case (resourceTypeName, resourceId) => withNonAdminResourceType(ResourceTypeName(resourceTypeName)) { resourceType => val resource = FullyQualifiedResourceId(resourceType.name, ResourceId(resourceId)) pathEndOrSingleSlash { - getWithTelemetry(samRequestContext, resourceParams(resource): _*) { - requireAdminResourceAction(adminReadPolicies, resourceType, user, samRequestContext) { + asWorkbenchAdmin(user) { + delete { complete { resourceService - .listResourcePolicies(resource, samRequestContext) - .map(response => OK -> response.toSet) + .deleteResource(resource, samRequestContext) + .map(_ => NoContent) } } } } ~ - pathPrefix(Segment / "memberEmails" / Segment) { case (policyName, userEmail) => - val policyId = FullyQualifiedPolicyId(resource, AccessPolicyName(policyName)) - val workbenchEmail = WorkbenchEmail(userEmail) + pathPrefix("policies") { pathEndOrSingleSlash { - withSubject(workbenchEmail, samRequestContext) { subject => - putWithTelemetry(samRequestContext, policyParams(policyId).appended(emailParam(workbenchEmail)): _*) { - requireAdminResourceAction(adminAddMember, resourceType, user, samRequestContext) { - complete { - resourceService - .addSubjectToPolicy(policyId, subject, samRequestContext) - .as(NoContent) - } + getWithTelemetry(samRequestContext, resourceParams(resource): _*) { + requireAdminResourceAction(adminReadPolicies, resourceType, user, samRequestContext) { + complete { + resourceService + .listResourcePolicies(resource, samRequestContext) + .map(response => OK -> response.toSet) } - } ~ - deleteWithTelemetry(samRequestContext, policyParams(policyId).appended(emailParam(workbenchEmail)): _*) { - requireAdminResourceAction(adminRemoveMember, resourceType, user, samRequestContext) { - complete { - resourceService - .removeSubjectFromPolicy(policyId, subject, samRequestContext) - .as(NoContent) + } + } + } ~ + pathPrefix(Segment / "memberEmails" / Segment) { case (policyName, userEmail) => + val policyId = FullyQualifiedPolicyId(resource, AccessPolicyName(policyName)) + val workbenchEmail = WorkbenchEmail(userEmail) + pathEndOrSingleSlash { + withSubject(workbenchEmail, samRequestContext) { subject => + putWithTelemetry(samRequestContext, policyParams(policyId).appended(emailParam(workbenchEmail)): _*) { + requireAdminResourceAction(adminAddMember, resourceType, user, samRequestContext) { + complete { + resourceService + .addSubjectToPolicy(policyId, subject, samRequestContext) + .as(NoContent) + } + } + } ~ + deleteWithTelemetry(samRequestContext, policyParams(policyId).appended(emailParam(workbenchEmail)): _*) { + requireAdminResourceAction(adminRemoveMember, resourceType, user, samRequestContext) { + complete { + resourceService + .removeSubjectFromPolicy(policyId, subject, samRequestContext) + .as(NoContent) + } } } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/GoogleServicesConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/GoogleServicesConfig.scala index cdb0d5d50f..1a86cd75e0 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/GoogleServicesConfig.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/GoogleServicesConfig.scala @@ -33,7 +33,8 @@ final case class GoogleServicesConfig( adminSdkServiceAccountPaths: Option[NonEmptyList[String]], googleKms: GoogleKmsConfig, terraGoogleOrgNumber: String, - traceExporter: TraceExporterConfig + traceExporter: TraceExporterConfig, + serviceAgents: Set[String] ) object GoogleServicesConfig { @@ -98,7 +99,8 @@ object GoogleServicesConfig { config.as[Option[NonEmptyList[String]]]("adminSdkServiceAccountPaths"), config.as[GoogleKmsConfig]("kms"), config.getString("terraGoogleOrgNumber"), - config.as[TraceExporterConfig]("traceExporter") + config.as[TraceExporterConfig]("traceExporter"), + config.as[Option[String]]("serviceAgents").map(_.split(",").toSet).getOrElse(Set.empty) ) } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala index eeca37f183..54552ad84d 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala @@ -2,7 +2,7 @@ package org.broadinstitute.dsde.workbench.sam.dataAccess import cats.effect.IO import org.broadinstitute.dsde.workbench.model._ -import org.broadinstitute.dsde.workbench.model.google.ServiceAccountSubjectId +import org.broadinstitute.dsde.workbench.model.google.{GoogleProject, ServiceAccountSubjectId} import org.broadinstitute.dsde.workbench.sam.azure.{ ActionManagedIdentity, ActionManagedIdentityId, @@ -12,7 +12,15 @@ import org.broadinstitute.dsde.workbench.sam.azure.{ PetManagedIdentityId } import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes} -import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, FullyQualifiedResourceId, ResourceAction, ResourceTypeName, SamUserTos} +import org.broadinstitute.dsde.workbench.sam.model.{ + BasicWorkbenchGroup, + FullyQualifiedResourceId, + PetServiceAgents, + ResourceAction, + ResourceTypeName, + SamUserTos, + ServiceAgent +} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.time.Instant @@ -190,4 +198,29 @@ trait DirectoryDAO { resourceTypeName: ResourceTypeName, samRequestContext: SamRequestContext ): IO[Set[FullyQualifiedResourceId]] + + def getPetServiceAgents( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + samRequestContext: SamRequestContext + ): IO[Option[PetServiceAgents]] + + def addPetServiceAgent( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgent: String, + samRequestContext: SamRequestContext + ): IO[Set[ServiceAgent]] + + def removePetServiceAgent( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgent: String, + samRequestContext: SamRequestContext + ): IO[Set[ServiceAgent]] } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala index 8018692e94..5ca806a4e8 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala @@ -1465,4 +1465,130 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val .toSet } + def getPetServiceAgents( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + samRequestContext: SamRequestContext + ): IO[Option[PetServiceAgents]] = + readOnlyTransaction("getPetServiceAgents", samRequestContext) { implicit session => + val petServiceAgentsTable = PetServiceAgentsTable.syntax + val loadPetServiceAgentsQuery = samsql"""select ${petServiceAgentsTable.resultAll} + from ${PetServiceAgentsTable as petServiceAgentsTable} + where ${petServiceAgentsTable.samUserId} = $userId + and ${petServiceAgentsTable.petServiceAccountProject} = $petServiceAccountProject + and ${petServiceAgentsTable.destinationProject} = $destinationProject""" + loadPetServiceAgentsQuery + .map(PetServiceAgentsTable(petServiceAgentsTable)) + .single() + .apply() + .map(record => + PetServiceAgents( + record.samUserId, + record.petServiceAccountProject, + record.destinationProject, + record.destinationProjectNumber, + record.serviceAgents.map(a => ServiceAgent(a, record.destinationProjectNumber)) + ) + ) + } + + def addPetServiceAgent( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgent: String, + samRequestContext: SamRequestContext + ): IO[Set[ServiceAgent]] = + serializableWriteTransaction("addPetServiceAgent", samRequestContext) { implicit session => + val updatedAt = Instant.now() + val petServiceAgentsTable = PetServiceAgentsTable.syntax + val petServiceAgentsColumns = PetServiceAgentsTable.column + samsql""" + insert into ${PetServiceAgentsTable as petServiceAgentsTable} + (${petServiceAgentsColumns.samUserId}, ${petServiceAgentsColumns.petServiceAccountProject}, ${petServiceAgentsColumns.destinationProject}, ${petServiceAgentsColumns.destinationProjectNumber}, ${petServiceAgentsColumns.serviceAgents}, ${petServiceAgentsColumns.updatedAt}) + values ($userId, $petServiceAccountProject, $destinationProject, $destinationProjectNumber, ARRAY [$serviceAgent], $updatedAt) + on conflict (${petServiceAgentsColumns.samUserId}, ${petServiceAgentsColumns.petServiceAccountProject}, ${petServiceAgentsColumns.destinationProject}) + do update set ${petServiceAgentsColumns.serviceAgents} = ARRAY_APPEND(${petServiceAgentsTable.serviceAgents}, $serviceAgent) + where ${petServiceAgentsTable.samUserId} = $userId + and ${petServiceAgentsTable.petServiceAccountProject} = $petServiceAccountProject + and ${petServiceAgentsTable.destinationProject} = $destinationProject + returning ${petServiceAgentsColumns.serviceAgents} as ${petServiceAgentsTable.resultName.serviceAgents} + """ + .map { rs => + rs.array(petServiceAgentsTable.resultName.serviceAgents) + .getArray() + .asInstanceOf[Array[String]] + .map(sa => ServiceAgent(sa, destinationProjectNumber)) + .toSet + } + .single() + .apply() + .getOrElse( + throw new WorkbenchException( + s"PetServiceAgents not found for user $userId, petServiceAccountProject $petServiceAccountProject, destinationProject $destinationProject" + ) + ) + } + + def removePetServiceAgent( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgent: String, + samRequestContext: SamRequestContext + ): IO[Set[ServiceAgent]] = + serializableWriteTransaction("removePetServiceAgent", samRequestContext) { implicit session => + val updatedAt = Instant.now() + val petServiceAgentsTable = PetServiceAgentsTable.syntax + val petServiceAgentsColumns = PetServiceAgentsTable.column + val currentServiceAgents = + samsql""" + select ${petServiceAgentsTable.result.serviceAgents} + from ${PetServiceAgentsTable as petServiceAgentsTable} + where ${petServiceAgentsColumns.samUserId} = $userId + and ${petServiceAgentsColumns.petServiceAccountProject} = $petServiceAccountProject + and ${petServiceAgentsColumns.destinationProject} = $destinationProject + """ + .map(rs => + rs.array(petServiceAgentsTable.resultName.serviceAgents) + .getArray() + .asInstanceOf[Array[String]] + .map(sa => ServiceAgent(sa, destinationProjectNumber)) + .toSet + ) + .single() + .apply() + .getOrElse(Set.empty) + + val updatedServiceAgents = ServiceAgentsArray(currentServiceAgents - ServiceAgent(serviceAgent, destinationProjectNumber)) + samsql""" + insert into ${PetServiceAgentsTable as petServiceAgentsTable} + (${petServiceAgentsColumns.samUserId}, ${petServiceAgentsColumns.petServiceAccountProject}, ${petServiceAgentsColumns.destinationProject}, ${petServiceAgentsColumns.destinationProjectNumber}, ${petServiceAgentsColumns.serviceAgents}, ${petServiceAgentsColumns.updatedAt}) + values ($userId, $petServiceAccountProject, $destinationProject, $destinationProjectNumber, $updatedServiceAgents, $updatedAt) + on conflict (${petServiceAgentsColumns.samUserId}, ${petServiceAgentsColumns.petServiceAccountProject}, ${petServiceAgentsColumns.destinationProject}) + do update set ${petServiceAgentsColumns.serviceAgents} = $updatedServiceAgents + where ${petServiceAgentsTable.samUserId} = $userId + and ${petServiceAgentsTable.petServiceAccountProject} = $petServiceAccountProject + and ${petServiceAgentsTable.destinationProject} = $destinationProject + returning ${petServiceAgentsColumns.serviceAgents} as ${petServiceAgentsTable.resultName.serviceAgents} + """ + .map { rs => + rs.array(petServiceAgentsTable.resultName.serviceAgents) + .getArray() + .asInstanceOf[Array[String]] + .map(sa => ServiceAgent(sa, destinationProjectNumber)) + .toSet + } + .single() + .apply() + .getOrElse( + throw new WorkbenchException( + s"PetServiceAgents not found for user $userId, petServiceAccountProject $petServiceAccountProject, destinationProject $destinationProject" + ) + ) + } + } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/SamTypeBinders.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/SamTypeBinders.scala index 9ed071b456..dadd4699c9 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/SamTypeBinders.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/SamTypeBinders.scala @@ -183,4 +183,9 @@ object SamTypeBinders { def apply(rs: ResultSet, index: Int): LastQuotaErrorPK = LastQuotaErrorPK(rs.getLong(index)) } + implicit def setTypeBinder[A]: TypeBinder[Set[A]] = new TypeBinder[Set[A]] { + def apply(rs: ResultSet, label: String): Set[A] = rs.getArray(label).getArray.asInstanceOf[Array[A]].toSet + def apply(rs: ResultSet, index: Int): Set[A] = rs.getArray(index).getArray.asInstanceOf[Array[A]].toSet + } + } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/PetServiceAgentsTable.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/PetServiceAgentsTable.scala new file mode 100644 index 0000000000..928cdeda19 --- /dev/null +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/PetServiceAgentsTable.scala @@ -0,0 +1,40 @@ +package org.broadinstitute.dsde.workbench.sam.db.tables + +import org.broadinstitute.dsde.workbench.model.google.GoogleProject +import org.broadinstitute.dsde.workbench.model.WorkbenchUserId +import org.broadinstitute.dsde.workbench.sam.db.{DatabaseArray, SamTypeBinders} +import org.broadinstitute.dsde.workbench.sam.model.ServiceAgent +import scalikejdbc._ + +import java.time.Instant + +final case class PetServiceAgentsRecord( + samUserId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgents: Set[String], + updatedAt: Instant +) + +final case class ServiceAgentsArray(serviceAgents: Set[ServiceAgent]) extends DatabaseArray { + override val baseTypeName: String = "VARCHAR" + override def asJavaArray: Array[Object] = serviceAgents.toArray.map(_.name.asInstanceOf[Object]) +} + +object PetServiceAgentsTable extends SQLSyntaxSupportWithDefaultSamDB[PetServiceAgentsRecord] { + override def tableName: String = "SAM_PET_SERVICE_AGENTS" + + import SamTypeBinders._ + + def apply(e: ResultName[PetServiceAgentsRecord])(rs: WrappedResultSet): PetServiceAgentsRecord = PetServiceAgentsRecord( + rs.get(e.samUserId), + rs.get(e.petServiceAccountProject), + rs.get(e.destinationProject), + rs.get(e.destinationProjectNumber), + rs.get(e.serviceAgents), + rs.get(e.updatedAt) + ) + + def apply(p: SyntaxProvider[PetServiceAgentsRecord])(rs: WrappedResultSet): PetServiceAgentsRecord = apply(p.resultName)(rs) +} diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala index b1d08ecedc..948ddfdbdc 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala @@ -48,9 +48,9 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with getWithTelemetry(samRequestContext, "userEmail" -> email) { complete { import spray.json._ - googleExtensions.getArbitraryPetServiceAccountKey(email, samRequestContext) map { + googleExtensions.getSingletonPetServiceAccountAndKeyForUser(email, None, samRequestContext) map { // parse json to ensure it is json and tells akka http the right content-type - case Some(key) => StatusCodes.OK -> key.parseJson + case Some((_, key)) => StatusCodes.OK -> key.parseJson case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, "pet service account not found")) } @@ -61,7 +61,7 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with postWithTelemetry(samRequestContext, "userEmail" -> email) { entity(as[Set[String]]) { scopes => complete { - googleExtensions.getArbitraryPetServiceAccountToken(email, scopes, samRequestContext).map { + googleExtensions.getSingletonPetServiceAccountToken(email, scopes, samRequestContext).map { case Some(token) => StatusCodes.OK -> JsString(token) case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, "pet service account not found")) @@ -78,9 +78,9 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with getWithTelemetry(samRequestContext, "userEmail" -> email, "googleProject" -> googleProject) { complete { import spray.json._ - googleExtensions.getPetServiceAccountKey(email, googleProject, samRequestContext) map { + googleExtensions.getSingletonPetServiceAccountAndKeyForUser(email, Some(googleProject), samRequestContext) map { // parse json to ensure it is json and tells akka http the right content-type - case Some(key) => StatusCodes.OK -> key.parseJson + case Some((_, key)) => StatusCodes.OK -> key.parseJson case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, "pet service account not found")) } @@ -91,7 +91,7 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with postWithTelemetry(samRequestContext, "userEmail" -> email) { entity(as[Set[String]]) { scopes => complete { - googleExtensions.getPetServiceAccountToken(email, googleProject, scopes, samRequestContext).map { + googleExtensions.getSingletonPetServiceAccountToken(email, scopes, samRequestContext).map { case Some(token) => StatusCodes.OK -> JsString(token) case None => throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.NotFound, "pet service account not found")) @@ -111,8 +111,8 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with complete { import spray.json._ googleExtensions - .getArbitraryPetServiceAccountKey(samUser, samRequestContext) - .map(key => StatusCodes.OK -> key.parseJson) + .getSingletonPetServiceAccountAndKeyForUser(samUser, None, samRequestContext) + .map(tup => StatusCodes.OK -> tup._2.parseJson) } } } @@ -122,7 +122,7 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with postWithTelemetry(samRequestContext) { entity(as[Set[String]]) { scopes => complete { - googleExtensions.getArbitraryPetServiceAccountToken(samUser, scopes, samRequestContext).map { token => + googleExtensions.getSingletonPetServiceAccountToken(samUser, scopes, samRequestContext).map { token => StatusCodes.OK -> JsString(token) } } @@ -144,9 +144,9 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with import spray.json._ // parse json to ensure it is json and tells akka http the right content-type googleExtensions - .getPetServiceAccountKey(samUser, GoogleProject(project), samRequestContext) - .map { key => - StatusCodes.OK -> key.parseJson + .getSingletonPetServiceAccountAndKeyForUser(samUser, Some(GoogleProject(project)), samRequestContext) + .map { tup => + StatusCodes.OK -> tup._2.parseJson } } } @@ -219,8 +219,8 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with ) { getWithTelemetry(samRequestContext, "googleProject" -> projectResourceId) { complete { - googleExtensions.createUserPetServiceAccount(samUser, GoogleProject(project), samRequestContext).map { petSA => - StatusCodes.OK -> petSA.serviceAccount.email + googleExtensions.getSingletonPetServiceAccountAndKeyForUser(samUser, Some(GoogleProject(project)), samRequestContext).map { petSA => + StatusCodes.OK -> petSA._1.serviceAccount.email } } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 518736e397..98b8a23a66 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -2,7 +2,6 @@ package org.broadinstitute.dsde.workbench.sam.google import akka.actor.ActorSystem import akka.http.scaladsl.model.StatusCodes -import cats.effect.unsafe.implicits.global import cats.effect.{Clock, IO} import cats.implicits._ import com.google.api.client.googleapis.json.GoogleJsonResponseException @@ -455,6 +454,12 @@ class GoogleExtensions( key <- googleKeyCache.getKey(pet) } yield key + def getPetServiceAccountAndKey(user: SamUser, project: GoogleProject, samRequestContext: SamRequestContext): IO[(PetServiceAccount, String)] = + for { + pet <- createUserPetServiceAccount(user, project, samRequestContext) + key <- googleKeyCache.getKey(pet) + } yield (pet, key) + def getPetServiceAccountToken(user: SamUser, project: GoogleProject, scopes: Set[String], samRequestContext: SamRequestContext): IO[String] = getPetServiceAccountKey(user, project, samRequestContext).flatMap { key => IO.fromFuture(IO(getAccessTokenUsingJson(key, scopes))) @@ -475,51 +480,146 @@ class GoogleExtensions( } } yield token - def getArbitraryPetServiceAccountKey(userEmail: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[String]] = + private[google] def getSingletonPetServiceAccountAndKeyForUser( + userEmail: WorkbenchEmail, + destinationProject: Option[GoogleProject], + samRequestContext: SamRequestContext + ): IO[Option[(PetServiceAccount, String)]] = for { subject <- directoryDAO.loadSubjectFromEmail(userEmail, samRequestContext) - key <- subject match { + opt <- subject match { case Some(userId: WorkbenchUserId) => - getArbitraryPetServiceAccountKey(SamUser(userId, None, userEmail, None, false), samRequestContext).map(Option(_)) + getSingletonPetServiceAccountAndKeyForUser(SamUser(userId, None, userEmail, None, false), destinationProject, samRequestContext).map(Option(_)) case _ => IO.none } - } yield key + } yield opt - def getArbitraryPetServiceAccountKey(user: SamUser, samRequestContext: SamRequestContext): IO[String] = - IO.fromFuture(IO(getDefaultServiceAccountForShellProject(user, samRequestContext))) + private[google] def getSingletonPetServiceAccountAndKeyForUser( + user: SamUser, + destinationProject: Option[GoogleProject], + samRequestContext: SamRequestContext + ): IO[(PetServiceAccount, String)] = + for { + sa <- getSingletonPetServiceAccountForUser(user, destinationProject, samRequestContext) + key <- getPetServiceAccountKey(user, sa.id.project, samRequestContext) + } yield (sa, key) - def getArbitraryPetServiceAccountToken(userEmail: WorkbenchEmail, scopes: Set[String], samRequestContext: SamRequestContext): IO[Option[String]] = + private[google] def getSingletonPetServiceAccountKeyForUser( + user: SamUser, + destinationProject: Option[GoogleProject], + samRequestContext: SamRequestContext + ): IO[String] = for { - subject <- directoryDAO.loadSubjectFromEmail(userEmail, samRequestContext) - token <- subject match { - case Some(userId: WorkbenchUserId) => - getArbitraryPetServiceAccountToken(SamUser(userId, None, userEmail, None, false), scopes, samRequestContext).map(Option(_)) - case _ => IO.none - } - } yield token + (_, key) <- getSingletonPetServiceAccountAndKeyForUser(user, destinationProject, samRequestContext) + } yield key - def getArbitraryPetServiceAccountToken(user: SamUser, scopes: Set[String], samRequestContext: SamRequestContext): IO[String] = - getArbitraryPetServiceAccountKey(user, samRequestContext).flatMap { key => - IO.fromFuture(IO(getAccessTokenUsingJson(key, scopes))) - } + private[google] def getSingletonPetServiceAccountKeyForUser( + userEmail: WorkbenchEmail, + destinationProject: Option[GoogleProject], + samRequestContext: SamRequestContext + ): IO[Option[String]] = + for { + opt <- getSingletonPetServiceAccountAndKeyForUser(userEmail, destinationProject, samRequestContext) + } yield opt.map(_._2) - private[google] def getDefaultServiceAccountForShellProject(user: SamUser, samRequestContext: SamRequestContext): Future[String] = { + private[google] def getSingletonPetServiceAccountForUser( + user: SamUser, + destinationProject: Option[GoogleProject], + samRequestContext: SamRequestContext + ): IO[PetServiceAccount] = { val projectName = s"fc-${googleServicesConfig.environment.substring(0, Math.min(googleServicesConfig.environment.length(), 5))}-${user.id.value}" // max 30 characters. subject ID is 21 for { - creationOperationId <- googleProjectDAO - .createProject(projectName, googleServicesConfig.terraGoogleOrgNumber, GoogleResourceTypes.Organization) - .map(opId => Option(opId)) recover { - case gjre: GoogleJsonResponseException if gjre.getDetails.getCode == StatusCodes.Conflict.intValue => None + creationOperationId <- IO.fromFuture { + IO { + googleProjectDAO + .createProject(projectName, googleServicesConfig.terraGoogleOrgNumber, GoogleResourceTypes.Organization) + .map(opId => Option(opId)) recover { + case gjre: GoogleJsonResponseException if gjre.getDetails.getCode == StatusCodes.Conflict.intValue => None + } + } } _ <- creationOperationId match { - case Some(opId) => pollShellProjectCreation(opId) // poll until it's created - case None => Future.successful(()) + case Some(opId) => IO.fromFuture(IO(pollShellProjectCreation(opId))) // poll until it's created + case None => IO.unit } - key <- getPetServiceAccountKey(user, GoogleProject(projectName), samRequestContext).unsafeToFuture() - } yield key + sa <- createUserPetServiceAccount(user, GoogleProject(projectName), samRequestContext) + _ <- destinationProject.map(p => syncServiceAgents(user, sa, p, samRequestContext)).getOrElse(IO.unit) + } yield sa } + private def syncServiceAgents( + user: SamUser, + petServiceAccount: PetServiceAccount, + destinationProject: GoogleProject, + samRequestContext: SamRequestContext + ): IO[Unit] = + for { + petServiceAgents <- directoryDAO.getPetServiceAgents(user.id, petServiceAccount.id.project, destinationProject, samRequestContext) + existingServiceAgents = petServiceAgents.map(_.serviceAgents.map(_.name)).getOrElse(Set.empty) + serviceAgentsToAdd = googleServicesConfig.serviceAgents -- existingServiceAgents + destinationProjectNumber <- petServiceAgents + .map(psa => IO.pure(Option(psa.destinationProjectNumber))) + .getOrElse(IO.fromFuture(IO(googleProjectDAO.getProjectNumber(destinationProject.value)))) + .map(_.getOrElse(throw new WorkbenchException(s"Could not find project number for project ${destinationProject.value}"))) + _ <- serviceAgentsToAdd.toList.traverse { serviceAgentName => + val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) + for { + _ <- IO.fromFuture( + IO( + googleIamDAO.addIamPolicyBindingOnServiceAccount( + petServiceAccount.id.project, + petServiceAccount.serviceAccount.email, + serviceAgent.email, + Set("roles/iam.serviceAccountTokenCreator") + ) + ) + ) + _ <- directoryDAO.addPetServiceAgent( + user.id, + petServiceAccount.id.project, + destinationProject, + destinationProjectNumber, + serviceAgentName, + samRequestContext + ) + } yield () + } + serviceAgentsToRemove = existingServiceAgents -- googleServicesConfig.serviceAgents + _ <- serviceAgentsToRemove.toList.traverse { serviceAgentName => +// val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) + for { + // removeIamPolicyBindingOnServiceAccount is not implemented in googleIamDAO yet + // TODO do it +// _ <- googleIamDAO.removeIamPolicyBindingOnServiceAccount(petServiceAccount.id.project, petServiceAccount.serviceAccount.email, serviceAgent.email, Set("roles/iam.serviceAccountTokenCreator")) + _ <- directoryDAO.removePetServiceAgent( + user.id, + petServiceAccount.id.project, + destinationProject, + destinationProjectNumber, + serviceAgentName, + samRequestContext + ) + } yield () + } + + } yield () + + def getSingletonPetServiceAccountToken(userEmail: WorkbenchEmail, scopes: Set[String], samRequestContext: SamRequestContext): IO[Option[String]] = + for { + saAndKey <- getSingletonPetServiceAccountAndKeyForUser(userEmail, None, samRequestContext) + token <- saAndKey match { + case Some((_, key)) => IO.fromFuture(IO(getAccessTokenUsingJson(key, scopes))).map(Option(_)) + case None => IO.pure(None) + } + } yield token + + def getSingletonPetServiceAccountToken(user: SamUser, scopes: Set[String], samRequestContext: SamRequestContext): IO[String] = + for { + (_, key) <- getSingletonPetServiceAccountAndKeyForUser(user, None, samRequestContext) + token <- IO.fromFuture(IO(getAccessTokenUsingJson(key, scopes))) + } yield token + private def pollShellProjectCreation(operationId: String): Future[Boolean] = { def whenCreating(throwable: Throwable): Boolean = throwable match { @@ -715,7 +815,7 @@ class GoogleExtensions( val bucket = GcsBucketName(blobId.getBucket) val objectName = GcsBlobName(blobId.getName) for { - petKey <- getArbitraryPetServiceAccountKey(samUser, samRequestContext) + petKey <- getSingletonPetServiceAccountKeyForUser(samUser, None, samRequestContext) serviceAccountCredentials = ServiceAccountCredentials.fromStream(new ByteArrayInputStream(petKey.getBytes())) url <- getSignedUrl(samUser, bucket, objectName, duration, urlParamsMap, serviceAccountCredentials) } yield url diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala index b101ee2bd6..dd403226f7 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala @@ -2,6 +2,7 @@ package org.broadinstitute.dsde.workbench.sam.model import monocle.macros.Lenses import org.broadinstitute.dsde.workbench.model._ +import org.broadinstitute.dsde.workbench.model.google.GoogleProject import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, AccessPolicyMembershipResponse, SamUser} import spray.json.{DefaultJsonProtocol, JsValue, RootJsonFormat} @@ -274,6 +275,19 @@ final case class SamUserTos(id: WorkbenchUserId, version: String, action: String } def toHistoryRecord: TermsOfServiceHistoryRecord = TermsOfServiceHistoryRecord(action, version, createdAt) } + +final case class PetServiceAgents( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgents: Set[ServiceAgent] +) + +final case class ServiceAgent(name: String, projectNumber: Long) { + val email: WorkbenchEmail = WorkbenchEmail(s"service-$projectNumber@$name.iam.gserviceaccount.com") +} + object SamLenses { val resourceIdentityAccessPolicy = AccessPolicy.id composeLens FullyQualifiedPolicyId.resource val resourceTypeNameInAccessPolicy = resourceIdentityAccessPolicy composeLens FullyQualifiedResourceId.resourceTypeName diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala index 5744681dc2..1a098dd02b 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala @@ -248,6 +248,7 @@ object TestSupport extends TestSupport { GroupMemberTable, GroupMemberFlatTable, PetServiceAccountTable, + PetServiceAgentsTable, AzureManagedResourceGroupTable, PetManagedIdentityTable, UserTable, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala index ed417cd397..fe631126c5 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockDirectoryDAO.scala @@ -4,7 +4,7 @@ import akka.http.scaladsl.model.StatusCodes import cats.effect.IO import cats.implicits._ import org.broadinstitute.dsde.workbench.model._ -import org.broadinstitute.dsde.workbench.model.google.ServiceAccountSubjectId +import org.broadinstitute.dsde.workbench.model.google.{GoogleProject, ServiceAccountSubjectId} import org.broadinstitute.dsde.workbench.sam._ import org.broadinstitute.dsde.workbench.sam.azure.{ ActionManagedIdentity, @@ -16,7 +16,16 @@ import org.broadinstitute.dsde.workbench.sam.azure.{ } import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable import org.broadinstitute.dsde.workbench.sam.model.api.{AdminUpdateUserRequest, SamUser, SamUserAttributes} -import org.broadinstitute.dsde.workbench.sam.model.{AccessPolicy, BasicWorkbenchGroup, FullyQualifiedResourceId, ResourceAction, ResourceTypeName, SamUserTos} +import org.broadinstitute.dsde.workbench.sam.model.{ + AccessPolicy, + BasicWorkbenchGroup, + FullyQualifiedResourceId, + PetServiceAgents, + ResourceAction, + ResourceTypeName, + SamUserTos, + ServiceAgent +} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import java.time.Instant @@ -45,6 +54,8 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench private val userFavoriteResources: mutable.Map[WorkbenchUserId, Set[FullyQualifiedResourceId]] = new TrieMap() + private val petServiceAgents: mutable.Map[(WorkbenchUserId, GoogleProject, GoogleProject), PetServiceAgents] = new TrieMap() + override def createGroup( group: BasicWorkbenchGroup, accessInstruction: Option[String] = None, @@ -511,4 +522,47 @@ class MockDirectoryDAO(val groups: mutable.Map[WorkbenchGroupIdentity, Workbench samRequestContext: SamRequestContext ): IO[Set[FullyQualifiedResourceId]] = IO.pure(userFavoriteResources.getOrElse(userId, Set.empty).filter(_.resourceTypeName == resourceTypeName)) + + override def getPetServiceAgents( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + samRequestContext: SamRequestContext + ): IO[Option[PetServiceAgents]] = + IO.pure(petServiceAgents.get((userId, petServiceAccountProject, destinationProject))) + + override def addPetServiceAgent( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgent: String, + samRequestContext: SamRequestContext + ): IO[Set[ServiceAgent]] = { + val agents = petServiceAgents.getOrElse( + (userId, petServiceAccountProject, destinationProject), + PetServiceAgents(userId, petServiceAccountProject, destinationProject, destinationProjectNumber, Set.empty) + ) + val updated = agents.copy(serviceAgents = agents.serviceAgents + ServiceAgent(serviceAgent, destinationProjectNumber)) + petServiceAgents.put((userId, petServiceAccountProject, destinationProject), updated) + IO.pure(updated.serviceAgents) + } + + override def removePetServiceAgent( + userId: WorkbenchUserId, + petServiceAccountProject: GoogleProject, + destinationProject: GoogleProject, + destinationProjectNumber: Long, + serviceAgent: String, + samRequestContext: SamRequestContext + ): IO[Set[ServiceAgent]] = { + val agents = petServiceAgents.getOrElse( + (userId, petServiceAccountProject, destinationProject), + PetServiceAgents(userId, petServiceAccountProject, destinationProject, destinationProjectNumber, Set.empty) + ) + val updated = agents.copy(serviceAgents = agents.serviceAgents - ServiceAgent(serviceAgent, destinationProjectNumber)) + petServiceAgents.put((userId, petServiceAccountProject, destinationProject), updated) + IO.pure(updated.serviceAgents) + } + } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala index fcb0d910a0..c819df9a02 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala @@ -22,6 +22,7 @@ import java.time.Instant import java.time.temporal.ChronoUnit import java.util.{Date, UUID} import scala.concurrent.duration._ +import scala.util.Random class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with BeforeAndAfterEach with TimeMatchers with OptionValues { val dao = new PostgresDirectoryDAO(TestSupport.dbRef, TestSupport.dbRef) @@ -2122,4 +2123,88 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B } } } + + "Service Agents " - { + "adds a service agent to a singleton pet service account if a record doesn't exist" in { + assume(databaseEnabled, databaseEnabledClue) + val destinationProject = GoogleProject(UUID.randomUUID().toString) + val destinationProjectNumber = Random.between(0L, Long.MaxValue) + val serviceAgents = Set("compute-system") + dao.createUser(defaultUser, samRequestContext).unsafeRunSync() + dao.createPetServiceAccount(defaultPetSA, samRequestContext).unsafeRunSync() + val petServiceAgents = dao + .addPetServiceAgent(defaultUser.id, defaultPetSA.id.project, destinationProject, destinationProjectNumber, serviceAgents.head, samRequestContext) + .unsafeRunSync() + + petServiceAgents shouldEqual serviceAgents.map(ServiceAgent(_, destinationProjectNumber)) + } + + "updates an existing PetServiceAgents record" in { + assume(databaseEnabled, databaseEnabledClue) + val destinationProject = GoogleProject(UUID.randomUUID().toString) + val destinationProjectNumber = Random.between(0L, Long.MaxValue) + val serviceAgents = Set("compute-system") + dao.createUser(defaultUser, samRequestContext).unsafeRunSync() + dao.createPetServiceAccount(defaultPetSA, samRequestContext).unsafeRunSync() + dao + .addPetServiceAgent(defaultUser.id, defaultPetSA.id.project, destinationProject, destinationProjectNumber, serviceAgents.head, samRequestContext) + .unsafeRunSync() + val newServiceAgent = "storage-system" + val petServiceAgents = dao + .addPetServiceAgent(defaultUser.id, defaultPetSA.id.project, destinationProject, destinationProjectNumber, newServiceAgent, samRequestContext) + .unsafeRunSync() + + petServiceAgents shouldEqual (serviceAgents + newServiceAgent).map(ServiceAgent(_, destinationProjectNumber)) + } + + "removes a service agent from an existing PetServiceAgents record" in { + assume(databaseEnabled, databaseEnabledClue) + val destinationProject = GoogleProject(UUID.randomUUID().toString) + val destinationProjectNumber = Random.between(0L, Long.MaxValue) + + val serviceAgents = Set("compute-system", "storage-system") + dao.createUser(defaultUser, samRequestContext).unsafeRunSync() + dao.createPetServiceAccount(defaultPetSA, samRequestContext).unsafeRunSync() + + serviceAgents.foreach(serviceAgent => + dao + .addPetServiceAgent(defaultUser.id, defaultPetSA.id.project, destinationProject, destinationProjectNumber, serviceAgent, samRequestContext) + .unsafeRunSync() + ) + val petServiceAgents = dao.getPetServiceAgents(defaultUser.id, defaultPetSA.id.project, destinationProject, samRequestContext).unsafeRunSync() + petServiceAgents.map(_.serviceAgents) shouldEqual Some(serviceAgents.map(ServiceAgent(_, destinationProjectNumber))) + + val removedServiceAgent = "storage-system" + dao + .removePetServiceAgent(defaultUser.id, defaultPetSA.id.project, destinationProject, destinationProjectNumber, removedServiceAgent, samRequestContext) + .unsafeRunSync() + val updatedPetServiceAgents = dao.getPetServiceAgents(defaultUser.id, defaultPetSA.id.project, destinationProject, samRequestContext).unsafeRunSync() + updatedPetServiceAgents.map(_.serviceAgents) shouldEqual Some(Set(ServiceAgent("compute-system", destinationProjectNumber))) + } + + "gets a PetServiceAgents record" in { + assume(databaseEnabled, databaseEnabledClue) + val destinationProject = GoogleProject(UUID.randomUUID().toString) + val destinationProjectNumber = Random.between(0L, Long.MaxValue) + val serviceAgents = Set("compute-system", "storage-system") + dao.createUser(defaultUser, samRequestContext).unsafeRunSync() + dao.createPetServiceAccount(defaultPetSA, samRequestContext).unsafeRunSync() + serviceAgents.foreach(serviceAgent => + dao + .addPetServiceAgent(defaultUser.id, defaultPetSA.id.project, destinationProject, destinationProjectNumber, serviceAgent, samRequestContext) + .unsafeRunSync() + ) + val petServiceAgents = dao.getPetServiceAgents(defaultUser.id, defaultPetSA.id.project, destinationProject, samRequestContext).unsafeRunSync() + + petServiceAgents shouldEqual Some( + PetServiceAgents( + defaultUser.id, + defaultPetSA.id.project, + destinationProject, + destinationProjectNumber, + serviceAgents.map(ServiceAgent(_, destinationProjectNumber)) + ) + ) + } + } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala index e3d751527d..3c301fc524 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala @@ -42,11 +42,11 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca val workspaceResourceId = "workspace" val v2GoogleProjectResourceId = "v2project" - private def getPetServiceAccount(projectName: String, routes: SamRoutes) = + private def getPetServiceAccount(projectName: String, user: SamUser, routes: SamRoutes) = Get(s"/api/google/user/petServiceAccount/${projectName}") ~> routes.route ~> check { status shouldEqual StatusCodes.OK val response = responseAs[WorkbenchEmail] - response.value should endWith(s"@${projectName}.iam.gserviceaccount.com") + response.value should be(singletonServiceAccountForUser(user)) response.value } @@ -57,7 +57,7 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca response shouldEqual expectedJson } - "GET /api/google/user/petServiceAccount" should "get or create a pet service account for a user in a v2 project" in { + "GET /api/google/user/petServiceAccount/{projectName}" should "get or create a pet service account for a user in a v2 project" in { assume(databaseEnabled, databaseEnabledClue) val policyEvaluatorService = mock[PolicyEvaluatorService](RETURNS_SMART_NULLS) @@ -79,13 +79,13 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca ) ) .thenReturn(IO(true)) - val (_, _, routes) = createTestUser(policyEvaluatorServiceOpt = Option(policyEvaluatorService), resourceServiceOpt = Option(resourceService)) + val (user, _, routes) = createTestUser(policyEvaluatorServiceOpt = Option(policyEvaluatorService), resourceServiceOpt = Option(resourceService)) // create a pet service account - getPetServiceAccount(v2GoogleProjectResourceId, routes) + getPetServiceAccount(v2GoogleProjectResourceId, user, routes) // same result a second time - getPetServiceAccount(v2GoogleProjectResourceId, routes) + getPetServiceAccount(v2GoogleProjectResourceId, user, routes) } it should "200 when the user doesn't have the right permission on the google-project resource, but it is v1" in { @@ -93,10 +93,10 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca val projectName = "myproject" - val (_, _, routes) = createTestUser() + val (user, _, routes) = createTestUser() // try to create a pet service account - getPetServiceAccount(projectName, routes) + getPetServiceAccount(projectName, user, routes) } it should "403 when the user doesn't have the right permission on the google-project resource" in { @@ -189,7 +189,7 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca val (user, _, routes) = createTestUser() - val petEmail = getPetServiceAccount("myproject", routes) + val petEmail = getPetServiceAccount("myproject", user, routes) Get(s"/api/google/user/proxyGroup/$petEmail") ~> routes.route ~> check { status shouldEqual StatusCodes.OK @@ -320,12 +320,14 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String) = createMockGoogleIamDaoForSAKeyTests + val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) + mockWithUser(user) + // create a pet service account - getPetServiceAccount("myproject", routes) + getPetServiceAccount("myproject", user, routes) // create a pet service account key getPetServiceAccountKey("myproject", expectedJson, routes) @@ -335,12 +337,13 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String) = createMockGoogleIamDaoForSAKeyTests + val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) + mockWithUser(user) // create a pet service account - getPetServiceAccount("myproject", routes) + getPetServiceAccount("myproject", user, routes) // create a pet service account key getPetServiceAccountKey("myproject", expectedJson, routes) @@ -409,13 +412,20 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca user: SamUser = Generator.genWorkbenchUserBoth.sample.get, googleProjectDAO: Option[GoogleProjectDAO] = None ): (SamUser, SamDependencies, SamRoutes) = { + lazy val googleProjectDAOToUse = googleProjectDAO.orElse(Option(new MockGoogleProjectDAO() { + override def pollOperation(operationId: String): Future[Operation] = { + val operation = new com.google.api.services.cloudresourcemanager.model.Operation + operation.setDone(true) + Future.successful(operation) + } + })) lazy val samDependencies = genSamDependencies( resourceTypes, googIamDAO, googleServicesConfig, policyEvaluatorServiceOpt = policyEvaluatorServiceOpt, resourceServiceOpt = resourceServiceOpt, - googProjectDAO = googleProjectDAO + googProjectDAO = googleProjectDAOToUse ) lazy val createRoutes = genSamRoutes(samDependencies, user) @@ -453,17 +463,24 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca googleIamDAO } - def createMockGoogleIamDaoForSAKeyTests: (GoogleIamDAO, String) = { + + def singletonServiceAccountForUser(user: SamUser) = + s"pet-${user.id.value}@fc-${googleServicesConfig.environment.substring(0, Math.min(googleServicesConfig.environment.length(), 5))}-${user.id.value}.iam.gserviceaccount.com" + + def createMockGoogleIamDaoForSAKeyTests: (GoogleIamDAO, String, SamUser => Unit) = { val googleIamDAO = mock[GoogleIamDAO](RETURNS_SMART_NULLS) val expectedJson = """{"json":"yes I am"}""" lenient().when(googleIamDAO.findServiceAccount(any[GoogleProject], any[ServiceAccountName])).thenReturn(Future.successful(None)) - lenient() - .when(googleIamDAO.createServiceAccount(any[GoogleProject], any[ServiceAccountName], any[ServiceAccountDisplayName])) - .thenReturn( - Future.successful( - ServiceAccount(ServiceAccountSubjectId("12312341234"), WorkbenchEmail("pet@myproject.iam.gserviceaccount.com"), ServiceAccountDisplayName("")) + val mockWithUser = (user: SamUser) => { + lenient() + .when(googleIamDAO.createServiceAccount(any[GoogleProject], any[ServiceAccountName], any[ServiceAccountDisplayName])) + .thenReturn( + Future.successful( + ServiceAccount(ServiceAccountSubjectId("12312341234"), WorkbenchEmail(singletonServiceAccountForUser(user)), ServiceAccountDisplayName("")) + ) ) - ) + () + } lenient() .when(googleIamDAO.createServiceAccountKey(any[GoogleProject], any[WorkbenchEmail])) .thenReturn( @@ -476,25 +493,24 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca lenient() .when(googleIamDAO.addServiceAccountUserRoleForUser(any[GoogleProject], any[WorkbenchEmail], any[WorkbenchEmail])) .thenReturn(Future.successful(())) - (googleIamDAO, expectedJson) + (googleIamDAO, expectedJson, mockWithUser) } def setupSignedUrlTest(): (SamUser, SamRoutes, String) = { val googleIamDAO = new RealKeyMockGoogleIamDAO - val googleProjectDAO = new MockGoogleProjectDAO() { - override def pollOperation(operationId: String): Future[Operation] = { - val operation = new com.google.api.services.cloudresourcemanager.model.Operation - operation.setDone(true) - Future.successful(operation) - } - } +// val googleProjectDAO = new MockGoogleProjectDAO() { +// override def pollOperation(operationId: String): Future[Operation] = { +// val operation = new com.google.api.services.cloudresourcemanager.model.Operation +// operation.setDone(true) +// Future.successful(operation) +// } +// } val samUser = Generator.genWorkbenchUserGoogle.sample.get val (user, samDeps, routes) = createTestUser( configResourceTypes, Some(googleIamDAO), TestSupport.googleServicesConfig.copy(serviceAccountClientEmail = samUser.email, serviceAccountClientId = samUser.googleSubjectId.get.value), - user = samUser, - googleProjectDAO = Some(googleProjectDAO) + user = samUser ) val resourceType = ResourceType( SamResourceTypes.googleProjectName, @@ -528,7 +544,7 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca } def setupPetSATest(): (SamUser, SamRoutes, String) = { - val (googleIamDAO, expectedJson: String) = createMockGoogleIamDaoForSAKeyTests + val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests val samUser = Generator.genWorkbenchUserGoogle.sample.get val (user, samDeps, routes) = createTestUser( configResourceTypes, @@ -537,6 +553,8 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca user = samUser ) + mockWithUser(user) + samDeps.cloudExtensions .asInstanceOf[GoogleExtensions] .onBoot( diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala index 4758fe6b89..9d9344da0a 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala @@ -34,14 +34,14 @@ class GoogleExtensionRoutesV1Spec extends GoogleExtensionRoutesSpecHelper with S Get("/api/google/v1/user/petServiceAccount/myproject") ~> routes.route ~> check { status shouldEqual StatusCodes.OK val response = responseAs[WorkbenchEmail] - response.value should endWith(s"@myproject.iam.gserviceaccount.com") + response.value should endWith(s"${user.id.value}.iam.gserviceaccount.com") } // same result a second time Get("/api/google/v1/user/petServiceAccount/myproject") ~> routes.route ~> check { status shouldEqual StatusCodes.OK val response = responseAs[WorkbenchEmail] - response.value should endWith(s"@myproject.iam.gserviceaccount.com") + response.value should endWith(s"${user.id.value}.iam.gserviceaccount.com") } } @@ -62,7 +62,7 @@ class GoogleExtensionRoutesV1Spec extends GoogleExtensionRoutesSpecHelper with S val petEmail = Get("/api/google/v1/user/petServiceAccount/myproject") ~> routes.route ~> check { status shouldEqual StatusCodes.OK val response = responseAs[WorkbenchEmail] - response.value should endWith(s"@myproject.iam.gserviceaccount.com") + response.value should endWith(s"${user.id.value}.iam.gserviceaccount.com") response.value } @@ -168,15 +168,17 @@ class GoogleExtensionRoutesV1Spec extends GoogleExtensionRoutesSpecHelper with S assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String) = createMockGoogleIamDaoForSAKeyTests + val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) + mockWithUser(user) + // create a pet service account Get("/api/google/v1/user/petServiceAccount/myproject") ~> routes.route ~> check { status shouldEqual StatusCodes.OK val response = responseAs[WorkbenchEmail] - response.value should endWith(s"@myproject.iam.gserviceaccount.com") + response.value should endWith(s"${user.id.value}.iam.gserviceaccount.com") } // create a pet service account key @@ -191,15 +193,17 @@ class GoogleExtensionRoutesV1Spec extends GoogleExtensionRoutesSpecHelper with S assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String) = createMockGoogleIamDaoForSAKeyTests + val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) + mockWithUser(user) + // create a pet service account Get("/api/google/v1/user/petServiceAccount/myproject") ~> routes.route ~> check { status shouldEqual StatusCodes.OK val response = responseAs[WorkbenchEmail] - response.value should endWith(s"@myproject.iam.gserviceaccount.com") + response.value should endWith(s"${user.id.value}.iam.gserviceaccount.com") } // create a pet service account key diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala index 918f47f994..50947a181f 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala @@ -9,8 +9,8 @@ import org.broadinstitute.dsde.workbench.RetryConfig import org.broadinstitute.dsde.workbench.dataaccess.NotificationDAO import org.broadinstitute.dsde.workbench.google.{GoogleDirectoryDAO, GoogleIamDAO, GoogleKmsService, GoogleProjectDAO, GooglePubSubDAO, GoogleStorageDAO} import org.broadinstitute.dsde.workbench.google2.{GcsBlobName, GoogleStorageService} -import org.broadinstitute.dsde.workbench.model.TraceId -import org.broadinstitute.dsde.workbench.model.google.GcsBucketName +import org.broadinstitute.dsde.workbench.model.{PetServiceAccountId, TraceId} +import org.broadinstitute.dsde.workbench.model.google.{GcsBucketName, GoogleProject, ServiceAccountName} import org.broadinstitute.dsde.workbench.sam.Generator.{ genFirecloudEmail, genGcsBlobName, @@ -30,7 +30,7 @@ import org.broadinstitute.dsde.workbench.sam.model.{ResourceType, ResourceTypeNa import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext import org.mockito.ArgumentMatchersSugar._ import org.mockito.IdiomaticMockito -import org.mockito.Mockito.{RETURNS_SMART_NULLS, clearInvocations, doReturn, verifyNoInteractions} +import org.mockito.Mockito.{RETURNS_SMART_NULLS, clearInvocations, doReturn, times, verifyNoInteractions} import org.mockito.MockitoSugar.verify import org.scalatest.concurrent.ScalaFutures import org.scalatest.freespec.AnyFreeSpecLike @@ -172,7 +172,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) doReturn(IO.pure(arbitraryPetServiceAccountKey)) .when(googleExtensions) - .getArbitraryPetServiceAccountKey(eqTo(newGoogleUser), any[SamRequestContext]) + .getSingletonPetServiceAccountKeyForUser(eqTo(newGoogleUser), eqTo(None), any[SamRequestContext]) "includes the requester pays user project if provided" in { runAndWait( @@ -222,6 +222,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) val mockDirectoryDAO = mock[DirectoryDAO](RETURNS_SMART_NULLS) val mockGoogleKeyCache = mock[GoogleKeyCache](RETURNS_SMART_NULLS) + val mockGoogleIamDAO = mock[GoogleIamDAO](RETURNS_SMART_NULLS) val googleExtensions: GoogleExtensions = spy( new GoogleExtensions( @@ -232,7 +233,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) mock[GooglePubSubDAO](RETURNS_SMART_NULLS), mock[GooglePubSubDAO](RETURNS_SMART_NULLS), mock[GooglePubSubDAO](RETURNS_SMART_NULLS), - mock[GoogleIamDAO](RETURNS_SMART_NULLS), + mockGoogleIamDAO, mock[GoogleStorageDAO](RETURNS_SMART_NULLS), mock[GoogleProjectDAO](RETURNS_SMART_NULLS), mockGoogleKeyCache, @@ -250,6 +251,14 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) .when(mockDirectoryDAO) .loadSubjectFromEmail(eqTo(email), any[SamRequestContext]) + doReturn(IO.some(petServiceAccount)) + .when(mockDirectoryDAO) + .loadPetServiceAccount(any[PetServiceAccountId], any[SamRequestContext]) + + doReturn(Future.successful(Some(petServiceAccount.serviceAccount))) + .when(mockGoogleIamDAO) + .findServiceAccount(any[GoogleProject], any[ServiceAccountName]) + doReturn(IO.pure(petServiceAccount)) .when(googleExtensions) .createUserPetServiceAccount(eqTo(user), eqTo(googleProject), any[SamRequestContext]) @@ -258,9 +267,9 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) .when(googleExtensions) .getAccessTokenUsingJson(eqTo(expectedKey), eqTo(scopes)) - doReturn(Future.successful(expectedKey)) + doReturn(IO.pure(petServiceAccount)) .when(googleExtensions) - .getDefaultServiceAccountForShellProject(eqTo(user), any[SamRequestContext]) + .getSingletonPetServiceAccountForUser(eqTo(user), eqTo(None), any[SamRequestContext]) doReturn(IO.pure(expectedKey)) .when(mockGoogleKeyCache) @@ -323,25 +332,25 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) "gets a key for an email" in { clearInvocations(mockDirectoryDAO, googleExtensions, mockGoogleKeyCache) - val key = runAndWait(googleExtensions.getArbitraryPetServiceAccountKey(email, samRequestContext)) + val key = runAndWait(googleExtensions.getSingletonPetServiceAccountKeyForUser(email, None, samRequestContext)) key should be(Some(expectedKey)) verify(mockDirectoryDAO).loadSubjectFromEmail(eqTo(email), any[SamRequestContext]) - verifyNoInteractions(mockGoogleKeyCache) - verify(googleExtensions).getDefaultServiceAccountForShellProject(eqTo(user), any[SamRequestContext]) + verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) + verify(googleExtensions).getSingletonPetServiceAccountAndKeyForUser(eqTo(user), eqTo(None), any[SamRequestContext]) } "gets a key for a SamUser" in { clearInvocations(mockDirectoryDAO, googleExtensions, mockGoogleKeyCache) - val key = runAndWait(googleExtensions.getArbitraryPetServiceAccountKey(user, samRequestContext)) + val key = runAndWait(googleExtensions.getSingletonPetServiceAccountKeyForUser(user, None, samRequestContext)) key should be(expectedKey) - verifyNoInteractions(mockDirectoryDAO) - verifyNoInteractions(mockGoogleKeyCache) - verify(googleExtensions).getDefaultServiceAccountForShellProject(eqTo(user), any[SamRequestContext]) + verify(mockDirectoryDAO, times(0)).loadSubjectFromEmail(eqTo(email), any[SamRequestContext]) + verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) + verify(googleExtensions).getSingletonPetServiceAccountKeyForUser(eqTo(user), eqTo(None), any[SamRequestContext]) } } @@ -349,26 +358,26 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) "gets a token for an email" in { clearInvocations(mockDirectoryDAO, googleExtensions, mockGoogleKeyCache) - val token = runAndWait(googleExtensions.getArbitraryPetServiceAccountToken(email, scopes, samRequestContext)) + val token = runAndWait(googleExtensions.getSingletonPetServiceAccountToken(email, scopes, samRequestContext)) token should be(Some(expectedToken)) verify(mockDirectoryDAO).loadSubjectFromEmail(eqTo(email), any[SamRequestContext]) - verifyNoInteractions(mockGoogleKeyCache) - verify(googleExtensions).getDefaultServiceAccountForShellProject(eqTo(user), any[SamRequestContext]) + verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) + verify(googleExtensions).getSingletonPetServiceAccountAndKeyForUser(eqTo(user), eqTo(None), any[SamRequestContext]) verify(googleExtensions).getAccessTokenUsingJson(eqTo(expectedKey), eqTo(scopes)) } "gets a token for a SamUser" in { clearInvocations(mockDirectoryDAO, googleExtensions, mockGoogleKeyCache) - val token = runAndWait(googleExtensions.getArbitraryPetServiceAccountToken(user, scopes, samRequestContext)) + val token = runAndWait(googleExtensions.getSingletonPetServiceAccountToken(user, scopes, samRequestContext)) token should be(expectedToken) - verifyNoInteractions(mockDirectoryDAO) - verifyNoInteractions(mockGoogleKeyCache) - verify(googleExtensions).getDefaultServiceAccountForShellProject(eqTo(user), any[SamRequestContext]) + verify(mockDirectoryDAO, times(0)).loadSubjectFromEmail(eqTo(email), any[SamRequestContext]) + verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) + verify(googleExtensions).getSingletonPetServiceAccountAndKeyForUser(eqTo(user), eqTo(None), any[SamRequestContext]) verify(googleExtensions).getAccessTokenUsingJson(eqTo(expectedKey), eqTo(scopes)) } } From c37b8096e83f2e02a75d87f6a513dd1de828d74c Mon Sep 17 00:00:00 2001 From: tlangs Date: Mon, 16 Sep 2024 16:42:04 -0400 Subject: [PATCH 2/9] service agents need to be in terra org --- .../dsde/workbench/sam/google/GoogleExtensions.scala | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 98b8a23a66..82e47a5579 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -562,6 +562,10 @@ class GoogleExtensions( .map(psa => IO.pure(Option(psa.destinationProjectNumber))) .getOrElse(IO.fromFuture(IO(googleProjectDAO.getProjectNumber(destinationProject.value)))) .map(_.getOrElse(throw new WorkbenchException(s"Could not find project number for project ${destinationProject.value}"))) + ancestry <- IO.fromFuture(IO(googleProjectDAO.getAncestry(destinationProject.value))) + organization = ancestry.find(_.getResourceId.getType.equals("organization")).map(_.getResourceId) + _ <- IO.raiseWhen(organization.isEmpty)(new WorkbenchException(s"Project $destinationProject is not in an organization")) + _ <- IO.raiseUnless(organization.exists(_.getId.equals(googleServicesConfig.terraGoogleOrgNumber)))(new WorkbenchException(s"Project $destinationProject is not in organization ${googleServicesConfig.terraGoogleOrgNumber}")) _ <- serviceAgentsToAdd.toList.traverse { serviceAgentName => val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) for { From f97ccc52c068e9ea756ac7270f5a1907d0e7631f Mon Sep 17 00:00:00 2001 From: tlangs Date: Mon, 16 Sep 2024 16:45:10 -0400 Subject: [PATCH 3/9] scalafmt... --- .../dsde/workbench/sam/google/GoogleExtensions.scala | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 82e47a5579..2e5f67c519 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -565,7 +565,9 @@ class GoogleExtensions( ancestry <- IO.fromFuture(IO(googleProjectDAO.getAncestry(destinationProject.value))) organization = ancestry.find(_.getResourceId.getType.equals("organization")).map(_.getResourceId) _ <- IO.raiseWhen(organization.isEmpty)(new WorkbenchException(s"Project $destinationProject is not in an organization")) - _ <- IO.raiseUnless(organization.exists(_.getId.equals(googleServicesConfig.terraGoogleOrgNumber)))(new WorkbenchException(s"Project $destinationProject is not in organization ${googleServicesConfig.terraGoogleOrgNumber}")) + _ <- IO.raiseUnless(organization.exists(_.getId.equals(googleServicesConfig.terraGoogleOrgNumber)))( + new WorkbenchException(s"Project $destinationProject is not in organization ${googleServicesConfig.terraGoogleOrgNumber}") + ) _ <- serviceAgentsToAdd.toList.traverse { serviceAgentName => val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) for { From 0ba176c49d125d4807b9bbaad3e459a963ed4c00 Mon Sep 17 00:00:00 2001 From: tlangs Date: Tue, 17 Sep 2024 10:16:39 -0400 Subject: [PATCH 4/9] Correct error status code --- .../dsde/workbench/sam/google/GoogleExtensions.scala | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 2e5f67c519..1761e7a79c 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -561,10 +561,18 @@ class GoogleExtensions( destinationProjectNumber <- petServiceAgents .map(psa => IO.pure(Option(psa.destinationProjectNumber))) .getOrElse(IO.fromFuture(IO(googleProjectDAO.getProjectNumber(destinationProject.value)))) - .map(_.getOrElse(throw new WorkbenchException(s"Could not find project number for project ${destinationProject.value}"))) + .map( + _.getOrElse( + throw new WorkbenchExceptionWithErrorReport( + ErrorReport(StatusCodes.BadRequest, s"Could not find project number for project ${destinationProject.value}") + ) + ) + ) ancestry <- IO.fromFuture(IO(googleProjectDAO.getAncestry(destinationProject.value))) organization = ancestry.find(_.getResourceId.getType.equals("organization")).map(_.getResourceId) - _ <- IO.raiseWhen(organization.isEmpty)(new WorkbenchException(s"Project $destinationProject is not in an organization")) + _ <- IO.raiseWhen(organization.isEmpty)( + new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, s"Project $destinationProject is not in an organization")) + ) _ <- IO.raiseUnless(organization.exists(_.getId.equals(googleServicesConfig.terraGoogleOrgNumber)))( new WorkbenchException(s"Project $destinationProject is not in organization ${googleServicesConfig.terraGoogleOrgNumber}") ) From 4bcbd519e00d270848b72d7db145df6c9fd0d7cc Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 19 Sep 2024 10:14:09 -0400 Subject: [PATCH 5/9] unit tests pass --- .../dsde/workbench/sam/api/AdminRoutes.scala | 7 +- .../sam/google/GoogleExtensionRoutes.scala | 5 +- .../sam/google/GoogleExtensions.scala | 87 ++-- .../sam/service/CloudExtensions.scala | 4 + src/test/resources/sam.conf | 6 +- .../dsde/workbench/sam/TestSupport.scala | 7 + .../google/GoogleExtensionRoutesSpec.scala | 6 +- .../sam/google/GoogleExtensionSpec.scala | 381 ++++++++++++------ .../sam/google/NewGoogleExtensionsSpec.scala | 51 ++- .../service/MockCloudExtensionsBuilder.scala | 1 + 10 files changed, 396 insertions(+), 159 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala index 1b6fe82ab2..346e7dfc17 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/api/AdminRoutes.scala @@ -122,9 +122,10 @@ trait AdminRoutes extends SecurityDirectives with SamRequestContextDirectives wi val googleProject = GoogleProject(project) deleteWithTelemetry(samRequestContext, userIdParam(workbenchUserId), googleProjectParam(googleProject)) { complete { - cloudExtensions - .deleteUserPetServiceAccount(workbenchUserId, googleProject, samRequestContext) - .map(_ => NoContent) + for { + _ <- cloudExtensions.removeProjectServiceAgents(workbenchUserId, GoogleProject(project), samRequestContext) + _ <- cloudExtensions.deleteUserPetServiceAccount(workbenchUserId, GoogleProject(project), samRequestContext) + } yield StatusCodes.NoContent } } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala index 948ddfdbdc..607a533bee 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutes.scala @@ -227,7 +227,10 @@ trait GoogleExtensionRoutes extends ExtensionRoutes with SamUserDirectives with } ~ deleteWithTelemetry(samRequestContext, "googleProject" -> projectResourceId) { complete { - googleExtensions.deleteUserPetServiceAccount(samUser.id, GoogleProject(project), samRequestContext).map(_ => StatusCodes.NoContent) + for { + _ <- googleExtensions.removeProjectServiceAgents(samUser.id, GoogleProject(project), samRequestContext) + _ <- googleExtensions.deleteUserPetServiceAccount(samUser.id, GoogleProject(project), samRequestContext) + } yield StatusCodes.NoContent } } } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 1761e7a79c..ae49bfc7d5 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -343,7 +343,16 @@ class GoogleExtensions( } } yield deletedSomething - def createUserPetServiceAccount(user: SamUser, project: GoogleProject, samRequestContext: SamRequestContext): IO[PetServiceAccount] = { + def removeProjectServiceAgents(userId: WorkbenchUserId, project: GoogleProject, samRequestContext: SamRequestContext): IO[Boolean] = + for { + maybePet <- directoryDAO.loadPetServiceAccount(PetServiceAccountId(userId, projectForSingletonServiceAccount(userId)), samRequestContext) + removedSomething <- maybePet match { + case Some(pet) => removeProjectServiceAgentsFromPet(userId, pet, project, samRequestContext).map(_ => true) + case None => IO.pure(false) + } + } yield removedSomething + + private[google] def createUserPetServiceAccount(user: SamUser, project: GoogleProject, samRequestContext: SamRequestContext): IO[PetServiceAccount] = { val (petSaName, petSaDisplayName) = toPetSAFromUser(user) // The normal situation is that the pet either exists in both the database and google or neither. // Sometimes, especially in tests, the pet may be removed from the database, but not google or the other way around. @@ -450,16 +459,10 @@ class GoogleExtensions( def getPetServiceAccountKey(user: SamUser, project: GoogleProject, samRequestContext: SamRequestContext): IO[String] = for { - pet <- createUserPetServiceAccount(user, project, samRequestContext) + pet <- getSingletonPetServiceAccountForUser(user, Some(project), samRequestContext) key <- googleKeyCache.getKey(pet) } yield key - def getPetServiceAccountAndKey(user: SamUser, project: GoogleProject, samRequestContext: SamRequestContext): IO[(PetServiceAccount, String)] = - for { - pet <- createUserPetServiceAccount(user, project, samRequestContext) - key <- googleKeyCache.getKey(pet) - } yield (pet, key) - def getPetServiceAccountToken(user: SamUser, project: GoogleProject, scopes: Set[String], samRequestContext: SamRequestContext): IO[String] = getPetServiceAccountKey(user, project, samRequestContext).flatMap { key => IO.fromFuture(IO(getAccessTokenUsingJson(key, scopes))) @@ -522,18 +525,23 @@ class GoogleExtensions( opt <- getSingletonPetServiceAccountAndKeyForUser(userEmail, destinationProject, samRequestContext) } yield opt.map(_._2) + private[google] def projectForSingletonServiceAccount(samUser: SamUser): GoogleProject = + projectForSingletonServiceAccount(samUser.id) + + private[google] def projectForSingletonServiceAccount(userId: WorkbenchUserId): GoogleProject = + GoogleProject(s"fc-${googleServicesConfig.environment.substring(0, Math.min(googleServicesConfig.environment.length(), 5))}-${userId.value}") + private[google] def getSingletonPetServiceAccountForUser( user: SamUser, destinationProject: Option[GoogleProject], samRequestContext: SamRequestContext ): IO[PetServiceAccount] = { - val projectName = - s"fc-${googleServicesConfig.environment.substring(0, Math.min(googleServicesConfig.environment.length(), 5))}-${user.id.value}" // max 30 characters. subject ID is 21 + val project = projectForSingletonServiceAccount(user) for { creationOperationId <- IO.fromFuture { IO { googleProjectDAO - .createProject(projectName, googleServicesConfig.terraGoogleOrgNumber, GoogleResourceTypes.Organization) + .createProject(project.value, googleServicesConfig.terraGoogleOrgNumber, GoogleResourceTypes.Organization) .map(opId => Option(opId)) recover { case gjre: GoogleJsonResponseException if gjre.getDetails.getCode == StatusCodes.Conflict.intValue => None } @@ -543,11 +551,51 @@ class GoogleExtensions( case Some(opId) => IO.fromFuture(IO(pollShellProjectCreation(opId))) // poll until it's created case None => IO.unit } - sa <- createUserPetServiceAccount(user, GoogleProject(projectName), samRequestContext) + sa <- createUserPetServiceAccount(user, project, samRequestContext) _ <- destinationProject.map(p => syncServiceAgents(user, sa, p, samRequestContext)).getOrElse(IO.unit) } yield sa } + private def removeProjectServiceAgentsFromPet( + userId: WorkbenchUserId, + petServiceAccount: PetServiceAccount, + destinationProject: GoogleProject, + samRequestContext: SamRequestContext + ): IO[Unit] = + for { + petServiceAgents <- directoryDAO.getPetServiceAgents(userId, petServiceAccount.id.project, destinationProject, samRequestContext) + existingServiceAgents = petServiceAgents.map(_.serviceAgents.map(_.name)).getOrElse(Set.empty) + _ <- assertProjectInTerraOrg(destinationProject) + _ <- assertProjectIsActive(destinationProject) + destinationProjectNumber <- petServiceAgents + .map(psa => IO.pure(Option(psa.destinationProjectNumber))) + .getOrElse(IO.fromFuture(IO(googleProjectDAO.getProjectNumber(destinationProject.value)))) + .map( + _.getOrElse( + throw new WorkbenchExceptionWithErrorReport( + ErrorReport(StatusCodes.BadRequest, s"Could not find project number for project ${destinationProject.value}") + ) + ) + ) + _ <- existingServiceAgents.toList.traverse { serviceAgentName => + // val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) + for { + // removeIamPolicyBindingOnServiceAccount is not implemented in googleIamDAO yet + // TODO do it + // _ <- googleIamDAO.removeIamPolicyBindingOnServiceAccount(petServiceAccount.id.project, petServiceAccount.serviceAccount.email, serviceAgent.email, Set("roles/iam.serviceAccountTokenCreator")) + _ <- directoryDAO.removePetServiceAgent( + userId, + petServiceAccount.id.project, + destinationProject, + destinationProjectNumber, + serviceAgentName, + samRequestContext + ) + } yield () + } + + } yield () + private def syncServiceAgents( user: SamUser, petServiceAccount: PetServiceAccount, @@ -558,6 +606,8 @@ class GoogleExtensions( petServiceAgents <- directoryDAO.getPetServiceAgents(user.id, petServiceAccount.id.project, destinationProject, samRequestContext) existingServiceAgents = petServiceAgents.map(_.serviceAgents.map(_.name)).getOrElse(Set.empty) serviceAgentsToAdd = googleServicesConfig.serviceAgents -- existingServiceAgents + _ <- assertProjectInTerraOrg(destinationProject) + _ <- assertProjectIsActive(destinationProject) destinationProjectNumber <- petServiceAgents .map(psa => IO.pure(Option(psa.destinationProjectNumber))) .getOrElse(IO.fromFuture(IO(googleProjectDAO.getProjectNumber(destinationProject.value)))) @@ -568,14 +618,6 @@ class GoogleExtensions( ) ) ) - ancestry <- IO.fromFuture(IO(googleProjectDAO.getAncestry(destinationProject.value))) - organization = ancestry.find(_.getResourceId.getType.equals("organization")).map(_.getResourceId) - _ <- IO.raiseWhen(organization.isEmpty)( - new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, s"Project $destinationProject is not in an organization")) - ) - _ <- IO.raiseUnless(organization.exists(_.getId.equals(googleServicesConfig.terraGoogleOrgNumber)))( - new WorkbenchException(s"Project $destinationProject is not in organization ${googleServicesConfig.terraGoogleOrgNumber}") - ) _ <- serviceAgentsToAdd.toList.traverse { serviceAgentName => val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) for { @@ -634,7 +676,7 @@ class GoogleExtensions( token <- IO.fromFuture(IO(getAccessTokenUsingJson(key, scopes))) } yield token - private def pollShellProjectCreation(operationId: String): Future[Boolean] = { + private[google] def pollShellProjectCreation(operationId: String): Future[Boolean] = { def whenCreating(throwable: Throwable): Boolean = throwable match { case t: WorkbenchException => throw t @@ -846,8 +888,7 @@ class GoogleExtensions( ): IO[URL] = { val urlParamsMap: Map[String, String] = if (requesterPays) Map(userProjectQueryParam -> project.value) else Map.empty for { - petServiceAccount <- createUserPetServiceAccount(samUser, project, samRequestContext) - petKey <- googleKeyCache.getKey(petServiceAccount) + petKey <- getSingletonPetServiceAccountKeyForUser(samUser, Some(project), samRequestContext) serviceAccountCredentials = ServiceAccountCredentials.fromStream(new ByteArrayInputStream(petKey.getBytes())) url <- getSignedUrl(samUser, bucket, name, duration, urlParamsMap, serviceAccountCredentials) } yield url diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/CloudExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/CloudExtensions.scala index ce96e83d8a..e1b2b1f1c5 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/CloudExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/CloudExtensions.scala @@ -53,6 +53,8 @@ trait CloudExtensions { def deleteUserPetServiceAccount(userId: WorkbenchUserId, project: GoogleProject, samRequestContext: SamRequestContext): IO[Boolean] + def removeProjectServiceAgents(userId: WorkbenchUserId, project: GoogleProject, samRequestContext: SamRequestContext): IO[Boolean] + def getUserProxy(userEmail: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[WorkbenchEmail]] def fireAndForgetNotifications[T <: Notification](notifications: Set[T]): Unit @@ -100,6 +102,8 @@ trait NoExtensions extends CloudExtensions { override def deleteUserPetServiceAccount(userId: WorkbenchUserId, project: GoogleProject, samRequestContext: SamRequestContext): IO[Boolean] = IO.pure(true) + override def removeProjectServiceAgents(userId: WorkbenchUserId, project: GoogleProject, samRequestContext: SamRequestContext): IO[Boolean] = IO.pure(true) + override def getUserProxy(userEmail: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[WorkbenchEmail]] = IO.pure(Option(userEmail)) diff --git a/src/test/resources/sam.conf b/src/test/resources/sam.conf index bf30396db9..b98404f3aa 100644 --- a/src/test/resources/sam.conf +++ b/src/test/resources/sam.conf @@ -6,6 +6,10 @@ admin { serviceAccountAdmins = ${?SERVICE_ACCOUNT_ADMINS} } +googleServices { + serviceAgents = "compute-service" +} + resourceAccessPolicies { resource_type_admin { workspace { @@ -26,4 +30,4 @@ resourceAccessPolicies { } } } -} \ No newline at end of file +} diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala index 1a098dd02b..32d532d704 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala @@ -13,6 +13,7 @@ import org.broadinstitute.dsde.workbench.google.mock._ import org.broadinstitute.dsde.workbench.google.{GoogleDirectoryDAO, GoogleIamDAO, GoogleProjectDAO} import org.broadinstitute.dsde.workbench.google2.mock.FakeGoogleStorageInterpreter import org.broadinstitute.dsde.workbench.model._ +import org.broadinstitute.dsde.workbench.model.google.GoogleProject import org.broadinstitute.dsde.workbench.oauth2.OpenIDConnectConfiguration import org.broadinstitute.dsde.workbench.oauth2.mock.FakeOpenIDConnectConfiguration import org.broadinstitute.dsde.workbench.sam.api._ @@ -285,6 +286,12 @@ object TestSupport extends TestSupport { val enabledMapNoTosAccepted = Map("ldap" -> true, "allUsersGroup" -> true, "google" -> true, "tosAccepted" -> false, "adminEnabled" -> true) val enabledMapTosAccepted = Map("ldap" -> true, "allUsersGroup" -> true, "google" -> true, "tosAccepted" -> true, "adminEnabled" -> true) + + def singletonServiceAccountForUser(user: SamUser) = + s"pet-${user.id.value}@${singletonServiceAccountProject(user).value}.iam.gserviceaccount.com" + + def singletonServiceAccountProject(user: SamUser): GoogleProject = + GoogleProject(s"fc-${googleServicesConfig.environment.substring(0, Math.min(googleServicesConfig.environment.length(), 5))}-${user.id.value}") } final case class SamDependencies( diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala index 3c301fc524..52b088c5b7 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala @@ -464,9 +464,6 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca googleIamDAO } - def singletonServiceAccountForUser(user: SamUser) = - s"pet-${user.id.value}@fc-${googleServicesConfig.environment.substring(0, Math.min(googleServicesConfig.environment.length(), 5))}-${user.id.value}.iam.gserviceaccount.com" - def createMockGoogleIamDaoForSAKeyTests: (GoogleIamDAO, String, SamUser => Unit) = { val googleIamDAO = mock[GoogleIamDAO](RETURNS_SMART_NULLS) val expectedJson = """{"json":"yes I am"}""" @@ -493,6 +490,9 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca lenient() .when(googleIamDAO.addServiceAccountUserRoleForUser(any[GoogleProject], any[WorkbenchEmail], any[WorkbenchEmail])) .thenReturn(Future.successful(())) + lenient + .when(googleIamDAO.addIamPolicyBindingOnServiceAccount(any[GoogleProject], any[WorkbenchEmail], any[WorkbenchEmail], any[Set[String]])) + .thenReturn(Future.successful(())) (googleIamDAO, expectedJson, mockWithUser) } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala index 3456f2e38f..e0924392ce 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala @@ -19,7 +19,13 @@ import org.broadinstitute.dsde.workbench.google2.mock.FakeGoogleStorageInterpret import org.broadinstitute.dsde.workbench.model.Notifications.NotificationFormat import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.model.google.GoogleProject -import org.broadinstitute.dsde.workbench.sam.TestSupport.{databaseEnabled, databaseEnabledClue, truncateAll} +import org.broadinstitute.dsde.workbench.sam.TestSupport.{ + databaseEnabled, + databaseEnabledClue, + singletonServiceAccountForUser, + singletonServiceAccountProject, + truncateAll +} import org.broadinstitute.dsde.workbench.sam.dataAccess._ import org.broadinstitute.dsde.workbench.sam.mock.RealKeyMockGoogleIamDAO import org.broadinstitute.dsde.workbench.sam.model._ @@ -413,11 +419,18 @@ class GoogleExtensionSpec(_system: ActorSystem) // create a pet service account val googleProject = GoogleProject("testproject") - val petServiceAccount = googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() + + petServiceAccount.serviceAccount.email.value should equal(singletonServiceAccountForUser(defaultUser)) + + dirDAO + .loadPetServiceAccount(PetServiceAccountId(defaultUser.id, singletonServiceAccountProject(defaultUser)), samRequestContext) + .unsafeRunSync() shouldBe Some(petServiceAccount) - petServiceAccount.serviceAccount.email.value should endWith(s"@${googleProject.value}.iam.gserviceaccount.com") + val serviceAgents = + dirDAO.getPetServiceAgents(defaultUser.id, singletonServiceAccountProject(defaultUser), googleProject, samRequestContext).unsafeRunSync() - dirDAO.loadPetServiceAccount(PetServiceAccountId(defaultUser.id, googleProject), samRequestContext).unsafeRunSync() shouldBe Some(petServiceAccount) + serviceAgents.map(_.serviceAgents.map(_.name)).get should be(googleServicesConfig.serviceAgents) // verify google mockGoogleIamDAO.serviceAccounts should contain key petServiceAccount.serviceAccount.email @@ -425,11 +438,16 @@ class GoogleExtensionSpec(_system: ActorSystem) mockGoogleDirectoryDAO.groups(defaultUserProxyEmail) shouldBe Set(defaultUser.email, petServiceAccount.serviceAccount.email) // create one again, it should work - val petSaResponse2 = googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + val petSaResponse2 = googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() petSaResponse2 shouldBe petServiceAccount - // delete the pet service account - googleExtensions.deleteUserPetServiceAccount(newUser.userInfo.userSubjectId, googleProject, samRequestContext).unsafeRunSync() shouldBe true + // should not delete the pet service account if its project-specific (it doesn't exist) + googleExtensions.deleteUserPetServiceAccount(newUser.userInfo.userSubjectId, googleProject, samRequestContext).unsafeRunSync() shouldBe false + + // should delete the pet service account if its the singleton project + googleExtensions + .deleteUserPetServiceAccount(newUser.userInfo.userSubjectId, singletonServiceAccountProject(defaultUser), samRequestContext) + .unsafeRunSync() shouldBe true // the user should still exist in DB dirDAO.loadUser(defaultUser.id, samRequestContext).unsafeRunSync() shouldBe Some(defaultUser.copy(enabled = true)) @@ -451,26 +469,34 @@ class GoogleExtensionSpec(_system: ActorSystem) val mockGoogleDirectoryDAO = new MockGoogleDirectoryDAO val mockGoogleProjectDAO = new MockGoogleProjectDAO - val googleExtensions = new GoogleExtensions( - TestSupport.distributedLock, - dirDAO, - null, - mockGoogleDirectoryDAO, - null, - null, - null, - mockGoogleIamDAO, - null, - mockGoogleProjectDAO, - null, - null, - null, - null, - googleServicesConfig, - petServiceAccountConfig, - configResourceTypes, - superAdminsGroup + val googleExtensions = spy( + new GoogleExtensions( + TestSupport.distributedLock, + dirDAO, + null, + mockGoogleDirectoryDAO, + null, + null, + null, + mockGoogleIamDAO, + null, + mockGoogleProjectDAO, + null, + null, + null, + null, + googleServicesConfig, + petServiceAccountConfig, + configResourceTypes, + superAdminsGroup + ), + lenient = true ) + + doReturn(Future.successful(true)) + .when(googleExtensions) + .pollShellProjectCreation(any[String]) + val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig) val service = new UserService(dirDAO, googleExtensions, Seq.empty, tosService) @@ -504,18 +530,72 @@ class GoogleExtensionSpec(_system: ActorSystem) val googleProject = GoogleProject("testproject") val (saName, saDisplayName) = googleExtensions.toPetSAFromUser(defaultUser) - val serviceAccount = mockGoogleIamDAO.createServiceAccount(googleProject, saName, saDisplayName).futureValue + val serviceAccount = mockGoogleIamDAO.createServiceAccount(singletonServiceAccountProject(defaultUser), saName, saDisplayName).futureValue // create a user val newUser = newUserWithAcceptedTos(service, tosService, defaultUser, samRequestContext) newUser shouldBe UserStatus(UserStatusDetails(defaultUser.id, defaultUser.email), TestSupport.enabledMapTosAccepted) // create a pet service account - val petServiceAccount = googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() petServiceAccount.serviceAccount shouldBe serviceAccount } + it should "add a project's service agents to the singleton service account" in { + assume(databaseEnabled, databaseEnabledClue) + + val ( + dirDAO: DirectoryDAO, + tosService: TosService, + mockGoogleIamDAO: MockGoogleIamDAO, + mockGoogleDirectoryDAO: MockGoogleDirectoryDAO, + googleExtensions: GoogleExtensions, + service: UserService, + defaultUserProxyEmail: WorkbenchEmail, + defaultUser: SamUser + ) = initPetTest + + // create a user + val newUser = newUserWithAcceptedTos(service, tosService, defaultUser, samRequestContext) + newUser shouldBe UserStatus(UserStatusDetails(defaultUser.id, defaultUser.email), TestSupport.enabledMapTosAccepted) + + // create a pet service account + val googleProject2 = GoogleProject("testproject2") + val googleProject = GoogleProject("testproject") + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, None, samRequestContext).unsafeRunSync() + + petServiceAccount.serviceAccount.email.value should equal(singletonServiceAccountForUser(defaultUser)) + + dirDAO + .loadPetServiceAccount(PetServiceAccountId(defaultUser.id, singletonServiceAccountProject(defaultUser)), samRequestContext) + .unsafeRunSync() shouldBe Some(petServiceAccount) + + val noServiceAgents = + dirDAO.getPetServiceAgents(defaultUser.id, singletonServiceAccountProject(defaultUser), googleProject, samRequestContext).unsafeRunSync() + + noServiceAgents should be(None) + + val projectPetServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() + projectPetServiceAccount should be(petServiceAccount) + + val oneServiceAgents = + dirDAO.getPetServiceAgents(defaultUser.id, singletonServiceAccountProject(defaultUser), googleProject, samRequestContext).unsafeRunSync() + + oneServiceAgents should not be empty + oneServiceAgents.get.destinationProject should be(googleProject) + oneServiceAgents.get.serviceAgents.map(_.name) should be(googleServicesConfig.serviceAgents) + + googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject2), samRequestContext).unsafeRunSync() + projectPetServiceAccount should be(petServiceAccount) + + val anotherServiceAgents = + dirDAO.getPetServiceAgents(defaultUser.id, singletonServiceAccountProject(defaultUser), googleProject2, samRequestContext).unsafeRunSync() + anotherServiceAgents should not be empty + anotherServiceAgents.get.destinationProject should be(googleProject2) + anotherServiceAgents.get.serviceAgents.map(_.name) should be(googleServicesConfig.serviceAgents) + } + it should "recreate service account when missing for pet" in { assume(databaseEnabled, databaseEnabledClue) @@ -536,13 +616,17 @@ class GoogleExtensionSpec(_system: ActorSystem) // create a pet service account val googleProject = GoogleProject("testproject") - val petServiceAccount = googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() + + // should have service agents for the google project attached + val serviceAgents = dirDAO.getPetServiceAgents(defaultUser.id, petServiceAccount.id.project, googleProject, samRequestContext).unsafeRunSync() + serviceAgents.map(_.serviceAgents).getOrElse(Set.empty).map(_.name) should be(googleServicesConfig.serviceAgents) import org.broadinstitute.dsde.workbench.model.google.toAccountName - mockGoogleIamDAO.removeServiceAccount(googleProject, toAccountName(petServiceAccount.serviceAccount.email)).futureValue - mockGoogleIamDAO.findServiceAccount(googleProject, petServiceAccount.serviceAccount.email).futureValue shouldBe None + mockGoogleIamDAO.removeServiceAccount(singletonServiceAccountProject(defaultUser), toAccountName(petServiceAccount.serviceAccount.email)).futureValue + mockGoogleIamDAO.findServiceAccount(singletonServiceAccountProject(defaultUser), petServiceAccount.serviceAccount.email).futureValue shouldBe None - val petServiceAccount2 = googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + val petServiceAccount2 = googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() petServiceAccount.serviceAccount shouldNot equal(petServiceAccount2.serviceAccount) val res = dirDAO.loadPetServiceAccount(petServiceAccount.id, samRequestContext).unsafeRunSync() res shouldBe Some(petServiceAccount2) @@ -1246,26 +1330,34 @@ class GoogleExtensionSpec(_system: ActorSystem) petServiceAccountConfig ) - val googleExtensions = new GoogleExtensions( - TestSupport.distributedLock, - dirDAO, - null, - mockGoogleDirectoryDAO, - mockGoogleNotificationPubSubDAO, - null, - null, - mockGoogleIamDAO, - mockGoogleStorageDAO, - mockGoogleProjectDAO, - googleKeyCache, - notificationDAO, - null, - FakeGoogleStorageInterpreter, - googleServicesConfig, - petServiceAccountConfig, - configResourceTypes, - superAdminsGroup + val googleExtensions: GoogleExtensions = spy( + new GoogleExtensions( + TestSupport.distributedLock, + dirDAO, + null, + mockGoogleDirectoryDAO, + mockGoogleNotificationPubSubDAO, + null, + null, + mockGoogleIamDAO, + mockGoogleStorageDAO, + mockGoogleProjectDAO, + googleKeyCache, + notificationDAO, + null, + FakeGoogleStorageInterpreter, + googleServicesConfig, + petServiceAccountConfig, + configResourceTypes, + superAdminsGroup + ), + lenient = true ) + + doReturn(Future.successful(true)) + .when(googleExtensions) + .pollShellProjectCreation(any[String]) + val tosService = new TosService(NoExtensions, dirDAO, TestSupport.tosConfig) val service = new UserService(dirDAO, googleExtensions, Seq.empty, tosService) @@ -1288,7 +1380,7 @@ class GoogleExtensionSpec(_system: ActorSystem) // create a pet service account val googleProject = GoogleProject("testproject") - val petServiceAccount = googleExtensions.createUserPetServiceAccount(createDefaultUser, googleProject, samRequestContext).unsafeRunSync() + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(createDefaultUser, Some(googleProject), samRequestContext).unsafeRunSync() // get a key, which should create a brand new one val firstKey = googleExtensions.getPetServiceAccountKey(createDefaultUser, googleProject, samRequestContext).unsafeRunSync() @@ -1297,6 +1389,34 @@ class GoogleExtensionSpec(_system: ActorSystem) assert(firstKey == secondKey) } + it should "add a service agent when a project-specific pet service account is requested" in { + assume(databaseEnabled, databaseEnabledClue) + + implicit val patienceConfig = PatienceConfig(1 second) + val (googleExtensions, service, tosService) = setupGoogleKeyCacheTests + + val createDefaultUser = Generator.genWorkbenchUserGoogle.sample.get + // create a user + val newUser = newUserWithAcceptedTos(service, tosService, createDefaultUser, samRequestContext) + newUser shouldBe UserStatus( + UserStatusDetails(createDefaultUser.id, createDefaultUser.email), + TestSupport.enabledMapTosAccepted + ) + + // create a pet service account + val googleProject = GoogleProject("testproject") + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(createDefaultUser, Some(googleProject), samRequestContext).unsafeRunSync() + + // get a key, which should add service agents to the singleton pet service account + googleExtensions.getPetServiceAccountKey(createDefaultUser, googleProject, samRequestContext).unsafeRunSync() + + val petServiceAgents = + service.directoryDAO.getPetServiceAgents(createDefaultUser.id, petServiceAccount.id.project, googleProject, samRequestContext).unsafeRunSync() + + petServiceAgents should not be empty + petServiceAgents.get.destinationProject should be(googleProject) + } + it should "remove an existing key and then return a brand new one" in { assume(databaseEnabled, databaseEnabledClue) @@ -1313,17 +1433,19 @@ class GoogleExtensionSpec(_system: ActorSystem) // create a pet service account val googleProject = GoogleProject("testproject") - val petServiceAccount = googleExtensions.createUserPetServiceAccount(createDefaultUser, googleProject, samRequestContext).unsafeRunSync() + val singletonGoogleProject = singletonServiceAccountProject(createDefaultUser) + + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(createDefaultUser, Some(googleProject), samRequestContext).unsafeRunSync() // get a key, which should create a brand new one val firstKey = googleExtensions.getPetServiceAccountKey(createDefaultUser, googleProject, samRequestContext).unsafeRunSync() // remove the key we just created runAndWait(for { - keys <- googleExtensions.googleIamDAO.listServiceAccountKeys(googleProject, petServiceAccount.serviceAccount.email) + keys <- googleExtensions.googleIamDAO.listServiceAccountKeys(singletonGoogleProject, petServiceAccount.serviceAccount.email) _ <- keys.toList .parTraverse { key => - googleExtensions.removePetServiceAccountKey(createDefaultUser.id, googleProject, key.id, samRequestContext) + googleExtensions.removePetServiceAccountKey(createDefaultUser.id, singletonGoogleProject, key.id, samRequestContext) } .unsafeToFuture() } yield ()) @@ -1350,7 +1472,8 @@ class GoogleExtensionSpec(_system: ActorSystem) // create a pet service account val googleProject = GoogleProject("testproject") - val petServiceAccount = googleExtensions.createUserPetServiceAccount(createDefaultUser, googleProject, samRequestContext).unsafeRunSync() + val singletonGoogleProject = singletonServiceAccountProject(createDefaultUser) + val petServiceAccount = googleExtensions.getSingletonPetServiceAccountForUser(createDefaultUser, Some(googleProject), samRequestContext).unsafeRunSync() // get a key, which should create a brand new one val firstKey = googleExtensions.getPetServiceAccountKey(createDefaultUser, googleProject, samRequestContext).unsafeRunSync() @@ -1359,7 +1482,7 @@ class GoogleExtensionSpec(_system: ActorSystem) val removedKeyObjects = (for { keyObject <- googleExtensions.googleKeyCache.googleStorageAlg.listObjectsWithPrefix( googleExtensions.googleServicesConfig.googleKeyCacheConfig.bucketName, - googleExtensions.googleKeyCache.keyNamePrefix(googleProject, petServiceAccount.serviceAccount.email) + googleExtensions.googleKeyCache.keyNamePrefix(singletonGoogleProject, petServiceAccount.serviceAccount.email) ) _ <- googleExtensions.googleKeyCache.googleStorageAlg.removeObject( googleExtensions.googleServicesConfig.googleKeyCacheConfig.bucketName, @@ -1379,7 +1502,8 @@ class GoogleExtensionSpec(_system: ActorSystem) // assert that keys have been removed from service account assert(removedKeyObjects.forall { removed => - val existingKeys = runAndWait(googleExtensions.googleIamDAO.listUserManagedServiceAccountKeys(googleProject, petServiceAccount.serviceAccount.email)) + val existingKeys = + runAndWait(googleExtensions.googleIamDAO.listUserManagedServiceAccountKeys(singletonGoogleProject, petServiceAccount.serviceAccount.email)) !existingKeys.exists(key => removed.value.endsWith(key.id.value)) }) @@ -1992,32 +2116,39 @@ class GoogleExtensionSpec(_system: ActorSystem) val mockGoogleDirectoryDAO = new MockGoogleDirectoryDAO val mockGoogleProjectDAO = new MockGoogleProjectDAO val garbageOrgGoogleServicesConfig = TestSupport.googleServicesConfig.copy(terraGoogleOrgNumber = "garbage") - val googleExtensions = new GoogleExtensions( - TestSupport.distributedLock, - dirDAO, - null, - mockGoogleDirectoryDAO, - null, - null, - null, - mockGoogleIamDAO, - null, - mockGoogleProjectDAO, - null, - null, - null, - null, - garbageOrgGoogleServicesConfig, - petServiceAccountConfig, - configResourceTypes, - superAdminsGroup + val googleExtensions = spy( + new GoogleExtensions( + TestSupport.distributedLock, + dirDAO, + null, + mockGoogleDirectoryDAO, + null, + null, + null, + mockGoogleIamDAO, + null, + mockGoogleProjectDAO, + null, + null, + null, + null, + garbageOrgGoogleServicesConfig, + petServiceAccountConfig, + configResourceTypes, + superAdminsGroup + ), + lenient = true ) + doReturn(Future.successful(true)) + .when(googleExtensions) + .pollShellProjectCreation(any[String]) + val defaultUser = Generator.genWorkbenchUserBoth.sample.get val googleProject = GoogleProject("testproject") val report = intercept[WorkbenchExceptionWithErrorReport] { - googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() } report.errorReport.statusCode shouldEqual Some(StatusCodes.BadRequest) @@ -2036,32 +2167,39 @@ class GoogleExtensionSpec(_system: ActorSystem) override def getAncestry(projectName: String): Future[Seq[Ancestor]] = Future.failed(new HttpResponseException.Builder(403, "Made up error message", new HttpHeaders()).build()) } - val googleExtensions = new GoogleExtensions( - TestSupport.distributedLock, - dirDAO, - null, - mockGoogleDirectoryDAO, - null, - null, - null, - mockGoogleIamDAO, - null, - mockGoogleProjectDAO, - null, - null, - null, - null, - googleServicesConfig, - petServiceAccountConfig, - configResourceTypes, - superAdminsGroup + val googleExtensions = spy( + new GoogleExtensions( + TestSupport.distributedLock, + dirDAO, + null, + mockGoogleDirectoryDAO, + null, + null, + null, + mockGoogleIamDAO, + null, + mockGoogleProjectDAO, + null, + null, + null, + null, + googleServicesConfig, + petServiceAccountConfig, + configResourceTypes, + superAdminsGroup + ), + lenient = true ) + doReturn(Future.successful(true)) + .when(googleExtensions) + .pollShellProjectCreation(any[String]) + val defaultUser = Generator.genWorkbenchUserBoth.sample.get val googleProject = GoogleProject("testproject") val report = intercept[WorkbenchExceptionWithErrorReport] { - googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() } report.errorReport.statusCode shouldEqual Some(StatusCodes.BadRequest) @@ -2080,32 +2218,39 @@ class GoogleExtensionSpec(_system: ActorSystem) override def isProjectActive(projectName: String): Future[Boolean] = Future.successful(false) } - val googleExtensions = new GoogleExtensions( - TestSupport.distributedLock, - dirDAO, - null, - mockGoogleDirectoryDAO, - null, - null, - null, - mockGoogleIamDAO, - null, - mockGoogleProjectDAO, - null, - null, - null, - null, - googleServicesConfig, - petServiceAccountConfig, - configResourceTypes, - superAdminsGroup + val googleExtensions = spy( + new GoogleExtensions( + TestSupport.distributedLock, + dirDAO, + null, + mockGoogleDirectoryDAO, + null, + null, + null, + mockGoogleIamDAO, + null, + mockGoogleProjectDAO, + null, + null, + null, + null, + googleServicesConfig, + petServiceAccountConfig, + configResourceTypes, + superAdminsGroup + ), + lenient = true ) + doReturn(Future.successful(true)) + .when(googleExtensions) + .pollShellProjectCreation(any[String]) + val defaultUser = Generator.genWorkbenchUserBoth.sample.get val googleProject = GoogleProject("testproject") val report = intercept[WorkbenchExceptionWithErrorReport] { - googleExtensions.createUserPetServiceAccount(defaultUser, googleProject, samRequestContext).unsafeRunSync() + googleExtensions.getSingletonPetServiceAccountForUser(defaultUser, Some(googleProject), samRequestContext).unsafeRunSync() } report.errorReport.statusCode shouldEqual Some(StatusCodes.BadRequest) diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala index 50947a181f..cc1a38217e 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/NewGoogleExtensionsSpec.scala @@ -9,6 +9,7 @@ import org.broadinstitute.dsde.workbench.RetryConfig import org.broadinstitute.dsde.workbench.dataaccess.NotificationDAO import org.broadinstitute.dsde.workbench.google.{GoogleDirectoryDAO, GoogleIamDAO, GoogleKmsService, GoogleProjectDAO, GooglePubSubDAO, GoogleStorageDAO} import org.broadinstitute.dsde.workbench.google2.{GcsBlobName, GoogleStorageService} +import org.broadinstitute.dsde.workbench.model.google.GoogleResourceTypes.GoogleParentResourceType import org.broadinstitute.dsde.workbench.model.{PetServiceAccountId, TraceId} import org.broadinstitute.dsde.workbench.model.google.{GcsBucketName, GoogleProject, ServiceAccountName} import org.broadinstitute.dsde.workbench.sam.Generator.{ @@ -38,6 +39,7 @@ import org.scalatest.matchers.should.Matchers import org.scalatest.{Inside, OptionValues} import java.net.URL +import java.util.UUID import java.util.concurrent.TimeUnit import scala.concurrent.{ExecutionContextExecutor, Future} @@ -67,6 +69,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) val mockGoogleKeyCache = mock[GoogleKeyCache](RETURNS_SMART_NULLS) val mockGoogleStorageService = mock[GoogleStorageService[IO]](RETURNS_SMART_NULLS) + val mockGoogleProjectDAO = mock[GoogleProjectDAO](RETURNS_SMART_NULLS) lazy val petServiceAccountConfig = TestSupport.petServiceAccountConfig lazy val googleServicesConfig = TestSupport.googleServicesConfig @@ -82,7 +85,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) mock[GooglePubSubDAO](RETURNS_SMART_NULLS), mock[GoogleIamDAO](RETURNS_SMART_NULLS), mock[GoogleStorageDAO](RETURNS_SMART_NULLS), - mock[GoogleProjectDAO](RETURNS_SMART_NULLS), + mockGoogleProjectDAO, mockGoogleKeyCache, mock[NotificationDAO](RETURNS_SMART_NULLS), mock[GoogleKmsService[IO]](RETURNS_SMART_NULLS), @@ -91,12 +94,17 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) petServiceAccountConfig, Map.empty[ResourceTypeName, ResourceType], genFirecloudEmail.sample.get - ) + ), + lenient = true ) doReturn(IO.pure(petServiceAccount)) .when(googleExtensions) - .createUserPetServiceAccount(eqTo(newGoogleUser), eqTo(googleProject), any[SamRequestContext]) + .createUserPetServiceAccount(eqTo(newGoogleUser), eqTo(TestSupport.singletonServiceAccountProject(newGoogleUser)), any[SamRequestContext]) + + doReturn(IO.pure(petServiceAccount)) + .when(googleExtensions) + .getSingletonPetServiceAccountForUser(eqTo(newGoogleUser), any[Option[GoogleProject]], any[SamRequestContext]) doReturn(IO.pure(petServiceAccountKey)) .when(mockGoogleKeyCache) @@ -114,6 +122,15 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) any[TimeUnit], any[Map[String, String]] ) + + doReturn(Future.successful(true)) + .when(googleExtensions) + .pollShellProjectCreation(any[String]) + + doReturn(Future.successful(UUID.randomUUID().toString)) + .when(mockGoogleProjectDAO) + .createProject(any[String], any[String], any[GoogleParentResourceType]) + "getSignedUrl" - { "generates a signed URL for a GCS Object" in { val signedUrl = @@ -121,7 +138,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) signedUrl should be(expectedUrl) - verify(googleExtensions).createUserPetServiceAccount(eqTo(newGoogleUser), eqTo(googleProject), any[SamRequestContext]) + verify(googleExtensions).getSingletonPetServiceAccountKeyForUser(eqTo(newGoogleUser), eqTo(Some(googleProject)), any[SamRequestContext]) verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) @@ -223,6 +240,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) val mockDirectoryDAO = mock[DirectoryDAO](RETURNS_SMART_NULLS) val mockGoogleKeyCache = mock[GoogleKeyCache](RETURNS_SMART_NULLS) val mockGoogleIamDAO = mock[GoogleIamDAO](RETURNS_SMART_NULLS) + val mockGoogleProjectDAO = mock[GoogleProjectDAO](RETURNS_SMART_NULLS) val googleExtensions: GoogleExtensions = spy( new GoogleExtensions( @@ -235,7 +253,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) mock[GooglePubSubDAO](RETURNS_SMART_NULLS), mockGoogleIamDAO, mock[GoogleStorageDAO](RETURNS_SMART_NULLS), - mock[GoogleProjectDAO](RETURNS_SMART_NULLS), + mockGoogleProjectDAO, mockGoogleKeyCache, mock[NotificationDAO](RETURNS_SMART_NULLS), mock[GoogleKmsService[IO]](RETURNS_SMART_NULLS), @@ -244,7 +262,8 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) TestSupport.petServiceAccountConfig, Map.empty[ResourceTypeName, ResourceType], genFirecloudEmail.sample.get - ) + ), + lenient = true ) doReturn(IO.some(subject)) @@ -263,6 +282,10 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) .when(googleExtensions) .createUserPetServiceAccount(eqTo(user), eqTo(googleProject), any[SamRequestContext]) + doReturn(IO.pure(petServiceAccount)) + .when(googleExtensions) + .getSingletonPetServiceAccountForUser(eqTo(user), any[Option[GoogleProject]], any[SamRequestContext]) + doReturn(Future.successful(expectedToken)) .when(googleExtensions) .getAccessTokenUsingJson(eqTo(expectedKey), eqTo(scopes)) @@ -275,6 +298,14 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) .when(mockGoogleKeyCache) .getKey(eqTo(petServiceAccount)) + doReturn(Future.successful(true)) + .when(googleExtensions) + .pollShellProjectCreation(any[String]) + + doReturn(Future.successful(UUID.randomUUID().toString)) + .when(mockGoogleProjectDAO) + .createProject(any[String], any[String], any[GoogleParentResourceType]) + "getPetServiceAccountKey" - { "gets a key for an email" in { clearInvocations(mockDirectoryDAO, googleExtensions, mockGoogleKeyCache) @@ -284,7 +315,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) key should be(Some(expectedKey)) verify(mockDirectoryDAO).loadSubjectFromEmail(eqTo(email), any[SamRequestContext]) - verify(googleExtensions).createUserPetServiceAccount(eqTo(user), eqTo(googleProject), any[SamRequestContext]) + verify(googleExtensions).getSingletonPetServiceAccountForUser(eqTo(user), eqTo(Some(googleProject)), any[SamRequestContext]) verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) } @@ -296,7 +327,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) key should be(expectedKey) verifyNoInteractions(mockDirectoryDAO) - verify(googleExtensions).createUserPetServiceAccount(eqTo(user), eqTo(googleProject), any[SamRequestContext]) + verify(googleExtensions).getSingletonPetServiceAccountForUser(eqTo(user), eqTo(Some(googleProject)), any[SamRequestContext]) verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) } @@ -309,7 +340,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) token should be(Some(expectedToken)) verify(mockDirectoryDAO).loadSubjectFromEmail(eqTo(email), any[SamRequestContext]) - verify(googleExtensions).createUserPetServiceAccount(eqTo(user), eqTo(googleProject), any[SamRequestContext]) + verify(googleExtensions).getSingletonPetServiceAccountForUser(eqTo(user), eqTo(Some(googleProject)), any[SamRequestContext]) verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) verify(googleExtensions).getAccessTokenUsingJson(eqTo(expectedKey), eqTo(scopes)) @@ -322,7 +353,7 @@ class NewGoogleExtensionsSpec(_system: ActorSystem) token should be(expectedToken) verifyNoInteractions(mockDirectoryDAO) - verify(googleExtensions).createUserPetServiceAccount(eqTo(user), eqTo(googleProject), any[SamRequestContext]) + verify(googleExtensions).getSingletonPetServiceAccountForUser(eqTo(user), eqTo(Some(googleProject)), any[SamRequestContext]) verify(mockGoogleKeyCache).getKey(eqTo(petServiceAccount)) verify(googleExtensions).getAccessTokenUsingJson(eqTo(expectedKey), eqTo(scopes)) } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockCloudExtensionsBuilder.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockCloudExtensionsBuilder.scala index f8ca060d43..641ab66117 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockCloudExtensionsBuilder.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/MockCloudExtensionsBuilder.scala @@ -27,6 +27,7 @@ case class MockCloudExtensionsBuilder(allUsersGroup: WorkbenchGroup) extends Idi mockedCloudExtensions.allSubSystems returns Set.empty mockedCloudExtensions.checkStatus returns Map.empty mockedCloudExtensions.deleteUserPetServiceAccount(any[WorkbenchUserId], any[GoogleProject], any[SamRequestContext]) returns IO(true) + mockedCloudExtensions.removeProjectServiceAgents(any[WorkbenchUserId], any[GoogleProject], any[SamRequestContext]) returns IO(true) def withEnabledUser(samUser: SamUser): MockCloudExtensionsBuilder = withEnabledUsers(Set(samUser)) def withEnabledUsers(samUsers: Iterable[SamUser]): MockCloudExtensionsBuilder = { From ecc3cb4969682672483007bfc8a221f4e740d109 Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 19 Sep 2024 11:11:25 -0400 Subject: [PATCH 6/9] some logging --- .../dsde/workbench/sam/google/GoogleExtensions.scala | 3 +++ .../workbench/sam/google/GoogleExtensionRoutesSpec.scala | 7 ------- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index ae49bfc7d5..2c0aad00cb 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -30,6 +30,7 @@ import org.broadinstitute.dsde.workbench.sam.model.api.SamUser import org.broadinstitute.dsde.workbench.sam.service.UserService._ import org.broadinstitute.dsde.workbench.sam.service.{CloudExtensions, CloudExtensionsInitializer, ManagedGroupService, SamApplication} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext +import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.{FutureWithLogging, IOWithLogging} import org.broadinstitute.dsde.workbench.util.health.{HealthMonitor, SubsystemStatus, Subsystems} import org.broadinstitute.dsde.workbench.util.{FutureSupport, Retry} import spray.json._ @@ -606,6 +607,7 @@ class GoogleExtensions( petServiceAgents <- directoryDAO.getPetServiceAgents(user.id, petServiceAccount.id.project, destinationProject, samRequestContext) existingServiceAgents = petServiceAgents.map(_.serviceAgents.map(_.name)).getOrElse(Set.empty) serviceAgentsToAdd = googleServicesConfig.serviceAgents -- existingServiceAgents + _ = logger.info(s"Adding $serviceAgentsToAdd to $petServiceAccount") _ <- assertProjectInTerraOrg(destinationProject) _ <- assertProjectIsActive(destinationProject) destinationProjectNumber <- petServiceAgents @@ -642,6 +644,7 @@ class GoogleExtensions( } yield () } serviceAgentsToRemove = existingServiceAgents -- googleServicesConfig.serviceAgents + _ = logger.info(s"Removing $serviceAgentsToRemove from $petServiceAccount") _ <- serviceAgentsToRemove.toList.traverse { serviceAgentName => // val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) for { diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala index 52b088c5b7..16ec8476f9 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala @@ -498,13 +498,6 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca def setupSignedUrlTest(): (SamUser, SamRoutes, String) = { val googleIamDAO = new RealKeyMockGoogleIamDAO -// val googleProjectDAO = new MockGoogleProjectDAO() { -// override def pollOperation(operationId: String): Future[Operation] = { -// val operation = new com.google.api.services.cloudresourcemanager.model.Operation -// operation.setDone(true) -// Future.successful(operation) -// } -// } val samUser = Generator.genWorkbenchUserGoogle.sample.get val (user, samDeps, routes) = createTestUser( configResourceTypes, From a6bc456fc58d5890cd46dce4a1b0f8dadfc40583 Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 19 Sep 2024 11:20:07 -0400 Subject: [PATCH 7/9] sigh, compilers --- .../dsde/workbench/sam/google/GoogleExtensions.scala | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 2c0aad00cb..412f478b52 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -30,7 +30,6 @@ import org.broadinstitute.dsde.workbench.sam.model.api.SamUser import org.broadinstitute.dsde.workbench.sam.service.UserService._ import org.broadinstitute.dsde.workbench.sam.service.{CloudExtensions, CloudExtensionsInitializer, ManagedGroupService, SamApplication} import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext -import org.broadinstitute.dsde.workbench.sam.util.AsyncLogging.{FutureWithLogging, IOWithLogging} import org.broadinstitute.dsde.workbench.util.health.{HealthMonitor, SubsystemStatus, Subsystems} import org.broadinstitute.dsde.workbench.util.{FutureSupport, Retry} import spray.json._ From d76548be6bd32de15e5ce6c5e63f445a4a637ad8 Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 26 Sep 2024 14:10:45 -0400 Subject: [PATCH 8/9] Create singleton SA on user create --- .../sam/google/GoogleExtensions.scala | 1 + .../dsde/workbench/sam/TestSupport.scala | 3 +- .../google/GoogleExtensionRoutesSpec.scala | 92 ++++++++----------- .../google/GoogleExtensionRoutesV1Spec.scala | 10 +- .../sam/google/GoogleExtensionSpec.scala | 19 +++- .../sam/mock/MockSamGoogleProjectDAO.scala | 15 +++ 6 files changed, 71 insertions(+), 69 deletions(-) create mode 100644 src/test/scala/org/broadinstitute/dsde/workbench/sam/mock/MockSamGoogleProjectDAO.scala diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 412f478b52..3bd791d1ed 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -289,6 +289,7 @@ class GoogleExtensions( } allUsersGroup <- getOrCreateAllUsersGroup(directoryDAO, samRequestContext) _ <- IO.fromFuture(IO(googleDirectoryDAO.addMemberToGroup(allUsersGroup.email, proxyEmail))) + _ <- getSingletonPetServiceAccountForUser(user, None, samRequestContext) } yield () } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala index 32d532d704..d0bed15819 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/TestSupport.scala @@ -24,6 +24,7 @@ import org.broadinstitute.dsde.workbench.sam.dataAccess._ import org.broadinstitute.dsde.workbench.sam.db.TestDbReference import org.broadinstitute.dsde.workbench.sam.db.tables._ import org.broadinstitute.dsde.workbench.sam.google.{GoogleExtensionRoutes, GoogleExtensions, GoogleGroupSynchronizer, GoogleKeyCache} +import org.broadinstitute.dsde.workbench.sam.mock.MockSamGoogleProjectDAO import org.broadinstitute.dsde.workbench.sam.model._ import org.broadinstitute.dsde.workbench.sam.model.api.SamUser import org.broadinstitute.dsde.workbench.sam.service.UserService._ @@ -100,7 +101,7 @@ object TestSupport extends TestSupport { val googleDisableUsersPubSubDAO = new MockGooglePubSubDAO() val googleKeyCachePubSubDAO = new MockGooglePubSubDAO() val googleStorageDAO = new MockGoogleStorageDAO() - val googleProjectDAO = googProjectDAO.getOrElse(new MockGoogleProjectDAO()) + val googleProjectDAO = googProjectDAO.getOrElse(new MockSamGoogleProjectDAO()) val notificationDAO = new PubSubNotificationDAO(notificationPubSubDAO, "foo") val cloudKeyCache = new GoogleKeyCache( distributedLock, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala index 16ec8476f9..a419b24cf7 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesSpec.scala @@ -6,8 +6,6 @@ import akka.http.scaladsl.model.StatusCodes import akka.http.scaladsl.testkit.{RouteTestTimeout, ScalatestRouteTest} import cats.effect.IO import cats.effect.unsafe.implicits.global -import com.google.api.services.cloudresourcemanager.model.Operation -import org.broadinstitute.dsde.workbench.google.mock.MockGoogleProjectDAO import org.broadinstitute.dsde.workbench.google.{GoogleIamDAO, GoogleProjectDAO} import org.broadinstitute.dsde.workbench.model.WorkbenchIdentityJsonSupport._ import org.broadinstitute.dsde.workbench.model._ @@ -16,7 +14,7 @@ import org.broadinstitute.dsde.workbench.sam.Generator._ import org.broadinstitute.dsde.workbench.sam.TestSupport.{genSamDependencies, genSamRoutes, _} import org.broadinstitute.dsde.workbench.sam.api.SamRoutes import org.broadinstitute.dsde.workbench.sam.config.GoogleServicesConfig -import org.broadinstitute.dsde.workbench.sam.mock.RealKeyMockGoogleIamDAO +import org.broadinstitute.dsde.workbench.sam.mock.{MockSamGoogleProjectDAO, RealKeyMockGoogleIamDAO} import org.broadinstitute.dsde.workbench.sam.model._ import org.broadinstitute.dsde.workbench.sam.model.api.SamJsonSupport._ import org.broadinstitute.dsde.workbench.sam.model.api._ @@ -320,11 +318,8 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests - val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) - - mockWithUser(user) + val (user, _, routes) = createTestUser(resourceTypes) // create a pet service account getPetServiceAccount("myproject", user, routes) @@ -337,10 +332,8 @@ class GoogleExtensionRoutesSpec extends GoogleExtensionRoutesSpecHelper with Sca assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests - val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) - mockWithUser(user) + val (user, _, routes) = createTestUser(resourceTypes) // create a pet service account getPetServiceAccount("myproject", user, routes) @@ -403,6 +396,35 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca val configResourceTypes = TestSupport.configResourceTypes + val expectedJson = """{"json":"yes I am"}""" + private def mockGoogleIamDAO(user: SamUser) = { + val googleIamDAO = mock[GoogleIamDAO](RETURNS_SMART_NULLS) + lenient().when(googleIamDAO.findServiceAccount(any[GoogleProject], any[ServiceAccountName])).thenReturn(Future.successful(None)) + lenient() + .when(googleIamDAO.createServiceAccount(any[GoogleProject], any[ServiceAccountName], any[ServiceAccountDisplayName])) + .thenReturn( + Future.successful( + ServiceAccount(ServiceAccountSubjectId("12312341234"), WorkbenchEmail(singletonServiceAccountForUser(user)), ServiceAccountDisplayName("")) + ) + ) + lenient() + .when(googleIamDAO.createServiceAccountKey(any[GoogleProject], any[WorkbenchEmail])) + .thenReturn( + Future.successful( + ServiceAccountKey(ServiceAccountKeyId("foo"), ServiceAccountPrivateKeyData(ServiceAccountPrivateKeyData(expectedJson).encode), None, None) + ) + ) + lenient().when(googleIamDAO.removeServiceAccountKey(any[GoogleProject], any[WorkbenchEmail], any[ServiceAccountKeyId])).thenReturn(Future.successful(())) + lenient().when(googleIamDAO.listUserManagedServiceAccountKeys(any[GoogleProject], any[WorkbenchEmail])).thenReturn(Future.successful(Seq.empty)) + lenient() + .when(googleIamDAO.addServiceAccountUserRoleForUser(any[GoogleProject], any[WorkbenchEmail], any[WorkbenchEmail])) + .thenReturn(Future.successful(())) + lenient + .when(googleIamDAO.addIamPolicyBindingOnServiceAccount(any[GoogleProject], any[WorkbenchEmail], any[WorkbenchEmail], any[Set[String]])) + .thenReturn(Future.successful(())) + googleIamDAO + } + def createTestUser( resourceTypes: Map[ResourceTypeName, ResourceType] = Map.empty[ResourceTypeName, ResourceType], googIamDAO: Option[GoogleIamDAO] = None, @@ -412,16 +434,11 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca user: SamUser = Generator.genWorkbenchUserBoth.sample.get, googleProjectDAO: Option[GoogleProjectDAO] = None ): (SamUser, SamDependencies, SamRoutes) = { - lazy val googleProjectDAOToUse = googleProjectDAO.orElse(Option(new MockGoogleProjectDAO() { - override def pollOperation(operationId: String): Future[Operation] = { - val operation = new com.google.api.services.cloudresourcemanager.model.Operation - operation.setDone(true) - Future.successful(operation) - } - })) + lazy val googleProjectDAOToUse = googleProjectDAO.orElse(Option(new MockSamGoogleProjectDAO)) + lazy val googleIamDAOToUse = googIamDAO.orElse(Option(mockGoogleIamDAO(user))) lazy val samDependencies = genSamDependencies( resourceTypes, - googIamDAO, + googleIamDAOToUse, googleServicesConfig, policyEvaluatorServiceOpt = policyEvaluatorServiceOpt, resourceServiceOpt = resourceServiceOpt, @@ -464,38 +481,6 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca googleIamDAO } - def createMockGoogleIamDaoForSAKeyTests: (GoogleIamDAO, String, SamUser => Unit) = { - val googleIamDAO = mock[GoogleIamDAO](RETURNS_SMART_NULLS) - val expectedJson = """{"json":"yes I am"}""" - lenient().when(googleIamDAO.findServiceAccount(any[GoogleProject], any[ServiceAccountName])).thenReturn(Future.successful(None)) - val mockWithUser = (user: SamUser) => { - lenient() - .when(googleIamDAO.createServiceAccount(any[GoogleProject], any[ServiceAccountName], any[ServiceAccountDisplayName])) - .thenReturn( - Future.successful( - ServiceAccount(ServiceAccountSubjectId("12312341234"), WorkbenchEmail(singletonServiceAccountForUser(user)), ServiceAccountDisplayName("")) - ) - ) - () - } - lenient() - .when(googleIamDAO.createServiceAccountKey(any[GoogleProject], any[WorkbenchEmail])) - .thenReturn( - Future.successful( - ServiceAccountKey(ServiceAccountKeyId("foo"), ServiceAccountPrivateKeyData(ServiceAccountPrivateKeyData(expectedJson).encode), None, None) - ) - ) - lenient().when(googleIamDAO.removeServiceAccountKey(any[GoogleProject], any[WorkbenchEmail], any[ServiceAccountKeyId])).thenReturn(Future.successful(())) - lenient().when(googleIamDAO.listUserManagedServiceAccountKeys(any[GoogleProject], any[WorkbenchEmail])).thenReturn(Future.successful(Seq.empty)) - lenient() - .when(googleIamDAO.addServiceAccountUserRoleForUser(any[GoogleProject], any[WorkbenchEmail], any[WorkbenchEmail])) - .thenReturn(Future.successful(())) - lenient - .when(googleIamDAO.addIamPolicyBindingOnServiceAccount(any[GoogleProject], any[WorkbenchEmail], any[WorkbenchEmail], any[Set[String]])) - .thenReturn(Future.successful(())) - (googleIamDAO, expectedJson, mockWithUser) - } - def setupSignedUrlTest(): (SamUser, SamRoutes, String) = { val googleIamDAO = new RealKeyMockGoogleIamDAO val samUser = Generator.genWorkbenchUserGoogle.sample.get @@ -537,17 +522,14 @@ trait GoogleExtensionRoutesSpecHelper extends AnyFlatSpec with Matchers with Sca } def setupPetSATest(): (SamUser, SamRoutes, String) = { - val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests val samUser = Generator.genWorkbenchUserGoogle.sample.get val (user, samDeps, routes) = createTestUser( configResourceTypes, - Some(googleIamDAO), - TestSupport.googleServicesConfig.copy(serviceAccountClientEmail = samUser.email, serviceAccountClientId = samUser.googleSubjectId.get.value), + googleServicesConfig = + TestSupport.googleServicesConfig.copy(serviceAccountClientEmail = samUser.email, serviceAccountClientId = samUser.googleSubjectId.get.value), user = samUser ) - mockWithUser(user) - samDeps.cloudExtensions .asInstanceOf[GoogleExtensions] .onBoot( diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala index 9d9344da0a..40c59fc06e 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionRoutesV1Spec.scala @@ -168,11 +168,8 @@ class GoogleExtensionRoutesV1Spec extends GoogleExtensionRoutesSpecHelper with S assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests - val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) - - mockWithUser(user) + val (user, _, routes) = createTestUser(resourceTypes) // create a pet service account Get("/api/google/v1/user/petServiceAccount/myproject") ~> routes.route ~> check { @@ -193,11 +190,8 @@ class GoogleExtensionRoutesV1Spec extends GoogleExtensionRoutesSpecHelper with S assume(databaseEnabled, databaseEnabledClue) val resourceTypes = Map(resourceType.name -> resourceType) - val (googleIamDAO, expectedJson: String, mockWithUser) = createMockGoogleIamDaoForSAKeyTests - val (user, _, routes) = createTestUser(resourceTypes, Some(googleIamDAO)) - - mockWithUser(user) + val (user, _, routes) = createTestUser(resourceTypes) // create a pet service account Get("/api/google/v1/user/petServiceAccount/myproject") ~> routes.route ~> check { diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala index e0924392ce..013f3e4d52 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensionSpec.scala @@ -18,7 +18,7 @@ import org.broadinstitute.dsde.workbench.google2.GcsBlobName import org.broadinstitute.dsde.workbench.google2.mock.FakeGoogleStorageInterpreter import org.broadinstitute.dsde.workbench.model.Notifications.NotificationFormat import org.broadinstitute.dsde.workbench.model._ -import org.broadinstitute.dsde.workbench.model.google.GoogleProject +import org.broadinstitute.dsde.workbench.model.google.{GoogleProject, ServiceAccount} import org.broadinstitute.dsde.workbench.sam.TestSupport.{ databaseEnabled, databaseEnabledClue, @@ -27,7 +27,7 @@ import org.broadinstitute.dsde.workbench.sam.TestSupport.{ truncateAll } import org.broadinstitute.dsde.workbench.sam.dataAccess._ -import org.broadinstitute.dsde.workbench.sam.mock.RealKeyMockGoogleIamDAO +import org.broadinstitute.dsde.workbench.sam.mock.{MockSamGoogleProjectDAO, RealKeyMockGoogleIamDAO} import org.broadinstitute.dsde.workbench.sam.model._ import org.broadinstitute.dsde.workbench.sam.model.api._ import org.broadinstitute.dsde.workbench.sam.service._ @@ -935,6 +935,7 @@ class GoogleExtensionSpec(_system: ActorSystem) // don't do any of the real boot stuff, it is all googley override def onBoot()(implicit system: ActorSystem): IO[Unit] = IO.unit } + val mockGoogleProjectDAO = new MockSamGoogleProjectDAO val ge = new GoogleExtensions( TestSupport.distributedLock, @@ -946,7 +947,7 @@ class GoogleExtensionSpec(_system: ActorSystem) mockGoogleDisableUsersPubSubDAO, mockGoogleIamDAO, mockGoogleStorageDAO, - null, + mockGoogleProjectDAO, googleKeyCache, notificationDAO, FakeGoogleKmsInterpreter, @@ -1052,7 +1053,15 @@ class GoogleExtensionSpec(_system: ActorSystem) val proxyEmail = WorkbenchEmail(s"PROXY_${user.id}@${googleServicesConfig.appsDomain}") val mockDirectoryDAO = mock[DirectoryDAO](RETURNS_SMART_NULLS) + when(mockDirectoryDAO.loadPetServiceAccount(any[PetServiceAccountId], any[SamRequestContext])).thenReturn(IO.none) + when(mockDirectoryDAO.createPetServiceAccount(any[PetServiceAccount], any[SamRequestContext])).thenAnswer( + (pet: PetServiceAccount, ctx: SamRequestContext) => IO.pure(pet) + ) + when(mockDirectoryDAO.enableIdentity(any[WorkbenchSubject], any[SamRequestContext])).thenReturn(IO.unit) val mockGoogleDirectoryDAO = mock[GoogleDirectoryDAO](RETURNS_SMART_NULLS) + when(mockGoogleDirectoryDAO.addServiceAccountToGroup(any[WorkbenchEmail], any[ServiceAccount])).thenReturn(Future.successful(())) + val mockGoogleProjectDAO = new MockSamGoogleProjectDAO + val mockGoogleIamDAO = new MockGoogleIamDAO val googleExtensions = new GoogleExtensions( TestSupport.distributedLock, mockDirectoryDAO, @@ -1061,9 +1070,9 @@ class GoogleExtensionSpec(_system: ActorSystem) null, null, null, + mockGoogleIamDAO, null, - null, - null, + mockGoogleProjectDAO, null, null, null, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/mock/MockSamGoogleProjectDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/mock/MockSamGoogleProjectDAO.scala new file mode 100644 index 0000000000..80290a5219 --- /dev/null +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/mock/MockSamGoogleProjectDAO.scala @@ -0,0 +1,15 @@ +package org.broadinstitute.dsde.workbench.sam.mock + +import com.google.api.services.cloudresourcemanager.model.Operation +import org.broadinstitute.dsde.workbench.google.mock.MockGoogleProjectDAO + +import scala.concurrent.Future + +class MockSamGoogleProjectDAO extends MockGoogleProjectDAO { + + override def pollOperation(operationId: String): Future[Operation] = { + val operation = new com.google.api.services.cloudresourcemanager.model.Operation + operation.setDone(true) + Future.successful(operation) + } +} From 6ddc3a12c9ebd71c488ea1dfb450401b5b2fa97c Mon Sep 17 00:00:00 2001 From: tlangs Date: Thu, 26 Sep 2024 15:13:42 -0400 Subject: [PATCH 9/9] try this out for size --- .../sam/google/GoogleExtensions.scala | 116 +++++++++--------- 1 file changed, 61 insertions(+), 55 deletions(-) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala index 3bd791d1ed..8851813f5f 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala @@ -603,66 +603,72 @@ class GoogleExtensions( destinationProject: GoogleProject, samRequestContext: SamRequestContext ): IO[Unit] = - for { - petServiceAgents <- directoryDAO.getPetServiceAgents(user.id, petServiceAccount.id.project, destinationProject, samRequestContext) - existingServiceAgents = petServiceAgents.map(_.serviceAgents.map(_.name)).getOrElse(Set.empty) - serviceAgentsToAdd = googleServicesConfig.serviceAgents -- existingServiceAgents - _ = logger.info(s"Adding $serviceAgentsToAdd to $petServiceAccount") - _ <- assertProjectInTerraOrg(destinationProject) - _ <- assertProjectIsActive(destinationProject) - destinationProjectNumber <- petServiceAgents - .map(psa => IO.pure(Option(psa.destinationProjectNumber))) - .getOrElse(IO.fromFuture(IO(googleProjectDAO.getProjectNumber(destinationProject.value)))) - .map( - _.getOrElse( - throw new WorkbenchExceptionWithErrorReport( - ErrorReport(StatusCodes.BadRequest, s"Could not find project number for project ${destinationProject.value}") + if (destinationProject.equals(petServiceAccount.id.project)) { + logger.info("Not adding service agents from singleton pet SA project to singleton pet SA") + IO.unit + } else { + for { + petServiceAgents <- directoryDAO.getPetServiceAgents(user.id, petServiceAccount.id.project, destinationProject, samRequestContext) + _ = logger.info(s"$petServiceAgents") + existingServiceAgents = petServiceAgents.map(_.serviceAgents.map(_.name)).getOrElse(Set.empty) + serviceAgentsToAdd = googleServicesConfig.serviceAgents -- existingServiceAgents + _ = logger.info(s"Adding $serviceAgentsToAdd to $petServiceAccount") + _ <- assertProjectInTerraOrg(destinationProject) + _ <- assertProjectIsActive(destinationProject) + destinationProjectNumber <- petServiceAgents + .map(psa => IO.pure(Option(psa.destinationProjectNumber))) + .getOrElse(IO.fromFuture(IO(googleProjectDAO.getProjectNumber(destinationProject.value)))) + .map( + _.getOrElse( + throw new WorkbenchExceptionWithErrorReport( + ErrorReport(StatusCodes.BadRequest, s"Could not find project number for project ${destinationProject.value}") + ) ) ) - ) - _ <- serviceAgentsToAdd.toList.traverse { serviceAgentName => - val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) - for { - _ <- IO.fromFuture( - IO( - googleIamDAO.addIamPolicyBindingOnServiceAccount( - petServiceAccount.id.project, - petServiceAccount.serviceAccount.email, - serviceAgent.email, - Set("roles/iam.serviceAccountTokenCreator") + _ <- serviceAgentsToAdd.toList.traverse { serviceAgentName => + val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) + for { + _ <- IO.fromFuture( + IO( + googleIamDAO.addIamPolicyBindingOnServiceAccount( + petServiceAccount.id.project, + petServiceAccount.serviceAccount.email, + serviceAgent.email, + Set("roles/iam.serviceAccountTokenCreator") + ) ) ) - ) - _ <- directoryDAO.addPetServiceAgent( - user.id, - petServiceAccount.id.project, - destinationProject, - destinationProjectNumber, - serviceAgentName, - samRequestContext - ) - } yield () - } - serviceAgentsToRemove = existingServiceAgents -- googleServicesConfig.serviceAgents - _ = logger.info(s"Removing $serviceAgentsToRemove from $petServiceAccount") - _ <- serviceAgentsToRemove.toList.traverse { serviceAgentName => -// val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) - for { - // removeIamPolicyBindingOnServiceAccount is not implemented in googleIamDAO yet - // TODO do it -// _ <- googleIamDAO.removeIamPolicyBindingOnServiceAccount(petServiceAccount.id.project, petServiceAccount.serviceAccount.email, serviceAgent.email, Set("roles/iam.serviceAccountTokenCreator")) - _ <- directoryDAO.removePetServiceAgent( - user.id, - petServiceAccount.id.project, - destinationProject, - destinationProjectNumber, - serviceAgentName, - samRequestContext - ) - } yield () - } + _ <- directoryDAO.addPetServiceAgent( + user.id, + petServiceAccount.id.project, + destinationProject, + destinationProjectNumber, + serviceAgentName, + samRequestContext + ) + } yield () + } + serviceAgentsToRemove = existingServiceAgents -- googleServicesConfig.serviceAgents + _ = logger.info(s"Removing $serviceAgentsToRemove from $petServiceAccount") + _ <- serviceAgentsToRemove.toList.traverse { serviceAgentName => + // val serviceAgent = ServiceAgent(serviceAgentName, destinationProjectNumber) + for { + // removeIamPolicyBindingOnServiceAccount is not implemented in googleIamDAO yet + // TODO do it + // _ <- googleIamDAO.removeIamPolicyBindingOnServiceAccount(petServiceAccount.id.project, petServiceAccount.serviceAccount.email, serviceAgent.email, Set("roles/iam.serviceAccountTokenCreator")) + _ <- directoryDAO.removePetServiceAgent( + user.id, + petServiceAccount.id.project, + destinationProject, + destinationProjectNumber, + serviceAgentName, + samRequestContext + ) + } yield () + } - } yield () + } yield () + } def getSingletonPetServiceAccountToken(userEmail: WorkbenchEmail, scopes: Set[String], samRequestContext: SamRequestContext): IO[Option[String]] = for {