Merge remote-tracking branch 'origin/develop' into grantaar-azuremana…

…gedapps

.github/workflows/tag.yml

@@ -13,11 +13,6 @@ on:
         default: false
         required: false
         type: string
-      print-tag:
-        description: "Echo generated tag to console"
-        default: "true"
-        required: false
-        type: string
       release-branches:
         description: "Default branch (main, develop, etc)"
         default: 'main'
@@ -30,6 +25,9 @@ on:
       new-tag:
         description: "The value of the newly created tag"
         value: ${{ jobs.tag-job.outputs.new-tag }}
+      app-version:
+        description: "The app version"
+        value: ${{ jobs.tag-job.outputs.app-version }}
     secrets:
       BROADBOT_TOKEN:
         required: true
@@ -44,6 +42,7 @@ jobs:
     outputs:
       tag: ${{ steps.tag.outputs.tag }}
       new-tag: ${{ steps.tag.outputs.new_tag }}
+      app-version: ${{ steps.output-version.outputs.app-version }}
     steps:
       - name: Checkout current code
         uses: actions/checkout@v3
@@ -60,7 +59,14 @@ jobs:
           DRY_RUN: ${{ inputs.dry-run }}
           RELEASE_BRANCHES: ${{ inputs.release-branches }}
           WITH_V: true
-      - name: Echo generated tag to console
-        if: ${{ inputs.print-tag == 'true' }}
+      - name: Output app version
+        id: output-version
         run: |
-          echo "Newly created version tag: '${{ steps.tag.outputs.new_tag }}'"
+          # See https://broadworkbench.atlassian.net/browse/QA-2282 for context
+          if [[ -z "${{ steps.tag.outputs.new_tag }}" ]]; then
+            echo "App version tag for this commit has already been dispatched: '${{ steps.tag.outputs.tag }}'"
+            echo "app-version=${{ steps.tag.outputs.tag }}" >> $GITHUB_OUTPUT
+          else
+            echo "New app version tag: '${{ steps.tag.outputs.new_tag }}'"
+            echo "app-version=${{ steps.tag.outputs.new_tag }}" >> $GITHUB_OUTPUT
+          fi

.github/workflows/verify_consumer_pacts.yml

@@ -151,7 +151,7 @@ jobs:
           # for publishing the results of provider verification.
           if [[ -z "${{ inputs.pb-event-type }}" ]]; then
             echo "PROVIDER_BRANCH=${{ env.CURRENT_BRANCH }}" >> $GITHUB_ENV
-            echo "PROVIDER_VERSION=${{ needs.regulated-tag-job.outputs.new-tag }}" >> $GITHUB_ENV
+            echo "PROVIDER_VERSION=${{ needs.regulated-tag-job.outputs.app-version }}" >> $GITHUB_ENV
           else
             echo "PROVIDER_VERSION=${{ env.PROVIDER_TAG }}" >> $GITHUB_ENV
           fi

automation/project/Dependencies.scala

