diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 38c3e0fc82..be4316ef83 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -147,17 +147,6 @@ be ordered as ranked by the plugins (if there are any). + By default 1. -[[audit]] -=== Section audit - -[[audit.maskSensitiveData]]audit.maskSensitiveData:: -+ -If true, command parameters marked as sensitive are masked in audit logs. -+ -This option only affects audit. Other means of logging will always be masked. -+ -By default `false`. - [[auth]] === Section auth diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index 4a64e68165..43aa886981 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -2659,15 +2659,18 @@ new RestApi("accounts").id("self").view("username") Gerrit provides an extension point that enables Plugins to rank the list of reviewer suggestion a user receives upon clicking "Add Reviewer" on the change screen. + Gerrit supports both a default suggestion that appears when the user has not yet typed anything and a filtered suggestion that is shown as the user starts typing. -Plugins receive a candidate list and can return a Set of suggested reviewers -containing the Account.Id and a score for each reviewer. -The candidate list is non-binding and plugins can choose to return reviewers not -initially contained in the candidate list. -Server administrators can configure the overall weight of each plugin using the -weight config parameter on [addreviewer ""]. + +Plugins receive a candidate list and can return a `Set` of suggested reviewers +containing the `Account.Id` and a score for each reviewer. The candidate list is +non-binding and plugins can choose to return reviewers not initially contained in +the candidate list. + +Server administrators can configure the overall weight of each plugin by setting +the `addreviewer.pluginName-exportName.weight` value in `gerrit.config`. [source, java] ---- diff --git a/Documentation/note-db.txt b/Documentation/note-db.txt index 28066bcf63..fd2bef37f8 100644 --- a/Documentation/note-db.txt +++ b/Documentation/note-db.txt @@ -104,6 +104,11 @@ To run the offline migration, run the `migrate-to-note-db` program: Once started, it is safe to cancel and restart the migration process, or to switch to the online process. +[NOTE] +Migration requires a heap size comparable to running a Gerrit server. If you +normally run `gerrit.war daemon` with an `-Xmx` flag, pass that to the migration +tool as well. + *Advantages* * Much faster than online; can use all available CPUs, since no live traffic diff --git a/WORKSPACE b/WORKSPACE index 6044953e85..c41719cc73 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -600,8 +600,8 @@ maven_jar( maven_jar( name = "dropwizard_core", - artifact = "io.dropwizard.metrics:metrics-core:3.2.5", - sha1 = "ea2316646e9787c5b2d14ca97f4ef7ad5c6b94e9", + artifact = "io.dropwizard.metrics:metrics-core:4.0.2", + sha1 = "ec9878842d510cabd6bd6a9da1bebae1ae0cd199", ) # When updading Bouncy Castle, also update it in bazlets. diff --git a/java/com/google/gerrit/server/change/ReviewerSuggestion.java b/java/com/google/gerrit/server/change/ReviewerSuggestion.java index a2dd8b5fd6..f64c9d04f0 100644 --- a/java/com/google/gerrit/server/change/ReviewerSuggestion.java +++ b/java/com/google/gerrit/server/change/ReviewerSuggestion.java @@ -24,7 +24,7 @@ import java.util.Set; /** * Listener to provide reviewer suggestions. * - *

Invoked by Gerrit a user who is searching for a reviewer to add to a change. + *

Invoked by Gerrit when a user clicks "Add Reviewer" on a change. */ @ExtensionPoint public interface ReviewerSuggestion { diff --git a/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/java/com/google/gerrit/server/index/change/ChangeIndexer.java index e95470da1f..cf51197281 100644 --- a/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -34,6 +34,7 @@ import com.google.gerrit.server.index.IndexExecutor; import com.google.gerrit.server.index.IndexUtils; import com.google.gerrit.server.notedb.ChangeNotes; import com.google.gerrit.server.notedb.NotesMigration; +import com.google.gerrit.server.project.NoSuchChangeException; import com.google.gerrit.server.query.change.ChangeData; import com.google.gerrit.server.util.RequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; @@ -53,6 +54,7 @@ import java.util.List; import java.util.concurrent.Callable; import java.util.concurrent.Future; import java.util.concurrent.atomic.AtomicReference; +import org.eclipse.jgit.errors.RepositoryNotFoundException; import org.eclipse.jgit.lib.Config; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -430,11 +432,23 @@ public class ChangeIndexer { @Override public Boolean callImpl(Provider db) throws Exception { - if (!stalenessChecker.isStale(id)) { - return false; + try { + if (stalenessChecker.isStale(id)) { + index(newChangeData(db.get(), project, id)); + return true; + } + } catch (NoSuchChangeException nsce) { + log.debug("Change {} was deleted, aborting reindexing the change.", id.get()); + } catch (Exception e) { + if (!isCausedByRepositoryNotFoundException(e)) { + throw e; + } + log.debug( + "Change {} belongs to deleted project {}, aborting reindexing the change.", + id.get(), + project.get()); } - index(newChangeData(db.get(), project, id)); - return true; + return false; } @Override @@ -443,6 +457,16 @@ public class ChangeIndexer { } } + private boolean isCausedByRepositoryNotFoundException(Throwable throwable) { + while (throwable != null) { + if (throwable instanceof RepositoryNotFoundException) { + return true; + } + throwable = throwable.getCause(); + } + return false; + } + // Avoid auto-rebuilding when reindexing if reading is disabled. This just // increases contention on the meta ref from a background indexing thread // with little benefit. The next actual write to the entity may still incur a diff --git a/java/com/google/gerrit/server/restapi/change/CreateChange.java b/java/com/google/gerrit/server/restapi/change/CreateChange.java index 9bbd56f7c1..499cbb6f30 100644 --- a/java/com/google/gerrit/server/restapi/change/CreateChange.java +++ b/java/com/google/gerrit/server/restapi/change/CreateChange.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.restapi.change; import static org.eclipse.jgit.lib.Constants.SIGNED_OFF_BY_TAG; +import com.google.common.base.Joiner; import com.google.common.base.MoreObjects; import com.google.common.base.Strings; import com.google.common.collect.Iterables; @@ -240,14 +241,23 @@ public class CreateChange AccountState accountState = me.state(); GeneralPreferencesInfo info = accountState.getGeneralPreferences(); - ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree(); - ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, input.subject); - String commitMessage = ChangeIdUtil.insertId(input.subject, id); + // Add a Change-Id line if there isn't already one + String commitMessage = input.subject; + if (ChangeIdUtil.indexOfChangeId(commitMessage, "\n") == -1) { + ObjectId treeId = mergeTip == null ? emptyTreeId(oi) : mergeTip.getTree(); + ObjectId id = ChangeIdUtil.computeChangeId(treeId, mergeTip, author, author, commitMessage); + commitMessage = ChangeIdUtil.insertId(commitMessage, id); + } + if (Boolean.TRUE.equals(info.signedOffBy)) { - commitMessage += - String.format( - "%s%s", - SIGNED_OFF_BY_TAG, accountState.getAccount().getNameEmail(anonymousCowardName)); + commitMessage = + Joiner.on("\n") + .join( + commitMessage.trim(), + String.format( + "%s%s", + SIGNED_OFF_BY_TAG, + accountState.getAccount().getNameEmail(anonymousCowardName))); } RevCommit c; diff --git a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java index a27d376e1e..b2b728fb4a 100644 --- a/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java +++ b/java/com/google/gerrit/server/restapi/change/ReviewerRecommender.java @@ -133,21 +133,16 @@ public class ReviewerRecommender { changeNotes.getChangeId(), query, reviewerScores.keySet())); - String pluginWeight = - config.getString( - "addReviewer", plugin.getPluginName() + "-" + plugin.getExportName(), "weight"); + String key = plugin.getPluginName() + "-" + plugin.getExportName(); + String pluginWeight = config.getString("addReviewer", key, "weight"); if (Strings.isNullOrEmpty(pluginWeight)) { pluginWeight = "1"; } + log.debug("weight for {}: {}", key, pluginWeight); try { weights.add(Double.parseDouble(pluginWeight)); } catch (NumberFormatException e) { - log.error( - "Exception while parsing weight for " - + plugin.getPluginName() - + "-" - + plugin.getExportName(), - e); + log.error("Exception while parsing weight for {}", key, e); weights.add(1d); } } diff --git a/java/com/google/gerrit/sshd/BaseCommand.java b/java/com/google/gerrit/sshd/BaseCommand.java index 1833f9c971..cf54a476e0 100644 --- a/java/com/google/gerrit/sshd/BaseCommand.java +++ b/java/com/google/gerrit/sshd/BaseCommand.java @@ -48,10 +48,6 @@ import java.io.OutputStreamWriter; import java.io.PrintWriter; import java.io.StringWriter; import java.nio.charset.Charset; -import java.util.ArrayList; -import java.util.HashSet; -import java.util.List; -import java.util.Set; import java.util.concurrent.Future; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.atomic.AtomicReference; @@ -74,8 +70,6 @@ public abstract class BaseCommand implements Command { static final int STATUS_NOT_FOUND = PRIVATE_STATUS | 2; public static final int STATUS_NOT_ADMIN = PRIVATE_STATUS | 3; - private static final String MASK = "***"; - @Option(name = "--", usage = "end of options", handler = EndOfOptionsHandler.class) private boolean endOfOptions; @@ -99,8 +93,6 @@ public abstract class BaseCommand implements Command { @Inject private SshScope.Context context; - @Inject private SshCommandSensitiveFieldsCache cache; - /** Commands declared by a plugin can be scoped by the plugin name. */ @Inject(optional = true) @PluginName @@ -119,10 +111,6 @@ public abstract class BaseCommand implements Command { /** Unparsed command line options. */ private String[] argv; - private List maskedArgv = new ArrayList<>(); - - private Set sensitiveParameters = new HashSet<>(); - public BaseCommand() { task = Atomics.newReference(); } @@ -168,22 +156,6 @@ public abstract class BaseCommand implements Command { this.argv = argv; } - public List getMaskedArguments() { - return maskedArgv; - } - - public String getFormattedMaskedArguments(String delimiter) { - return String.join(delimiter, maskedArgv); - } - - public void setMaskedArguments(List argv) { - this.maskedArgv = argv; - } - - public boolean isSensitiveParameter(String param) { - return sensitiveParameters.contains(param); - } - @Override public void destroy() { Future future = task.getAndSet(null); @@ -354,7 +326,7 @@ public abstract class BaseCommand implements Command { m.append(")"); } m.append(" during "); - m.append(getFormattedMaskedArguments(" ")); + m.append(context.getCommandLine()); log.error(m.toString(), e); } @@ -400,7 +372,7 @@ public abstract class BaseCommand implements Command { protected String getTaskDescription() { StringBuilder m = new StringBuilder(); - m.append(getFormattedMaskedArguments(" ")); + m.append(context.getCommandLine()); return m.toString(); } @@ -416,49 +388,12 @@ public abstract class BaseCommand implements Command { return m.toString(); } - private void maskSensitiveParameters() { - if (argv == null) { - return; - } - sensitiveParameters = cache.get(this.getClass()); - maskedArgv = new ArrayList<>(); - maskedArgv.add(commandName); - boolean maskNext = false; - for (int i = 0; i < argv.length; i++) { - if (maskNext) { - maskedArgv.add(MASK); - maskNext = false; - continue; - } - String arg = argv[i]; - String key = extractKey(arg); - if (isSensitiveParameter(key)) { - maskNext = arg.equals(key); - // When arg != key then parameter contains '=' sign and we mask them right away. - // Otherwise we mask the next parameter as indicated by maskNext. - if (!maskNext) { - arg = key + "=" + MASK; - } - } - maskedArgv.add(arg); - } - } - - private String extractKey(String arg) { - int eqPos = arg.indexOf('='); - if (eqPos > 0) { - return arg.substring(0, eqPos); - } - return arg; - } - private final class TaskThunk implements CancelableRunnable, ProjectRunnable { private final CommandRunnable thunk; private final String taskName; private Project.NameKey projectName; private TaskThunk(CommandRunnable thunk) { - maskSensitiveParameters(); this.thunk = thunk; this.taskName = getTaskName(); } diff --git a/java/com/google/gerrit/sshd/CommandFactoryProvider.java b/java/com/google/gerrit/sshd/CommandFactoryProvider.java index d0615351d7..0287ceb5e0 100644 --- a/java/com/google/gerrit/sshd/CommandFactoryProvider.java +++ b/java/com/google/gerrit/sshd/CommandFactoryProvider.java @@ -29,7 +29,6 @@ import java.io.IOException; import java.io.InputStream; import java.io.OutputStream; import java.util.ArrayList; -import java.util.Arrays; import java.util.List; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -160,7 +159,7 @@ class CommandFactoryProvider implements Provider, LifecycleListe } catch (Exception e) { logger.warn( "Cannot start command \"" - + cmd.getFormattedMaskedArguments(" ") + + ctx.getCommandLine() + "\" for user " + ctx.getSession().getUsername(), e); @@ -180,10 +179,6 @@ class CommandFactoryProvider implements Provider, LifecycleListe try { cmd = dispatcher.get(); cmd.setArguments(argv); - cmd.setMaskedArguments( - argv.length > 0 - ? Arrays.asList(argv[0]) - : Arrays.asList(ctx.getCommandLine().split(" ")[0])); cmd.setInputStream(in); cmd.setOutputStream(out); cmd.setErrorStream(err); diff --git a/java/com/google/gerrit/sshd/DispatchCommand.java b/java/com/google/gerrit/sshd/DispatchCommand.java index 62f80c9ab2..3f2e258537 100644 --- a/java/com/google/gerrit/sshd/DispatchCommand.java +++ b/java/com/google/gerrit/sshd/DispatchCommand.java @@ -107,10 +107,6 @@ final class DispatchCommand extends BaseCommand { atomicCmd.set(cmd); cmd.start(env); - if (cmd instanceof BaseCommand) { - setMaskedArguments(((BaseCommand) cmd).getMaskedArguments()); - } - } catch (UnloggedFailure e) { String msg = e.getMessage(); if (!msg.endsWith("\n")) { diff --git a/java/com/google/gerrit/sshd/SensitiveData.java b/java/com/google/gerrit/sshd/SensitiveData.java deleted file mode 100644 index 1dd7896013..0000000000 --- a/java/com/google/gerrit/sshd/SensitiveData.java +++ /dev/null @@ -1,28 +0,0 @@ -// Copyright (C) 2017 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.sshd; - -import static java.lang.annotation.ElementType.FIELD; -import static java.lang.annotation.RetentionPolicy.RUNTIME; - -import java.lang.annotation.Retention; -import java.lang.annotation.Target; - -/** - * Annotation tagged on a field of an ssh command to indicate the value must be hidden from logs. - */ -@Target({FIELD}) -@Retention(RUNTIME) -public @interface SensitiveData {} diff --git a/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java b/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java deleted file mode 100644 index 8c79299394..0000000000 --- a/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java +++ /dev/null @@ -1,24 +0,0 @@ -// Copyright (C) 2018 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.sshd; - -import java.util.Set; - -/** Keeps data about ssh commands' parameters that have extra secure annotation. */ -public interface SshCommandSensitiveFieldsCache { - Set get(Class command); - - void evictAll(); -} diff --git a/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java b/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java deleted file mode 100644 index b5933886b4..0000000000 --- a/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java +++ /dev/null @@ -1,76 +0,0 @@ -// Copyright (C) 2018 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.sshd; - -import com.google.common.cache.CacheLoader; -import com.google.common.cache.LoadingCache; -import com.google.gerrit.server.cache.CacheModule; -import com.google.inject.Inject; -import com.google.inject.Module; -import com.google.inject.TypeLiteral; -import com.google.inject.name.Named; -import java.lang.reflect.Field; -import java.util.HashSet; -import java.util.Set; -import org.kohsuke.args4j.Option; - -public class SshCommandSensitiveFieldsCacheImpl implements SshCommandSensitiveFieldsCache { - private static final String CACHE_NAME = "sshd_sensitive_command_params"; - private final LoadingCache, Set> sshdCommandsCache; - - static Module module() { - return new CacheModule() { - @Override - protected void configure() { - cache(CACHE_NAME, new TypeLiteral>() {}, new TypeLiteral>() {}) - .loader(Loader.class); - bind(SshCommandSensitiveFieldsCache.class).to(SshCommandSensitiveFieldsCacheImpl.class); - } - }; - } - - @Inject - SshCommandSensitiveFieldsCacheImpl(@Named(CACHE_NAME) LoadingCache, Set> cache) { - sshdCommandsCache = cache; - } - - @Override - public Set get(Class cmd) { - return sshdCommandsCache.getUnchecked(cmd); - } - - @Override - public void evictAll() { - sshdCommandsCache.invalidateAll(); - } - - static class Loader extends CacheLoader, Set> { - - @Override - public Set load(Class cmd) throws Exception { - Set datas = new HashSet<>(); - for (Field field : cmd.getDeclaredFields()) { - if (field.isAnnotationPresent(SensitiveData.class)) { - Option option = field.getAnnotation(Option.class); - datas.add(option.name()); - for (String opt : option.aliases()) { - datas.add(opt); - } - } - } - return datas; - } - } -} diff --git a/java/com/google/gerrit/sshd/SshLog.java b/java/com/google/gerrit/sshd/SshLog.java index 035989a541..6465a3073a 100644 --- a/java/com/google/gerrit/sshd/SshLog.java +++ b/java/com/google/gerrit/sshd/SshLog.java @@ -48,12 +48,9 @@ class SshLog implements LifecycleListener { private static final String P_STATUS = "status"; private static final String P_AGENT = "agent"; - private static final String MASK = "***"; - private final Provider session; private final Provider context; private final AsyncAppender async; - private final boolean auditMask; private final AuditService auditService; @Inject @@ -67,7 +64,6 @@ class SshLog implements LifecycleListener { this.context = context; this.auditService = auditService; - auditMask = config.getBoolean("audit", "maskSensitiveData", false); if (!config.getBoolean("sshd", "requestLog", true)) { async = null; return; @@ -125,7 +121,8 @@ class SshLog implements LifecycleListener { final Context ctx = context.get(); ctx.finished = TimeUtil.nowMs(); - String cmd = extractWhat(dcmd, true); + String cmd = extractWhat(dcmd); + final LoggingEvent event = log(cmd); event.setProperty(P_WAIT, (ctx.started - ctx.created) + "ms"); event.setProperty(P_EXEC, (ctx.finished - ctx.started) + "ms"); @@ -157,11 +154,7 @@ class SshLog implements LifecycleListener { if (async != null) { async.append(event); } - - if (!auditMask) { - cmd = extractWhat(dcmd, false); - } - audit(ctx, status, cmd, extractParameters(dcmd)); + audit(context.get(), status, dcmd); } private ListMultimap extractParameters(DispatchCommand dcmd) { @@ -184,10 +177,7 @@ class SshLog implements LifecycleListener { // --param=value int eqPos = arg.indexOf('='); if (arg.startsWith("--") && eqPos > 0) { - String param = arg.substring(0, eqPos); - String value = - auditMask && dcmd.isSensitiveParameter(param) ? MASK : arg.substring(eqPos + 1); - parms.put(param, value); + parms.put(arg.substring(0, eqPos), arg.substring(eqPos + 1)); continue; } // -p value or --param value @@ -202,7 +192,7 @@ class SshLog implements LifecycleListener { if (paramName == null) { parms.put("$" + argPos++, arg); } else { - parms.put(paramName, auditMask && dcmd.isSensitiveParameter(paramName) ? MASK : arg); + parms.put(paramName, arg); paramName = null; } } @@ -266,6 +256,10 @@ class SshLog implements LifecycleListener { audit(ctx, result, cmd, null); } + void audit(Context ctx, Object result, DispatchCommand cmd) { + audit(ctx, result, extractWhat(cmd), extractParameters(cmd)); + } + private void audit(Context ctx, Object result, String cmd, ListMultimap params) { String sessionId; CurrentUser currentUser; @@ -283,16 +277,11 @@ class SshLog implements LifecycleListener { auditService.dispatch(new SshAuditEvent(sessionId, currentUser, cmd, created, params, result)); } - private String extractWhat(DispatchCommand dcmd, boolean hideSensitive) { + private String extractWhat(DispatchCommand dcmd) { if (dcmd == null) { return "Command was already destroyed"; } - return hideSensitive ? dcmd.getFormattedMaskedArguments(".") : extractWhat(dcmd); - } - - private String extractWhat(DispatchCommand dcmd) { - String name = dcmd.getCommandName(); - StringBuilder commandName = new StringBuilder(name == null ? "" : name); + StringBuilder commandName = new StringBuilder(dcmd.getCommandName()); String[] args = dcmd.getArguments(); for (int i = 1; i < args.length; i++) { commandName.append(".").append(args[i]); diff --git a/java/com/google/gerrit/sshd/SshModule.java b/java/com/google/gerrit/sshd/SshModule.java index c672b00b94..748277e266 100644 --- a/java/com/google/gerrit/sshd/SshModule.java +++ b/java/com/google/gerrit/sshd/SshModule.java @@ -65,7 +65,6 @@ public class SshModule extends LifecycleModule { configureRequestScope(); install(new AsyncReceiveCommits.Module()); configureAliases(); - install(SshCommandSensitiveFieldsCacheImpl.module()); bind(SshLog.class); bind(SshInfo.class).to(SshDaemon.class).in(SINGLETON); diff --git a/java/com/google/gerrit/sshd/SshPluginStarterCallback.java b/java/com/google/gerrit/sshd/SshPluginStarterCallback.java index 047690ce74..9ae1814f56 100644 --- a/java/com/google/gerrit/sshd/SshPluginStarterCallback.java +++ b/java/com/google/gerrit/sshd/SshPluginStarterCallback.java @@ -33,16 +33,13 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe private final DispatchCommandProvider root; private final DynamicMap dynamicBeans; - private final SshCommandSensitiveFieldsCache cache; @Inject SshPluginStarterCallback( @CommandName(Commands.ROOT) DispatchCommandProvider root, - DynamicMap dynamicBeans, - SshCommandSensitiveFieldsCache cache) { + DynamicMap dynamicBeans) { this.root = root; this.dynamicBeans = dynamicBeans; - this.cache = cache; } @Override @@ -59,7 +56,6 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe if (cmd != null) { newPlugin.add(root.replace(Commands.named(newPlugin.getName()), cmd)); } - cache.evictAll(); } private Provider load(Plugin plugin) { diff --git a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index af609a6e8a..dce65e1bb6 100644 --- a/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/javatests/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -106,9 +106,32 @@ public class CreateChangeIT extends AbstractDaemonTest { "invalid Change-Id line format in commit message footer"); } + @Test + public void createEmptyChange_InvalidSubject() throws Exception { + ChangeInput ci = newChangeInput(ChangeStatus.NEW); + ci.subject = "Change-Id: I1234000000000000000000000000000000000000"; + assertCreateFails( + ci, + ResourceConflictException.class, + "missing subject; Change-Id must be in commit message footer"); + } + @Test public void createNewChange() throws Exception { - assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); + ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); + assertThat(info.revisions.get(info.currentRevision).commit.message) + .contains("Change-Id: " + info.changeId); + } + + @Test + public void createNewChangeWithChangeId() throws Exception { + ChangeInput ci = newChangeInput(ChangeStatus.NEW); + String changeId = "I1234000000000000000000000000000000000000"; + String changeIdLine = "Change-Id: " + changeId; + ci.subject = "Subject\n\n" + changeIdLine; + ChangeInfo info = assertCreateSucceeds(ci); + assertThat(info.changeId).isEqualTo(changeId); + assertThat(info.revisions.get(info.currentRevision).commit.message).contains(changeIdLine); } @Test @@ -136,14 +159,38 @@ public class CreateChangeIT extends AbstractDaemonTest { @Test public void createNewChangeSignedOffByFooter() throws Exception { - setSignedOffByFooter(); + setSignedOffByFooter(true); + try { + ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); + String message = info.revisions.get(info.currentRevision).commit.message; + assertThat(message) + .contains( + String.format( + "%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress())); + } finally { + setSignedOffByFooter(false); + } + } - ChangeInfo info = assertCreateSucceeds(newChangeInput(ChangeStatus.NEW)); - String message = info.revisions.get(info.currentRevision).commit.message; - assertThat(message) - .contains( - String.format( - "%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress())); + @Test + public void createNewChangeSignedOffByFooterWithChangeId() throws Exception { + setSignedOffByFooter(true); + try { + ChangeInput ci = newChangeInput(ChangeStatus.NEW); + String changeId = "I1234000000000000000000000000000000000000"; + String changeIdLine = "Change-Id: " + changeId; + ci.subject = "Subject\n\n" + changeIdLine; + ChangeInfo info = assertCreateSucceeds(ci); + assertThat(info.changeId).isEqualTo(changeId); + String message = info.revisions.get(info.currentRevision).commit.message; + assertThat(message).contains(changeIdLine); + assertThat(message) + .contains( + String.format( + "%sAdministrator <%s>", SIGNED_OFF_BY_TAG, admin.getIdent().getEmailAddress())); + } finally { + setSignedOffByFooter(false); + } } @Test @@ -378,7 +425,7 @@ public class CreateChangeIT extends AbstractDaemonTest { ChangeInfo out = gApi.changes().create(in).get(); assertThat(out.project).isEqualTo(in.project); assertThat(out.branch).isEqualTo(in.branch); - assertThat(out.subject).isEqualTo(in.subject); + assertThat(out.subject).isEqualTo(in.subject.split("\n")[0]); assertThat(out.topic).isEqualTo(in.topic); assertThat(out.status).isEqualTo(in.status); assertThat(out.isPrivate).isEqualTo(in.isPrivate); @@ -398,17 +445,21 @@ public class CreateChangeIT extends AbstractDaemonTest { } // TODO(davido): Expose setting of account preferences in the API - private void setSignedOffByFooter() throws Exception { + private void setSignedOffByFooter(boolean value) throws Exception { RestResponse r = adminRestSession.get("/accounts/" + admin.email + "/preferences"); r.assertOK(); GeneralPreferencesInfo i = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class); - i.signedOffBy = true; + i.signedOffBy = value; r = adminRestSession.put("/accounts/" + admin.email + "/preferences", i); r.assertOK(); GeneralPreferencesInfo o = newGson().fromJson(r.getReader(), GeneralPreferencesInfo.class); - assertThat(o.signedOffBy).isTrue(); + if (value) { + assertThat(o.signedOffBy).isTrue(); + } else { + assertThat(o.signedOffBy).isNull(); + } resetCurrentApiUser(); }