From 4e0a432f8e05e0b07b4af8a67a8fba21560f5b99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 12 Dec 2019 17:12:16 +0100 Subject: Log also thread name in the httpd_log Having thread name in the httpd_log makes it easier to match entries from the error_log and httpd_log (note that error_log entries already contain the thread name). With the thread name included in each log entry in the httpd_log we have more data to match an error to a request than just the timestamp. Even with the request trace feature [1] this is still useful as we can do more analysis before we ask the user to reproduce an error using the request trace. [1] https://gerrit-review.googlesource.com/Documentation/user-request-tracing.html Change-Id: I60f0fde1c2708bdc56e6ab458d66abee1b9c59aa --- java/com/google/gerrit/pgm/http/jetty/HttpLog.java | 2 +- java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/pgm/http/jetty/HttpLog.java b/java/com/google/gerrit/pgm/http/jetty/HttpLog.java index b7ec2be684..a1d1d73ce6 100644 --- a/java/com/google/gerrit/pgm/http/jetty/HttpLog.java +++ b/java/com/google/gerrit/pgm/http/jetty/HttpLog.java @@ -72,7 +72,7 @@ class HttpLog extends AbstractLifeCycle implements RequestLog { TimeUtil.nowMs(), // when Level.INFO, // level "", // message text - "HTTPD", // thread name + Thread.currentThread().getName(), // thread name null, // exception information null, // current NDC string null, // caller location diff --git a/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java b/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java index 2eea88d2fd..9f240ac7c9 100644 --- a/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java +++ b/java/com/google/gerrit/pgm/http/jetty/HttpLogLayout.java @@ -40,6 +40,11 @@ public final class HttpLogLayout extends Layout { opt(buf, event, HttpLog.P_HOST); + buf.append(' '); + buf.append('['); + buf.append(event.getThreadName()); + buf.append(']'); + buf.append(' '); buf.append('-'); // identd on client system (never requested) -- cgit v1.2.3 From af77638fb85255734342ec243e6d883335cab5a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Thu, 12 Dec 2019 17:18:39 +0100 Subject: Log also thread name in the sshd_log Change-Id: I0b583cd4adf9afb16e09d903e8641ed885536489 --- java/com/google/gerrit/sshd/SshLog.java | 2 +- java/com/google/gerrit/sshd/SshLogLayout.java | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/java/com/google/gerrit/sshd/SshLog.java b/java/com/google/gerrit/sshd/SshLog.java index 5fb75c8d21..6ea03c6e59 100644 --- a/java/com/google/gerrit/sshd/SshLog.java +++ b/java/com/google/gerrit/sshd/SshLog.java @@ -250,7 +250,7 @@ class SshLog implements LifecycleListener, GerritConfigListener { TimeUtil.nowMs(), // when Level.INFO, // level msg, // message text - "SSHD", // thread name + Thread.currentThread().getName(), // thread name null, // exception information null, // current NDC string null, // caller location diff --git a/java/com/google/gerrit/sshd/SshLogLayout.java b/java/com/google/gerrit/sshd/SshLogLayout.java index 442ec52cb2..fd7fef18e4 100644 --- a/java/com/google/gerrit/sshd/SshLogLayout.java +++ b/java/com/google/gerrit/sshd/SshLogLayout.java @@ -54,6 +54,12 @@ public final class SshLogLayout extends Layout { buf.append(']'); req(P_SESSION, buf, event); + + buf.append(' '); + buf.append('['); + buf.append(event.getThreadName()); + buf.append(']'); + req(P_USER_NAME, buf, event); req(P_ACCOUNT_ID, buf, event); -- cgit v1.2.3 From b2d6e5f77f951eb0b692233777d5af2a225bc18e Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Thu, 12 Dec 2019 13:41:33 +0000 Subject: Revert "Fix handling of interactive/batch users in the QoS filter" It has broken the Git/HTTP protocol. This reverts commit 850b1c312ce8c283b2bfd503d7a142f7ca599e84. Bug: Issue 12070 Change-Id: Ia61cd0eb713e6de758a1534799bc992eea3ec2f6 --- java/com/google/gerrit/pgm/Daemon.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index 591297335a..b08842ed57 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -579,14 +579,14 @@ public class Daemon extends SiteProgram { private Injector createWebInjector() { final List modules = new ArrayList<>(); + if (sshd) { + modules.add(new ProjectQoSFilter.Module()); + } modules.add(RequestContextFilter.module()); modules.add(RequestMetricsFilter.module()); modules.add(H2CacheBasedWebSession.module()); modules.add(sysInjector.getInstance(GerritAuthModule.class)); modules.add(sysInjector.getInstance(GitOverHttpModule.class)); - if (sshd) { - modules.add(new ProjectQoSFilter.Module()); - } modules.add(AllRequestFilter.module()); modules.add(sysInjector.getInstance(WebModule.class)); modules.add(sysInjector.getInstance(RequireSslFilter.Module.class)); -- cgit v1.2.3 From 609c50862383f8cd4a99520884cb37addaa7b9f5 Mon Sep 17 00:00:00 2001 From: Thomas Draebing Date: Thu, 12 Dec 2019 11:00:54 +0100 Subject: Link global eslint packages to local project With the update to eslint 6.0.0 eslint does not support a global installation anymore. Since a local installation is not feasible on the gerrit-ci and will also cause issues with bazel, since the packages will not be available locally in the sandbox, the lint_test.sh script now links a global eslint (and eslint-plugins) installation to the local project. This allows to use eslint 6.0.0 without having to reinstall it in every instance of the project, which would be the case for every build on the CI. Change-Id: I6c41a0450a6d23860356f0e2615a45c46e9d8a89 --- polygerrit-ui/app/lint_test.sh | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/polygerrit-ui/app/lint_test.sh b/polygerrit-ui/app/lint_test.sh index 35939ba174..f9459c340b 100755 --- a/polygerrit-ui/app/lint_test.sh +++ b/polygerrit-ui/app/lint_test.sh @@ -19,4 +19,8 @@ if [ -z "$eslint_bin" ] || [ "$eslint_config" -eq "0" ] || [ "$eslint_plugin" -e exit 1 fi -${eslint_bin} --ignore-pattern 'bower_components/' --ignore-pattern 'gr-linked-text' --ignore-pattern 'scripts/vendor' --ext .html,.js . +# Linking global eslint packages to the local project. Required to use eslint plugins with a global +# eslint installation. +npm link eslint eslint-config-google eslint-plugin-html + +${eslint_bin} --ignore-pattern 'node_modules/' --ignore-pattern 'bower_components/' --ignore-pattern 'gr-linked-text' --ignore-pattern 'scripts/vendor' --ext .html,.js . -- cgit v1.2.3 From bef0560e81a0d93cb7f67779c7c47257cc330992 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 13 Dec 2019 12:20:44 +0100 Subject: Add example command for how to find SHA1 of an external ID Change-Id: Iab96ab10c65cd3b1935cd5ff6f4fa671b89b3095 (cherry picked from commit 7bc254269476269ec020cb10d72829656352ee94) --- Documentation/config-accounts.txt | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/Documentation/config-accounts.txt b/Documentation/config-accounts.txt index e64242535d..7f3cd99251 100644 --- a/Documentation/config-accounts.txt +++ b/Documentation/config-accounts.txt @@ -298,6 +298,16 @@ for the external ID `username:jdoe` is `e0b751ae90ef039f320e097d7d212f490e933706 This ensures that an external ID is used only once (e.g. an external ID can never be assigned to multiple accounts at a point in time). +The following commands show how to find the SHA1 of an external ID: + +---- +$ echo -n 'gerrit:jdoe' | shasum +7c2a55657d911109dbc930836e7a770fb946e8ef - + +$ echo -n 'username:jdoe' | shasum +e0b751ae90ef039f320e097d7d212f490e933706 - +---- + [IMPORTANT] If the external ID key is changed manually you must adapt the note key to the new SHA1, otherwise the external ID becomes inconsistent and is -- cgit v1.2.3 From 5c7e90ef4de985ed6f3a14ed43619bd8c9988131 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sa=C5=A1a=20=C5=BDivkov?= Date: Fri, 13 Dec 2019 12:27:42 +0100 Subject: Add example command for how to display note of an external ID Change-Id: I78564b353aacf557ae22cc3ee8b61dfae1fc4eba (cherry picked from commit 3c6b3c8ab9dc6c3d29534cd935fb9d6bf98d8462) --- Documentation/config-accounts.txt | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/Documentation/config-accounts.txt b/Documentation/config-accounts.txt index 7f3cd99251..addada11fc 100644 --- a/Documentation/config-accounts.txt +++ b/Documentation/config-accounts.txt @@ -322,6 +322,20 @@ The note content is a Git config file: password = bcrypt:4:LCbmSBDivK/hhGVQMfkDpA==:XcWn0pKYSVU/UJgOvhidkEtmqCp6oKB7 ---- +Once SHA1 of an external ID is known the following command can be used to +show the content of the note: + +---- +$ echo -n 'gerrit:jdoe' | shasum +7c2a55657d911109dbc930836e7a770fb946e8ef - + +$ git show refs/meta/external-ids:7c/2a55657d911109dbc930836e7a770fb946e8ef +[externalId "username:jdoe"] + accountId = 1003407 + email = jdoe@example.com + password = bcrypt:4:LCbmSBDivK/hhGVQMfkDpA==:XcWn0pKYSVU/UJgOvhidkEtmqCp6oKB7 +---- + The config file has one `externalId` section. The external ID key, which consists of scheme and ID in the format ':', is used as subsection name. -- cgit v1.2.3 From 13a83f1804a37dad632ac1855e7a4a6a69bcd076 Mon Sep 17 00:00:00 2001 From: Tao Zhou Date: Fri, 13 Dec 2019 12:44:52 +0100 Subject: Fix linter errors under eslint 6.x Also changed the lint_test to only run within ui folder to speed it up. Change-Id: Ib65ae53176d7a6eece12bd85bcae3bb402d5ade8 --- polygerrit-ui/app/.eslintrc.json | 11 ++++++++++- polygerrit-ui/app/lint_test.sh | 19 +++++++++++++------ polygerrit-ui/app/template_test.sh | 8 ++++---- 3 files changed, 27 insertions(+), 11 deletions(-) diff --git a/polygerrit-ui/app/.eslintrc.json b/polygerrit-ui/app/.eslintrc.json index b5d3daed5a..0bd8bf7021 100644 --- a/polygerrit-ui/app/.eslintrc.json +++ b/polygerrit-ui/app/.eslintrc.json @@ -25,7 +25,13 @@ "block-spacing": ["error", "always"], "brace-style": ["error", "1tbs", { "allowSingleLine": true }], "camelcase": "off", - "comma-dangle": ["error", "always-multiline"], + "comma-dangle": ["error", { + "arrays": "always-multiline", + "objects": "always-multiline", + "imports": "always-multiline", + "exports": "always-multiline", + "functions": "never" + }], "eol-last": "off", "indent": "off", "indent-legacy": ["error", 2, { @@ -60,6 +66,9 @@ "no-undef": "off", "no-useless-escape": "off", "no-var": "error", + "no-prototype-builtins": "off", + "no-redeclare": "off", + "operator-linebreak": "off", "object-shorthand": ["error", "always"], "prefer-arrow-callback": "error", "prefer-const": "error", diff --git a/polygerrit-ui/app/lint_test.sh b/polygerrit-ui/app/lint_test.sh index f9459c340b..fce5fdd525 100755 --- a/polygerrit-ui/app/lint_test.sh +++ b/polygerrit-ui/app/lint_test.sh @@ -2,15 +2,15 @@ set -ex -eslint_bin=$(which npm) -if [ -z "$eslint_bin" ]; then +npm_bin=$(which npm) && true +if [ -z "$npm_bin" ]; then echo "NPM must be on the path." exit 1 fi -eslint_bin=$(which eslint) -eslint_config=$(npm list -g | grep -c eslint-config-google) -eslint_plugin=$(npm list -g | grep -c eslint-plugin-html) +eslint_bin=$(which eslint) && true +eslint_config=$(npm list -g | grep -c eslint-config-google) && true +eslint_plugin=$(npm list -g | grep -c eslint-plugin-html) && true if [ -z "$eslint_bin" ] || [ "$eslint_config" -eq "0" ] || [ "$eslint_plugin" -eq "0" ]; then echo "You must install ESLint and its dependencies from NPM." echo "> npm install -g eslint eslint-config-google eslint-plugin-html" @@ -19,8 +19,15 @@ if [ -z "$eslint_bin" ] || [ "$eslint_config" -eq "0" ] || [ "$eslint_plugin" -e exit 1 fi +# get absolute path to lint_test.sh path +SCRIPT=$(readlink -f "$0") +UI_PATH=$(dirname "$SCRIPT") + +# To make sure npm link happens in the right place +cd ${UI_PATH} + # Linking global eslint packages to the local project. Required to use eslint plugins with a global # eslint installation. npm link eslint eslint-config-google eslint-plugin-html -${eslint_bin} --ignore-pattern 'node_modules/' --ignore-pattern 'bower_components/' --ignore-pattern 'gr-linked-text' --ignore-pattern 'scripts/vendor' --ext .html,.js . +${eslint_bin} -c ${UI_PATH}/.eslintrc.json --ignore-pattern 'node_modules/' --ignore-pattern 'bower_components/' --ignore-pattern 'gr-linked-text' --ignore-pattern 'scripts/vendor' --ext .html,.js ${UI_PATH} diff --git a/polygerrit-ui/app/template_test.sh b/polygerrit-ui/app/template_test.sh index 7177e8afbb..eb986c5923 100755 --- a/polygerrit-ui/app/template_test.sh +++ b/polygerrit-ui/app/template_test.sh @@ -2,26 +2,26 @@ set -ex -node_bin=$(which node) +node_bin=$(which node) && true if [ -z "$node_bin" ]; then echo "node must be on the path." exit 1 fi -npm_bin=$(which npm) +npm_bin=$(which npm) && true if [[ -z "$npm_bin" ]]; then echo "NPM must be on the path. (https://www.npmjs.com/)" exit 1 fi -fried_twinkie_config=$(npm list -g | grep -c fried-twinkie) +fried_twinkie_config=$(npm list -g | grep -c fried-twinkie) && true if [ -z "$npm_bin" ] || [ "$fried_twinkie_config" -eq "0" ]; then echo "You must install fried twinkie and its dependencies from NPM." echo "> npm install -g fried-twinkie" exit 1 fi -twinkie_version=$(npm list -g fried-twinkie@\>0.1 | grep fried-twinkie || :) +twinkie_version=$(npm list -g fried-twinkie@\>0.1 | grep fried-twinkie || :) && true if [ -z "$twinkie_version" ]; then echo "Outdated version of fried-twinkie found. Bypassing template check." exit 0 -- cgit v1.2.3 From d2b12082143926e4feb362c8723602d6ef39e2c3 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Dec 2019 21:51:03 +0000 Subject: Test Git/HTTP Servlet when SSH is enabled The way Git/HTTP threads are managed is different when the SSH daemon is enabled. Make sure that all the Git/HTTP operations are working as expected by enabling also SSHD. In order to reuse the GitOverHttpServletIT logic, introduce a new abstract class and add it to the common library used by all git tests in Bazel BUILD file. Change-Id: I879881b6db5f4295c1c889a764e0c31d369463f7 --- .../acceptance/git/AbstractGitOverHttpServlet.java | 68 ++++++++++++++++++++++ javatests/com/google/gerrit/acceptance/git/BUILD | 2 +- .../acceptance/git/GitOverHttpServletIT.java | 53 +---------------- .../git/GitOverHttpServletWithSshdEnabledIT.java | 20 +++++++ 4 files changed, 90 insertions(+), 53 deletions(-) create mode 100644 javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java create mode 100644 javatests/com/google/gerrit/acceptance/git/GitOverHttpServletWithSshdEnabledIT.java diff --git a/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java b/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java new file mode 100644 index 0000000000..2f54f88508 --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/git/AbstractGitOverHttpServlet.java @@ -0,0 +1,68 @@ +// Copyright (C) 2019 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.git; + +import static com.google.common.truth.Truth.assertThat; + +import com.google.gerrit.server.AuditEvent; +import java.util.Collections; +import org.eclipse.jgit.transport.CredentialsProvider; +import org.eclipse.jgit.transport.RefSpec; +import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; +import org.junit.Before; +import org.junit.Test; + +public class AbstractGitOverHttpServlet extends AbstractPushForReview { + + @Before + public void beforeEach() throws Exception { + CredentialsProvider.setDefault( + new UsernamePasswordCredentialsProvider(admin.username, admin.httpPassword)); + selectProtocol(AbstractPushForReview.Protocol.HTTP); + auditService.clearEvents(); + } + + @Test + public void receivePackAuditEventLog() throws Exception { + testRepo + .git() + .push() + .setRemote("origin") + .setRefSpecs(new RefSpec("HEAD:refs/for/master")) + .call(); + + // Git smart protocol makes two requests: + // https://github.com/git/git/blob/master/Documentation/technical/http-protocol.txt + assertThat(auditService.auditEvents.size()).isEqualTo(2); + + AuditEvent e = auditService.auditEvents.get(1); + assertThat(e.who.getAccountId()).isEqualTo(admin.id); + assertThat(e.what).endsWith("/git-receive-pack"); + assertThat(e.params).isEmpty(); + } + + @Test + public void uploadPackAuditEventLog() throws Exception { + testRepo.git().fetch().call(); + + assertThat(auditService.auditEvents.size()).isEqualTo(1); + + AuditEvent e = auditService.auditEvents.get(0); + assertThat(e.who.toString()).isEqualTo("ANONYMOUS"); + assertThat(e.params.get("service")) + .containsExactlyElementsIn(Collections.singletonList("git-upload-pack")); + assertThat(e.what).endsWith("service=git-upload-pack"); + } +} diff --git a/javatests/com/google/gerrit/acceptance/git/BUILD b/javatests/com/google/gerrit/acceptance/git/BUILD index 5c35cc7904..1ecfd43a91 100644 --- a/javatests/com/google/gerrit/acceptance/git/BUILD +++ b/javatests/com/google/gerrit/acceptance/git/BUILD @@ -14,7 +14,7 @@ load("//javatests/com/google/gerrit/acceptance:tests.bzl", "acceptance_tests") java_library( name = "push_for_review", testonly = True, - srcs = ["AbstractPushForReview.java"], + srcs = glob(["Abstract*.java"]), deps = [ "//java/com/google/gerrit/acceptance:lib", "//java/com/google/gerrit/mail", diff --git a/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletIT.java b/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletIT.java index 42e046ad02..e9f9b82954 100644 --- a/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletIT.java +++ b/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletIT.java @@ -14,55 +14,4 @@ package com.google.gerrit.acceptance.git; -import static com.google.common.truth.Truth.assertThat; - -import com.google.gerrit.server.AuditEvent; -import java.util.Collections; -import org.eclipse.jgit.transport.CredentialsProvider; -import org.eclipse.jgit.transport.RefSpec; -import org.eclipse.jgit.transport.UsernamePasswordCredentialsProvider; -import org.junit.Before; -import org.junit.Test; - -public class GitOverHttpServletIT extends AbstractPushForReview { - - @Before - public void beforeEach() throws Exception { - CredentialsProvider.setDefault( - new UsernamePasswordCredentialsProvider(admin.username, admin.httpPassword)); - selectProtocol(AbstractPushForReview.Protocol.HTTP); - auditService.clearEvents(); - } - - @Test - public void receivePackAuditEventLog() throws Exception { - testRepo - .git() - .push() - .setRemote("origin") - .setRefSpecs(new RefSpec("HEAD:refs/for/master")) - .call(); - - // Git smart protocol makes two requests: - // https://github.com/git/git/blob/master/Documentation/technical/http-protocol.txt - assertThat(auditService.auditEvents.size()).isEqualTo(2); - - AuditEvent e = auditService.auditEvents.get(1); - assertThat(e.who.getAccountId()).isEqualTo(admin.id); - assertThat(e.what).endsWith("/git-receive-pack"); - assertThat(e.params).isEmpty(); - } - - @Test - public void uploadPackAuditEventLog() throws Exception { - testRepo.git().fetch().call(); - - assertThat(auditService.auditEvents.size()).isEqualTo(1); - - AuditEvent e = auditService.auditEvents.get(0); - assertThat(e.who.toString()).isEqualTo("ANONYMOUS"); - assertThat(e.params.get("service")) - .containsExactlyElementsIn(Collections.singletonList("git-upload-pack")); - assertThat(e.what).endsWith("service=git-upload-pack"); - } -} +public class GitOverHttpServletIT extends AbstractGitOverHttpServlet {} diff --git a/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletWithSshdEnabledIT.java b/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletWithSshdEnabledIT.java new file mode 100644 index 0000000000..9b9372f8ce --- /dev/null +++ b/javatests/com/google/gerrit/acceptance/git/GitOverHttpServletWithSshdEnabledIT.java @@ -0,0 +1,20 @@ +// Copyright (C) 2019 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.acceptance.git; + +import com.google.gerrit.acceptance.UseSsh; + +@UseSsh +public class GitOverHttpServletWithSshdEnabledIT extends AbstractGitOverHttpServlet {} -- cgit v1.2.3 From a89aefab270f751b25551a3aa2678b8b570901d7 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Dec 2019 20:21:23 +0000 Subject: Assert RequestCleanup ran only once Prevent the RequestCleanup.run() to be called more than once. Helps preventing bugs in the request context cleanup process and allow making changes to the Git/HTTP and Git/SSH protocols validating the correct request context management. Change-Id: I432d36f591ee9015856ac18ad27e98dcdca8e465 --- java/com/google/gerrit/server/RequestCleanup.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/server/RequestCleanup.java b/java/com/google/gerrit/server/RequestCleanup.java index 7ed9287c80..f405c57147 100644 --- a/java/com/google/gerrit/server/RequestCleanup.java +++ b/java/com/google/gerrit/server/RequestCleanup.java @@ -31,9 +31,7 @@ public class RequestCleanup implements Runnable { /** Register a task to be completed after the request ends. */ public void add(Runnable task) { synchronized (cleanup) { - if (ran) { - throw new IllegalStateException("Request has already been cleaned up"); - } + assertNotRan(); cleanup.add(task); } } @@ -41,6 +39,7 @@ public class RequestCleanup implements Runnable { @Override public void run() { synchronized (cleanup) { + assertNotRan(); ran = true; for (Iterator i = cleanup.iterator(); i.hasNext(); ) { try { @@ -52,4 +51,10 @@ public class RequestCleanup implements Runnable { } } } + + private void assertNotRan() { + if (ran) { + throw new IllegalStateException("Request has already been cleaned up"); + } + } } -- cgit v1.2.3 From faa6c83ae913cd520143701d16b9042a0e10937a Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Dec 2019 13:01:46 +0000 Subject: Separate request context filter from cleanup Preparation work to allow the allocation of the HTTP request context for use in the ProjectQoSFilter. Having separate filters for setting the current context and cleaning it up, allows other filters, like ProjectQoSFilter, to access the current user without having necessarily to have its context cleaned up. The first attempt to have ProjectQoSFilter with a context has failed because the cleanup was done on the upstream filter, causing a double invocation and thus an exception. By having the context set *before* the ProjectQoSFilter and then cleaned up *after* the filter, allows to have the current user injected and still doing the cleanup only once the filter has correctly finished its processing. Bug: Issue 12070 Change-Id: I7fbda3d787f54ef6af6d6ae75c28d4d73ba197fb --- .../google/gerrit/httpd/RequestCleanupFilter.java | 65 ++++++++++++++++++++++ .../google/gerrit/httpd/RequestContextFilter.java | 14 +---- .../gerrit/httpd/init/WebAppInitializer.java | 2 + java/com/google/gerrit/pgm/Daemon.java | 2 + 4 files changed, 72 insertions(+), 11 deletions(-) create mode 100644 java/com/google/gerrit/httpd/RequestCleanupFilter.java diff --git a/java/com/google/gerrit/httpd/RequestCleanupFilter.java b/java/com/google/gerrit/httpd/RequestCleanupFilter.java new file mode 100644 index 0000000000..30c795e185 --- /dev/null +++ b/java/com/google/gerrit/httpd/RequestCleanupFilter.java @@ -0,0 +1,65 @@ +// Copyright (C) 2019 The Android Open Source Project +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package com.google.gerrit.httpd; + +import com.google.gerrit.server.RequestCleanup; +import com.google.inject.Inject; +import com.google.inject.Module; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import com.google.inject.servlet.ServletModule; +import java.io.IOException; +import javax.servlet.Filter; +import javax.servlet.FilterChain; +import javax.servlet.FilterConfig; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; + +/** Executes any pending {@link RequestCleanup} at the end of a request. */ +@Singleton +public class RequestCleanupFilter implements Filter { + public static Module module() { + return new ServletModule() { + @Override + protected void configureServlets() { + filter("/*").through(RequestCleanupFilter.class); + } + }; + } + + private final Provider cleanup; + + @Inject + RequestCleanupFilter(Provider r) { + cleanup = r; + } + + @Override + public void init(FilterConfig filterConfig) {} + + @Override + public void destroy() {} + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + try { + chain.doFilter(request, response); + } finally { + cleanup.get().run(); + } + } +} diff --git a/java/com/google/gerrit/httpd/RequestContextFilter.java b/java/com/google/gerrit/httpd/RequestContextFilter.java index 6e027969fa..effbac0ee0 100644 --- a/java/com/google/gerrit/httpd/RequestContextFilter.java +++ b/java/com/google/gerrit/httpd/RequestContextFilter.java @@ -14,7 +14,6 @@ package com.google.gerrit.httpd; -import com.google.gerrit.server.RequestCleanup; import com.google.gerrit.server.util.RequestContext; import com.google.gerrit.server.util.ThreadLocalRequestContext; import com.google.inject.Inject; @@ -30,7 +29,7 @@ import javax.servlet.ServletException; import javax.servlet.ServletRequest; import javax.servlet.ServletResponse; -/** Executes any pending {@link RequestCleanup} at the end of a request. */ +/** Set the request context for the downstream filters and invocation. */ @Singleton public class RequestContextFilter implements Filter { public static Module module() { @@ -42,14 +41,11 @@ public class RequestContextFilter implements Filter { }; } - private final Provider cleanup; private final Provider requestContext; private final ThreadLocalRequestContext local; @Inject - RequestContextFilter( - Provider r, Provider c, ThreadLocalRequestContext l) { - cleanup = r; + RequestContextFilter(Provider c, ThreadLocalRequestContext l) { requestContext = c; local = l; } @@ -65,11 +61,7 @@ public class RequestContextFilter implements Filter { throws IOException, ServletException { RequestContext old = local.setContext(requestContext.get()); try { - try { - chain.doFilter(request, response); - } finally { - cleanup.get().run(); - } + chain.doFilter(request, response); } finally { local.setContext(old); } diff --git a/java/com/google/gerrit/httpd/init/WebAppInitializer.java b/java/com/google/gerrit/httpd/init/WebAppInitializer.java index aadfeba4c5..c9cf2f4e85 100644 --- a/java/com/google/gerrit/httpd/init/WebAppInitializer.java +++ b/java/com/google/gerrit/httpd/init/WebAppInitializer.java @@ -28,6 +28,7 @@ import com.google.gerrit.httpd.GetUserFilter; import com.google.gerrit.httpd.GitOverHttpModule; import com.google.gerrit.httpd.H2CacheBasedWebSession; import com.google.gerrit.httpd.HttpCanonicalWebUrlProvider; +import com.google.gerrit.httpd.RequestCleanupFilter; import com.google.gerrit.httpd.RequestContextFilter; import com.google.gerrit.httpd.RequestMetricsFilter; import com.google.gerrit.httpd.RequireSslFilter; @@ -416,6 +417,7 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi modules.add(RequestMetricsFilter.module()); modules.add(sysInjector.getInstance(GerritAuthModule.class)); modules.add(sysInjector.getInstance(GitOverHttpModule.class)); + modules.add(RequestCleanupFilter.module()); modules.add(AllRequestFilter.module()); modules.add(sysInjector.getInstance(WebModule.class)); modules.add(sysInjector.getInstance(RequireSslFilter.Module.class)); diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index b08842ed57..8be8456c8d 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -33,6 +33,7 @@ import com.google.gerrit.httpd.GetUserFilter; import com.google.gerrit.httpd.GitOverHttpModule; import com.google.gerrit.httpd.H2CacheBasedWebSession; import com.google.gerrit.httpd.HttpCanonicalWebUrlProvider; +import com.google.gerrit.httpd.RequestCleanupFilter; import com.google.gerrit.httpd.RequestContextFilter; import com.google.gerrit.httpd.RequestMetricsFilter; import com.google.gerrit.httpd.RequireSslFilter; @@ -587,6 +588,7 @@ public class Daemon extends SiteProgram { modules.add(H2CacheBasedWebSession.module()); modules.add(sysInjector.getInstance(GerritAuthModule.class)); modules.add(sysInjector.getInstance(GitOverHttpModule.class)); + modules.add(RequestCleanupFilter.module()); modules.add(AllRequestFilter.module()); modules.add(sysInjector.getInstance(WebModule.class)); modules.add(sysInjector.getInstance(RequireSslFilter.Module.class)); -- cgit v1.2.3 From 7b2190b6b2c47712d8c3c72ac1e369ac2c52bf37 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Dec 2019 13:07:36 +0000 Subject: Revert "Revert "Fix handling of interactive/batch users in the QoS filter"" This reverts commit b2d6e5f77f951eb0b692233777d5af2a225bc18e because two major problems have been resolved by now: 1. The lack of Git/HTTP with SSHD test coverage has been resolved and there is one more assertion against double run of request cleanups. 2. The request context and cleanup have been decoupled, allowing the QoS filter to access the current user without causing a double cleanup of the request context. Change-Id: I2df49f7a21369a37ab21f75909da4f2871185ba8 --- java/com/google/gerrit/pgm/Daemon.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/java/com/google/gerrit/pgm/Daemon.java b/java/com/google/gerrit/pgm/Daemon.java index 8be8456c8d..8fb208b557 100644 --- a/java/com/google/gerrit/pgm/Daemon.java +++ b/java/com/google/gerrit/pgm/Daemon.java @@ -580,14 +580,14 @@ public class Daemon extends SiteProgram { private Injector createWebInjector() { final List modules = new ArrayList<>(); - if (sshd) { - modules.add(new ProjectQoSFilter.Module()); - } modules.add(RequestContextFilter.module()); modules.add(RequestMetricsFilter.module()); modules.add(H2CacheBasedWebSession.module()); modules.add(sysInjector.getInstance(GerritAuthModule.class)); modules.add(sysInjector.getInstance(GitOverHttpModule.class)); + if (sshd) { + modules.add(new ProjectQoSFilter.Module()); + } modules.add(RequestCleanupFilter.module()); modules.add(AllRequestFilter.module()); modules.add(sysInjector.getInstance(WebModule.class)); -- cgit v1.2.3 From cdb61900d53d2ec96cf5096b3a9ff69d51bac404 Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Dec 2019 23:39:03 +0000 Subject: Set version to 2.16.15 Change-Id: I294b95860ebae4e313c460adaeee68b6aa891f0c --- tools/maven/gerrit-acceptance-framework_pom.xml | 2 +- tools/maven/gerrit-extension-api_pom.xml | 2 +- tools/maven/gerrit-plugin-api_pom.xml | 2 +- tools/maven/gerrit-plugin-gwtui_pom.xml | 2 +- tools/maven/gerrit-war_pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index c2c6f1e68d..17a36b7a51 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.16.15-SNAPSHOT + 2.16.15 jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index c357aeadd5..5cf63f7e59 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.16.15-SNAPSHOT + 2.16.15 jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index 8b3da79516..fd02919d84 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.16.15-SNAPSHOT + 2.16.15 jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml index 3fac3b4730..7339240453 100644 --- a/tools/maven/gerrit-plugin-gwtui_pom.xml +++ b/tools/maven/gerrit-plugin-gwtui_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.16.15-SNAPSHOT + 2.16.15 jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index 730511a8c7..e2a1ee72c2 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.16.15-SNAPSHOT + 2.16.15 war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index e074c11e64..a66961ea72 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.16.15-SNAPSHOT" +GERRIT_VERSION = "2.16.15" -- cgit v1.2.3 From a0730b1cfa658483e14041be3d55a694c6ef68ba Mon Sep 17 00:00:00 2001 From: Luca Milanesio Date: Fri, 13 Dec 2019 23:56:15 +0000 Subject: Set version to 2.16.16-SNAPSHOT Change-Id: I385b48a9ceb9ba2ef9f7e6a8c9e215f6a6665c50 --- tools/maven/gerrit-acceptance-framework_pom.xml | 2 +- tools/maven/gerrit-extension-api_pom.xml | 2 +- tools/maven/gerrit-plugin-api_pom.xml | 2 +- tools/maven/gerrit-plugin-gwtui_pom.xml | 2 +- tools/maven/gerrit-war_pom.xml | 2 +- version.bzl | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/tools/maven/gerrit-acceptance-framework_pom.xml b/tools/maven/gerrit-acceptance-framework_pom.xml index 17a36b7a51..d0607f13af 100644 --- a/tools/maven/gerrit-acceptance-framework_pom.xml +++ b/tools/maven/gerrit-acceptance-framework_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-acceptance-framework - 2.16.15 + 2.16.16-SNAPSHOT jar Gerrit Code Review - Acceptance Test Framework Framework for Gerrit's acceptance tests diff --git a/tools/maven/gerrit-extension-api_pom.xml b/tools/maven/gerrit-extension-api_pom.xml index 5cf63f7e59..470aec26d8 100644 --- a/tools/maven/gerrit-extension-api_pom.xml +++ b/tools/maven/gerrit-extension-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-extension-api - 2.16.15 + 2.16.16-SNAPSHOT jar Gerrit Code Review - Extension API API for Gerrit Extensions diff --git a/tools/maven/gerrit-plugin-api_pom.xml b/tools/maven/gerrit-plugin-api_pom.xml index fd02919d84..bf898655d7 100644 --- a/tools/maven/gerrit-plugin-api_pom.xml +++ b/tools/maven/gerrit-plugin-api_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-api - 2.16.15 + 2.16.16-SNAPSHOT jar Gerrit Code Review - Plugin API API for Gerrit Plugins diff --git a/tools/maven/gerrit-plugin-gwtui_pom.xml b/tools/maven/gerrit-plugin-gwtui_pom.xml index 7339240453..498ab82b7b 100644 --- a/tools/maven/gerrit-plugin-gwtui_pom.xml +++ b/tools/maven/gerrit-plugin-gwtui_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-plugin-gwtui - 2.16.15 + 2.16.16-SNAPSHOT jar Gerrit Code Review - Plugin GWT UI Common Classes for Gerrit GWT UI Plugins diff --git a/tools/maven/gerrit-war_pom.xml b/tools/maven/gerrit-war_pom.xml index e2a1ee72c2..f991f6ed0f 100644 --- a/tools/maven/gerrit-war_pom.xml +++ b/tools/maven/gerrit-war_pom.xml @@ -2,7 +2,7 @@ 4.0.0 com.google.gerrit gerrit-war - 2.16.15 + 2.16.16-SNAPSHOT war Gerrit Code Review - WAR Gerrit WAR diff --git a/version.bzl b/version.bzl index a66961ea72..3a11a12da0 100644 --- a/version.bzl +++ b/version.bzl @@ -2,4 +2,4 @@ # Used by :api_install and :api_deploy targets # when talking to the destination repository. # -GERRIT_VERSION = "2.16.15" +GERRIT_VERSION = "2.16.16-SNAPSHOT" -- cgit v1.2.3