opendev/gerrit
Code Issues Proposed changes

Merge branch 'stable-2.15'

Browse Source 
* stable-2.15:
  GitOverHttpModule: Bind REST auth filter in its own module
  Ensure user authentication in AllRequestFilter filters
  InitSshd: Use correct flag to set empty passphrase
  SshSession: Specify charset in constructor of Scanner
  Specify charset in constructors of InputStreamReader
  Update JGit dependencies to fix building from source
  PolyGerrit: Correctly detect the ACL for SetReadyForReview endpoint

Change-Id: I0e54a3d5a43e1898b1e4e36193b1dc5e2d69bef1
This commit is contained in:
David Ostrovsky
2018-09-01 07:25:16 +02:00
parent d29fe44c1b bb46f7570a
commit 4dd3dd8b8a
9 changed files with 94 additions and 48 deletions
Download Patch File Download Diff File Expand all files Collapse all files

4
java/com/google/gerrit/acceptance/HttpResponse.java
View File

@@ -14,6 +14,8 @@
package com.google.gerrit.acceptance;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.common.base.Preconditions;
import java.io.IOException;
import java.io.InputStreamReader;
@@ -34,7 +36,7 @@ public class HttpResponse {
public Reader getReader() throws IllegalStateException, IOException {
if (reader == null && response.getEntity() != null) {
reader = new InputStreamReader(response.getEntity().getContent());
reader = new InputStreamReader(response.getEntity().getContent(), UTF_8);
}
return reader;
}

5
java/com/google/gerrit/acceptance/SshSession.java
View File

@@ -17,6 +17,7 @@ package com.google.gerrit.acceptance;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.truth.Truth.assertThat;
import static com.google.common.truth.Truth.assertWithMessage;
import static java.nio.charset.StandardCharsets.UTF_8;
import com.google.gerrit.acceptance.testsuite.account.TestSshKeys;
import com.jcraft.jsch.ChannelExec;
@@ -54,10 +55,10 @@ public class SshSession {
InputStream err = channel.getErrStream();
channel.connect();
Scanner s = new Scanner(err).useDelimiter("\\A");
Scanner s = new Scanner(err, UTF_8.name()).useDelimiter("\\A");
error = s.hasNext() ? s.next() : null;
s = new Scanner(in).useDelimiter("\\A");
s = new Scanner(in, UTF_8.name()).useDelimiter("\\A");
return s.hasNext() ? s.next() : "";
} finally {
channel.disconnect();

55
java/com/google/gerrit/httpd/GerritAuthModule.java Normal file
View File

@@ -0,0 +1,55 @@
// 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.httpd;
import static com.google.gerrit.extensions.api.lfs.LfsDefinitions.LFS_URL_WO_AUTH_REGEX;
import com.google.gerrit.extensions.client.GitBasicAuthPolicy;
import com.google.gerrit.server.config.AuthConfig;
import com.google.inject.Inject;
import com.google.inject.servlet.ServletModule;
import javax.servlet.Filter;
/** Configures filter for authenticating REST requests. */
public class GerritAuthModule extends ServletModule {
private static final String NOT_AUTHORIZED_LFS_URL_REGEX = "^(?:(?!/a/))" + LFS_URL_WO_AUTH_REGEX;
private final AuthConfig authConfig;
@Inject
GerritAuthModule(AuthConfig authConfig) {
this.authConfig = authConfig;
}
@Override
protected void configureServlets() {
Class<? extends Filter> authFilter = retreiveAuthFilterFromConfig(authConfig);
filterRegex(NOT_AUTHORIZED_LFS_URL_REGEX).through(authFilter);
filter("/a/*").through(authFilter);
}
static Class<? extends Filter> retreiveAuthFilterFromConfig(AuthConfig authConfig) {
Class<? extends Filter> authFilter;
if (authConfig.isTrustContainerAuth()) {
authFilter = ContainerAuthFilter.class;
} else {
authFilter =
authConfig.getGitBasicAuthPolicy() == GitBasicAuthPolicy.OAUTH
? ProjectOAuthFilter.class
: ProjectBasicAuthFilter.class;
}
return authFilter;
}
}

32
java/com/google/gerrit/httpd/GitOverHttpModule.java
View File

@@ -14,20 +14,16 @@
package com.google.gerrit.httpd;
import static com.google.gerrit.extensions.api.lfs.LfsDefinitions.LFS_URL_WO_AUTH_REGEX;
import static com.google.gerrit.httpd.GitOverHttpServlet.URL_REGEX;
import com.google.gerrit.extensions.client.GitBasicAuthPolicy;
import com.google.gerrit.reviewdb.client.CoreDownloadSchemes;
import com.google.gerrit.server.config.AuthConfig;
import com.google.gerrit.server.config.DownloadConfig;
import com.google.inject.Inject;
import com.google.inject.servlet.ServletModule;
import javax.servlet.Filter;
/** Configures Git access over HTTP with authentication. */
public class GitOverHttpModule extends ServletModule {
private static final String NOT_AUTHORIZED_LFS_URL_REGEX = "^(?:(?!/a/))" + LFS_URL_WO_AUTH_REGEX;
private final AuthConfig authConfig;
private final DownloadConfig downloadConfig;
@@ -39,28 +35,10 @@ public class GitOverHttpModule extends ServletModule {
@Override
protected void configureServlets() {
Class<? extends Filter> authFilter;
if (authConfig.isTrustContainerAuth()) {
authFilter = ContainerAuthFilter.class;
} else {
authFilter =
authConfig.getGitBasicAuthPolicy() == GitBasicAuthPolicy.OAUTH
? ProjectOAuthFilter.class
: ProjectBasicAuthFilter.class;
if (downloadConfig.getDownloadSchemes().contains(CoreDownloadSchemes.ANON_HTTP)
|| downloadConfig.getDownloadSchemes().contains(CoreDownloadSchemes.HTTP)) {
filterRegex(URL_REGEX).through(GerritAuthModule.retreiveAuthFilterFromConfig(authConfig));
serveRegex(URL_REGEX).with(GitOverHttpServlet.class);
}
if (isHttpEnabled()) {
String git = GitOverHttpServlet.URL_REGEX;
filterRegex(git).through(authFilter);
serveRegex(git).with(GitOverHttpServlet.class);
}
filterRegex(NOT_AUTHORIZED_LFS_URL_REGEX).through(authFilter);
filter("/a/*").through(authFilter);
}
private boolean isHttpEnabled() {
return downloadConfig.getDownloadSchemes().contains(CoreDownloadSchemes.ANON_HTTP)
|| downloadConfig.getDownloadSchemes().contains(CoreDownloadSchemes.HTTP);
}
}

4
java/com/google/gerrit/httpd/init/WebAppInitializer.java
View File

@@ -23,6 +23,7 @@ import com.google.gerrit.elasticsearch.ElasticIndexModule;
import com.google.gerrit.extensions.client.AuthType;
import com.google.gerrit.gpg.GpgModule;
import com.google.gerrit.httpd.AllRequestFilter;
import com.google.gerrit.httpd.GerritAuthModule;
import com.google.gerrit.httpd.GetUserFilter;
import com.google.gerrit.httpd.GitOverHttpModule;
import com.google.gerrit.httpd.H2CacheBasedWebSession;
@@ -422,9 +423,10 @@ public class WebAppInitializer extends GuiceServletContextListener implements Fi
private Injector createWebInjector() {
final List<Module> modules = new ArrayList<>();
modules.add(RequestContextFilter.module());
modules.add(AllRequestFilter.module());
modules.add(RequestMetricsFilter.module());
modules.add(sysInjector.getInstance(GerritAuthModule.class));
modules.add(sysInjector.getInstance(GitOverHttpModule.class));
modules.add(AllRequestFilter.module());
modules.add(sysInjector.getInstance(WebModule.class));
modules.add(sysInjector.getInstance(RequireSslFilter.Module.class));
if (sshInjector != null) {

4
java/com/google/gerrit/pgm/Daemon.java
View File

@@ -26,6 +26,7 @@ import com.google.gerrit.elasticsearch.ElasticIndexModule;
import com.google.gerrit.extensions.client.AuthType;
import com.google.gerrit.gpg.GpgModule;
import com.google.gerrit.httpd.AllRequestFilter;
import com.google.gerrit.httpd.GerritAuthModule;
import com.google.gerrit.httpd.GetUserFilter;
import com.google.gerrit.httpd.GitOverHttpModule;
import com.google.gerrit.httpd.H2CacheBasedWebSession;
@@ -575,10 +576,11 @@ public class Daemon extends SiteProgram {
modules.add(new ProjectQoSFilter.Module());
}
modules.add(RequestContextFilter.module());
modules.add(AllRequestFilter.module());
modules.add(RequestMetricsFilter.module());
modules.add(H2CacheBasedWebSession.module());
modules.add(sysInjector.getInstance(GerritAuthModule.class));
modules.add(sysInjector.getInstance(GitOverHttpModule.class));
modules.add(AllRequestFilter.module());
modules.add(sysInjector.getInstance(WebModule.class));
modules.add(sysInjector.getInstance(RequireSslFilter.Module.class));
modules.add(new HttpPluginModule());

2
javatests/com/google/gerrit/gpg/PushCertificateCheckerTest.java
View File

@@ -184,7 +184,7 @@ public class PushCertificateCheckerTest {
}
String cert = payload + new String(bout.toByteArray(), UTF_8);
Reader reader = new InputStreamReader(new ByteArrayInputStream(cert.getBytes(UTF_8)));
Reader reader = new InputStreamReader(new ByteArrayInputStream(cert.getBytes(UTF_8)), UTF_8);
PushCertificateParser parser = new PushCertificateParser(repo, signedPushConfig);
return parser.parse(reader);
}

8
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view.js
View File

@@ -122,7 +122,7 @@
_changeComments: Object,
_canStartReview: {
type: Boolean,
computed: '_computeCanStartReview(_loggedIn, _change, _account)',
computed: '_computeCanStartReview(_change)',
},
_comments: Object,
/** @type {?} */
@@ -1341,9 +1341,9 @@
});
},
_computeCanStartReview(loggedIn, change, account) {
return !!(loggedIn && change.work_in_progress &&
change.owner._account_id === account._account_id);
_computeCanStartReview(change) {
return !!(change.actions && change.actions.ready &&
change.actions.ready.enabled);
},
_computeReplyDisabled() { return false; },

28
polygerrit-ui/app/elements/change/gr-change-view/gr-change-view_test.html
View File

@@ -1411,18 +1411,24 @@ limitations under the License.
});
test('canStartReview computation', () => {
const account1 = {_account_id: 1};
const account2 = {_account_id: 2};
const change = {
owner: {_account_id: 1},
const change1 = {};
const change2 = {
actions: {
ready: {
enabled: true,
},
},
};
assert.isFalse(element._computeCanStartReview(true, change, account1));
change.work_in_progress = false;
assert.isFalse(element._computeCanStartReview(true, change, account1));
change.work_in_progress = true;
assert.isTrue(element._computeCanStartReview(true, change, account1));
assert.isFalse(element._computeCanStartReview(false, change, account1));
assert.isFalse(element._computeCanStartReview(true, change, account2));
const change3 = {
actions: {
ready: {
label: 'Ready for Review',
},
},
};
assert.isFalse(element._computeCanStartReview(change1));
assert.isTrue(element._computeCanStartReview(change2));
assert.isFalse(element._computeCanStartReview(change3));
});
});