From 9c2e051c56e1ef6b9c9901f492fb506ee5d16def Mon Sep 17 00:00:00 2001
From: Chocobozzz <me@florianbigard.com>
Date: Wed, 30 Jun 2021 11:45:06 +0200
Subject: [PATCH] Handle broken plugin install

---
 server/lib/plugins/plugin-manager.ts          | 27 +++++++++-------
 server/tests/api/server/plugins.ts            | 32 +++++++++++++++++++
 .../peertube-plugin-test-broken/main.js       | 12 +++++++
 .../peertube-plugin-test-broken/package.json  | 20 ++++++++++++
 4 files changed, 80 insertions(+), 11 deletions(-)
 create mode 100644 server/tests/fixtures/peertube-plugin-test-broken/main.js
 create mode 100644 server/tests/fixtures/peertube-plugin-test-broken/package.json

diff --git a/server/lib/plugins/plugin-manager.ts b/server/lib/plugins/plugin-manager.ts
index 6b9a255a4..6599bccca 100644
--- a/server/lib/plugins/plugin-manager.ts
+++ b/server/lib/plugins/plugin-manager.ts
@@ -304,22 +304,28 @@ export class PluginManager implements ServerHook {
         uninstalled: false,
         peertubeEngine: packageJSON.engine.peertube
       }, { returning: true })
-    } catch (err) {
-      logger.error('Cannot install plugin %s, removing it...', toInstall, { err })
+
+      logger.info('Successful installation of plugin %s.', toInstall)
+
+      await this.registerPluginOrTheme(plugin)
+    } catch (rootErr) {
+      logger.error('Cannot install plugin %s, removing it...', toInstall, { err: rootErr })
 
       try {
-        await removeNpmPlugin(npmName)
+        await this.uninstall(npmName)
       } catch (err) {
-        logger.error('Cannot remove plugin %s after failed installation.', toInstall, { err })
+        logger.error('Cannot uninstall plugin %s after failed installation.', toInstall, { err })
+
+        try {
+          await removeNpmPlugin(npmName)
+        } catch (err) {
+          logger.error('Cannot remove plugin %s after failed installation.', toInstall, { err })
+        }
       }
 
-      throw err
+      throw rootErr
     }
 
-    logger.info('Successful installation of plugin %s.', toInstall)
-
-    await this.registerPluginOrTheme(plugin)
-
     return plugin
   }
 
@@ -425,8 +431,7 @@ export class PluginManager implements ServerHook {
 
     await ensureDir(registerOptions.peertubeHelpers.plugin.getDataDirectoryPath())
 
-    library.register(registerOptions)
-           .catch(err => logger.error('Cannot register plugin %s.', npmName, { err }))
+    await library.register(registerOptions)
 
     logger.info('Add plugin %s CSS to global file.', npmName)
 
diff --git a/server/tests/api/server/plugins.ts b/server/tests/api/server/plugins.ts
index 6046ab97e..6b61c7c33 100644
--- a/server/tests/api/server/plugins.ts
+++ b/server/tests/api/server/plugins.ts
@@ -2,6 +2,7 @@
 
 import 'mocha'
 import * as chai from 'chai'
+import { HttpStatusCode } from '@shared/core-utils'
 import {
   cleanupTests,
   closeAllSequelize,
@@ -10,6 +11,7 @@ import {
   getMyUserInformation,
   getPlugin,
   getPluginPackageJSON,
+  getPluginTestPath,
   getPublicSettings,
   installPlugin,
   killallServers,
@@ -400,6 +402,36 @@ describe('Test plugins', function () {
     expect((res.body as User).theme).to.equal('instance-default')
   })
 
+  it('Should not install a broken plugin', async function () {
+    this.timeout(60000)
+
+    async function check () {
+      const res = await listPlugins({
+        url: server.url,
+        accessToken: server.accessToken,
+        pluginType: PluginType.PLUGIN
+      })
+
+      const plugins: PeerTubePlugin[] = res.body.data
+
+      expect(plugins.find(p => p.name === 'test-broken')).to.not.exist
+    }
+
+    await installPlugin({
+      url: server.url,
+      accessToken: server.accessToken,
+      path: getPluginTestPath('-broken'),
+      expectedStatus: HttpStatusCode.BAD_REQUEST_400
+    })
+
+    await check()
+
+    killallServers([ server ])
+    await reRunServer(server)
+
+    await check()
+  })
+
   after(async function () {
     await closeAllSequelize([ server ])
     await cleanupTests([ server ])
diff --git a/server/tests/fixtures/peertube-plugin-test-broken/main.js b/server/tests/fixtures/peertube-plugin-test-broken/main.js
new file mode 100644
index 000000000..afdb6f7a0
--- /dev/null
+++ b/server/tests/fixtures/peertube-plugin-test-broken/main.js
@@ -0,0 +1,12 @@
+async function register (options) {
+  options.unknownFunction()
+}
+
+async function unregister () {
+  return
+}
+
+module.exports = {
+  register,
+  unregister
+}
diff --git a/server/tests/fixtures/peertube-plugin-test-broken/package.json b/server/tests/fixtures/peertube-plugin-test-broken/package.json
new file mode 100644
index 000000000..fd03df216
--- /dev/null
+++ b/server/tests/fixtures/peertube-plugin-test-broken/package.json
@@ -0,0 +1,20 @@
+{
+  "name": "peertube-plugin-test-broken",
+  "version": "0.0.1",
+  "description": "Plugin test broken",
+  "engine": {
+    "peertube": ">=1.3.0"
+  },
+  "keywords": [
+    "peertube",
+    "plugin"
+  ],
+  "homepage": "https://github.com/Chocobozzz/PeerTube",
+  "author": "Chocobozzz",
+  "bugs": "https://github.com/Chocobozzz/PeerTube/issues",
+  "library": "./main.js",
+  "staticDirs": {},
+  "css": [],
+  "clientScripts": [],
+  "translations": {}
+}