add test

src/main/scala/gitbucket/core/controller/PullRequestsController.scala

@@ -177,19 +177,22 @@ trait PullRequestsControllerBase extends ControllerBase {
         }
         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)
+                                   context.loginAccount.map{ u =>
-                                         .needStatusCheck(context.loginAccount.map(_.userName)),
+                                     !getProtectedBranchInfo(pullreq.requestUserName, pullreq.requestRepositoryName, pullreq.requestBranch).needStatusCheck(u.userName)
+                                   }.getOrElse(false),
            hasMergePermission  = hasMergePermission,
            commitIdTo          = pullreq.commitIdTo)
         html.mergeguide(
-          state,
+          mergeStatus,
           issue,
           pullreq,
           repository,

src/main/scala/gitbucket/core/controller/RepositoryViewerController.scala

@@ -220,7 +220,7 @@ trait RepositoryViewerControllerBase extends ControllerBase {
 
   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 @@ trait RepositoryViewerControllerBase extends ControllerBase {
 
   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))

src/main/scala/gitbucket/core/model/CommitStatus.scala

@@ -19,7 +19,7 @@ trait CommitStatusComponent extends TemplateComponent { self: Profile =>
     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 @@ case class CommitStatus(
   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)
 

src/main/scala/gitbucket/core/service/ProtectedBrancheService.scala

@@ -28,15 +28,12 @@ trait ProtectedBrancheService {
   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 @@ trait ProtectedBrancheService {
   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 @@ object ProtectedBrancheService {
      * 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 @@ object ProtectedBrancheService {
     } 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 = pusher match {
-    def needStatusCheck(pusher: String)(implicit session: Session): Boolean =
+      case _ if !enabled              => false
-      if(!enabled || contexts.isEmpty){
+      case _ if contexts.isEmpty      => false
-        false
+      case _ if includeAdministrators => true
-      }else if(includeAdministrators){
+      case p if isAdministrator(p)    => false
-        true
+      case _                          => 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())
   }
   object ProtectedBranchInfo{
     def disabled(owner: String, repository: String): ProtectedBranchInfo = ProtectedBranchInfo(owner, repository, false, Nil, false)

src/main/scala/gitbucket/core/service/PullRequestService.scala

@@ -156,7 +156,7 @@ object PullRequestService {
     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

src/main/scala/gitbucket/core/servlet/GitRepositoryServlet.scala

@@ -119,7 +119,7 @@ class CommitLogHook(owner: String, repository: String, pusher: String, baseUrl:
   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)
         }
       }

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
+      }
+    }
+  }
+}

src/test/scala/gitbucket/core/service/RepositoryServiceSpec.scala

@@ -8,11 +8,11 @@ import org.specs2.mutable.Specification
 
 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 service = new CommitStatusService with ProtectedBrancheService{}
-      val id = commitStatusService.createCommitStatus(
+      val id = service.createCommitStatus(
         userName    = "root",
         repositoryName = "repo",
         sha         = "0e97b8f59f7cdd709418bb59de53f741fd1c1bd7",
@@ -22,14 +22,20 @@ class RepositoryServiceSpec extends Specification with ServiceSpecBase with Repo
         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")
     }}
   }
 }