Skip to content

Commit 5233fbe

Browse files
takezoetakezoe
authored
Access control at endpoints rather than internal logic (#36)
1 parent cd9e226 commit 5233fbe

File tree

3 files changed

+98
-101
lines changed

3 files changed

+98
-101
lines changed

‎build.sbt

+1
Original file line numberDiff line numberDiff line change
@@ -11,4 +11,5 @@ libraryDependencies ++= Seq(
1111
"org.scalatest" %% "scalatest-funspec" % "3.2.3" % "test",
1212
"org.scalatest" %% "scalatest-funsuite" % "3.2.3" % "test",
1313
"org.scalatra" %% "scalatra-scalatest" % ScalatraVersion % "test",
14+
"org.mockito" % "mockito-core" % "3.9.0" % "test",
1415
)

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

+19-26
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ package fr.brouillard.gitbucket.h2.controller
33
import java.io.File
44
import java.util.Date
55
import fr.brouillard.gitbucket.h2._
6-
import fr.brouillard.gitbucket.h2.controller.H2BackupController.{defaultBackupFileName, doBackup, exportConnectedDatabase, logger}
6+
import fr.brouillard.gitbucket.h2.controller.H2BackupController.{defaultBackupFileName, exportConnectedDatabase}
77
import gitbucket.core.controller.ControllerBase
88
import gitbucket.core.model.Account
99
import gitbucket.core.util.AdminAuthenticator
@@ -25,16 +25,6 @@ object H2BackupController {
2525
"gitbucket-db-" + format.format(new Date()) + ".zip"
2626
}
2727

28-
def doBackup(exportDatabase: File => Unit, loginAccount: Option[Account], params: Params): ActionResult = {
29-
loginAccount match {
30-
case Some(x) if x.isAdmin =>
31-
val filePath: String = params.getOrElse("dest", defaultBackupFileName())
32-
exportDatabase(new File(filePath))
33-
Ok(filePath, Map("Content-Type" -> "text/plain"))
34-
case _ => org.scalatra.Unauthorized()
35-
}
36-
}
37-
3828
def exportConnectedDatabase(conn: Connection, exportFile: File): Unit = {
3929
val destFile = if (exportFile.isAbsolute) exportFile else new File(GitBucketHome + "/backup", exportFile.toString)
4030

@@ -58,38 +48,41 @@ class H2BackupController extends ControllerBase with AdminAuthenticator {
5848
"dest" -> trim(label("Destination", text(required)))
5949
)(BackupForm.apply)
6050

61-
def exportDatabase(exportFile: File): Unit = {
51+
private def exportDatabase(exportFile: File): Unit = {
6252
exportConnectedDatabase(Database.getSession(request).conn, exportFile)
6353
}
6454

55+
private def doBackupMoved(): ActionResult = {
56+
org.scalatra.MethodNotAllowed("This has moved to POST /api/v3/plugins/database/backup")
57+
}
58+
6559
get("/admin/h2backup")(adminOnly {
6660
html.export(flash.get("info"), flash.get("dest").orElse(Some(defaultBackupFileName())))
6761
})
6862

69-
get("/api/v3/plugins/database/backup") {
63+
// Migrated to POST method. This action will be removed in the future version.
64+
get("/api/v3/plugins/database/backup")(adminOnly {
7065
doBackupMoved()
71-
}
66+
})
7267

73-
post("/api/v3/plugins/database/backup") {
74-
doBackup(exportDatabase, context.loginAccount, params)
75-
}
68+
post("/api/v3/plugins/database/backup")(adminOnly {
69+
val filePath = params.getOrElse("dest", defaultBackupFileName())
70+
exportDatabase(new File(filePath))
71+
Ok(filePath, Map("Content-Type" -> "text/plain"))
72+
})
7673

77-
// Legacy api that was insecure/open by default
78-
get("/database/backup") {
74+
// Legacy api that was insecure/open by default. This action will be removed in the future version.
75+
get("/database/backup")(adminOnly {
7976
doBackupMoved()
80-
}
81-
82-
private def doBackupMoved(): ActionResult = {
83-
org.scalatra.MethodNotAllowed("This has moved to POST /api/v3/plugins/database/backup")
84-
}
77+
})
8578

8679
// Responds to a form post from a web page
87-
post("/database/backup", backupForm) { form: BackupForm =>
80+
post("/database/backup", backupForm)(adminOnly { form: BackupForm =>
8881
exportDatabase(new File(form.destFile))
8982
val msg: String = "H2 Database has been exported to '" + form.destFile + "'."
9083
flash.update("info", msg)
9184
flash.update("dest", form.destFile)
9285
redirect("/admin/h2backup")
93-
}
86+
})
9487

9588
}

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

+78-75
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,12 @@
11
package fr.brouillard.gitbucket.h2.controller
22

3+
import gitbucket.core.controller.Context
34
import gitbucket.core.model.Account
45
import gitbucket.core.servlet.ApiAuthenticationFilter
56
import org.apache.commons.io.FileSystemUtils
67
import org.h2.Driver
78
import org.h2.engine.Database
9+
import org.mockito.Mockito._
810
import org.scalatest.funsuite.AnyFunSuite
911
import org.scalatest.matchers.should.Matchers.{convertToAnyShouldWrapper, equal}
1012
import org.scalatra.{Ok, Params, ScalatraParams}
@@ -15,30 +17,76 @@ import java.nio.file.{Files, Path, Paths}
1517
import java.util.{Date, Properties}
1618
import scala.util.Using
1719

18-
class H2BackupControllerTests extends ScalatraFunSuite {
20+
import H2BackupControllerTests._
21+
import gitbucket.core.service.SystemSettingsService
22+
23+
class H2BackupControllerWithAdminTests extends ScalatraFunSuite {
1924
addFilter(classOf[ApiAuthenticationFilter], path="/api/*")
20-
addFilter(classOf[H2BackupController], "/*")
25+
addFilter(new H2BackupController() {
26+
override implicit val context = buildContext(isAdmin = true)
27+
}, "/*")
2128

22-
test("get database backup api") {
29+
test("get database backup api with admin") {
2330
get("/api/v3/plugins/database/backup") {
2431
status should equal (405)
2532
body should include ("This has moved")
2633
}
2734
}
2835

29-
test("get database backup legacy") {
36+
test("get database backup legacy with admin") {
3037
get("/database/backup") {
3138
status should equal (405)
3239
body should include ("This has moved")
3340
}
3441
}
42+
}
3543

36-
test("post database backup without credentials is unauthorized") {
44+
class H2BackupControllerWithNonAdminTests extends ScalatraFunSuite {
45+
addFilter(classOf[ApiAuthenticationFilter], path="/api/*")
46+
addFilter(new H2BackupController() {
47+
override implicit val context = buildContext(isAdmin = false)
48+
}, "/*")
49+
50+
test("get database backup api with non-admin") {
51+
get("/api/v3/plugins/database/backup") {
52+
status should equal (401)
53+
}
54+
}
55+
56+
test("get database backup legacy with non-admin") {
57+
get("/database/backup") {
58+
status should equal (401)
59+
}
60+
}
61+
62+
test("post database backup with non-admin") {
3763
post("/api/v3/plugins/database/backup") {
3864
status should equal (401)
3965
}
4066
}
67+
}
68+
69+
class H2BackupControllerWithoutLoginTests extends ScalatraFunSuite {
70+
addFilter(classOf[ApiAuthenticationFilter], path="/api/*")
71+
addFilter(classOf[H2BackupController], "/*")
72+
73+
test("get database backup api without login") {
74+
get("/api/v3/plugins/database/backup") {
75+
status should equal (401)
76+
}
77+
}
4178

79+
test("get database backup legacy without login") {
80+
get("/database/backup") {
81+
status should equal (401)
82+
}
83+
}
84+
85+
test("post database backup without login") {
86+
post("/api/v3/plugins/database/backup") {
87+
status should equal (401)
88+
}
89+
}
4290
}
4391

4492
class H2BackupControllerObjectTests extends AnyFunSuite {
@@ -47,23 +95,6 @@ class H2BackupControllerObjectTests extends AnyFunSuite {
4795
assert(name.endsWith(".zip"))
4896
}
4997

50-
private def buildAccount(isAdmin: Boolean) = {
51-
Account(
52-
userName = "a",
53-
fullName = "b",
54-
mailAddress = "c",
55-
password = "d",
56-
isAdmin = isAdmin,
57-
url = None,
58-
registeredDate = new Date(),
59-
updatedDate = new Date(),
60-
lastLoginDate = None,
61-
image = None,
62-
isGroupAccount = false,
63-
isRemoved = false,
64-
description = None)
65-
}
66-
6798
private def h2Url(file: File): String = {
6899
"jdbc:h2:file:" + file + ";DATABASE_TO_UPPER=false"
69100
}
@@ -110,62 +141,34 @@ class H2BackupControllerObjectTests extends AnyFunSuite {
110141
test("generates default file name") {
111142
assertDefaultFileName(H2BackupController.defaultBackupFileName())
112143
}
144+
}
113145

114-
test("post database backup with admin credentials is executed with default file name") {
115-
val account = buildAccount(true)
116-
val params: Params = new ScalatraParams(Map())
117-
118-
var executed = false;
119-
120-
val exportDatabase = (file: File) => {
121-
assert(!executed)
122-
assertDefaultFileName(file.getName)
123-
124-
executed = true
125-
}
126-
127-
val action = H2BackupController.doBackup(exportDatabase, Some(account), params)
128-
129-
assert(executed)
130-
assert(action.status == 200)
131-
132-
// Not JSON and not HTML
133-
assert(action.headers.get("Content-Type").contains("text/plain"))
134-
}
135-
136-
test("post database backup with admin credentials is executed with specific file name") {
137-
val fileName = "foo.zip"
138-
val account = buildAccount(true)
139-
val params: Params = new ScalatraParams(Map("dest" -> Seq(fileName)))
140-
141-
var executed = false;
142-
143-
val exportDatabase = (file: File) => {
144-
assert(!executed)
145-
assert(file.getName.equals(fileName))
146-
147-
executed = true
148-
}
149-
150-
val action = H2BackupController.doBackup(exportDatabase, Some(account), params)
151-
152-
assert(executed)
153-
assert(action.status == 200)
146+
object H2BackupControllerTests {
147+
val systemSetting = mock(classOf[SystemSettingsService.SystemSettings])
148+
when(systemSetting.sshAddress).thenReturn(None)
154149

155-
// Not JSON and not HTML
156-
assert(action.headers.get("Content-Type").contains("text/plain"))
150+
def buildContext(isAdmin: Boolean) = {
151+
val context = mock(classOf[Context])
152+
when(context.baseUrl).thenReturn("http://localhost:8080")
153+
when(context.loginAccount).thenReturn(Some(buildAccount(isAdmin)))
154+
when(context.settings).thenReturn(systemSetting)
155+
context
157156
}
158157

159-
test("post database backup with unprivileged credentials is unauthorized") {
160-
val account = buildAccount(false)
161-
val params: Params = new ScalatraParams(Map())
162-
163-
val exportDatabase = (file: File) => {
164-
fail()
165-
}
166-
167-
val action = H2BackupController.doBackup(exportDatabase, Some(account), params)
168-
assert(action.status == 401)
158+
def buildAccount(isAdmin: Boolean) = {
159+
Account(
160+
userName = "a",
161+
fullName = "b",
162+
mailAddress = "c",
163+
password = "d",
164+
isAdmin = isAdmin,
165+
url = None,
166+
registeredDate = new Date(),
167+
updatedDate = new Date(),
168+
lastLoginDate = None,
169+
image = None,
170+
isGroupAccount = false,
171+
isRemoved = false,
172+
description = None)
169173
}
170-
171174
}

0 commit comments

Comments
 (0)