diff --git a/.git-blame-ignore-revs b/.git-blame-ignore-revs index 4bd5edcd9d..778dc87cbd 100644 --- a/.git-blame-ignore-revs +++ b/.git-blame-ignore-revs @@ -1,2 +1,5 @@ # Scala Steward: Reformat with scalafmt 3.6.1 0c093c52c09d7b12cdbd38008e0bbc58c9d110be + +# Scala Steward: Reformat with scalafmt 3.8.3 +fc6844bda9d3bdf0b5751381ed9e402fdeb577b8 diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS new file mode 100644 index 0000000000..c8f96a43cd --- /dev/null +++ b/.github/CODEOWNERS @@ -0,0 +1 @@ +* @broadinstitute/dsp-core-services diff --git a/.github/workflows/auto-approve-broadbot-prs.yml b/.github/workflows/auto-approve-broadbot-prs.yml new file mode 100644 index 0000000000..189e1a33fc --- /dev/null +++ b/.github/workflows/auto-approve-broadbot-prs.yml @@ -0,0 +1,23 @@ +name: Broadbot auto-approve +on: pull_request + +permissions: + pull-requests: write + +jobs: + getActor: + runs-on: ubuntu-latest + steps: + - name: "Echo github actor" + env: + GH_ACTOR: ${{ github.actor }} + run: echo "$GH_ACTOR" + broadbot: + runs-on: ubuntu-latest + if: github.actor == 'broadbot' + steps: + - name: Approve a PR + run: gh pr review --approve "$PR_URL" + env: + PR_URL: ${{ github.event.pull_request.html_url }} + GH_TOKEN: ${{ secrets.BROADBOT_TOKEN }} diff --git a/.github/workflows/verify_consumer_pacts.yml b/.github/workflows/verify_consumer_pacts.yml index 6322defc99..7c66a5ec45 100644 --- a/.github/workflows/verify_consumer_pacts.yml +++ b/.github/workflows/verify_consumer_pacts.yml @@ -319,7 +319,7 @@ jobs: -e JANITOR_CLIENT_CREDENTIAL_FILE_PATH="" \ -e JANITOR_TRACK_RESOURCE_PROJECT_ID="" \ -e JANITOR_TRACK_RESOURCE_TOPIC_ID="" \ - sbtscala/scala-sbt:eclipse-temurin-jammy-17.0.10_7_1.10.2_2.13.15 \ + sbtscala/scala-sbt:eclipse-temurin-17.0.13_11_1.10.5_2.13.15 \ bash -c "git config --global --add safe.directory /working/sam && sbt \"set scalafmtOnCompile := false\" \"project pact4s\" \"testOnly *SamProviderSpec\"" can-i-deploy: # The can-i-deploy job will run as a result of a Sam PR. It reports the pact verification statuses on all deployed environments. diff --git a/.scalafmt.conf b/.scalafmt.conf index 47e45a8de1..b0234070a4 100644 --- a/.scalafmt.conf +++ b/.scalafmt.conf @@ -1,4 +1,4 @@ -version=3.6.1 +version=3.8.3 style = default runner.dialect = scala213 diff --git a/automation/Dockerfile-tests b/automation/Dockerfile-tests index d0f981415c..9b60b2be1a 100644 --- a/automation/Dockerfile-tests +++ b/automation/Dockerfile-tests @@ -1,4 +1,4 @@ -FROM sbtscala/scala-sbt:eclipse-temurin-jammy-17.0.10_7_1.10.2_2.13.15 +FROM sbtscala/scala-sbt:eclipse-temurin-17.0.13_11_1.10.5_2.13.15 COPY src /app/src COPY test.sh /app diff --git a/automation/project/Dependencies.scala b/automation/project/Dependencies.scala index a9793ad492..250a05d08d 100644 --- a/automation/project/Dependencies.scala +++ b/automation/project/Dependencies.scala @@ -29,13 +29,15 @@ object Dependencies { "com.fasterxml.jackson.module" % ("jackson-module-scala_" + scalaV) % jacksonV, "ch.qos.logback" % "logback-classic" % "1.4.5", "org.slf4j" % "slf4j-api" % "2.0.3", - "net.logstash.logback" % "logstash-logback-encoder" % "6.6", + "net.logstash.logback" % "logstash-logback-encoder" % "8.0", "com.google.apis" % "google-api-services-oauth2" % "v1-rev112-1.20.0" excludeAll ( ExclusionRule("com.google.guava", "guava-jdk5"), ExclusionRule("org.apache.httpcomponents", "httpclient") ), - "com.google.api-client" % "google-api-client" % "1.22.0" excludeAll (ExclusionRule("com.google.guava", "guava-jdk5"), - ExclusionRule("org.apache.httpcomponents", "httpclient")), + "com.google.api-client" % "google-api-client" % "1.22.0" excludeAll ( + ExclusionRule("com.google.guava", "guava-jdk5"), + ExclusionRule("org.apache.httpcomponents", "httpclient") + ), "com.typesafe.akka" %% "akka-http-core" % akkaHttpV, "com.typesafe.akka" %% "akka-stream-testkit" % akkaV, "com.typesafe.akka" %% "akka-http" % akkaHttpV, diff --git a/automation/project/build.properties b/automation/project/build.properties index 3829f19f71..c7450fc2a9 100644 --- a/automation/project/build.properties +++ b/automation/project/build.properties @@ -1 +1 @@ -sbt.version=1.10.2 \ No newline at end of file +sbt.version=1.10.5 \ No newline at end of file diff --git a/codegen_java/project/build.properties b/codegen_java/project/build.properties index 0b699c3052..db1723b086 100644 --- a/codegen_java/project/build.properties +++ b/codegen_java/project/build.properties @@ -1 +1 @@ -sbt.version=1.10.2 +sbt.version=1.10.5 diff --git a/docker/build.sh b/docker/build.sh index 2ec1aab775..8b47ce6d11 100755 --- a/docker/build.sh +++ b/docker/build.sh @@ -96,9 +96,9 @@ function make_jar() GIT_MODEL_HASH=$(git log -n 1 --pretty=format:%h) # make jar. cache sbt dependencies. - docker run --rm --link postgres:postgres -e GIT_MODEL_HASH=$GIT_MODEL_HASH -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-jammy-17.0.10_7_1.10.2_2.13.15 /working/docker/init_schema.sh /working + docker run --rm --link postgres:postgres -e GIT_MODEL_HASH=$GIT_MODEL_HASH -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-17.0.13_11_1.10.5_2.13.15 /working/docker/init_schema.sh /working sleep 40 - docker run --rm --link postgres:postgres -e GIT_MODEL_HASH=$GIT_MODEL_HASH -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-jammy-17.0.10_7_1.10.2_2.13.15 /working/docker/install.sh /working + docker run --rm --link postgres:postgres -e GIT_MODEL_HASH=$GIT_MODEL_HASH -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-17.0.13_11_1.10.5_2.13.15 /working/docker/install.sh /working EXIT_CODE=$? set -e # Turn error detection back on for the rest of the script diff --git a/docker/build_jar.sh b/docker/build_jar.sh index 9258e77ebd..15e40fd8a4 100755 --- a/docker/build_jar.sh +++ b/docker/build_jar.sh @@ -8,7 +8,7 @@ set -e # Get the last commit hash of the model directory and set it as an environment variable GIT_MODEL_HASH=$(git log -n 1 --pretty=format:%h) -docker run --rm -e GIT_MODEL_HASH=$GIT_MODEL_HASH -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-jammy-17.0.10_7_1.10.2_2.13.15 /working/docker/clean_install.sh /working +docker run --rm -e GIT_MODEL_HASH=$GIT_MODEL_HASH -v $PWD:/working -v jar-cache:/root/.ivy -v jar-cache:/root/.ivy2 sbtscala/scala-sbt:eclipse-temurin-17.0.13_11_1.10.5_2.13.15 /working/docker/clean_install.sh /working EXIT_CODE=$? if [ $EXIT_CODE != 0 ]; then diff --git a/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala b/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala index 5e3d9e603b..e7ed9ae97a 100644 --- a/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala +++ b/pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala @@ -158,7 +158,7 @@ object MockTestSupport extends MockTestSupport { policyDAO, googleExt, FakeOpenIDConnectConfiguration, - azureService:Option[AzureService] + azureService: Option[AzureService] ) } diff --git a/project/Dependencies.scala b/project/Dependencies.scala index d25feb9295..4e8af28f4c 100644 --- a/project/Dependencies.scala +++ b/project/Dependencies.scala @@ -8,10 +8,10 @@ object Dependencies { val scalaTestV = "3.2.19" val scalaCheckV = "1.18.1" val scalikejdbcVersion = "3.4.2" - val postgresDriverVersion = "42.7.2" + val postgresDriverVersion = "42.7.4" val sentryVersion = "6.15.0" - val workbenchLibV = "80e4b8d" // If updating this, make sure googleStorageLocal in test dependencies is up-to-date + val workbenchLibV = "fa46370" // If updating this, make sure googleStorageLocal in test dependencies is up-to-date val workbenchUtilV = s"0.10-$workbenchLibV" val workbenchUtil2V = s"0.9-$workbenchLibV" val workbenchModelV = s"0.20-$workbenchLibV" @@ -46,14 +46,14 @@ object Dependencies { val jacksonDatabind: ModuleID = "com.fasterxml.jackson.core" % "jackson-databind" % jacksonV val jacksonCore: ModuleID = "com.fasterxml.jackson.core" % "jackson-core" % jacksonV - val logstashLogback: ModuleID = "net.logstash.logback" % "logstash-logback-encoder" % "6.6" - val logbackClassic: ModuleID = "ch.qos.logback" % "logback-classic" % "1.4.14" + val logstashLogback: ModuleID = "net.logstash.logback" % "logstash-logback-encoder" % "8.0" + val logbackClassic: ModuleID = "ch.qos.logback" % "logback-classic" % "1.5.12" val ravenLogback: ModuleID = "com.getsentry.raven" % "raven-logback" % "7.8.6" val scalaLogging: ModuleID = "com.typesafe.scala-logging" %% "scala-logging" % scalaLoggingV val ficus: ModuleID = "com.iheart" %% "ficus" % "1.5.2" // val stackdriverLogging: ModuleID = "org.springframework.cloud" % "spring-cloud-gcp-logging" % "1.2.8.RELEASE" excludeAll(excludeSpring, excludeSpringBoot) val stackdriverLogging: ModuleID = "com.google.cloud" % "google-cloud-logging-logback" % "0.127.11-alpha" - val janino: ModuleID = "org.codehaus.janino" % "janino" % "3.1.7" // For if-else logic in logging config + val janino: ModuleID = "org.codehaus.janino" % "janino" % "3.1.12" // For if-else logic in logging config val akkaActor: ModuleID = "com.typesafe.akka" %% "akka-actor" % akkaV val akkaSlf4j: ModuleID = "com.typesafe.akka" %% "akka-slf4j" % akkaV @@ -65,13 +65,13 @@ object Dependencies { val scalaCheck: ModuleID = "org.scalacheck" %% "scalacheck" % scalaCheckV % "test" val nettyAll: ModuleID = "io.netty" % "netty-all" % "4.1.114.Final" - val reactorNetty: ModuleID = "io.projectreactor.netty" % "reactor-netty" % "1.0.39" + val reactorNetty: ModuleID = "io.projectreactor.netty" % "reactor-netty" % "1.0.48" val excludIoGrpc = ExclusionRule(organization = "io.grpc", name = "grpc-core") val ioGrpc: ModuleID = "io.grpc" % "grpc-core" % "1.34.1" val googleOAuth2: ModuleID = "com.google.auth" % "google-auth-library-oauth2-http" % "0.18.0" excludeAll excludIoGrpc - val googleStorage: ModuleID = "com.google.apis" % "google-api-services-storage" % "v1-rev20220401-1.32.1" excludeAll excludIoGrpc // force this version + val googleStorage: ModuleID = "com.google.apis" % "google-api-services-storage" % "v1-rev20241008-2.0.0" excludeAll excludIoGrpc // force this version val monocle: ModuleID = "com.github.julien-truffaut" %% "monocle-core" % monocleVersion val monocleMacro: ModuleID = "com.github.julien-truffaut" %% "monocle-macro" % monocleVersion @@ -95,20 +95,31 @@ object Dependencies { val excludeGoogleAutoValue = ExclusionRule(organization = "com.google.auto.value", name = "auto-value") val excludeBouncyCastle = ExclusionRule("org.bouncycastle") val workbenchGoogle2: ModuleID = - "org.broadinstitute.dsde.workbench" %% "workbench-google2" % workbenchGoogle2V excludeAll (excludeWorkbenchModel, excludeWorkbenchUtil, excludeGoogleAutoValue, excludeBouncyCastle) + "org.broadinstitute.dsde.workbench" %% "workbench-google2" % workbenchGoogle2V excludeAll ( + excludeWorkbenchModel, + excludeWorkbenchUtil, + excludeGoogleAutoValue, + excludeBouncyCastle + ) val workbenchNotifications: ModuleID = "org.broadinstitute.dsde.workbench" %% "workbench-notifications" % workbenchNotificationsV excludeAll (excludeWorkbenchGoogle, excludeWorkbenchModel) val workbenchGoogleTests: ModuleID = - "org.broadinstitute.dsde.workbench" %% "workbench-google" % workbenchGoogleV % "test" classifier "tests" excludeAll (excludeWorkbenchUtil, excludeWorkbenchModel) + "org.broadinstitute.dsde.workbench" %% "workbench-google" % workbenchGoogleV % "test" classifier "tests" excludeAll ( + excludeWorkbenchUtil, + excludeWorkbenchModel + ) val workbenchGoogle2Tests: ModuleID = - "org.broadinstitute.dsde.workbench" %% "workbench-google2" % workbenchGoogle2V % "test" classifier "tests" excludeAll (excludeWorkbenchUtil, excludeWorkbenchModel) + "org.broadinstitute.dsde.workbench" %% "workbench-google2" % workbenchGoogle2V % "test" classifier "tests" excludeAll ( + excludeWorkbenchUtil, + excludeWorkbenchModel + ) val googleStorageLocal: ModuleID = - "com.google.cloud" % "google-cloud-nio" % "0.127.25" % "test" // needed for mocking google cloud storage. Should use same version as wb-libs + "com.google.cloud" % "google-cloud-nio" % "0.127.26" % "test" // needed for mocking google cloud storage. Should use same version as wb-libs val liquibaseCore: ModuleID = "org.liquibase" % "liquibase-core" % "4.2.2" - val circeYAML: ModuleID = "io.circe" %% "circe-yaml" % "0.14.2" - val snakeYAML: ModuleID = "org.yaml" % "snakeyaml" % "1.33" + val circeYAML: ModuleID = "io.circe" %% "circe-yaml" % "0.16.0" + val snakeYAML: ModuleID = "org.yaml" % "snakeyaml" % "2.3" val scalikeCore = "org.scalikejdbc" %% "scalikejdbc" % scalikejdbcVersion val scalikeCoreConfig = "org.scalikejdbc" %% "scalikejdbc-config" % scalikejdbcVersion @@ -135,9 +146,16 @@ object Dependencies { ) val cloudResourceLib: ModuleID = - "bio.terra" % "terra-cloud-resource-lib" % crlVersion excludeAll (excludeGoogleServiceUsage, excludeGoogleCloudResourceManager, excludeJerseyCore, excludeJerseyMedia, excludeSLF4J, excludeAwsSdk) + "bio.terra" % "terra-cloud-resource-lib" % crlVersion excludeAll ( + excludeGoogleServiceUsage, + excludeGoogleCloudResourceManager, + excludeJerseyCore, + excludeJerseyMedia, + excludeSLF4J, + excludeAwsSdk + ) val azureManagedApplications: ModuleID = - "com.azure.resourcemanager" % "azure-resourcemanager-managedapplications" % "1.0.0-beta.1" + "com.azure.resourcemanager" % "azure-resourcemanager-managedapplications" % "1.0.0-beta.4" def excludeSpringBoot = ExclusionRule("org.springframework.boot") def excludeSpringAop = ExclusionRule("org.springframework.spring-aop") @@ -169,7 +187,7 @@ object Dependencies { val terraCommonLib = tclExclusions("bio.terra" % "terra-common-lib" % tclVersion classifier "plain") // was included transitively before, now explicit - val commonsCodec: ModuleID = "commons-codec" % "commons-codec" % "1.15" + val commonsCodec: ModuleID = "commons-codec" % "commons-codec" % "1.17.1" val rootDependencies = Seq( // proactively pull in latest versions of Jackson libs, instead of relying on the versions @@ -227,6 +245,6 @@ object Dependencies { // Needed because it looks like the dependency overrides of wb-libs doesn't propagate to the importing project... val rootDependencyOverrides = Seq( - "org.apache.commons" % "commons-compress" % "1.26.0" + "org.apache.commons" % "commons-compress" % "1.26.2" ) } diff --git a/project/build.properties b/project/build.properties index 0b699c3052..db1723b086 100644 --- a/project/build.properties +++ b/project/build.properties @@ -1 +1 @@ -sbt.version=1.10.2 +sbt.version=1.10.5 diff --git a/project/plugins.sbt b/project/plugins.sbt index 8224092da6..58bd304772 100644 --- a/project/plugins.sbt +++ b/project/plugins.sbt @@ -2,7 +2,7 @@ addSbtPlugin("com.eed3si9n" % "sbt-assembly" % "2.3.0") addSbtPlugin("io.spray" % "sbt-revolver" % "0.10.0") -addSbtPlugin("org.scoverage" % "sbt-scoverage" % "2.2.1") +addSbtPlugin("org.scoverage" % "sbt-scoverage" % "2.2.2") addSbtPlugin("org.scalameta" % "sbt-scalafmt" % "2.5.2") diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index d5430a19f1..2b1d89feea 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/src/main/resources/swagger/api-docs.yaml @@ -199,7 +199,7 @@ paths: post: tags: - Admin - summary: Gets a list of users for a list of Sam User IDs + summary: Gets a list of users for a list of Sam, Azure B2C, or Google Subject IDs operationId: adminGetUsersByIDs requestBody: content: diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala index bb33978304..755e027cdd 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/AccessPolicyDAO.scala @@ -130,7 +130,11 @@ trait AccessPolicyDAO { samRequestContext: SamRequestContext ): IO[Seq[FilterResourcesResult]] - def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] + def findPolicyGroupsInUse( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] + } sealed abstract class LoadResourceAuthDomainResult diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala index 23aa83d480..68d7e508db 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAO.scala @@ -951,6 +951,50 @@ class PostgresAccessPolicyDAO( } } + // Return value: [(policyToUpdate, policyToRemove)] + override def findPolicyGroupsInUse( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = + readOnlyTransaction("findAffectedPolicyGroups", samRequestContext) { implicit session => + val groupMemberTable = GroupMemberTable.syntax("group_member_table") + val policyTable = PolicyTable.syntax("policy_table") + val parentPolicyTable = PolicyTable.syntax("parent_policy_table") + val resourceType = ResourceTypeTable.syntax("resource_type") + val resourceTable = ResourceTable.syntax("resource_table") + + val query = samsql""" + WITH resourcePolicies as ( + SELECT ${policyTable.groupId} as childGroupId, ${policyTable.name} as policyName + FROM ${PolicyTable as policyTable} + WHERE ${policyTable.resourceId} = (${loadResourcePKSubQuery(resourceId)}) + ) + SELECT resourcePolicies.policyName as childPolicyName, ${parentPolicyTable.name} as parentPolicyName, ${resourceTable.name} as parentPolicyResourceName, ${resourceType.name} as parentPolicyResourceType +from ${GroupMemberTable as groupMemberTable} + JOIN resourcePolicies ON ${groupMemberTable.memberGroupId} = resourcePolicies.childGroupId + JOIN ${PolicyTable as parentPolicyTable} ON ${parentPolicyTable.groupId} = ${groupMemberTable.groupId} + JOIN ${ResourceTable as resourceTable} ON ${resourceTable.id} = ${parentPolicyTable.resourceId} + JOIN ${ResourceTypeTable as resourceType} ON ${resourceType.id} = ${resourceTable.resourceTypeId} +""" + query + .map { rs => + val parentPolicyResourceType = rs.get[ResourceTypeName]("parentPolicyResourceType") + val parentPolicyResourceId = rs.get[ResourceId]("parentPolicyResourceName") + val parentPolciyAccessName = rs.get[AccessPolicyName]("parentPolicyName") + val memberPolicyAccessName = rs.get[AccessPolicyName]("childPolicyName") + + val parentPolicyFullResourceId = + FullyQualifiedResourceId(parentPolicyResourceType, parentPolicyResourceId) + val parentPolicyFullId = FullyQualifiedPolicyId(parentPolicyFullResourceId, parentPolciyAccessName) + val memberPolicyFullId = FullyQualifiedPolicyId(resourceId, memberPolicyAccessName) + + (parentPolicyFullId, memberPolicyFullId) + } + .list() + .apply() + + } + private def deleteAllResourcePolicies(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext)(implicit session: DBSession ): Unit = { @@ -975,37 +1019,6 @@ class PostgresAccessPolicyDAO( } } - override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = { - val g = GroupTable.syntax("g") - val pg = GroupTable.syntax("pg") // problematic group - val gm = GroupMemberTable.syntax("gm") - val p = PolicyTable.syntax("p") - - readOnlyTransaction("checkPolicyGroupsInUse", samRequestContext) { implicit session => - val problematicGroupsQuery = - samsql"""select ${g.result.id}, ${g.result.name}, array_agg(${pg.name}) as ${pg.resultName.name} - from ${GroupTable as g} - join ${GroupMemberTable as gm} on ${g.id} = ${gm.memberGroupId} - join ${GroupTable as pg} on ${gm.groupId} = ${pg.id} - where ${g.id} in - (select distinct ${gm.result.memberGroupId} - from ${GroupMemberTable as gm} - join ${PolicyTable as p} on ${gm.memberGroupId} = ${p.groupId} - where ${p.resourceId} = (${loadResourcePKSubQuery(resourceId)})) - group by ${g.id}, ${g.name}""" - problematicGroupsQuery - .map(rs => - Map( - "groupId" -> rs.get[GroupPK](g.resultName.id).value.toString, - "groupName" -> rs.get[String](g.resultName.name), - "still used in group(s):" -> rs.get[String](pg.resultName.name) - ) - ) - .list() - .apply() - } - } - override def loadPolicy(resourceAndPolicyName: FullyQualifiedPolicyId, samRequestContext: SamRequestContext): IO[Option[AccessPolicy]] = listPolicies(resourceAndPolicyName.resource, limitOnePolicy = Option(resourceAndPolicyName.accessPolicyName), samRequestContext).map(_.headOption) 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..ed78fc8cfa 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 @@ -430,7 +430,19 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val } else { readOnlyTransaction("batchLoadUsers", samRequestContext) { implicit session => val userTable = UserTable.syntax - val loadUserQuery = samsql"select ${userTable.resultAll} from ${UserTable as userTable} where ${userTable.id} in (${samUserIds})" + // the with clause is to keep the query size down, we only send the samUserIds once and reuse it in each unioned query + val loadUserQuery = + samsql""" + with sam_user_ids (user_id) as (values ${samUserIds.map(id => samsqls"($id)")}) + select ${userTable.resultAll} from ${UserTable as userTable} + join sam_user_ids ids on ids.user_id = ${userTable.id} + union + select ${userTable.resultAll} from ${UserTable as userTable} + join sam_user_ids ids on ids.user_id = ${userTable.azureB2cId} + union + select ${userTable.resultAll} from ${UserTable as userTable} + join sam_user_ids ids on ids.user_id = ${userTable.googleSubjectId} + """ loadUserQuery .map(UserTable(userTable)) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala index 7507fca4d7..265495033e 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala @@ -28,8 +28,8 @@ import scala.util.{Failure, Try} * * Example database records: group(7795) contains user(userid) group(7798) contains user(userid) group(7801) contains group(7798) and group(7799) * - * testdb=# select * from sam_group_member; id | group_id | member_group_id | member_user_id - * -------+----------+-----------------+---------------- 15636 | 7795 | | userid 15637 | 7798 | | userid 15638 | 7801 | 7798 | 15639 | 7801 | 7799 | + * testdb=# select * from sam_group_member; id | group_id | member_group_id | member_user_id -------+----------+-----------------+---------------- 15636 | 7795 + * \| | userid 15637 | 7798 | | userid 15638 | 7801 | 7798 | 15639 | 7801 | 7799 | * * testdb=# select * from sam_group_member_flat; id | group_id | member_group_id | member_user_id | group_membership_path | last_group_membership_element * --------+----------+-----------------+----------------+-----------------------+------------------------------ 345985 | 7795 | | userid | {7795} | 7795 @@ -115,19 +115,16 @@ trait PostgresGroupDAO { * the head path + tail path * * Example: Insert group T into group H. H starts empty but is already a member of groups A and B. T already has member groups X and Y which are empty. The - * flat group model starts containing: Group | Member Group | Path - * ------|--------------|------ A | H | {A} B | H | {B} T | X | {T} T | Y | {T} + * flat group model starts containing: Group | Member Group | Path ------|--------------|------ A | H | {A} B | H | {B} T | X | {T} T | Y | {T} * - * step 1 inserts direct membership of T in H Group | Member Group | Path - * ------|--------------|------ H | T | {H} + * step 1 inserts direct membership of T in H Group | Member Group | Path ------|--------------|------ H | T | {H} * - * step 2 inserts indirect memberships T in A and B Group | Member Group | Path - * ------|--------------|------ A | T | {A,H} B | T | {B,H} + * step 2 inserts indirect memberships T in A and B Group | Member Group | Path ------|--------------|------ A | T | {A,H} B | T | {B,H} * * step 3 inserts T's lower group hierarchy so that X and Y are members of H, A and B. The tail records are all of the records above where Group is T: ((T, * X, {T}), (T, Y, {T}) The head records are all of the records above where Member Group is T and the last path element is H: ((H, T, {H}), (A, T, {A,H}), - * (B, T, {B,H})) Group | Member Group | Path - * ------|--------------|------ H | X | {H,T} H | Y | {H,T} A | X | {A,H,T} A | Y | {A,H,T} B | X | {B,H,T} B | Y | {B,H,T} + * (B, T, {B,H})) Group | Member Group | Path ------|--------------|------ H | X | {H,T} H | Y | {H,T} A | X | {A,H,T} A | Y | {A,H,T} B | X | {B,H,T} B | Y + * \| {B,H,T} * * @param groupId * group being added to diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala index c7adfe3043..5ba6767af8 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceService.scala @@ -432,7 +432,6 @@ class ResourceService( def deleteResource(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = for { _ <- checkNoChildren(resource, samRequestContext) - _ <- checkNoPoliciesInUse(resource, samRequestContext) // remove from cloud first so a failure there does not leave sam in a bad state _ <- cloudDeletePolicies(resource, samRequestContext) @@ -440,22 +439,13 @@ class ResourceService( // leave a tomb stone if the resource type does not allow reuse leaveTombStone = !resourceTypes(resource.resourceTypeName).reuseIds + affectedPolicies <- accessPolicyDAO.findPolicyGroupsInUse(resource, samRequestContext) // New method + _ <- affectedPolicies.traverse { case (policyToUpdate, policyToRemove) => removeSubjectFromPolicy(policyToUpdate, policyToRemove, samRequestContext) } _ <- accessPolicyDAO.deleteResource(resource, leaveTombStone, samRequestContext) _ <- AuditLogger.logAuditEventIO(samRequestContext, ResourceEvent(ResourceDeleted, resource)) } yield () - private def checkNoPoliciesInUse(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = - accessPolicyDAO.checkPolicyGroupsInUse(resource, samRequestContext).flatMap { problematicGroups => - if (problematicGroups.nonEmpty) - IO.raiseError( - new WorkbenchExceptionWithErrorReport( // throws a 500 since that's the current behavior - ErrorReport(StatusCodes.InternalServerError, s"Foreign Key Violation(s) while deleting group(s): ${problematicGroups}") - ) - ) - else IO.unit - } - private def deleteActionManagedIdentitiesForResource(resource: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[Unit] = azureService .map { service => diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala index 41c1f7a38d..b9a00c3e4b 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/MockAccessPolicyDAO.scala @@ -123,8 +123,10 @@ class MockAccessPolicyDAO(private val resourceTypes: mutable.Map[ResourceTypeNam policies -= policy } - override def checkPolicyGroupsInUse(resourceId: FullyQualifiedResourceId, samRequestContext: SamRequestContext): IO[List[Map[String, String]]] = - IO.pure(List.empty) + override def findPolicyGroupsInUse( + resourceId: FullyQualifiedResourceId, + samRequestContext: SamRequestContext + ): IO[List[(FullyQualifiedPolicyId, FullyQualifiedPolicyId)]] = IO.pure(List.empty) override def listAccessPolicies( resourceTypeName: ResourceTypeName, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala index 178c99b22b..6c85d9c525 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresAccessPolicyDAOSpec.scala @@ -3553,6 +3553,99 @@ class PostgresAccessPolicyDAOSpec extends AnyFreeSpec with Matchers with BeforeA dao.listResourcesUsingAuthDomain(authDomainGroupName, samRequestContext).unsafeRunSync() shouldEqual Set.empty } } + + "findAffectedPolicyGroups" - { + "returns parent and member policy groups" in { + assume(databaseEnabled, databaseEnabledClue) + + // Create a resource with a policy + dao.createResourceType(resourceType, samRequestContext).unsafeRunSync() + + val user = Generator.genWorkbenchUserBoth.sample.get + dirDao.createUser(user, samRequestContext).unsafeRunSync() + + val parentResourceFullyQualifiedId = FullyQualifiedResourceId(resourceType.name, ResourceId("parent_resource")) + val childResourceFullyQualifiedId = FullyQualifiedResourceId(resourceType.name, ResourceId("child_resource")) + + val parentPolicy = AccessPolicy( + FullyQualifiedPolicyId(parentResourceFullyQualifiedId, AccessPolicyName("parentPolicyName")), + Set(user.id), + WorkbenchEmail("parentPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + val childPolicy = AccessPolicy( + FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("childPolicyName")), + Set(user.id), + WorkbenchEmail("childPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + + val otherParentResourceFullyQualifiedId = FullyQualifiedResourceId(resourceType.name, ResourceId("other_parent_resource")) + val otherParentPolicy = AccessPolicy( + FullyQualifiedPolicyId(otherParentResourceFullyQualifiedId, AccessPolicyName("otherParentPolicyName")), + Set(user.id), + WorkbenchEmail("otherParentPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + + val otherChildPolicy = AccessPolicy( + FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("otherChildPolicyName")), + Set(user.id), + WorkbenchEmail("otherChildPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + + val thirdPolicy = AccessPolicy( + FullyQualifiedPolicyId(childResourceFullyQualifiedId, AccessPolicyName("thirdPolicyName")), + Set(user.id), + WorkbenchEmail("thirdPolicy@email.com"), + resourceType.roles.map(_.roleName), + Set(readAction, writeAction), + Set.empty, + false + ) + + val parentResource = + Resource(parentResourceFullyQualifiedId.resourceTypeName, parentResourceFullyQualifiedId.resourceId, Set.empty, Set(parentPolicy)) + val childResource = + Resource( + childResourceFullyQualifiedId.resourceTypeName, + childResourceFullyQualifiedId.resourceId, + Set.empty, + Set(childPolicy, otherChildPolicy, thirdPolicy) + ) + val otherResource = Resource( + otherParentResourceFullyQualifiedId.resourceTypeName, + otherParentResourceFullyQualifiedId.resourceId, + Set.empty, + Set(otherParentPolicy) + ) + dao.createResource(parentResource, samRequestContext).unsafeRunSync() + dao.createResource(childResource, samRequestContext).unsafeRunSync() + dao.createResource(otherResource, samRequestContext).unsafeRunSync() + + dirDao.addGroupMember(otherParentPolicy.id, otherChildPolicy.id, samRequestContext).unsafeRunSync() + + // Add child policy to parent policy + dirDao.addGroupMember(parentPolicy.id, childPolicy.id, samRequestContext).unsafeRunSync() + + val policyGroups = dao.findPolicyGroupsInUse(childResourceFullyQualifiedId, samRequestContext).unsafeRunSync() + + policyGroups should contain theSameElementsAs List((parentPolicy.id, childPolicy.id), (otherParentPolicy.id, otherChildPolicy.id)) + } + } } private def uuid: String = 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..7f2adb5f15 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 @@ -630,8 +630,19 @@ class PostgresDirectoryDAOSpec extends RetryableAnyFreeSpec with Matchers with B assume(databaseEnabled, databaseEnabledClue) val users = Seq.range(0, 10).map(_ => Generator.genWorkbenchUserBoth.sample.get) users.foreach(user => dao.createUser(user, samRequestContext).unsafeRunSync()) - val loadedUsers = dao.batchLoadUsers(users.map(_.id).toSet, samRequestContext).unsafeRunSync() - loadedUsers should contain theSameElementsAs users + val loadedUsersBySamId = dao.batchLoadUsers(users.map(_.id).toSet, samRequestContext).unsafeRunSync() + loadedUsersBySamId should contain theSameElementsAs users + + val b2cIds = users.flatMap(_.azureB2CId.map(id => WorkbenchUserId(id.value))).toSet + val loadedUsersByAzureB2cId = dao.batchLoadUsers(b2cIds, samRequestContext).unsafeRunSync() + loadedUsersByAzureB2cId should contain theSameElementsAs users + + val googleSubjectIds = users.flatMap(_.googleSubjectId.map(id => WorkbenchUserId(id.value))).toSet + val loadedUsersByGoogleSubjectId = dao.batchLoadUsers(googleSubjectIds, samRequestContext).unsafeRunSync() + loadedUsersByGoogleSubjectId should contain theSameElementsAs users + + val loadedBy2Ids = dao.batchLoadUsers(googleSubjectIds ++ b2cIds, samRequestContext).unsafeRunSync() + loadedBy2Ids should contain theSameElementsAs users } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala index 081d1c80e5..5e85e19e3a 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ResourceServiceSpec.scala @@ -2278,7 +2278,7 @@ class ResourceServiceSpec testDeleteResource(managedGroupResourceType) } - it should "delete any action managed identites for the resource while it deletes the resource" in { + it should "delete any action managed identities for the resource while it deletes the resource" in { assume(databaseEnabled, databaseEnabledClue) val resource = FullyQualifiedResourceId(defaultResourceType.name, ResourceId("my-resource")) @@ -2378,6 +2378,64 @@ class ResourceServiceSpec assert(policyDAO.listAccessPolicies(parentResource, samRequestContext).unsafeRunSync().nonEmpty) } + it should "delete any policy members from groups before deleting resource" in { + assume(databaseEnabled, databaseEnabledClue) + + // Create a resource with a policy + val ownerRoleName = ResourceRoleName("owner") + val resourceType = ResourceType( + defaultResourceType.name, + Set(SamResourceActionPatterns.delete, ResourceActionPattern("view", "", false)), + Set(ResourceRole(ownerRoleName, Set(ResourceAction("delete"), ResourceAction("view")))), + ownerRoleName + ) + val resourceName = ResourceId("resource") + + runAndWait(service.createResourceType(resourceType, samRequestContext)) + + // creating policy and to refer to by policy identifiers + val policyMembership = AccessPolicyMembershipRequest(Set(dummyUser.email), Set(ResourceAction("view")), Set(ownerRoleName), Option(Set.empty)) + val policyName = AccessPolicyName("foo") + val resource = + runAndWait(service.createResource(resourceType, resourceName, Map(policyName -> policyMembership), Set.empty, None, dummyUser.id, samRequestContext)) + val policies: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName), samRequestContext).unsafeRunSync() + + // creating policy to refer to by policy email + val resourceName2 = ResourceId("resource2") + val policyMembership2 = AccessPolicyMembershipRequest(Set(dummyUser.email), Set(ResourceAction("view")), Set(ownerRoleName), None, None) + val policyName2 = AccessPolicyName("foo2") + val resource2 = + runAndWait( + service.createResource(resourceType, resourceName2, Map(policyName2 -> policyMembership2), Set.empty, None, dummyUser.id, samRequestContext) + ) + val policies2: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName2), samRequestContext).unsafeRunSync() + + // Add second resource to first policy + policies2.foreach { policy => + runAndWait( + service.addSubjectToPolicy( + FullyQualifiedPolicyId(FullyQualifiedResourceId(resourceType.name, resourceName), policyName), + FullyQualifiedPolicyId(FullyQualifiedResourceId(resourceType.name, resourceName2), policyName2), + samRequestContext + ) + ) + } + val policy2Emails = policies2.map(p => p.email).toSet + // Verify that policies are in group + val updatedPolicies: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName), samRequestContext).unsafeRunSync() + policy2Emails.subsetOf(updatedPolicies.head.policy.memberEmails) shouldBe true + // Delete resource; should not throw an error + runAndWait(service.deleteResource(FullyQualifiedResourceId(resourceType.name, resourceName2), samRequestContext)) + + // Verify that policies are no longer in group + val updatedPolicies2: Seq[AccessPolicyResponseEntry] = + service.listResourcePolicies(FullyQualifiedResourceId(resourceType.name, resourceName), samRequestContext).unsafeRunSync() + policy2Emails.subsetOf(updatedPolicies2.head.policy.memberEmails) shouldBe false + } + "validatePolicy" should "succeed with a correct policy" in { val emailToMaybeSubject = Map(dummyUser.email -> Option(dummyUser.id.asInstanceOf[WorkbenchSubject])) val policy = service.ValidatableAccessPolicy(AccessPolicyName("a"), emailToMaybeSubject, Set(ownerRoleName), Set(ResourceAction("alter_policies")), Set())