From 6efa8fca5e592e49a90492d5f49bbc5f9a5e432c Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 6 Feb 2018 12:02:34 +0900 Subject: [PATCH 01/12] CreateChangeIT: Disable "Insert Signed-off-by" after test The test createNewChangeSignedOffByFooter enables the option to insert the "Signed-off-by" line, but doesn't disable it again after the test has completed, resulting it the option being enabled for any tests that are subsequently run. Change-Id: Ia4913f7b70191781aa466e104e7bd8620e2be98b --- .../rest/change/CreateChangeIT.java | 26 ++++++++++++------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index e2cd7bc0e3..ed0dde04b4 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -111,12 +111,16 @@ public class CreateChangeIT extends AbstractDaemonTest { @Test public void createNewChangeSignedOffByFooter() throws Exception { - setSignedOffByFooter(); - 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())); + 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); + } } @Test @@ -288,20 +292,24 @@ 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(); + } } private ChangeInput newMergeChangeInput(String targetBranch, String sourceRef, From c8c55e2131638509dcd8d5b8b31afea636de311a Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Mon, 5 Feb 2018 18:25:34 +0900 Subject: [PATCH 02/12] CreateChange: Only insert Change-Id if there isn't already one When creating a new change, the Change-Id is inserted using JGit's ChangeIdUtil. This does not detect an existing Change-Id in the subject line, resulting in the commit message having two Change-Id lines. The latter Change-Id, automatically added, is the one that is actually used. Add an extra check to detect the Change-Id in the subject line and not add another one. Bug: Issue 8284 Change-Id: Ic3c5953a86e8287c1b2228f406fdc54c2b4d914a --- .../rest/change/CreateChangeIT.java | 25 +++++++++++++++++-- .../gerrit/server/change/CreateChange.java | 15 +++++++---- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index ed0dde04b4..3692aa50d0 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -104,9 +104,30 @@ 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 @@ -266,7 +287,7 @@ public class CreateChangeIT extends AbstractDaemonTest { private ChangeInfo assertCreateSucceeds(ChangeInput in) throws Exception { ChangeInfo out = gApi.changes().create(in).get(); 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.revisions).hasSize(1); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index f5ddfe5f31..3079441539 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -231,11 +231,16 @@ public class CreateChange implements GeneralPreferencesInfo info = account.getAccount().getGeneralPreferencesInfo(); - 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, From 3c18ab315e99949d04735598d6033e15e19714e9 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Tue, 6 Feb 2018 12:19:09 +0900 Subject: [PATCH 03/12] CreateChange: Fix appending Signed-off-by line after Change-Id The Signed-off-by line was appended onto the existing message without a newline. If there was already a Change-Id line in the commit message footer, this resulted in the Change-Id being malformed. Bug: Issue 8299 Change-Id: I0b8084bcf27737eec3291a79504f20e697a3b187 --- .../rest/change/CreateChangeIT.java | 20 +++++++++++++++++++ .../gerrit/server/change/CreateChange.java | 8 +++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java index 3692aa50d0..993f3f5dcc 100644 --- a/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java +++ b/gerrit-acceptance-tests/src/test/java/com/google/gerrit/acceptance/rest/change/CreateChangeIT.java @@ -144,6 +144,26 @@ public class CreateChangeIT extends AbstractDaemonTest { } } + @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 public void createNewDraftChange() throws Exception { assume().that(isAllowDrafts()).isTrue(); diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java index 3079441539..cf4be185b7 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/change/CreateChange.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.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; @@ -242,9 +243,10 @@ public class CreateChange implements } if (Boolean.TRUE.equals(info.signedOffBy)) { - commitMessage += String.format("%s%s", - SIGNED_OFF_BY_TAG, - account.getAccount().getNameEmail(anonymousCowardName)); + commitMessage = + Joiner.on("\n").join(commitMessage.trim(), String.format( + "%s%s", SIGNED_OFF_BY_TAG, + account.getAccount().getNameEmail(anonymousCowardName))); } RevCommit c; From dfc7e6d313052a40058a648d9eb6413ae910cdfa Mon Sep 17 00:00:00 2001 From: Jacek Centkowski Date: Tue, 6 Feb 2018 09:31:59 +0100 Subject: [PATCH 04/12] Migrate metrics-core to 4.0.2 version Change-Id: Ie0e4cb9341f34a3241e32373894999d0a12165fa Signed-off-by: Jacek Centkowski --- WORKSPACE | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index c9bd66b041..7641e8019b 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -608,8 +608,8 @@ maven_jar( maven_jar( name = "dropwizard_core", - artifact = "io.dropwizard.metrics:metrics-core:3.2.2", - sha1 = "cd9886f498ee2ab2d994f0c779e5553b2c450416", + artifact = "io.dropwizard.metrics:metrics-core:4.0.2", + sha1 = "ec9878842d510cabd6bd6a9da1bebae1ae0cd199", ) BC_VERS = "1.56" From d2dc45716fd502e01d52b668a93683fc12e4171c Mon Sep 17 00:00:00 2001 From: Paladox none Date: Fri, 21 Apr 2017 15:32:02 +0000 Subject: [PATCH 05/12] PolyGerrit: Fix gr-diff-view arrows to use html code See https://www.toptal.com/designers/htmlarrows/ Change-Id: I5255ec40da179292718ade64a93536f27a4fc6a8 (cherry picked from commit 2280cbbcbc432280eea0254ee663c4682d2b9dea) --- .../app/elements/diff/gr-diff-view/gr-diff-view.html | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html index 1204170ca9..40f98125f4 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-view/gr-diff-view.html @@ -286,11 +286,11 @@ limitations under the License.
< + href$="[[_computeNavLinkURL(_path, _fileList, -1, 1)]]"><
[[_computeFileDisplayName(_path)]]
> + href$="[[_computeNavLinkURL(_path, _fileList, 1, 1)]]">>
Date: Fri, 2 Feb 2018 15:09:47 -0500 Subject: [PATCH 06/12] Handle the ReindexIfStale event when a change is deleted It is possible that the ReindexIfStale task is executed by the time the change is deleted, because the ReindexIfStale task is done asynchronously. The NoSuchChangeException is handled for the Reindexing event so that the ReindexIfStale task is not executed if the changed is deleted. Change-Id: I2d5f5c5df768fff858936b772695e171b073f1e0 --- .../gerrit/server/index/change/ChangeIndexer.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java index ee45324b66..3007317053 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -33,6 +33,7 @@ import com.google.gerrit.server.index.Index; import com.google.gerrit.server.index.IndexExecutor; 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; @@ -447,11 +448,15 @@ 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 e) { + log.debug("Change {} was deleted, aborting reindexing the change.", id.get()); } - index(newChangeData(db.get(), project, id)); - return true; + return false; } @Override From 9feac0b937a279b834d449ed07c33fc928de0bb0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20Ar=C3=A8s?= Date: Tue, 6 Feb 2018 12:50:44 -0500 Subject: [PATCH 07/12] Handle deleted project in ReindexIfStaleTask Even if unlikely, it's possible that ReindexIfStale is executed after the project is deleted leading to an error in the logs. Bug: Issue 8301 Change-Id: I41b1e34c48984b0c86ee63a136e947d67e9670ac --- .../server/index/change/ChangeIndexer.java | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java index 3007317053..eb5ccb1680 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/index/change/ChangeIndexer.java @@ -54,6 +54,7 @@ import java.util.concurrent.Callable; import java.util.concurrent.ExecutionException; 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; @@ -453,8 +454,16 @@ public class ChangeIndexer { index(newChangeData(db.get(), project, id)); return true; } - } catch (NoSuchChangeException e) { + } 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()); } return false; } @@ -465,6 +474,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 From 4657155eecfa5993b22f59fe4379deb3c504d53e Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 7 Feb 2018 10:23:25 +0900 Subject: [PATCH 08/12] ReviewerRecommender: Add debug log of plugin provided weight Also update existing log calls to use the Logger's built in string formatting rather than concatenation. Change-Id: Icfaf65eec65aa17a70f85a3b2910a4419212ea43 --- .../google/gerrit/server/ReviewerRecommender.java | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java index b79f4966a7..6a307d64e6 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java +++ b/gerrit-server/src/main/java/com/google/gerrit/server/ReviewerRecommender.java @@ -137,21 +137,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); } } From 74a2af64db4c6318ffa8ee277498e63774a7a794 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 7 Feb 2018 10:13:20 +0900 Subject: [PATCH 09/12] ReviewerSuggestion: Reword Javadoc Change-Id: If0abb7ac9fd99ae9241cc9dd0d27d77eb39c34d8 --- .../com/google/gerrit/server/change/ReviewerSuggestion.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestion.java b/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestion.java index a2dd8b5fd6..f64c9d04f0 100644 --- a/gerrit-server/src/main/java/com/google/gerrit/server/change/ReviewerSuggestion.java +++ b/gerrit-server/src/main/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 { From c2233a492d9654a6decd303a3262dd1c65624bfd Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 7 Feb 2018 10:31:05 +0900 Subject: [PATCH 10/12] dev-plugins: Improve formatting of reviewer suggestion documentation - Add backticks around code - Add line breaks to split the wall-of-text into logical paragraphs - Reword the paragraph about configuring the plugin weight Change-Id: Ic9997036c94ef2fc0e46a1ff62fa8c42c7ef7fc2 --- Documentation/dev-plugins.txt | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Documentation/dev-plugins.txt b/Documentation/dev-plugins.txt index fa6fc96b97..1f290eb297 100644 --- a/Documentation/dev-plugins.txt +++ b/Documentation/dev-plugins.txt @@ -2493,15 +2493,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] ---- From a5bea0ba1416314b44fb4759edd321cccf79b2f0 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Wed, 7 Feb 2018 10:46:46 +0900 Subject: [PATCH 11/12] Revert "Hide sensitive data from audit and gerrit logs" There is concern that this new feature might introduce regressions on the stable branch. Revert it so that it can be done again on master. This reverts commit a8a7a84ae55dd9b70cce8d7eb1dfbfd43600893a. This reverts commit dfa74b835f451461a33c8a93c909fef11e04ff86. Change-Id: I21e2046dfcdbba7d5f4c2704b620ea6ea689abc8 --- Documentation/config-gerrit.txt | 11 --- .../com/google/gerrit/sshd/BaseCommand.java | 69 +---------------- .../gerrit/sshd/CommandFactoryProvider.java | 7 +- .../google/gerrit/sshd/DispatchCommand.java | 4 - .../com/google/gerrit/sshd/SensitiveData.java | 28 ------- .../sshd/SshCommandSensitiveFieldsCache.java | 24 ------ .../SshCommandSensitiveFieldsCacheImpl.java | 76 ------------------- .../java/com/google/gerrit/sshd/SshLog.java | 33 +++----- .../com/google/gerrit/sshd/SshModule.java | 1 - .../gerrit/sshd/SshPluginStarterCallback.java | 7 +- 10 files changed, 15 insertions(+), 245 deletions(-) delete mode 100644 gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java delete mode 100644 gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java delete mode 100644 gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 072dcb8091..0ee3fd7f1d 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -144,17 +144,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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java index e3e367c428..e38c0802f5 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/BaseCommand.java @@ -42,10 +42,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.atomic.AtomicReference; import org.apache.sshd.common.SshException; @@ -67,8 +63,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; @@ -90,8 +84,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 @@ -106,10 +98,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(); } @@ -155,22 +143,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); @@ -353,7 +325,7 @@ public abstract class BaseCommand implements Command { m.append(")"); } m.append(" during "); - m.append(getFormattedMaskedArguments(" ")); + m.append(context.getCommandLine()); log.error(m.toString(), e); } @@ -399,7 +371,7 @@ public abstract class BaseCommand implements Command { protected String getTaskDescription() { StringBuilder m = new StringBuilder(); - m.append(getFormattedMaskedArguments(" ")); + m.append(context.getCommandLine()); return m.toString(); } @@ -413,49 +385,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(final CommandRunnable thunk) { - maskSensitiveParameters(); this.thunk = thunk; this.taskName = getTaskName(); } diff --git a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java index e1f7a65194..c6d750c438 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/CommandFactoryProvider.java +++ b/gerrit-sshd/src/main/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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java index 5e23cdb479..2f3d10f61b 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/DispatchCommand.java @@ -100,10 +100,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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SensitiveData.java deleted file mode 100644 index 1dd7896013..0000000000 --- a/gerrit-sshd/src/main/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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCache.java deleted file mode 100644 index 8c79299394..0000000000 --- a/gerrit-sshd/src/main/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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshCommandSensitiveFieldsCacheImpl.java deleted file mode 100644 index b5933886b4..0000000000 --- a/gerrit-sshd/src/main/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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java index aacd8bc458..dfd56f1f88 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshLog.java +++ b/gerrit-sshd/src/main/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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java index 4108cabb2a..d25c58be3a 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshModule.java @@ -63,7 +63,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/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java index 7932290907..7f76ec6c83 100644 --- a/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java +++ b/gerrit-sshd/src/main/java/com/google/gerrit/sshd/SshPluginStarterCallback.java @@ -30,14 +30,10 @@ class SshPluginStarterCallback implements StartPluginListener, ReloadPluginListe private static final Logger log = LoggerFactory.getLogger(SshPluginStarterCallback.class); private final DispatchCommandProvider root; - private final SshCommandSensitiveFieldsCache cache; @Inject - SshPluginStarterCallback( - @CommandName(Commands.ROOT) DispatchCommandProvider root, - SshCommandSensitiveFieldsCache cache) { + SshPluginStarterCallback(@CommandName(Commands.ROOT) DispatchCommandProvider root) { this.root = root; - this.cache = cache; } @Override @@ -54,7 +50,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) { From 7788ae9cb2165d8d4f99e9cbdd1887ba3d201f41 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 7 Feb 2018 09:58:40 -0500 Subject: [PATCH 12/12] Move downloaded artifact cache from buck-cache to bazel-cache On master, we are removing all traces of Buck (Idb93a48345). To avoid recreating buck-cache when maintainers switch between stable branches, backport the directory renaming to stable-2.14 and later. Change-Id: Ia9c58bc5ac8fd28bac7ee220e917580b60f25a69 --- tools/download_file.py | 3 +-- tools/js/download_bower.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/tools/download_file.py b/tools/download_file.py index 38c2974a7d..b705c1158d 100755 --- a/tools/download_file.py +++ b/tools/download_file.py @@ -25,8 +25,7 @@ from util import hash_file, resolve_url from zipfile import ZipFile, BadZipfile, LargeZipFile GERRIT_HOME = path.expanduser('~/.gerritcodereview') -# TODO(davido): Rename in bazel-cache -CACHE_DIR = path.join(GERRIT_HOME, 'buck-cache', 'downloaded-artifacts') +CACHE_DIR = path.join(GERRIT_HOME, 'bazel-cache', 'downloaded-artifacts') LOCAL_PROPERTIES = 'local.properties' diff --git a/tools/js/download_bower.py b/tools/js/download_bower.py index f5b7bf57cc..80cb56e402 100755 --- a/tools/js/download_bower.py +++ b/tools/js/download_bower.py @@ -26,7 +26,7 @@ import sys import bowerutil CACHE_DIR = os.path.expanduser(os.path.join( - '~', '.gerritcodereview', 'buck-cache', 'downloaded-artifacts')) + '~', '.gerritcodereview', 'bazel-cache', 'downloaded-artifacts')) def bower_cmd(bower, *args):