From aa8681d75c8e69f831e3aa9e926d58f9c7484bb2 Mon Sep 17 00:00:00 2001
From: Florian M <florian@scm.io>
Date: Fri, 29 Jan 2021 11:45:49 +0100
Subject: [PATCH 1/4] Prevent deleting teams that are still referenced

---
 CHANGELOG.unreleased.md                |  2 +-
 app/controllers/TeamController.scala   | 29 +++++++++++++------------
 app/models/annotation/Annotation.scala |  6 ++++++
 app/models/binary/DataSet.scala        |  6 ++++++
 app/models/project/Project.scala       | 25 +++++++++++----------
 app/models/task/TaskType.scala         |  5 +++++
 app/models/team/Team.scala             | 30 +++++++++++++++++---------
 app/models/user/User.scala             |  2 +-
 conf/messages                          |  6 ++++++
 9 files changed, 74 insertions(+), 37 deletions(-)

diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md
index 9d20c6fa749..bffa856f44a 100644
--- a/CHANGELOG.unreleased.md
+++ b/CHANGELOG.unreleased.md
@@ -20,7 +20,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
 - Users are no longer allowed to deactivate their own accounts.  [#5070](https://github.com/scalableminds/webknossos/pull/5070)
 
 ### Fixed
--
+- Fixed a bug where the user could delete teams that were still referenced in annotations, projects or task types, thus creating invalid state. [#5107](https://github.com/scalableminds/webknossos/pull/5107/files)
 
 ### Removed
 - The isosurface setting was removed. Instead, isosurfaces can be generated via the "Meshes" tab. Also note that the Shift+Click binding for generating an isosurface was removed (for now). Please refer to the "Meshes" tab, too. [#4917](https://github.com/scalableminds/webknossos/pull/4917)
diff --git a/app/controllers/TeamController.scala b/app/controllers/TeamController.scala
index 2fc83fcf7e7..bed37536e60 100755
--- a/app/controllers/TeamController.scala
+++ b/app/controllers/TeamController.scala
@@ -8,12 +8,15 @@ import oxalis.security.WkEnv
 import play.api.i18n.Messages
 import play.api.libs.json._
 import utils.ObjectId
-
 import javax.inject.Inject
+import models.binary.DataSetAllowedTeamsDAO
+import play.api.mvc.{Action, AnyContent}
+
 import scala.concurrent.ExecutionContext
 
 class TeamController @Inject()(teamDAO: TeamDAO,
                                userTeamRolesDAO: UserTeamRolesDAO,
+                               datasetAllowedTeamsDAO: DataSetAllowedTeamsDAO,
                                teamService: TeamService,
                                sil: Silhouette[WkEnv])(implicit ec: ExecutionContext)
     extends Controller {
@@ -21,37 +24,35 @@ class TeamController @Inject()(teamDAO: TeamDAO,
   private def teamNameReads: Reads[String] =
     (__ \ "name").read[String]
 
-  def list(isEditable: Option[Boolean]) = sil.SecuredAction.async { implicit request =>
+  def list(isEditable: Option[Boolean]): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
     val onlyEditableTeams = isEditable.getOrElse(false)
     for {
       allTeams <- if (onlyEditableTeams) teamDAO.findAllEditable else teamDAO.findAll
       js <- Fox.serialCombined(allTeams)(t => teamService.publicWrites(t))
-    } yield {
-      Ok(Json.toJson(js))
-    }
+    } yield Ok(Json.toJson(js))
   }
 
-  def delete(id: String) = sil.SecuredAction.async { implicit request =>
+  def delete(id: String): Action[AnyContent] = sil.SecuredAction.async { implicit request =>
     for {
       teamIdValidated <- ObjectId.parse(id)
-      _ <- teamDAO.findOne(teamIdValidated) ?~> "team.notFound" ~> NOT_FOUND
+      _ <- bool2Fox(request.identity.isAdmin) ?~> "user.noAdmin" ~> FORBIDDEN
+      team <- teamDAO.findOne(teamIdValidated) ?~> "team.notFound" ~> NOT_FOUND
+      _ <- bool2Fox(!team.isOrganizationTeam) ?~> "team.delete.organizationTeam" ~> FORBIDDEN
       _ <- teamDAO.deleteOne(teamIdValidated)
+      _ <- teamService.assertNoReferences(teamIdValidated) ?~> "team.delete.inUse" ~> FORBIDDEN
       _ <- userTeamRolesDAO.removeTeamFromAllUsers(teamIdValidated)
-    } yield {
-      JsonOk(Messages("team.deleted"))
-    }
+      _ <- datasetAllowedTeamsDAO.removeTeamFromAllDatasets(teamIdValidated)
+    } yield JsonOk(Messages("team.deleted"))
   }
 
-  def create = sil.SecuredAction.async(parse.json) { implicit request =>
+  def create: Action[JsValue] = sil.SecuredAction.async(parse.json) { implicit request =>
     withJsonBodyUsing(teamNameReads) { teamName =>
       for {
         _ <- bool2Fox(request.identity.isAdmin) ?~> "user.noAdmin" ~> FORBIDDEN
         team = Team(ObjectId.generate, request.identity._organization, teamName)
         _ <- teamDAO.insertOne(team)
         js <- teamService.publicWrites(team)
-      } yield {
-        JsonOk(js, Messages("team.created"))
-      }
+      } yield JsonOk(js, Messages("team.created"))
     }
   }
 }
diff --git a/app/models/annotation/Annotation.scala b/app/models/annotation/Annotation.scala
index 76db1cb6a83..55a321f1eb0 100755
--- a/app/models/annotation/Annotation.scala
+++ b/app/models/annotation/Annotation.scala
@@ -151,6 +151,12 @@ class AnnotationDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContex
     } yield parsed
   }
 
+  def countForTeam(teamId: ObjectId): Fox[Int] =
+    for {
+      countList <- run(sql"select count(_id) from #$existingCollectionName where _team = $teamId".as[Int])
+      count <- countList.headOption
+    } yield count
+
   // Does not use access query (because they dont support prefixes). Use only after separate access check!
   def findAllFinishedForProject(projectId: ObjectId): Fox[List[Annotation]] =
     for {
diff --git a/app/models/binary/DataSet.scala b/app/models/binary/DataSet.scala
index 5e0200311fe..15d0b6afc21 100755
--- a/app/models/binary/DataSet.scala
+++ b/app/models/binary/DataSet.scala
@@ -501,6 +501,12 @@ class DataSetAllowedTeamsDAO @Inject()(sqlClient: SQLClient)(implicit ec: Execut
                retryIfErrorContains = List(transactionSerializationError))
     } yield ()
   }
