diff --git a/.github/workflows/tag.yml b/.github/workflows/tag.yml index 42b9e5363..8a91e5cdf 100644 --- a/.github/workflows/tag.yml +++ b/.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 diff --git a/.github/workflows/verify_consumer_pacts.yml b/.github/workflows/verify_consumer_pacts.yml index c7c54b6ee..29935e8f3 100644 --- a/.github/workflows/verify_consumer_pacts.yml +++ b/.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 diff --git a/automation/project/Dependencies.scala b/automation/project/Dependencies.scala index 66f2cb8b4..b59d17a17 100644 --- a/automation/project/Dependencies.scala +++ b/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" diff --git a/pact4s/src/test/resources/reference.conf b/pact4s/src/test/resources/reference.conf index 69bd71152..01b10882c 100644 --- a/pact4s/src/test/resources/reference.conf +++ b/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 { diff --git a/project/Dependencies.scala b/project/Dependencies.scala index 3dbe9cd01..c7a786ba2 100644 --- a/project/Dependencies.scala +++ b/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" diff --git a/src/main/resources/reference.conf b/src/main/resources/reference.conf index 02c033dfd..39f6233c8 100644 --- a/src/main/resources/reference.conf +++ b/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 + } } diff --git a/src/main/resources/sam.conf b/src/main/resources/sam.conf index ce7a0e1d3..b7189782b 100644 --- a/src/main/resources/sam.conf +++ b/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 { diff --git a/src/main/resources/swagger/api-docs.yaml b/src/main/resources/swagger/api-docs.yaml index fc0b3a65b..dd17565fa 100755 --- a/src/main/resources/swagger/api-docs.yaml +++ b/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 \ No newline at end of file diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala index 97a326a1e..2f9b647d2 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala +++ b/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 = diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala index 232c9c96b..8654553e3 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AppConfig.scala +++ b/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") ) } diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/OidcConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/OidcConfig.scala index aa79e156f..aa9d7dc14 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/OidcConfig.scala +++ b/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) diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupService.scala index 716346f53..83117303b 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupService.scala +++ b/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, diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala index d49ec08cc..8490a5175 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/service/UserService.scala +++ b/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) diff --git a/src/test/resources/reference.conf b/src/test/resources/reference.conf index 1b81d8194..641f41cfb 100644 --- a/src/test/resources/reference.conf +++ b/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 { diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupServiceSpec.scala index 85b90173a..e734a510b 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/ManagedGroupServiceSpec.scala +++ b/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 } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala index 131f636d4..d00f6491e 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpec.scala @@ -8,7 +8,7 @@ import cats.effect.IO import cats.effect.unsafe.implicits.{global => globalEc} import org.broadinstitute.dsde.workbench.model._ import org.broadinstitute.dsde.workbench.sam.Generator.{arbNonPetEmail => _, _} -import org.broadinstitute.dsde.workbench.sam.TestSupport.{databaseEnabled, databaseEnabledClue} +import org.broadinstitute.dsde.workbench.sam.TestSupport.{databaseEnabled, databaseEnabledClue, truncateAll} import org.broadinstitute.dsde.workbench.sam.dataAccess.{DirectoryDAO, PostgresDirectoryDAO} import org.broadinstitute.dsde.workbench.sam.google.GoogleExtensions import org.broadinstitute.dsde.workbench.sam.matchers.BeSameUserMatcher.beSameUserAs @@ -35,7 +35,7 @@ import scala.concurrent.duration._ // TODO: continue breaking down old UserServiceSpec tests into nested suites // See: https://www.scalatest.org/scaladoc/3.2.3/org/scalatest/Suite.html -class UserServiceSpec(_system: ActorSystem) extends TestKit(_system) with Suite with BeforeAndAfterAll { +class UserServiceSpec(_system: ActorSystem) extends TestKit(_system) with Suite with BeforeAndAfterAll with BeforeAndAfterEach { override def nestedSuites: IndexedSeq[Suite] = IndexedSeq( new CreateUserSpec, @@ -54,6 +54,11 @@ class UserServiceSpec(_system: ActorSystem) extends TestKit(_system) with Suite TestKit.shutdownActorSystem(system) super.afterAll() } + + override def beforeEach(): Unit = { + truncateAll + super.beforeEach() + } } // This test suite is deprecated. It is still used and still has valid tests in it, but it should be broken out @@ -641,7 +646,7 @@ class OldUserServiceSpec(_system: ActorSystem) implicit val arbEmail: Arbitrary[WorkbenchEmail] = Arbitrary(genEmail) forAll { email: WorkbenchEmail => - assert(service.validateEmailAddress(email, Seq.empty).attempt.unsafeRunSync().isRight) + assert(service.validateEmailAddress(email, Seq.empty, Seq.empty).attempt.unsafeRunSync().isRight) } } @@ -662,7 +667,7 @@ class OldUserServiceSpec(_system: ActorSystem) implicit val arbEmail: Arbitrary[WorkbenchEmail] = Arbitrary(genEmail) forAll { email: WorkbenchEmail => - assert(service.validateEmailAddress(email, Seq.empty).attempt.unsafeRunSync().isLeft) + assert(service.validateEmailAddress(email, Seq.empty, Seq.empty).attempt.unsafeRunSync().isLeft) } } @@ -683,7 +688,7 @@ class OldUserServiceSpec(_system: ActorSystem) implicit val arbEmail: Arbitrary[WorkbenchEmail] = Arbitrary(genEmail) forAll { email: WorkbenchEmail => - assert(service.validateEmailAddress(email, Seq.empty).attempt.unsafeRunSync().isLeft) + assert(service.validateEmailAddress(email, Seq.empty, Seq.empty).attempt.unsafeRunSync().isLeft) } } @@ -697,12 +702,17 @@ class OldUserServiceSpec(_system: ActorSystem) implicit val arbEmail: Arbitrary[WorkbenchEmail] = Arbitrary(genEmail) forAll { email: WorkbenchEmail => - assert(service.validateEmailAddress(email, Seq.empty).attempt.unsafeRunSync().isLeft) + assert(service.validateEmailAddress(email, Seq.empty, Seq.empty).attempt.unsafeRunSync().isLeft) } } it should "reject blocked email domain" in { - assert(service.validateEmailAddress(WorkbenchEmail("foo@splat.bar.com"), Seq("bar.com")).attempt.unsafeRunSync().isLeft) - assert(service.validateEmailAddress(WorkbenchEmail("foo@bar.com"), Seq("bar.com")).attempt.unsafeRunSync().isLeft) + assert(service.validateEmailAddress(WorkbenchEmail("foo@splat.bar.com"), Seq("bar.com"), Seq.empty).attempt.unsafeRunSync().isLeft) + assert(service.validateEmailAddress(WorkbenchEmail("foo@bar.com"), Seq("bar.com"), Seq.empty).attempt.unsafeRunSync().isLeft) + } + + it should "reject an un-invitable email domain" in { + assert(service.validateEmailAddress(WorkbenchEmail("foo@splat.bar.com"), Seq.empty, Seq("bar.com")).attempt.unsafeRunSync().isLeft) + assert(service.validateEmailAddress(WorkbenchEmail("foo@bar.com"), Seq.empty, Seq("bar.com")).attempt.unsafeRunSync().isLeft) } } diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/CreateUserSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/CreateUserSpec.scala index 755d2de4c..8c6c55bbd 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/CreateUserSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/service/UserServiceSpecs/CreateUserSpec.scala @@ -395,6 +395,23 @@ class CreateUserSpec extends UserServiceTestTraits { describe("An invited User") { + it("should not be able to be invited with a non-invitable domain") { + // Arrange + val nonInvitableDomain = "non-invitable-domain.com" + val invitedGoogleUser = genWorkbenchUserGoogle.sample.get.copy(email = WorkbenchEmail(s"user@$nonInvitableDomain")) + val directoryDAO = MockDirectoryDaoBuilder(allUsersGroup).build + val cloudExtensions = MockCloudExtensionsBuilder(allUsersGroup).build + val userService = new UserService(directoryDAO, cloudExtensions, Seq.empty, defaultTosService, None, Seq(nonInvitableDomain)) + + // Act + val exception = intercept[WorkbenchExceptionWithErrorReport] { + runAndWait(userService.inviteUser(invitedGoogleUser.email, samRequestContext)) + } + + // Assert + exception.errorReport.message should include("Email domain cannot be invited") + } + it("should be able to be invited with no marketing consent") { // Arrange val invitedGoogleUser = genWorkbenchUserGoogle.sample.get