From 51e282804d0b17a1489599c09a7d55a971e74ee6 Mon Sep 17 00:00:00 2001 From: aaronegrant Date: Thu, 6 Jun 2024 09:34:12 -0600 Subject: [PATCH] Testing create-bee failures with azureconfig update --- env/local.env | 1 + env/test.env | 3 ++ src/main/resources/sam.conf | 3 ++ .../workbench/sam/azure/AzureService.scala | 42 ++++++++++++++++--- .../dsde/workbench/sam/config/AppConfig.scala | 4 ++ .../sam/config/AzureServicesConfig.scala | 4 ++ .../workbench/sam/api/TestSamRoutes.scala | 8 ++-- .../sam/azure/AzureServiceSpec.scala | 33 +++++++++++++++ .../UserServiceSpecs/CreateUserSpec.scala | 16 ++++++- 9 files changed, 103 insertions(+), 11 deletions(-) diff --git a/env/local.env b/env/local.env index e42196a1b..dc6d48a41 100644 --- a/env/local.env +++ b/env/local.env @@ -5,6 +5,7 @@ export ADMIN_SERVICE_ACCOUNT_4="src/main/resources/rendered/admin-service-accoun export ADMIN_SERVICE_ACCOUNT_5="src/main/resources/rendered/admin-service-account-5.json" export SERVICE_ACCOUNT_ADMINS="service-admin@dev.test.firecloud.org, service-admin2@dev.test.firecloud.org" export AZURE_ENABLED="false" +export AZURE_SERVICE_CATALOG_APPS_ENABLED="false" export EMAIL_DOMAIN="dev.test.firecloud.org" export ENVIRONMENT="dev" export GOOGLE_APPS_DOMAIN="test.firecloud.org" diff --git a/env/test.env b/env/test.env index 54a7e6a39..d3125fd67 100644 --- a/env/test.env +++ b/env/test.env @@ -4,6 +4,9 @@ export CREATE_BEE_WORKFLOW_TEST="true" export AZURE_MANAGED_APP_CLIENT_ID="foo" export AZURE_MANAGED_APP_CLIENT_SECRET="foo" export AZURE_MANAGED_APP_TENANT_ID="foo" +export AZURE_MANAGED_APP_WORKLOAD_CLIENT_ID="foo" +export AZURE_ALLOW_MANAGED_IDENTITY_USER_CREATION="false" +export AZURE_SERVICE_CATALOG_APPS_ENABLED="false" export EMAIL_DOMAIN="dev.test.firecloud.org" export ENVIRONMENT="local" export GOOGLE_APPS_DOMAIN="test.firecloud.org" diff --git a/src/main/resources/sam.conf b/src/main/resources/sam.conf index 29d7860ff..98ca1614b 100644 --- a/src/main/resources/sam.conf +++ b/src/main/resources/sam.conf @@ -187,6 +187,9 @@ azureServices { managedAppClientId = ${?AZURE_MANAGED_APP_CLIENT_ID} managedAppClientSecret = ${?AZURE_MANAGED_APP_CLIENT_SECRET} managedAppTenantId = ${?AZURE_MANAGED_APP_TENANT_ID} + authorizedUserKey = "authorizedTerraUser"; + managedAppTypeServiceCatalog = "ServiceCatalog"; + managedAppWorkloadClientId = ${?AZURE_MANAGED_APP_WORKLOAD_CLIENT_ID} managedAppPlans = [ { name = "terra-prod" diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala index 873d904b1..44e38e746 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala @@ -46,7 +46,12 @@ class AzureService( def createManagedResourceGroup(managedResourceGroup: ManagedResourceGroup, samRequestContext: SamRequestContext): IO[Unit] = for { - _ <- validateManagedResourceGroup(managedResourceGroup.managedResourceGroupCoordinates, samRequestContext) + _ <- + if (config.azureServiceCatalogAppsEnabled) { + validateServiceCatalogManagedResourceGroup(managedResourceGroup.managedResourceGroupCoordinates, samRequestContext) + } else { + validateManagedResourceGroup(managedResourceGroup.managedResourceGroupCoordinates, samRequestContext) + } existingByCoords <- azureManagedResourceGroupDAO.getManagedResourceGroupByCoordinates( managedResourceGroup.managedResourceGroupCoordinates, @@ -267,22 +272,49 @@ class AzureService( appsInSubscription <- IO(appManager.applications().list().asScala.toSeq) managedApp <- IO.fromOption(appsInSubscription.find(_.managedResourceGroupId() == mrg.id()))(managedAppValidationFailure) plan <- validatePlan(managedApp, crlService.getManagedAppPlans) - _ <- if (validateUser) validateAuthorizedAppUser(managedApp, plan, samRequestContext) else IO.unit + _ <- if (validateUser) validateAuthorizedAppUser(managedApp, plan.authorizedUserKey, samRequestContext) else IO.unit + } yield mrg + } + + /** Validates a managed resource group deployed from Azure Service Catalog. Service Catalog apps do not contain a "plan" or publisher. Algorithm: + * 1. Resolve the MRG in Azure 2. Get the managed app id from the MRG 3. Resolve the managed app 4. Validate the app kind is "ServiceCatalog" 5. Validate + * that the caller is on the list of authorized users for the app + */ + private def validateServiceCatalogManagedResourceGroup( + mrgCoords: ManagedResourceGroupCoordinates, + samRequestContext: SamRequestContext, + validateUser: Boolean = true + ): IO[ResourceGroup] = + traceIOWithContext("validateServiceCatalogManagedResourceGroup", samRequestContext) { _ => + for { + resourceManager <- crlService.buildResourceManager(mrgCoords.tenantId, mrgCoords.subscriptionId) + mrg <- lookupMrg(mrgCoords, resourceManager) + appManager <- crlService.buildApplicationManager(mrgCoords.tenantId, mrgCoords.subscriptionId) + appsInSubscription <- IO(appManager.applications().list().asScala) + managedApp <- IO.fromOption(appsInSubscription.find(_.managedResourceGroupId() == mrg.id()))(managedAppValidationFailure) + _ <- + if (managedApp.kind() == config.managedAppTypeServiceCatalog && validateUser) { + validateAuthorizedAppUser( + managedApp, + config.authorizedUserKey, + samRequestContext + ) + } else IO.unit } yield mrg } /** The users authorized to setup a managed application are stored as a comma separated list of email addresses in the parameters of the application. The * azure api is java so this code needs to deal with possible nulls and java Maps. Also the application parameters are untyped, fun. * @param app - * @param plan + * @param authorizedUserKey * @param samRequestContext * @return */ - private def validateAuthorizedAppUser(app: Application, plan: ManagedAppPlan, samRequestContext: SamRequestContext): IO[Unit] = { + private def validateAuthorizedAppUser(app: Application, authorizedUserKey: String, samRequestContext: SamRequestContext): IO[Unit] = { val authorizedUsersValue = for { parametersObj <- Option(app.parameters()) if parametersObj.isInstanceOf[java.util.Map[_, _]] parametersMap = parametersObj.asInstanceOf[java.util.Map[_, _]] - paramValuesObj <- Option(parametersMap.get(plan.authorizedUserKey)) if paramValuesObj.isInstanceOf[java.util.Map[_, _]] + paramValuesObj <- Option(parametersMap.get(authorizedUserKey)) if paramValuesObj.isInstanceOf[java.util.Map[_, _]] paramValues = paramValuesObj.asInstanceOf[java.util.Map[_, _]] authorizedUsersValue <- Option(paramValues.get("value")) } yield authorizedUsersValue.toString 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 8af2a2859..396558e46 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 @@ -174,6 +174,10 @@ object AppConfig { if (azureEnabled) { Option( AzureServicesConfig( + config.getBoolean("azureServiceCatalogAppsEnabled"), + config.getString("authorizedUserKey"), + config.getString("managedAppTypeServiceCatalog"), + config.getString("managedAppWorkloadClientId"), config.getString("managedAppClientId"), config.getString("managedAppClientSecret"), config.getString("managedAppTenantId"), diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AzureServicesConfig.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AzureServicesConfig.scala index eb5df244b..85b0b7e00 100644 --- a/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AzureServicesConfig.scala +++ b/src/main/scala/org/broadinstitute/dsde/workbench/sam/config/AzureServicesConfig.scala @@ -2,6 +2,10 @@ package org.broadinstitute.dsde.workbench.sam.config case class ManagedAppPlan(name: String, publisher: String, authorizedUserKey: String) case class AzureServicesConfig( + azureServiceCatalogAppsEnabled: Boolean, + authorizedUserKey: String, + managedAppTypeServiceCatalog: String, + managedAppWorkloadClientId: String, managedAppClientId: String, managedAppClientSecret: String, managedAppTenantId: String, diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala index 1cdf21201..e9034d857 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/api/TestSamRoutes.scala @@ -209,10 +209,10 @@ object TestSamRoutes { val mockStatusService = new StatusService(directoryDAO, cloudXtns) val defaultManagedAppPlan: ManagedAppPlan = ManagedAppPlan("mock-plan", "mock-publisher", "mock-auth-user-key") val mockAzureServicesConfig = AzureServicesConfig( - // azureServiceCatalogAppsEnabled = false, - // "mock-auth-user-key", - // "mock-kind", - // "mock-managedapp-workload-clientid", + azureServiceCatalogAppsEnabled = false, + "mock-auth-user-key", + "mock-kind", + "mock-managedapp-workload-clientid", "mock-managedapp-clientid", "mock-managedapp-clientsecret", "mock-managedapp-tenantid", diff --git a/src/test/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureServiceSpec.scala b/src/test/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureServiceSpec.scala index 62d7a63b6..273947156 100644 --- a/src/test/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureServiceSpec.scala +++ b/src/test/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureServiceSpec.scala @@ -294,6 +294,12 @@ class AzureServiceSpec(_system: ActorSystem) mockMrgDAO ) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + + when(mockCrlService.getManagedAppPlans) + .thenReturn(Seq(MockCrlService.defaultManagedAppPlan)) + svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() mockMrgDAO.mrgs should contain(managedResourceGroup) } @@ -340,6 +346,9 @@ class AzureServiceSpec(_system: ActorSystem) mockMrgDAO ) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + mockMrgDAO.insertManagedResourceGroup(managedResourceGroup.copy(billingProfileId = BillingProfileId("no the same")), samRequestContext).unsafeRunSync() val err = intercept[WorkbenchExceptionWithErrorReport] { svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() @@ -363,6 +372,9 @@ class AzureServiceSpec(_system: ActorSystem) mockMrgDAO ) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + when(mockCrlService.getManagedAppPlans) .thenReturn(Seq(MockCrlService.defaultManagedAppPlan)) @@ -387,6 +399,9 @@ class AzureServiceSpec(_system: ActorSystem) val mockAzureServicesConfig = mock[AzureServicesConfig] val svc = new AzureService(mockAzureServicesConfig, MockCrlService(user), new MockDirectoryDAO(), mockMrgDAO) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + val err = intercept[WorkbenchExceptionWithErrorReport] { svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() } @@ -415,6 +430,9 @@ class AzureServiceSpec(_system: ActorSystem) .head when(mockApplication.managedResourceGroupId()).thenReturn("something else") + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + val err = intercept[WorkbenchExceptionWithErrorReport] { svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() } @@ -444,6 +462,9 @@ class AzureServiceSpec(_system: ActorSystem) when(mockApplication.plan()) .thenReturn(null) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + val err = intercept[WorkbenchExceptionWithErrorReport] { svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() } @@ -472,6 +493,9 @@ class AzureServiceSpec(_system: ActorSystem) when(mockApplication.plan()) .thenReturn(new Plan().withName(MockCrlService.defaultManagedAppPlan.name).withPublisher("other publisher")) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + val err = intercept[WorkbenchExceptionWithErrorReport] { svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() } @@ -500,6 +524,9 @@ class AzureServiceSpec(_system: ActorSystem) when(mockApplication.plan()) .thenReturn(new Plan().withName("other name").withPublisher(MockCrlService.defaultManagedAppPlan.publisher)) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + val err = intercept[WorkbenchExceptionWithErrorReport] { svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() } @@ -521,6 +548,9 @@ class AzureServiceSpec(_system: ActorSystem) mockMrgDAO ) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + val err = intercept[WorkbenchExceptionWithErrorReport] { svc .createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user.map(_.copy(email = WorkbenchEmail("other@email.com"))))) @@ -562,6 +592,9 @@ class AzureServiceSpec(_system: ActorSystem) when(mockApplication.parameters()) .thenReturn(parameters) + when(mockAzureServicesConfig.azureServiceCatalogAppsEnabled) + .thenReturn(false) + val err = intercept[WorkbenchExceptionWithErrorReport] { svc.createManagedResourceGroup(managedResourceGroup, samRequestContext.copy(samUser = user)).unsafeRunSync() } 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 3940a4823..21ded059a 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 @@ -76,7 +76,13 @@ class CreateUserSpec extends UserServiceTestTraits { val directoryDAO: DirectoryDAO = MockDirectoryDaoBuilder(allUsersGroup).build val cloudExtensions: CloudExtensions = MockCloudExtensionsBuilder(allUsersGroup).build val userService: UserService = - new UserService(directoryDAO, cloudExtensions, Seq.empty, defaultTosService, Some(AzureServicesConfig("", "", "", Seq.empty, true))) + new UserService( + directoryDAO, + cloudExtensions, + Seq.empty, + defaultTosService, + Some(AzureServicesConfig(azureServiceCatalogAppsEnabled = false, "", "", "", "", "", "", Seq.empty, allowManagedIdentityUserCreation = true)) + ) // Act val newUsersStatus = runAndWait(userService.createUser(newAzureUser, samRequestContext)) @@ -284,7 +290,13 @@ class CreateUserSpec extends UserServiceTestTraits { val directoryDAO: DirectoryDAO = MockDirectoryDaoBuilder(allUsersGroup).build val cloudExtensions: CloudExtensions = MockCloudExtensionsBuilder(allUsersGroup).build val userServiceDisabledFeature: UserService = - new UserService(directoryDAO, cloudExtensions, Seq.empty, defaultTosService, Some(AzureServicesConfig("", "", "", Seq.empty, false))) + new UserService( + directoryDAO, + cloudExtensions, + Seq.empty, + defaultTosService, + Some(AzureServicesConfig(azureServiceCatalogAppsEnabled = false, "", "", "", "", "", "", Seq.empty, allowManagedIdentityUserCreation = false)) + ) // Act and Assert assertThrows[WorkbenchExceptionWithErrorReport] {