Merge b94f142 into 3096814

broadinstitute · Feb 5, 2021 · c983f4b · c983f4b
2 parents 3096814 + b94f142
commit c983f4b
Show file tree

Hide file tree

Showing 2 changed files with 37 additions and 0 deletions.
diff --git a/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala b/src/main/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresGroupDAO.scala
@@ -73,10 +73,29 @@ trait PostgresGroupDAO {
     }.toList
   }
 
+  def verifyNoCycles(groupId: GroupPK, memberGroupPKs: List[GroupPK])(implicit session: DBSession): Unit = {
+    if (memberGroupPKs.nonEmpty) {
+      val gmf = GroupMemberFlatTable.syntax("gmf")
+      val g = GroupTable.syntax("g")
+      val groupsCausingCycle =
+        samsql"""select ${g.result.email}
+                 from ${GroupMemberFlatTable as gmf}
+                 join ${GroupTable as g} on ${gmf.groupId} = ${g.id}
+                 where ${gmf.groupId} in ($memberGroupPKs)
+                 and ${gmf.memberGroupId} = $groupId""".map(_.get[WorkbenchEmail](g.resultName.email)).list().apply()
+
+      if (groupsCausingCycle.nonEmpty) {
+        throw new WorkbenchExceptionWithErrorReport(ErrorReport(StatusCodes.BadRequest, s"Could not add member group(s) ${groupsCausingCycle.mkString("[", ",", "]")} because it would cause a cycle"))
+      }
+    }
+  }
+
   protected def insertGroupMemberPKs(groupId: GroupPK, memberGroupPKs: List[GroupPK], memberUserIds: List[WorkbenchUserId])(implicit session: DBSession): Int = {
     if (memberGroupPKs.isEmpty && memberUserIds.isEmpty) {
       0
     } else {
+      verifyNoCycles(groupId, memberGroupPKs)
+
       val insertCount = insertGroupMembersIntoHierarchical(groupId, memberGroupPKs, memberUserIds)
 
       if (insertCount > 0) {

diff --git a/...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala b/...est/scala/org/broadinstitute/dsde/workbench/sam/dataAccess/PostgresDirectoryDAOSpec.scala
@@ -289,6 +289,24 @@ class PostgresDirectoryDAOSpec extends AnyFreeSpec with Matchers with BeforeAndA
           dao.addGroupMember(defaultGroup.id, subGroup.id, samRequestContext).unsafeRunSync() shouldBe true
         }
       }
+
+      "prevents group cycles" in {
+        val subGroup = emptyWorkbenchGroup("subGroup")
+        val badGroup = emptyWorkbenchGroup("badGroup")
+        dao.createGroup(defaultGroup, samRequestContext = samRequestContext).unsafeRunSync()
+        dao.createGroup(subGroup, samRequestContext = samRequestContext).unsafeRunSync()
+        dao.createGroup(badGroup, samRequestContext = samRequestContext).unsafeRunSync()
+
+        dao.addGroupMember(defaultGroup.id, subGroup.id, samRequestContext).unsafeRunSync() shouldBe true
+        dao.addGroupMember(subGroup.id, badGroup.id, samRequestContext).unsafeRunSync() shouldBe true
+
+        val exception = intercept[WorkbenchExceptionWithErrorReport] {
+          dao.addGroupMember(badGroup.id, defaultGroup.id, samRequestContext).unsafeRunSync()
+        }
+
+        exception.errorReport.statusCode shouldBe Some(StatusCodes.BadRequest)
+        exception.errorReport.message should include (defaultGroup.email.value)
+      }
     }
 
     "batchLoadGroupEmail" - {