From 5b9de1c8698f2b9fb4d7344d9089c6ae5994b999 Mon Sep 17 00:00:00 2001 From: Rikard Almgren Date: Tue, 12 Mar 2019 17:55:16 +0100 Subject: [PATCH 01/14] Add sendemail.denyrcpt functionality Currently sendemail.allowrcpt exists in the configuration to whitelist specific emails or domains. This add sendemail.denyrcpt to blacklist specific emails or domains. Example usecase: To be able to block e-mails from being sent to service users that don't have an email and preventing log-spam such as: * EmailException: Recipient address rejected: User unknown in local recipient table denyrcpt is evaluated first. Neither allowrcpt or denyrcpt will prevent account creation but both will prevent confirmation emails. Change-Id: I97d6b378dd0678689f65c42e79de68162c889a92 --- Documentation/config-gerrit.txt | 13 ++++++ Documentation/rest-api-accounts.txt | 3 ++ .../server/mail/send/OutgoingEmail.java | 4 +- .../server/mail/send/SmtpEmailSender.java | 44 ++++++++++++++++--- 4 files changed, 55 insertions(+), 9 deletions(-) diff --git a/Documentation/config-gerrit.txt b/Documentation/config-gerrit.txt index 167f1cb07f..2c1d1464d9 100644 --- a/Documentation/config-gerrit.txt +++ b/Documentation/config-gerrit.txt @@ -4392,6 +4392,19 @@ email address, that one address is added to the white list. If set to a domain name, any address at that domain can receive email from Gerrit. + +If allowrcpt is configured, The set of allowed recipients is: +`allowrcpt - denyrcpt`. ++ +By default, unset, permitting delivery to any email address. + +[[sendemail.denyrcpt]]sendemail.denyrcpt:: ++ +If present, each value adds one entry to the blacklist of email +addresses that Gerrit can send email to. If set to a complete +email address, that one address is added to the blacklist. +If set to a domain name, any address at that domain can *not* receive +email from Gerrit. ++ By default, unset, permitting delivery to any email address. [[sendemail.includeDiff]]sendemail.includeDiff:: diff --git a/Documentation/rest-api-accounts.txt b/Documentation/rest-api-accounts.txt index 615accc0fd..bec8a4b717 100644 --- a/Documentation/rest-api-accounts.txt +++ b/Documentation/rest-api-accounts.txt @@ -639,6 +639,9 @@ link:#email-input[EmailInput]. If link:config-gerrit.html#sendemail.allowrcpt[sendemail.allowrcpt] is configured, the added email address must belong to a domain that is allowed, unless `no_confirmation` is set. +If link:config-gerrit.html#sendemail.denyrcpt[sendemail.denyrcpt] +is configured, make sure that the added email address is *not* disallowed or +belongs to a domain that is disallowed. The link:#email-input[EmailInput] object in the request body may contain additional options for the email address. diff --git a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java index 400bc4f300..664c1bf8a5 100644 --- a/java/com/google/gerrit/server/mail/send/OutgoingEmail.java +++ b/java/com/google/gerrit/server/mail/send/OutgoingEmail.java @@ -504,9 +504,7 @@ public abstract class OutgoingEmail { if (addr != null && addr.getEmail() != null && addr.getEmail().length() > 0) { if (!args.validator.isValid(addr.getEmail())) { logger.atWarning().log("Not emailing %s (invalid email address)", addr.getEmail()); - } else if (!args.emailSender.canEmail(addr.getEmail())) { - logger.atWarning().log("Not emailing %s (prohibited by allowrcpt)", addr.getEmail()); - } else { + } else if (args.emailSender.canEmail(addr.getEmail())) { if (!smtpRcptTo.add(addr)) { if (!override) { return; diff --git a/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java b/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java index 8615c04a02..a407cabe90 100644 --- a/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java +++ b/java/com/google/gerrit/server/mail/send/SmtpEmailSender.java @@ -16,6 +16,7 @@ package com.google.gerrit.server.mail.send; import static java.nio.charset.StandardCharsets.UTF_8; +import com.google.common.flogger.FluentLogger; import com.google.common.io.BaseEncoding; import com.google.common.primitives.Ints; import com.google.gerrit.common.Nullable; @@ -56,6 +57,8 @@ public class SmtpEmailSender implements EmailSender { /** The socket's connect timeout (0 = infinite timeout) */ private static final int DEFAULT_CONNECT_TIMEOUT = 0; + private static final FluentLogger logger = FluentLogger.forEnclosingClass(); + public static class Module extends AbstractModule { @Override protected void configure() { @@ -73,6 +76,7 @@ public class SmtpEmailSender implements EmailSender { private Encryption smtpEncryption; private boolean sslVerify; private Set allowrcpt; + private Set denyrcpt; private String importance; private int expiryDays; @@ -117,6 +121,9 @@ public class SmtpEmailSender implements EmailSender { Set rcpt = new HashSet<>(); Collections.addAll(rcpt, cfg.getStringList("sendemail", null, "allowrcpt")); allowrcpt = Collections.unmodifiableSet(rcpt); + Set rcptdeny = new HashSet<>(); + Collections.addAll(rcptdeny, cfg.getStringList("sendemail", null, "denyrcpt")); + denyrcpt = Collections.unmodifiableSet(rcptdeny); importance = cfg.getString("sendemail", null, "importance"); expiryDays = cfg.getInt("sendemail", null, "expiryDays", 0); } @@ -129,22 +136,47 @@ public class SmtpEmailSender implements EmailSender { @Override public boolean canEmail(String address) { if (!isEnabled()) { + logger.atWarning().log("Not emailing %s (email is disabled)", address); return false; } + String domain = address.substring(address.lastIndexOf('@') + 1); + if (isDenied(address, domain)) { + return false; + } + + return isAllowed(address, domain); + } + + private boolean isDenied(String address, String domain) { + + if (denyrcpt.isEmpty()) { + return false; + } + + if (denyrcpt.contains(address) + || denyrcpt.contains(domain) + || denyrcpt.contains("@" + domain)) { + logger.atWarning().log("Not emailing %s (prohibited by sendemail.denyrcpt)", address); + return true; + } + + return false; + } + + private boolean isAllowed(String address, String domain) { + if (allowrcpt.isEmpty()) { return true; } - if (allowrcpt.contains(address)) { - return true; - } - - String domain = address.substring(address.lastIndexOf('@') + 1); - if (allowrcpt.contains(domain) || allowrcpt.contains("@" + domain)) { + if (allowrcpt.contains(address) + || allowrcpt.contains(domain) + || allowrcpt.contains("@" + domain)) { return true; } + logger.atWarning().log("Not emailing %s (prohibited by sendemail.allowrcpt)", address); return false; } From 73ac69264a29094e5220ca1722e09db14a3e5991 Mon Sep 17 00:00:00 2001 From: Nguyen Tuan Khang Phan Date: Sun, 1 Mar 2020 22:09:29 -0500 Subject: [PATCH 02/14] Update git submodules * Update plugins/replication from branch 'stable-2.16' to e5b9b1752b27024439ca96e8e89f59d6058f2380 - Merge branch 'stable-2.15' into stable-2.16 * stable-2.15: ReplicationQueue: Check nulls in firePendingEvents Log stack trace when an error occur while deleting event Append LF to the json string of persisted replication event Change default for the replicateOnStartup to false Don't lose ref-updated events on plugin restart Change-Id: Icc855dd26ee9f8fb195435d8902404b364242940 - ReplicationQueue: Check nulls in firePendingEvents After a sudden reboot (for unknown reason) Gerrit at startup couldn't load because of NullPointerException. There is a possibility that stored event was null at that point. Extra logging added to handle null events. Change-Id: I72f34d8def6e0246196cd865f33f6e795b21664b - Log stack trace when an error occur while deleting event This will help figuring out root cause of failure to delete event file. Change-Id: I2f9774c3daf19a04f6b04414ba8145c99bb6e0fe (cherry picked from commit b62f006b1350180de0af02c82fb18fb290a2548f) - Append LF to the json string of persisted replication event Change-Id: I83ed3f37071125018bf23f6dcd137ef819ef3559 (cherry picked from commit 5e91925cfd391898e8e33fd149b9e1a115dafee4) - Change default for the replicateOnStartup to false Now that replication events are persistent and non-finished replications rescheduled after plugin restart the replicateOnStartup feature becomes less important. We can change the default value for this option to false. Change-Id: I237d8d8514e01b8786b7db9f39bead4eb475a0a4 (cherry picked from commit 807790f7d4058235a19b2a766e84628168b64ae6) - Don't lose ref-updated events on plugin restart When a ref-updated event is received, persist the event in the directory defined by the replication.eventsDirectory. When the updated ref is replicated deleted the persisted event. If replication queue is non-empty and plugin gets stopped, ref updates will not be replicated and, therefore, the persisted events will not get deleted. When the plugin starts it will schedule replication for all persisted events and delete them. This change provides two benefits: * no ref-updated events are lost on plugin restart * eliminate need for the replicateOnStartup=true setting which schedules replication of all refs for all projects and typically creates a humongous replication queue on every plugin restart. Change-Id: Ieacd084fabe703333241ffda11c8b6c78cced37a (cherry picked from commit bdaea910694dd5a3474dbc051b298aaee9d77950) --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index 6e3c80d352..e5b9b1752b 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit 6e3c80d352681d7e00bf78c5f73dfea7ac4a40a4 +Subproject commit e5b9b1752b27024439ca96e8e89f59d6058f2380 From 3c83ebcef098cea5170510e9c7916bf7022ffd2a Mon Sep 17 00:00:00 2001 From: Nguyen Tuan Khang Phan Date: Sun, 1 Mar 2020 22:09:46 -0500 Subject: [PATCH 03/14] Update git submodules * Update plugins/replication from branch 'stable-3.0' to 81c3b5a8ff4f5fc649b5d4bfa7c62c74c890b19e - Merge branch 'stable-2.16' into stable-3.0 * stable-2.16: ReplicationQueue: Check nulls in firePendingEvents Log stack trace when an error occur while deleting event Append LF to the json string of persisted replication event Change default for the replicateOnStartup to false Don't lose ref-updated events on plugin restart Change-Id: Ic1f42587fce15cfce546c7a3946c0e2d8e75922d - Merge branch 'stable-2.15' into stable-2.16 * stable-2.15: ReplicationQueue: Check nulls in firePendingEvents Log stack trace when an error occur while deleting event Append LF to the json string of persisted replication event Change default for the replicateOnStartup to false Don't lose ref-updated events on plugin restart Change-Id: Icc855dd26ee9f8fb195435d8902404b364242940 - ReplicationQueue: Check nulls in firePendingEvents After a sudden reboot (for unknown reason) Gerrit at startup couldn't load because of NullPointerException. There is a possibility that stored event was null at that point. Extra logging added to handle null events. Change-Id: I72f34d8def6e0246196cd865f33f6e795b21664b - Log stack trace when an error occur while deleting event This will help figuring out root cause of failure to delete event file. Change-Id: I2f9774c3daf19a04f6b04414ba8145c99bb6e0fe (cherry picked from commit b62f006b1350180de0af02c82fb18fb290a2548f) - Append LF to the json string of persisted replication event Change-Id: I83ed3f37071125018bf23f6dcd137ef819ef3559 (cherry picked from commit 5e91925cfd391898e8e33fd149b9e1a115dafee4) - Change default for the replicateOnStartup to false Now that replication events are persistent and non-finished replications rescheduled after plugin restart the replicateOnStartup feature becomes less important. We can change the default value for this option to false. Change-Id: I237d8d8514e01b8786b7db9f39bead4eb475a0a4 (cherry picked from commit 807790f7d4058235a19b2a766e84628168b64ae6) - Don't lose ref-updated events on plugin restart When a ref-updated event is received, persist the event in the directory defined by the replication.eventsDirectory. When the updated ref is replicated deleted the persisted event. If replication queue is non-empty and plugin gets stopped, ref updates will not be replicated and, therefore, the persisted events will not get deleted. When the plugin starts it will schedule replication for all persisted events and delete them. This change provides two benefits: * no ref-updated events are lost on plugin restart * eliminate need for the replicateOnStartup=true setting which schedules replication of all refs for all projects and typically creates a humongous replication queue on every plugin restart. Change-Id: Ieacd084fabe703333241ffda11c8b6c78cced37a (cherry picked from commit bdaea910694dd5a3474dbc051b298aaee9d77950) --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index d19c5e3002..81c3b5a8ff 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit d19c5e3002b13919c337a49e2a48c4bb13d004c3 +Subproject commit 81c3b5a8ff4f5fc649b5d4bfa7c62c74c890b19e From 89f47716a764f7be0f7752079489932bad58cc7f Mon Sep 17 00:00:00 2001 From: Nguyen Tuan Khang Phan Date: Sun, 1 Mar 2020 22:10:01 -0500 Subject: [PATCH 04/14] Update git submodules * Update plugins/replication from branch 'stable-3.1' to fac45f033a80cd0187bc9d1d4ed12c11882d3721 - Merge branch 'stable-3.0' into stable-3.1 * stable-3.0: ReplicationQueue: Check nulls in firePendingEvents Log stack trace when an error occur while deleting event Append LF to the json string of persisted replication event Change default for the replicateOnStartup to false Don't lose ref-updated events on plugin restart Change-Id: I505f3769fe89720533e93c197ffe791eda82e848 - Merge branch 'stable-2.16' into stable-3.0 * stable-2.16: ReplicationQueue: Check nulls in firePendingEvents Log stack trace when an error occur while deleting event Append LF to the json string of persisted replication event Change default for the replicateOnStartup to false Don't lose ref-updated events on plugin restart Change-Id: Ic1f42587fce15cfce546c7a3946c0e2d8e75922d - Merge branch 'stable-2.15' into stable-2.16 * stable-2.15: ReplicationQueue: Check nulls in firePendingEvents Log stack trace when an error occur while deleting event Append LF to the json string of persisted replication event Change default for the replicateOnStartup to false Don't lose ref-updated events on plugin restart Change-Id: Icc855dd26ee9f8fb195435d8902404b364242940 - ReplicationQueue: Check nulls in firePendingEvents After a sudden reboot (for unknown reason) Gerrit at startup couldn't load because of NullPointerException. There is a possibility that stored event was null at that point. Extra logging added to handle null events. Change-Id: I72f34d8def6e0246196cd865f33f6e795b21664b - Log stack trace when an error occur while deleting event This will help figuring out root cause of failure to delete event file. Change-Id: I2f9774c3daf19a04f6b04414ba8145c99bb6e0fe (cherry picked from commit b62f006b1350180de0af02c82fb18fb290a2548f) - Append LF to the json string of persisted replication event Change-Id: I83ed3f37071125018bf23f6dcd137ef819ef3559 (cherry picked from commit 5e91925cfd391898e8e33fd149b9e1a115dafee4) - Change default for the replicateOnStartup to false Now that replication events are persistent and non-finished replications rescheduled after plugin restart the replicateOnStartup feature becomes less important. We can change the default value for this option to false. Change-Id: I237d8d8514e01b8786b7db9f39bead4eb475a0a4 (cherry picked from commit 807790f7d4058235a19b2a766e84628168b64ae6) - Don't lose ref-updated events on plugin restart When a ref-updated event is received, persist the event in the directory defined by the replication.eventsDirectory. When the updated ref is replicated deleted the persisted event. If replication queue is non-empty and plugin gets stopped, ref updates will not be replicated and, therefore, the persisted events will not get deleted. When the plugin starts it will schedule replication for all persisted events and delete them. This change provides two benefits: * no ref-updated events are lost on plugin restart * eliminate need for the replicateOnStartup=true setting which schedules replication of all refs for all projects and typically creates a humongous replication queue on every plugin restart. Change-Id: Ieacd084fabe703333241ffda11c8b6c78cced37a (cherry picked from commit bdaea910694dd5a3474dbc051b298aaee9d77950) --- plugins/replication | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/plugins/replication b/plugins/replication index b74a252bbf..fac45f033a 160000 --- a/plugins/replication +++ b/plugins/replication @@ -1 +1 @@ -Subproject commit b74a252bbff8e2a54cdd236e24eab86d54fa008d +Subproject commit fac45f033a80cd0187bc9d1d4ed12c11882d3721 From a62582cf658ed7e64e48bf973e3e68f43f5d30cf Mon Sep 17 00:00:00 2001 From: brohlfs Date: Thu, 28 Feb 2019 17:56:24 +0100 Subject: [PATCH 05/14] Add a coverage layer to gr-diff Change-Id: Iaa768824a0c32694a9f0dbfe29522b15bb7d7198 --- .../gr-coverage-layer/gr-coverage-layer.html | 24 +++ .../gr-coverage-layer/gr-coverage-layer.js | 129 ++++++++++++++++ .../gr-coverage-layer_test.html | 138 ++++++++++++++++++ .../diff/gr-diff-builder/gr-diff-builder.html | 32 ++++ .../app/elements/diff/gr-diff/gr-diff.html | 10 ++ .../app/elements/diff/gr-diff/gr-diff.js | 5 + 6 files changed, 338 insertions(+) create mode 100644 polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.html create mode 100644 polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js create mode 100644 polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer_test.html diff --git a/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.html b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.html new file mode 100644 index 0000000000..56a6fb9b1a --- /dev/null +++ b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.html @@ -0,0 +1,24 @@ + + + + + + + + diff --git a/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js new file mode 100644 index 0000000000..5e7cc26932 --- /dev/null +++ b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js @@ -0,0 +1,129 @@ +/** + * @license + * 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. + */ +(function() { + 'use strict'; + + /** @enum {string} */ + Gerrit.CoverageType = { + /** + * start_character and end_character of the range will be ignored for this + * type. + */ + COVERED: 'COVERED', + /** + * start_character and end_character of the range will be ignored for this + * type. + */ + NOT_COVERED: 'NOT_COVERED', + PARTIALLY_COVERED: 'PARTIALLY_COVERED', + /** + * You don't have to use this. If there is no coverage information for a + * range, then it implicitly means NOT_INSTRUMENTED. start_character and + * end_character of the range will be ignored for this type. + */ + NOT_INSTRUMENTED: 'NOT_INSTRUMENTED', + }; + + /** + * @typedef {{ + * side: string, + * type: Gerrit.CoverageType, + * code_range: Gerrit.Range, + * }} + */ + Gerrit.CoverageRange; + + Polymer({ + is: 'gr-coverage-layer', + + properties: { + /** + * Must be sorted by code_range.start_line. + * Must only contain ranges that match the side. + * + * @type {!Array} + */ + coverageRanges: Array, + side: String, + + /** + * We keep track of the line number from the previous annotate() call, + * and also of the index of the coverage range that had matched. + * annotate() calls are coming in with increasing line numbers and + * coverage ranges are sorted by line number. So this is a very simple + * and efficient way for finding the coverage range that matches a given + * line number. + */ + _lineNumber: { + type: Number, + value: 0, + }, + _index: { + type: Number, + value: 0, + }, + }, + + /** + * Layer method to add annotations to a line. + * + * @param {!HTMLElement} el Not used for this layer. + * @param {!HTMLElement} lineNumberEl The element with the line number. + * @param {!Object} line Not used for this layer. + */ + annotate(el, lineNumberEl, line) { + if (!lineNumberEl || !lineNumberEl.classList.contains(this.side)) { + return; + } + const elementLineNumber = parseInt( + lineNumberEl.getAttribute('data-value'), 10); + if (!elementLineNumber || elementLineNumber < 1) return; + + // If the line number is smaller than before, then we have to reset our + // algorithm and start searching the coverage ranges from the beginning. + // That happens for example when you expand diff sections. + if (elementLineNumber < this._lineNumber) { + this._index = 0; + } + this._lineNumber = elementLineNumber; + + // We simply loop through all the coverage ranges until we find one that + // matches the line number. + while (this._index < this.coverageRanges.length) { + const coverageRange = this.coverageRanges[this._index]; + + // If the line number has moved past the current coverage range, then + // try the next coverage range. + if (this._lineNumber > coverageRange.code_range.end_line) { + this._index++; + continue; + } + + // If the line number has not reached the next coverage range (and the + // range before also did not match), then this line has not been + // instrumented. Nothing to do for this line. + if (this._lineNumber < coverageRange.code_range.start_line) { + return; + } + + // The line number is within the current coverage range. Style it! + lineNumberEl.classList.add(coverageRange.type); + return; + } + }, + }); +})(); diff --git a/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer_test.html b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer_test.html new file mode 100644 index 0000000000..edd88a24e1 --- /dev/null +++ b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer_test.html @@ -0,0 +1,138 @@ + + + + +gr-coverage-layer + + + + + + + + + + + + + + + diff --git a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html index 443d88690f..d42d8c1fd6 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-builder/gr-diff-builder.html @@ -16,6 +16,7 @@ limitations under the License. --> + @@ -31,6 +32,14 @@ limitations under the License. + + @@ -108,6 +117,19 @@ limitations under the License. type: Array, value: () => [], }, + /** @type {!Array} */ + coverageRanges: { + type: Array, + value: () => [], + }, + _leftCoverageRanges: { + type: Array, + computed: '_computeLeftCoverageRanges(coverageRanges)', + }, + _rightCoverageRanges: { + type: Array, + computed: '_computeRightCoverageRanges(coverageRanges)', + }, /** * The promise last returned from `render()` while the asynchronous * rendering is running - `null` otherwise. Provides a `cancel()` @@ -126,6 +148,14 @@ limitations under the License. '_groupsChanged(_groups.splices)', ], + _computeLeftCoverageRanges(coverageRanges) { + return coverageRanges.filter(range => range && range.side === 'left'); + }, + + _computeRightCoverageRanges(coverageRanges) { + return coverageRanges.filter(range => range && range.side === 'right'); + }, + render(keyLocations, prefs) { // Setting up annotation layers must happen after plugins are // installed, and |render| satisfies the requirement, however, @@ -185,6 +215,8 @@ limitations under the License. this._createIntralineLayer(), this._createTabIndicatorLayer(), this.$.rangeLayer, + this.$.coverageLayerLeft, + this.$.coverageLayerRight, ]; // Get layers from plugins (if any). diff --git a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html index 8cf0e3f105..72fc1ee469 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html +++ b/polygerrit-ui/app/elements/diff/gr-diff/gr-diff.html @@ -271,6 +271,15 @@ limitations under the License. .newlineWarning.hidden { display: none; } + .lineNum.COVERED { + background-color: #E0F2F1; + } + .lineNum.NOT_COVERED { + background-color: #FFD1A4; + } + .lineNum.PARTIALLY_COVERED { + background: linear-gradient(to right bottom, #FFD1A4 0%, #FFD1A4 50%, #E0F2F1 50%, #E0F2F1 100%); + }
@@ -290,6 +299,7 @@ limitations under the License. [], }, + /** @type {!Array} */ + coverageRanges: { + type: Array, + value: () => [], + }, lineWrapping: { type: Boolean, value: false, From 4cd7c02b2a7c146df7c0d44b3ce1ec3c1ba94c36 Mon Sep 17 00:00:00 2001 From: brohlfs Date: Fri, 1 Mar 2019 17:28:32 +0100 Subject: [PATCH 06/14] Add a plugin API for coverage data Change-Id: Id09bf2da548b8def10420ea85addc9575c734ab4 --- .../diff/gr-diff-host/gr-diff-host.html | 3 ++ .../diff/gr-diff-host/gr-diff-host.js | 25 ++++++++++++++ .../gr-annotation-actions-js-api.js | 33 +++++++++++++++++++ .../gr-js-api-interface.js | 30 +++++++++++++++++ 4 files changed, 91 insertions(+) diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html index 63af2b00e0..53ce6e6975 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.html @@ -20,6 +20,7 @@ limitations under the License. + @@ -45,8 +46,10 @@ limitations under the License. error-message="[[_errorMessage]]" base-image="[[_baseImage]]" revision-image=[[_revisionImage]] + coverage-ranges="[[_coverageRanges]]" blame="[[_blame]]" diff="[[diff]]"> + diff --git a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js index a177fb121e..9760d50580 100644 --- a/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js +++ b/polygerrit-ui/app/elements/diff/gr-diff-host/gr-diff-host.js @@ -193,6 +193,16 @@ value: null, }, + /** + * TODO(brohlfs): Replace Object type by Gerrit.CoverageRange. + * + * @type {!Array} + */ + _coverageRanges: { + type: Array, + value: () => [], + }, + _loadedWhitespaceLevel: String, _parentIndex: { @@ -247,6 +257,21 @@ this._errorMessage = null; const whitespaceLevel = this._getIgnoreWhitespace(); + this._coverageRanges = []; + const {changeNum, path, patchRange: {basePatchNum, patchNum}} = this; + this.$.jsAPI.getCoverageRanges(changeNum, path, basePatchNum, patchNum). + then(coverageRanges => { + if (changeNum !== this.changeNum || + path !== this.path || + basePatchNum !== this.patchRange.basePatchNum || + patchNum !== this.patchRange.patchNum) { + return; + } + this._coverageRanges = coverageRanges; + }).catch(err => { + console.warn('Loading coverage ranges failed: ', err); + }); + const diffRequest = this._getDiff() .then(diff => { this._loadedWhitespaceLevel = whitespaceLevel; diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.js index 09ff92a7f6..3f91a826c5 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.js +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-annotation-actions-js-api.js @@ -26,6 +26,8 @@ // notifying their listeners in the notify function. this._annotationLayers = []; + this._coverageProvider = null; + // Default impl is a no-op. this._addLayerFunc = annotationActionsContext => {}; } @@ -57,6 +59,37 @@ return this; }; + /** + * The specified function will be called when a gr-diff component is built, + * and feeds the returned coverage data into the diff. Optional. + * + * Be sure to call this only once and only from one plugin. Multiple coverage + * providers are not supported. A second call will just overwrite the + * provider of the first call. + * + * TODO(brohlfs): Replace Array type by Array. + * + * @param {function(changeNum, path, basePatchNum, patchNum): + * !Promise>} coverageProvider + * @return {GrAnnotationActionsInterface} + */ + GrAnnotationActionsInterface.prototype.setCoverageProvider = function( + coverageProvider) { + if (this._coverageProvider) { + console.warn('Overwriting an existing coverage provider.'); + } + this._coverageProvider = coverageProvider; + return this; + }; + + /** + * Used by Gerrit to look up the coverage provider. Not intended to be called + * by plugins. + */ + GrAnnotationActionsInterface.prototype.getCoverageProvider = function() { + return this._coverageProvider; + }; + /** * Returns a checkbox HTMLElement that can be used to toggle annotations * on/off. The checkbox will be initially disabled. Plugins should enable it diff --git a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface.js b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface.js index 16fb7b773d..ad318d7a74 100644 --- a/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface.js +++ b/polygerrit-ui/app/elements/shared/gr-js-api-interface/gr-js-api-interface.js @@ -228,6 +228,36 @@ return layers; }, + /** + * Retrieves coverage data possibly provided by a plugin. + * + * Will wait for plugins to be loaded. If multiple plugins offer a coverage + * provider, the first one is used. If no plugin offers a coverage provider, + * will resolve to []. + * + * TODO(brohlfs): Replace Array type by Array. + * + * @param {string|number} changeNum + * @param {string} path + * @param {string|number} basePatchNum + * @param {string|number} patchNum + * @return {!Promise>} + */ + getCoverageRanges(changeNum, path, basePatchNum, patchNum) { + return Gerrit.awaitPluginsLoaded().then(() => { + for (const annotationApi of + this._getEventCallbacks(EventType.ANNOTATE_DIFF)) { + const provider = annotationApi.getCoverageProvider(); + // Only one coverage provider makes sense. If there are more, then we + // simply ignore them. + if (provider) { + return provider(changeNum, path, basePatchNum, patchNum); + } + } + return []; + }); + }, + getAdminMenuLinks() { const links = []; for (const adminApi of From cf47c07dfbf5ad426abbaa562648353ce205df0f Mon Sep 17 00:00:00 2001 From: Ben Rohlfs Date: Fri, 26 Apr 2019 15:03:57 +0200 Subject: [PATCH 07/14] Add tooltips to coverage layer line elements Change-Id: I37bd363882d50cf4bcc07a339f899659b9f5fe8d --- .../elements/diff/gr-coverage-layer/gr-coverage-layer.js | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js index 5e7cc26932..e8d6900c7d 100644 --- a/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js +++ b/polygerrit-ui/app/elements/diff/gr-coverage-layer/gr-coverage-layer.js @@ -38,6 +38,13 @@ NOT_INSTRUMENTED: 'NOT_INSTRUMENTED', }; + const TOOLTIP_MAP = new Map([ + [Gerrit.CoverageType.COVERED, 'Covered by tests.'], + [Gerrit.CoverageType.NOT_COVERED, 'Not covered by tests.'], + [Gerrit.CoverageType.PARTIALLY_COVERED, 'Partially covered by tests.'], + [Gerrit.CoverageType.NOT_INSTRUMENTED, 'Not instrumented by any tests.'], + ]); + /** * @typedef {{ * side: string, @@ -122,6 +129,7 @@ // The line number is within the current coverage range. Style it! lineNumberEl.classList.add(coverageRange.type); + lineNumberEl.title = TOOLTIP_MAP.get(coverageRange.type); return; } }, From 474449e6c5eb955992cf54d6a20b5517090948a2 Mon Sep 17 00:00:00 2001 From: Paladox none Date: Wed, 4 Mar 2020 16:00:52 +0000 Subject: [PATCH 08/14] Add support for fsharp in highlighting syntax Change-Id: I904e02e29c7e389dabcdb685bb20ca6833f1f0d9 --- .../app/elements/diff/gr-syntax-layer/gr-syntax-layer.js | 1 + 1 file changed, 1 insertion(+) diff --git a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js index 446805e291..c11fe451eb 100644 --- a/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js +++ b/polygerrit-ui/app/elements/diff/gr-syntax-layer/gr-syntax-layer.js @@ -46,6 +46,7 @@ 'text/x-elm': 'elm', 'text/x-erlang': 'erlang', 'text/x-fortran': 'fortran', + 'text/x-fsharp': 'fsharp', 'text/x-go': 'go', 'text/x-groovy': 'groovy', 'text/x-haml': 'haml', From 235f9ccb1a5c31c883e1761d94d678851d81fed6 Mon Sep 17 00:00:00 2001 From: Paladox none Date: Wed, 4 Mar 2020 15:22:07 +0000 Subject: [PATCH 09/14] Add support for going to linNum in inline editor Bug: Issue 12364 Change-Id: Id9d64c0b9c2a953490af1b32554bfe68c6fd82b6 (cherry picked from commit 5e45f039fa287861b131389e9e83a309f2b5a60f) --- .../app/elements/core/gr-router/gr-router.js | 5 +-- .../core/gr-router/gr-router_test.html | 31 +++++++++++++++++++ .../edit/gr-editor-view/gr-editor-view.html | 1 + .../edit/gr-editor-view/gr-editor-view.js | 2 ++ 4 files changed, 37 insertions(+), 2 deletions(-) diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router.js b/polygerrit-ui/app/elements/core/gr-router/gr-router.js index 2c9a7326e5..f6c763f01f 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router.js +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router.js @@ -129,8 +129,8 @@ // eslint-disable-next-line max-len DIFF: /^\/c\/(.+)\/\+\/(\d+)(\/((-?\d+|edit)(\.\.(\d+|edit))?(\/(.+))))\/?$/, - // Matches /c//+//[]/,edit - DIFF_EDIT: /^\/c\/(.+)\/\+\/(\d+)\/(\d+|edit)\/(.+),edit$/, + // Matches /c//+//[]/,edit[#lineNum] + DIFF_EDIT: /^\/c\/(.+)\/\+\/(\d+)\/(\d+|edit)\/(.+),edit(\#\d+)?$/, // Matches non-project-relative // /c//[..]/. @@ -1363,6 +1363,7 @@ changeNum: ctx.params[1], patchNum: ctx.params[2], path: ctx.params[3], + lineNum: ctx.hash, view: Gerrit.Nav.View.EDIT, }); }, diff --git a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html index a907db25c8..08ed9777c0 100644 --- a/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html +++ b/polygerrit-ui/app/elements/core/gr-router/gr-router_test.html @@ -1555,6 +1555,37 @@ limitations under the License. view: Gerrit.Nav.View.EDIT, path: 'foo/bar/baz', patchNum: 3, + lineNum: undefined, + }; + + element._handleDiffEditRoute(ctx); + assert.isFalse(redirectStub.called); + assert.isTrue(normalizeRangeSpy.calledOnce); + assert.deepEqual(normalizeRangeSpy.lastCall.args[0], appParams); + assert.isFalse(normalizeRangeSpy.lastCall.returnValue); + assert.deepEqual(setParamsStub.lastCall.args[0], appParams); + }); + + test('_handleDiffEditRoute with lineNum', () => { + const normalizeRangeSpy = + sandbox.spy(element, '_normalizePatchRangeParams'); + sandbox.stub(element.$.restAPI, 'setInProjectLookup'); + const ctx = { + params: [ + 'foo/bar', // 0 Project + 1234, // 1 Change number + 3, // 2 Patch num + 'foo/bar/baz', // 3 File path + ], + hash: 4, + }; + const appParams = { + project: 'foo/bar', + changeNum: 1234, + view: Gerrit.Nav.View.EDIT, + path: 'foo/bar/baz', + patchNum: 3, + lineNum: 4, }; element._handleDiffEditRoute(ctx); diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html index f80e9f82b2..b107221fed 100644 --- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html +++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.html @@ -116,6 +116,7 @@ limitations under the License. + diff --git a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js index 51dc0f6b3e..f89139ba63 100644 --- a/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js +++ b/polygerrit-ui/app/elements/edit/gr-editor-view/gr-editor-view.js @@ -70,6 +70,7 @@ computed: '_computeSaveDisabled(_content, _newContent, _saving)', }, _prefs: Object, + _lineNum: Number, }, behaviors: [ @@ -108,6 +109,7 @@ this._changeNum = value.changeNum; this._path = value.path; this._patchNum = value.patchNum || this.EDIT_NAME; + this._lineNum = value.lineNum; // NOTE: This may be called before attachment (e.g. while parentElement is // null). Fire title-change in an async so that, if attachment to the DOM From 5780e488d523aa467746fac3b5aa5ab381a5d10e Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 5 Mar 2020 08:57:00 +0900 Subject: [PATCH 10/14] Upgrade jackson-core to 2.10.3 This version includes a few minor fixes since 2.10.2. There are no fixes that we specifically need; this is just to keep up-to-date with the latest. Change-Id: I1d57501d5fb06f38d54ca68048f4e7b1d2dfe153 --- tools/nongoogle.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/nongoogle.bzl b/tools/nongoogle.bzl index 7819502d5e..ba073c2f62 100644 --- a/tools/nongoogle.bzl +++ b/tools/nongoogle.bzl @@ -100,8 +100,8 @@ def declare_nongoogle_deps(): maven_jar( name = "jackson-core", - artifact = "com.fasterxml.jackson.core:jackson-core:2.10.2", - sha1 = "73d4322a6bda684f676a2b5fe918361c4e5c7cca", + artifact = "com.fasterxml.jackson.core:jackson-core:2.10.3", + sha1 = "f7ee7b55c7d292ac72fbaa7648c089f069c938d2", ) # Test-only dependencies below. From 7074214c4027c94272393907e5e2bc1c01ef4665 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 5 Mar 2020 13:03:45 +0900 Subject: [PATCH 11/14] Bump Bazel version to 2.2.0 There is nothing that we specifically need in this version; this upgrade is only to ensure that the project is still buildable with the latest version. Change-Id: I676049e0625208ade16d5540d323c5d1f327d23b --- .bazelversion | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.bazelversion b/.bazelversion index 7ec1d6db40..ccbccc3dc6 100644 --- a/.bazelversion +++ b/.bazelversion @@ -1 +1 @@ -2.1.0 +2.2.0 From 462bc1b5865ca6a5e386cb6fee95874c9d74bcb3 Mon Sep 17 00:00:00 2001 From: David Pursehouse Date: Thu, 5 Mar 2020 14:48:51 +0900 Subject: [PATCH 12/14] Format bzl files with buildifier version 1.0.0 Update the contribution documentation to recommend using buildifier version 1.0.0. Use version 1.0.0 to reformat bzl files with --lint=fix. Note: Usually we include the output of `buildifier --version' in the commit message, but in this version it is broken and doesn't provide any meaningful information. See [1]. [1] https://github.com/bazelbuild/buildtools/issues/782 Change-Id: Id3392bae5115e3cd52ad910fa7b1a38e06cf28cb --- Documentation/dev-contributing.txt | 2 +- tools/bzl/asciidoc.bzl | 2 +- tools/bzl/js.bzl | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/Documentation/dev-contributing.txt b/Documentation/dev-contributing.txt index e7a4284846..1454b81458 100644 --- a/Documentation/dev-contributing.txt +++ b/Documentation/dev-contributing.txt @@ -171,7 +171,7 @@ To format Java source code, Gerrit uses the link:https://github.com/google/google-java-format[`google-java-format`] tool (version 1.7), and to format Bazel BUILD, WORKSPACE and .bzl files the link:https://github.com/bazelbuild/buildtools/tree/master/buildifier[`buildifier`] -tool (version 0.29.0). Unused dependencies are found and removed using the +tool (version 1.0.0). Unused dependencies are found and removed using the link:https://github.com/bazelbuild/buildtools/tree/master/unused_deps[`unused_deps`] build tool, a sibling of `buildifier`. diff --git a/tools/bzl/asciidoc.bzl b/tools/bzl/asciidoc.bzl index f3c4646b2b..1e7ec96ce3 100644 --- a/tools/bzl/asciidoc.bzl +++ b/tools/bzl/asciidoc.bzl @@ -1,7 +1,7 @@ def documentation_attributes(): return [ "toc2", - 'newline="\\n"', + "newline=\"\\n\"", 'asterisk="*"', 'plus="+"', 'caret="^"', diff --git a/tools/bzl/js.bzl b/tools/bzl/js.bzl index 8184d75393..d637ad4d2c 100644 --- a/tools/bzl/js.bzl +++ b/tools/bzl/js.bzl @@ -526,7 +526,7 @@ def polygerrit_plugin(name, app, srcs = [], deps = [], assets = None, plugin_nam cmd = "cp $(SRCS) $(@D)", output_to_bindir = True, ) - static_files += [":" + name + "_copy_assets"] + static_files.append(":" + name + "_copy_assets") native.filegroup( name = name, From 11b517e2cc3bc9c37ae6508ce4ae5d25f09946b0 Mon Sep 17 00:00:00 2001 From: Matthias Sohn Date: Thu, 5 Mar 2020 17:43:12 +0100 Subject: [PATCH 13/14] JGit metrics: expose total load time for block cache entries This allows to calculate current average load time within a time interval by dividing increase of total_load_time by increase of load_success_count within a given time interval. The already exposed metric avg_load_time gives the average since the gerrit process was started. Change-Id: I40eb61cc716b7d545aeae4eb522a439cc48bd74d --- Documentation/metrics.txt | 7 ++++--- .../google/gerrit/metrics/proc/JGitMetricModule.java | 11 +++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/Documentation/metrics.txt b/Documentation/metrics.txt index 9fe2df9395..e6654c3de4 100644 --- a/Documentation/metrics.txt +++ b/Documentation/metrics.txt @@ -139,9 +139,10 @@ topic submissions that concluded successfully. === JGit -* `jgit/block_cache/cache_used`: Bytes of memory retained in JGit block cache. -* `jgit/block_cache/open_files`: File handles held open by JGit block cache. -* `avg_load_time` Average time to load a cache entry for JGit block cache. +* `jgit/block_cache/cache_used` : Bytes of memory retained in JGit block cache. +* `jgit/block_cache/open_files` : File handles held open by JGit block cache. +* `avg_load_time` : Average time to load a cache entry for JGit block cache. +* `total_load_time` : Total time to load cache entries for JGit block cache. * `eviction_count` : Cache evictions for JGit block cache. * `eviction_ratio` : Cache eviction ratio for JGit block cache. * `hit_count` : Cache hits for JGit block cache. diff --git a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java index b9d86504fe..a44f907cc7 100644 --- a/java/com/google/gerrit/metrics/proc/JGitMetricModule.java +++ b/java/com/google/gerrit/metrics/proc/JGitMetricModule.java @@ -65,6 +65,17 @@ public class JGitMetricModule extends MetricModule { } }); + metrics.newCallbackMetric( + "jgit/block_cache/total_load_time", + Long.class, + new Description("Total time to load cache entries for JGit block cache.").setGauge(), + new Supplier() { + @Override + public Long get() { + return WindowCacheStats.getStats().getTotalLoadTime(); + } + }); + metrics.newCallbackMetric( "jgit/block_cache/eviction_count", Long.class, From d02084da95e9eefccebb28b21117cdb712669bfc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Wed, 4 Mar 2020 17:14:15 +0100 Subject: [PATCH 14/14] Fix the access-path for AbstractGitCommand subclasses The access path for the Receive.currentUser in the receive-pack command was wrongly set to SSH_COMMAND instead of, as intended [1], to GIT. This allowed project owners to force-update a ref using git-over-SSH without having en explicit permission for that, see [2]. Interestingly, the current SSH session in the Receive.session had properly set access-path for the associated user: Receive.session.getUser().getAccessPath() == GIT. but it wasn't used. Yet another interesting thing is that both currentUser and session fields in the Receive class are redundant: we already have current SSH session field in the base class (AbstractGitCommand) which has associated current user with properly set access path to GIT. Remove both redundant fields and use the current session from the superclass and the current user from the current session. Refactor the ForcePushIT into SshForcePushIT and HttpForcePushIT in order to test permission over both protocols. [1] https://gerrit.googlesource.com/gerrit/+/462bc1b5865ca6a5e386cb6fee95874c9d74bcb3/java/com/google/gerrit/sshd/AbstractGitCommand.java#72 [2] https://gerrit.googlesource.com/gerrit/+/462bc1b5865ca6a5e386cb6fee95874c9d74bcb3/java/com/google/gerrit/server/permissions/RefControl.java#196 Bug: Issue 12440 Change-Id: I909713411c1c576e6e63e074609d4198fd46397d --- .../gerrit/sshd/AbstractGitCommand.java | 4 +-- .../google/gerrit/sshd/commands/Receive.java | 9 +++--- .../google/gerrit/sshd/commands/Upload.java | 2 -- ...orcePushIT.java => AbstractForcePush.java} | 4 +-- .../acceptance/git/HttpForcePushIT.java | 29 +++++++++++++++++++ .../gerrit/acceptance/git/SshForcePushIT.java | 29 +++++++++++++++++++ 6 files changed, 65 insertions(+), 12 deletions(-) rename javatests/com/google/gerrit/acceptance/git/{ForcePushIT.java => AbstractForcePush.java} (97%) create mode 100644 javatests/com/google/gerrit/acceptance/git/HttpForcePushIT.java create mode 100644 javatests/com/google/gerrit/acceptance/git/SshForcePushIT.java diff --git a/java/com/google/gerrit/sshd/AbstractGitCommand.java b/java/com/google/gerrit/sshd/AbstractGitCommand.java index c49ae82de2..f617ebb79b 100644 --- a/java/com/google/gerrit/sshd/AbstractGitCommand.java +++ b/java/com/google/gerrit/sshd/AbstractGitCommand.java @@ -36,12 +36,12 @@ public abstract class AbstractGitCommand extends BaseCommand { @Inject private GitRepositoryManager repoManager; - @Inject private SshSession session; - @Inject private SshScope.Context context; @Inject private IdentifiedUser.GenericFactory userFactory; + @Inject protected SshSession session; + protected Repository repo; protected Project.NameKey projectName; protected Project project; diff --git a/java/com/google/gerrit/sshd/commands/Receive.java b/java/com/google/gerrit/sshd/commands/Receive.java index 53a9ca252b..0b089b68ce 100644 --- a/java/com/google/gerrit/sshd/commands/Receive.java +++ b/java/com/google/gerrit/sshd/commands/Receive.java @@ -20,7 +20,7 @@ import com.google.common.flogger.FluentLogger; import com.google.gerrit.common.data.Capable; import com.google.gerrit.extensions.restapi.AuthException; import com.google.gerrit.reviewdb.client.Account; -import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.CurrentUser; import com.google.gerrit.server.git.DefaultAdvertiseRefsHook; import com.google.gerrit.server.git.receive.AsyncReceiveCommits; import com.google.gerrit.server.notedb.ReviewerStateInternal; @@ -29,7 +29,6 @@ import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.sshd.AbstractGitCommand; import com.google.gerrit.sshd.CommandMetaData; -import com.google.gerrit.sshd.SshSession; import com.google.inject.Inject; import java.io.IOException; import java.util.ArrayList; @@ -50,8 +49,6 @@ final class Receive extends AbstractGitCommand { private static final FluentLogger logger = FluentLogger.forEnclosingClass(); @Inject private AsyncReceiveCommits.Factory factory; - @Inject private IdentifiedUser currentUser; - @Inject private SshSession session; @Inject private PermissionBackend permissionBackend; private final SetMultimap reviewers = @@ -77,6 +74,7 @@ final class Receive extends AbstractGitCommand { @Override protected void runImpl() throws IOException, Failure { + CurrentUser currentUser = session.getUser(); try { permissionBackend .user(currentUser) @@ -88,7 +86,8 @@ final class Receive extends AbstractGitCommand { throw new Failure(1, "fatal: unable to check permissions " + e); } - AsyncReceiveCommits arc = factory.create(projectState, currentUser, repo, null); + AsyncReceiveCommits arc = + factory.create(projectState, currentUser.asIdentifiedUser(), repo, null); try { Capable r = arc.canUpload(); diff --git a/java/com/google/gerrit/sshd/commands/Upload.java b/java/com/google/gerrit/sshd/commands/Upload.java index 30fd80aca5..e195098a64 100644 --- a/java/com/google/gerrit/sshd/commands/Upload.java +++ b/java/com/google/gerrit/sshd/commands/Upload.java @@ -27,7 +27,6 @@ import com.google.gerrit.server.permissions.PermissionBackend.RefFilterOptions; import com.google.gerrit.server.permissions.PermissionBackendException; import com.google.gerrit.server.permissions.ProjectPermission; import com.google.gerrit.sshd.AbstractGitCommand; -import com.google.gerrit.sshd.SshSession; import com.google.inject.Inject; import java.io.IOException; import java.util.List; @@ -45,7 +44,6 @@ final class Upload extends AbstractGitCommand { @Inject private DynamicSet postUploadHooks; @Inject private DynamicSet uploadPackInitializers; @Inject private UploadValidators.Factory uploadValidatorsFactory; - @Inject private SshSession session; @Inject private PermissionBackend permissionBackend; private PackStatistics stats; diff --git a/javatests/com/google/gerrit/acceptance/git/ForcePushIT.java b/javatests/com/google/gerrit/acceptance/git/AbstractForcePush.java similarity index 97% rename from javatests/com/google/gerrit/acceptance/git/ForcePushIT.java rename to javatests/com/google/gerrit/acceptance/git/AbstractForcePush.java index d80faa8ee1..9b5fd7ab8c 100644 --- a/javatests/com/google/gerrit/acceptance/git/ForcePushIT.java +++ b/javatests/com/google/gerrit/acceptance/git/AbstractForcePush.java @@ -21,7 +21,6 @@ import static org.eclipse.jgit.transport.RemoteRefUpdate.Status.OK; import static org.eclipse.jgit.transport.RemoteRefUpdate.Status.REJECTED_OTHER_REASON; import com.google.gerrit.acceptance.AbstractDaemonTest; -import com.google.gerrit.acceptance.NoHttpd; import com.google.gerrit.acceptance.PushOneCommit; import com.google.gerrit.common.data.Permission; import com.google.gerrit.extensions.api.projects.BranchInput; @@ -31,8 +30,7 @@ import org.eclipse.jgit.transport.PushResult; import org.eclipse.jgit.transport.RemoteRefUpdate; import org.junit.Test; -@NoHttpd -public class ForcePushIT extends AbstractDaemonTest { +public abstract class AbstractForcePush extends AbstractDaemonTest { @Test public void forcePushNotAllowed() throws Exception { diff --git a/javatests/com/google/gerrit/acceptance/git/HttpForcePushIT.java b/javatests/com/google/gerrit/acceptance/git/HttpForcePushIT.java new file mode 100644 index 0000000000..3fdc0238c9 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/git/HttpForcePushIT.java @@ -0,0 +1,29 @@ +// Copyright (C) 2020 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.acceptance.git; + +import com.google.gerrit.acceptance.GitUtil; +import org.eclipse.jgit.transport.CredentialsProvider; +import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; +import org.junit.Before; + +public class HttpForcePushIT extends AbstractForcePush { + @Before + public void cloneProjectOverHttp() throws Exception { + CredentialsProvider.setDefault( + new UsernamePasswordCredentialsProvider(admin.username, admin.httpPassword)); + testRepo = GitUtil.cloneProject(project, admin.getHttpUrl(server) + "/" + project.get()); + } +} diff --git a/javatests/com/google/gerrit/acceptance/git/SshForcePushIT.java b/javatests/com/google/gerrit/acceptance/git/SshForcePushIT.java new file mode 100644 index 0000000000..4ccb347355 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/git/SshForcePushIT.java @@ -0,0 +1,29 @@ +// Copyright (C) 2020 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.acceptance.git; + +import com.google.gerrit.acceptance.GitUtil; +import com.google.gerrit.acceptance.NoHttpd; +import com.google.gerrit.acceptance.UseSsh; +import org.junit.Before; + +@NoHttpd +@UseSsh +public class SshForcePushIT extends AbstractForcePush { + @Before + public void cloneProjectOverSsh() throws Exception { + testRepo = GitUtil.cloneProject(project, adminSshSession.getUrl() + "/" + project.get()); + } +}