fix merge conflicts

src/main/resources/org/broadinstitute/dsde/sam/liquibase/changelog.xml

@@ -25,4 +25,5 @@
     <include file="changesets/20221115_azure_managed_resource_group.xml" relativeToChangelogFile="true"/>
     <include file="changesets/20230203_last_quota_error.xml" relativeToChangelogFile="true"/>
     <include file="changesets/20230328_add_date_columns_to_user.xml" relativeToChangelogFile="true"/>
+    <include file="changesets/20230929_add_tos_audit_table.xml" relativeToChangelogFile="true"/>
 </databaseChangeLog>

src/main/resources/org/broadinstitute/dsde/sam/liquibase/changesets/20230929_add_tos_audit_table.xml

@@ -0,0 +1,40 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>
+<databaseChangeLog logicalFilePath="dummy"
+  xmlns="http://www.liquibase.org/xml/ns/dbchangelog"
+  xmlns:ext="http://www.liquibase.org/xml/ns/dbchangelog-ext"
+  xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+  xsi:schemaLocation="http://www.liquibase.org/xml/ns/dbchangelog-ext http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-ext.xsd http://www.liquibase.org/xml/ns/dbchangelog http://www.liquibase.org/xml/ns/dbchangelog/dbchangelog-3.4.xsd">
+
+  <changeSet logicalFilePath="dummy" author="tgarwood" id="add_tos_audit_table">
+    <createTable tableName="SAM_USER_TERMS_OF_SERVICE">
+      <column name="sam_user_id" type="VARCHAR">
+        <constraints primaryKey="true" foreignKeyName="FK_TOS_USER_ID" referencedTableName="SAM_USER" referencedColumnNames="id" deleteCascade="true"/>
+      </column>
+      <column name="version" type="VARCHAR(40)">
+        <constraints primaryKey="true"/>
+      </column>
+      <column name="action" type="VARCHAR">
+        <constraints primaryKey="true"/>
+      </column>
+      <column name="created_at" type="timestamptz"/>
+    </createTable>
+
+    <sql stripComments="true">
+      INSERT INTO sam_user_terms_of_service
+      (sam_user_id,
+       version,
+       action,
+       created_at)
+      SELECT su.id                    as sam_user_id,
+             su.accepted_tos_version  as version,
+             'ACCEPT'                 as action,
+       '1970-01-01 00:00:00-00' as created_at
+      FROM sam_user su
+      WHERE su.accepted_tos_version is not null;
+    </sql>
+
+      <dropColumn tableName="sam_user">
+      <column name="accepted_tos_version"/>
+    </dropColumn>
+  </changeSet>
+</databaseChangeLog>

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

@@ -61,7 +61,7 @@ trait StandardSamUserDirectives extends SamUserDirectives with LazyLogging with
     val googleSubjectId = (oidcHeaders.externalId.left.toOption ++ oidcHeaders.googleSubjectIdFromAzure).headOption
     val azureB2CId = oidcHeaders.externalId.toOption // .right is missing (compared to .left above) since Either is Right biased
 