+
+  def removeTeamFromAllDatasets(teamId: ObjectId): Fox[Unit] =
+    for {
+      _ <- run(sqlu"delete from webknossos.dataSet_allowedTeams where _team = $teamId")
+    } yield ()
+
 }
 
 class DataSetLastUsedTimesDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
diff --git a/app/models/project/Project.scala b/app/models/project/Project.scala
index 2f283f600fe..a1921f6fd8b 100755
--- a/app/models/project/Project.scala
+++ b/app/models/project/Project.scala
@@ -31,7 +31,7 @@ case class Project(
     isDeleted: Boolean = false
 ) extends FoxImplicits {
 
-  def isDeletableBy(user: User) = user._id == _owner || user.isAdmin
+  def isDeletableBy(user: User): Boolean = user._id == _owner || user.isAdmin
 
 }
 
@@ -86,8 +86,7 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
     for {
       accessQuery <- readAccessQuery
       rList <- run(
-        sql"select #${columns} from #${existingCollectionName} where _id = ${id.id} and #${accessQuery}"
-          .as[ProjectsRow])
+        sql"select #$columns from #$existingCollectionName where _id = ${id.id} and #$accessQuery".as[ProjectsRow])
       r <- rList.headOption.toFox ?~> ("Could not find object " + id + " in " + collectionName)
       parsed <- parse(r) ?~> ("SQLDAO Error: Could not parse database row for object " + id + " in " + collectionName)
     } yield parsed
@@ -95,8 +94,7 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
   override def findAll(implicit ctx: DBAccessContext): Fox[List[Project]] =
     for {
       accessQuery <- readAccessQuery
-      r <- run(
-        sql"select #${columns} from #${existingCollectionName} where #${accessQuery} order by created".as[ProjectsRow])
+      r <- run(sql"select #$columns from #$existingCollectionName where #$accessQuery order by created".as[ProjectsRow])
       parsed <- Fox.combined(r.toList.map(parse))
     } yield parsed
 
@@ -108,7 +106,7 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
               from webknossos.projects_ p
               join webknossos.tasks_ t on t._project = p._id
               join webknossos.taskTypes_ tt on t._taskType = tt._id
