Remove backup api at /database/backup; change method from GET to POST #33
Conversation
… on /api/v3/plugins/database/backup
|
Resolves #24. Would appreciate someone else testing out the changes locally. |
src/main/scala/fr/brouillard/gitbucket/h2/controller/H2BackupController.scala
Outdated
Show resolved
Hide resolved
c0llab0rat0r
changed the title
…response in scripts; add tests around authorized and unauthorized requests and using a non-default file name
c0llab0rat0r
changed the title
| @@ -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 = { | |||
There was a problem hiding this comment.
With the design in this pull request (moving functional methods to the companion object), this method also can be moved to the companion object. Connection can be taken as an argument.
There was a problem hiding this comment.
I hadn't made any changes to exportDatabase and was trying to minimize the scope of the code changes while still having good hygiene (like writing tests) - so I did the minimum refactor to have tests.
Testing exportDatabase is a bit more involved since it would require calling a live database. I didn't want to change that area of the code and thus become responsible for it.
There was a problem hiding this comment.
Separately, I feel like there's some design or constraint issues with how backup file names are dealt with.
It seems problematic that an external service can pass a dest with an absolute (from root /) folder or an upwardly-mobile relative folder ( ../../somewhere-else) to a path on the GitBucket server. I think the path should be, at minimum, constrained to a subfolder that's determined by the GitBucket server configuration.
There was a problem hiding this comment.
Yeah, that is true. Also, I found that actions provided by this plugin don't have permission checking although usage of these actions should be limited to admin only. Anyway, they are out of the scope of this pull request.
src/test/scala/fr/brouillard/gitbucket/h2/controller/H2BackupControllerTests.scala
Show resolved
Hide resolved
|
@c0llab0rat0r This looks good to me, though some security issues pointed in comments need to be dealt with in another pull request. Is this ready to merge? |
Yes. I found a second security issue and I'm working on a change for it right now. You can merge this one and I'll put up a separate pr for it. It will be up within 30 minutes. |
|
Great. Thank you! |
… on /api/v3/plugins/database/backup