-    SamUser(genWorkbenchUserId(System.currentTimeMillis()), googleSubjectId, oidcHeaders.email, azureB2CId, false, None)
+    SamUser(genWorkbenchUserId(System.currentTimeMillis()), googleSubjectId, oidcHeaders.email, azureB2CId, false)
   }
 
   /** Utility function that knows how to convert all the various headers into OIDCHeaders
@@ -122,7 +122,7 @@ object StandardSamUserDirectives {
   def getActiveSamUser(oidcHeaders: OIDCHeaders, userService: UserService, tosService: TosService, samRequestContext: SamRequestContext): IO[SamUser] =
     for {
       user <- getSamUser(oidcHeaders, userService, samRequestContext)
-      tosComplianceDetails <- tosService.getTosComplianceStatus(user)
+      tosComplianceDetails <- tosService.getTosComplianceStatus(user, samRequestContext)
     } yield {
       if (!tosComplianceDetails.permitsSystemUsage) {
         throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.Unauthorized, "User must accept the latest terms of service."))

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

@@ -66,7 +66,7 @@ trait UserRoutes extends SamUserDirectives with SamRequestContextDirectives {
                 get {
                   withUserAllowInactive(samRequestContext) { samUser =>
                     complete {
-                      tosService.getTosComplianceStatus(samUser).map { tosAcceptanceStatus =>
+                      tosService.getTosComplianceStatus(samUser, samRequestContext).map { tosAcceptanceStatus =>
                         StatusCodes.OK -> Option(JsBoolean(tosAcceptanceStatus.permitsSystemUsage))
                       }
                     }
@@ -139,12 +139,12 @@ trait UserRoutes extends SamUserDirectives with SamRequestContextDirectives {
                 } ~
                 path("termsOfServiceDetails") {
                   get {
-                    complete(tosService.getTosDetails(user))
+                    complete(tosService.getTosDetails(user, samRequestContext))
                   }
                 } ~
                 path("termsOfServiceComplianceStatus") {
                   get {
-                    complete(tosService.getTosComplianceStatus(user))
+                    complete(tosService.getTosComplianceStatus(user, samRequestContext))
                   }
                 }
             }

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/DirectoryDAO.scala

@@ -4,7 +4,7 @@ import cats.effect.IO
 import org.broadinstitute.dsde.workbench.model._
 import org.broadinstitute.dsde.workbench.model.google.ServiceAccountSubjectId
 import org.broadinstitute.dsde.workbench.sam.azure.{ManagedIdentityObjectId, PetManagedIdentity, PetManagedIdentityId}
-import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUser}
+import org.broadinstitute.dsde.workbench.sam.model.{BasicWorkbenchGroup, SamUser, SamUserTos}
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 
 import java.time.Instant
@@ -74,7 +74,8 @@ trait DirectoryDAO {
   def setGoogleSubjectId(userId: WorkbenchUserId, googleSubjectId: GoogleSubjectId, samRequestContext: SamRequestContext): IO[Unit]
 
   def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean]
-  def rejectTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean]
+  def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean]
+  def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]]
 
   def createPetManagedIdentity(petManagedIdentity: PetManagedIdentity, samRequestContext: SamRequestContext): IO[PetManagedIdentity]
   def loadPetManagedIdentity(petManagedIdentityId: PetManagedIdentityId, samRequestContext: SamRequestContext): IO[Option[PetManagedIdentity]]

src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAO.scala

@@ -353,7 +353,6 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
                   ${userColumn.googleSubjectId},
                   ${userColumn.enabled},
                   ${userColumn.azureB2cId},
-                  ${userColumn.acceptedTosVersion},
                   ${userColumn.createdAt},
                   ${userColumn.registeredAt},
                   ${userColumn.updatedAt})
@@ -363,7 +362,6 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
                   ${newUser.googleSubjectId},
                   ${newUser.enabled},
                   ${newUser.azureB2CId},
-                  ${newUser.acceptedTosVersion},
                   ${newUser.createdAt},
                   ${newUser.registeredAt},
                   ${newUser.updatedAt})"""
@@ -631,25 +629,38 @@ class PostgresDirectoryDAO(protected val writeDbRef: DbReference, protected val
       }
     }
 
-  override def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] =
+  override def acceptTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = {
+    val tosTable = TosTable.syntax
+    val tosColumns = TosTable.column
     serializableWriteTransaction("acceptTermsOfService", samRequestContext) { implicit session =>
-      val u = UserTable.column
-      samsql"""update ${UserTable.table}
-               set (${u.acceptedTosVersion}, ${u.updatedAt}) =
-               (${tosVersion}, ${Instant.now()})
-               where ${u.id} = ${userId}
-                and (${u.acceptedTosVersion} is null
-                or ${u.acceptedTosVersion} != ${tosVersion})""".update().apply() > 0
+      samsql"""insert into ${TosTable as tosTable} (${tosColumns.samUserId}, ${tosColumns.version}, ${tosColumns.action}, ${tosColumns.createdAt})
+               values ($userId, $tosVersion, ${TosTable.ACCEPT}, ${Instant.now()})""".update().apply() > 0
     }
+  }
 
-  override def rejectTermsOfService(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] =
+  override def rejectTermsOfService(userId: WorkbenchUserId, tosVersion: String, samRequestContext: SamRequestContext): IO[Boolean] = {
+    val tosTable = TosTable.syntax
+    val tosColumns = TosTable.column
     serializableWriteTransaction("rejectTermsOfService", samRequestContext) { implicit session =>
-      val u = UserTable.column
-      samsql"""update ${UserTable.table}
-               set (${u.acceptedTosVersion}, ${u.updatedAt}) =
-               (null, ${Instant.now()})
-               where ${u.id} = ${userId}
-                and ${u.acceptedTosVersion} is not null""".update().apply() > 0
+      samsql"""insert into ${TosTable as tosTable} (${tosColumns.samUserId}, ${tosColumns.version}, ${tosColumns.action}, ${tosColumns.createdAt})
+         values ($userId, $tosVersion, ${TosTable.REJECT}, ${Instant.now()})""".update().apply() > 0
+    }
+  }
+
+  override def getUserTos(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Option[SamUserTos]] =
+    readOnlyTransaction("getUserTos", samRequestContext) { implicit session =>
+      val tosTable = TosTable.syntax
+      val column = TosTable.column
+
+      val loadUserTosQuery =
+        samsql"""select ${tosTable.resultAll}
+              from ${TosTable as tosTable}
+              where ${column.samUserId} = ${userId}
+              order by ${column.createdAt} desc
+              limit 1"""
+
+      val userTosRecordOpt: Option[TosRecord] = loadUserTosQuery.map(TosTable(tosTable)).first().apply()
+      userTosRecordOpt.map(TosTable.unmarshalUserRecord)
     }
 
   override def isEnabled(subject: WorkbenchSubject, samRequestContext: SamRequestContext): IO[Boolean] =

src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/TosTable.scala

@@ -0,0 +1,39 @@
+package org.broadinstitute.dsde.workbench.sam.db.tables
+
+import org.broadinstitute.dsde.workbench.model._
+import org.broadinstitute.dsde.workbench.sam.db.SamTypeBinders
+import org.broadinstitute.dsde.workbench.sam.model.SamUserTos
+import scalikejdbc._
+
+import java.time.Instant
+
+final case class TosRecord(
+    samUserId: WorkbenchUserId,
+    version: String,
+    action: String,
+    createdAt: Instant
+)
+
+object TosTable extends SQLSyntaxSupportWithDefaultSamDB[TosRecord] {
+  val ACCEPT = "ACCEPT"
+  val REJECT = "REJECT"
+  override def tableName: String = "SAM_USER_TERMS_OF_SERVICE"
+
+  import SamTypeBinders._
+  def apply(e: ResultName[TosRecord])(rs: WrappedResultSet): TosRecord = TosRecord(
+    rs.get(e.samUserId),
+    rs.get(e.version),
+    rs.get(e.action),
+    rs.get(e.createdAt)
+  )
+
+  def apply(o: SyntaxProvider[TosRecord])(rs: WrappedResultSet): TosRecord = apply(o.resultName)(rs)
+
+  def unmarshalUserRecord(tosRecord: TosRecord): SamUserTos =
+    SamUserTos(
+      tosRecord.samUserId,
+      tosRecord.version,
+      tosRecord.action,
+      tosRecord.createdAt
+    )
+}

src/main/scala/org/broadinstitute/dsde/workbench/sam/db/tables/UserTable.scala

@@ -13,7 +13,6 @@ final case class UserRecord(
     googleSubjectId: Option[GoogleSubjectId],
     enabled: Boolean,
     azureB2cId: Option[AzureB2CId],
-    acceptedTosVersion: Option[String],
     createdAt: Instant,
     registeredAt: Option[Instant],
     updatedAt: Instant
@@ -29,7 +28,6 @@ object UserTable extends SQLSyntaxSupportWithDefaultSamDB[UserRecord] {
     rs.stringOpt(e.googleSubjectId).map(GoogleSubjectId),
     rs.get(e.enabled),
     rs.stringOpt(e.azureB2cId).map(AzureB2CId),
-    rs.stringOpt(e.acceptedTosVersion),
     rs.get(e.createdAt),
     rs.timestampOpt(e.registeredAt).map(_.toInstant),
     rs.get(e.updatedAt)
@@ -44,7 +42,6 @@ object UserTable extends SQLSyntaxSupportWithDefaultSamDB[UserRecord] {
       userRecord.email,
       userRecord.azureB2cId,
       userRecord.enabled,
-      userRecord.acceptedTosVersion,
       userRecord.createdAt,
       userRecord.registeredAt,
       userRecord.updatedAt

src/main/scala/org/broadinstitute/dsde/workbench/sam/google/GoogleExtensions.scala

@@ -140,8 +140,7 @@ class GoogleExtensions(
                 Option(googleSubjectId),
                 googleServicesConfig.serviceAccountClientEmail,
                 None,
-                false,
-                None
+                false
               )
               samApplication.userService.createUser(newUser, samRequestContext).map(_ => newUser)
           }
@@ -424,7 +423,7 @@ class GoogleExtensions(
       subject <- directoryDAO.loadSubjectFromEmail(userEmail, samRequestContext)
       key <- subject match {
         case Some(userId: WorkbenchUserId) =>
-          getPetServiceAccountKey(SamUser(userId, None, userEmail, None, false, None), project, samRequestContext).map(Option(_))
+          getPetServiceAccountKey(SamUser(userId, None, userEmail, None, false), project, samRequestContext).map(Option(_))
         case _ => IO.pure(None)
       }
     } yield key
@@ -445,7 +444,7 @@ class GoogleExtensions(
       subject <- directoryDAO.loadSubjectFromEmail(userEmail, samRequestContext)
       key <- subject match {
         case Some(userId: WorkbenchUserId) =>
-          IO.fromFuture(IO(getArbitraryPetServiceAccountKey(SamUser(userId, None, userEmail, None, false, None), samRequestContext))).map(Option(_))
+          IO.fromFuture(IO(getArbitraryPetServiceAccountKey(SamUser(userId, None, userEmail, None, false), samRequestContext))).map(Option(_))
         case _ => IO.none
       }
     } yield key

src/main/scala/org/broadinstitute/dsde/workbench/sam/model/SamModel.scala

@@ -3,8 +3,8 @@ package org.broadinstitute.dsde.workbench.sam.model
 import monocle.macros.Lenses
 import org.broadinstitute.dsde.workbench.model._
 import org.broadinstitute.dsde.workbench.model.google.GoogleModelJsonSupport.InstantFormat
-import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, AccessPolicyMembershipResponse, AdminUpdateUserRequest}
 import org.broadinstitute.dsde.workbench.sam.model.api.SamApiJsonProtocol.PolicyInfoResponseBodyJsonFormat
+import org.broadinstitute.dsde.workbench.sam.model.api.{AccessPolicyMembershipRequest, AccessPolicyMembershipResponse, AdminUpdateUserRequest}
 import org.broadinstitute.dsde.workbench.sam.service.ManagedGroupService.MangedGroupRoleName
 import spray.json.{DefaultJsonProtocol, JsValue, RootJsonFormat}
 
@@ -28,7 +28,7 @@ object SamJsonSupport {
 
   implicit val ResourceTypeFormat = jsonFormat6(ResourceType.apply)
 
-  implicit val SamUserFormat = jsonFormat9(SamUser.apply)
+  implicit val SamUserFormat = jsonFormat8(SamUser.apply)
 
   implicit val UserStatusDetailsFormat = jsonFormat2(UserStatusDetails.apply)
 
@@ -322,10 +322,9 @@ object SamUser {
       googleSubjectId: Option[GoogleSubjectId],
       email: WorkbenchEmail,
       azureB2CId: Option[AzureB2CId],
-      enabled: Boolean,
-      acceptedTosVersion: Option[String]
+      enabled: Boolean
   ): SamUser =
-    SamUser(id, googleSubjectId, email, azureB2CId, enabled, acceptedTosVersion, Instant.EPOCH, None, Instant.EPOCH)
+    SamUser(id, googleSubjectId, email, azureB2CId, enabled, Instant.EPOCH, None, Instant.EPOCH)
 }
 
 final case class SamUser(
@@ -334,7 +333,6 @@ final case class SamUser(
     email: WorkbenchEmail,
     azureB2CId: Option[AzureB2CId],
     enabled: Boolean,
-    acceptedTosVersion: Option[String],
     createdAt: Instant,
     registeredAt: Option[Instant],
     updatedAt: Instant
@@ -347,8 +345,17 @@ final case class SamUser(
       this.googleSubjectId == user.googleSubjectId &&
       this.email == user.email &&
       this.azureB2CId == user.azureB2CId &&
-      this.enabled == user.enabled &&
-      this.acceptedTosVersion == user.acceptedTosVersion
+      this.enabled == user.enabled
+    case _ => false
+  }
+}
+
+final case class SamUserTos(id: WorkbenchUserId, version: String, action: String, createdAt: Instant) {
+  override def equals(other: Any): Boolean = other match {
+    case userTos: SamUserTos =>
+      this.id == userTos.id &&
+      this.version == userTos.version &&
+      this.action == userTos.action
     case _ => false
   }
 }

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

@@ -14,7 +14,8 @@ import org.broadinstitute.dsde.workbench.sam.dataAccess.DirectoryDAO
 import org.broadinstitute.dsde.workbench.sam.errorReportSource
 import org.broadinstitute.dsde.workbench.sam.util.SamRequestContext
 import org.broadinstitute.dsde.workbench.sam.config.TermsOfServiceConfig
-import org.broadinstitute.dsde.workbench.sam.model.{SamUser, TermsOfServiceComplianceStatus, TermsOfServiceDetails}
+import org.broadinstitute.dsde.workbench.sam.db.tables.TosTable
+import org.broadinstitute.dsde.workbench.sam.model.{SamUser, SamUserTos, TermsOfServiceComplianceStatus, TermsOfServiceDetails}
 
 import java.io.{FileNotFoundException, IOException}
 import scala.concurrent.{Await, ExecutionContext}
@@ -40,32 +41,41 @@ class TosService(val directoryDao: DirectoryDAO, val tosConfig: TermsOfServiceCo
 
   def rejectTosStatus(userId: WorkbenchUserId, samRequestContext: SamRequestContext): IO[Boolean] =
     directoryDao
-      .rejectTermsOfService(userId, samRequestContext)
+      .rejectTermsOfService(userId, tosConfig.version, samRequestContext)
       .withInfoLogMessage(s"$userId has rejected version ${tosConfig.version} of the Terms of Service")
 
   @Deprecated
-  def getTosDetails(samUser: SamUser): IO[TermsOfServiceDetails] =
-    IO.pure(TermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, samUser.acceptedTosVersion))
+  def getTosDetails(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceDetails] =
+    directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
+      TermsOfServiceDetails(isEnabled = true, tosConfig.isGracePeriodEnabled, tosConfig.version, tos.map(_.version))
+    }
 
-  def getTosComplianceStatus(samUser: SamUser): IO[TermsOfServiceComplianceStatus] = {
-    val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(samUser)
-    val permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser)
-    IO.pure(TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage))
-  }
+  def getTosComplianceStatus(samUser: SamUser, samRequestContext: SamRequestContext): IO[TermsOfServiceComplianceStatus] =
+    directoryDao.getUserTos(samUser.id, samRequestContext).map { tos =>
+      val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(tos)
+      val permitsSystemUsage = tosAcceptancePermitsSystemUsage(samUser, tos)
+      TermsOfServiceComplianceStatus(samUser.id, userHasAcceptedLatestVersion, permitsSystemUsage)
+    }
 
   /** If grace period enabled, don't check ToS, return true If ToS disabled, return true Otherwise return true if user has accepted ToS, or is a service account
     */
