Remove backup api at /database/backup; change method from GET to POST (#33)

* Remove backup api at /database/backup; change method from GET to POST on /api/v3/plugins/database/backup
* Inline H2BackupController::doExport
* Remove "done: " prefix from backup response so its easier to use the response in scripts; add tests around authorized and unauthorized requests and using a non-default file name

Author: c0llab0rat0r

README.MD

@@ -40,7 +40,7 @@ You can pass an optional argument `dest` that references the destination file pa
 For example calling `http://YOUR_GITBUCKET/api/v3/plugins/database/backup?dest=/var/backups/gitbucket.zip` will do an H2 backup of the gitbucket database into the file `/var/backups/gitbucket.zip`.
 Since `1.3.0`, the _dest_ parameter can denote a relative file path, in this case the file will be created relatively to `GITBUCKET_DATA`.
 
-On success, you will receive a `HTTP 200` answer with a body containing `done: FULL_PATH_OF_BACKUP_FILE`.
+On success, you will receive a `HTTP 200` answer with a `text/plain` body containing `FULL_PATH_OF_BACKUP_FILE`.
 
 ### HTTP API Authorization
 
@@ -83,6 +83,11 @@ sbt clean assembly
 
 ## Release Notes
 
+### Unreleased
+- remove backup api at GET /database/backup
+- change method from GET to POST on /api/v3/plugins/database/backup
+- backup endpoint is secure by default, and requires an api token for a user with admin rights
+
 ### 1.9.0
 - compatibility with GitBucket 4.35.x
 
@@ -102,7 +107,6 @@ sbt clean assembly
 
 - compatibility with GitBucket 4.10, scala 2.12 [#20](https://github.com/gitbucket-plugins/gitbucket-h2-backup-plugin/issues/20)
 - allow to secure `database/backup` endpoint [#1](https://github.com/gitbucket-plugins/gitbucket-h2-backup-plugin/issues/1),[#19](https://github.com/gitbucket-plugins/gitbucket-h2-backup-plugin/issues/19)  
-   see [Securing backup endpoint](#securing-backup-endpoint) paragraph
 
 ### 1.3.0
 
@@ -122,15 +126,3 @@ sbt clean assembly
 
 - introduce gitbucket-h2-backup-plugin
 - allows to backup h2 database via a live dump
-
-## Securing backup endpoint
-
-In version 1.4.0, it is possible to secure the `database/backup` endpoint:
-
-- launch GitBucket with System property _secure.backup_ set to true (for example `-Dsecure.backup=true` on the command line)
-- due to actual limitations of GitBucket & plugins security, once the previous setting is activated,
-a call to `http://YOUR_GITBUCKET/database/backup` will be temporary redirected `http://YOUR_GITBUCKET/api/v3/plugins/database/backup`.
-You have to follow this temporary redirection.
-   - if you call the endpoint using _httpie_, use the `--follow` parameter
-- this secured endpoint route is TEMPORARY you should not call it directly.
-If you do think that it will change in the future when GitBucket will support secured routes for plugins.  

build.sbt

@@ -1,6 +1,14 @@
+val ScalatraVersion = "2.7.1"
+
 organization := "fr.brouillard.gitbucket"
 name := "gitbucket-h2-backup-plugin"
 version := "1.9.0"
 scalaVersion := "2.13.3"
 gitbucketVersion := "4.35.0"
 scalacOptions += "-deprecation"
+
+libraryDependencies ++= Seq(
+  "org.scalatest"                    %% "scalatest-funspec"          % "3.2.3" % "test",
+  "org.scalatest"                    %% "scalatest-funsuite"         % "3.2.3" % "test",
+  "org.scalatra"                     %% "scalatra-scalatest"         % ScalatraVersion % "test",
+)

src/main/scala/fr/brouillard/gitbucket/h2/controller/H2BackupController.scala

@@ -2,19 +2,34 @@ package fr.brouillard.gitbucket.h2.controller
 
 import java.io.File
 import java.util.Date
-
 import fr.brouillard.gitbucket.h2._
-
+import fr.brouillard.gitbucket.h2.controller.H2BackupController.{defaultBackupFileName, doBackup}
 import gitbucket.core.controller.ControllerBase
+import gitbucket.core.model.Account
 import gitbucket.core.util.AdminAuthenticator
 import gitbucket.core.util.Directory._
 import gitbucket.core.servlet.Database
-
-import org.scalatra.Ok
+import org.scalatra.{ActionResult, Ok, Params}
 import org.slf4j.LoggerFactory
-
 import org.scalatra.forms._
 
+object H2BackupController {
+  def defaultBackupFileName(): String = {
+    val format = new java.text.SimpleDateFormat("yyyy-MM-dd_HH-mm")
+    "gitbucket-db-" + format.format(new Date()) + ".zip"
+  }
+
+  def doBackup(exportDatabase: File => Unit, loginAccount: Option[Account], params: Params): ActionResult = {
+    loginAccount match {
+      case Some(x) if x.isAdmin =>
+        val filePath: String = params.getOrElse("dest", defaultBackupFileName())
+        exportDatabase(new File(filePath))
+        Ok(filePath, Map("Content-Type" -> "text/plain"))
+      case _ => org.scalatra.Unauthorized()
+    }
+  }
+}
+
 class H2BackupController extends ControllerBase with AdminAuthenticator {
   private val logger = LoggerFactory.getLogger(classOf[H2BackupController])
 
@@ -27,7 +42,7 @@ class H2BackupController extends ControllerBase with AdminAuthenticator {
   // private val defaultBackupFile:String = new File(GitBucketHome, "gitbucket-database-backup.zip").toString;
 
   def exportDatabase(exportFile: File): Unit = {
-    val destFile = if (exportFile.isAbsolute()) exportFile else new File(GitBucketHome + "/backup", exportFile.toString)
+    val destFile = if (exportFile.isAbsolute) exportFile else new File(GitBucketHome + "/backup", exportFile.toString)
 
     val session = Database.getSession(request)
     val conn = session.conn
@@ -42,26 +57,23 @@ class H2BackupController extends ControllerBase with AdminAuthenticator {
   })
 
   get("/api/v3/plugins/database/backup") {
-    context.loginAccount match {
-      case Some(x) if (x.isAdmin) => doExport()
-      case _ => org.scalatra.Unauthorized()
-    }
+    doBackupMoved()
+  }
+
+  post("/api/v3/plugins/database/backup") {
+    doBackup(exportDatabase, context.loginAccount, params)
   }
 
+  // Legacy api that was insecure/open by default
   get("/database/backup") {
-    if (sys.props.get("secure.backup") exists (_ equalsIgnoreCase "true"))
-      org.scalatra.TemporaryRedirect("/api/v3/plugins/database/backup?dest=" + params.getOrElse("dest", defaultBackupFileName()))
-    else {
-      doExport()
-    }
+    doBackupMoved()
   }
 
-  private def doExport(): Unit = {
-    val filePath: String = params.getOrElse("dest", defaultBackupFileName())
-    exportDatabase(new File(filePath))
-    Ok("done: " + filePath)
+  private def doBackupMoved(): ActionResult = {
+    org.scalatra.MethodNotAllowed("This has moved to POST /api/v3/plugins/database/backup")
   }
 
+  // Responds to a form post from a web page
   post("/database/backup", backupForm) { form: BackupForm =>
     exportDatabase(new File(form.destFile))
     val msg: String = "H2 Database has been exported to '" + form.destFile + "'."
@@ -70,8 +82,4 @@ class H2BackupController extends ControllerBase with AdminAuthenticator {
     redirect("/admin/h2backup")
   }
 
-  private def defaultBackupFileName(): String = {
-    val format = new java.text.SimpleDateFormat("yyyy-MM-dd_HH-mm")
-    "gitbucket-db-" + format.format(new Date()) + ".zip"
-  }
 }

src/main/twirl/fr/brouillard/gitbucket/h2/export.scala.html

@@ -15,7 +15,7 @@
             <fieldset>
                 <label><span class="strong">H2 Database Backup File:</span></label>
                 <p class="muted">
-                    Allows to export/backup the H2 database content. The same action can be achieved via an HTTP GET call to @path/database/backup .
+                    Allows to export/backup the H2 database content. The same action can be achieved via an HTTP POST call to @path/api/v3/plugins/database/backup .
                 </p>
                 backup/<input type="text" name="dest" value="@dest" style="width: 400px" />
             </fieldset>

src/test/scala/fr/brouillard/gitbucket/h2/controller/H2BackupControllerTests.scala

@@ -0,0 +1,123 @@
+package fr.brouillard.gitbucket.h2.controller
+
+import gitbucket.core.model.Account
+import gitbucket.core.servlet.ApiAuthenticationFilter
+import org.scalatest.funsuite.AnyFunSuite
+import org.scalatest.matchers.should.Matchers.{convertToAnyShouldWrapper, equal}
+import org.scalatra.{Ok, Params, ScalatraParams}
+import org.scalatra.test.scalatest.ScalatraFunSuite
+
+import java.io.File
+import java.util.Date
+
+class H2BackupControllerTests extends ScalatraFunSuite {
+  addFilter(classOf[ApiAuthenticationFilter], path="/api/*")
+  addFilter(classOf[H2BackupController], "/*")
+
+  test("get database backup api") {
+    get("/api/v3/plugins/database/backup") {
+      status should equal (405)
+      body should include ("This has moved")
+    }
+  }
+
+  test("get database backup legacy") {
+    get("/database/backup") {
+      status should equal (405)
+      body should include ("This has moved")
+    }
+  }
+
+  test("post database backup without credentials is unauthorized") {
+    post("/api/v3/plugins/database/backup") {
+      status should equal (401)
+    }
+  }
+
+}
+
+class H2BackupControllerObjectTests extends AnyFunSuite {
+  private def assertDefaultFileName(name: String): Unit = {
+    assert(name.startsWith("gitbucket-db"))
+    assert(name.endsWith(".zip"))
+  }
+
+  private def buildAccount(isAdmin: Boolean) = {
+    Account(
+      userName = "a",
+      fullName = "b",
+      mailAddress = "c",
+      password = "d",
+      isAdmin = isAdmin,
+      url = None,
+      registeredDate = new Date(),
+      updatedDate = new Date(),
+      lastLoginDate = None,
+      image = None,
+      isGroupAccount = false,
+      isRemoved = false,
+      description = None)
+  }
+
+  test("generates default file name") {
+    assertDefaultFileName(H2BackupController.defaultBackupFileName())
+  }
+
+  test("post database backup with admin credentials is executed with default file name") {
+    val account = buildAccount(true)
+    val params: Params = new ScalatraParams(Map())
+
+    var executed = false;
+
+    val exportDatabase = (file: File) => {
+      assert(!executed)
+      assertDefaultFileName(file.getName)
+
+      executed = true
+    }
+
+    val action = H2BackupController.doBackup(exportDatabase, Some(account), params)
+
+    assert(executed)
+    assert(action.status == 200)
+
+    // Not JSON and not HTML
+    assert(action.headers.get("Content-Type").contains("text/plain"))
+  }
+
+  test("post database backup with admin credentials is executed with specific file name") {
+    val fileName = "foo.zip"
+    val account = buildAccount(true)
+    val params: Params = new ScalatraParams(Map("dest" -> Seq(fileName)))
+
+    var executed = false;
+
+    val exportDatabase = (file: File) => {
+      assert(!executed)
+      assert(file.getName.equals(fileName))
+
+      executed = true
+    }
+
+    val action = H2BackupController.doBackup(exportDatabase, Some(account), params)
+
+    assert(executed)
+    assert(action.status == 200)
+
+    // Not JSON and not HTML
+    assert(action.headers.get("Content-Type").contains("text/plain"))
+  }
+
+  test("post database backup with unprivileged credentials is unauthorized") {
+    val account = buildAccount(false)
+    val params: Params = new ScalatraParams(Map())
+
+    val exportDatabase = (file: File) => {
+      fail()
+    }
+
+    val action = H2BackupController.doBackup(exportDatabase, Some(account), params)
+    assert(action.status == 401)
+  }
+
+}