Prevent deleting teams that are still referenced (#5108)

* Prevent deleting teams that are still referenced * fix pr number in changelog * add correct error message to snapshot test * PR feedback
scalableminds · Feb 1, 2021 · 6ec6fd1 · 6ec6fd1
1 parent 406fcc5
commit 6ec6fd1
Show file tree

Hide file tree

Showing 10 changed files with 77 additions and 40 deletions.
diff --git 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. [#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)
diff --git a/app/controllers/TeamController.scala b/app/controllers/TeamController.scala
@@ -8,50 +8,51 @@ 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 {
 
   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
@@ -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
@@ -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
@@ -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,17 +86,15 @@ 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
 
   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
@@ -146,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 ()
 
@@ -163,12 +160,18 @@ 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) =
+  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
@@ -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
@@ -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
@@ -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
@@ -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 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
+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

diff --git 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.");
   });
 });