-              where tt._id = ${taskTypeId}
+              where tt._id = $taskTypeId
            """.as[ProjectsRow]
       )
       parsed <- Fox.combined(r.toList.map(parse))
@@ -118,22 +116,21 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
     for {
       accessQuery <- readAccessQuery
       rList <- run(
-        sql"select #${columns} from #${existingCollectionName} where name = '#${sanitize(name)}' and #${accessQuery}"
+        sql"select #$columns from #$existingCollectionName where name = '#${sanitize(name)}' and #$accessQuery"
           .as[ProjectsRow])
       r <- rList.headOption.toFox
       parsed <- parse(r)
     } yield parsed
 
-  def findUsersWithActiveTasks(name: String)(implicit ctx: DBAccessContext): Fox[List[(String, String, String, Int)]] =
+  def findUsersWithActiveTasks(name: String): Fox[List[(String, String, String, Int)]] =
     for {
-      accessQuery <- readAccessQuery
       rSeq <- run(sql"""select u.email, u.firstName, u.lastName, count(a._id)
                          from
                          webknossos.annotations_ a
                          join webknossos.tasks_ t on a._task = t._id
                          join webknossos.projects_ p on t._project = p._id
                          join webknossos.users_ u on a._user = u._id
-                         where p.name = ${name}
+                         where p.name = $name
                          and a.state = '#${AnnotationState.Active.toString}'
                          and a.typ = '#${AnnotationType.Task}'
                          group by u.email, u.firstName, u.lastName
@@ -166,9 +163,15 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
                           where _id = ${p._id.id}""")
     } yield ()
 
-  def updatePaused(id: ObjectId, isPaused: Boolean)(implicit ctx: DBAccessContext) =
+  def updatePaused(id: ObjectId, isPaused: Boolean)(implicit ctx: DBAccessContext): Fox[Unit] =
     updateBooleanCol(id, _.paused, isPaused)
 
+  def countForTeam(teamId: ObjectId): Fox[Int] =
+    for {
+      countList <- run(sql"select count(_id) from #$existingCollectionName where _team = $teamId".as[Int])
+      count <- countList.headOption
+    } yield count
+
 }
 
 class ProjectService @Inject()(projectDAO: ProjectDAO, teamDAO: TeamDAO, userService: UserService, taskDAO: TaskDAO)(
diff --git a/app/models/task/TaskType.scala b/app/models/task/TaskType.scala
index 9e6226a7a55..ac48d24d869 100755
--- a/app/models/task/TaskType.scala
+++ b/app/models/task/TaskType.scala
@@ -158,4 +158,9 @@ class TaskTypeDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
                           where _id = ${t._id.id}""")
     } yield ()
 
+  def countForTeam(teamId: ObjectId): Fox[Int] =
+    for {
+      countList <- run(sql"select count(_id) from #$existingCollectionName where _team = $teamId".as[Int])
+      count <- countList.headOption
+    } yield count
 }
