Merge branch 'develop' into ID-787-get-user-tos-history

# Conflicts:
#	src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala
#	src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala
#	src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala
#	src/main/scala/org/broadinstitute/dsde/workbench/sam/service/TosService.scala
#	src/test/scala/org/broadinstitute/dsde/workbench/sam/service/TosServiceSpec.scala

env/test.env

@@ -34,5 +34,6 @@ export TOS_ENABLED="true"
 export TOS_GRACE_PERIOD_ENABLED="true"
 export TOS_BASE_URL="classpath:tos"
 export TOS_VERSION="1"
+export TOS_PREVIOUS_VERSION="0"
 
 export SAM_LOG_APPENDER="Console-Standard"

pact4s/src/test/resources/reference.conf

@@ -103,9 +103,9 @@ testStuff = {
   }
   # Test MRG in which to create managed identities
   azure {
-    tenantId = "0cb7a640-45a2-4ed6-be9f-63519f86e04b"
-    subscriptionId = "3efc5bdf-be0e-44e7-b1d7-c08931e3c16c"
-    managedResourceGroupName = "mrg-terra-dev-previ-20220930130036"
+    tenantId = "fad90753-2022-4456-9b0a-c7e5b934e408"
+    subscriptionId = "f557c728-871d-408c-a28b-eb6b2141a087"
+    managedResourceGroupName = "e2e-8n6xqg"
   }
 }
 

pact4s/src/test/scala/org/broadinstitute/dsde/workbench/sam/MockTestSupport.scala

@@ -202,7 +202,7 @@ object MockTestSupport extends MockTestSupport {
 
     // Override the withUserAllowInactive in MockSamUserDirectives to include
     // support for user status info request with or without access token
-    override def withUserAllowInactive(samRequestContext: SamRequestContext): Directive1[SamUser] = {
+    override def withUserAllowInactive(samRequestContext: SamRequestContext): Directive1[SamUser] =
       extractRequest.flatMap { request =>
         // Use an extractRequest Directive to capture the headers for debugging purpose
         val headers = request.headers
@@ -221,9 +221,9 @@ object MockTestSupport extends MockTestSupport {
             complete(StatusCodes.Unauthorized)
         }
       }
-    }
 
-    override val adminConfig: AdminConfig = AdminConfig(superAdminsGroup = WorkbenchEmail(""), allowedEmailDomains = Set.empty, serviceAccountAdmins = Set.empty)
+    override val adminConfig: AdminConfig =
+      AdminConfig(superAdminsGroup = WorkbenchEmail(""), allowedEmailDomains = Set.empty, serviceAccountAdmins = Set.empty)
 
     override def asAdminServiceUser: Directive0 = Directive.Empty
   }

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

@@ -3289,7 +3289,6 @@ paths:
         - name: doc
           in: query
           description: Array of strings containing 'termsOfService' and/or 'privacyPolicy'.
-          required: true
           schema:
             type: array
             items:
@@ -3301,7 +3300,7 @@ paths:
             text/plain:
               schema:
                 type: string
-  /termsOfService/v1/user/self:
+  /api/termsOfService/v1/user/self:
     get:
       tags:
         - TermsOfService
@@ -3314,31 +3313,31 @@ paths:
             application/json:
               schema:
                 $ref: '#/components/schemas/UserTermsOfServiceDetails'
-        404:
+        403:
           description: user not found
           content:
             application/json:
               schema:
                 $ref: '#/components/schemas/ErrorReport'
-  /termsOfService/v1/user/self/accept:
-    get:
+  /api/termsOfService/v1/user/self/accept:
+    put:
       tags:
         - TermsOfService
       summary: Accepts the current terms of service for the requesting user.
       operationId: userTermsOfServiceAcceptSelf
       responses:
         204:
           description: OK
-  /termsOfService/v1/user/self/reject:
-    get:
+  /api/termsOfService/v1/user/self/reject:
+    put:
       tags:
         - TermsOfService
       summary: Rejects the current terms of service for the requesting user.
       operationId: userTermsOfServiceRejectSelf
       responses:
         204:
           description: OK
-  /termsOfService/v1/user/{sam_user_id}:
+  /api/termsOfService/v1/user/{sam_user_id}:
     get:
       tags:
         - TermsOfService