@@ -7,7 +7,7 @@ object Dependencies {
   val akkaV = "2.6.19"
   val akkaHttpV = "10.2.2"
 
-  val workbenchLibV = "a0519cb"
+  val workbenchLibV = "d16cba9"
   val workbenchGoogleV = s"0.30-$workbenchLibV"
   val workbenchGoogle2V = s"0.34-$workbenchLibV"
   val workbenchServiceTestV = "2.0-5863cbd"

pact4s/src/test/resources/reference.conf

@@ -119,8 +119,6 @@ testStuff = {
 oidc {
   authorityEndpoint = "https://accounts.google.com"
   oidcClientId = "some-client"
-  oidcClientSecret = "some-secret"
-  legacyGoogleClientId = "another-client"
 }
 
 liquibase {

project/Dependencies.scala

@@ -11,14 +11,14 @@ object Dependencies {
   val postgresDriverVersion = "42.7.2"
   val sentryVersion = "6.15.0"
 
-  val workbenchLibV = "1c0cf92" // If updating this, make sure googleStorageLocal in test dependencies is up-to-date
+  val workbenchLibV = "d16cba9" // 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.19-$workbenchLibV"
   val workbenchGoogleV = s"0.30-$workbenchLibV"
   val workbenchGoogle2V = s"0.36-$workbenchLibV"
   val workbenchNotificationsV = s"0.6-$workbenchLibV"
-  val workbenchOauth2V = s"0.5-$workbenchLibV"
+  val workbenchOauth2V = s"0.7-$workbenchLibV"
   val monocleVersion = "2.0.5"
   val crlVersion = "1.2.30-SNAPSHOT"
   val tclVersion = "1.0.5-SNAPSHOT"

src/main/resources/reference.conf

@@ -1604,6 +1604,67 @@ resourceTypes = {
     reuseIds = true
   }
 
+  private_azure_container_registry = {
+    actionPatterns = {
+      delete = {
+        description = "Delete this private acr"
+      }
+      read_policies = {
+        description = "view all policies and policy details for this private acr"
+      }
+      identify = {
+        description = "use the identity that has access to this private acr"
+      }
+      "share_policy::admin" = {
+        description = "change the membership of the admin policy for this private acr"
+      }
+      "share_policy::user" = {
+        description = "change the membership of the user policy for this private acr"
+      }
+    }
+    ownerRoleName = "admin"
+    roles = {
+      admin = {
+        roleActions = ["delete", "read_policies", "use", "share_policy::admin", "share_policy::user", "identify"]
+      }
+      user = {
+        roleActions = ["identify"]
+      }
+    }
+    allowLeaving = false
+    reuseIds = true
+  }
+
+  private_azure_storage_account = {
+    actionPatterns = {
+      delete = {
+        description = "Delete this private azure storage account"
+      }
+      read_policies = {
+        description = "view all policies and policy details for this private azure storage account"
+      }
+      identify = {
+        description = "use the identity that has access to this private azure storage account"
+      }
+      "share_policy::admin" = {
+        description = "change the membership of the admin policy for this private azure storage account"
+      }
+      "share_policy::user" = {
+        description = "change the membership of the user policy for this private azure storage account"
+      }
+    }
+    ownerRoleName = "admin"
+    roles = {
+      admin = {
+        roleActions = ["delete", "read_policies", "use", "share_policy::admin", "share_policy::user", "identify"]
+      }
+      user = {
+        roleActions = ["identify"]
+      }
+    }
+    allowLeaving = false
+    reuseIds = true
+  }
 }
 
 

src/main/resources/sam.conf

@@ -37,8 +37,6 @@ termsOfService {
 oidc {
   authorityEndpoint = ${?OIDC_AUTHORITY_ENDPOINT}
   oidcClientId = ${?OIDC_CLIENT_ID}
-  oidcClientSecret = ${?OIDC_CLIENT_SECRET}
-  legacyGoogleClientId = ${?LEGACY_GOOGLE_CLIENT_ID}
 }
 
 schemaLock {

src/main/resources/swagger/api-docs.yaml

@@ -11,14 +11,8 @@ info:
 servers:
   - url: /
 security:
-  - googleoauth:
-      - openid
-      - email
-      - profile
   - oidc:
       - openid
-      - email
-      - profile
 paths:
   /api/admin/v1/user/{userId}:
     get:
@@ -4638,22 +4632,7 @@ components:
           items:
             type: string
   securitySchemes:
-    googleoauth:
-      type: oauth2
-      flows:
-        implicit:
-          authorizationUrl: https://accounts.google.com/o/oauth2/auth
-          scopes:
-            openid: open id authorization
-            email: email authorization
-            profile: profile authorization
     oidc:
-      type: oauth2
-      flows:
-        authorizationCode:
-          authorizationUrl: /oauth2/authorize
-          tokenUrl: /oauth2/token
-          scopes:
-            openid: open id authorization
-            email: email authorization
-            profile: profile authorization
+      type: openIdConnect
+      openIdConnectUrl: OPEN_ID_CONNECT_URL
+      x-tokenName: id_token

src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala

@@ -34,7 +34,7 @@ import org.broadinstitute.dsde.workbench.google.{
 }
 import org.broadinstitute.dsde.workbench.google2.{GoogleStorageInterpreter, GoogleStorageService}
 import org.broadinstitute.dsde.workbench.model.WorkbenchEmail
-import org.broadinstitute.dsde.workbench.oauth2.{ClientId, ClientSecret, OpenIDConnectConfiguration}
+import org.broadinstitute.dsde.workbench.oauth2.{ClientId, OpenIDConnectConfiguration}
 import org.broadinstitute.dsde.workbench.sam.api.{LivenessRoutes, SamRoutes, StandardSamUserDirectives}
 import org.broadinstitute.dsde.workbench.sam.azure.{AzureService, CrlService}
 import org.broadinstitute.dsde.workbench.sam.config.AppConfig.AdminConfig
@@ -140,8 +140,6 @@ object Boot extends IOApp with LazyLogging {
         OpenIDConnectConfiguration[IO](
           appConfig.oidcConfig.authorityEndpoint,
           ClientId(appConfig.oidcConfig.clientId),
-          oidcClientSecret = appConfig.oidcConfig.clientSecret.map(ClientSecret),
-          extraGoogleClientId = appConfig.oidcConfig.legacyGoogleClientId.map(ClientId),
           extraAuthParams = Some("prompt=login")
         )
       )
@@ -423,7 +421,14 @@ object Boot extends IOApp with LazyLogging {
     )
     val tosService = new TosService(cloudExtensionsInitializer.cloudExtensions, directoryDAO, config.termsOfServiceConfig)
     val userService =
-      new UserService(directoryDAO, cloudExtensionsInitializer.cloudExtensions, config.blockedEmailDomains, tosService, config.azureServicesConfig)
+      new UserService(
+        directoryDAO,
+        cloudExtensionsInitializer.cloudExtensions,
+        config.blockedEmailDomains,
+        tosService,
+        config.azureServicesConfig,
+        Seq(config.emailDomain)
+      )
     val statusService =
       new StatusService(directoryDAO, cloudExtensionsInitializer.cloudExtensions, 10 seconds, 1 minute)
     val managedGroupService =

src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala

@@ -37,9 +37,7 @@ object AppConfig {
   implicit val oidcReader: ValueReader[OidcConfig] = ValueReader.relative { config =>
     OidcConfig(
       config.getString("authorityEndpoint"),
-      config.getString("oidcClientId"),
-      config.as[Option[String]]("oidcClientSecret"),
-      config.as[Option[String]]("legacyGoogleClientId")
+      config.getString("oidcClientId")
     )
   }
 

src/main/scala/org/broadinstitute/dsde/workbench/sam/config/OidcConfig.scala

@@ -1,3 +1,3 @@
 package org.broadinstitute.dsde.workbench.sam.config
 
-case class OidcConfig(authorityEndpoint: String, clientId: String, clientSecret: Option[String], legacyGoogleClientId: Option[String])
+case class OidcConfig(authorityEndpoint: String, clientId: String)

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupService.scala

@@ -50,7 +50,13 @@ class ManagedGroupService(
       )
 
     validateGroupName(groupId.value)
+    val groupEmail = WorkbenchEmail(constructEmail(groupId.value))
     for {
+      _ <- directoryDAO.loadSubjectFromEmail(groupEmail, samRequestContext).flatMap {
+        case Some(_) =>
+          IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Conflict, s"subject with email $groupEmail already exists")))
+        case None => IO.pure(())
+      }
       managedGroup <- resourceService.createResource(
         managedGroupType,
         groupId,

src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala

@@ -29,7 +29,8 @@ class UserService(
     val cloudExtensions: CloudExtensions,
     blockedEmailDomains: Seq[String],
     tosService: TosService,
-    azureConfig: Option[AzureServicesConfig] = None
+    azureConfig: Option[AzureServicesConfig] = None,
+    nonInvitableDomains: Seq[String] = Seq.empty
 )(implicit
     val executionContext: ExecutionContext
 ) extends LazyLogging {
@@ -247,7 +248,7 @@ class UserService(
 
   def inviteUser(inviteeEmail: WorkbenchEmail, samRequestContext: SamRequestContext): IO[UserStatusDetails] =
     for {
-      _ <- validateEmailAddress(inviteeEmail, blockedEmailDomains)
+      _ <- validateEmailAddress(inviteeEmail, blockedEmailDomains, nonInvitableDomains)
       existingSubject <- directoryDAO.loadSubjectFromEmail(inviteeEmail, samRequestContext)
       createdUser <- existingSubject match {
         case None => createUserInternal(SamUser(genWorkbenchUserId(System.currentTimeMillis()), None, inviteeEmail, None, false), samRequestContext)
@@ -436,14 +437,26 @@ class UserService(
 
   // moved this method from the UserService companion object into this class
   // because Mockito would not let us spy/mock the static method
-  def validateEmailAddress(email: WorkbenchEmail, blockedEmailDomains: Seq[String]): IO[Unit] =
+  def validateEmailAddress(email: WorkbenchEmail, blockedEmailDomains: Seq[String], nonInvitableDomain: Seq[String]): IO[Unit] =
     email.value match {
-      case emailString if blockedEmailDomains.exists(domain => emailString.endsWith("@" + domain) || emailString.endsWith("." + domain)) =>
+      case emailString if matchesBadDomain(emailString, blockedEmailDomains) =>
         IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, s"email domain not permitted [${email.value}]")))
+      case emailString if matchesBadDomain(emailString, nonInvitableDomain) =>
+        IO.raiseError(
+          new WorkbenchExceptionWithErrorReport(
+            ErrorReport(
+              StatusCodes.BadRequest,
+              s"Email domain cannot be invited [${email.value}]. If you are trying to invite a group, please make sure that group exists before adding it to a resource policy."
+            )
+          )
+        )
       case UserService.emailRegex() => IO.unit
       case _ => IO.raiseError(new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, s"invalid email address [${email.value}]")))
     }
 
+  private def matchesBadDomain(emailString: String, badDomains: Seq[String]): Boolean =
+    badDomains.exists(domain => emailString.endsWith("@" + domain) || emailString.endsWith("." + domain))
+
   def getUserAllowances(samUser: SamUser, samRequestContext: SamRequestContext): IO[SamUserAllowances] =
     for {
       tosStatus <- tosService.getTermsOfServiceComplianceStatus(samUser, samRequestContext)

src/test/resources/reference.conf

@@ -111,8 +111,6 @@ testStuff = {
 oidc {
   authorityEndpoint = "https://accounts.google.com"
   oidcClientId = "some-client"
-  oidcClientSecret = "some-secret"
-  legacyGoogleClientId = "another-client"
 }
 
 liquibase {

src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupServiceSpec.scala

@@ -152,7 +152,7 @@ class ManagedGroupServiceSpec
     val exception = intercept[WorkbenchExceptionWithErrorReport] {
       runAndWait(managedGroupService.createManagedGroup(ResourceId(groupName), dummyUser, samRequestContext = samRequestContext))
     }
-    exception.getMessage should include("A resource of this type and name already exists")
+    exception.getMessage should include(s"subject with email $groupName@$testDomain already exists")
     managedGroupService.loadManagedGroup(resourceId, samRequestContext).unsafeRunSync() shouldEqual None
   }