diff --git a/app/models/team/Team.scala b/app/models/team/Team.scala
index 83e92f4a25f..10e7e748d86 100755
--- a/app/models/team/Team.scala
+++ b/app/models/team/Team.scala
@@ -4,7 +4,11 @@ import com.scalableminds.util.accesscontext.{DBAccessContext, GlobalAccessContex
 import com.scalableminds.util.tools.{Fox, FoxImplicits}
 import com.scalableminds.webknossos.schema.Tables._
 import javax.inject.Inject
+import models.annotation.AnnotationDAO
+import models.project.ProjectDAO
+import models.task.TaskTypeDAO
 import models.user.User
+import play.api.i18n.{Messages, MessagesProvider}
 import play.api.libs.json._
 import slick.jdbc.PostgresProfile.api._
 import slick.lifted.Rep
@@ -26,7 +30,11 @@ case class Team(
 
 }
 
-class TeamService @Inject()(organizationDAO: OrganizationDAO)(implicit ec: ExecutionContext) {
+class TeamService @Inject()(organizationDAO: OrganizationDAO,
+                            annotationDAO: AnnotationDAO,
+                            projectDAO: ProjectDAO,
+                            taskTypeDAO: TaskTypeDAO)(implicit ec: ExecutionContext)
+    extends FoxImplicits {
 
   def publicWrites(team: Team, organizationOpt: Option[Organization] = None): Fox[JsObject] =
     for {
@@ -38,6 +46,17 @@ class TeamService @Inject()(organizationDAO: OrganizationDAO)(implicit ec: Execu
         "organization" -> organization.name
       )
     }
+
+  def assertNoReferences(teamId: ObjectId)(implicit mp: MessagesProvider): Fox[Unit] =
+    for {
+      projectCount <- projectDAO.countForTeam(teamId)
+      _ <- bool2Fox(projectCount == 0) ?~> Messages("team.inUse.projects", projectCount)
+      taskTypeCount <- taskTypeDAO.countForTeam(teamId)
+      _ <- bool2Fox(projectCount == 0) ?~> Messages("team.inUse.taskTypes", taskTypeCount)
+      annotationCount <- annotationDAO.countForTeam(teamId)
+      _ <- bool2Fox(projectCount == 0) ?~> Messages("team.inUse.annotations", annotationCount)
+    } yield ()
+
 }
 
 class TeamDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
@@ -75,15 +94,6 @@ class TeamDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
       parsed <- parse(r) ?~> ("SQLDAO Error: Could not parse database row for object " + id + " in " + collectionName)
     } yield parsed
 
-  def findOneByName(name: String)(implicit ctx: DBAccessContext): Fox[Team] =
-    for {
-      accessQuery <- readAccessQuery
-      rList <- run(
-        sql"select #$columns from #$existingCollectionName where name = $name and #$accessQuery".as[TeamsRow])
-      r <- rList.headOption.toFox
-      parsed <- parse(r)
-    } yield parsed
-
   override def findAll(implicit ctx: DBAccessContext): Fox[List[Team]] =
     for {
       accessQuery <- readAccessQuery
diff --git a/app/models/user/User.scala b/app/models/user/User.scala
index 9e28decd5c0..e5db81394df 100755
--- a/app/models/user/User.scala
+++ b/app/models/user/User.scala
@@ -275,7 +275,7 @@ class UserTeamRolesDAO @Inject()(userDAO: UserDAO, sqlClient: SQLClient)(implici
 
   def removeTeamFromAllUsers(teamId: ObjectId): Fox[Unit] =
     for {
-      r <- run(sqlu"delete from webknossos.user_team_roles where _team = ${teamId}")
+      _ <- run(sqlu"delete from webknossos.user_team_roles where _team = $teamId")
     } yield ()
 
   def findMemberDifference(potentialSubteam: ObjectId, superteams: List[ObjectId]): Fox[List[User]] =
diff --git a/conf/messages b/conf/messages
index 49cf1771e49..f78bf028dbd 100644
--- a/conf/messages
+++ b/conf/messages
@@ -40,6 +40,12 @@ team.notAllowed=You are not part of this team. Project can’t be created.
 team.admin.notPossibleBy=User {1} can’t be assigned administrative rights in team {0} because he doesn't belong to the teams parent team.
 team.admin.notAllowed=You are not authorized to administrate this team.
 team.notFound=Team could not be found.
+team.nameInUse=Team name is or was already in use. Please chose a different name.
+team.delete.organizationTeam=Team cannot be deleted as it is the oganization base team
+team.delete.inUse=Team cannot be deleted as it is referenced in an at least one annotation, project or task type
+team.inUse.annotations=Team is referenced by {0} annotations
+team.inUse.projects=Team is referenced by {0} projects
+team.inUse.tasktypes=Team is referenced by {0} task types
 
 organization.create.forbidden=You are not allowed to create a new organization
 organization.create.failed=Failed to create a new organization

From 3297bbf0c9f76163e1e36f1854e7bd2edc516012 Mon Sep 17 00:00:00 2001
From: Florian M <florian@scm.io>
Date: Fri, 29 Jan 2021 11:48:21 +0100
Subject: [PATCH 2/4] fix pr number in changelog

---
 CHANGELOG.unreleased.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/CHANGELOG.unreleased.md b/CHANGELOG.unreleased.md
index bffa856f44a..443e1429b79 100644
--- a/CHANGELOG.unreleased.md
+++ b/CHANGELOG.unreleased.md
@@ -20,7 +20,7 @@ For upgrade instructions, please check the [migration guide](MIGRATIONS.released
 - Users are no longer allowed to deactivate their own accounts.  [#5070](https://github.com/scalableminds/webknossos/pull/5070)
 
 ### Fixed
-- Fixed a bug where the user could delete teams that were still referenced in annotations, projects or task types, thus creating invalid state. [#5107](https://github.com/scalableminds/webknossos/pull/5107/files)
+- Fixed a bug where the user could delete teams that were still referenced in annotations, projects or task types, thus creating invalid state. [#5108](https://github.com/scalableminds/webknossos/pull/5108/files)
 
 ### Removed
 - The isosurface setting was removed. Instead, isosurfaces can be generated via the "Meshes" tab. Also note that the Shift+Click binding for generating an isosurface was removed (for now). Please refer to the "Meshes" tab, too. [#4917](https://github.com/scalableminds/webknossos/pull/4917)

From f8f9c838b0f507177ed7d1e8b7a4db6f561c898b Mon Sep 17 00:00:00 2001
From: Florian M <florian@scm.io>
Date: Fri, 29 Jan 2021 12:29:29 +0100
Subject: [PATCH 3/4] add correct error message to snapshot test

---
 .../test/backend-snapshot-tests/teamstructure.e2e.js            | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/frontend/javascripts/test/backend-snapshot-tests/teamstructure.e2e.js b/frontend/javascripts/test/backend-snapshot-tests/teamstructure.e2e.js
index 164287ad275..9ff9f791711 100644
--- a/frontend/javascripts/test/backend-snapshot-tests/teamstructure.e2e.js
+++ b/frontend/javascripts/test/backend-snapshot-tests/teamstructure.e2e.js
@@ -72,7 +72,7 @@ test("teams_delete_user_D", async t => {
   t.plan(1);
 
   await api.deleteTeam("69882b370d889b84020efd4f").catch(err => {
-    t.is(err.messages[0].error, "Access denied.");
+    t.is(err.messages[0].error, "Access denied. Only admin users can execute this operation.");
   });
 });
 

From 72036a74bca33393246b08b20aca2eb334d70fc0 Mon Sep 17 00:00:00 2001
From: Florian M <florian@scm.io>
Date: Fri, 29 Jan 2021 13:57:16 +0100
Subject: [PATCH 4/4] PR feedback

---
 app/models/project/Project.scala | 6 +++---
 conf/messages                    | 2 +-
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/project/Project.scala b/app/models/project/Project.scala
index a1921f6fd8b..54097818a23 100755
--- a/app/models/project/Project.scala
+++ b/app/models/project/Project.scala
@@ -86,7 +86,7 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
     for {
       accessQuery <- readAccessQuery
       rList <- run(
-        sql"select #$columns from #$existingCollectionName where _id = ${id.id} and #$accessQuery".as[ProjectsRow])
+        sql"select #$columns from #$existingCollectionName where _id = $id and #$accessQuery".as[ProjectsRow])
       r <- rList.headOption.toFox ?~> ("Could not find object " + id + " in " + collectionName)
       parsed <- parse(r) ?~> ("SQLDAO Error: Could not parse database row for object " + id + " in " + collectionName)
     } yield parsed
@@ -143,7 +143,7 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
     for {
       _ <- run(
         sqlu"""insert into webknossos.projects(_id, _team, _owner, name, priority, paused, expectedTime, isblacklistedfromreport, created, isDeleted)
-                         values(${p._id.id}, ${p._team.id}, ${p._owner.id}, ${p.name}, ${p.priority}, ${p.paused}, ${p.expectedTime}, ${p.isBlacklistedFromReport}, ${new java.sql.Timestamp(
+                         values(${p._id}, ${p._team}, ${p._owner}, ${p.name}, ${p.priority}, ${p.paused}, ${p.expectedTime}, ${p.isBlacklistedFromReport}, ${new java.sql.Timestamp(
           p.created)}, ${p.isDeleted})""")
     } yield ()
 
@@ -160,7 +160,7 @@ class ProjectDAO @Inject()(sqlClient: SQLClient)(implicit ec: ExecutionContext)
                             expectedTime = ${p.expectedTime},
                             isblacklistedfromreport = ${p.isBlacklistedFromReport},
                             isDeleted = ${p.isDeleted}
-                          where _id = ${p._id.id}""")
+                          where _id = ${p._id}""")
     } yield ()
 
   def updatePaused(id: ObjectId, isPaused: Boolean)(implicit ctx: DBAccessContext): Fox[Unit] =
diff --git a/conf/messages b/conf/messages
index f78bf028dbd..fbaf18295d3 100644
--- a/conf/messages
+++ b/conf/messages
@@ -41,7 +41,7 @@ team.admin.notPossibleBy=User {1} can’t be assigned administrative rights in t
 team.admin.notAllowed=You are not authorized to administrate this team.
 team.notFound=Team could not be found.
 team.nameInUse=Team name is or was already in use. Please chose a different name.
-team.delete.organizationTeam=Team cannot be deleted as it is the oganization base team
+team.delete.organizationTeam=Team cannot be deleted as it is the organization base team
 team.delete.inUse=Team cannot be deleted as it is referenced in an at least one annotation, project or task type
 team.inUse.annotations=Team is referenced by {0} annotations
 team.inUse.projects=Team is referenced by {0} projects