@@ -4088,6 +4087,7 @@ components:
         - latestAcceptedVersion
         - acceptedOn
         - permitsSystemUsage
+        - isCurrentVersion
       type: object
       properties:
         latestAcceptedVersion:
@@ -4101,6 +4101,9 @@ components:
           type: boolean
           description: based on the user's currently accepted terms of service version and when the terms of service were last changed,
             should the user still be permitted to use the system?
+        isCurrentVersion:
+          type: boolean
+          description: Is the Terms of Service version the user has accepted the current Terms of Service version of the system.
     TermsOfServiceDetails:
       required:
         - isEnabled
@@ -4302,6 +4305,9 @@ components:
     SamUserRegistrationRequest:
       type: object
       properties:
+        acceptsTermsOfService:
+          type: boolean
+          description: does the user accept the terms of service
         userAttributes:
           $ref: '#/components/schemas/SamUserAttributes'
     UpdateUserRequest:

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/SamRoutes.scala

@@ -64,6 +64,7 @@ abstract class SamRoutes(
     oidcConfig.oauth2Routes ~
     statusRoutes ~
     oldTermsOfServiceRoutes ~
+    publicTermsOfServiceRoutes ~
     withExecutionContext(ExecutionContext.global) {
       withSamRequestContext { samRequestContext =>
         pathPrefix("register")(oldUserRoutes(samRequestContext)) ~
@@ -72,16 +73,15 @@ abstract class SamRoutes(
           // the whitelisted service admin account email is in the header of the request
           serviceAdminRoutes(samRequestContext) ~
           userRoutesV2(samRequestContext) ~
-          publicTermsOfServiceRoutes ~
+          userTermsOfServiceRoutes(samRequestContext) ~
           withActiveUser(samRequestContext) { samUser =>
             val samRequestContextWithUser = samRequestContext.copy(samUser = Option(samUser))
             resourceRoutes(samUser, samRequestContextWithUser) ~
             adminRoutes(samUser, samRequestContextWithUser) ~
             extensionRoutes(samUser, samRequestContextWithUser) ~
             groupRoutes(samUser, samRequestContextWithUser) ~
             azureRoutes(samUser, samRequestContextWithUser) ~
-            userRoutesV1(samUser, samRequestContextWithUser) ~
-            userTermsOfServiceRoutes(samUser, samRequestContextWithUser)
+            userRoutesV1(samUser, samRequestContextWithUser)
           }
         }
       }

src/main/scala/org/broadinstitute/dsde/workbench/sam/api/TermsOfServiceRoutes.scala

@@ -41,25 +41,25 @@ trait TermsOfServiceRoutes extends SamUserDirectives {
 
   def publicTermsOfServiceRoutes: server.Route =
     pathPrefix("termsOfService") {
-      pathPrefix("v1") { // api/termsOfService/v1
+      pathPrefix("v1") { // /termsOfService/v1
         pathEndOrSingleSlash {
           get {
             complete(tosService.getTermsOfServiceConfig())
           }
         } ~
-        pathPrefix("docs") { // api/termsOfService/v1/docs
+        pathPrefix("docs") { // /termsOfService/v1/docs
           pathEndOrSingleSlash {
             get {
               parameters("doc".as[String].?) { (doc: Option[String]) =>
                 val docSet = doc.map(_.split(",").toSet).getOrElse(Set.empty)
                 if (docSet.subsetOf(Set(tosService.termsOfServiceTextKey, tosService.privacyPolicyTextKey)))
-                  complete(tosService.getTermsOfServiceText(docSet))
+                  complete(tosService.getTermsOfServiceTexts(docSet))
                 else
                   complete(StatusCodes.BadRequest)
               }
             }
           } ~
-          pathPrefix("redirect") { // api/termsOfService/v1/docs/redirect
+          pathPrefix("redirect") { // /termsOfService/v1/docs/redirect
             pathEndOrSingleSlash {
               get {
                 complete(StatusCodes.NotImplemented)
@@ -70,67 +70,70 @@ trait TermsOfServiceRoutes extends SamUserDirectives {
       }
     }
 
-  def userTermsOfServiceRoutes(samUser: SamUser, samRequestContext: SamRequestContext): server.Route =
-    pathPrefix("termsOfService") {
-      pathPrefix("v1") {
-        pathPrefix("user") { // api/termsOfService/v1/user
-          pathPrefix("self") { // api/termsOfService/v1/user/self
-            pathEndOrSingleSlash {
-              get {
-                complete {
-                  tosService.getTermsOfServiceDetailsForUser(samUser.id, samRequestContext)
-                }
-              }
-            } ~
-            pathPrefix("accept") { // api/termsOfService/v1/user/accept
-              pathEndOrSingleSlash {
-                put {
-                  complete(tosService.acceptCurrentTermsOfService(samUser.id, samRequestContext).map(_ => StatusCodes.NoContent))
-                }
-              }
-            } ~
-            pathPrefix("reject") { // api/termsOfService/v1/user/reject
-              pathEndOrSingleSlash {
-                put {
-                  complete(tosService.rejectCurrentTermsOfService(samUser.id, samRequestContext).map(_ => StatusCodes.NoContent))
-                }
-              }
-            } ~
-            pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
+  def userTermsOfServiceRoutes(samRequestContextWithoutUser: SamRequestContext): server.Route =
+    withUserAllowInactive(samRequestContextWithoutUser) { samUser: SamUser =>
+      val samRequestContext = samRequestContextWithoutUser.copy(samUser = Some(samUser))
+      pathPrefix("termsOfService") {
+        pathPrefix("v1") {
+          pathPrefix("user") { // api/termsOfService/v1/user
+            pathPrefix("self") { // api/termsOfService/v1/user/self
               pathEndOrSingleSlash {
                 get {
-                  parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
-                    complete {
-                      tosService.getTermsOfServiceHistoryForUser(samUser.id, samRequestContext, limit)
-                    }
+                  complete {
+                    tosService.getTermsOfServiceDetailsForUser(samUser.id, samRequestContext)
                   }
                 }
-              }
-            }
-          } ~
-          // The {user_id} route must be last otherwise it will try to parse the other routes incorrectly as user id's
-          pathPrefix(Segment) { userId => // api/termsOfService/v1/user/{userId}
-            validate(samUserIdPattern.matches(userId), "User ID must be alpha numeric") {
-              val requestUserId = WorkbenchUserId(userId)
-              pathEndOrSingleSlash {
-                get {
-                  complete {
-                    tosService.getTermsOfServiceDetailsForUser(requestUserId, samRequestContext)
+              } ~
+              pathPrefix("accept") { // api/termsOfService/v1/user/accept
+                pathEndOrSingleSlash {
+                  put {
+                    complete(tosService.acceptCurrentTermsOfService(samUser.id, samRequestContext).map(_ => StatusCodes.NoContent))
                   }
                 }
               } ~
+              pathPrefix("reject") { // api/termsOfService/v1/user/reject
+                pathEndOrSingleSlash {
+                  put {
+                    complete(tosService.rejectCurrentTermsOfService(samUser.id, samRequestContext).map(_ => StatusCodes.NoContent))
+                  }
+                }
+              }
+            } ~
               pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
                 pathEndOrSingleSlash {
                   get {
                     parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
-                      complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit))
+                      complete {
+                        tosService.getTermsOfServiceHistoryForUser(samUser.id, samRequestContext, limit)
+                      }
                     }
                   }
                 }
+              } ~
+            // The {user_id} route must be last otherwise it will try to parse the other routes incorrectly as user id's
+            pathPrefix(Segment) { userId => // api/termsOfService/v1/user/{userId}
+              validate(samUserIdPattern.matches(userId), "User ID must be alpha numeric") {
+                val requestUserId = WorkbenchUserId(userId)
+                pathEndOrSingleSlash {
+                  get {
+                    complete {
+                      tosService.getTermsOfServiceDetailsForUser(requestUserId, samRequestContext)
+                    }
+                  }
+                } ~
+                pathPrefix("history") { // api/termsOfService/v1/user/{userId}/history
+                  pathEndOrSingleSlash {
+                    get {
+                      parameters("limit".as[Integer].withDefault(100)) { (limit: Int) =>
+                        complete(tosService.getTermsOfServiceHistoryForUser(requestUserId, samRequestContext, limit))
+                      }                    }
+                  }
+                }
               }
             }
           }
         }
       }
     }
+
 }

src/main/scala/org/broadinstitute/dsde/workbench/sam/azure/AzureService.scala

@@ -22,12 +22,6 @@ import scala.jdk.CollectionConverters._
 
 class AzureService(crlService: CrlService, directoryDAO: DirectoryDAO, azureManagedResourceGroupDAO: AzureManagedResourceGroupDAO) {
 
-  // Static region in which to create all managed identities.
-  // Managed identities are regional resources in Azure but they can be used across
-  // regions. So it's safe to just use a static region and not make this configurable.
-  // See: https://docs.microsoft.com/en-us/azure/active-directory/managed-identities-azure-resources/managed-identities-faq#can-the-same-managed-identity-be-used-across-multiple-regions
-  private val managedIdentityRegion = Region.US_EAST
-
   // Tag on the MRG to specify the Sam billing-profile id
   private val billingProfileTag = "terra.billingProfileId"
 
@@ -111,16 +105,18 @@ class AzureService(crlService: CrlService, directoryDAO: DirectoryDAO, azureMana
       samRequestContext: SamRequestContext
   ): IO[(PetManagedIdentity, Boolean)] =
     for {
-      manager <- crlService.buildMsiManager(mrgCoords.tenantId, mrgCoords.subscriptionId)
+      msiManager <- crlService.buildMsiManager(mrgCoords.tenantId, mrgCoords.subscriptionId)
+      mrgManager <- crlService.buildResourceManager(mrgCoords.tenantId, mrgCoords.subscriptionId)
       petName = toManagedIdentityNameFromUser(user)
-      context = managedIdentityContext(mrgCoords, petName)
+      region <- getRegionFromMrg(mrgCoords, mrgManager, samRequestContext)
+      context = managedIdentityContext(mrgCoords, petName, region)
       azureUami <- traceIOWithContext("createUAMI", samRequestContext) { _ =>
         IO(
           // note that this will not fail when the UAMI already exists
-          manager
+          msiManager
             .identities()
             .define(petName.value)
-            .withRegion(managedIdentityRegion)
+            .withRegion(region)
             .withExistingResourceGroup(mrgCoords.managedResourceGroupName.value)
             .withTags(managedIdentityTags(user).asJava)
             .create(context)
@@ -130,6 +126,11 @@ class AzureService(crlService: CrlService, directoryDAO: DirectoryDAO, azureMana
       createdPet <- directoryDAO.createPetManagedIdentity(petToCreate, samRequestContext)
     } yield (createdPet, true)
 
+  private def getRegionFromMrg(mrgCoords: ManagedResourceGroupCoordinates, mrgManager: ResourceManager, samRequestContext: SamRequestContext) =
+    traceIOWithContext("getRegionFromMrg", samRequestContext) { _ =>
+      IO(mrgManager.resourceGroups().getByName(mrgCoords.managedResourceGroupName.value).region())
+    }
+
   /** Loads a SamUser from the database by email.
     */
   def getSamUser(email: WorkbenchEmail, samRequestContext: SamRequestContext): IO[Option[SamUser]] =
@@ -233,15 +234,15 @@ class AzureService(crlService: CrlService, directoryDAO: DirectoryDAO, azureMana
   private def managedIdentityTags(user: SamUser): Map[String, String] =
     Map("samUserId" -> user.id.value, "samUserEmail" -> user.email.value)
 
-  private def managedIdentityContext(mrgCoords: ManagedResourceGroupCoordinates, petName: ManagedIdentityDisplayName): Context =
+  private def managedIdentityContext(mrgCoords: ManagedResourceGroupCoordinates, petName: ManagedIdentityDisplayName, region: Region): Context =
     Defaults.buildContext(
       CreateUserAssignedManagedIdentityRequestData
         .builder()
         .setTenantId(mrgCoords.tenantId.value)
         .setSubscriptionId(mrgCoords.subscriptionId.value)
         .setResourceGroupName(mrgCoords.managedResourceGroupName.value)
         .setName(petName.value)
-        .setRegion(managedIdentityRegion)
+        .setRegion(region)
         .build()
     )