diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala index aa11fa3e..8491bb0d 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/SQLUserDAO.scala @@ -31,7 +31,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { } def findById(userId: ObjectId) = findOneWhere(_.id === userId) - + def findByEmail(email: String) = findOneWhere(_.emailLowerCase === email.toLowerCase) def findByLowerCasedLogin(login: String) = db.withTransaction { implicit session => @@ -124,7 +124,7 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { } yield (u, a, s, l) userQuery.firstOption.map(queryUserAliases).map(untuple) } - + private def queryUserAliases(tuple: (UserTuple, SQLAuth, SQLSettings, SQLLastNotif))(implicit session: Session) = { val userId = tuple._1._1 val aliases = userAliases.where(_.userId === userId).list() @@ -135,4 +135,11 @@ class SQLUserDAO(val database: SQLDatabase) extends UserDAO with SQLUserSchema { val aliases = userAliases.where(_.userId === tuple._1).list() (tuple._1, tuple._2, tuple._3, tuple._4, UserAliases(aliases.map(_.toUserAlias))) } -} \ No newline at end of file + + def delete(userId: ObjectId) { + db.withTransaction { implicit session => + users.filter(_.id === userId).delete + } + } + +} diff --git a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala index 2f228520..6b2b30b6 100644 --- a/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala +++ b/codebrag-dao/src/main/scala/com/softwaremill/codebrag/dao/user/UserDAO.scala @@ -40,4 +40,7 @@ trait UserDAO { def countAll(): Long def countAllActive(): Long + + def delete(userId: ObjectId) + } diff --git a/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala b/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala index 016f17d7..6f95914b 100644 --- a/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala +++ b/codebrag-dao/src/test/scala/com/softwaremill/codebrag/dao/user/UserDAOSpec.scala @@ -370,7 +370,38 @@ class SQLUserDAOSpec extends FlatSpecWithSQL with ClearSQLDataAfterTest with Bef foundUser.get.notifications.commits.get.getMillis should equal(commitDate.getMillis) foundUser.get.notifications.followups.get.getMillis should equal(followupDate.getMillis) } + it should "deleta a user " taggedAs RequiresDb in { + // given + val user = UserAssembler.randomUser.withBasicAuth("user", "pass").withAdmin(set = false).withActive(set = false).get + userDAO.add(user) + val newAuth = Authentication.basic(user.authentication.username, "newpass") + // when + val modifiedUser = user.copy(authentication = newAuth, admin = true, active = true) + userDAO.delete(modifiedUser.id) + val deletedUser = userDAO.findById(user.id) + + // then + assert(deletedUser === None , "Deletion was attempted but found " + deletedUser) + } + it should "deleta a user and should not show in the list " taggedAs RequiresDb in { + // given + val user = UserAssembler.randomUser.withBasicAuth("user", "pass").withAdmin(set = false).withActive(set = false).get + userDAO.add(user) + val newAuth = Authentication.basic(user.authentication.username, "newpass") + + // when + val tobeDeletedUser = user.copy(authentication = newAuth, admin = true, active = true) + val userCountBeforeDelete = userDAO.findAll().length + userDAO.delete(tobeDeletedUser.id) + val deletedUser = userDAO.findById(user.id) + val userCountAfterDelete = userDAO.findAll().length + // then + assert(deletedUser === None , "Deletion was attempted but found " + deletedUser) + userCountAfterDelete should be(userCountBeforeDelete -1) + + } + "rememberNotifications" should "store dates properly" taggedAs RequiresDb in { // given val user = UserAssembler.randomUser.get @@ -479,4 +510,5 @@ class SQLUserDAOSpec extends FlatSpecWithSQL with ClearSQLDataAfterTest with Bef partial should have size (2) partial.map(_.name).toSet should be (users.map(_.name).toSet) } + } \ No newline at end of file diff --git a/codebrag-rest/src/main/scala/ScalatraBootstrap.scala b/codebrag-rest/src/main/scala/ScalatraBootstrap.scala index f7c91ef6..d45a3419 100644 --- a/codebrag-rest/src/main/scala/ScalatraBootstrap.scala +++ b/codebrag-rest/src/main/scala/ScalatraBootstrap.scala @@ -69,7 +69,7 @@ class ScalatraBootstrap extends LifeCycle with Logging { RepositoryUpdateScheduler.scheduleUpdates(actorSystem, repositories, commitImportService) context.mount(new RegistrationServlet(registerService, registerNewUserUseCase, listReposAfterRegistration, listRepoBranchesAfterRegistration, watchBranchAfterRegistration, unwatchBranchAfterRegistration), Prefix + RegistrationServlet.MappingPath) context.mount(new SessionServlet(authenticator, loginUserUseCase, userFinder), Prefix + SessionServlet.MappingPath) - context.mount(new UsersServlet(authenticator, userFinder, modifyUserDetailsUseCase, config), Prefix + UsersServlet.MappingPath) + context.mount(new UsersServlet(authenticator, userFinder, modifyUserDetailsUseCase, deleteUserUseCase, config), Prefix + UsersServlet.MappingPath) context.mount(new UserAliasesEndpoint(authenticator, addUserAliasUseCase, deleteUserAliasUseCase), Prefix + UserAliasesEndpoint.MappingPath) context.mount(new UsersSettingsServlet(authenticator, userDao, changeUserSettingsUseCase), Prefix + "users/settings") context.mount(new CommitsServlet(authenticator, toReviewCommitsFinder, allCommitsFinder, reactionFinder, addCommentUseCase, diff --git a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala index e9cf4221..5c2caeec 100644 --- a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala +++ b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/Beans.scala @@ -79,6 +79,7 @@ trait Beans extends ActorSystemSupport with CommitsModule with Daos { lazy val generateInvitationCodeUseCase = new GenerateInvitationCodeUseCase(invitationsService, userDao) lazy val sendInvitationEmailUseCase = new SendInvitationEmailUseCase(invitationsService, userDao) lazy val modifyUserDetailsUseCase = new ModifyUserDetailsUseCase(userDao) + lazy val deleteUserUseCase = new DeleteUserUseCase(userDao) lazy val updateUserBrowsingContextUseCase = new UpdateUserBrowsingContextUseCase(userRepoDetailsDao) lazy val addUserAliasUseCase = new AddUserAliasUseCase(userAliasDao, userDao) lazy val deleteUserAliasUseCase = new DeleteUserAliasUseCase(userAliasDao) diff --git a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala index 2ad75fa5..10fe15dc 100644 --- a/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala +++ b/codebrag-rest/src/main/scala/com/softwaremill/codebrag/rest/UsersServlet.scala @@ -6,12 +6,13 @@ import org.bson.types.ObjectId import org.scalatra import com.softwaremill.codebrag.finders.user.UserFinder import com.softwaremill.codebrag.finders.user.ManagedUsersListView -import com.softwaremill.codebrag.usecases.user.{RegisterNewUserUseCase, ModifyUserDetailsUseCase, ModifyUserDetailsForm} +import com.softwaremill.codebrag.usecases.user.{RegisterNewUserUseCase, ModifyUserDetailsUseCase, ModifyUserDetailsForm,DeleteUserUseCase,DeleteUserForm} class UsersServlet( val authenticator: Authenticator, userFinder: UserFinder, modifyUserUseCase: ModifyUserDetailsUseCase, + deleteUserUseCase: DeleteUserUseCase, config: CodebragConfig) extends JsonServletWithAuthentication { get("/") { @@ -34,8 +35,16 @@ class UsersServlet( case _ => scalatra.Ok() } } - +delete("/:userId") { + haltIfNotAuthenticated() + val targetUserId = new ObjectId(params("userId")) + deleteUserUseCase.execute(user.id, DeleteUserForm(targetUserId)) match { + case Left(errors) => scalatra.BadRequest(errors) + case _ => scalatra.Ok() + } + } } + object UsersServlet { val MappingPath = "users" diff --git a/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala b/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala index cebd1c7a..cf7ba27f 100644 --- a/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala +++ b/codebrag-rest/src/test/scala/com/softwaremill/codebrag/rest/UsersServletSpec.scala @@ -13,11 +13,12 @@ import com.softwaremill.codebrag.finders.user.ManagedUserView import com.softwaremill.codebrag.finders.user.ManagedUsersListView import com.softwaremill.codebrag.domain.builder.UserAssembler import com.softwaremill.codebrag.domain.User -import com.softwaremill.codebrag.usecases.user.ModifyUserDetailsUseCase +import com.softwaremill.codebrag.usecases.user.{ ModifyUserDetailsUseCase, DeleteUserUseCase } class UsersServletSpec extends AuthenticatableServletSpec { val modifyUserUseCase = mock[ModifyUserDetailsUseCase] + val deleteUserUseCase = mock[DeleteUserUseCase] var userFinder: UserFinder = _ var config: CodebragConfig = _ @@ -50,7 +51,6 @@ class UsersServletSpec extends AuthenticatableServletSpec { } } - def configWithDemo(mode: Boolean) = { val p = new Properties() p.setProperty("codebrag.demo", mode.toString) @@ -60,7 +60,7 @@ class UsersServletSpec extends AuthenticatableServletSpec { } class TestableUsersServlet(fakeAuthenticator: Authenticator, fakeScentry: Scentry[User]) - extends UsersServlet(fakeAuthenticator, userFinder, modifyUserUseCase, config) { + extends UsersServlet(fakeAuthenticator, userFinder, modifyUserUseCase, deleteUserUseCase, config) { override def scentry(implicit request: javax.servlet.http.HttpServletRequest) = fakeScentry } diff --git a/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala new file mode 100644 index 00000000..fa0a8bfb --- /dev/null +++ b/codebrag-service/src/main/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCase.scala @@ -0,0 +1,37 @@ +package com.softwaremill.codebrag.usecases.user + +import com.softwaremill.codebrag.dao.user.UserDAO +import com.softwaremill.codebrag.usecases.assertions.UserAssertions +import org.bson.types.ObjectId +import com.softwaremill.codebrag.domain.{Authentication, User} +import com.typesafe.scalalogging.slf4j.Logging +import com.softwaremill.scalaval.Validation._ + +case class DeleteUserForm(userId: ObjectId) { + +} + +class DeleteUserUseCase(protected val userDao: UserDAO) extends Logging { + + import UserAssertions._ + + def execute(executorId: ObjectId, form: DeleteUserForm): Either[Errors, Unit] = { + assertUserWithId(executorId, mustBeActive, mustBeAdmin)(userDao) + val targetUser = loadUser(form.userId) + validateUserDetails(executorId, targetUser, form).whenOk[Unit] { + logger.debug(s"Validation passed, attempting to delete user $targetUser") + userDao.delete(form.userId) + } + } + + private def loadUser(userId: ObjectId) = userDao.findById(userId).getOrElse(throw new IllegalStateException(s"User $userId not found")) + + private def validateUserDetails(executorId: ObjectId, user: User, form: DeleteUserForm) = { + val changeOwnFlagsCheck = rule("userId") { + val isDeleteFlags = user.admin || user.active + (!isDeleteFlags || (isDeleteFlags && executorId != user.id), "Cannot delete own user") + } + validate(changeOwnFlagsCheck) + } + +} diff --git a/codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala b/codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala new file mode 100644 index 00000000..6e44d268 --- /dev/null +++ b/codebrag-service/src/test/scala/com/softwaremill/codebrag/usecases/user/DeleteUserUseCaseSpec.scala @@ -0,0 +1,88 @@ +package com.softwaremill.codebrag.usecases.user + +import org.scalatest.{BeforeAndAfter, FlatSpec} +import org.scalatest.matchers.ShouldMatchers +import org.scalatest.mock.MockitoSugar +import com.softwaremill.codebrag.dao.user.UserDAO +import org.mockito.Matchers._ +import org.mockito.Mockito._ +import com.softwaremill.codebrag.domain.builder.UserAssembler +import com.softwaremill.codebrag.domain.{Authentication, User} +import com.softwaremill.codebrag.usecases.assertions.{ActiveUserStatusRequiredException, AdminRoleRequiredException} +import org.bson.types.ObjectId + +class DeleteUserUseCaseSpec extends FlatSpec with ShouldMatchers with BeforeAndAfter with MockitoSugar { + + val userDao = mock[UserDAO] + val useCase = new DeleteUserUseCase(userDao) + + val InactiveUser = UserAssembler.randomUser.withActive(set = false).get + val ActiveUser = UserAssembler.randomUser.withActive().get + + val ValidExecutor = UserAssembler.randomUser.withAdmin().withActive().get + val NonAdminExecutor = UserAssembler.randomUser.withActive().withAdmin(set = false).get + val InactiveExecutor = UserAssembler.randomUser.withActive(set = false).withAdmin().get + + after { + reset(userDao) + } + + it should "not delete user when executing user is neither admin nor active" in { + // given + setupReturningUserFromDB(NonAdminExecutor, InactiveExecutor) + val form = DeleteUserForm(ActiveUser.id) + + // when + intercept[AdminRoleRequiredException] { + useCase.execute(NonAdminExecutor.id, form) + } + intercept[ActiveUserStatusRequiredException] { + useCase.execute(InactiveExecutor.id, form) + } + + // then + verify(userDao, never()).delete(any[ObjectId]) + } + + it should "not allow to delete yourself" in { + // given + setupReturningUserFromDB(ValidExecutor) + + // when + val ownChangeForm = DeleteUserForm(ValidExecutor.id) + val Left(result) = useCase.execute(ValidExecutor.id, ownChangeForm) + + // then + result should be(Map("userId" -> List("Cannot delete own user"))) + verify(userDao, never()).delete(any[ObjectId]) + } + + + it should "delete user when validation passes" in { + // given + stubCurrentlyActiveUsersCountTo(0) + setupReturningUserFromDB(ValidExecutor, ActiveUser) + + // when + val newAuth = Authentication.basic(ActiveUser.authentication.username, "secret") + val form = new DeleteUserForm(ActiveUser.id) + val result = useCase.execute(ValidExecutor.id, form) + + // then + result should be('right) + val expectedUser = form + verify(userDao).delete(expectedUser.userId) + } + + private def stubCurrentlyActiveUsersCountTo(activeUsersCount: Int) { + when(userDao.countAllActive()).thenReturn(activeUsersCount) + } + + private def setupReturningUserFromDB(users: User*) { + users.foreach { user => + when(userDao.findById(user.id)).thenReturn(Some(user)) + } + } + +} + \ No newline at end of file diff --git a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js index 747172dc..945a40e4 100644 --- a/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js +++ b/codebrag-ui/app/scripts/users/manageUsersPopupCtrl.js @@ -26,6 +26,12 @@ angular.module('codebrag.userMgmt') }); }; + $scope.deleteUser = function(user) { + $scope.flash.clear(); + var userData = { userId: user.userId }; + userMgmtService.deleteUser(userData).then(deleteSuccess(user), deleteFailed('active', user.userId)) + }; + $scope.askForNewPassword = function(user) { $scope.flash.clear(); var modal = popupsService.openSetUserPasswordPopup(user); @@ -33,10 +39,24 @@ angular.module('codebrag.userMgmt') $scope.flash.add('info', 'User password changed'); }); }; + function deleteSuccess(user) { + $scope.flash.add('error', 'User ' + user.email + ' is deleted'); + userMgmtService.loadUsers().then(function(users) { + $scope.users = users; + }); + } function modifySuccess() { $scope.flash.add('info', 'User details changed'); } + function deleteFailed(flag, userId) { + return function(errorsMap) { + $scope.flash.add('error', ' Could not delete user '); + flattenErrorsMap(errorsMap).forEach(function(error) { + $scope.flash.add('error', error); + }); + } + } function modifyFailed(flag, user) { return function(errorsMap) { diff --git a/codebrag-ui/app/scripts/users/userMgmtService.js b/codebrag-ui/app/scripts/users/userMgmtService.js index 21465f08..41b3d211 100644 --- a/codebrag-ui/app/scripts/users/userMgmtService.js +++ b/codebrag-ui/app/scripts/users/userMgmtService.js @@ -19,6 +19,11 @@ angular.module('codebrag.userMgmt') return $http.put(modifyUserUrl, userData).then(null, modifyUserFailed); }; + this.deleteUser = function(userData) { + var deleteUserUrl = [usersApiUrl, '/', userData.userId].join(''); + return $http.delete(deleteUserUrl).then(null, modifyUserFailed); + }; + function modifyUserFailed(response) { return $q.reject(response.data); } @@ -34,4 +39,4 @@ angular.module('codebrag.userMgmt') $modal.open(config) } - }); \ No newline at end of file + }); diff --git a/codebrag-ui/app/views/popups/manageUsers.html b/codebrag-ui/app/views/popups/manageUsers.html index af2df3aa..c813660c 100644 --- a/codebrag-ui/app/views/popups/manageUsers.html +++ b/codebrag-ui/app/views/popups/manageUsers.html @@ -1,48 +1,59 @@ \ No newline at end of file +
+ +
+ + + +