diff --git a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala index c3aec88..eaafacc 100644 --- a/src/main/scala/gitbucket/core/controller/PullRequestsController.scala +++ b/src/main/scala/gitbucket/core/controller/PullRequestsController.scala @@ -177,19 +177,22 @@ } val hasMergePermission = hasWritePermission(owner, name, context.loginAccount) val branchProtection = getProtectedBranchInfo(owner, name, pullreq.branch) - val state = PullRequestService.MergeStatus( + val mergeStatus = PullRequestService.MergeStatus( hasConflict = hasConflict, commitStatues = getCommitStatues(owner, name, pullreq.commitIdTo), branchProtection = branchProtection, branchIsOutOfDate = JGitUtil.getShaByRef(owner, name, pullreq.branch) != Some(pullreq.commitIdFrom), - needStatusCheck = branchProtection.needStatusCheck(context.loginAccount.map(_.userName)), + needStatusCheck = context.loginAccount.map{ u => + branchProtection.needStatusCheck(u.userName) + }.getOrElse(true), hasUpdatePermission = hasWritePermission(pullreq.requestUserName, pullreq.requestRepositoryName, context.loginAccount) && - !getProtectedBranchInfo(pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch) - .needStatusCheck(context.loginAccount.map(_.userName)), + context.loginAccount.map{ u => + !getProtectedBranchInfo(pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch).needStatusCheck(u.userName) + }.getOrElse(false), hasMergePermission = hasMergePermission, commitIdTo = pullreq.commitIdTo) html.mergeguide( - state, + mergeStatus, issue, pullreq, repository, diff --git a/src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala b/src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala index 78cf23b..331bf64 100644 --- a/src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala +++ b/src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala @@ -220,7 +220,7 @@ get("/:owner/:repository/new/*")(collaboratorsOnly { repository => val (branch, path) = splitPath(repository, multiParams("splat").head) - val protectedBranch = isProtectedBranchNeedStatusCheck(repository.owner, repository.name, branch, context.loginAccount.get.userName) + val protectedBranch = getProtectedBranchInfo(repository.owner, repository.name, branch).needStatusCheck(context.loginAccount.get.userName) html.editor(branch, repository, if(path.length == 0) Nil else path.split("/").toList, None, JGitUtil.ContentInfo("text", None, Some("UTF-8")), protectedBranch) @@ -228,7 +228,7 @@ get("/:owner/:repository/edit/*")(collaboratorsOnly { repository => val (branch, path) = splitPath(repository, multiParams("splat").head) - val protectedBranch = isProtectedBranchNeedStatusCheck(repository.owner, repository.name, branch, context.loginAccount.get.userName) + val protectedBranch = getProtectedBranchInfo(repository.owner, repository.name, branch).needStatusCheck(context.loginAccount.get.userName) using(Git.open(getRepositoryDir(repository.owner, repository.name))){ git => val revCommit = JGitUtil.getRevCommitFromId(git, git.getRepository.resolve(branch)) diff --git a/src/main/scala/gitbucket/core/model/CommitStatus.scala b/src/main/scala/gitbucket/core/model/CommitStatus.scala index 87b74f1..75ed261 100644 --- a/src/main/scala/gitbucket/core/model/CommitStatus.scala +++ b/src/main/scala/gitbucket/core/model/CommitStatus.scala @@ -19,7 +19,7 @@ val creator = column[String]("CREATOR") val registeredDate = column[java.util.Date]("REGISTERED_DATE") val updatedDate = column[java.util.Date]("UPDATED_DATE") - def * = (commitStatusId, userName, repositoryName, commitId, context, state, targetUrl, description, creator, registeredDate, updatedDate) <> (CommitStatus.tupled, CommitStatus.unapply) + def * = (commitStatusId, userName, repositoryName, commitId, context, state, targetUrl, description, creator, registeredDate, updatedDate) <> ((CommitStatus.apply _).tupled, CommitStatus.unapply) def byPrimaryKey(id: Int) = commitStatusId === id.bind } } @@ -38,7 +38,20 @@ registeredDate: java.util.Date, updatedDate: java.util.Date ) - +object CommitStatus { + def pending(owner: String, repository: String, context: String) = CommitStatus( + commitStatusId = 0, + userName = owner, + repositoryName = repository, + commitId = "", + context = context, + state = CommitState.PENDING, + targetUrl = None, + description = Some("Waiting for status to be reported"), + creator = "", + registeredDate = new java.util.Date(), + updatedDate = new java.util.Date()) +} sealed abstract class CommitState(val name: String) diff --git a/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala b/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala index c9518b1..cfbe9e3 100644 --- a/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala +++ b/src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala @@ -28,15 +28,12 @@ def getProtectedBranchInfo(owner: String, repository: String, branch: String)(implicit session: Session): ProtectedBranchInfo = getProtectedBranchInfoOpt(owner, repository, branch).getOrElse(ProtectedBranchInfo.disabled(owner, repository)) - def isProtectedBranchNeedStatusCheck(owner: String, repository: String, branch: String, user: String)(implicit session: Session): Boolean = - getProtectedBranchInfo(owner, repository, branch).needStatusCheck(user) - def getProtectedBranchList(owner: String, repository: String)(implicit session: Session): List[String] = ProtectedBranches.filter(_.byRepository(owner, repository)).map(_.branch).list def enableBranchProtection(owner: String, repository: String, branch:String, includeAdministrators: Boolean, contexts: Seq[String])(implicit session: Session): Unit = { disableBranchProtection(owner, repository, branch) - ProtectedBranches.insert(new ProtectedBranch(owner, repository, branch, includeAdministrators)) + ProtectedBranches.insert(new ProtectedBranch(owner, repository, branch, includeAdministrators && contexts.nonEmpty)) contexts.map{ context => ProtectedBranchContexts.insert(new ProtectedBranchContext(owner, repository, branch, context)) } @@ -45,10 +42,10 @@ def disableBranchProtection(owner: String, repository: String, branch:String)(implicit session: Session): Unit = ProtectedBranches.filter(_.byPrimaryKey(owner, repository, branch)).delete - def getBranchProtectedReason(owner: String, repository: String, receivePack: ReceivePack, command: ReceiveCommand, pusher: String)(implicit session: Session): Option[String] = { + def getBranchProtectedReason(owner: String, repository: String, isAllowNonFastForwards: Boolean, command: ReceiveCommand, pusher: String)(implicit session: Session): Option[String] = { val branch = command.getRefName.stripPrefix("refs/heads/") if(branch != command.getRefName){ - getProtectedBranchInfo(owner, repository, branch).getStopReason(receivePack, command, pusher) + getProtectedBranchInfo(owner, repository, branch).getStopReason(isAllowNonFastForwards, command, pusher) }else{ None } @@ -79,10 +76,10 @@ * Can't be deleted * Can't have changes merged into them until required status checks pass */ - def getStopReason(receivePack: ReceivePack, command: ReceiveCommand, pusher: String)(implicit session: Session): Option[String] = { + def getStopReason(isAllowNonFastForwards: Boolean, command: ReceiveCommand, pusher: String)(implicit session: Session): Option[String] = { if(enabled){ command.getType() match { - case ReceiveCommand.Type.UPDATE|ReceiveCommand.Type.UPDATE_NONFASTFORWARD if receivePack.isAllowNonFastForwards => + case ReceiveCommand.Type.UPDATE|ReceiveCommand.Type.UPDATE_NONFASTFORWARD if isAllowNonFastForwards => Some("Cannot force-push to a protected branch") case ReceiveCommand.Type.UPDATE|ReceiveCommand.Type.UPDATE_NONFASTFORWARD if needStatusCheck(pusher) => unSuccessedContexts(command.getNewId.name) match { @@ -103,27 +100,13 @@ } else { contexts.toSet -- getCommitStatues(owner, repository, sha1).filter(_.state == CommitState.SUCCESS).map(_.context).toSet } - def needStatusCheck(pusher: Option[String])(implicit session: Session): Boolean = pusher.map(needStatusCheck).getOrElse(false) - def needStatusCheck(pusher: String)(implicit session: Session): Boolean = - if(!enabled || contexts.isEmpty){ - false - }else if(includeAdministrators){ - true - }else{ - !isAdministrator(pusher) - } - def pendingCommitStatus(context: String) = CommitStatus( - commitStatusId = 0, - userName = owner, - repositoryName = repository, - commitId = "", - context = context, - state = CommitState.PENDING, - targetUrl = None, - description = Some("Waiting for status to be reported"), - creator = "", - registeredDate = new java.util.Date(), - updatedDate = new java.util.Date()) + def needStatusCheck(pusher: String)(implicit session: Session): Boolean = pusher match { + case _ if !enabled => false + case _ if contexts.isEmpty => false + case _ if includeAdministrators => true + case p if isAdministrator(p) => false + case _ => true + } } object ProtectedBranchInfo{ def disabled(owner: String, repository: String): ProtectedBranchInfo = ProtectedBranchInfo(owner, repository, false, Nil, false) diff --git a/src/main/scala/gitbucket/core/service/PullRequestService.scala b/src/main/scala/gitbucket/core/service/PullRequestService.scala index b383139..789a09d 100644 --- a/src/main/scala/gitbucket/core/service/PullRequestService.scala +++ b/src/main/scala/gitbucket/core/service/PullRequestService.scala @@ -156,7 +156,7 @@ commitIdTo: String){ val statuses: List[CommitStatus] = - commitStatues ++ (branchProtection.contexts.toSet -- commitStatues.map(_.context).toSet).map(branchProtection.pendingCommitStatus(_)) + commitStatues ++ (branchProtection.contexts.toSet -- commitStatues.map(_.context).toSet).map(CommitStatus.pending(branchProtection.owner, branchProtection.repository, _)) val hasRequiredStatusProblem = needStatusCheck && branchProtection.contexts.exists(context => statuses.find(_.context == context).map(_.state) != Some(CommitState.SUCCESS)) val hasProblem = hasRequiredStatusProblem || hasConflict || (!statuses.isEmpty && CommitState.combine(statuses.map(_.state).toSet) != CommitState.SUCCESS) val canUpdate = branchIsOutOfDate && !hasConflict diff --git a/src/main/scala/gitbucket/core/servlet/GitRepositoryServlet.scala b/src/main/scala/gitbucket/core/servlet/GitRepositoryServlet.scala index 371f777..f92207c 100644 --- a/src/main/scala/gitbucket/core/servlet/GitRepositoryServlet.scala +++ b/src/main/scala/gitbucket/core/servlet/GitRepositoryServlet.scala @@ -119,7 +119,7 @@ def onPreReceive(receivePack: ReceivePack, commands: java.util.Collection[ReceiveCommand]): Unit = { try { commands.asScala.foreach { command => - getBranchProtectedReason(owner, repository, receivePack, command, pusher).map{ reason => + getBranchProtectedReason(owner, repository, receivePack.isAllowNonFastForwards, command, pusher).map{ reason => command.setResult(ReceiveCommand.Result.REJECTED_OTHER_REASON, reason) } } diff --git a/src/test/scala/gitbucket/core/service/ProtectedBrancheServiceSpec.scala b/src/test/scala/gitbucket/core/service/ProtectedBrancheServiceSpec.scala new file mode 100644 index 0000000..165c221 --- /dev/null +++ b/src/test/scala/gitbucket/core/service/ProtectedBrancheServiceSpec.scala @@ -0,0 +1,170 @@ +package gitbucket.core.service + +import org.specs2.mutable.Specification +import org.eclipse.jgit.transport.ReceiveCommand +import org.eclipse.jgit.lib.ObjectId +import gitbucket.core.model.CommitState +import ProtectedBrancheService.ProtectedBranchInfo + + +class ProtectedBrancheServiceSpec extends Specification with ServiceSpecBase with ProtectedBrancheService with CommitStatusService { + val now = new java.util.Date() + val sha = "0c77148632618b59b6f70004e3084002be2b8804" + val sha2 = "0c77148632618b59b6f70004e3084002be2b8805" + "getProtectedBranchInfo" should { + "empty is disabled" in { + withTestDB { implicit session => + getProtectedBranchInfo("user1", "repo1", "branch") must_== ProtectedBranchInfo.disabled("user1", "repo1") + } + } + "enable and update and disable" in { + withTestDB { implicit session => + generateNewUserWithDBRepository("user1", "repo1") + enableBranchProtection("user1", "repo1", "branch", false, Nil) + getProtectedBranchInfo("user1", "repo1", "branch") must_== ProtectedBranchInfo("user1", "repo1", true, Nil, false) + enableBranchProtection("user1", "repo1", "branch", true, Seq("hoge","huge")) + getProtectedBranchInfo("user1", "repo1", "branch") must_== ProtectedBranchInfo("user1", "repo1", true, Seq("hoge","huge"), true) + disableBranchProtection("user1", "repo1", "branch") + getProtectedBranchInfo("user1", "repo1", "branch") must_== ProtectedBranchInfo.disabled("user1", "repo1") + } + } + "empty contexts is no-include-administrators" in { + withTestDB { implicit session => + generateNewUserWithDBRepository("user1", "repo1") + enableBranchProtection("user1", "repo1", "branch", false, Nil) + getProtectedBranchInfo("user1", "repo1", "branch").includeAdministrators must_== false + enableBranchProtection("user1", "repo1", "branch", true, Nil) + getProtectedBranchInfo("user1", "repo1", "branch").includeAdministrators must_== false + } + } + "getProtectedBranchList" in { + withTestDB { implicit session => + generateNewUserWithDBRepository("user1", "repo1") + enableBranchProtection("user1", "repo1", "branch", false, Nil) + enableBranchProtection("user1", "repo1", "branch2", false, Seq("fuga")) + enableBranchProtection("user1", "repo1", "branch3", true, Seq("hoge")) + getProtectedBranchList("user1", "repo1").toSet must_== Set("branch", "branch2", "branch3") + } + } + "getBranchProtectedReason on force push from admin" in { + withTestDB { implicit session => + val rc = new ReceiveCommand(ObjectId.fromString(sha), ObjectId.fromString(sha2), "refs/heads/branch", ReceiveCommand.Type.UPDATE_NONFASTFORWARD) + generateNewUserWithDBRepository("user1", "repo1") + getBranchProtectedReason("user1", "repo1", true, rc, "user1") must_== None + enableBranchProtection("user1", "repo1", "branch", false, Nil) + getBranchProtectedReason("user1", "repo1", true, rc, "user1") must_== Some("Cannot force-push to a protected branch") + } + } + "getBranchProtectedReason on force push from othre" in { + withTestDB { implicit session => + val rc = new ReceiveCommand(ObjectId.fromString(sha), ObjectId.fromString(sha2), "refs/heads/branch", ReceiveCommand.Type.UPDATE_NONFASTFORWARD) + generateNewUserWithDBRepository("user1", "repo1") + getBranchProtectedReason("user1", "repo1", true, rc, "user2") must_== None + enableBranchProtection("user1", "repo1", "branch", false, Nil) + getBranchProtectedReason("user1", "repo1", true, rc, "user2") must_== Some("Cannot force-push to a protected branch") + } + } + "getBranchProtectedReason check status on push from othre" in { + withTestDB { implicit session => + val rc = new ReceiveCommand(ObjectId.fromString(sha), ObjectId.fromString(sha2), "refs/heads/branch", ReceiveCommand.Type.UPDATE) + val user1 = generateNewUserWithDBRepository("user1", "repo1") + getBranchProtectedReason("user1", "repo1", false, rc, "user2") must_== None + enableBranchProtection("user1", "repo1", "branch", false, Seq("must")) + getBranchProtectedReason("user1", "repo1", false, rc, "user2") must_== Some("Required status check \"must\" is expected") + enableBranchProtection("user1", "repo1", "branch", false, Seq("must", "must2")) + getBranchProtectedReason("user1", "repo1", false, rc, "user2") must_== Some("2 of 2 required status checks are expected") + createCommitStatus("user1", "repo1", sha2, "context", CommitState.SUCCESS, None, None, now, user1) + getBranchProtectedReason("user1", "repo1", false, rc, "user2") must_== Some("2 of 2 required status checks are expected") + createCommitStatus("user1", "repo1", sha2, "must", CommitState.SUCCESS, None, None, now, user1) + getBranchProtectedReason("user1", "repo1", false, rc, "user2") must_== Some("Required status check \"must2\" is expected") + createCommitStatus("user1", "repo1", sha2, "must2", CommitState.SUCCESS, None, None, now, user1) + getBranchProtectedReason("user1", "repo1", false, rc, "user2") must_== None + } + } + "getBranchProtectedReason check status on push from admin" in { + withTestDB { implicit session => + val rc = new ReceiveCommand(ObjectId.fromString(sha), ObjectId.fromString(sha2), "refs/heads/branch", ReceiveCommand.Type.UPDATE) + val user1 = generateNewUserWithDBRepository("user1", "repo1") + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== None + enableBranchProtection("user1", "repo1", "branch", false, Seq("must")) + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== None + enableBranchProtection("user1", "repo1", "branch", true, Seq("must")) + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== Some("Required status check \"must\" is expected") + enableBranchProtection("user1", "repo1", "branch", false, Seq("must", "must2")) + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== None + enableBranchProtection("user1", "repo1", "branch", true, Seq("must", "must2")) + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== Some("2 of 2 required status checks are expected") + createCommitStatus("user1", "repo1", sha2, "context", CommitState.SUCCESS, None, None, now, user1) + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== Some("2 of 2 required status checks are expected") + createCommitStatus("user1", "repo1", sha2, "must", CommitState.SUCCESS, None, None, now, user1) + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== Some("Required status check \"must2\" is expected") + createCommitStatus("user1", "repo1", sha2, "must2", CommitState.SUCCESS, None, None, now, user1) + getBranchProtectedReason("user1", "repo1", false, rc, "user1") must_== None + } + } + } + "ProtectedBranchInfo" should { + "administrator is owner" in { + withTestDB { implicit session => + generateNewUserWithDBRepository("user1", "repo1") + var x = ProtectedBranchInfo("user1", "repo1", true, Nil, false) + x.isAdministrator("user1") must_== true + x.isAdministrator("user2") must_== false + } + } + "administrator is manager" in { + withTestDB { implicit session => + var x = ProtectedBranchInfo("grp1", "repo1", true, Nil, false) + x.createGroup("grp1", None) + generateNewAccount("user1") + generateNewAccount("user2") + generateNewAccount("user3") + + x.updateGroupMembers("grp1", List("user1"->true, "user2"->false)) + x.isAdministrator("user1") must_== true + x.isAdministrator("user2") must_== false + x.isAdministrator("user3") must_== false + } + } + "unSuccessedContexts" in { + withTestDB { implicit session => + val user1 = generateNewUserWithDBRepository("user1", "repo1") + var x = ProtectedBranchInfo("user1", "repo1", true, List("must"), false) + x.unSuccessedContexts(sha) must_== Set("must") + createCommitStatus("user1", "repo1", sha, "context", CommitState.SUCCESS, None, None, now, user1) + x.unSuccessedContexts(sha) must_== Set("must") + createCommitStatus("user1", "repo1", sha, "must", CommitState.ERROR, None, None, now, user1) + x.unSuccessedContexts(sha) must_== Set("must") + createCommitStatus("user1", "repo1", sha, "must", CommitState.PENDING, None, None, now, user1) + x.unSuccessedContexts(sha) must_== Set("must") + createCommitStatus("user1", "repo1", sha, "must", CommitState.FAILURE, None, None, now, user1) + x.unSuccessedContexts(sha) must_== Set("must") + createCommitStatus("user1", "repo1", sha, "must", CommitState.SUCCESS, None, None, now, user1) + x.unSuccessedContexts(sha) must_== Set() + } + } + "unSuccessedContexts when empty" in { + withTestDB { implicit session => + val user1 = generateNewUserWithDBRepository("user1", "repo1") + var x = ProtectedBranchInfo("user1", "repo1", true, Nil, false) + val sha = "0c77148632618b59b6f70004e3084002be2b8804" + x.unSuccessedContexts(sha) must_== Nil + createCommitStatus("user1", "repo1", sha, "context", CommitState.SUCCESS, None, None, now, user1) + x.unSuccessedContexts(sha) must_== Nil + } + } + "if disabled, needStatusCheck is false" in { + withTestDB { implicit session => + ProtectedBranchInfo("user1", "repo1", false, Seq("must"), true).needStatusCheck("user1") must_== false + } + } + "needStatusCheck includeAdministrators" in { + withTestDB { implicit session => + ProtectedBranchInfo("user1", "repo1", true, Seq("must"), false).needStatusCheck("user2") must_== true + ProtectedBranchInfo("user1", "repo1", true, Seq("must"), false).needStatusCheck("user1") must_== false + ProtectedBranchInfo("user1", "repo1", true, Seq("must"), true ).needStatusCheck("user2") must_== true + ProtectedBranchInfo("user1", "repo1", true, Seq("must"), true ).needStatusCheck("user1") must_== true + } + } + } +} \ No newline at end of file diff --git a/src/test/scala/gitbucket/core/service/RepositoryServiceSpec.scala b/src/test/scala/gitbucket/core/service/RepositoryServiceSpec.scala index b8f7bd4..5d1ab03 100644 --- a/src/test/scala/gitbucket/core/service/RepositoryServiceSpec.scala +++ b/src/test/scala/gitbucket/core/service/RepositoryServiceSpec.scala @@ -8,11 +8,11 @@ class RepositoryServiceSpec extends Specification with ServiceSpecBase with RepositoryService with AccountService{ "RepositoryService" should { - "renameRepository can rename CommitState" in { withTestDB { implicit session => + "renameRepository can rename CommitState, ProtectedBranches" in { withTestDB { implicit session => val tester = generateNewAccount("tester") createRepository("repo","root",None,false) - val commitStatusService = new CommitStatusService{} - val id = commitStatusService.createCommitStatus( + val service = new CommitStatusService with ProtectedBrancheService{} + val id = service.createCommitStatus( userName = "root", repositoryName = "repo", sha = "0e97b8f59f7cdd709418bb59de53f741fd1c1bd7", @@ -22,14 +22,20 @@ description = Some("description"), creator = tester, now = new java.util.Date) - val org = commitStatusService.getCommitStatus("root","repo", id).get + service.enableBranchProtection("root", "repo", "branch", true, Seq("must1", "must2")) + var orgPbi = service.getProtectedBranchInfo("root", "repo", "branch") + val org = service.getCommitStatus("root","repo", id).get + renameRepository("root","repo","tester","repo2") - val neo = commitStatusService.getCommitStatus("tester","repo2", org.commitId, org.context).get + + val neo = service.getCommitStatus("tester","repo2", org.commitId, org.context).get neo must_== org.copy( commitStatusId=neo.commitStatusId, repositoryName="repo2", userName="tester") + service.getProtectedBranchInfo("tester", "repo2", "branch") must_== + orgPbi.copy(owner="tester", repository="repo2") }} } }