From a3c8e518f7895f83aeb6a34e66c85e43a342ed54 Mon Sep 17 00:00:00 2001 From: dvoet Date: Fri, 26 Apr 2024 16:35:12 -0400 Subject: [PATCH 1/2] TOAZ-351 openIdConnect in api-docs.yaml (#1410) --- pact4s/src/test/resources/reference.conf | 2 -- project/Dependencies.scala | 4 +-- src/main/resources/sam.conf | 2 -- src/main/resources/swagger/api-docs.yaml | 27 +++---------------- .../dsde/workbench/sam/Boot.scala | 4 +-- .../dsde/workbench/sam/config/AppConfig.scala | 4 +-- .../workbench/sam/config/OidcConfig.scala | 2 +- src/test/resources/reference.conf | 2 -- 8 files changed, 8 insertions(+), 39 deletions(-) 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/sam.conf b/src/main/resources/sam.conf index d4104ce91..d9bd1d758 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..a1129e4b3 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") ) ) 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 5ff886fff..8af2a2859 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/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 { From 48128813f68bd713fe95af7d006196cc215fe223 Mon Sep 17 00:00:00 2001 From: Trevyn Langsford Date: Mon, 29 Apr 2024 12:52:40 -0400 Subject: [PATCH 2/2] ID-614 Ensure Email Uniqueness for Managed Groups (#1411) * ID-614 Ensure Email Uniqueness for Managed Groups * a better error message --- .../dsde/workbench/sam/Boot.scala | 9 ++++++- .../sam/service/ManagedGroupService.scala | 6 +++++ .../workbench/sam/service/UserService.scala | 21 ++++++++++++--- .../sam/service/ManagedGroupServiceSpec.scala | 2 +- .../sam/service/UserServiceSpec.scala | 26 +++++++++++++------ .../UserServiceSpecs/CreateUserSpec.scala | 17 ++++++++++++ 6 files changed, 67 insertions(+), 14 deletions(-) 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 a1129e4b3..2f9b647d2 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/Boot.scala @@ -421,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/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/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 f6953a893..3940a4823 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 @@ -383,6 +383,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