-  private def tosAcceptancePermitsSystemUsage(user: SamUser): Boolean = {
-    val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(user)
-    val userCanUseSystemUnderGracePeriod = tosConfig.isGracePeriodEnabled && user.acceptedTosVersion.isDefined
+  private def tosAcceptancePermitsSystemUsage(user: SamUser, userTos: Option[SamUserTos]): Boolean = {
     val userIsServiceAccount = StandardSamUserDirectives.SAdomain.matches(user.email.value) // Service Account users do not need to accept ToS
-    val tosDisabled = !tosConfig.isTosEnabled
+    val userIsPermitted = userTos.exists { tos =>
+      val userHasAcceptedLatestVersion = userHasAcceptedLatestTosVersion(Option(tos))
+      val userCanUseSystemUnderGracePeriod = tosConfig.isGracePeriodEnabled && tos.action == TosTable.ACCEPT
+      val tosDisabled = !tosConfig.isTosEnabled
 
-    userHasAcceptedLatestVersion || userCanUseSystemUnderGracePeriod || userIsServiceAccount || tosDisabled
+      userHasAcceptedLatestVersion || userCanUseSystemUnderGracePeriod || tosDisabled
+
+    }
+    userIsPermitted || userIsServiceAccount
   }
 
-  private def userHasAcceptedLatestTosVersion(samUser: SamUser): Boolean =
-    samUser.acceptedTosVersion.contains(tosConfig.version)
+  private def userHasAcceptedLatestTosVersion(userTos: Option[SamUserTos]): Boolean =
+    userTos.exists { tos =>
+      tos.version.contains(tosConfig.version) && tos.action == TosTable.ACCEPT
+    }
 }
 
 trait RemoteDocument {