From cae842ec6b578c00843f4114053c4e60bc7de78c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Mon, 13 May 2019 14:38:20 +0200 Subject: [PATCH] Block disabling mandatory plugin Change-Id: I0358cfc5629de859d4f0d9ac6b5ac34d16bcdcc4 --- Documentation/cmd-plugin-remove.txt | 1 + Documentation/rest-api-plugins.txt | 18 +++++++ .../gerrit/server/plugins/DisablePlugin.java | 11 +++- .../plugins/MandatoryPluginsCollection.java | 50 +++++++++++++++++++ .../gerrit/server/plugins/PluginLoader.java | 15 +++--- .../gerrit/server/plugins/PluginModule.java | 1 + .../acceptance/api/plugin/PluginIT.java | 17 ++++++- 7 files changed, 105 insertions(+), 8 deletions(-) create mode 100644 java/com/google/gerrit/server/plugins/MandatoryPluginsCollection.java diff --git a/Documentation/cmd-plugin-remove.txt b/Documentation/cmd-plugin-remove.txt index f5fe56b2d6..012bf7b238 100644 --- a/Documentation/cmd-plugin-remove.txt +++ b/Documentation/cmd-plugin-remove.txt @@ -20,6 +20,7 @@ jars in the site path's `plugins` directory to `.disabled`. * Caller must be a member of the privileged 'Administrators' group. * link:config-gerrit.html#plugins.allowRemoteAdmin[plugins.allowRemoteAdmin] must be enabled in `$site_path/etc/gerrit.config`. +* Mandatory plugin cannot be disabled == SCRIPTING This command is intended to be used in scripts. diff --git a/Documentation/rest-api-plugins.txt b/Documentation/rest-api-plugins.txt index 938d101d8e..c34fe77ee2 100644 --- a/Documentation/rest-api-plugins.txt +++ b/Documentation/rest-api-plugins.txt @@ -387,6 +387,24 @@ describes the plugin. } ---- +Disabling of a link:config-gerrit.html#plugins.mandatory[mandatory plugin] +is rejected: + +.Request +---- + DELETE /plugins/replication HTTP/1.0 +---- + +.Response +---- + HTTP/1.1 405 Method Not Allowed + Content-Disposition: attachment + Content-Type: application/json; charset=UTF-8 + + )]}' + Plugin replication is mandatory +---- + [[reload-plugin]] === Reload Plugin -- diff --git a/java/com/google/gerrit/server/plugins/DisablePlugin.java b/java/com/google/gerrit/server/plugins/DisablePlugin.java index 62eb993118..877b348817 100644 --- a/java/com/google/gerrit/server/plugins/DisablePlugin.java +++ b/java/com/google/gerrit/server/plugins/DisablePlugin.java @@ -17,6 +17,7 @@ package com.google.gerrit.server.plugins; import com.google.common.collect.ImmutableSet; import com.google.gerrit.extensions.common.Input; import com.google.gerrit.extensions.common.PluginInfo; +import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.RestApiException; import com.google.gerrit.extensions.restapi.RestModifyView; import com.google.gerrit.server.permissions.GlobalPermission; @@ -30,11 +31,16 @@ public class DisablePlugin implements RestModifyView { private final PluginLoader loader; private final PermissionBackend permissionBackend; + private final MandatoryPluginsCollection mandatoryPluginsCollection; @Inject - DisablePlugin(PluginLoader loader, PermissionBackend permissionBackend) { + DisablePlugin( + PluginLoader loader, + PermissionBackend permissionBackend, + MandatoryPluginsCollection mandatoryPluginsCollection) { this.loader = loader; this.permissionBackend = permissionBackend; + this.mandatoryPluginsCollection = mandatoryPluginsCollection; } @Override @@ -46,6 +52,9 @@ public class DisablePlugin implements RestModifyView { } loader.checkRemoteAdminEnabled(); String name = resource.getName(); + if (mandatoryPluginsCollection.contains(name)) { + throw new MethodNotAllowedException("Plugin " + name + " is mandatory"); + } loader.disablePlugins(ImmutableSet.of(name)); return ListPlugins.toPluginInfo(loader.get(name)); } diff --git a/java/com/google/gerrit/server/plugins/MandatoryPluginsCollection.java b/java/com/google/gerrit/server/plugins/MandatoryPluginsCollection.java new file mode 100644 index 0000000000..70a0fff640 --- /dev/null +++ b/java/com/google/gerrit/server/plugins/MandatoryPluginsCollection.java @@ -0,0 +1,50 @@ +// Copyright (C) 2019 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.server.plugins; + +import com.google.common.annotations.VisibleForTesting; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Sets; +import com.google.gerrit.server.config.GerritServerConfig; +import com.google.inject.Inject; +import com.google.inject.Singleton; +import java.util.Arrays; +import java.util.Set; +import java.util.concurrent.CopyOnWriteArraySet; +import org.eclipse.jgit.lib.Config; + +@Singleton +public class MandatoryPluginsCollection { + private final CopyOnWriteArraySet members; + + @Inject + MandatoryPluginsCollection(@GerritServerConfig Config cfg) { + members = Sets.newCopyOnWriteArraySet(); + members.addAll(Arrays.asList(cfg.getStringList("plugins", null, "mandatory"))); + } + + public boolean contains(String name) { + return members.contains(name); + } + + public Set asSet() { + return ImmutableSet.copyOf(members); + } + + @VisibleForTesting + public void add(String name) { + members.add(name); + } +} diff --git a/java/com/google/gerrit/server/plugins/PluginLoader.java b/java/com/google/gerrit/server/plugins/PluginLoader.java index 7dd4b613e2..e8c89a3dc3 100644 --- a/java/com/google/gerrit/server/plugins/PluginLoader.java +++ b/java/com/google/gerrit/server/plugins/PluginLoader.java @@ -20,7 +20,6 @@ import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import com.google.common.collect.ComparisonChain; import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableSet; import com.google.common.collect.Iterables; import com.google.common.collect.LinkedHashMultimap; import com.google.common.collect.Lists; @@ -51,7 +50,6 @@ import java.nio.file.Paths; import java.util.AbstractMap; import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collection; import java.util.Comparator; import java.util.HashSet; @@ -90,7 +88,7 @@ public class PluginLoader implements LifecycleListener { private final Provider urlProvider; private final PersistentCacheFactory persistentCacheFactory; private final boolean remoteAdmin; - private final Set mandatoryPlugins; + private final MandatoryPluginsCollection mandatoryPlugins; private final UniversalServerPluginProvider serverPluginFactory; private final GerritRuntime gerritRuntime; @@ -105,6 +103,7 @@ public class PluginLoader implements LifecycleListener { @CanonicalWebUrl Provider provider, PersistentCacheFactory cacheFactory, UniversalServerPluginProvider pluginFactory, + MandatoryPluginsCollection mpc, GerritRuntime gerritRuntime) { pluginsDir = sitePaths.plugins_dir; dataDir = sitePaths.data_dir; @@ -118,8 +117,7 @@ public class PluginLoader implements LifecycleListener { serverPluginFactory = pluginFactory; remoteAdmin = cfg.getBoolean("plugins", null, "allowRemoteAdmin", false); - mandatoryPlugins = - ImmutableSet.copyOf(Arrays.asList(cfg.getStringList("plugins", null, "mandatory"))); + mandatoryPlugins = mpc; this.gerritRuntime = gerritRuntime; long checkFrequency = @@ -232,6 +230,11 @@ public class PluginLoader implements LifecycleListener { continue; } + if (mandatoryPlugins.contains(name)) { + logger.atWarning().log("Mandatory plugin %s cannot be disabled", name); + continue; + } + logger.atInfo().log("Disabling plugin %s", active.getName()); Path off = active.getSrcFile().resolveSibling(active.getSrcFile().getFileName() + ".disabled"); @@ -435,7 +438,7 @@ public class PluginLoader implements LifecycleListener { } } - Set missingMandatory = Sets.difference(mandatoryPlugins, loadedPlugins); + Set missingMandatory = Sets.difference(mandatoryPlugins.asSet(), loadedPlugins); if (!missingMandatory.isEmpty()) { throw new MissingMandatoryPluginsException(missingMandatory); } diff --git a/java/com/google/gerrit/server/plugins/PluginModule.java b/java/com/google/gerrit/server/plugins/PluginModule.java index 6bc37bd792..71186e51e7 100644 --- a/java/com/google/gerrit/server/plugins/PluginModule.java +++ b/java/com/google/gerrit/server/plugins/PluginModule.java @@ -34,6 +34,7 @@ public class PluginModule extends LifecycleModule { bind(PluginLoader.class); bind(CopyConfigModule.class); listener().to(PluginLoader.class); + bind(MandatoryPluginsCollection.class); DynamicSet.setOf(binder(), ServerPluginProvider.class); DynamicSet.bind(binder(), ServerPluginProvider.class).to(JarPluginProvider.class); diff --git a/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java b/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java index 2943445a8d..c212ce28e4 100644 --- a/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java +++ b/javatests/com/google/gerrit/acceptance/api/plugin/PluginIT.java @@ -15,6 +15,7 @@ package com.google.gerrit.acceptance.api.plugin; import static com.google.common.truth.Truth.assertThat; +import static com.google.common.truth.Truth.assert_; import static com.google.gerrit.testing.GerritJUnit.assertThrows; import static java.nio.charset.StandardCharsets.UTF_8; import static java.util.stream.Collectors.toList; @@ -35,6 +36,7 @@ import com.google.gerrit.extensions.restapi.MethodNotAllowedException; import com.google.gerrit.extensions.restapi.RawInput; import com.google.gerrit.extensions.restapi.ResourceNotFoundException; import com.google.gerrit.extensions.restapi.RestApiException; +import com.google.gerrit.server.plugins.MandatoryPluginsCollection; import com.google.inject.Inject; import java.util.List; import org.junit.Test; @@ -53,6 +55,7 @@ public class PluginIT extends AbstractDaemonTest { "plugin-a.js", "plugin-b.html", "plugin-c.js", "plugin-d.html", "plugin_e.js"); @Inject private RequestScopeOperations requestScopeOperations; + @Inject private MandatoryPluginsCollection mandatoryPluginsCollection; @Test @GerritConfig(name = "plugins.allowRemoteAdmin", value = "true") @@ -99,7 +102,19 @@ public class PluginIT extends AbstractDaemonTest { assertBadRequest(list().regex(".*in-b").prefix("a")); assertBadRequest(list().substring(".*in-b").prefix("a")); - // Disable + // Disable mandatory + mandatoryPluginsCollection.add("plugin_e"); + api = gApi.plugins().name("plugin_e"); + try { + api.disable(); + assert_().fail("Disabling mandatory plugin should have failed"); + } catch (MethodNotAllowedException e) { + // expected + } + api = gApi.plugins().name("plugin_e"); + assertThat(api.get().disabled).isNull(); + + // Disable non-mandatory api = gApi.plugins().name("plugin-a"); api.disable(); api = gApi.plugins().name("plugin-a");